From ea88d9a5e0accde035c99b10f1f4d6fed275d169 Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 26 Aug 2026 13:34:25 +0000 Subject: [PATCH 1/7] feat(server): refuse workspace creation without may_create_workspaces consent (IDEA-2756) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OAuth consent screen's "Let this app create new workspaces" checkbox gated only the post-creation auto-add. A connection whose user left it unticked could still create workspaces; it simply could not then see them. A permission that does not prevent the action it names is a consent mismatch. Dave ruled it: the checkbox is a permission on whether the connected token may CREATE, and it has to be true to what a user would honestly expect from the option. The behaviour-change-for-existing-connections argument loses to honest consent semantics. Adds Server.requireWorkspaceCreationConsent, a shared gate at the top of both endpoints that mint a workspace under the caller's account: POST /api/v1/workspaces handleCreateWorkspace POST /api/v1/workspaces/import handleImportWorkspace Import reaches CreateWorkspace via store.ImportWorkspace, so it is the same permission at a second door — lead-ruled as an application of the same rationale, not a new decision. The gate sits above the Content-Type dispatch, so it covers the tar.gz bundle path (whose only route is that handler) and refuses before the 64 MiB body read. Refusal is a 403, mirroring handleAuditLog's consent refusal (BUG-2102): a hard decline rather than a narrowed response, because there is no narrower version of creating a workspace. Three non-refusal cases and one refusal, all but the last with a test: - not an OAuth grant (PAT, CLI session, local stdio) — creation rides on ordinary account authority - ErrOAuthConnectionNotFound (pre-Phase-C grant) — ALLOW, matching the backfill's may_create_workspaces=ON default. Deliberately asymmetric with maybeAutoAddCreatorConnection's not-found branch, which declines a convenience where this one would invent a refusal - flag set — proceeds; the auto-add is unchanged - a store I/O error — REFUSED, failing closed with a 500, because allowing the create when the deciding state could not be read grants a declined permission on the strength of a database blip. This is the one branch with no test: injecting a store read failure needs a fault-injecting store the package does not have, so it is reasoned rather than measured Population enumerated before the fix (CONVE-18): five CreateWorkspace call sites, two of them HTTP endpoints reachable by an OAuth token (both gated). Excluded with reasons: autoCreateWorkspace (signup-time, no connection in context), workspace restore (un-deletes an existing workspace), /oauth/claim (grants access, does not mint), cmd_db.go (local store copy, no HTTP). Search boundary: the sweep traced Store.CreateWorkspace callers and did not look for a path that inserts a workspace by raw SQL. Ten tests, all driving the real router rather than calling handlers directly (CONVE-19). Every refusal leg asserts that no workspace of that name exists afterwards, not merely the status code (CONVE-12) — a guard that 403s after the write passes a status-only assertion. Seven mutants, seven detected, including both guard-placement mutations. MCP tool surface 0.25 -> 0.26. Behaviour bump on the v0.9/v0.16/v0.25 grounds: no tool name, action enum or param shape changed, but pad_workspace.create now refuses a call it used to permit. Closest precedent is v0.10; unlike v0.10 there is deliberately no escape-hatch param, because the gate encodes a decision the USER made at consent time and a bypass flag would be the app overriding its own grant. CONVE-23 sweep for prose the change falsified: instructions.md told agents the create still succeeds and to use the claim flow (it would have sent them to claim something that was never created); the TASK-2753 allow-list guard entry asserted the same and posed IDEA-2756 as open; the MCP catalog and CLI help described only the flag=true path; maybeAutoAddCreatorConnection's flag-off branch is now unreachable from its sole caller and is documented as dead code kept for contract, to be deleted only with the guard. CLAUDE.md was already stale at v0.24 (v0.25 bumped the constant without it) — brought to v0.26 with a backfilled v0.25 line. The consent screen and console copy are unchanged: they were the misleading half of this bug, and the fix makes them true. --- CLAUDE.md | 6 +- README.md | 4 +- cmd/pad/cmd_workspace.go | 5 +- internal/mcp/catalog_workspace.go | 4 + .../mcp/dispatch_http_allowlist_guard_test.go | 7 +- internal/mcp/dispatch_http_routes.go | 4 +- internal/mcp/instructions.md | 4 +- internal/mcp/version.go | 33 +- .../handlers_workspace_create_consent_test.go | 396 ++++++++++++++++++ internal/server/handlers_workspaces.go | 99 ++++- 10 files changed, 547 insertions(+), 15 deletions(-) create mode 100644 internal/server/handlers_workspace_create_consent_test.go diff --git a/CLAUDE.md b/CLAUDE.md index a2610cfc9..1dc181cce 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -249,7 +249,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.24) 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.24 (#1066) makes the `pad_item` `fields` OBJECT a real write form on create/update — reads return `fields` as a native object (BUG-991 normalization), and writing that shape back was a silent no-op: not a declared param, no `additionalProperties`, so it was accepted, never mapped by `BuildCLIArgs`, and dropped while the PATCH still bumped `updated_at`. The alias merges into the same path as `field: ["key=value"]` / the dedicated params (`catalog_item_fields.go`), refusing the same key in two places with conflicting values; and input validation is now STRICT across all catalog tools — an undeclared top-level key fails with a structured `validation_failed` naming it, instead of being silently dropped (a small documented compat list survives: pad_item's v0.16 `assigned_user_id` / `agent_role_id` remote clear form). One bump covers both halves — they are one contract change. v0.23 (BUG-2627 part 2 + BUG-2675, PR #1166) refuses raw `field` setters naming system-metadata keys in `fields_patch` on every transport (`github_pr` exempt on UPDATE only — the sole remote writer, itself broken: BUG-2696) and adds the retry-hostile `stored_state_unreadable` error code. v0.22 (BUG-2674, PR #1165) makes reserved metadata survive a move and refuses `field` setters naming those keys on move/copy — see `internal/mcp/version.go` for both full entries. 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. +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.26) 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.26 (IDEA-2756) makes `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant carries `may_create_workspaces=false` — that consent checkbox previously gated only the post-creation auto-add, so a connection whose user declined it created workspaces it could then not see. The same gate covers `POST /workspaces/import` (a second door onto `store.ImportWorkspace` → `CreateWorkspace`, with no MCP action today). No escape-hatch param, deliberately: the gate expresses the USER's consent decision, so only re-authorization lifts it. v0.25 (TASK-2657 / BUG-2702) makes `pad_library.activate` resolve its destination collection from the target's declared artifact kind rather than the literal `conventions` / `playbooks` slugs, and surfaces a lookup ERROR instead of falling back. v0.24 (#1066) makes the `pad_item` `fields` OBJECT a real write form on create/update — reads return `fields` as a native object (BUG-991 normalization), and writing that shape back was a silent no-op: not a declared param, no `additionalProperties`, so it was accepted, never mapped by `BuildCLIArgs`, and dropped while the PATCH still bumped `updated_at`. The alias merges into the same path as `field: ["key=value"]` / the dedicated params (`catalog_item_fields.go`), refusing the same key in two places with conflicting values; and input validation is now STRICT across all catalog tools — an undeclared top-level key fails with a structured `validation_failed` naming it, instead of being silently dropped (a small documented compat list survives: pad_item's v0.16 `assigned_user_id` / `agent_role_id` remote clear form). One bump covers both halves — they are one contract change. v0.23 (BUG-2627 part 2 + BUG-2675, PR #1166) refuses raw `field` setters naming system-metadata keys in `fields_patch` on every transport (`github_pr` exempt on UPDATE only — the sole remote writer, itself broken: BUG-2696) and adds the retry-hostile `stored_state_unreadable` error code. v0.22 (BUG-2674, PR #1165) makes reserved metadata survive a move and refuses `field` setters naming those keys on move/copy — see `internal/mcp/version.go` for both full entries. 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. @@ -263,7 +263,7 @@ pad mcp status # Install state across supported clients ``` Surface: -- **Tools:** the v0.24 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.26 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`. @@ -271,7 +271,7 @@ 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.24"`) — the MCP tool catalog contract. Bump when tool names, action enums, or parameter shapes change incompatibly. **v0.24** (#1066) adds the `fields` OBJECT param to `pad_item` create/update — an alias merging into the same path as `field`/the dedicated params, so the shape reads return is finally a valid write shape; the same key supplied twice with conflicting values is REFUSED (refuse-on-ambiguity, the v0.18/v0.19 disposition), equal duplicates collapse to one write, and non-writer actions refuse a `fields` param loudly. It also makes input validation STRICT for every catalog tool: undeclared top-level keys are rejected with a structured error naming them, instead of being accepted and silently dropped by `BuildCLIArgs` — which is the mechanism that made the `fields` object a session-scoped silent no-op in the first place. Compat carve-out: `pad_item`'s v0.16 `assigned_user_id` / `agent_role_id` remote-transport clear form stays accepted (documented, undeprecated, deliberately never schema-declared). The strict half changes behaviour for inputs that previously "succeeded", but that reliance was indistinguishable from a caller bug (the key never did anything), so the break is the fix; one bump covers both halves. **v0.23** (BUG-2627 part 2 + BUG-2675) refuses system-metadata keys through `fields_patch` on all three doors at once, `github_pr` exempt on update (move/copy still refuse it), and adds the retry-hostile `stored_state_unreadable` code. **v0.22** (BUG-2674) stops `pad_item.action=move` destroying system metadata and refuses `field` setters naming the reserved keys there. **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). +- `ToolSurfaceVersion` (currently `"0.26"`) — the MCP tool catalog contract. Bump when tool names, action enums, or parameter shapes change incompatibly. **v0.26** (IDEA-2756) is a BEHAVIOR bump on the v0.9/v0.16/v0.25 grounds — no tool name, action enum, or param shape changed, but `pad_workspace.create` now refuses a call it used to permit. Closest precedent is v0.10, which likewise turned a server-side gate into a structured refusal; unlike v0.10 there is no `allow_draft`-style override, because the gate encodes a decision the USER made at consent time and a bypass param would be the app overriding its own grant. `POST /workspaces/import` is gated by the same shared helper (import mints a workspace through `store.ImportWorkspace`), though it has no MCP action today. **v0.25** (TASK-2657 / BUG-2702) resolves `pad_library.activate`'s destination collection from the target's declared artifact kind rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly; a lookup ERROR is surfaced rather than silently falling back. **v0.24** (#1066) adds the `fields` OBJECT param to `pad_item` create/update — an alias merging into the same path as `field`/the dedicated params, so the shape reads return is finally a valid write shape; the same key supplied twice with conflicting values is REFUSED (refuse-on-ambiguity, the v0.18/v0.19 disposition), equal duplicates collapse to one write, and non-writer actions refuse a `fields` param loudly. It also makes input validation STRICT for every catalog tool: undeclared top-level keys are rejected with a structured error naming them, instead of being accepted and silently dropped by `BuildCLIArgs` — which is the mechanism that made the `fields` object a session-scoped silent no-op in the first place. Compat carve-out: `pad_item`'s v0.16 `assigned_user_id` / `agent_role_id` remote-transport clear form stays accepted (documented, undeprecated, deliberately never schema-declared). The strict half changes behaviour for inputs that previously "succeeded", but that reliance was indistinguishable from a caller bug (the key never did anything), so the break is the fix; one bump covers both halves. **v0.23** (BUG-2627 part 2 + BUG-2675) refuses system-metadata keys through `fields_patch` on all three doors at once, `github_pr` exempt on update (move/copy still refuse it), and adds the retry-hostile `stored_state_unreadable` code. **v0.22** (BUG-2674) stops `pad_item.action=move` destroying system metadata and refuses `field` setters naming the reserved keys there. **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`. diff --git a/README.md b/README.md index 71d033227..c9b5d3238 100644 --- a/README.md +++ b/README.md @@ -388,7 +388,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.25)** — ten resource × action tools plus `pad_set_workspace` (eleven total), no flat verb explosion. Undeclared input keys are rejected with a structured error rather than silently dropped. `pad_item` create/update accept field values as a `fields` object (the same shape reads return) as an equivalent to the dedicated params / `field: ["key=value"]`. `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.26)** — ten resource × action tools plus `pad_set_workspace` (eleven total), no flat verb explosion. Undeclared input keys are rejected with a structured error rather than silently dropped. `pad_item` create/update accept field values as a `fields` object (the same shape reads return) as an equivalent to the dedicated params / `field: ["key=value"]`. `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 | |---|---| @@ -417,7 +417,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.25"` — 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); v0.22 stopped `pad_item.move` destroying an item’s system metadata — implementation notes, decision log, linked PR and convention data now survive a move, any field the destination schema has no home for is REPORTED in the move’s activity entry rather than vanishing, and a `field` setter naming one of those reserved keys is refused with `malformed_override` instead of writing it (BUG-2674); v0.23 closed the same door on the ordinary update — a `field` setter naming `implementation_notes`, `decision_log` or `convention` is now refused on every transport at once (`validation_error` on HTTP, surfaced to MCP clients as `validation_failed`); the one gate covers the CLI, remote MCP and stdio MCP at once because all three lower a `field` setter into the same `fields_patch`; `github_pr` is deliberately exempt ON UPDATE (move and copy still refuse it), since `pad github link` cannot run on remote MCP and refusing it would leave those agents with no door at all (that door is itself broken — BUG-2696); item CREATE stays open, deliberately, because its full-`fields` payload is shared with Pad’s own writers. v0.23 also added the retry-hostile `stored_state_unreadable` error code so an agent told its target item’s stored data is unreadable stops instead of retrying a permanent failure (BUG-2627 / BUG-2675); v0.24 made the `pad_item` `fields` object a real write form on create/update — reads return `fields` as a native object, and writing that shape back was a silent no-op (accepted, never mapped, dropped while the PATCH still bumped `updated_at`) — merging it into the same path as `field`/the dedicated params with conflicting duplicate keys refused, and made input validation strict across all catalog tools: undeclared top-level keys now fail with a structured error instead of being silently dropped (#1066); v0.25 made `pad_library.activate` resolve its DESTINATION collection from the target’s declared artifact kind (SPEC-5 collection traits) rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly instead of failing not-found with the collection sitting right there (BUG-2702); a lookup ERROR is now surfaced rather than silently falling back to the canonical slug, because falling back on an error means writing to a slug nothing was confirmed about (TASK-2657); see `internal/mcp/version.go` for the full changelog) +- `tool_surface_version: "0.26"` — 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); v0.22 stopped `pad_item.move` destroying an item’s system metadata — implementation notes, decision log, linked PR and convention data now survive a move, any field the destination schema has no home for is REPORTED in the move’s activity entry rather than vanishing, and a `field` setter naming one of those reserved keys is refused with `malformed_override` instead of writing it (BUG-2674); v0.23 closed the same door on the ordinary update — a `field` setter naming `implementation_notes`, `decision_log` or `convention` is now refused on every transport at once (`validation_error` on HTTP, surfaced to MCP clients as `validation_failed`); the one gate covers the CLI, remote MCP and stdio MCP at once because all three lower a `field` setter into the same `fields_patch`; `github_pr` is deliberately exempt ON UPDATE (move and copy still refuse it), since `pad github link` cannot run on remote MCP and refusing it would leave those agents with no door at all (that door is itself broken — BUG-2696); item CREATE stays open, deliberately, because its full-`fields` payload is shared with Pad’s own writers. v0.23 also added the retry-hostile `stored_state_unreadable` error code so an agent told its target item’s stored data is unreadable stops instead of retrying a permanent failure (BUG-2627 / BUG-2675); v0.24 made the `pad_item` `fields` object a real write form on create/update — reads return `fields` as a native object, and writing that shape back was a silent no-op (accepted, never mapped, dropped while the PATCH still bumped `updated_at`) — merging it into the same path as `field`/the dedicated params with conflicting duplicate keys refused, and made input validation strict across all catalog tools: undeclared top-level keys now fail with a structured error instead of being silently dropped (#1066); v0.25 made `pad_library.activate` resolve its DESTINATION collection from the target’s declared artifact kind (SPEC-5 collection traits) rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly instead of failing not-found with the collection sitting right there (BUG-2702); a lookup ERROR is now surfaced rather than silently falling back to the canonical slug, because falling back on an error means writing to a slug nothing was confirmed about (TASK-2657); v0.26 made `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant has `may_create_workspaces=false` — that checkbox previously gated only the post-creation auto-add, so a connection whose user declined it could still create workspaces it could then not see; the same gate covers `POST /workspaces/import`, which mints a workspace through a second door. There is deliberately no escape-hatch parameter: the gate expresses the USER's consent decision, so only re-authorization lifts it (IDEA-2756); 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_workspace.go b/cmd/pad/cmd_workspace.go index b0cfece53..346de3eab 100644 --- a/cmd/pad/cmd_workspace.go +++ b/cmd/pad/cmd_workspace.go @@ -356,7 +356,10 @@ that just want the workspace row. When called over an OAuth-bound MCP session whose grant has may_create_workspaces=true, the new workspace is auto-added to that connection's allow-list (PLAN-1519 / TASK-1521 / IDEA-1517 §1) so -the agent can use it immediately without re-auth.`, +the agent can use it immediately without re-auth. When that grant has +may_create_workspaces=false the create is REFUSED with a 403 and no +workspace is made (IDEA-2756) — the user must re-authorize the +connection with workspace creation enabled.`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { client, _ := getClient() diff --git a/internal/mcp/catalog_workspace.go b/internal/mcp/catalog_workspace.go index 6a5ba49c0..8aa38ecaa 100644 --- a/internal/mcp/catalog_workspace.go +++ b/internal/mcp/catalog_workspace.go @@ -153,6 +153,10 @@ Actions: When called over an OAuth-bound MCP session whose grant has may_create_workspaces=true, the new workspace is auto-added to that connection's allow-list — usable immediately, no re-auth. + When that grant has may_create_workspaces=FALSE the call is + REFUSED with a 403 and no workspace is created (IDEA-2756); + retrying will not help — the user must re-authorize the + connection with workspace creation enabled. claim — Redeem a 6-digit claim code to add a workspace to the calling OAuth connection's allow-list. Required: workspace, code. diff --git a/internal/mcp/dispatch_http_allowlist_guard_test.go b/internal/mcp/dispatch_http_allowlist_guard_test.go index cb639b359..22fcd994e 100644 --- a/internal/mcp/dispatch_http_allowlist_guard_test.go +++ b/internal/mcp/dispatch_http_allowlist_guard_test.go @@ -190,9 +190,10 @@ var allowlistCoverage = map[string]allowlistClassification{ "data the allow-list protects is reachable through it. The consent model already has a dedicated capability " + "for this — the connection's may_create_workspaces checkbox — and when it is set, " + "maybeAutoAddCreatorConnection adds the new workspace to the allow-list (added_by='agent-create', " + - "PLAN-1519/TASK-1521) so the agent can use what it just made. NOTE the flag gates only that AUTO-ADD, not " + - "the creation itself; with it unset the create still succeeds and the workspace simply never joins the " + - "allow-list. Whether that should instead REFUSE is IDEA-2756 (codex round 2 corrected this entry's original " + + "PLAN-1519/TASK-1521) so the agent can use what it just made. Since IDEA-2756 that flag also DECIDES the " + + "call: with may_create_workspaces unset the create is refused outright with a 403 and no workspace is " + + "made — it no longer gates the auto-add alone. The classification is unchanged either way, because " + + "refusing to create is still not a disclosure question (codex round 2 corrected this entry's original " + "reasoning, which wrongly claimed no capability existed)"}, // --- specialRoutes: observed via the recording handler --- diff --git a/internal/mcp/dispatch_http_routes.go b/internal/mcp/dispatch_http_routes.go index 448720a8f..c9b5f02c6 100644 --- a/internal/mcp/dispatch_http_routes.go +++ b/internal/mcp/dispatch_http_routes.go @@ -429,7 +429,9 @@ func init() { // over MCP. `create` POSTs to /api/v1/workspaces; the handler's // auto-add side effect uses the request context's OAuth identity // (WithMCPTokenIdentity) to insert into oauth_connection_workspaces - // when may_create_workspaces=true. `claim` POSTs to + // when may_create_workspaces=true. That same identity now also + // DECIDES the call: with may_create_workspaces=false the handler + // 403s before creating anything (IDEA-2756). `claim` POSTs to // /api/v1/oauth/claim with the same payload shape. "workspace create": mapWorkspaceCreate, "workspace claim": mapWorkspaceClaim, diff --git a/internal/mcp/instructions.md b/internal/mcp/instructions.md index 4a46c7f10..623c83949 100644 --- a/internal/mcp/instructions.md +++ b/internal/mcp/instructions.md @@ -6,7 +6,7 @@ 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.25) +## Tool surface (v0.26) Ten resource × action tools, plus `pad_set_workspace` (which takes a `workspace` slug only — no action enum). Eleven tools total. @@ -87,7 +87,7 @@ Filter by trigger (`always`, `on-implement`, `on-task-complete`, etc.) when rele If the user references a workspace this connection can't see (you'll get a 403 from workspace tools, or the workspace won't appear in `pad_workspace.list`), tell the user you can't see that workspace with your current permissions, then walk them through how to grant access: open Pad in their browser → switch to that workspace → avatar menu → "Connect project..." A 6-digit claim code will appear. Have them paste it back in chat, then call `pad_workspace.claim` with `{workspace: "", code: "<6 digits>"}`. The workspace joins this connection's allow-list and stays until the user revokes it via `/console/connected-apps`. No re-auth required. -For brand-new workspaces, `pad_workspace.create` with `{name: ""}` (and optional `template`) creates the workspace AND auto-adds it to this connection's allow-list in one call — no claim code needed. Only works when the user granted "may create workspaces" at consent time; if that scope was declined the create call still succeeds but the workspace doesn't auto-join — direct the user to the claim flow above to bring it in. +For brand-new workspaces, `pad_workspace.create` with `{name: ""}` (and optional `template`) creates the workspace AND auto-adds it to this connection's allow-list in one call — no claim code needed. Only works when the user granted "may create workspaces" at consent time. If that scope was declined, the create is **refused with a 403 and no workspace is made** — retrying won't help and neither will the claim flow, since there is nothing to claim. Tell the user this connection isn't permitted to create workspaces, and offer them the two ways forward: create the workspace themselves in the browser and grant access with a claim code, or re-authorize this connection with "Let this app create new workspaces" enabled (`/console/connected-apps` can also flip it on an existing connection). ## New workspace: offer to set it up diff --git a/internal/mcp/version.go b/internal/mcp/version.go index 3e235609d..628fdd810 100644 --- a/internal/mcp/version.go +++ b/internal/mcp/version.go @@ -620,7 +620,36 @@ const CmdhelpVersion = "0.1" // condition would stop matching. That client was retrying a // permanent failure. -// - "0.25" — current. TASK-2657: `pad_library.activate` resolves its +// - "0.26" — current. IDEA-2756: `pad_workspace.action=create` is now +// REFUSED with a 403 when the calling OAuth connection's grant has +// `may_create_workspaces=false`. Previously that flag gated only the +// post-creation auto-add, so the create succeeded and the agent was +// handed a workspace it could not then see. +// +// BEHAVIOR bump, not a shape one — no tool name, action enum, or +// parameter changed. Same grounds as v0.25 (activation destination), +// v0.16 (empty-string clear) and v0.10, which is the closest +// precedent: a server-side gate that starts refusing a call it used +// to permit, with the refusal surfaced as a structured error. +// +// Unlike v0.10 there is no `allow_draft`-style escape hatch, and +// deliberately so: the gate expresses a decision the USER made on +// the consent screen, so a parameter that let the caller bypass it +// would be the app overriding its own grant. The remedy is +// re-authorization, which only the user can perform — the refusal +// message says so, and instructions.md tells the agent not to retry +// and not to reach for the claim flow (there is nothing to claim +// when nothing was created). +// +// Ruled by Dave on IDEA-2756: the consent checkbox is a permission +// on whether the connected token may CREATE, and has to be true to +// what a user would honestly expect from the option. Applied to +// `POST /workspaces/import` as well as `POST /workspaces` — import +// mints a workspace through store.ImportWorkspace, so it is the +// same permission at a second door — though import has no MCP +// action today and so is invisible from this surface. +// +// - "0.25" — TASK-2657: `pad_library.activate` resolves its // DESTINATION collection from the target's declared artifact kind // (SPEC-5 collection traits) rather than from the literal slugs // "conventions" / "playbooks". @@ -678,7 +707,7 @@ const CmdhelpVersion = "0.1" // That reliance was indistinguishable from a bug in the caller // (the key never did anything), so the break is the fix. Single // bump covers both halves; they are one contract change. -const ToolSurfaceVersion = "0.25" +const ToolSurfaceVersion = "0.26" // 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_workspace_create_consent_test.go b/internal/server/handlers_workspace_create_consent_test.go new file mode 100644 index 000000000..1fd4005dd --- /dev/null +++ b/internal/server/handlers_workspace_create_consent_test.go @@ -0,0 +1,396 @@ +package server + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" +) + +// Consent gate on workspace CREATION (IDEA-2756, Dave-ruled 2026-08-26). +// +// The OAuth consent screen's `may_create_workspaces` checkbox used to gate +// only maybeAutoAddCreatorConnection's allow-list insert: a connection whose +// user left the box unticked could still create workspaces, it just could not +// then see them. The ruling: the checkbox is a permission on whether the +// connected token may CREATE, so an unset flag refuses outright with a 403, +// mirroring handleAuditLog's consent refusal (BUG-2102). +// +// Two doors mint a workspace under the caller's account, both guarded here by +// the shared requireWorkspaceCreationConsent helper: +// +// - POST /api/v1/workspaces — handleCreateWorkspace +// - POST /api/v1/workspaces/import — handleImportWorkspace, which reaches +// CreateWorkspace via store.ImportWorkspace, and whose gzip Content-Type +// branch dispatches to handleImportWorkspaceBundle from inside itself. +// +// Every refusal leg asserts the WRONG behaviour's observable consequence — that +// no workspace by that slug exists afterwards — and not merely the status code +// (CONVE-12). A guard that 403s AFTER store.CreateWorkspace would pass a +// status-only assertion while leaving the workspace behind, which is the exact +// failure this endpoint's users would report. +// +// Everything drives the real router via srv.ServeHTTP rather than calling the +// handler directly, so the tests have an opinion about the ROUTE and not only +// about the function (CONVE-19). + +// consentEnv is a server with one authenticated user, a PAT for it, and an +// OAuth connection row whose may_create_workspaces flag the test chooses. +type consentEnv struct { + srv *Server + user *models.User + pat string + requestID string +} + +func newConsentEnv(t *testing.T, mayCreate bool) *consentEnv { + t.Helper() + srv := testServer(t) + + user, err := srv.store.CreateUser(models.UserCreate{ + Email: "consent-test@example.com", Name: "Consent Tester", Password: "pw-consent-12345", + }) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + // The PAT needs a workspace to bind to, and the handler under test is the + // one that makes workspaces — so seed this one through the store directly. + // It is the token's home, never the workspace any test asserts about. + home, err := srv.store.CreateWorkspace(models.WorkspaceCreate{ + Name: "Consent Home", OwnerID: user.ID, + }) + if err != nil { + t.Fatalf("CreateWorkspace(home): %v", err) + } + if err := srv.store.AddWorkspaceMember(home.ID, user.ID, "owner"); err != nil { + t.Fatalf("AddWorkspaceMember: %v", err) + } + tok, err := srv.store.CreateAPIToken(user.ID, models.APITokenCreate{ + Name: "consent-test-pat", WorkspaceID: home.ID, + }, 30, 0) + if err != nil { + t.Fatalf("CreateAPIToken: %v", err) + } + + requestID := "req-consent-" + user.ID + if err := srv.store.CreateOAuthConnection(store.OAuthConnection{ + RequestID: requestID, + UserID: user.ID, + Name: "Consent Test App", + MayCreateWorkspaces: mayCreate, + }); err != nil { + t.Fatalf("CreateOAuthConnection: %v", err) + } + + return &consentEnv{srv: srv, user: user, pat: tok.Token, requestID: requestID} +} + +// do issues a request with Bearer PAT auth. When requestID is non-empty the +// request context is decorated with an OAuth grant identity AFTER TokenAuth +// runs, which is what makes the caller look like an OAuth-bound MCP session. +// Mirrors handlers_oauth_claim_test.go's doClaim. +func (e *consentEnv) do(method, path, contentType string, body []byte, requestID string) *httptest.ResponseRecorder { + var req *http.Request + if body != nil { + req = httptest.NewRequest(method, path, bytes.NewReader(body)) + } else { + req = httptest.NewRequest(method, path, nil) + } + if contentType != "" { + req.Header.Set("Content-Type", contentType) + } + req.Header.Set("Authorization", "Bearer "+e.pat) + req.RemoteAddr = "192.0.2.1:1234" + + rr := httptest.NewRecorder() + if requestID == "" { + e.srv.ServeHTTP(rr, req) + return rr + } + wrap := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r = r.WithContext(WithMCPTokenIdentity(r.Context(), "oauth", requestID)) + e.srv.ServeHTTP(w, r) + }) + wrap.ServeHTTP(rr, req) + return rr +} + +func (e *consentEnv) createWorkspace(name, requestID string) *httptest.ResponseRecorder { + body, _ := json.Marshal(map[string]any{"name": name}) + return e.do("POST", "/api/v1/workspaces", "application/json", body, requestID) +} + +// lookupByName finds a workspace by its NAME rather than its slug. Name is +// what the request supplies verbatim; slug is derived, and the derivation +// differs between the create path (normalizeWorkspaceInput) and the import +// path (which takes the ?name= override as the slug and lets CreateWorkspace +// normalize it). Asserting on the name keeps these tests about the guard +// instead of about slugification. +func (e *consentEnv) lookupByName(t *testing.T, name string) *models.Workspace { + t.Helper() + all, err := e.srv.store.ListWorkspaces() + if err != nil { + t.Fatalf("ListWorkspaces: %v", err) + } + for i := range all { + if all[i].Name == name { + return &all[i] + } + } + return nil +} + +// mustNotExist is the counterfactual half of every refusal leg: the thing a +// 403-after-the-write would have left behind. +func (e *consentEnv) mustNotExist(t *testing.T, name string) { + t.Helper() + if ws := e.lookupByName(t, name); ws != nil { + t.Errorf("workspace %q exists after a refused create — the guard ran too late "+ + "(id=%s slug=%s); the refusal must happen before store.CreateWorkspace", + name, ws.ID, ws.Slug) + } +} + +func (e *consentEnv) mustExist(t *testing.T, name string) *models.Workspace { + t.Helper() + ws := e.lookupByName(t, name) + if ws == nil { + t.Fatalf("workspace %q does not exist, want created", name) + } + return ws +} + +func errorCode(t *testing.T, rr *httptest.ResponseRecorder) string { + t.Helper() + var env struct { + Error struct { + Code string `json:"code"` + Message string `json:"message"` + } `json:"error"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &env); err != nil { + t.Fatalf("parse error envelope: %v (body=%s)", err, rr.Body.String()) + } + return env.Error.Code +} + +// --- POST /api/v1/workspaces --- + +func TestCreateWorkspace_OAuthConnectionWithoutCreateConsent_403(t *testing.T) { + e := newConsentEnv(t, false) + + rr := e.createWorkspace("Refused WS", e.requestID) + + if rr.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 (body=%s)", rr.Code, rr.Body.String()) + } + if code := errorCode(t, rr); code != "forbidden" { + t.Errorf("error code = %q, want %q", code, "forbidden") + } + e.mustNotExist(t, "Refused WS") +} + +// The gate must sit above the body decode, so a refused caller gets the +// refusal and not a validation error. This is what distinguishes a guard at +// the top of the handler from one placed after decodeJSON: with the guard +// lower, this request answers 400 and the caller learns its body shape was +// wrong rather than that it may not create workspaces at all. +func TestCreateWorkspace_ConsentRefusalPrecedesBodyValidation(t *testing.T) { + e := newConsentEnv(t, false) + + rr := e.do("POST", "/api/v1/workspaces", "application/json", + []byte(`{"name":`), e.requestID) + + if rr.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 on a malformed body from a connection "+ + "that may not create (body=%s)", rr.Code, rr.Body.String()) + } +} + +// An empty name is the OTHER validation the handler does, and it is checked +// after the decode succeeds — same ordering claim, second instance, because a +// guard placed between decodeJSON and the name check would pass the test above +// and fail this one. +func TestCreateWorkspace_ConsentRefusalPrecedesNameValidation(t *testing.T) { + e := newConsentEnv(t, false) + + rr := e.do("POST", "/api/v1/workspaces", "application/json", + []byte(`{"name":""}`), e.requestID) + + if rr.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 on an empty name from a connection "+ + "that may not create (body=%s)", rr.Code, rr.Body.String()) + } +} + +// Control leg 1: the flag SET is the unchanged path — creates AND auto-adds. +// Without this, a guard that refused every OAuth caller would pass every +// refusal test in this file. +func TestCreateWorkspace_OAuthConnectionWithCreateConsent_CreatesAndAutoAdds(t *testing.T) { + e := newConsentEnv(t, true) + + rr := e.createWorkspace("Allowed WS", e.requestID) + + if rr.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201 (body=%s)", rr.Code, rr.Body.String()) + } + ws := e.mustExist(t, "Allowed WS") + + slugs, err := e.srv.store.ListConnectionWorkspaceSlugs(e.requestID) + if err != nil { + t.Fatalf("ListConnectionWorkspaceSlugs: %v", err) + } + found := false + for _, s := range slugs { + if s == ws.Slug { + found = true + break + } + } + if !found { + t.Errorf("connection allow-list = %v, want it to contain %q — the auto-add "+ + "behaviour for flag=true is explicitly unchanged by this unit", slugs, ws.Slug) + } +} + +// Control leg 2: a caller that is not an OAuth grant at all. PATs, CLI session +// tokens and local stdio carry no request_id, so creation rides on ordinary +// account authority and this flag has no opinion about it. A guard that read +// the flag off the wrong identity source — or defaulted a missing one to +// false — would break every CLI `pad init` on the platform, which is the most +// expensive way this change could go wrong. +func TestCreateWorkspace_NonOAuthCallerUnaffected(t *testing.T) { + e := newConsentEnv(t, false) + + rr := e.createWorkspace("Pat Made This", "") + + if rr.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201 for a PAT caller (body=%s)", rr.Code, rr.Body.String()) + } + e.mustExist(t, "Pat Made This") +} + +// Control leg 3: an OAuth grant that predates Phase C has no oauth_connections +// row. The backfill mints those rows with may_create_workspaces ON, so the +// missing-row case must ALLOW — treating "no row" as "no permission" would +// refuse every un-backfilled grant. +func TestCreateWorkspace_PrePhaseCGrantWithoutConnectionRow_Allowed(t *testing.T) { + e := newConsentEnv(t, false) + + rr := e.createWorkspace("Legacy Grant WS", "req-no-such-connection") + + if rr.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201 for an OAuth grant with no connection row "+ + "(body=%s)", rr.Code, rr.Body.String()) + } + e.mustExist(t, "Legacy Grant WS") +} + +// --- POST /api/v1/workspaces/import --- + +// exportBody produces a valid JSON export payload by round-tripping a +// workspace the test seeded through the store. +func (e *consentEnv) exportBody(t *testing.T) []byte { + t.Helper() + src, err := e.srv.store.CreateWorkspace(models.WorkspaceCreate{ + Name: "Export Source", OwnerID: e.user.ID, + }) + if err != nil { + t.Fatalf("CreateWorkspace(export source): %v", err) + } + export, err := e.srv.store.ExportWorkspace(src.Slug) + if err != nil { + t.Fatalf("ExportWorkspace: %v", err) + } + body, err := json.Marshal(export) + if err != nil { + t.Fatalf("marshal export: %v", err) + } + return body +} + +func TestImportWorkspace_OAuthConnectionWithoutCreateConsent_403(t *testing.T) { + e := newConsentEnv(t, false) + body := e.exportBody(t) + + rr := e.do("POST", "/api/v1/workspaces/import?name=Imported-Refused", + "application/json", body, e.requestID) + + if rr.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 (body=%s)", rr.Code, rr.Body.String()) + } + if code := errorCode(t, rr); code != "forbidden" { + t.Errorf("error code = %q, want %q", code, "forbidden") + } + e.mustNotExist(t, "Imported-Refused") +} + +// The gzip Content-Type branch dispatches to handleImportWorkspaceBundle from +// inside handleImportWorkspace. The guard sits above that dispatch, so the +// bundle path is covered by the same check — and a refused caller is turned +// away before the 64 MiB body read rather than after uploading a bundle. +// The payload here is deliberately NOT a valid tar.gz: if the guard were below +// the dispatch, this request would fail with a bundle-parse error instead of +// the consent refusal, so the assertion discriminates placement rather than +// merely re-testing the JSON leg. +func TestImportWorkspace_BundlePathRefusedBeforeParsing(t *testing.T) { + e := newConsentEnv(t, false) + + rr := e.do("POST", "/api/v1/workspaces/import?name=Bundle-Refused", + "application/gzip", []byte("not a gzip stream at all"), e.requestID) + + if rr.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 before the bundle is parsed (body=%s)", + rr.Code, rr.Body.String()) + } + e.mustNotExist(t, "Bundle-Refused") +} + +// Control: import still works for a connection that may create. Without this, +// a guard that refused all imports outright would pass the two legs above. +func TestImportWorkspace_WithCreateConsent_Imports(t *testing.T) { + e := newConsentEnv(t, true) + body := e.exportBody(t) + + rr := e.do("POST", "/api/v1/workspaces/import?name=Imported-Allowed", + "application/json", body, e.requestID) + + if rr.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201 (body=%s)", rr.Code, rr.Body.String()) + } + e.mustExist(t, "Imported-Allowed") +} + +// Control: a non-OAuth caller importing is unaffected, same reasoning as the +// create-side PAT leg. +func TestImportWorkspace_NonOAuthCallerUnaffected(t *testing.T) { + e := newConsentEnv(t, false) + body := e.exportBody(t) + + rr := e.do("POST", "/api/v1/workspaces/import?name=Imported-By-Pat", + "application/json", body, "") + + if rr.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201 for a PAT caller (body=%s)", rr.Code, rr.Body.String()) + } + e.mustExist(t, "Imported-By-Pat") +} + +// The refusal message has to tell the caller what to DO — an agent that reads +// "forbidden" alone will retry. Pinned loosely (the actionable noun, not the +// whole sentence) so rewording stays cheap. +func TestCreateWorkspace_RefusalMessageNamesTheRemedy(t *testing.T) { + e := newConsentEnv(t, false) + + rr := e.createWorkspace("Message WS", e.requestID) + + body := rr.Body.String() + if !strings.Contains(strings.ToLower(body), "re-authorize") { + t.Errorf("refusal message does not tell the caller how to fix it: %s", body) + } +} diff --git a/internal/server/handlers_workspaces.go b/internal/server/handlers_workspaces.go index 800e46010..f548fd42a 100644 --- a/internal/server/handlers_workspaces.go +++ b/internal/server/handlers_workspaces.go @@ -3,6 +3,7 @@ package server import ( "context" "database/sql" + "errors" "fmt" "log/slog" "math" @@ -290,7 +291,84 @@ func (s *Server) handleReorderWorkspaces(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) } +// requireWorkspaceCreationConsent gates every endpoint that MINTS a +// workspace under the caller's account on the calling OAuth connection's +// `may_create_workspaces` grant. Returns true when the request may +// proceed; on false it has already written the response. +// +// Ruled by Dave on IDEA-2756 (2026-08-26): the consent screen's "may +// create workspaces" checkbox is a permission on whether the connected +// token has the right to CREATE a workspace, and it has to be true to +// what a user would honestly expect from the option. Before this, the +// flag gated only maybeAutoAddCreatorConnection's allow-list insert — +// so a connection whose user explicitly left the box unticked could +// still create workspaces, it just could not then see them. A +// permission that does not prevent the action it names is a consent +// mismatch, and the behaviour-change-for-existing-connections argument +// lost to honest consent semantics. +// +// Shape mirrors handleAuditLog's consent refusal (BUG-2102): a hard 403 +// rather than a narrowed response, because there is no narrower version +// of creating a workspace. +// +// Three non-refusal cases, each deliberate: +// +// - Not an OAuth grant (PAT, CLI session token, local stdio — no +// request_id in context). Creation rides on ordinary account +// authority; this flag has no opinion about it. +// - ErrOAuthConnectionNotFound — a pre-Phase-C grant whose row has +// not been backfilled yet. The backfill mints these with +// may_create_workspaces ON (oauth_connections_backfill.go), so +// allowing here is that same default applied early, not a gap. +// Note the deliberate asymmetry with maybeAutoAddCreatorConnection, +// which treats not-found as "no auto-add": that path is declining a +// convenience, this one would be inventing a refusal. +// - Flag set — proceeds, and the auto-add downstream is unchanged. +// +// A real I/O error reading the connection FAILS CLOSED with a 500. The +// alternative — allowing the create when the deciding state could not +// be read — silently grants a permission the user declined, on the +// strength of a database blip. It is a 500 and not stored_state_ +// unreadable because the state is not unreadable-in-principle; the read +// failed, the fault is ours, and a retry can legitimately succeed. +func (s *Server) requireWorkspaceCreationConsent(w http.ResponseWriter, r *http.Request) bool { + kind, requestID := MCPTokenIdentityFromContext(r.Context()) + if kind != "oauth" || requestID == "" { + return true + } + conn, err := s.store.GetOAuthConnection(requestID) + if err != nil { + if errors.Is(err, store.ErrOAuthConnectionNotFound) { + return true + } + slog.Error("workspace creation consent check failed to read connection", + "request_id", requestID, "error", err) + writeError(w, http.StatusInternalServerError, "internal_error", + "Could not verify this connection's workspace-creation permission") + return false + } + if !conn.MayCreateWorkspaces { + // The quoted string is the checkbox's ACTUAL label, verbatim from + // the consent template (handlers_oauth.go) and the connections + // page (web console). A remedy that names a control the user + // cannot find is not a remedy; if either label is reworded, this + // message is part of that change. + writeError(w, http.StatusForbidden, "forbidden", + "This connection is not permitted to create workspaces. "+ + "Re-authorize it with \"Let this app create new workspaces\" enabled, "+ + "or enable it for this connection under /console/connected-apps.") + return false + } + return true +} + func (s *Server) handleCreateWorkspace(w http.ResponseWriter, r *http.Request) { + // Consent gate first — before decoding the body, so a refusal never + // depends on body validity and cannot be probed by shape. + if !s.requireWorkspaceCreationConsent(w, r) { + return + } + var input models.WorkspaceCreate if err := decodeJSON(r, &input); err != nil { writeError(w, http.StatusBadRequest, "bad_request", err.Error()) @@ -382,7 +460,15 @@ func (s *Server) handleCreateWorkspace(w http.ResponseWriter, r *http.Request) { // - The grant's connection row doesn't exist (pre-Phase-C tokens // fall here until backfill). // - The flag is off (user explicitly scoped out creation power at -// consent time or via the connections-page mutation UI). +// consent time or via the connections-page mutation UI). Since +// IDEA-2756 this branch is UNREACHABLE from the only caller — +// handleCreateWorkspace now refuses the create outright before it +// gets here, so a flag-off connection never reaches a create that +// could auto-add. It stays because this function's contract is "add +// only when the grant permits", and a helper whose safety depends on +// its single caller checking first is one refactor away from being +// wrong. It is dead code, not redundant logic: delete it only +// together with the guard. // // Errors are logged at WARN, never propagated. The caller's response // must not fail because of an auth-bookkeeping issue post-creation. @@ -682,6 +768,17 @@ func (s *Server) handleExportWorkspace(w http.ResponseWriter, r *http.Request) { } func (s *Server) handleImportWorkspace(w http.ResponseWriter, r *http.Request) { + // Consent gate — import is workspace creation through a second door + // (store.ImportWorkspace calls CreateWorkspace), so the same + // may_create_workspaces grant decides it. Ruled with the create-side + // gate on IDEA-2756. It sits ABOVE the Content-Type dispatch so it + // covers the tar.gz bundle path too — handleImportWorkspaceBundle is + // reachable only from here, so this is its only door — and above the + // 64 MiB body read, so a refused caller never uploads a bundle. + if !s.requireWorkspaceCreationConsent(w, r) { + return + } + // Content-Type dispatch: // application/gzip / application/x-gzip / application/x-tar // → tar.gz bundle path (TASK-885) — handles attachments. From a0c7c181801d2fa1d2b236e2d6f22d4ceb7380ec Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 26 Aug 2026 13:53:51 +0000 Subject: [PATCH 2/7] docs(server): state the import gate's reachability precisely (IDEA-2756) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The import-side gate is correct but currently unexercised in production, and the first framing of this change did not say so. WithMCPTokenIdentity is stashed by exactly one middleware, MCPBearerAuth, mounted on /mcp alone. An OAuth connection reaches an /api/v1 handler only through the in-process MCP dispatcher, and that dispatcher's route table has a workspace create action but no workspace import. So no OAuth-bound caller can reach handleImportWorkspace today. The gate stays, and the comment now says why: adding that action later must not silently reopen the door, which is the state a create-only fix would have left armed. Found on a verify pass reading the middleware mount points, not by the tests — they synthesize the OAuth identity into the request context, so they prove the handler's behaviour GIVEN an identity and have no opinion about which routes supply one (CONVE-19). Codex round 2 reached the same conclusion independently. --- internal/server/handlers_workspaces.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/server/handlers_workspaces.go b/internal/server/handlers_workspaces.go index f548fd42a..09fa6b1aa 100644 --- a/internal/server/handlers_workspaces.go +++ b/internal/server/handlers_workspaces.go @@ -775,6 +775,16 @@ func (s *Server) handleImportWorkspace(w http.ResponseWriter, r *http.Request) { // covers the tar.gz bundle path too — handleImportWorkspaceBundle is // reachable only from here, so this is its only door — and above the // 64 MiB body read, so a refused caller never uploads a bundle. + // + // Reachability, stated precisely because the create-side gate's is + // different: NO OAuth-bound caller can reach this handler today. The + // OAuth identity is stashed only by MCPBearerAuth, which is mounted + // on /mcp alone, so an OAuth connection reaches an /api/v1 handler + // only through the in-process MCP dispatcher — and its route table + // has no `workspace import` action. This gate is therefore correct + // but currently unexercised in production: it exists so that adding + // that action later cannot silently reopen the door, which is the + // failure mode a create-only fix would have left armed. if !s.requireWorkspaceCreationConsent(w, r) { return } From ca1b71391b58f082171eac73b86de9170e96744b Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 26 Aug 2026 14:07:42 +0000 Subject: [PATCH 3/7] fix(server): correct five overstated claims from Codex round 3 (IDEA-2756) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All five were mine, all P2, none changing the gate's behaviour — four are claims that were broader than the code, one is a test that proved less than its name. 1. "Only re-authorization lifts it" was wrong in five places (version.go, README, CLAUDE.md, the MCP catalog description, CLI help). A user can also enable the flag on the EXISTING connection via PATCH /connected-apps/{id}/flags, which the console page drives — instructions.md said so and contradicted the others. All five now name both remedies, and both are still the user's, which is the part that matters: neither is reachable by the app. 2. "This branch is UNREACHABLE ... it is dead code" on maybeAutoAddCreatorConnection's flag-off branch was false. The gate reads the connection and that function reads it AGAIN after creation; a user revoking creation power from the console between those two reads lands exactly there. It is a real second check across a real TOCTOU window, failing in the safe direction. The claim was written from the call graph, which cannot see a concurrent write between two reads. 3. handlers_import_bundle.go's "Auth: any authenticated user" was made false by this change and the concept sweep never had a chance at it — it greps may_create / auto-add / creation power, and that sentence contains none of them. Corrected in place. 4. The two NonOAuthCallerUnaffected tests claimed PAT, CLI session and local stdio; each drives one PAT. The comments now state the fixture's real scope and why one caller stands for the class (the guard branches on an identity only MCPBearerAuth sets, so callers that skipped it are indistinguishable) rather than implying three fixtures. 5. The JSON import refusal leg would have passed with the gate below decodeJSONWithLimit — only the bundle leg pinned placement, and only for gzip. Adds TestImportWorkspace_ConsentRefusalPrecedesBodyDecode (malformed body: 400 if the gate is late, 403 if it is early), mirroring the create-side ordering legs. Mutation matrix now 9 mutants, 9 detected. M8 (guard below the JSON decode) is killed by the bundle leg too, so it shows the new test is covered rather than necessary; M9 gates the bundle path and moves only the JSON path's guard, and dies to the new leg ALONE. That is the mutant that justifies the test. --- CLAUDE.md | 2 +- README.md | 2 +- cmd/pad/cmd_workspace.go | 5 ++- internal/mcp/catalog_workspace.go | 5 ++- internal/mcp/instructions.md | 2 +- internal/mcp/version.go | 12 +++--- internal/server/handlers_import_bundle.go | 5 ++- .../handlers_workspace_create_consent_test.go | 40 +++++++++++++++---- internal/server/handlers_workspaces.go | 27 +++++++++---- 9 files changed, 72 insertions(+), 28 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1dc181cce..b536d2943 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -249,7 +249,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.26) 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.26 (IDEA-2756) makes `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant carries `may_create_workspaces=false` — that consent checkbox previously gated only the post-creation auto-add, so a connection whose user declined it created workspaces it could then not see. The same gate covers `POST /workspaces/import` (a second door onto `store.ImportWorkspace` → `CreateWorkspace`, with no MCP action today). No escape-hatch param, deliberately: the gate expresses the USER's consent decision, so only re-authorization lifts it. v0.25 (TASK-2657 / BUG-2702) makes `pad_library.activate` resolve its destination collection from the target's declared artifact kind rather than the literal `conventions` / `playbooks` slugs, and surfaces a lookup ERROR instead of falling back. v0.24 (#1066) makes the `pad_item` `fields` OBJECT a real write form on create/update — reads return `fields` as a native object (BUG-991 normalization), and writing that shape back was a silent no-op: not a declared param, no `additionalProperties`, so it was accepted, never mapped by `BuildCLIArgs`, and dropped while the PATCH still bumped `updated_at`. The alias merges into the same path as `field: ["key=value"]` / the dedicated params (`catalog_item_fields.go`), refusing the same key in two places with conflicting values; and input validation is now STRICT across all catalog tools — an undeclared top-level key fails with a structured `validation_failed` naming it, instead of being silently dropped (a small documented compat list survives: pad_item's v0.16 `assigned_user_id` / `agent_role_id` remote clear form). One bump covers both halves — they are one contract change. v0.23 (BUG-2627 part 2 + BUG-2675, PR #1166) refuses raw `field` setters naming system-metadata keys in `fields_patch` on every transport (`github_pr` exempt on UPDATE only — the sole remote writer, itself broken: BUG-2696) and adds the retry-hostile `stored_state_unreadable` error code. v0.22 (BUG-2674, PR #1165) makes reserved metadata survive a move and refuses `field` setters naming those keys on move/copy — see `internal/mcp/version.go` for both full entries. 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. +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.26) 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.26 (IDEA-2756) makes `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant carries `may_create_workspaces=false` — that consent checkbox previously gated only the post-creation auto-add, so a connection whose user declined it created workspaces it could then not see. The same gate covers `POST /workspaces/import` (a second door onto `store.ImportWorkspace` → `CreateWorkspace`, with no MCP action today). No escape-hatch param, deliberately: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps`. v0.25 (TASK-2657 / BUG-2702) makes `pad_library.activate` resolve its destination collection from the target's declared artifact kind rather than the literal `conventions` / `playbooks` slugs, and surfaces a lookup ERROR instead of falling back. v0.24 (#1066) makes the `pad_item` `fields` OBJECT a real write form on create/update — reads return `fields` as a native object (BUG-991 normalization), and writing that shape back was a silent no-op: not a declared param, no `additionalProperties`, so it was accepted, never mapped by `BuildCLIArgs`, and dropped while the PATCH still bumped `updated_at`. The alias merges into the same path as `field: ["key=value"]` / the dedicated params (`catalog_item_fields.go`), refusing the same key in two places with conflicting values; and input validation is now STRICT across all catalog tools — an undeclared top-level key fails with a structured `validation_failed` naming it, instead of being silently dropped (a small documented compat list survives: pad_item's v0.16 `assigned_user_id` / `agent_role_id` remote clear form). One bump covers both halves — they are one contract change. v0.23 (BUG-2627 part 2 + BUG-2675, PR #1166) refuses raw `field` setters naming system-metadata keys in `fields_patch` on every transport (`github_pr` exempt on UPDATE only — the sole remote writer, itself broken: BUG-2696) and adds the retry-hostile `stored_state_unreadable` error code. v0.22 (BUG-2674, PR #1165) makes reserved metadata survive a move and refuses `field` setters naming those keys on move/copy — see `internal/mcp/version.go` for both full entries. 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. diff --git a/README.md b/README.md index c9b5d3238..751c0ae06 100644 --- a/README.md +++ b/README.md @@ -417,7 +417,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.26"` — 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); v0.22 stopped `pad_item.move` destroying an item’s system metadata — implementation notes, decision log, linked PR and convention data now survive a move, any field the destination schema has no home for is REPORTED in the move’s activity entry rather than vanishing, and a `field` setter naming one of those reserved keys is refused with `malformed_override` instead of writing it (BUG-2674); v0.23 closed the same door on the ordinary update — a `field` setter naming `implementation_notes`, `decision_log` or `convention` is now refused on every transport at once (`validation_error` on HTTP, surfaced to MCP clients as `validation_failed`); the one gate covers the CLI, remote MCP and stdio MCP at once because all three lower a `field` setter into the same `fields_patch`; `github_pr` is deliberately exempt ON UPDATE (move and copy still refuse it), since `pad github link` cannot run on remote MCP and refusing it would leave those agents with no door at all (that door is itself broken — BUG-2696); item CREATE stays open, deliberately, because its full-`fields` payload is shared with Pad’s own writers. v0.23 also added the retry-hostile `stored_state_unreadable` error code so an agent told its target item’s stored data is unreadable stops instead of retrying a permanent failure (BUG-2627 / BUG-2675); v0.24 made the `pad_item` `fields` object a real write form on create/update — reads return `fields` as a native object, and writing that shape back was a silent no-op (accepted, never mapped, dropped while the PATCH still bumped `updated_at`) — merging it into the same path as `field`/the dedicated params with conflicting duplicate keys refused, and made input validation strict across all catalog tools: undeclared top-level keys now fail with a structured error instead of being silently dropped (#1066); v0.25 made `pad_library.activate` resolve its DESTINATION collection from the target’s declared artifact kind (SPEC-5 collection traits) rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly instead of failing not-found with the collection sitting right there (BUG-2702); a lookup ERROR is now surfaced rather than silently falling back to the canonical slug, because falling back on an error means writing to a slug nothing was confirmed about (TASK-2657); v0.26 made `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant has `may_create_workspaces=false` — that checkbox previously gated only the post-creation auto-add, so a connection whose user declined it could still create workspaces it could then not see; the same gate covers `POST /workspaces/import`, which mints a workspace through a second door. There is deliberately no escape-hatch parameter: the gate expresses the USER's consent decision, so only re-authorization lifts it (IDEA-2756); see `internal/mcp/version.go` for the full changelog) +- `tool_surface_version: "0.26"` — 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); v0.22 stopped `pad_item.move` destroying an item’s system metadata — implementation notes, decision log, linked PR and convention data now survive a move, any field the destination schema has no home for is REPORTED in the move’s activity entry rather than vanishing, and a `field` setter naming one of those reserved keys is refused with `malformed_override` instead of writing it (BUG-2674); v0.23 closed the same door on the ordinary update — a `field` setter naming `implementation_notes`, `decision_log` or `convention` is now refused on every transport at once (`validation_error` on HTTP, surfaced to MCP clients as `validation_failed`); the one gate covers the CLI, remote MCP and stdio MCP at once because all three lower a `field` setter into the same `fields_patch`; `github_pr` is deliberately exempt ON UPDATE (move and copy still refuse it), since `pad github link` cannot run on remote MCP and refusing it would leave those agents with no door at all (that door is itself broken — BUG-2696); item CREATE stays open, deliberately, because its full-`fields` payload is shared with Pad’s own writers. v0.23 also added the retry-hostile `stored_state_unreadable` error code so an agent told its target item’s stored data is unreadable stops instead of retrying a permanent failure (BUG-2627 / BUG-2675); v0.24 made the `pad_item` `fields` object a real write form on create/update — reads return `fields` as a native object, and writing that shape back was a silent no-op (accepted, never mapped, dropped while the PATCH still bumped `updated_at`) — merging it into the same path as `field`/the dedicated params with conflicting duplicate keys refused, and made input validation strict across all catalog tools: undeclared top-level keys now fail with a structured error instead of being silently dropped (#1066); v0.25 made `pad_library.activate` resolve its DESTINATION collection from the target’s declared artifact kind (SPEC-5 collection traits) rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly instead of failing not-found with the collection sitting right there (BUG-2702); a lookup ERROR is now surfaced rather than silently falling back to the canonical slug, because falling back on an error means writing to a slug nothing was confirmed about (TASK-2657); v0.26 made `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant has `may_create_workspaces=false` — that checkbox previously gated only the post-creation auto-add, so a connection whose user declined it could still create workspaces it could then not see; the same gate covers `POST /workspaces/import`, which mints a workspace through a second door. There is deliberately no escape-hatch parameter: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps` (IDEA-2756); 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_workspace.go b/cmd/pad/cmd_workspace.go index 346de3eab..4b05b820e 100644 --- a/cmd/pad/cmd_workspace.go +++ b/cmd/pad/cmd_workspace.go @@ -358,8 +358,9 @@ may_create_workspaces=true, the new workspace is auto-added to that connection's allow-list (PLAN-1519 / TASK-1521 / IDEA-1517 §1) so the agent can use it immediately without re-auth. When that grant has may_create_workspaces=false the create is REFUSED with a 403 and no -workspace is made (IDEA-2756) — the user must re-authorize the -connection with workspace creation enabled.`, +workspace is made (IDEA-2756). Only the user can lift it, either by +re-authorizing or by enabling the flag on the existing connection at +/console/connected-apps.`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { client, _ := getClient() diff --git a/internal/mcp/catalog_workspace.go b/internal/mcp/catalog_workspace.go index 8aa38ecaa..a2611b0ed 100644 --- a/internal/mcp/catalog_workspace.go +++ b/internal/mcp/catalog_workspace.go @@ -155,8 +155,9 @@ Actions: that connection's allow-list — usable immediately, no re-auth. When that grant has may_create_workspaces=FALSE the call is REFUSED with a 403 and no workspace is created (IDEA-2756); - retrying will not help — the user must re-authorize the - connection with workspace creation enabled. + retrying will not help. Only the USER can lift it, either by + re-authorizing or by enabling the flag on the existing + connection at /console/connected-apps. claim — Redeem a 6-digit claim code to add a workspace to the calling OAuth connection's allow-list. Required: workspace, code. diff --git a/internal/mcp/instructions.md b/internal/mcp/instructions.md index 623c83949..e3061d8fe 100644 --- a/internal/mcp/instructions.md +++ b/internal/mcp/instructions.md @@ -87,7 +87,7 @@ Filter by trigger (`always`, `on-implement`, `on-task-complete`, etc.) when rele If the user references a workspace this connection can't see (you'll get a 403 from workspace tools, or the workspace won't appear in `pad_workspace.list`), tell the user you can't see that workspace with your current permissions, then walk them through how to grant access: open Pad in their browser → switch to that workspace → avatar menu → "Connect project..." A 6-digit claim code will appear. Have them paste it back in chat, then call `pad_workspace.claim` with `{workspace: "", code: "<6 digits>"}`. The workspace joins this connection's allow-list and stays until the user revokes it via `/console/connected-apps`. No re-auth required. -For brand-new workspaces, `pad_workspace.create` with `{name: ""}` (and optional `template`) creates the workspace AND auto-adds it to this connection's allow-list in one call — no claim code needed. Only works when the user granted "may create workspaces" at consent time. If that scope was declined, the create is **refused with a 403 and no workspace is made** — retrying won't help and neither will the claim flow, since there is nothing to claim. Tell the user this connection isn't permitted to create workspaces, and offer them the two ways forward: create the workspace themselves in the browser and grant access with a claim code, or re-authorize this connection with "Let this app create new workspaces" enabled (`/console/connected-apps` can also flip it on an existing connection). +For brand-new workspaces, `pad_workspace.create` with `{name: ""}` (and optional `template`) creates the workspace AND auto-adds it to this connection's allow-list in one call — no claim code needed. Only works when the connection currently carries the "may create workspaces" grant — granted at consent time, or enabled later on the connections page. If it does not, the create is **refused with a 403 and no workspace is made** — retrying won't help and neither will the claim flow, since there is nothing to claim. Tell the user this connection isn't permitted to create workspaces, and offer them the two ways forward: create the workspace themselves in the browser and grant access with a claim code, or re-authorize this connection with "Let this app create new workspaces" enabled (`/console/connected-apps` can also flip it on an existing connection). ## New workspace: offer to set it up diff --git a/internal/mcp/version.go b/internal/mcp/version.go index 628fdd810..55a97e4e3 100644 --- a/internal/mcp/version.go +++ b/internal/mcp/version.go @@ -635,11 +635,13 @@ const CmdhelpVersion = "0.1" // Unlike v0.10 there is no `allow_draft`-style escape hatch, and // deliberately so: the gate expresses a decision the USER made on // the consent screen, so a parameter that let the caller bypass it -// would be the app overriding its own grant. The remedy is -// re-authorization, which only the user can perform — the refusal -// message says so, and instructions.md tells the agent not to retry -// and not to reach for the claim flow (there is nothing to claim -// when nothing was created). +// would be the app overriding its own grant. Both remedies are the +// USER's and neither is the app's: re-authorize, or enable the flag +// on the existing connection via PATCH /connected-apps/{id}/flags +// (the console page). The refusal message names both, and +// instructions.md tells the agent not to retry and not to reach for +// the claim flow (there is nothing to claim when nothing was +// created). // // Ruled by Dave on IDEA-2756: the consent checkbox is a permission // on whether the connected token may CREATE, and has to be true to diff --git a/internal/server/handlers_import_bundle.go b/internal/server/handlers_import_bundle.go index c4c29b16c..a8d1384af 100644 --- a/internal/server/handlers_import_bundle.go +++ b/internal/server/handlers_import_bundle.go @@ -63,7 +63,10 @@ func (s *Server) effectiveBlobMaxBytes() int64 { // The handler does the JSON / bundle dispatch; the actual work is // done by importBundle which is unit-testable without the http stack. // -// Auth: any authenticated user. The global RequireAuth middleware +// Auth: any authenticated user EXCEPT an OAuth-bound caller whose +// connection carries may_create_workspaces=false — import mints a +// workspace, so handleImportWorkspace's consent gate refuses it before +// dispatching here (IDEA-2756). The global RequireAuth middleware // (server.go:539) gates this endpoint when users exist on the host. // There is no per-workspace role check because import CREATES a new // workspace — there's nothing pre-existing to authorize against. diff --git a/internal/server/handlers_workspace_create_consent_test.go b/internal/server/handlers_workspace_create_consent_test.go index 1fd4005dd..7fc5af7b2 100644 --- a/internal/server/handlers_workspace_create_consent_test.go +++ b/internal/server/handlers_workspace_create_consent_test.go @@ -258,12 +258,18 @@ func TestCreateWorkspace_OAuthConnectionWithCreateConsent_CreatesAndAutoAdds(t * } } -// Control leg 2: a caller that is not an OAuth grant at all. PATs, CLI session -// tokens and local stdio carry no request_id, so creation rides on ordinary -// account authority and this flag has no opinion about it. A guard that read -// the flag off the wrong identity source — or defaulted a missing one to -// false — would break every CLI `pad init` on the platform, which is the most +// Control leg 2: a caller that is not an OAuth grant. A guard that read the +// flag off the wrong identity source — or defaulted a missing one to false — +// would break every CLI `pad init` on the platform, which is the most // expensive way this change could go wrong. +// +// Scope of this fixture, stated because the name is broader than it: it drives +// ONE non-OAuth caller, a PAT. CLI session tokens and local stdio are not +// separately exercised. They are the same case by construction rather than by +// coincidence — the guard branches on the MCP token identity, which only +// MCPBearerAuth sets, so every caller that did not pass through it is +// indistinguishable here. A second fixture would re-test the same branch. +// (Codex round 3 flagged the original comment for claiming all three.) func TestCreateWorkspace_NonOAuthCallerUnaffected(t *testing.T) { e := newConsentEnv(t, false) @@ -330,6 +336,24 @@ func TestImportWorkspace_OAuthConnectionWithoutCreateConsent_403(t *testing.T) { e.mustNotExist(t, "Imported-Refused") } +// The JSON leg above proves the refusal but NOT its placement: it sends a valid +// export, which a gate sitting after decodeJSONWithLimit would also refuse. This +// leg sends a malformed body, so a gate below the decode answers 400 and only a +// gate above it answers 403 — the same discrimination the create-side ordering +// legs make, which the import side was missing. (Codex round 3.) +func TestImportWorkspace_ConsentRefusalPrecedesBodyDecode(t *testing.T) { + e := newConsentEnv(t, false) + + rr := e.do("POST", "/api/v1/workspaces/import?name=Malformed-Refused", + "application/json", []byte(`{"version":`), e.requestID) + + if rr.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 on a malformed import body from a connection "+ + "that may not create (body=%s)", rr.Code, rr.Body.String()) + } + e.mustNotExist(t, "Malformed-Refused") +} + // The gzip Content-Type branch dispatches to handleImportWorkspaceBundle from // inside handleImportWorkspace. The guard sits above that dispatch, so the // bundle path is covered by the same check — and a refused caller is turned @@ -366,8 +390,10 @@ func TestImportWorkspace_WithCreateConsent_Imports(t *testing.T) { e.mustExist(t, "Imported-Allowed") } -// Control: a non-OAuth caller importing is unaffected, same reasoning as the -// create-side PAT leg. +// Control: a non-OAuth caller importing is unaffected — same reasoning, and the +// same fixture scope, as the create-side PAT leg: one PAT stands for the whole +// non-OAuth class because the guard branches on an identity only MCPBearerAuth +// sets. func TestImportWorkspace_NonOAuthCallerUnaffected(t *testing.T) { e := newConsentEnv(t, false) body := e.exportBody(t) diff --git a/internal/server/handlers_workspaces.go b/internal/server/handlers_workspaces.go index 09fa6b1aa..e8e08059c 100644 --- a/internal/server/handlers_workspaces.go +++ b/internal/server/handlers_workspaces.go @@ -353,6 +353,12 @@ func (s *Server) requireWorkspaceCreationConsent(w http.ResponseWriter, r *http. // page (web console). A remedy that names a control the user // cannot find is not a remedy; if either label is reworded, this // message is part of that change. + // + // Both remedies are named because there are two: a fresh + // authorization, and flipping the flag on the EXISTING + // connection via PATCH /connected-apps/{id}/flags, which the + // console page drives. Saying only "re-authorize" would send a + // user through a longer path than they need. writeError(w, http.StatusForbidden, "forbidden", "This connection is not permitted to create workspaces. "+ "Re-authorize it with \"Let this app create new workspaces\" enabled, "+ @@ -461,14 +467,19 @@ func (s *Server) handleCreateWorkspace(w http.ResponseWriter, r *http.Request) { // fall here until backfill). // - The flag is off (user explicitly scoped out creation power at // consent time or via the connections-page mutation UI). Since -// IDEA-2756 this branch is UNREACHABLE from the only caller — -// handleCreateWorkspace now refuses the create outright before it -// gets here, so a flag-off connection never reaches a create that -// could auto-add. It stays because this function's contract is "add -// only when the grant permits", and a helper whose safety depends on -// its single caller checking first is one refactor away from being -// wrong. It is dead code, not redundant logic: delete it only -// together with the guard. +// IDEA-2756 handleCreateWorkspace refuses a flag-off connection +// before it reaches here, so in the common case this branch does +// not fire — but it is NOT unreachable, and calling it dead would +// be wrong twice over. The gate reads the connection, and this +// function reads it AGAIN after the workspace is created; a user +// revoking creation power from /console/connected-apps in between +// (PATCH /connected-apps/{id}/flags) lands exactly here. So this is +// a real second check across a real TOCTOU window, and it fails in +// the safe direction: the workspace exists but does not silently +// join a connection whose grant was withdrawn mid-flight. +// (Codex round 3 caught the earlier "unreachable / dead code" +// claim in this comment. It was written from the call graph alone, +// which cannot see a concurrent write between two reads.) // // Errors are logged at WARN, never propagated. The caller's response // must not fail because of an auth-bookkeeping issue post-creation. From de2ed2141136a5d0da5de88959cff241a4a49ca1 Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 26 Aug 2026 14:12:51 +0000 Subject: [PATCH 4/7] docs(server): the second consent check narrows the race, it does not close it (BUG-2792) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 caught me calling maybeAutoAddCreatorConnection's flag-off branch dead code. The replacement comment then claimed the branch means a revoked grant cannot silently gain a workspace — which is more safety than the code delivers, and round 4 caught that. The read and the AddConnectionWorkspace insert below it are separate unconditional statements, so a revocation landing BETWEEN them still adds the workspace. The check narrows the window; it does not close it. Filed as BUG-2792 rather than folded in: the race is pre-existing and unchanged by IDEA-2756, and closing it needs an atomic check-and-insert at the store layer, written and gated for both dialects — materially more diff and risk than this handler-level guard. Both mistakes were the same shape in opposite directions: a claim about concurrency derived from reading the call graph, which cannot see a concurrent write between two reads. --- internal/server/handlers_workspaces.go | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/internal/server/handlers_workspaces.go b/internal/server/handlers_workspaces.go index e8e08059c..35a4ef365 100644 --- a/internal/server/handlers_workspaces.go +++ b/internal/server/handlers_workspaces.go @@ -473,13 +473,22 @@ func (s *Server) handleCreateWorkspace(w http.ResponseWriter, r *http.Request) { // be wrong twice over. The gate reads the connection, and this // function reads it AGAIN after the workspace is created; a user // revoking creation power from /console/connected-apps in between -// (PATCH /connected-apps/{id}/flags) lands exactly here. So this is -// a real second check across a real TOCTOU window, and it fails in -// the safe direction: the workspace exists but does not silently -// join a connection whose grant was withdrawn mid-flight. -// (Codex round 3 caught the earlier "unreachable / dead code" -// claim in this comment. It was written from the call graph alone, -// which cannot see a concurrent write between two reads.) +// (PATCH /connected-apps/{id}/flags) lands exactly here, and the +// workspace then exists without silently joining a connection whose +// grant was withdrawn mid-flight. +// +// What this check does NOT do is close that window — it narrows it. +// The read below and the AddConnectionWorkspace insert after it are +// separate unconditional statements, so a revocation landing between +// THEM still adds the workspace. That residual race is BUG-2792: +// pre-existing, unchanged by IDEA-2756, and needing an atomic +// check-and-insert at the store layer rather than another read here. +// +// (Codex round 3 caught the earlier "unreachable / dead code" claim +// in this comment — written from the call graph alone, which cannot +// see a concurrent write between two reads. Round 4 then caught the +// replacement claiming more safety than the code delivers. Both +// errors were the same shape in opposite directions.) // // Errors are logged at WARN, never propagated. The caller's response // must not fail because of an auth-bookkeeping issue post-creation. From c68185008e83371ea1b3c111fddfeff90db89803 Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 26 Aug 2026 14:25:45 +0000 Subject: [PATCH 5/7] style(server): gofmt the doc comment (IDEA-2756) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gofmt wants blank lines between list items once one item spans multiple paragraphs, which the BUG-2792 note made true. My error, and worth naming exactly: I ran build, vet and the targeted tests on this commit but not lint, because lint had passed on the PREVIOUS commit and the change was 'only a comment'. The gate has to run on the tree being pushed, not on an earlier one that resembles it. CI's golangci-lint is pinned to the same v2.11.4 the Makefile installs, so there was no version skew to blame — the local gate would have caught this in 51 seconds. --- internal/server/handlers_workspaces.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/server/handlers_workspaces.go b/internal/server/handlers_workspaces.go index 35a4ef365..133811a76 100644 --- a/internal/server/handlers_workspaces.go +++ b/internal/server/handlers_workspaces.go @@ -463,8 +463,10 @@ func (s *Server) handleCreateWorkspace(w http.ResponseWriter, r *http.Request) { // // - The calling token isn't an OAuth grant (PAT, CLI session token — // they don't carry a request_id). +// // - The grant's connection row doesn't exist (pre-Phase-C tokens // fall here until backfill). +// // - The flag is off (user explicitly scoped out creation power at // consent time or via the connections-page mutation UI). Since // IDEA-2756 handleCreateWorkspace refuses a flag-off connection From cb47c76351021450f02412191c970ced2adea5ca Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 26 Aug 2026 14:54:37 +0000 Subject: [PATCH 6/7] docs(server): correct ten overstated prose claims from Codex round 8 (IDEA-2756) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 8 reviewed only the prose this change adds. Ten claims were broader than the code. All ten are mine; none changes behaviour. Rounds 3, 4 and 7 each caught one of these, which is why round 8 was pointed at the class rather than at a new dimension. The substantive ones: - "gates every endpoint that MINTS a workspace" — autoCreateWorkspace mints from registration, bootstrap and oauth-login and is deliberately outside this gate. The helper doc and the test header now name the two callers and the exclusion instead of claiming universality. - "the agent was handed a workspace it could not then see" (version.go, README, CLAUDE.md) — only true for a connection with an EXPLICIT allow-list. An all_current_workspaces=true connection is not gated per slug and could see what it made. The consent mismatch is the constant; the invisibility was its most visible symptom, not its definition. - "ErrOAuthConnectionNotFound — a pre-Phase-C grant" asserted a cause the code cannot know: ANY missing row takes that branch. Now stated as the expected cause, with the limit of what the code can tell. - "above the 64 MiB body read" conflated the two import paths. 64 MiB is the JSON decode's bound; the bundle path has its own, much larger. The gate precedes both, which is the property that actually matters. - "the request context is decorated AFTER TokenAuth runs" was false, and inherited verbatim from the sibling helper this was modelled on (handlers_oauth_claim_test.go's doClaim), where it is also false. The wrapper sets the identity BEFORE ServeHTTP; it survives because nothing on the /api/v1 chain writes that key. - "lets CreateWorkspace normalize it" — CreateWorkspace slugifies only when the supplied slug is EMPTY, and import supplies a non-empty one, so an imported workspace keeps the ?name= value verbatim. - "The PAT needs a workspace to bind to" — CreateAPIToken takes WorkspaceID as optional. And one where the first fix was worse than the finding: - "Every refusal leg asserts no workspace exists afterwards" was false — the two ordering legs assert status only. My first correction ADDED those assertions, which is the trap the finding was pointing at: a malformed body and an empty name are rejected before creation under every guard placement, so "no such workspace exists" is true of broken and working code alike. Reverted; the header now states which legs carry the counterfactual, and why the ordering legs discriminate on status instead. Gates re-run on the tree being pushed, not an earlier one: gofmt clean, lint 0 issues, internal/server and internal/mcp green, mutation matrix still 9/9. --- CLAUDE.md | 2 +- README.md | 2 +- internal/mcp/instructions.md | 2 +- internal/mcp/version.go | 9 ++- .../handlers_workspace_create_consent_test.go | 64 +++++++++++++------ internal/server/handlers_workspaces.go | 35 +++++++--- 6 files changed, 81 insertions(+), 33 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b536d2943..34141f850 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -249,7 +249,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.26) 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.26 (IDEA-2756) makes `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant carries `may_create_workspaces=false` — that consent checkbox previously gated only the post-creation auto-add, so a connection whose user declined it created workspaces it could then not see. The same gate covers `POST /workspaces/import` (a second door onto `store.ImportWorkspace` → `CreateWorkspace`, with no MCP action today). No escape-hatch param, deliberately: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps`. v0.25 (TASK-2657 / BUG-2702) makes `pad_library.activate` resolve its destination collection from the target's declared artifact kind rather than the literal `conventions` / `playbooks` slugs, and surfaces a lookup ERROR instead of falling back. v0.24 (#1066) makes the `pad_item` `fields` OBJECT a real write form on create/update — reads return `fields` as a native object (BUG-991 normalization), and writing that shape back was a silent no-op: not a declared param, no `additionalProperties`, so it was accepted, never mapped by `BuildCLIArgs`, and dropped while the PATCH still bumped `updated_at`. The alias merges into the same path as `field: ["key=value"]` / the dedicated params (`catalog_item_fields.go`), refusing the same key in two places with conflicting values; and input validation is now STRICT across all catalog tools — an undeclared top-level key fails with a structured `validation_failed` naming it, instead of being silently dropped (a small documented compat list survives: pad_item's v0.16 `assigned_user_id` / `agent_role_id` remote clear form). One bump covers both halves — they are one contract change. v0.23 (BUG-2627 part 2 + BUG-2675, PR #1166) refuses raw `field` setters naming system-metadata keys in `fields_patch` on every transport (`github_pr` exempt on UPDATE only — the sole remote writer, itself broken: BUG-2696) and adds the retry-hostile `stored_state_unreadable` error code. v0.22 (BUG-2674, PR #1165) makes reserved metadata survive a move and refuses `field` setters naming those keys on move/copy — see `internal/mcp/version.go` for both full entries. 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. +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.26) 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.26 (IDEA-2756) makes `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant carries `may_create_workspaces=false` — that consent checkbox previously gated only the post-creation auto-add, so a connection whose user declined it created workspaces anyway — invisible to it when the connection carried an explicit allow-list, visible when it carried the `all_current_workspaces` wildcard; the consent mismatch is the defect in both cases. The same gate covers `POST /workspaces/import` (a second door onto `store.ImportWorkspace` → `CreateWorkspace`, with no MCP action today). No escape-hatch param, deliberately: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps`. v0.25 (TASK-2657 / BUG-2702) makes `pad_library.activate` resolve its destination collection from the target's declared artifact kind rather than the literal `conventions` / `playbooks` slugs, and surfaces a lookup ERROR instead of falling back. v0.24 (#1066) makes the `pad_item` `fields` OBJECT a real write form on create/update — reads return `fields` as a native object (BUG-991 normalization), and writing that shape back was a silent no-op: not a declared param, no `additionalProperties`, so it was accepted, never mapped by `BuildCLIArgs`, and dropped while the PATCH still bumped `updated_at`. The alias merges into the same path as `field: ["key=value"]` / the dedicated params (`catalog_item_fields.go`), refusing the same key in two places with conflicting values; and input validation is now STRICT across all catalog tools — an undeclared top-level key fails with a structured `validation_failed` naming it, instead of being silently dropped (a small documented compat list survives: pad_item's v0.16 `assigned_user_id` / `agent_role_id` remote clear form). One bump covers both halves — they are one contract change. v0.23 (BUG-2627 part 2 + BUG-2675, PR #1166) refuses raw `field` setters naming system-metadata keys in `fields_patch` on every transport (`github_pr` exempt on UPDATE only — the sole remote writer, itself broken: BUG-2696) and adds the retry-hostile `stored_state_unreadable` error code. v0.22 (BUG-2674, PR #1165) makes reserved metadata survive a move and refuses `field` setters naming those keys on move/copy — see `internal/mcp/version.go` for both full entries. 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. diff --git a/README.md b/README.md index 751c0ae06..d78b81a72 100644 --- a/README.md +++ b/README.md @@ -417,7 +417,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.26"` — 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); v0.22 stopped `pad_item.move` destroying an item’s system metadata — implementation notes, decision log, linked PR and convention data now survive a move, any field the destination schema has no home for is REPORTED in the move’s activity entry rather than vanishing, and a `field` setter naming one of those reserved keys is refused with `malformed_override` instead of writing it (BUG-2674); v0.23 closed the same door on the ordinary update — a `field` setter naming `implementation_notes`, `decision_log` or `convention` is now refused on every transport at once (`validation_error` on HTTP, surfaced to MCP clients as `validation_failed`); the one gate covers the CLI, remote MCP and stdio MCP at once because all three lower a `field` setter into the same `fields_patch`; `github_pr` is deliberately exempt ON UPDATE (move and copy still refuse it), since `pad github link` cannot run on remote MCP and refusing it would leave those agents with no door at all (that door is itself broken — BUG-2696); item CREATE stays open, deliberately, because its full-`fields` payload is shared with Pad’s own writers. v0.23 also added the retry-hostile `stored_state_unreadable` error code so an agent told its target item’s stored data is unreadable stops instead of retrying a permanent failure (BUG-2627 / BUG-2675); v0.24 made the `pad_item` `fields` object a real write form on create/update — reads return `fields` as a native object, and writing that shape back was a silent no-op (accepted, never mapped, dropped while the PATCH still bumped `updated_at`) — merging it into the same path as `field`/the dedicated params with conflicting duplicate keys refused, and made input validation strict across all catalog tools: undeclared top-level keys now fail with a structured error instead of being silently dropped (#1066); v0.25 made `pad_library.activate` resolve its DESTINATION collection from the target’s declared artifact kind (SPEC-5 collection traits) rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly instead of failing not-found with the collection sitting right there (BUG-2702); a lookup ERROR is now surfaced rather than silently falling back to the canonical slug, because falling back on an error means writing to a slug nothing was confirmed about (TASK-2657); v0.26 made `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant has `may_create_workspaces=false` — that checkbox previously gated only the post-creation auto-add, so a connection whose user declined it could still create workspaces it could then not see; the same gate covers `POST /workspaces/import`, which mints a workspace through a second door. There is deliberately no escape-hatch parameter: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps` (IDEA-2756); see `internal/mcp/version.go` for the full changelog) +- `tool_surface_version: "0.26"` — 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); v0.22 stopped `pad_item.move` destroying an item’s system metadata — implementation notes, decision log, linked PR and convention data now survive a move, any field the destination schema has no home for is REPORTED in the move’s activity entry rather than vanishing, and a `field` setter naming one of those reserved keys is refused with `malformed_override` instead of writing it (BUG-2674); v0.23 closed the same door on the ordinary update — a `field` setter naming `implementation_notes`, `decision_log` or `convention` is now refused on every transport at once (`validation_error` on HTTP, surfaced to MCP clients as `validation_failed`); the one gate covers the CLI, remote MCP and stdio MCP at once because all three lower a `field` setter into the same `fields_patch`; `github_pr` is deliberately exempt ON UPDATE (move and copy still refuse it), since `pad github link` cannot run on remote MCP and refusing it would leave those agents with no door at all (that door is itself broken — BUG-2696); item CREATE stays open, deliberately, because its full-`fields` payload is shared with Pad’s own writers. v0.23 also added the retry-hostile `stored_state_unreadable` error code so an agent told its target item’s stored data is unreadable stops instead of retrying a permanent failure (BUG-2627 / BUG-2675); v0.24 made the `pad_item` `fields` object a real write form on create/update — reads return `fields` as a native object, and writing that shape back was a silent no-op (accepted, never mapped, dropped while the PATCH still bumped `updated_at`) — merging it into the same path as `field`/the dedicated params with conflicting duplicate keys refused, and made input validation strict across all catalog tools: undeclared top-level keys now fail with a structured error instead of being silently dropped (#1066); v0.25 made `pad_library.activate` resolve its DESTINATION collection from the target’s declared artifact kind (SPEC-5 collection traits) rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly instead of failing not-found with the collection sitting right there (BUG-2702); a lookup ERROR is now surfaced rather than silently falling back to the canonical slug, because falling back on an error means writing to a slug nothing was confirmed about (TASK-2657); v0.26 made `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant has `may_create_workspaces=false` — that checkbox previously gated only the post-creation auto-add, so a connection whose user declined it could still create workspaces — and on a connection with an explicit workspace allow-list, could not then see them (a wildcard `all_current_workspaces` connection could, which is why the consent mismatch rather than the invisibility is the defect); the same gate covers `POST /workspaces/import`, which mints a workspace through a second door. There is deliberately no escape-hatch parameter: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps` (IDEA-2756); 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/internal/mcp/instructions.md b/internal/mcp/instructions.md index e3061d8fe..a8af914cb 100644 --- a/internal/mcp/instructions.md +++ b/internal/mcp/instructions.md @@ -87,7 +87,7 @@ Filter by trigger (`always`, `on-implement`, `on-task-complete`, etc.) when rele If the user references a workspace this connection can't see (you'll get a 403 from workspace tools, or the workspace won't appear in `pad_workspace.list`), tell the user you can't see that workspace with your current permissions, then walk them through how to grant access: open Pad in their browser → switch to that workspace → avatar menu → "Connect project..." A 6-digit claim code will appear. Have them paste it back in chat, then call `pad_workspace.claim` with `{workspace: "", code: "<6 digits>"}`. The workspace joins this connection's allow-list and stays until the user revokes it via `/console/connected-apps`. No re-auth required. -For brand-new workspaces, `pad_workspace.create` with `{name: ""}` (and optional `template`) creates the workspace AND auto-adds it to this connection's allow-list in one call — no claim code needed. Only works when the connection currently carries the "may create workspaces" grant — granted at consent time, or enabled later on the connections page. If it does not, the create is **refused with a 403 and no workspace is made** — retrying won't help and neither will the claim flow, since there is nothing to claim. Tell the user this connection isn't permitted to create workspaces, and offer them the two ways forward: create the workspace themselves in the browser and grant access with a claim code, or re-authorize this connection with "Let this app create new workspaces" enabled (`/console/connected-apps` can also flip it on an existing connection). +For brand-new workspaces, `pad_workspace.create` with `{name: ""}` (and optional `template`) creates the workspace AND auto-adds it to this connection's allow-list in one call — no claim code needed. Only works when the connection currently carries the "may create workspaces" grant — granted at consent time, or enabled later on the connections page. If the grant is present and set to false, the create is **refused with a 403 and no workspace is made** — retrying won't help and neither will the claim flow, since there is nothing to claim. Tell the user this connection isn't permitted to create workspaces, and offer them the two ways forward: create the workspace themselves in the browser and grant access with a claim code, or re-authorize this connection with "Let this app create new workspaces" enabled (`/console/connected-apps` can also flip it on an existing connection). ## New workspace: offer to set it up diff --git a/internal/mcp/version.go b/internal/mcp/version.go index 55a97e4e3..36ce7c938 100644 --- a/internal/mcp/version.go +++ b/internal/mcp/version.go @@ -623,8 +623,13 @@ const CmdhelpVersion = "0.1" // - "0.26" — current. IDEA-2756: `pad_workspace.action=create` is now // REFUSED with a 403 when the calling OAuth connection's grant has // `may_create_workspaces=false`. Previously that flag gated only the -// post-creation auto-add, so the create succeeded and the agent was -// handed a workspace it could not then see. +// post-creation auto-add, so the create succeeded — and on a +// connection with an EXPLICIT workspace allow-list the agent was +// handed a workspace it could not then see. (Not universal: a +// connection with `all_current_workspaces=true` is not gated per +// slug, so it could see what it made. The consent mismatch is the +// constant across both; the invisibility was only its most visible +// symptom.) // // BEHAVIOR bump, not a shape one — no tool name, action enum, or // parameter changed. Same grounds as v0.25 (activation destination), diff --git a/internal/server/handlers_workspace_create_consent_test.go b/internal/server/handlers_workspace_create_consent_test.go index 7fc5af7b2..245dbef3b 100644 --- a/internal/server/handlers_workspace_create_consent_test.go +++ b/internal/server/handlers_workspace_create_consent_test.go @@ -16,24 +16,40 @@ import ( // // The OAuth consent screen's `may_create_workspaces` checkbox used to gate // only maybeAutoAddCreatorConnection's allow-list insert: a connection whose -// user left the box unticked could still create workspaces, it just could not -// then see them. The ruling: the checkbox is a permission on whether the +// user left the box unticked could still create workspaces — invisible to it +// when the connection carried an explicit allow-list, visible when it carried +// the all_current_workspaces wildcard, and unconsented either way. The ruling: the checkbox is a permission on whether the // connected token may CREATE, so an unset flag refuses outright with a 403, // mirroring handleAuditLog's consent refusal (BUG-2102). // -// Two doors mint a workspace under the caller's account, both guarded here by -// the shared requireWorkspaceCreationConsent helper: +// Two doors let an OAuth-bound caller mint a workspace, both guarded here by +// the shared requireWorkspaceCreationConsent helper. (Not every path to +// store.CreateWorkspace: autoCreateWorkspace mints one at signup, outside this +// gate and deliberately so — no OAuth connection is in context there.) // // - POST /api/v1/workspaces — handleCreateWorkspace // - POST /api/v1/workspaces/import — handleImportWorkspace, which reaches // CreateWorkspace via store.ImportWorkspace, and whose gzip Content-Type // branch dispatches to handleImportWorkspaceBundle from inside itself. // -// Every refusal leg asserts the WRONG behaviour's observable consequence — that -// no workspace by that slug exists afterwards — and not merely the status code -// (CONVE-12). A guard that 403s AFTER store.CreateWorkspace would pass a -// status-only assertion while leaving the workspace behind, which is the exact -// failure this endpoint's users would report. +// Every refusal leg whose request COULD have created something asserts the +// wrong behaviour's observable consequence — that no workspace of that NAME +// exists afterwards — and not merely the status code (CONVE-12). A guard that +// 403s AFTER store.CreateWorkspace would pass a status-only assertion while +// leaving the workspace behind, which is the exact failure this endpoint's +// users would report. +// +// The two ORDERING legs are the deliberate exception, and adding the assertion +// there would be worse than omitting it: a malformed body and an empty name are +// rejected before creation under every guard placement, so "no such workspace +// exists" is true of broken and working code alike. Those legs discriminate on +// the STATUS instead — 400 if the gate is late, 403 if it is early — which is +// the only signal that separates the two placements. (Round 8 flagged the +// original blanket claim; the first fix added the vacuous assertions, which is +// the failure that claim was warning about.) +// +// By NAME rather than by slug: the name is what the request supplies verbatim, +// while the slug is derived differently on the two paths (see lookupByName). // // Everything drives the real router via srv.ServeHTTP rather than calling the // handler directly, so the tests have an opinion about the ROUTE and not only @@ -58,9 +74,12 @@ func newConsentEnv(t *testing.T, mayCreate bool) *consentEnv { if err != nil { t.Fatalf("CreateUser: %v", err) } - // The PAT needs a workspace to bind to, and the handler under test is the - // one that makes workspaces — so seed this one through the store directly. - // It is the token's home, never the workspace any test asserts about. + // A PAT does not require a workspace (CreateAPIToken takes WorkspaceID as + // optional), but binding one keeps this fixture close to a real CLI token + // and gives the auth chain something ordinary to resolve. Seeded through + // the store directly because the handler under test is the one that makes + // workspaces. It is the token's home, never the workspace any test asserts + // about. home, err := srv.store.CreateWorkspace(models.WorkspaceCreate{ Name: "Consent Home", OwnerID: user.ID, }) @@ -91,9 +110,16 @@ func newConsentEnv(t *testing.T, mayCreate bool) *consentEnv { } // do issues a request with Bearer PAT auth. When requestID is non-empty the -// request context is decorated with an OAuth grant identity AFTER TokenAuth -// runs, which is what makes the caller look like an OAuth-bound MCP session. -// Mirrors handlers_oauth_claim_test.go's doClaim. +// request context is decorated with an OAuth grant identity, which is what +// makes the caller look like an OAuth-bound MCP session. +// +// The wrapper sets the identity BEFORE srv.ServeHTTP, so it is in place before +// TokenAuth runs — and survives, because nothing on the /api/v1 chain writes +// that context key. Only MCPBearerAuth does, and it is mounted on /mcp alone. +// (The sibling helper this is modelled on, handlers_oauth_claim_test.go's +// doClaim, describes the same wrapper as decorating the context "AFTER +// TokenAuth runs"; that ordering claim is wrong for both, and the mechanism +// works for the reason given above instead.) func (e *consentEnv) do(method, path, contentType string, body []byte, requestID string) *httptest.ResponseRecorder { var req *http.Request if body != nil { @@ -128,9 +154,10 @@ func (e *consentEnv) createWorkspace(name, requestID string) *httptest.ResponseR // lookupByName finds a workspace by its NAME rather than its slug. Name is // what the request supplies verbatim; slug is derived, and the derivation // differs between the create path (normalizeWorkspaceInput) and the import -// path (which takes the ?name= override as the slug and lets CreateWorkspace -// normalize it). Asserting on the name keeps these tests about the guard -// instead of about slugification. +// path, which passes the ?name= override through as the SLUG — and +// CreateWorkspace slugifies only when the supplied slug is empty, so an +// imported workspace keeps that value verbatim. Asserting on the name keeps +// these tests about the guard instead of about slug derivation. func (e *consentEnv) lookupByName(t *testing.T, name string) *models.Workspace { t.Helper() all, err := e.srv.store.ListWorkspaces() @@ -419,4 +446,5 @@ func TestCreateWorkspace_RefusalMessageNamesTheRemedy(t *testing.T) { if !strings.Contains(strings.ToLower(body), "re-authorize") { t.Errorf("refusal message does not tell the caller how to fix it: %s", body) } + e.mustNotExist(t, "Message WS") } diff --git a/internal/server/handlers_workspaces.go b/internal/server/handlers_workspaces.go index 133811a76..92f743b23 100644 --- a/internal/server/handlers_workspaces.go +++ b/internal/server/handlers_workspaces.go @@ -291,10 +291,18 @@ func (s *Server) handleReorderWorkspaces(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) } -// requireWorkspaceCreationConsent gates every endpoint that MINTS a -// workspace under the caller's account on the calling OAuth connection's -// `may_create_workspaces` grant. Returns true when the request may -// proceed; on false it has already written the response. +// requireWorkspaceCreationConsent gates a request that MINTS a workspace +// on the calling OAuth connection's `may_create_workspaces` grant. +// Returns true when the request may proceed; on false it has already +// written the response. +// +// Two callers, which are the two endpoints an OAuth-bound caller can +// mint through: handleCreateWorkspace and handleImportWorkspace. NOT +// every path to store.CreateWorkspace — autoCreateWorkspace +// (handlers_cloud.go) mints a workspace during registration, bootstrap +// and OAuth-login, and is deliberately outside this gate: it runs at +// signup with no OAuth connection in context, provisioning the user's +// own first workspace rather than acting for a connected app. // // Ruled by Dave on IDEA-2756 (2026-08-26): the consent screen's "may // create workspaces" checkbox is a permission on whether the connected @@ -316,10 +324,13 @@ func (s *Server) handleReorderWorkspaces(w http.ResponseWriter, r *http.Request) // - Not an OAuth grant (PAT, CLI session token, local stdio — no // request_id in context). Creation rides on ordinary account // authority; this flag has no opinion about it. -// - ErrOAuthConnectionNotFound — a pre-Phase-C grant whose row has -// not been backfilled yet. The backfill mints these with -// may_create_workspaces ON (oauth_connections_backfill.go), so -// allowing here is that same default applied early, not a gap. +// - ErrOAuthConnectionNotFound — no connection row for this grant. +// The expected cause is a pre-Phase-C grant not yet backfilled, and +// the backfill mints those with may_create_workspaces ON +// (oauth_connections_backfill.go), so allowing here is that same +// default applied early rather than a gap. Stated as the expected +// cause and not the only one, because the code cannot tell them +// apart: any missing row takes this branch. // Note the deliberate asymmetry with maybeAutoAddCreatorConnection, // which treats not-found as "no auto-add": that path is declining a // convenience, this one would be inventing a refusal. @@ -795,8 +806,12 @@ func (s *Server) handleImportWorkspace(w http.ResponseWriter, r *http.Request) { // may_create_workspaces grant decides it. Ruled with the create-side // gate on IDEA-2756. It sits ABOVE the Content-Type dispatch so it // covers the tar.gz bundle path too — handleImportWorkspaceBundle is - // reachable only from here, so this is its only door — and above the - // 64 MiB body read, so a refused caller never uploads a bundle. + // reachable only from here, so this is its only door — and above + // either body read, so a refused caller never uploads anything. The + // two reads have different bounds (the JSON path's 64 MiB + // decodeJSONWithLimit, the bundle path's own configurable and much + // larger limit); the gate precedes both, which is the property that + // matters here. // // Reachability, stated precisely because the create-side gate's is // different: NO OAuth-bound caller can reach this handler today. The From 952b0ff1261ed6d809e818f733bd2f16c3e3909d Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 26 Aug 2026 15:23:13 +0000 Subject: [PATCH 7/7] ci: re-trigger CI after a GitHub startup_failure (IDEA-2756) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No code change. The Go job on cb47c763 failed on BUG-2786 (the recurring internal/events subscribe-confirm guard, which fails by asserting its own premise: 'the acknowledgement never landed before the mark; this test could not have discriminated'). CONVE-11 owes that failure a re-run before it can be called a flake. rerun-failed-jobs produced attempt 2 = startup_failure with the Go job stuck in 'queued' — a GitHub infrastructure fault, not a test result — after which the run refuses further retries ('This workflow run cannot be retried'). The CI workflow has no workflow_dispatch trigger, so a push is the only way to get a fresh run. Evidence the failure is unrelated to this branch, gathered before re-running rather than after: the branch touches 0 files under internal/events (11 files total, none in that package), and the parent tip c6818500 had Go: SUCCESS with the only non-comment Go difference being one added assertion in this branch's own test file. Go (PostgreSQL) also passed on cb47c763, exercising the same package.