diff --git a/CLAUDE.md b/CLAUDE.md index 344f76ec..ee0610ba 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -228,7 +228,7 @@ Collection names accept singular forms: `task`→`tasks`, `idea`→`ideas`, `doc ## MCP server -Pad runs as a local Model Context Protocol server so Claude Desktop / Cursor / Windsurf can call non-interactive `pad` commands as tools. The tool surface is a **hand-curated catalog** (currently v0.20) in `internal/mcp/catalog_*.go` — one ToolDef per resource (`pad_item`, `pad_workspace`, `pad_collection`, `pad_project`, `pad_role`, `pad_search`, `pad_meta`, `pad_playbook`, `pad_library`, `pad_attachment`) with an `action` enum dispatching to underlying CLI commands. v0.20 (BUG-2302 + BUG-2305, one bump) adds explicit MCP tool annotations (`readOnlyHint`/`destructiveHint`/`idempotentHint` derived from the catalog's own write-shape knowledge, fixing read-only tools that advertised `destructiveHint:true`) and makes `pad_item.list` summary-shaped on the REMOTE /mcp transport too (the hand-written `dispatchItemList` projects via `cli.ToItemSummaries`; `full=true` opts back into complete bodies) — see `internal/mcp/version.go` for the authoritative per-version changelog. Post-0.20 without a bump (BUG-2304): `item backlinks` / `item history` / `project report` gained HTTP route coverage — they were advertised but answered "not yet implemented over HTTP transport" — and a catalog↔route parity test (`dispatch_http_parity_test.go`) now drives every catalog action and fails on any future advertised-but-unrouted action; no names, enums, or shapes changed, hence no bump. v0.19 adds a `clear_parent` boolean to `pad_item` — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` bareword flag on `pad item update` (BUG-2078). v0.18 adds `clear_assigned_user` / `clear_agent_role` booleans to `pad_item` — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` bareword flags on `pad item update` (IDEA-2584). Update-only, deliberately asymmetric with create. v0.17 carries the empty-string clear to the LOCAL STDIO transport, which shells out to the CLI — `cmd/pad/cmd_item.go` now lifts `assigned_user_id` / `agent_role_id` onto their columns instead of into the fields blob, on create and update (BUG-2583). v0.16 makes an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an MCP agent can finally unassign an item (TASK-2571). v0.15 adds the `pad_item.list` `unparented` boolean, mutually exclusive with `parent`, for items with no parent or implements relationship (TASK-2096). v0.2 introduced the catalog (PLAN-969 / TASK-981); v0.3 added `pad_playbook`, `pad_meta.action: bootstrap`, `pad_set_workspace`'s embedded-bootstrap response, and the `pad://workspace/{ws}/bootstrap` resource (PLAN-1377 / TASK-1380); v0.4 trimmed the bootstrap payload by ~40% (PLAN-1410) — slim `BootstrapCollection` + `BootstrapRole` projections (no UUIDs/timestamps/settings; nested `schema` object; redundant labels omitted), removed top-level `recent_activity` duplicate, dropped convention `slug`, and added a `BootstrapDashboard` wrapper that caps five sub-arrays (`attention`, `recent_activity`, `active_items`, `active_plans`, `by_role`) at 5 entries each with parallel `*_overflow_count` fields. The pre-catalog v0.1 cmdhelp leaf walker is retired. +Pad runs as a local Model Context Protocol server so Claude Desktop / Cursor / Windsurf can call non-interactive `pad` commands as tools. The tool surface is a **hand-curated catalog** (currently v0.21) in `internal/mcp/catalog_*.go` — one ToolDef per resource (`pad_item`, `pad_workspace`, `pad_collection`, `pad_project`, `pad_role`, `pad_search`, `pad_meta`, `pad_playbook`, `pad_library`, `pad_attachment`) with an `action` enum dispatching to underlying CLI commands. v0.21 (BUG-2608) bounds `pad_item.action=history`, which was unbounded on every surface: the `limit` param now covers it (default 50, max 300 — the NEWEST N versions, with no `offset`, because reverse-patch storage makes only a newest-end window cheap to reconstruct), applied in the CATALOG action so it lands on both transports, and summary mode now asks the server to skip patch resolution (`?summary=true`) instead of resolving every body and discarding it. Additive param bump — `limit` already existed and nothing changed shape. v0.20 (BUG-2302 + BUG-2305, one bump) adds explicit MCP tool annotations (`readOnlyHint`/`destructiveHint`/`idempotentHint` derived from the catalog's own write-shape knowledge, fixing read-only tools that advertised `destructiveHint:true`) and makes `pad_item.list` summary-shaped on the REMOTE /mcp transport too (the hand-written `dispatchItemList` projects via `cli.ToItemSummaries`; `full=true` opts back into complete bodies) — see `internal/mcp/version.go` for the authoritative per-version changelog. Post-0.20 without a bump (BUG-2304): `item backlinks` / `item history` / `project report` gained HTTP route coverage — they were advertised but answered "not yet implemented over HTTP transport" — and a catalog↔route parity test (`dispatch_http_parity_test.go`) now drives every catalog action and fails on any future advertised-but-unrouted action; no names, enums, or shapes changed, hence no bump. v0.19 adds a `clear_parent` boolean to `pad_item` — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` bareword flag on `pad item update` (BUG-2078). v0.18 adds `clear_assigned_user` / `clear_agent_role` booleans to `pad_item` — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` bareword flags on `pad item update` (IDEA-2584). Update-only, deliberately asymmetric with create. v0.17 carries the empty-string clear to the LOCAL STDIO transport, which shells out to the CLI — `cmd/pad/cmd_item.go` now lifts `assigned_user_id` / `agent_role_id` onto their columns instead of into the fields blob, on create and update (BUG-2583). v0.16 makes an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an MCP agent can finally unassign an item (TASK-2571). v0.15 adds the `pad_item.list` `unparented` boolean, mutually exclusive with `parent`, for items with no parent or implements relationship (TASK-2096). v0.2 introduced the catalog (PLAN-969 / TASK-981); v0.3 added `pad_playbook`, `pad_meta.action: bootstrap`, `pad_set_workspace`'s embedded-bootstrap response, and the `pad://workspace/{ws}/bootstrap` resource (PLAN-1377 / TASK-1380); v0.4 trimmed the bootstrap payload by ~40% (PLAN-1410) — slim `BootstrapCollection` + `BootstrapRole` projections (no UUIDs/timestamps/settings; nested `schema` object; redundant labels omitted), removed top-level `recent_activity` duplicate, dropped convention `slug`, and added a `BootstrapDashboard` wrapper that caps five sub-arrays (`attention`, `recent_activity`, `active_items`, `active_plans`, `by_role`) at 5 entries each with parallel `*_overflow_count` fields. The pre-catalog v0.1 cmdhelp leaf walker is retired. cmdhelp is still consumed at dispatch time — `BuildCLIArgs` reads individual command schemas to translate the catalog's snake_case input map into CLI args. cmdhelp no longer drives tool naming or count. @@ -242,7 +242,7 @@ pad mcp status # Install state across supported clients ``` Surface: -- **Tools:** the v0.19 catalog — ten resource × action tools (`pad_item`, `pad_workspace`, `pad_collection`, `pad_project`, `pad_role`, `pad_search`, `pad_meta`, `pad_playbook`, `pad_library`, `pad_attachment`) plus `pad_set_workspace` (takes a `workspace` slug only — no action enum). The ten resource × action tools take `action: ` to choose what they do. `pad_item` (v0.19) exposes `clear_parent` as the canonical parent-detach (update only); (v0.18) exposes `clear_assigned_user` / `clear_agent_role` booleans as the canonical unassign (update only); (v0.17) treats an empty-string `assigned_user_id` / `agent_role_id` as a clear on BOTH transports via `field: ["assigned_user_id="]` — the direct param form is remote-only, since it isn't schema-declared and stdio's BuildCLIArgs drops unknown keys (IDEA-2584); v0.16 fixed remote only; (v0.15) adds the `unparented` list parameter; v0.14 added `history` + `expected_updated_at`. `pad_project` (v0.13) adds `ready` (actionable backlog) + `stale` (items needing attention); `pad_project.activity` (v0.12) is the non-streaming, bounded activity feed — catch up on what other agents/users changed since you last worked. `pad_attachment` is the read-only attachment-metadata surface — `list`/`show` (upload/download/view stay CLI-only). `pad_library` is the convention+playbook library surface — `list`/`get`/`activate`. `pad_playbook` is the playbook surface from PLAN-1377 — `list`/`get`/`run` mirror the CLI's `pad playbook` subcommands; `run` is side-effect-free and returns the body + bound args for the agent to execute. v0.4 (PLAN-1410) didn't change the tool/action surface; it trimmed the bootstrap JSON those tools/resources return — see the Stability contract subsection below for details. +- **Tools:** the v0.21 catalog — ten resource × action tools (`pad_item`, `pad_workspace`, `pad_collection`, `pad_project`, `pad_role`, `pad_search`, `pad_meta`, `pad_playbook`, `pad_library`, `pad_attachment`) plus `pad_set_workspace` (takes a `workspace` slug only — no action enum). The ten resource × action tools take `action: ` to choose what they do. `pad_item` (v0.19) exposes `clear_parent` as the canonical parent-detach (update only); (v0.18) exposes `clear_assigned_user` / `clear_agent_role` booleans as the canonical unassign (update only); (v0.17) treats an empty-string `assigned_user_id` / `agent_role_id` as a clear on BOTH transports via `field: ["assigned_user_id="]` — the direct param form is remote-only, since it isn't schema-declared and stdio's BuildCLIArgs drops unknown keys (IDEA-2584); v0.16 fixed remote only; (v0.15) adds the `unparented` list parameter; v0.14 added `history` + `expected_updated_at`. `pad_project` (v0.13) adds `ready` (actionable backlog) + `stale` (items needing attention); `pad_project.activity` (v0.12) is the non-streaming, bounded activity feed — catch up on what other agents/users changed since you last worked. `pad_attachment` is the read-only attachment-metadata surface — `list`/`show` (upload/download/view stay CLI-only). `pad_library` is the convention+playbook library surface — `list`/`get`/`activate`. `pad_playbook` is the playbook surface from PLAN-1377 — `list`/`get`/`run` mirror the CLI's `pad playbook` subcommands; `run` is side-effect-free and returns the body + bound args for the agent to execute. v0.4 (PLAN-1410) didn't change the tool/action surface; it trimmed the bootstrap JSON those tools/resources return — see the Stability contract subsection below for details. - **Resources:** `pad://workspace/{ws}/items/{ref}`, `pad://workspace/{ws}/items`, `pad://workspace/{ws}/dashboard`, `pad://workspace/{ws}/collections`, `pad://workspace/{ws}/attachments/{id}` (bounded base64 image via `thumb-md`; non-images and image bytes over 1 MiB (pre-base64) rejected), `pad://workspace/{ws}/bootstrap` (one-shot workspace overview — user + collections + always-on conventions + roles + playbook metadata + dashboard + recent activity), plus the server-wide `pad://_meta/version`. - **Prompts:** `pad_plan`, `pad_ideate`, `pad_retro`, `pad_onboard` — multi-step workflows lifted from `skills/pad/SKILL.md`. @@ -250,10 +250,12 @@ Surface: **Stability contract.** Two version constants live in `internal/mcp/version.go`, advertised in the handshake under `capabilities.experimental.padCmdhelp` and `capabilities.experimental.padToolSurface`: - `CmdhelpVersion` (currently `"0.1"`) — the cmdhelp CLI help-tree contract. Bump when CLI flag/arg schemas change incompatibly. -- `ToolSurfaceVersion` (currently `"0.19"`) — the MCP tool catalog contract. Bump when tool names, action enums, or parameter shapes change incompatibly. **v0.19** adds a `clear_parent` boolean to `pad_item` (BUG-2078) — an ADDITIVE param bump, same grounds as v0.18; nothing existing changed shape. The server has supported clearing a parent since BUG-2013 (`extractParentLink` treats a present-but-empty `parent` key in `fields_patch` as detach), but neither client surface could reach it — `--parent ""` was a silent no-op on the CLI and the MCP `parent` param has the same "empty means not provided" convention every other declared string on the tool has. Boolean rather than overloading the empty string, same two reasons as v0.18: keeps that invariant intact for every other param, and only a boolean reaches LOCAL STDIO via `BuildCLIArgs`, mapping to a new `--clear-parent` bareword flag exactly as `clear_assigned_user` maps to `--clear-assigned-user`. Update-only, same asymmetry as v0.18. A simultaneous `parent` + `clear_parent` — including via `field: ["parent=..."]` or the `plan` alias `extractParentLink` also accepts — is REFUSED on both transports, not silently resolved (codex round 1). Also refused, not silently applied: `clear_parent` against a collection whose schema declares its own `parent`/`plan` field — `extractParentLink` skips hierarchy handling entirely for a schema-shadowed key and lets it fall through as an ordinary field write, so the wire shape `{"parent":""}` can no longer distinguish clear-hierarchy intent from a legitimate blank-a-real-field write once it reaches the server; the ambiguity is created at the client surface that accepted `clear_parent`, so that surface refuses rather than guessing (codex round 2). **v0.18** adds `clear_assigned_user` / `clear_agent_role` booleans to `pad_item` (IDEA-2584) — an ADDITIVE param bump (v0.5/v0.6 precedent); nothing existing changed shape and v0.16/v0.17's empty-string forms still work, undeprecated. v0.16 and v0.17 made the clear WORK; nothing advertised it, because the params that do it were never in the catalog, so an agent reading the schema reached for `assign: ""` (a no-op, and it stays one). Booleans rather than declaring the string params, for two reasons: an empty DECLARED string is inert everywhere else on the tool, so giving one a destructive meaning would let a param-padding client silently unassign everything; and only a boolean can reach LOCAL STDIO, since `BuildCLIArgs` emits the CLI's real flags and a param with no flag behind it is dropped — these map to new `--clear-assigned-user` / `--clear-agent-role` bareword flags, exactly as `allow_draft` maps to `--allow-draft`. Update-only, deliberately asymmetric with create (clearing at create has no honest behaviour but a no-op; a test fails if someone adds them there). Server-side it is wiring, not new semantics: `models.ItemUpdate.ClearAssignedUser`/`ClearAgentRole` already existed with store support since BUG-2566. **v0.17** closes the transport gap v0.16 documented: local stdio MCP shells out to the CLI, which wrote `--field assigned_user_id=` into the item's FIELDS BLOB while the column stayed stale and then printed "Updated TASK-9". `cmd/pad/cmd_item.go` now lifts `columnFieldKeys` onto the columns on create AND update, mirroring `liftFieldsToColumns` and its INVARIANT. Two compat changes, ruled separately: non-empty values move to the column and stop writing the blob key (relying on the old behaviour is relying on a shadowing defect), and empty values clear (falls out of the lift, inherits BUG-2566). Existing stray blob keys are left alone — the fix stops minting new ones. Another behaviour-only bump (BUG-2583). **v0.16** lets an MCP agent UNASSIGN an item over the REMOTE transport (TASK-2571). No tool/action/param shape changed — this is a BEHAVIOR bump on the same grounds as v0.9: an empty-string `assigned_user_id` / `agent_role_id`, passed at the top level or as `field: ["assigned_user_id="]`, was silently dropped by two dispatch-path filters (`mapItemUpdate`, `liftFieldsToColumns`) and is now forwarded as a clear-to-NULL. The store has had defined clear semantics for exactly these two columns since BUG-2566 and HTTP inherited them, so this is uniformity restoration — MCP was the only surface with no way to unassign. Compat posture accepted deliberately: today's `""` senders get a no-op, and a no-op is the surprising reading. The empty-string filter on `tags` at the same call site STAYS (codex #547 r3 P2) — `tags: ""` is a corrupt JSONB/TEXT write, not a clear; same-looking guard, opposite justification. `clear_assigned_user` / `clear_agent_role` schema flags (option (b)) deliberately skipped as additive sugar, though codex review reopened the case — the catalog exposes `assign` / `role`, NOT the ID params, so an agent reading the schema still can't discover the clear (IDEA-2584); an empty `assign` is deliberately left inert because every other schema-declared string on that mapper treats empty as not-provided. **Transport scope:** v0.16 fixed the REMOTE /mcp transport only; v0.17 (BUG-2583) closed the local-stdio half at the CLI. **v0.15** adds the `unparented` boolean to `pad_item.list`, mutually exclusive with `parent`, for structural loose-item filtering (TASK-2096). **v0.14** added a `history` action to `pad_item` (read-only item version history — newest-first metadata; content body omitted for token thrift) and an `expected_updated_at` param for optimistic concurrency on `update` (round-trip the `updated_at` you last read; a stale value fails with a structured 409 `code=update_conflict`). The `update` action's field writes are now a server-side field-level MERGE (only the keys you set change) rather than a full-blob replace, closing the concurrent-update lost-write race (IDEA-1480 / TASK-2022) — pure addition to the action enum + param vocabulary; existing `pad_item` actions/params are unchanged and backwards-compatible. **v0.13** adds `ready` + `stale` actions to `pad_project`, mirroring the existing CLI `pad project ready` / `pad project stale` (TASK-2019): `ready` (read-only) returns the actionable backlog — the query-oriented counterpart to `next`, reusing the dashboard's suggested-next logic; `stale` (read-only) lists items needing attention (stalled, blocked, overdue, or out of the active workflow). Both HTTP dispatchers already existed (`dispatch_http_project.go`); this just wires them onto the catalog. `pad project reconcile` stays CLI-only (shells out to `gh` for live PR state — a local-git dependency MCP agents lack). Pure addition of two read-only actions — existing actions unchanged; backwards-compatible for v0.12 consumers that don't enumerate the new actions. **v0.12** adds an `activity` action to `pad_project`, mirroring the new CLI `pad project activity [--limit N] [--actor user|agent] [--since DATE]` (TASK-2018) — the non-streaming, bounded query counterpart to the CLI-only `pad project watch` SSE stream. Read-only snapshot of the workspace's enriched activity feed (item refs, titles, field-level change details) backed by the existing `GET /workspaces/{ws}/activity` endpoint (previously web-UI-only, now extended with a server-side `since` date filter so `limit`/`actor`/`since` behave identically across CLI, stdio MCP, and cloud HTTP), so agents can catch up on what other agents/users did since they last worked. Adds `actor` + `limit` params to the `pad_project` vocabulary (`since` already existed for changelog); pure addition — existing actions unchanged; backwards-compatible for v0.11 consumers that don't enumerate the new action. **v0.11** adds the read-only `pad_attachment` tool (the tenth resource × action tool) with `list` + `show` actions, mirroring the CLI `pad attachment list` / `pad attachment show` (TASK-2017): `list` enumerates a workspace's attachments (optional filters: item / category / collection / attached / unattached / sort / limit / offset); `show` returns one attachment's metadata (MIME, size, filename, ETag, last-modified) via a HEAD request without transferring bytes. Both HTTP dispatchers already existed (`dispatch_http_attachments.go`); this just wires them onto the catalog. Upload / download / view stay CLI-only (filesystem-bound, excluded per the catalog's exclusion rules). Pure addition — existing tools/actions unchanged; backwards-compatible for v0.10 consumers that don't enumerate the new tool. The base64 image RESOURCE for multimodal agents (`pad://workspace/{ws}/attachments/{id}`) shipped later in TASK-2077 (PR #930) as a bounded, image-only resource; TASK-2101 brought it — and the full read-only resource set — to the remote /mcp transport via the in-process `HTTPResourceFetcher`, so resources are no longer local-stdio-only. **v0.10** enforces the draft-playbook gate server-side: `pad_playbook.run` (and the underlying `POST /playbooks/{ref}/run`) now refuses a playbook whose `status` isn't `active` with a structured `playbook_not_active` error, adds an `allow_draft` boolean param (bareword `--allow-draft` on the CLI) as the escape hatch, and echoes the playbook `status` on both the `run` and `get` responses (BUG-2020). **v0.9** makes `pad_item.list` summary-shaped by default (drops item `content`, adds a default result limit of 50 / hard max 300 on MCP; CLI `--full` restores the complete shape) — a behavior change to the tool's return shape, hence the bump, though tool names, action enums, and parameter shapes are unchanged (TASK-2000). **v0.8** adds `restore` + `deleted` actions to `pad_workspace`, mirroring the CLI `pad workspace restore` / `pad workspace deleted` (TASK-1972): `deleted` (read-only) lists the caller's soft-deleted workspaces still inside the 30-day restore window; `restore` (mutating, not destructive, owner-only) un-soft-deletes a workspace by `slug` while it's still restorable. Both reuse the existing `slug` param — no new params; pure addition. **v0.7** adds `export` + `import` actions to `pad_item`, mirroring the CLI `pad item export` / `pad item import` (covers playbooks AND conventions). `export` (read-only) takes `ref` and returns the portable artifact text — it forces the CLI's stdout sink (`-o -`) so the bytes come back as the result instead of a file. `import` (mutating, not destructive) takes a new `artifact` param (the full artifact text) and returns `{ref, slug, warnings}`; the ExecDispatcher can't pipe stdin, so it spills the artifact to a temp file and dispatches `item import `. v0.6 added the `pad_item.backlinks` action; v0.5 added `pad_library`. v0.3 (PLAN-1377 / TASK-1380) introduced `pad_meta.action: bootstrap`, `pad_set_workspace`'s embedded-bootstrap response, and the `pad://workspace/{ws}/bootstrap` resource. **v0.4 (PLAN-1410)** is a comprehensive bootstrap-payload trim — same tool catalog, slimmer JSON shape inside bootstrap responses: `BootstrapCollection` projection drops `id`/`workspace_id`/timestamps/`settings` and emits `schema` as a nested object; `BootstrapRole` projection drops UUIDs/timestamps/`tools`; convention `slug` dropped; top-level `recent_activity` (a duplicate of `dashboard.recent_activity`) removed; new `BootstrapDashboard` wrapper caps five sub-arrays (`attention`, `recent_activity`, `active_items`, `active_plans`, `by_role`) at 5 entries each with parallel `*_overflow_count` fields; redundant schema labels omitted when `label == TitleCase(key)`. Cumulative size reduction: ~40% on a representative workspace, ~54% on the fixture (see PLAN-1410's Result section for per-section deltas). Compatibility: most changes are subtractive (dropped fields) or additive (overflow counts), but **one type change is breaking**: `collections[].schema` went from a JSON-encoded string to a nested JSON object — clients that JSON.parse()'d the string need to consume it directly as an object now. The dropped fields (UUIDs, timestamps, settings, duplicate `recent_activity`, convention `slug`) have canonical alternatives (slugs for addressing; `pad collection list` / `pad role list` for the full models when needed). +- `ToolSurfaceVersion` (currently `"0.21"`) — the MCP tool catalog contract. Bump when tool names, action enums, or parameter shapes change incompatibly. **v0.21** bounds `pad_item.action=history` (BUG-2608): the `limit` param now covers it, default 50 / max 300, applied in the CATALOG action so it reaches both transports (HTTP reads the input; stdio gets the CLI's new `--limit` via BuildCLIArgs). The window is the NEWEST N and there is deliberately no `offset` — versions are reverse patches, so only a newest-end window is cheap to reconstruct. Additive param bump; a v0.20 consumer sending no limit now receives the newest 50 rather than every version, which is the fix. Summary mode additionally asks the server to skip patch resolution rather than resolving bodies the dispatcher discards. **v0.19** adds a `clear_parent` boolean to `pad_item` (BUG-2078) — an ADDITIVE param bump, same grounds as v0.18; nothing existing changed shape. The server has supported clearing a parent since BUG-2013 (`extractParentLink` treats a present-but-empty `parent` key in `fields_patch` as detach), but neither client surface could reach it — `--parent ""` was a silent no-op on the CLI and the MCP `parent` param has the same "empty means not provided" convention every other declared string on the tool has. Boolean rather than overloading the empty string, same two reasons as v0.18: keeps that invariant intact for every other param, and only a boolean reaches LOCAL STDIO via `BuildCLIArgs`, mapping to a new `--clear-parent` bareword flag exactly as `clear_assigned_user` maps to `--clear-assigned-user`. Update-only, same asymmetry as v0.18. A simultaneous `parent` + `clear_parent` — including via `field: ["parent=..."]` or the `plan` alias `extractParentLink` also accepts — is REFUSED on both transports, not silently resolved (codex round 1). Also refused, not silently applied: `clear_parent` against a collection whose schema declares its own `parent`/`plan` field — `extractParentLink` skips hierarchy handling entirely for a schema-shadowed key and lets it fall through as an ordinary field write, so the wire shape `{"parent":""}` can no longer distinguish clear-hierarchy intent from a legitimate blank-a-real-field write once it reaches the server; the ambiguity is created at the client surface that accepted `clear_parent`, so that surface refuses rather than guessing (codex round 2). **v0.18** adds `clear_assigned_user` / `clear_agent_role` booleans to `pad_item` (IDEA-2584) — an ADDITIVE param bump (v0.5/v0.6 precedent); nothing existing changed shape and v0.16/v0.17's empty-string forms still work, undeprecated. v0.16 and v0.17 made the clear WORK; nothing advertised it, because the params that do it were never in the catalog, so an agent reading the schema reached for `assign: ""` (a no-op, and it stays one). Booleans rather than declaring the string params, for two reasons: an empty DECLARED string is inert everywhere else on the tool, so giving one a destructive meaning would let a param-padding client silently unassign everything; and only a boolean can reach LOCAL STDIO, since `BuildCLIArgs` emits the CLI's real flags and a param with no flag behind it is dropped — these map to new `--clear-assigned-user` / `--clear-agent-role` bareword flags, exactly as `allow_draft` maps to `--allow-draft`. Update-only, deliberately asymmetric with create (clearing at create has no honest behaviour but a no-op; a test fails if someone adds them there). Server-side it is wiring, not new semantics: `models.ItemUpdate.ClearAssignedUser`/`ClearAgentRole` already existed with store support since BUG-2566. **v0.17** closes the transport gap v0.16 documented: local stdio MCP shells out to the CLI, which wrote `--field assigned_user_id=` into the item's FIELDS BLOB while the column stayed stale and then printed "Updated TASK-9". `cmd/pad/cmd_item.go` now lifts `columnFieldKeys` onto the columns on create AND update, mirroring `liftFieldsToColumns` and its INVARIANT. Two compat changes, ruled separately: non-empty values move to the column and stop writing the blob key (relying on the old behaviour is relying on a shadowing defect), and empty values clear (falls out of the lift, inherits BUG-2566). Existing stray blob keys are left alone — the fix stops minting new ones. Another behaviour-only bump (BUG-2583). **v0.16** lets an MCP agent UNASSIGN an item over the REMOTE transport (TASK-2571). No tool/action/param shape changed — this is a BEHAVIOR bump on the same grounds as v0.9: an empty-string `assigned_user_id` / `agent_role_id`, passed at the top level or as `field: ["assigned_user_id="]`, was silently dropped by two dispatch-path filters (`mapItemUpdate`, `liftFieldsToColumns`) and is now forwarded as a clear-to-NULL. The store has had defined clear semantics for exactly these two columns since BUG-2566 and HTTP inherited them, so this is uniformity restoration — MCP was the only surface with no way to unassign. Compat posture accepted deliberately: today's `""` senders get a no-op, and a no-op is the surprising reading. The empty-string filter on `tags` at the same call site STAYS (codex #547 r3 P2) — `tags: ""` is a corrupt JSONB/TEXT write, not a clear; same-looking guard, opposite justification. `clear_assigned_user` / `clear_agent_role` schema flags (option (b)) deliberately skipped as additive sugar, though codex review reopened the case — the catalog exposes `assign` / `role`, NOT the ID params, so an agent reading the schema still can't discover the clear (IDEA-2584); an empty `assign` is deliberately left inert because every other schema-declared string on that mapper treats empty as not-provided. **Transport scope:** v0.16 fixed the REMOTE /mcp transport only; v0.17 (BUG-2583) closed the local-stdio half at the CLI. **v0.15** adds the `unparented` boolean to `pad_item.list`, mutually exclusive with `parent`, for structural loose-item filtering (TASK-2096). **v0.14** added a `history` action to `pad_item` (read-only item version history — newest-first metadata; content body omitted for token thrift) and an `expected_updated_at` param for optimistic concurrency on `update` (round-trip the `updated_at` you last read; a stale value fails with a structured 409 `code=update_conflict`). The `update` action's field writes are now a server-side field-level MERGE (only the keys you set change) rather than a full-blob replace, closing the concurrent-update lost-write race (IDEA-1480 / TASK-2022) — pure addition to the action enum + param vocabulary; existing `pad_item` actions/params are unchanged and backwards-compatible. **v0.13** adds `ready` + `stale` actions to `pad_project`, mirroring the existing CLI `pad project ready` / `pad project stale` (TASK-2019): `ready` (read-only) returns the actionable backlog — the query-oriented counterpart to `next`, reusing the dashboard's suggested-next logic; `stale` (read-only) lists items needing attention (stalled, blocked, overdue, or out of the active workflow). Both HTTP dispatchers already existed (`dispatch_http_project.go`); this just wires them onto the catalog. `pad project reconcile` stays CLI-only (shells out to `gh` for live PR state — a local-git dependency MCP agents lack). Pure addition of two read-only actions — existing actions unchanged; backwards-compatible for v0.12 consumers that don't enumerate the new actions. **v0.12** adds an `activity` action to `pad_project`, mirroring the new CLI `pad project activity [--limit N] [--actor user|agent] [--since DATE]` (TASK-2018) — the non-streaming, bounded query counterpart to the CLI-only `pad project watch` SSE stream. Read-only snapshot of the workspace's enriched activity feed (item refs, titles, field-level change details) backed by the existing `GET /workspaces/{ws}/activity` endpoint (previously web-UI-only, now extended with a server-side `since` date filter so `limit`/`actor`/`since` behave identically across CLI, stdio MCP, and cloud HTTP), so agents can catch up on what other agents/users did since they last worked. Adds `actor` + `limit` params to the `pad_project` vocabulary (`since` already existed for changelog); pure addition — existing actions unchanged; backwards-compatible for v0.11 consumers that don't enumerate the new action. **v0.11** adds the read-only `pad_attachment` tool (the tenth resource × action tool) with `list` + `show` actions, mirroring the CLI `pad attachment list` / `pad attachment show` (TASK-2017): `list` enumerates a workspace's attachments (optional filters: item / category / collection / attached / unattached / sort / limit / offset); `show` returns one attachment's metadata (MIME, size, filename, ETag, last-modified) via a HEAD request without transferring bytes. Both HTTP dispatchers already existed (`dispatch_http_attachments.go`); this just wires them onto the catalog. Upload / download / view stay CLI-only (filesystem-bound, excluded per the catalog's exclusion rules). Pure addition — existing tools/actions unchanged; backwards-compatible for v0.10 consumers that don't enumerate the new tool. The base64 image RESOURCE for multimodal agents (`pad://workspace/{ws}/attachments/{id}`) shipped later in TASK-2077 (PR #930) as a bounded, image-only resource; TASK-2101 brought it — and the full read-only resource set — to the remote /mcp transport via the in-process `HTTPResourceFetcher`, so resources are no longer local-stdio-only. **v0.10** enforces the draft-playbook gate server-side: `pad_playbook.run` (and the underlying `POST /playbooks/{ref}/run`) now refuses a playbook whose `status` isn't `active` with a structured `playbook_not_active` error, adds an `allow_draft` boolean param (bareword `--allow-draft` on the CLI) as the escape hatch, and echoes the playbook `status` on both the `run` and `get` responses (BUG-2020). **v0.9** makes `pad_item.list` summary-shaped by default (drops item `content`, adds a default result limit of 50 / hard max 300 on MCP; CLI `--full` restores the complete shape) — a behavior change to the tool's return shape, hence the bump, though tool names, action enums, and parameter shapes are unchanged (TASK-2000). **v0.8** adds `restore` + `deleted` actions to `pad_workspace`, mirroring the CLI `pad workspace restore` / `pad workspace deleted` (TASK-1972): `deleted` (read-only) lists the caller's soft-deleted workspaces still inside the 30-day restore window; `restore` (mutating, not destructive, owner-only) un-soft-deletes a workspace by `slug` while it's still restorable. Both reuse the existing `slug` param — no new params; pure addition. **v0.7** adds `export` + `import` actions to `pad_item`, mirroring the CLI `pad item export` / `pad item import` (covers playbooks AND conventions). `export` (read-only) takes `ref` and returns the portable artifact text — it forces the CLI's stdout sink (`-o -`) so the bytes come back as the result instead of a file. `import` (mutating, not destructive) takes a new `artifact` param (the full artifact text) and returns `{ref, slug, warnings}`; the ExecDispatcher can't pipe stdin, so it spills the artifact to a temp file and dispatches `item import `. v0.6 added the `pad_item.backlinks` action; v0.5 added `pad_library`. v0.3 (PLAN-1377 / TASK-1380) introduced `pad_meta.action: bootstrap`, `pad_set_workspace`'s embedded-bootstrap response, and the `pad://workspace/{ws}/bootstrap` resource. **v0.4 (PLAN-1410)** is a comprehensive bootstrap-payload trim — same tool catalog, slimmer JSON shape inside bootstrap responses: `BootstrapCollection` projection drops `id`/`workspace_id`/timestamps/`settings` and emits `schema` as a nested object; `BootstrapRole` projection drops UUIDs/timestamps/`tools`; convention `slug` dropped; top-level `recent_activity` (a duplicate of `dashboard.recent_activity`) removed; new `BootstrapDashboard` wrapper caps five sub-arrays (`attention`, `recent_activity`, `active_items`, `active_plans`, `by_role`) at 5 entries each with parallel `*_overflow_count` fields; redundant schema labels omitted when `label == TitleCase(key)`. Cumulative size reduction: ~40% on a representative workspace, ~54% on the fixture (see PLAN-1410's Result section for per-section deltas). Compatibility: most changes are subtractive (dropped fields) or additive (overflow counts), but **one type change is breaking**: `collections[].schema` went from a JSON-encoded string to a nested JSON object — clients that JSON.parse()'d the string need to consume it directly as an object now. The dropped fields (UUIDs, timestamps, settings, duplicate `recent_activity`, convention `slug`) have canonical alternatives (slugs for addressing; `pad collection list` / `pad role list` for the full models when needed). Both are also returned by `pad://_meta/version` and `pad_meta.action: version`. +**Where result caps live.** Two layers, deliberately different numbers. The MCP catalog action injects the agent-facing default and ceiling (list / backlinks / history: default 50, max 300) because a token budget is only knowable there. The HTTP endpoint's own clamp is a server-resource ceiling on what any caller may ASK for (`maxItemListQueryLimit` = 1000; `maxItemVersionsQueryLimit` = 500, lower because resolving a version can cost a patch application per row), and an ABSENT limit is left unbounded rather than defaulted — a server that truncates a request nobody bounded is a silent-truncation trap for direct API consumers. The CLI carries its own default for the same reason the catalog does. + **Dispatchers.** Two ship in `internal/mcp/`: - `ExecDispatcher` — shells out to the `pad` binary; subprocess inherits credentials from `~/.pad/credentials.json`. Used by `pad mcp serve` for local stdio MCP. diff --git a/README.md b/README.md index 03b31bd5..e5e7221b 100644 --- a/README.md +++ b/README.md @@ -349,7 +349,7 @@ directory for `claude-code`, and an `[mcp_servers.pad]` table in project-scoped, it's install-on-request only — `--all` and `pad mcp status` cover the per-user clients (including Codex) and skip it. -**Tool catalog (v0.20)** — ten resource × action tools plus `pad_set_workspace` (eleven total), no flat verb explosion. `pad_item.list` accepts `unparented: true` (mutually exclusive with `parent`) to select items with no parent or implements relationship, and is summary-shaped by default on both transports (`full: true` opts into complete content bodies): +**Tool catalog (v0.21)** — ten resource × action tools plus `pad_set_workspace` (eleven total), no flat verb explosion. `pad_item.list` accepts `unparented: true` (mutually exclusive with `parent`) to select items with no parent or implements relationship, and is summary-shaped by default on both transports (`full: true` opts into complete content bodies): | Tool | Actions | |---|---| @@ -378,7 +378,7 @@ initialize handshake under `capabilities.experimental.padCmdhelp` and `pad://_meta/version`): - `cmdhelp_version: "0.1"` — CLI help-tree contract (used at dispatch time) -- `tool_surface_version: "0.20"` — MCP tool catalog contract (v0.5 added `pad_library`; v0.6 `pad_item.backlinks`; v0.7 `pad_item` `export`/`import`; v0.8 `pad_workspace` `deleted`/`restore`; v0.9 made `pad_item.list` summary-shaped by default with a default+max result cap; v0.10 enforced the draft-playbook gate server-side on `pad_playbook.run` with an `allow_draft` escape hatch; v0.11 added the read-only `pad_attachment` tool (`list`/`show`); v0.12 added `pad_project.activity` (agent-accessible non-streaming activity feed); v0.13 added `pad_project` `ready`/`stale` (agent-oriented backlog + attention queries); v0.14 added `pad_item` `history` + optimistic concurrency (TASK-2022); v0.15 added the `pad_item.list` `unparented` parameter (TASK-2096); v0.16 made an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an agent can finally unassign an item (TASK-2571); v0.17 carried that to the LOCAL STDIO transport by teaching the CLI to lift those keys onto their columns instead of into the fields blob (BUG-2583); v0.18 added `clear_assigned_user` / `clear_agent_role` booleans — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` flags on `pad item update` (IDEA-2584); v0.19 added a `clear_parent` boolean — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` flag on `pad item update` (BUG-2078); v0.20 gave every tool an explicit annotation block derived from the catalog’s read-only knowledge — fully-read-only tools advertise `readOnlyHint: true` / `destructiveHint: false`, all-additive-write tools (`pad_workspace`, `pad_library`) drop `destructiveHint`, overwrite/delete-capable tools stay conservatively destructive, `openWorldHint: false` everywhere — replacing mcp-go’s defaults that marked every tool destructive (BUG-2302), and made `pad_item.list` summary-shaped on the remote HTTP transport too, with a declared `full` boolean as the opt-in for complete bodies on both transports (BUG-2305); see `internal/mcp/version.go` for the full changelog) +- `tool_surface_version: "0.21"` — MCP tool catalog contract (v0.5 added `pad_library`; v0.6 `pad_item.backlinks`; v0.7 `pad_item` `export`/`import`; v0.8 `pad_workspace` `deleted`/`restore`; v0.9 made `pad_item.list` summary-shaped by default with a default+max result cap; v0.10 enforced the draft-playbook gate server-side on `pad_playbook.run` with an `allow_draft` escape hatch; v0.11 added the read-only `pad_attachment` tool (`list`/`show`); v0.12 added `pad_project.activity` (agent-accessible non-streaming activity feed); v0.13 added `pad_project` `ready`/`stale` (agent-oriented backlog + attention queries); v0.14 added `pad_item` `history` + optimistic concurrency (TASK-2022); v0.15 added the `pad_item.list` `unparented` parameter (TASK-2096); v0.16 made an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an agent can finally unassign an item (TASK-2571); v0.17 carried that to the LOCAL STDIO transport by teaching the CLI to lift those keys onto their columns instead of into the fields blob (BUG-2583); v0.18 added `clear_assigned_user` / `clear_agent_role` booleans — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` flags on `pad item update` (IDEA-2584); v0.19 added a `clear_parent` boolean — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` flag on `pad item update` (BUG-2078); v0.20 gave every tool an explicit annotation block derived from the catalog’s read-only knowledge — fully-read-only tools advertise `readOnlyHint: true` / `destructiveHint: false`, all-additive-write tools (`pad_workspace`, `pad_library`) drop `destructiveHint`, overwrite/delete-capable tools stay conservatively destructive, `openWorldHint: false` everywhere — replacing mcp-go’s defaults that marked every tool destructive (BUG-2302), and made `pad_item.list` summary-shaped on the remote HTTP transport too, with a declared `full` boolean as the opt-in for complete bodies on both transports (BUG-2305); v0.21 bounded `pad_item.history`, which was unbounded on every surface — `limit` now covers it (default 50, max 300, the NEWEST N; no `offset`, because reverse-patch storage makes only a newest-end window cheap), applied in the catalog action so it lands on both transports, and summary mode now asks the server to skip patch resolution rather than resolving bodies the dispatcher discards (BUG-2608); see `internal/mcp/version.go` for the full changelog) External agents pin against these so a future rename doesn't break them silently. Errors come back as structured envelopes (`{error: {code, diff --git a/cmd/pad/cmd_item.go b/cmd/pad/cmd_item.go index e1dbada0..529d15a6 100644 --- a/cmd/pad/cmd_item.go +++ b/cmd/pad/cmd_item.go @@ -1294,8 +1294,18 @@ type itemVersionSummary struct { ChangeSummary string `json:"change_summary,omitempty"` } +// defaultItemHistoryLimit bounds `pad item history` when the caller does not +// ask for a window. The endpoint itself is deliberately unbounded when no +// limit is sent (see maxItemVersionsQueryLimit) — the default belongs here, +// where a terminal and a token budget are the actual constraint, mirroring how +// TASK-2000 put the item-list default on the clients rather than the server. +// A collab-heavy item accumulates a version every few seconds while someone +// types, so "all of them" is rarely the question being asked. +const defaultItemHistoryLimit = 50 + func historyCmd() *cobra.Command { var full bool + var limit int cmd := &cobra.Command{ Use: "history ", @@ -1307,11 +1317,21 @@ Each row is a snapshot captured when the item's content changed (edits from the web editor, CLI, MCP, collab flushes, and version restores). This is a READ-ONLY view — use the web UI to restore a specific version. +Shows the newest 50 by default; pass --limit 0 for the whole history. A +collab-edited item records a version every few seconds while someone types, so +histories get long. + +A single request is capped server-side, so a very large --limit returns the +cap rather than everything, and the "showing the newest N" notice cannot detect +that case. Use --limit 0 when you genuinely want the complete history. + Items can be referenced by issue ID (e.g. TASK-5) or slug. Examples: pad item history TASK-5 pad item versions TASK-5 --format json + pad item history TASK-5 --limit 10 # newest 10 only + pad item history TASK-5 --limit 0 # all versions pad item history TASK-5 --full --format json # include resolved content`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { @@ -1319,10 +1339,36 @@ Examples: ws := getWorkspace() slug := args[0] - versions, err := client.ListItemVersions(ws, slug) + if limit < 0 { + return fmt.Errorf("--limit must be zero or positive") + } + // Ask for ONE MORE than we intend to show. That extra row is the + // only honest way to tell "there are more" from "that is all + // of them" — comparing the response length against the requested + // limit calls an exactly-N history truncated, and stays silent + // when it really was cut short. + // + // Guarded against overflow: limit+1 at MaxInt wraps negative, the + // client then omits the parameter, and the "bounded" request comes + // back unbounded — the opposite of what was asked for. + ask := limit + if ask > 0 && ask < math.MaxInt { + ask = limit + 1 + } + + // Resolve content only when it will actually be SHOWN. --full + // alone is not enough: the table path prints no bodies at any + // setting, so pairing --full with table output would make the + // server walk the whole patch chain for output that discards it. + wantsContent := full && formatFlag == "json" + versions, err := client.ListItemVersionsPage(ws, slug, ask, !wantsContent) if err != nil { return err } + truncated := limit > 0 && len(versions) > limit + if truncated { + versions = versions[:limit] + } if formatFlag == "json" { if full { @@ -1360,11 +1406,26 @@ Examples: ) } fmt.Printf("\n%d version(s).\n", len(versions)) + // Say so when the window was capped. A silent cap reads as "this + // is the whole history", which is the wrong thing to believe + // about an audit trail. + // + // One case this cannot detect: a --limit AT OR ABOVE the server's + // own ceiling is clamped there, and the probe row is clamped away + // with it, so the result looks complete. The CLI does not + // hardcode the server's ceiling to paper over that — asking for + // hundreds of versions is already opting out of a bound, and a + // duplicated constant would go stale silently. + if truncated { + fmt.Printf("Showing the newest %d — pass --limit 0 for all, or --limit N for more.\n", limit) + } return nil }, } cmd.Flags().BoolVar(&full, "full", false, "include each version's resolved content body (JSON output only)") + cmd.Flags().IntVar(&limit, "limit", defaultItemHistoryLimit, + "show only the newest N versions (0 = all; very large values are capped server-side)") return cmd } diff --git a/internal/cli/client.go b/internal/cli/client.go index 6fa60673..73e0bcb7 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -286,8 +286,32 @@ func wrapItemNotFound(err error, itemSlug, wsSlug string) error { // `pad item history` (TASK-2022). Reuses the existing read-only // GET /items/{slug}/versions endpoint — no new store surface. func (c *Client) ListItemVersions(wsSlug, itemSlug string) ([]models.Version, error) { + return c.ListItemVersionsPage(wsSlug, itemSlug, 0, false) +} + +// ListItemVersionsPage is ListItemVersions with the BUG-2608 bounds: `limit` +// caps the newest-first window (0 = server default, i.e. unbounded), and +// `summary` asks the server to skip reverse-patch resolution and return +// metadata only. +// +// Pass summary=true whenever the caller is going to discard content. It is not +// merely a smaller response: resolving means walking the item's entire patch +// chain, so a history listing that projects to metadata was paying for bodies +// it never showed. +func (c *Client) ListItemVersionsPage(wsSlug, itemSlug string, limit int, summary bool) ([]models.Version, error) { + path := "/workspaces/" + wsSlug + "/items/" + itemSlug + "/versions" + q := url.Values{} + if limit > 0 { + q.Set("limit", strconv.Itoa(limit)) + } + if summary { + q.Set("summary", "true") + } + if len(q) > 0 { + path += "?" + q.Encode() + } var result []models.Version - return result, c.get("/workspaces/"+wsSlug+"/items/"+itemSlug+"/versions", &result) + return result, c.get(path, &result) } // RestoreItem un-archives a soft-deleted item via the restore endpoint, which diff --git a/internal/mcp/catalog_item.go b/internal/mcp/catalog_item.go index 76b1f623..24bff0fe 100644 --- a/internal/mcp/catalog_item.go +++ b/internal/mcp/catalog_item.go @@ -82,7 +82,7 @@ var padItemTool = ToolDef{ // (newest-first), a token-light summary shape (id, created_at, // created_by, source, change_summary) with the resolved content // body omitted. Restoring a version stays a web-UI action. - "history": passThrough([]string{"item", "history"}), + "history": actionItemHistory, // Bulk + notes + decisions // bulk-update is custom because the CLI takes repeatable @@ -208,7 +208,7 @@ var padItemSchemaParams = []ParamDef{ // ── List / starred ── {Name: "all", Type: "bool", Description: "Include archived/done items in list responses. Optional for: list, starred."}, - {Name: "limit", Type: "number", Description: "Maximum results. Optional for: list, backlinks. List defaults to 50, max 300. Backlinks defaults to 50, max 300."}, + {Name: "limit", Type: "number", Description: "Maximum results. Optional for: list, backlinks, history. Each defaults to 50, max 300. For history the window is the NEWEST N versions."}, {Name: "offset", Type: "number", Description: "Skip the first N results (paging). Optional for: backlinks."}, {Name: "sort", Type: "string", Description: "Sort field. Optional for: list."}, {Name: "group_by", Type: "string", Description: "Group-by field. Optional for: list."}, @@ -309,7 +309,8 @@ Actions: reference TASK-5?" without scanning the full content corpus. history — Read an item's version history (newest-first, read-only). - Required: ref. + Required: ref. Optional: limit (default 50, max 300 — + the NEWEST N versions), full. Returns a token-light summary per recorded version (id, created_at, created_by, source, change_summary); the resolved content body is omitted. Restoring a @@ -694,6 +695,43 @@ func actionItemList(ctx context.Context, input map[string]any, env ActionEnv) (* return env.Dispatch(ctx, []string{"item", "list"}, out) } +// mcpItemHistoryDefaultLimit / MaxLimit bound pad_item.action=history the same +// way list and backlinks are bounded, and for a sharper reason: a +// collab-edited item records a version every few seconds while someone types, +// so "the whole history" is routinely hundreds of rows nobody asked for +// (BUG-2608). Same numbers as list, so an agent does not have to remember a +// third pair. +const ( + mcpItemHistoryDefaultLimit = 50 + mcpItemHistoryMaxLimit = 300 +) + +// actionItemHistory handles pad_item.action=history. It injects a default +// limit when the agent did not ask for one and clamps an oversized one, so +// this lands on BOTH transports: the HTTP dispatcher reads it off the input, +// and the exec path gets it as the CLI's --limit through BuildCLIArgs. +// +// The catalog is the right home for the default (rather than either +// dispatcher) for the reason actionItemList already documents — it is the one +// place both transports pass through. +func actionItemHistory(ctx context.Context, input map[string]any, env ActionEnv) (*mcp.CallToolResult, error) { + out := make(map[string]any, len(input)+1) + for k, v := range input { + out[k] = v + } + + limit := mcpItemHistoryDefaultLimit + if n, ok := numericInput(input["limit"]); ok && n > 0 { + limit = int(n) + } + if limit > mcpItemHistoryMaxLimit { + limit = mcpItemHistoryMaxLimit + } + out["limit"] = limit + + return env.Dispatch(ctx, []string{"item", "history"}, out) +} + // actionItemExport handles pad_item.action=export. The CLI's // `pad item export ` defaults to WRITING A FILE (.pad.md), // which is useless to an MCP caller — the bytes have to come back as diff --git a/internal/mcp/catalog_readonly_test.go b/internal/mcp/catalog_readonly_test.go index 3b38dddb..e8190b35 100644 --- a/internal/mcp/catalog_readonly_test.go +++ b/internal/mcp/catalog_readonly_test.go @@ -687,6 +687,12 @@ func liveCmdhelpDoc(t *testing.T) *cmdhelp.Document { Flags: func() map[string]cmdhelp.Flag { f := mkFlags("workspace") f["full"] = cmdhelp.Flag{Type: "bool"} + // Mirrors `pad item history --limit` (BUG-2608). This + // fixture is what BuildCLIArgs consults in tests, so a + // flag missing here is silently dropped from the emitted + // args — the catalog default would look applied and reach + // the CLI as nothing. + f["limit"] = cmdhelp.Flag{Type: "int"} return f }(), }, diff --git a/internal/mcp/dispatch_http_history_bounds_test.go b/internal/mcp/dispatch_http_history_bounds_test.go new file mode 100644 index 00000000..a3f32698 --- /dev/null +++ b/internal/mcp/dispatch_http_history_bounds_test.go @@ -0,0 +1,146 @@ +package mcp + +// BUG-2608 — `pad_item.action=history` was unbounded, and summary mode paid +// for content it discarded: the endpoint resolved every version by walking the +// item's whole reverse-patch chain, and the dispatcher then projected that +// away to metadata. +// +// Two independent claims, tested where each actually lives: +// - the DEFAULT window is injected by the catalog action, so it reaches both +// transports — asserted on the CLI args the action produces, which is also +// the stdio half (BuildCLIArgs emits `--limit`). +// - the HTTP dispatcher asks the server to SKIP resolution when the caller +// did not ask for content — asserted on the request it builds. + +import ( + "context" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +func historyArgs(t *testing.T, input map[string]any) string { + t.Helper() + disp := &fakeDispatcher{} + env := ActionEnv{Doc: liveCmdhelpDoc(t), Workspace: NewWorkspaceState("docapp"), Dispatcher: disp} + res, err := actionItemHistory(context.Background(), input, env) + if err != nil { + t.Fatalf("actionItemHistory error: %v", err) + } + if res != nil && res.IsError { + t.Fatalf("error result: %s", textOf(res)) + } + return strings.Join(disp.gotArgs, " ") +} + +// A bare history call must be bounded. Asserting the CLI args covers the exec +// transport at the same time: the default only reaches stdio because +// BuildCLIArgs turns the param into the CLI's --limit flag. +func TestPadItemHistory_AppliesDefaultLimit(t *testing.T) { + joined := historyArgs(t, map[string]any{"ref": "TASK-5"}) + want := "--limit " + strconv.Itoa(mcpItemHistoryDefaultLimit) + if !strings.Contains(joined, want) { + t.Errorf("cliArgs %q should carry the injected default %q — an unbounded "+ + "history is the bug", joined, want) + } +} + +func TestPadItemHistory_ClampsOversizedLimit(t *testing.T) { + joined := historyArgs(t, map[string]any{"ref": "TASK-5", "limit": float64(99999)}) + want := "--limit " + strconv.Itoa(mcpItemHistoryMaxLimit) + if !strings.Contains(joined, want) { + t.Errorf("cliArgs %q should clamp to %q", joined, want) + } +} + +func TestPadItemHistory_HonorsInRangeLimit(t *testing.T) { + joined := historyArgs(t, map[string]any{"ref": "TASK-5", "limit": float64(7)}) + if !strings.Contains(joined, "--limit 7") { + t.Errorf("cliArgs %q should honor an in-range limit of 7", joined) + } +} + +// The HTTP half of the optimization. Summary mode is not merely a smaller +// response — it tells the server not to walk the patch chain at all. If the +// dispatcher stops sending it, the projection below still looks identical +// while the server goes back to resolving bodies nobody reads, which is the +// silent half of this bug. +func TestDispatchItemHistory_RequestsSummaryUnlessFullAsked(t *testing.T) { + for _, tc := range []struct { + name string + input map[string]any + wantSummary bool + }{ + {"default asks the server to skip resolution", map[string]any{ + "workspace": "docapp", "ref": "TASK-5", + }, true}, + {"full=true must NOT skip it", map[string]any{ + "workspace": "docapp", "ref": "TASK-5", "full": true, + }, false}, + } { + t.Run(tc.name, func(t *testing.T) { + rec := &queryRecordingHandler{respBody: "[]"} + d := &HTTPHandlerDispatcher{ + Handler: rec, + UserResolver: fixedUserResolver(&models.User{ + ID: "user-1", Name: "Dave", Email: "dave@example.com", + }), + } + ctx := WithDispatchInput(context.Background(), tc.input) + if _, err := d.Dispatch(ctx, []string{"item", "history"}, nil); err != nil { + t.Fatalf("Dispatch: %v", err) + } + got := rec.gotQuery.Get("summary") == "true" + if got != tc.wantSummary { + t.Errorf("summary=%v, want %v (query %q)", got, tc.wantSummary, rec.gotQuery.Encode()) + } + }) + } +} + +// The limit has to survive the trip to the server too, not just reach the +// action — the action's injection is worthless if the dispatcher drops it. +func TestDispatchItemHistory_ForwardsLimitToTheEndpoint(t *testing.T) { + rec := &queryRecordingHandler{respBody: "[]"} + d := &HTTPHandlerDispatcher{ + Handler: rec, + UserResolver: fixedUserResolver(&models.User{ + ID: "user-1", Name: "Dave", Email: "dave@example.com", + }), + } + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "ref": "TASK-5", "limit": float64(12), + }) + if _, err := d.Dispatch(ctx, []string{"item", "history"}, nil); err != nil { + t.Fatalf("Dispatch: %v", err) + } + if got := rec.gotQuery.Get("limit"); got != "12" { + t.Errorf("limit reached the endpoint as %q, want \"12\" (query %q)", + got, rec.gotQuery.Encode()) + } +} + +// queryRecordingHandler captures the QUERY STRING, which the shared +// recordingHandler does not keep — these tests are entirely about what ends up +// in it. +type queryRecordingHandler struct { + respBody string + gotQuery interface { + Get(string) string + Encode() string + } +} + +func (h *queryRecordingHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + h.gotQuery = r.URL.Query() + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(h.respBody)) +} + +var _ http.Handler = (*queryRecordingHandler)(nil) +var _ = httptest.NewRecorder diff --git a/internal/mcp/dispatch_http_routes.go b/internal/mcp/dispatch_http_routes.go index 797d09a7..8775c875 100644 --- a/internal/mcp/dispatch_http_routes.go +++ b/internal/mcp/dispatch_http_routes.go @@ -1387,7 +1387,23 @@ func (d *HTTPHandlerDispatcher) dispatchItemHistory( return validationFailedResult(cmdKey, "ref is required", "Pass ref= for the item whose history you want."), nil } + full, _ := input["full"].(bool) + urlPath := "/api/v1/workspaces/" + url.PathEscape(workspace) + "/items/" + url.PathEscape(ref) + "/versions" + q := url.Values{} + if n, ok := numericInput(input["limit"]); ok && n > 0 { + q.Set("limit", strconv.FormatInt(n, 10)) + } + // Ask the server to skip reverse-patch resolution whenever the bodies are + // about to be thrown away below. Without this the projection to + // itemVersionSummary was discarding content the server had just walked the + // item's entire patch chain to build (BUG-2608). + if !full { + q.Set("summary", "true") + } + if len(q) > 0 { + urlPath += "?" + q.Encode() + } req, err := d.buildAuthedRequest(ctx, http.MethodGet, urlPath, nil, user) if err != nil { return buildRequestErrorResult(cmdKey, err), nil @@ -1410,7 +1426,7 @@ func (d *HTTPHandlerDispatcher) dispatchItemHistory( return classifyHTTPStatusKind(req.Context(), cmdKey, urlPath, resp.StatusCode, bodyBytes, d.Lister, ResourceItem, ref), nil } - if full, _ := input["full"].(bool); full { + if full { // Complete rows requested — forward the endpoint's JSON // verbatim, the same shape stdio's --full emits. return packageJSONResult(string(bodyBytes)), nil diff --git a/internal/mcp/instructions.md b/internal/mcp/instructions.md index 45dd0708..18eebc73 100644 --- a/internal/mcp/instructions.md +++ b/internal/mcp/instructions.md @@ -6,11 +6,11 @@ Pad is a project tracker for developers and AI agents — issues (TASK, BUG), pl If the user is asking general code questions with no project-management thread, you don't need this server. -## Tool surface (v0.20) +## Tool surface (v0.21) Ten resource × action tools, plus `pad_set_workspace` (which takes a `workspace` slug only — no action enum). Eleven tools total. -- `pad_item` — Items: create / update / delete / get / list / move / restore / link / unlink / deps / star / unstar / starred / comment / list-comments / backlinks / bulk-update / note / decide / export / import / history. `list` accepts `unparented: true` to keep items with no parent or implements relationship (mutually exclusive with `parent`). `list` results are SUMMARY-shaped by default on both transports — no content bodies; pass `full: true` for complete bodies (token-expensive), or prefer `get` for a single item's body. `update` field writes are a server-side field-level merge (only the keys you set change); pass `expected_updated_at` for optimistic concurrency (a stale value fails with a structured 409 `update_conflict`). `history` returns read-only item version metadata (newest-first); pass `full: true` to include each version's resolved content body (token-expensive). To UNASSIGN an item, pass `clear_assigned_user: true` (or `clear_agent_role: true`) — the canonical form, works on both transports. To DETACH an item from its parent, pass `clear_parent: true` — same canonical shape, works on both transports. Setting and clearing the same field in one call is refused, not silently resolved, so don't pair `clear_assigned_user`/`clear_agent_role`/`clear_parent` with `assign`/`role`/`parent` respectively. An empty `assign` / `role` / `parent` does NOT clear: those name a person, a slug, or a ref, so an empty value reads as "not provided", exactly like every other optional string here. (Two older forms still work and are not deprecated: `field: ["assigned_user_id="]` on either transport, and a direct `assigned_user_id: ""` param over remote `/mcp` only — prefer the boolean, which is the only one this schema advertises.) +- `pad_item` — Items: create / update / delete / get / list / move / restore / link / unlink / deps / star / unstar / starred / comment / list-comments / backlinks / bulk-update / note / decide / export / import / history. `list` accepts `unparented: true` to keep items with no parent or implements relationship (mutually exclusive with `parent`). `list` results are SUMMARY-shaped by default on both transports — no content bodies; pass `full: true` for complete bodies (token-expensive), or prefer `get` for a single item's body. `update` field writes are a server-side field-level merge (only the keys you set change); pass `expected_updated_at` for optimistic concurrency (a stale value fails with a structured 409 `update_conflict`). `history` returns read-only item version metadata (newest-first), bounded to the NEWEST 50 versions by default (max 300 — pass `limit` to change the window); pass `full: true` to include each version's resolved content body (token-expensive). There is no `offset`: versions are stored as reverse patches, so only a newest-end window is cheap to reconstruct. To UNASSIGN an item, pass `clear_assigned_user: true` (or `clear_agent_role: true`) — the canonical form, works on both transports. To DETACH an item from its parent, pass `clear_parent: true` — same canonical shape, works on both transports. Setting and clearing the same field in one call is refused, not silently resolved, so don't pair `clear_assigned_user`/`clear_agent_role`/`clear_parent` with `assign`/`role`/`parent` respectively. An empty `assign` / `role` / `parent` does NOT clear: those name a person, a slug, or a ref, so an empty value reads as "not provided", exactly like every other optional string here. (Two older forms still work and are not deprecated: `field: ["assigned_user_id="]` on either transport, and a direct `assigned_user_id: ""` param over remote `/mcp` only — prefer the boolean, which is the only one this schema advertises.) - `pad_workspace` — Workspaces: list / members / invite / storage / audit-log / create / claim / deleted / restore. - `pad_collection` — Collections: list / create / update / delete. - `pad_project` — Project intelligence: dashboard / next / ready / stale / standup / changelog / report / activity. Use `ready` for the actionable backlog and `stale` for items needing attention; `activity` to catch up on what other agents/users changed since you last worked (non-streaming feed with item refs + change details). diff --git a/internal/mcp/version.go b/internal/mcp/version.go index 81cdd836..10486296 100644 --- a/internal/mcp/version.go +++ b/internal/mcp/version.go @@ -103,7 +103,36 @@ const CmdhelpVersion = "0.1" // pad_item actions unchanged. Backwards-compatible for v0.6 // consumers that don't enumerate the new actions. // -// - "0.20" — current. BUG-2302: every advertised tool now carries an +// - "0.21" — current. BUG-2608: bounds `pad_item.action=history`, which +// was unbounded on every surface. Extends the `limit` param's +// vocabulary to cover history (default 50, max 300 — the same pair +// list and backlinks already use, so an agent has no third set of +// numbers to remember) and applies it in the CATALOG action rather +// than either dispatcher, so it lands on BOTH transports: the HTTP +// path reads it off the input, the exec path receives it as the +// CLI's new --limit through BuildCLIArgs. +// +// ADDITIVE param bump on the v0.6/v0.18/v0.19 pattern: `limit` +// already existed, no tool, action enum, or param SHAPE changed, +// and a v0.20 consumer that sends no limit keeps working — it now +// receives the newest 50 versions instead of all of them, which is +// the point of the fix rather than a break in it. +// +// The window is the NEWEST N and there is deliberately no `offset`. +// Versions are stored as REVERSE patches, so reconstructing any +// version means walking back from the item's current content +// through everything newer: a newest-end window is the cheap prefix +// of that walk, while an older window would still pay for +// everything above it. Offering an offset would advertise a +// pagination whose later pages cost the same as no bound at all. +// +// Behaviour change worth stating even though the shape is stable: +// summary mode now asks the SERVER to skip patch resolution +// (`?summary=true`) instead of resolving every body and discarding +// it in the dispatcher. Same result payload, minus a full chain +// walk per call. +// +// - "0.20" — BUG-2302: every advertised tool now carries an // EXPLICIT annotation block derived from readOnlyActions (the same // single source the tool-surface serializer uses) instead of // inheriting mcp-go's NewTool defaults, which stamped @@ -491,7 +520,7 @@ const CmdhelpVersion = "0.1" // - result.capabilities.experimental.padToolSurface.version (handshake). // - pad://_meta/version resource (queryable JSON document). // - pad_meta.action: tool-surface (full catalog introspection). -const ToolSurfaceVersion = "0.20" +const ToolSurfaceVersion = "0.21" // MetaVersionURI is the canonical URI of the queryable version document. // Lives outside the pad://workspace/{ws}/... namespace because it's a diff --git a/internal/server/handlers_item_versions.go b/internal/server/handlers_item_versions.go index 42eaa61c..c801c9eb 100644 --- a/internal/server/handlers_item_versions.go +++ b/internal/server/handlers_item_versions.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "strconv" "github.com/go-chi/chi/v5" @@ -40,7 +41,31 @@ func (s *Server) handleListItemVersions(w http.ResponseWriter, r *http.Request) return } - versions, err := s.store.ListItemVersionsResolved(item.ID, item.Content) + // `limit` bounds the newest-first window. Absent means UNBOUNDED, matching + // the item-list endpoints (see maxItemListQueryLimit): the server does not + // truncate a request nobody asked to have truncated, and the agent-facing + // defaults live in the CLI and the MCP catalog where a token budget is + // actually known. An explicit oversized value is clamped. + limit := parseItemVersionsLimit(r.URL.Query().Get("limit")) + + // `summary` skips diff resolution entirely. Every consumer of this + // endpoint except `--full` throws the content away immediately — the CLI + // projects to metadata, and so does the MCP history dispatcher — so + // resolving it means walking the whole reverse-patch chain to build bodies + // that are discarded milliseconds later (BUG-2608). The rows themselves + // still carry patch text, so the projection below strips it rather than + // shipping patches as though they were content. + if r.URL.Query().Get("summary") == "true" { + versions, err := s.store.ListItemVersionsPage(item.ID, limit) + if err != nil { + writeInternalError(w, err) + return + } + writeJSON(w, http.StatusOK, itemVersionMetadata(versions)) + return + } + + versions, err := s.store.ListItemVersionsResolvedPage(item.ID, item.Content, limit) if err != nil { writeInternalError(w, err) return @@ -52,6 +77,70 @@ func (s *Server) handleListItemVersions(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusOK, versions) } +// maxItemVersionsQueryLimit clamps an explicit oversized `?limit=` on the +// version-history endpoint. Mirrors maxItemListQueryLimit's role: a ceiling on +// what a caller may ASK for, not a default applied to callers who ask for +// nothing. +// +// It is deliberately NOT the same number as the MCP catalog's max (300). The +// two caps answer different questions at different layers: this one is a +// server-resource ceiling, while the catalog's is an agent token budget, which +// is only knowable there. Item lists have the same split (1000 here, 300 in +// the catalog). Lower than the list ceiling because resolving a version can +// cost a patch application per row, not just a row read. +const maxItemVersionsQueryLimit = 500 + +// parseItemVersionsLimit turns the raw `?limit=` value into a row bound. +// Returns 0 — unbounded — for absent, unparseable, zero and negative input, +// so only a well-formed positive value bounds anything, and clamps anything +// above the ceiling. +// +// Split out of the handler so the clamp is directly testable. Asserting it +// through the endpoint would mean seeding 500+ versions per run to prove the +// ceiling binds; asserting it by re-implementing the arithmetic in a test +// would prove only that the test can multiply. +func parseItemVersionsLimit(raw string) int { + if raw == "" { + return 0 + } + n, err := strconv.Atoi(raw) + if err != nil { + // A value too large for an int is an OVERSIZED ask, not a malformed + // one, and Atoi still hands back the saturated bound alongside + // ErrRange. Treating it as unparseable would return 0 — unbounded — + // so `?limit=9223372036854775808` would defeat the very ceiling this + // function exists to impose (codex round 4). Range-negative falls + // through to the unbounded branch below, matching a plain negative. + if errors.Is(err, strconv.ErrRange) && n > 0 { + return maxItemVersionsQueryLimit + } + return 0 + } + if n <= 0 { + return 0 + } + if n > maxItemVersionsQueryLimit { + return maxItemVersionsQueryLimit + } + return n +} + +// itemVersionMetadata strips content from raw version rows for summary mode. +// +// Both fields are cleared together on purpose. An empty Content with IsDiff +// still true would describe a version whose body is a reverse patch waiting to +// be applied — telling a consumer to resolve something that is not there. The +// pair only means anything together, so summary mode returns neither. +func itemVersionMetadata(versions []models.Version) []models.Version { + out := make([]models.Version, 0, len(versions)) + for _, v := range versions { + v.Content = "" + v.IsDiff = false + out = append(out, v) + } + return out +} + // handleGetItemVersion returns a single version with its diff resolved to full // content. The paginated timeline serves raw reverse-patch text (it can't resolve // a partial window), so the timeline card calls this to reconstruct real content diff --git a/internal/server/handlers_item_versions_bounds_test.go b/internal/server/handlers_item_versions_bounds_test.go new file mode 100644 index 00000000..95c7a18a --- /dev/null +++ b/internal/server/handlers_item_versions_bounds_test.go @@ -0,0 +1,283 @@ +package server + +// BUG-2608 — item history was unbounded on every surface, and worse, summary +// mode paid for what it discarded: the endpoint resolved EVERY version's +// content by walking the item's whole reverse-patch chain, and both the CLI +// and the MCP dispatcher then projected that away to metadata. +// +// Two independent properties are covered here, because they fail +// independently: `limit` bounds the window, and `summary` skips the walk. + +import ( + "net/http" + "strconv" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// seedVersionedItem creates an item and edits it n times, producing n+1 +// versions' worth of history (the create plus each content change). +// +// Edits go through the STORE, not the HTTP API. That is not laziness: one of +// these fixtures needs more than 50 versions to arm (a smaller one cannot tell +// an unbounded server from one quietly defaulting to 50), and 60-odd PATCHes +// in a burst trip the server's rate limiter — which is exactly how this test +// failed in CI after the count was raised. The store is where versions are +// recorded on either path, and the endpoint under test is the READ side, so +// seeding underneath the transport costs the test nothing. +// +// Two details are load-bearing and easy to undo by accident: +// - the body is LARGE, so the store really stores reverse patches. A small +// body is cheaper whole, no version is ever is_diff, and every assertion +// about diff handling goes quietly vacuous. +// - each edit declares a DIFFERENT source, because the version throttle +// suppresses rapid snapshots from the same (actor, source) pair and would +// otherwise collapse the whole burst into one version. ForceVersion is not +// reachable from a request — it is `json:"-"` on ItemUpdate. +func seedVersionedItem(t *testing.T, srv *Server, wsSlug string, edits int) *models.Item { + t.Helper() + base := strings.Repeat("a line of body text that makes the patch worth storing\n", 200) + + rr := doRequest(srv, "POST", "/api/v1/workspaces/"+wsSlug+"/collections/tasks/items", + map[string]any{"title": "versioned", "content": base + "v0\n"}) + if rr.Code != http.StatusCreated { + t.Fatalf("create item: %d %s", rr.Code, rr.Body.String()) + } + var item models.Item + parseJSON(t, rr, &item) + + sources := []string{"web", "cli", "mcp", "skill"} + for i := 1; i <= edits; i++ { + content := base + "v" + strconv.Itoa(i) + "\n" + if _, err := srv.store.UpdateItem(item.ID, models.ItemUpdate{ + Content: &content, + Source: sources[i%len(sources)], + }); err != nil { + t.Fatalf("edit %d: %v", i, err) + } + } + return &item +} + +func fetchVersions(t *testing.T, srv *Server, wsSlug, itemSlug, query string) []models.Version { + t.Helper() + path := "/api/v1/workspaces/" + wsSlug + "/items/" + itemSlug + "/versions" + if query != "" { + path += "?" + query + } + rr := doRequest(srv, "GET", path, nil) + if rr.Code != http.StatusOK { + t.Fatalf("GET versions%s = %d: %s", query, rr.Code, rr.Body.String()) + } + var out []models.Version + parseJSON(t, rr, &out) + return out +} + +func TestItemVersions_LimitBoundsTheWindow(t *testing.T) { + srv := testServer(t) + ws := createTestWorkspaceViaAPI(t, srv) + item := seedVersionedItem(t, srv, ws, 5) + + // Armed: the unbounded response must be bigger than the window under + // test, or a limit that does nothing would still pass. + all := fetchVersions(t, srv, ws, item.Slug, "") + if len(all) < 4 { + t.Fatalf("fixture never armed: only %d versions recorded, need enough to truncate", len(all)) + } + + got := fetchVersions(t, srv, ws, item.Slug, "limit=2") + if len(got) != 2 { + t.Errorf("limit=2 returned %d versions, want 2", len(got)) + } + + // Newest-first, and the window is the NEWEST end — the only end that is + // cheap to reconstruct from reverse patches. + if len(got) == 2 && len(all) >= 2 { + if got[0].ID != all[0].ID || got[1].ID != all[1].ID { + t.Errorf("limit window = [%s %s], want the newest two [%s %s]", + got[0].ID, got[1].ID, all[0].ID, all[1].ID) + } + } +} + +// Absent limit stays UNBOUNDED, matching the item-list endpoints. The default +// belongs on the clients, where a token budget is actually known; a server +// that truncates a request nobody bounded is a silent-truncation trap for +// third-party API consumers. +func TestItemVersions_AbsentLimitIsUnbounded(t *testing.T) { + srv := testServer(t) + ws := createTestWorkspaceViaAPI(t, srv) + + // MORE than the default any client applies (50). A fixture below that + // number cannot tell an unbounded server from one that quietly defaults + // to 50 — which is exactly the behaviour this test denies, so the row + // count is load-bearing rather than incidental (codex round 2). + const seeded = 60 + item := seedVersionedItem(t, srv, ws, seeded) + + all := fetchVersions(t, srv, ws, item.Slug, "") + if len(all) <= 50 { + t.Errorf("unbounded request returned %d versions after seeding %d; the "+ + "server must not apply a default cap", len(all), seeded) + } +} + +func TestItemVersions_OversizedLimitIsClamped(t *testing.T) { + srv := testServer(t) + ws := createTestWorkspaceViaAPI(t, srv) + item := seedVersionedItem(t, srv, ws, 8) + + // An oversized ask must be treated as the ceiling, not rejected and not + // honoured. Seeding past the clamp to prove the ceiling BINDS would mean + // 500+ versions per run, so this asserts the reachable half — the request + // succeeds and returns what exists — and the clamp arithmetic itself is + // asserted directly below, where it is cheap and exact. + got := fetchVersions(t, srv, ws, item.Slug, + "limit="+strconv.Itoa(maxItemVersionsQueryLimit*10)) + if len(got) == 0 { + t.Error("oversized limit returned nothing; it should clamp, not reject") + } + + all := fetchVersions(t, srv, ws, item.Slug, "") + if len(got) != len(all) { + t.Errorf("oversized limit returned %d of %d existing versions; a clamp is "+ + "a ceiling on the ASK, not a truncation of the answer", len(got), len(all)) + } +} + +// The clamp, asserted against the REAL function rather than by re-implementing +// its arithmetic in the test (which would prove only that the test can +// multiply) or by seeding 500+ rows per run. Pairs with the request-level test +// above: that one proves an oversized ask is accepted, this one proves the +// number it is accepted AS — and covers the inputs a URL can actually carry. +func TestItemVersions_ClampArithmetic(t *testing.T) { + for _, tc := range []struct { + raw string + want int + }{ + {raw: "", want: 0}, // absent -> unbounded + {raw: "0", want: 0}, // explicit zero -> unbounded + {raw: "-5", want: 0}, // negative -> unbounded, not an error + {raw: "banana", want: 0}, // unparseable -> unbounded, not a 500 + {raw: "1", want: 1}, + {raw: strconv.Itoa(maxItemVersionsQueryLimit - 1), want: maxItemVersionsQueryLimit - 1}, + {raw: strconv.Itoa(maxItemVersionsQueryLimit), want: maxItemVersionsQueryLimit}, + {raw: strconv.Itoa(maxItemVersionsQueryLimit + 1), want: maxItemVersionsQueryLimit}, + {raw: strconv.Itoa(maxItemVersionsQueryLimit * 100), want: maxItemVersionsQueryLimit}, + // Past MaxInt64. Atoi reports ErrRange AND returns the saturated + // bound; treating that as unparseable would return 0 — unbounded — + // letting an absurd number defeat the ceiling entirely. + {raw: "9223372036854775808", want: maxItemVersionsQueryLimit}, + {raw: "99999999999999999999999999", want: maxItemVersionsQueryLimit}, + // Range-NEGATIVE stays unbounded, same as a plain negative. + {raw: "-9223372036854775809", want: 0}, + } { + if got := parseItemVersionsLimit(tc.raw); got != tc.want { + t.Errorf("parseItemVersionsLimit(%q) = %d, want %d", tc.raw, got, tc.want) + } + } +} + +// The summary property. This covers the SHAPE — same rows, no content, no +// stale is_diff — and deliberately does not claim more than that. +// +// WHAT THIS CANNOT SEE, stated because a reader would otherwise assume it +// does: an implementation that resolved every version and THEN blanked the +// fields would pass every assertion here, because the response is byte-identical +// either way. Verified by mutation — pointing the summary branch at +// ListItemVersionsResolvedPage leaves this file green (codex round 2). +// +// So the "no walk happened" half rests on two things instead: the handler's +// summary branch calls ListItemVersionsPage, which is one line and reviewable, +// and TestListItemVersionsPage_ReturnsUnresolvedRows in internal/store proves +// that reader genuinely returns unresolved rows rather than quietly resolving +// them. An end-to-end assertion would need a patch-application counter in the +// production path; the cost of being wrong here is performance, not +// correctness, so that instrument is not built. +func TestItemVersions_SummaryOmitsContentButKeepsMetadata(t *testing.T) { + srv := testServer(t) + ws := createTestWorkspaceViaAPI(t, srv) + item := seedVersionedItem(t, srv, ws, 3) + + full := fetchVersions(t, srv, ws, item.Slug, "") + summary := fetchVersions(t, srv, ws, item.Slug, "summary=true") + + if len(summary) != len(full) { + t.Fatalf("summary returned %d versions, full returned %d — summary must "+ + "change the SHAPE, not the row set", len(summary), len(full)) + } + + // Armed: the full response actually carries bodies, or "summary has no + // content" is trivially true of both. + var fullHasContent bool + for _, v := range full { + if v.Content != "" { + fullHasContent = true + break + } + } + if !fullHasContent { + t.Fatal("fixture never armed: the unbounded response carried no content, " + + "so the summary assertion below proves nothing") + } + + for i, v := range summary { + if v.Content != "" { + t.Errorf("summary version %d carried content (%d bytes)", i, len(v.Content)) + } + // IsDiff must be cleared with it: an empty body still claiming to be a + // reverse patch tells a consumer to resolve something that is absent. + if v.IsDiff { + t.Errorf("summary version %d still claims is_diff with no body to patch", i) + } + // Metadata is the whole point of the mode — it has to survive. + if v.ID == "" || v.CreatedAt.IsZero() || v.CreatedBy == "" { + t.Errorf("summary version %d lost metadata: %+v", i, v) + } + } +} + +// summary and limit compose: the bound applies to the metadata-only path too, +// which is the combination every agent call actually uses. +func TestItemVersions_SummaryRespectsLimit(t *testing.T) { + srv := testServer(t) + ws := createTestWorkspaceViaAPI(t, srv) + item := seedVersionedItem(t, srv, ws, 5) + + got := fetchVersions(t, srv, ws, item.Slug, "summary=true&limit=2") + if len(got) != 2 { + t.Errorf("summary+limit=2 returned %d versions, want 2", len(got)) + } + for i, v := range got { + if v.Content != "" { + t.Errorf("summary+limit version %d carried content", i) + } + } +} + +// The restore path must keep resolving the WHOLE chain. A version is +// reconstructed by walking back from current content, so bounding that walk +// would make older versions unrestorable — the one place the limit must not +// reach (BUG-1612's expand path has the same requirement). +func TestItemVersions_RestoreStillReachesOldVersions(t *testing.T) { + srv := testServer(t) + ws := createTestWorkspaceViaAPI(t, srv) + item := seedVersionedItem(t, srv, ws, 6) + + all := fetchVersions(t, srv, ws, item.Slug, "") + if len(all) < 5 { + t.Fatalf("fixture never armed: %d versions", len(all)) + } + // The OLDEST recorded version — past any default window a client applies. + oldest := all[len(all)-1] + + rr := doRequest(srv, "POST", + "/api/v1/workspaces/"+ws+"/items/"+item.Slug+"/versions/"+oldest.ID+"/restore", nil) + if rr.Code != http.StatusOK { + t.Fatalf("restore oldest version = %d: %s — bounding the resolve walk "+ + "would strand exactly these", rr.Code, rr.Body.String()) + } +} diff --git a/internal/store/items.go b/internal/store/items.go index d68b094b..3d3d3a8e 100644 --- a/internal/store/items.go +++ b/internal/store/items.go @@ -4618,8 +4618,27 @@ func (s *Store) shouldCreateItemVersion(itemID, actor, source string) (bool, err // ListItemVersionsResolved returns versions with full content (diffs resolved). // Requires the current item content to reconstruct diff-based versions. +// +// Unbounded: every version is read and every reverse patch applied. Callers +// that only need the newest N should use ListItemVersionsResolvedPage, which +// bounds BOTH the read and the patch walk (BUG-2608). This form remains +// correct — and required — where an arbitrary version must be located, since +// the chain can only be walked from current content backwards. func (s *Store) ListItemVersionsResolved(itemID, currentContent string) ([]models.Version, error) { - versions, err := s.ListItemVersions(itemID) + return s.ListItemVersionsResolvedPage(itemID, currentContent, 0) +} + +// ListItemVersionsResolvedPage is ListItemVersionsResolved bounded to the +// newest `limit` versions (limit <= 0 means unbounded). +// +// The bound is cheap ONLY because it takes the newest N. Versions are stored +// as REVERSE patches, so reconstructing any version means starting from the +// item's current content and walking backwards through everything newer — a +// window at the newest end is exactly the prefix of that walk, while an older +// window would still require walking everything above it. That asymmetry is +// why this offers a limit and not an offset (BUG-2608). +func (s *Store) ListItemVersionsResolvedPage(itemID, currentContent string, limit int) ([]models.Version, error) { + versions, err := s.ListItemVersionsPage(itemID, limit) if err != nil { return nil, err } @@ -4720,12 +4739,25 @@ func (s *Store) ListItemVersionsBeforeTime(itemID string, before time.Time, befo // ListItemVersions returns all versions for an item. func (s *Store) ListItemVersions(itemID string) ([]models.Version, error) { - rows, err := s.db.Query(s.q(` + return s.ListItemVersionsPage(itemID, 0) +} + +// ListItemVersionsPage returns an item's versions newest-first, bounded to +// `limit` rows (limit <= 0 means unbounded). Raw rows — reverse-patch versions +// still carry patch text, not content; see ListItemVersionsResolvedPage. +func (s *Store) ListItemVersionsPage(itemID string, limit int) ([]models.Version, error) { + query := ` SELECT id, item_id, content, change_summary, created_by, source, is_diff, created_at FROM item_versions WHERE item_id = ? ORDER BY created_at DESC, version_seq DESC - `), itemID) + ` + args := []interface{}{itemID} + if limit > 0 { + query += " LIMIT ?" + args = append(args, limit) + } + rows, err := s.db.Query(s.q(query), args...) if err != nil { return nil, err } diff --git a/internal/store/items_versions_page_test.go b/internal/store/items_versions_page_test.go new file mode 100644 index 00000000..459df232 --- /dev/null +++ b/internal/store/items_versions_page_test.go @@ -0,0 +1,146 @@ +package store + +// BUG-2608 — the claim that summary mode SKIPS reverse-patch resolution is a +// claim about work not done, which a response-shape assertion cannot make: a +// handler that resolved everything and then blanked the fields would look +// identical from outside (codex round 2). +// +// This asserts it where it is observable — the paged raw reader returns rows +// still carrying patch text and is_diff, while the resolved reader returns +// reconstructed content. Summary mode calls the former, and that one-line +// reading is what carries the performance claim. + +import ( + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +func TestListItemVersionsPage_ReturnsUnresolvedRows(t *testing.T) { + s := testStore(t) + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Versions WS"}) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + coll, err := s.CreateCollection(ws.ID, models.CollectionCreate{ + Name: "Tasks", Slug: "tasks", Prefix: "TASK", + Schema: `{"fields":[{"key":"status","type":"select","options":["open","done"],"default":"open"}]}`, + }) + if err != nil { + t.Fatalf("CreateCollection: %v", err) + } + + // A body large enough that a patch really is smaller than the whole + // content — otherwise the store keeps full copies, nothing is a diff, and + // every assertion below is vacuous. + base := strings.Repeat("a line of body text that makes the patch worth storing\n", 200) + item, err := s.CreateItem(ws.ID, coll.ID, models.ItemCreate{Title: "versioned", Content: base + "v0\n"}) + if err != nil { + t.Fatalf("CreateItem: %v", err) + } + sources := []string{"web", "cli", "mcp", "skill"} + for i := 1; i <= 4; i++ { + content := base + "v" + string(rune('0'+i)) + "\n" + if _, err := s.UpdateItem(item.ID, models.ItemUpdate{ + Content: &content, + Source: sources[i%len(sources)], + }); err != nil { + t.Fatalf("edit %d: %v", i, err) + } + } + + fresh, err := s.GetItem(item.ID) + if err != nil || fresh == nil { + t.Fatalf("reload item: %v", err) + } + + raw, err := s.ListItemVersionsPage(item.ID, 0) + if err != nil { + t.Fatalf("ListItemVersionsPage: %v", err) + } + resolved, err := s.ListItemVersionsResolvedPage(item.ID, fresh.Content, 0) + if err != nil { + t.Fatalf("ListItemVersionsResolvedPage: %v", err) + } + if len(raw) != len(resolved) || len(raw) == 0 { + t.Fatalf("raw=%d resolved=%d rows; need the same non-empty set", len(raw), len(resolved)) + } + + var rawDiffs int + for _, v := range raw { + if v.IsDiff { + rawDiffs++ + } + } + if rawDiffs == 0 { + t.Fatal("fixture never armed: the store recorded no reverse-patch versions, " + + "so 'raw rows are unresolved' is trivially true of full content too") + } + + // The resolved reader must have DONE the work the raw one skips. + for i, v := range resolved { + if v.IsDiff { + t.Errorf("resolved version %d still marked is_diff — it was not resolved", i) + } + } + // And the raw reader must NOT have: at least one row differs from its + // resolved counterpart, which can only be true if no patch was applied. + var differ int + for i := range raw { + if raw[i].ID == resolved[i].ID && raw[i].Content != resolved[i].Content { + differ++ + } + } + if differ == 0 { + t.Error("every raw row already equalled its resolved content — the paged " + + "reader is resolving patches, which is the work summary mode exists " + + "to skip") + } +} + +func TestListItemVersionsPage_LimitTakesTheNewest(t *testing.T) { + s := testStore(t) + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Versions WS 2"}) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + coll, err := s.CreateCollection(ws.ID, models.CollectionCreate{ + Name: "Tasks", Slug: "tasks", Prefix: "TASK", + Schema: `{"fields":[{"key":"status","type":"select","options":["open","done"],"default":"open"}]}`, + }) + if err != nil { + t.Fatalf("CreateCollection: %v", err) + } + item, err := s.CreateItem(ws.ID, coll.ID, models.ItemCreate{Title: "versioned", Content: "v0\n"}) + if err != nil { + t.Fatalf("CreateItem: %v", err) + } + sources := []string{"web", "cli", "mcp", "skill"} + for i := 1; i <= 6; i++ { + content := "v" + string(rune('0'+i)) + "\n" + if _, err := s.UpdateItem(item.ID, models.ItemUpdate{Content: &content, Source: sources[i%len(sources)]}); err != nil { + t.Fatalf("edit %d: %v", i, err) + } + } + + all, err := s.ListItemVersionsPage(item.ID, 0) + if err != nil { + t.Fatalf("unbounded: %v", err) + } + if len(all) < 4 { + t.Fatalf("fixture never armed: %d versions", len(all)) + } + got, err := s.ListItemVersionsPage(item.ID, 2) + if err != nil { + t.Fatalf("limited: %v", err) + } + if len(got) != 2 { + t.Fatalf("limit=2 returned %d rows", len(got)) + } + // The NEWEST two — the only window the reverse-patch chain makes cheap. + if got[0].ID != all[0].ID || got[1].ID != all[1].ID { + t.Errorf("limit window = [%s %s], want newest [%s %s]", + got[0].ID, got[1].ID, all[0].ID, all[1].ID) + } +}