From cb47c76351021450f02412191c970ced2adea5ca Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 26 Aug 2026 14:54:37 +0000 Subject: [PATCH] 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