From 8c8deeeb898722009f94b3bf10a3eedce4515bc2 Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 6 Oct 2026 12:16:32 -0400 Subject: [PATCH] fix: an invite asks an existing user instead of adding them; decline; CLI invitations (BUG-2136 U1) (#1831) * test(server): an invite asks an existing user; re-invite replaces; decline by id and by code (BUG-2136, red) * fix(server): an invite asks an existing user instead of adding them; a re-invite replaces the pending invitation; decline by id and by code (BUG-2136) * feat(cli,mcp): pad workspace invitations/accept/decline; invite output says pending; ToolSurfaceVersion 0.66 (BUG-2136) * test(e2e): invitees accept their invitation before the specs use the membership (BUG-2136) * test(store): concurrent re-invites of one address leave one pending invitation (BUG-2136) * test(store): an accept that loses to a decline adds no membership (BUG-2136, codex r1, red) * test(server): an unverified invitee can decline by code, as they can accept (BUG-2136, codex r1, red) * fix: an accept that loses to a decline or replace adds no membership; decline by code passes the verified-email gate; honest oracle comment (BUG-2136, codex r1) * fix(server): the verified-email exemption matches exactly /invitations/{code}/accept|decline (BUG-2136, codex r2) * test(server): drop the plan-limit harness's now-unused newUser (BUG-2136) --- README.md | 4 +- cmd/pad/cmd_workspace.go | 74 ++++- cmd/pad/groups.go | 3 + .../testdata/mcp_wire/remote_initialize.json | 4 +- .../testdata/mcp_wire/stdio_initialize.json | 4 +- internal/cli/client.go | 45 +++ internal/mcp/instructions.md | 2 +- internal/mcp/version.go | 15 +- internal/models/activity.go | 41 +-- .../server/bug2136_invite_consent_test.go | 261 ++++++++++++++++++ .../bug3348_unverified_email_access_test.go | 17 +- internal/server/handlers_members.go | 93 +++++-- internal/server/handlers_my_invitations.go | 30 ++ .../server/handlers_plan_limit_race_test.go | 49 +--- .../server/handlers_workspace_cap_test.go | 14 +- internal/server/middleware_verified_email.go | 16 +- .../server/middleware_verified_email_test.go | 39 +++ internal/server/my_invitations_test.go | 20 +- internal/server/server.go | 2 + .../server/workspace_access_notify_test.go | 16 +- .../bug2136_reinvite_concurrency_test.go | 95 +++++++ internal/store/workspace_members.go | 35 ++- web/e2e/lib/permission-walk.ts | 20 ++ web/e2e/workspace-access-live.spec.ts | 7 + 24 files changed, 787 insertions(+), 119 deletions(-) create mode 100644 internal/server/bug2136_invite_consent_test.go create mode 100644 internal/store/bug2136_reinvite_concurrency_test.go diff --git a/README.md b/README.md index 1b7d1430a..1d246ccc8 100644 --- a/README.md +++ b/README.md @@ -390,7 +390,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.65)** — ten resource × action tools plus `pad_set_workspace`. Undeclared input keys are rejected with a structured error rather than silently dropped, and field values are typed against the collection schema on the server: +**Tool catalog (v0.66)** — ten resource × action tools plus `pad_set_workspace`. Undeclared input keys are rejected with a structured error rather than silently dropped, and field values are typed against the collection schema on the server: | Tool | Actions | |---|---| @@ -414,7 +414,7 @@ Plus resources at `pad://workspaces`, `pad://workspace/{ws}/dashboard`, and `pad://_meta/version`. **Stability contract.** The catalog is versioned -(`tool_surface_version: "0.65"`) and advertised in the initialize handshake and at +(`tool_surface_version: "0.66"`) and advertised in the initialize handshake and at `pad://_meta/version`, so external agents can pin against it; the per-version changelog lives in [`internal/mcp/version.go`](internal/mcp/version.go). Errors come back as structured envelopes (`{error: {code, message, hint, ...}}`) from a diff --git a/cmd/pad/cmd_workspace.go b/cmd/pad/cmd_workspace.go index 6bd109b9d..2c35c5a5e 100644 --- a/cmd/pad/cmd_workspace.go +++ b/cmd/pad/cmd_workspace.go @@ -212,13 +212,15 @@ func inviteCmd() *cobra.Command { green := color.New(color.FgGreen).SprintFunc() + // A server before BUG-2136 still adds an existing account directly. if added, ok := result["added"].(bool); ok && added { name, _ := result["name"].(string) role, _ := result["role"].(string) fmt.Printf("%s Added %s (%s) as %s\n", green("✓"), name, email, role) } else { role, _ := result["role"].(string) - fmt.Printf("%s Invitation created for %s (%s)\n", green("✓"), email, role) + fmt.Printf("%s Invited %s (%s): pending until they accept\n", green("✓"), email, role) + fmt.Printf(" They will see it in Pad (the + menu), or with: pad workspace invitations\n") if joinURL, ok := result["join_url"].(string); ok && joinURL != "" { fmt.Printf(" Share this link: %s\n", joinURL) } else { @@ -257,6 +259,76 @@ func joinCmd() *cobra.Command { } } +// myInvitationsCmd lists the pending invitations addressed to the signed-in +// account (BUG-2136): what an invitee answers with accept or decline. +func myInvitationsCmd() *cobra.Command { + return &cobra.Command{ + Use: "invitations", + Short: "List workspace invitations waiting for your answer", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + client, _ := getClient() + invs, verified, err := client.ListMyInvitations() + if err != nil { + return err + } + if formatFlag == "json" { + return cli.PrintJSON(map[string]any{"invitations": invs, "email_verified": verified}) + } + if !verified { + fmt.Println("Your email address is not verified, so no invitations are listed. Verify it, or use the link in the invitation email.") + return nil + } + if len(invs) == 0 { + fmt.Println("No pending invitations.") + return nil + } + w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0) + fmt.Fprintln(w, "ID\tWORKSPACE\tROLE\tINVITED BY") + for _, inv := range invs { + fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", inv.ID, inv.WorkspaceName, inv.Role, inv.InvitedByName) + } + w.Flush() + fmt.Println("\nAnswer with: pad workspace accept or pad workspace decline ") + return nil + }, + } +} + +func acceptInvitationCmd() *cobra.Command { + return &cobra.Command{ + Use: "accept ", + Short: "Accept a workspace invitation from pad workspace invitations", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + client, _ := getClient() + role, slug, err := client.AcceptMyInvitation(args[0]) + if err != nil { + return fmt.Errorf("failed to accept invitation: %w", err) + } + green := color.New(color.FgGreen).SprintFunc() + fmt.Printf("%s Joined %s as %s\n", green("✓"), slug, role) + return nil + }, + } +} + +func declineInvitationCmd() *cobra.Command { + return &cobra.Command{ + Use: "decline ", + Short: "Decline a workspace invitation from pad workspace invitations", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + client, _ := getClient() + if err := client.DeclineMyInvitation(args[0]); err != nil { + return fmt.Errorf("failed to decline invitation: %w", err) + } + fmt.Println("Invitation declined.") + return nil + }, + } +} + // workspaceRestoreCmd un-soft-deletes a workspace by slug, provided it is // still inside the 30-day restore window (owner-only server-side). func workspaceRestoreCmd() *cobra.Command { diff --git a/cmd/pad/groups.go b/cmd/pad/groups.go index 8eb23d3e0..462fc9a84 100644 --- a/cmd/pad/groups.go +++ b/cmd/pad/groups.go @@ -69,6 +69,9 @@ func workspaceCmd() *cobra.Command { membersCmd(), inviteCmd(), joinCmd(), + myInvitationsCmd(), + acceptInvitationCmd(), + declineInvitationCmd(), workspaceDeletedCmd(), workspaceRestoreCmd(), storageCmd(), diff --git a/cmd/pad/testdata/mcp_wire/remote_initialize.json b/cmd/pad/testdata/mcp_wire/remote_initialize.json index d836ea095..8e9faef1f 100644 --- a/cmd/pad/testdata/mcp_wire/remote_initialize.json +++ b/cmd/pad/testdata/mcp_wire/remote_initialize.json @@ -8,7 +8,7 @@ }, "padToolSurface": { "tool_surface_stable": true, - "version": "0.65" + "version": "0.66" } }, "prompts": {}, @@ -21,5 +21,5 @@ "name": "pad-mcp", "version": "" }, - "instructions": "Pad is a project tracker for developers and AI agents — issues (TASK, BUG), plans (PLAN), ideas (IDEA), docs (DOC), conventions, comments, and dependencies. Use this server when a user mentions:\n\n- Issue refs like `TASK-5`, `BUG-12`, `PLAN-3`, `IDEA-8` — they are stable, human-readable IDs and the canonical way to address items. Use them, never slugs; they are what audit trails and PR titles show.\n- Tasks / issues / items / plans / progress / \"what's on my plate\" / \"what to work on next\" / standup / changelog / retrospective.\n- Project conventions, decision records, or \"how should this team do X.\"\n\nIf the user is asking general code questions with no project-management thread, you don't need this server.\n\n## Tool surface (v0.65)\n\nTen resource × action tools, plus `pad_set_workspace`. Eleven tools total.\n\nInputs are validated strictly: an undeclared top-level key is rejected with a structured `validation_failed` naming it, never accepted and silently dropped.\n\n- `pad_item` — Items: create / update / delete / get / list / move / restore / link / unlink / deps / star / unstar / starred / comment / list-comments / edit-comment / delete-comment / backlinks / bulk-update / note / decide / export / import / history / remind / ack-reminder / claim / release. On create/update, field values may be passed as a `fields` OBJECT (the same shape reads return, e.g. `{\"status\":\"done\",\"effort\":\"l\"}`) — it merges into the same path as the dedicated params and `field: [\"key=value\"]`; the same key in two places with CONFLICTING values is refused, not silently resolved. `list` accepts `unparented: true` to keep items with no parent or implements relationship (mutually exclusive with `parent`). `list` results are SUMMARY-shaped by default on both transports — no content bodies; pass `full: true` for complete bodies (token-expensive), or prefer `get` for a single item's body. `update` field writes are a server-side field-level merge (only the keys you set change); pass `expected_seq` for optimistic concurrency — the `seq` you last read, which is on every item AND every list summary — and a stale value fails with a structured 409 `update_conflict` whose details carry `actual_seq` to retry with. PREFER IT over `expected_updated_at`, which cannot tell two writes inside one second apart; that one still works, and if you send both, seq decides. A token cannot see a browser tab's unsaved edits, so `content` sent WITH one is refused with 409 `content_pending_flush` while any exist; re-reading will not clear it. Wait for the editor to save, or pass `overwrite_pending_edits: true` to replace them. `content_state: \"superseded_set_aside\"` is different and wins when both apply: an editor upgrade set aside edits no tab can restore, so EVERY content write is refused (token or not; `details.set_aside_rows`) and waiting never clears it. Tell the user, or pass `overwrite_pending_edits: true` to discard them. Writing `content` while a browser tab has the item open routes the markdown through that live document, and the stored copy is updated only by a later collab-snapshot flush — usually from the tab that applied it, though any such write updates the row, and nothing guarantees one happens: the 200 echoes the content you SENT and carries `warnings.content_outcome: \"applied_pending_flush\"`, which describes that WRITE — the content went to the document, not the row — rather than being a live reading of the row, which a concurrent flush may already have updated. A `get` (or a `list` with `full: true` — a default list carries no content at all) before such a flush lands reads the stored copy and answers with the PREVIOUS content, which is the lag rather than a failed write, so do not re-send on the strength of it. You do not have to infer that from the write, and you will also see it when someone ELSE's tab holds the unflushed edits and you wrote nothing: a read that serves a body the server knows is behind the live document carries `content_state: \"applied_pending_flush\"` — same value as the write warning, different question (what the ROW is, not what your REQUEST did). Treat it as re-read later, never as re-send, and do not wait on it in a loop: the server normally recovers the stored copy itself about a minute after the last tab leaves the item, but that is not guaranteed (a recovery can fail, and an operator can turn it off), and otherwise the row catches up only when a tab next opens the item. The stored form may also not be byte-identical to what you sent, since the markdown round-trips through the editor (bullet markers, emphasis characters, list numbering and blank-line runs normalise; already-canonical markdown survives unchanged), so compare on meaning rather than bytes. `move` changes an item's COLLECTION within its workspace: system metadata (implementation notes, decision log, linked PR) survives it, and any field value the target schema has no home for is dropped AND reported in the move's activity entry — check there rather than assuming a move is lossless. A carried unique-field collision is dropped, named in `warnings.not_unique`. Three system keys — `implementation_notes`, `decision_log`, `convention` — cannot be set through `field` on update or move; the call is refused with `validation_failed`, naming the key and the write path that does maintain it (`note`, `decide`, and library activation respectively). `github_pr` is refused as well (BUG-2696): a `field` value arrived as a string, so the PR data was stored double-encoded and no link appeared. Its writer is `pad github link` / `unlink`, which needs a local git checkout, and there is no PR-link action on this surface yet, so treat linking a PR as something to hand to a human rather than a call to retry. On `create` they are refused too (BUG-3163), with the same `validation_failed`: create the item first, then use `note` / `decide` on it; convention metadata comes from library activation. `history` returns read-only item version metadata (newest-first), bounded to the NEWEST 50 versions by default (max 300 — pass `limit` to change the window); pass `full: true` to include each version's resolved content body (token-expensive). There is no `offset`: versions are stored as reverse patches, so only a newest-end window is cheap to reconstruct. Unassign or detach with the `clear_assigned_user` / `clear_agent_role` / `clear_parent` booleans; an empty `assign` / `role` / `parent` does NOT clear. `remind` / `ack-reminder`: a fired reminder shows in `pad_project` next / ready (the delivery path on an instance with no webhook) until acknowledged, and one on a completed item is hidden there. `claim` / `release`: a loser's 409 is `lease_held`, and a lease bumps neither `updated_at` nor `seq`. `edit-comment` / `delete-comment` take a `comment_id` from `list-comments` (whose rows carry `edited`); only a comment's author may edit it. Correct a comment in place rather than appending a correction. Deleting a comment that has replies leaves a placeholder (`deleted: true`, empty body) so the replies keep their parent; editing, replying to or reacting to it answers 409 `comment_deleted`. (Two older forms still work and are not deprecated: `field: [\"assigned_user_id=\"]` on either transport, and a direct `assigned_user_id: \"\"` param over remote `/mcp` only — prefer the boolean, which is the only one this schema advertises.)\n\n**App attribution (v0.65).** An item an installed app created, a comment an app wrote and a history row an app's write made carry `via_app` (the install id) and `via_app_name` (the app's name). On an item they name the app that CREATED it, not the last writer. They are absent for anything a person or an agent wrote.\n\n**Relation fields (v0.31).** A `relation` value may be a UUID, an issue ref (`COLO-3`), or the target item's EXACT TITLE — in that order, so an item literally titled `COLO-3` is unreachable by title while the ref resolves. Title matching is scoped to the collection the field declares: a title that is unique only workspace-wide is REFUSED, naming the collection searched, and a title matching two or more items inside the declared collection is refused as `ambiguous` rather than `not_found` — it matched too much, not too little. Only items YOU can see count towards any of that, so a match you have no access to never changes your answer.\n\nReads carry a `relation_targets` member beside `fields`: field key → `{id, ref, title}`, so you can render a relation without a request per value. `fields` still holds the canonical id, which is what you write back. An entry with an `id` and NO `ref`/`title` means the target is gone **or** you may not see it — the two are deliberately indistinguishable, so do not render it as either \"deleted\" or \"hidden\"; \"unavailable\" is the honest word. An entry with `stored_as_text: true` differs: the stored value is not UUID-shaped, so it was never an id (an imported title, legacy text). Render it as text, not as unavailable.\n\n**`multi_relation` fields (v0.33).** A field declared `multi_relation` holds an ORDERED LIST of references — `[\"\u003cuuid\u003e\", \"COLO-3\", \"Red\"]` — and every element resolves through the same UUID → ref → exact-title ladder, with the same collection scoping. Declare one through the `fields` DSL as `owners:multi_relation:people`: the third part is the TARGET COLLECTION, and omitting it is refused at parse time rather than building a field no write can ever satisfy.\n\nFour rules differ from a scalar `relation`, and none of them is guessable from the scalar behaviour:\n\n- **There is exactly one stored form for \"no references\": the key ABSENT.** Send `[]` and the write normalises it to that. A `required` `multi_relation` means at least one element that RESOLVED, so `[]` and an absent key both refuse.\n- **An empty or whitespace-only ELEMENT is refused**, naming its index — it is not skipped the way an empty scalar value is. Remove the element rather than blanking it.\n- **Order is part of the value.** `[\"a\",\"b\"]` and `[\"b\",\"a\"]` are different values, and a write preserves the order you sent.\n- **Duplicates are refused**, with reason `duplicate_referent`, naming the SECOND occurrence. Two elements can be different strings naming one item — a UUID and a ref, a ref and a title — so this is decided after resolution, and the two forms you sent may look nothing alike.\n\nIf ANY element fails, the whole write is refused and nothing is stored — never a partial list. On a cross-workspace copy the value is dropped WHOLE and reported once in `warnings.dropped_fields`, so an import can never change an element count.\n\nIn `relation_targets`, a `multi_relation` key carries a JSON **ARRAY** of the same `{id, ref, title}` objects, in stored order and one per stored element — including an `id`-only entry for an element that names nothing, so a position in the list lines up with the same position in `fields`. A scalar key is unchanged: still a single object, byte-identical to v0.31. Decide which shape to expect from the field's declared type, not by probing.\n\n- `pad_workspace` — Workspaces: list / members / invite / storage / audit-log / create / claim / deleted / restore.\n- `pad_collection` — Collections: list / create / update / delete.\n- `pad_project` — Project intelligence: dashboard / next / ready / stale / standup / changelog / report / activity. Use `ready` for the actionable backlog and `stale` for items needing attention; `activity` to catch up on what other agents/users changed since you last worked (non-streaming feed with item refs + change details).\n- `pad_role` — Agent roles: list / create / update / delete.\n- `pad_search` — Full-text search across items: query.\n- `pad_playbook` — Invokable procedures: list / get / run / match. Use `run` to bind args against a playbook's declared spec and get the rendered body back; side-effect-free. `run` refuses a playbook whose status isn't `active` (a draft still being authored) with a `playbook_not_active` error — pass `allow_draft: true` to override. Both `run` and `get` echo the playbook's `status`. Both may also carry `content_state` (`\"applied_pending_flush\"`, or `\"superseded_set_aside\"` as under `pad_item`), meaning the BODY you are about to execute is behind the item's live collaborative document — an editor holds unflushed edits, so the steps may be superseded. It is a reason to re-read or to ask, not a reason to refuse to run; the body is still the best the server has. `match` finds a playbook from text.\n- `pad_library` — Convention + playbook library (the global catalog of pre-built entries workspaces activate): list / get / activate.\n- `pad_attachment` — Read-only attachment metadata: list / show. `list` enumerates a workspace's attachments (filter by item / category / collection / attached / unattached); `show` returns one attachment's MIME, size, filename, and ETag via a HEAD request. Uploading and general file downloads stay CLI-only; bounded image bytes are available through the attachment resource below.\n- `pad_meta` — Server introspection: server-info / version / tool-surface / bootstrap. The `bootstrap` action returns one-shot workspace context (user + collections + always-on conventions + a metadata-only `convention_index` of every active convention + roles + playbook metadata + dashboard + recent activity).\n- `pad_set_workspace` — Load workspace context; response embeds the bootstrap blob so you load context in one call. Whether it also pins a session default: see Workspace context below. Takes `workspace: \u003cslug\u003e` only (no `action`).\n\nFor the ten resource × action tools, always pass `action` as a top-level field. Per-action required parameters are documented in each tool's description.\n\n## Resources are cheaper than tool calls\n\nRead these directly when you need workspace state:\n\n- `pad://workspace/{ws}/dashboard` — computed project overview (active items, plans, attention, suggested next).\n- `pad://workspace/{ws}/collections` — collection types + schemas.\n- `pad://workspace/{ws}/items` — list of all items (use `pad_item.action: list` for filtering).\n- `pad://workspace/{ws}/items/{ref}` — single item rendered as markdown.\n- `pad://workspace/{ws}/attachments/{id}` — image attachment as a bounded base64 `thumb-md` resource; rejects non-images and image bytes over 1 MiB (pre-base64).\n- `pad://workspace/{ws}/bootstrap` — one-shot workspace context (same payload as `pad_meta.action: bootstrap` and `pad_set_workspace`'s embedded response).\n- `pad://_meta/version` — server version + stability tiers.\n\nResources support host-side prefetch — if the host can fetch them once at session start, you don't pay per turn.\n\nLoad the bootstrap resource once per workspace and reuse its collections, conventions, roles, and playbook metadata. Refresh it after switching workspaces, after changing those configuration surfaces, when Pad reports stale schema/context, or when the user asks. For changing work state, read the affected item or use a targeted project tool instead of fetching the whole bootstrap again.\n\n## Workspace context\n\nEvery action that operates within a workspace accepts an optional `workspace` parameter. Resolution order:\n\n1. Explicit `workspace` argument on the call (always wins).\n2. On a single-user local server only: the session default set via `pad_set_workspace`.\n3. On a single-user local server only: the CWD-linked workspace from `.pad.toml`.\n\nA multi-user/remote server does **not** persist a session default — pass `workspace` explicitly on every call. If none resolves, the action returns a structured `no_workspace` error with `available_workspaces`.\n\n## Update flow: read first, then patch\n\nFor `pad_item.action: update`, the server merges your patch with the item's current state. Pass only the fields you want to change. When changing `status`, ALWAYS include a `comment` explaining why — it builds the audit trail that helps the team understand history.\n\n## One error code you must not retry\n\nErrors come back as a structured envelope with a `code`. Most failures are worth a retry after you fix something, and a `server_error` may be transient. `stored_state_unreadable` is neither.\n\nIt means the ITEM'S STORED DATA cannot be decoded — your input was fine. Today it fires when `note` / `decide` would append to an `implementation_notes` / `decision_log` value that is not a list of entries. The operation is refused precisely because completing it would overwrite that value. The refusal is fully deterministic, so retrying — immediately or after a backoff — fails identically every time and changes nothing.\n\nWhen you see it: tell the user which item is affected, and stop. Whether you can inspect the bad value depends on which layer is broken — if the item's whole `fields` blob fails to parse, `action: get` hands it back as a raw string; if one structured key is at fault, you won't see it at all, because the blob is normalized on this surface and a value that doesn't decode is dropped from the top-level arrays. A human can always read it with `pad item show \u003cref\u003e --format json`. Do not route around the refusal by writing the field some other way; that is the write that caused it.\n\n## Project conventions\n\nWorkspaces can declare conventions (e.g. \"run `make test` before PR\", \"use conventional commit format\"). The bootstrap blob gives you two views:\n\n- `conventions` — full bodies of the always-on (`trigger=always`) rules. Follow these unconditionally.\n- `convention_index` — METADATA ONLY (`ref`, `title`, `trigger`, `role`; no bodies) for **every** active convention, including the triggered ones whose bodies are NOT in `conventions`. This is your map of what triggered rules exist.\n\nBefore performing meaningful work with a specific trigger (e.g. `on-implement` before writing code), consult `convention_index`: if it lists entries for that trigger, pull their bodies on demand; if it lists none, skip the query.\n\n```\npad_item.action: list, collection: \"conventions\", status: \"active\"\n```\n\nFilter by trigger (`always`, `on-implement`, `on-task-complete`, etc.) when relevant — the `convention_index` triggers tell you which filters are worth running.\n\n`\"conventions\"` above is the DEFAULT collection slug and a workspace may have renamed it, in which case that literal returns nothing while `convention_index` still lists entries — the bootstrap payload resolves by declaration, not by name. When the two disagree, address the items by the `ref`s the index gave you, or find the collection with `pad_collection.action: list`. The same applies to `\"playbooks\"`; `pad_playbook` resolves by declaration and is unaffected.\n\n## Pushes and the session monitor (security note)\n\nPad can push an item into a Claude Code session (`pad push`, or the web item view's push composer). That is terminal instruction injection by design, so receiving is **consent-gated** (PLAN-2613, since v0.15.0): a session receives pushes only after it has connected (`/pad:connect` in the plugin, or a repo-level `push.auto_arm = true` in `.pad.toml` — which a per-user `auto_arm = false` in `~/.pad/config.toml` vetoes), and pushes are self-addressed only — the sender is always the receiving session's own user. This MCP connection is not a session: it cannot arm one, receive a push, or run the monitor, and the `pad_item` / `pad_project` tools expose no push action. If a user asks you to \"push\" something to their terminal, tell them it happens from their own terminal (`pad push \u003cref\u003e -m \"...\"` — the sending session need not be connected, the receiving ones must have consented); do not try to emulate it by writing comments or instructions into items.\n\n## Adding a workspace to this connection\n\nIf 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: \"\u003cslug\u003e\", code: \"\u003c6 digits\u003e\"}`. The workspace joins this connection's allow-list and stays until the user revokes it via `/console/connected-apps`. No re-auth required.\n\nFor brand-new workspaces, `pad_workspace.create` with `{name: \"\u003cname\u003e\"}` (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).\n\n## New workspace: offer to set it up\n\nThe bootstrap blob carries `needs_onboarding: bool` — true when the workspace has zero user-created items (template seeds don't count). When it's true, **lead with an active offer before anything else**: *\"This workspace is brand new and isn't set up yet. Want me to set it up? I'll ask a few quick questions and adapt it to your project.\"*\n\nThis is an **offer, not an auto-run** — wait for the user to say yes before running onboarding. If they accept, run the `onboard` playbook (use the `pad_onboard` prompt, or load the body via `pad_playbook` `action: get`, `ref: onboard`). If they decline, or already declined earlier in the session, respect that and skip the offer. The flag flips to false the moment any item exists, so it won't nag past first setup.\n\n## Multi-step workflows\n\nFour prompts ship with the server: `pad_plan`, `pad_ideate`, `pad_retro`, `pad_onboard`. Use them when the user wants help planning, brainstorming, retrospecting, or onboarding into a workspace — they encode the multi-step Pad-aware playbook for each.\n" + "instructions": "Pad is a project tracker for developers and AI agents — issues (TASK, BUG), plans (PLAN), ideas (IDEA), docs (DOC), conventions, comments, and dependencies. Use this server when a user mentions:\n\n- Issue refs like `TASK-5`, `BUG-12`, `PLAN-3`, `IDEA-8` — they are stable, human-readable IDs and the canonical way to address items. Use them, never slugs; they are what audit trails and PR titles show.\n- Tasks / issues / items / plans / progress / \"what's on my plate\" / \"what to work on next\" / standup / changelog / retrospective.\n- Project conventions, decision records, or \"how should this team do X.\"\n\nIf the user is asking general code questions with no project-management thread, you don't need this server.\n\n## Tool surface (v0.66)\n\nTen resource × action tools, plus `pad_set_workspace`. Eleven tools total.\n\nInputs are validated strictly: an undeclared top-level key is rejected with a structured `validation_failed` naming it, never accepted and silently dropped.\n\n- `pad_item` — Items: create / update / delete / get / list / move / restore / link / unlink / deps / star / unstar / starred / comment / list-comments / edit-comment / delete-comment / backlinks / bulk-update / note / decide / export / import / history / remind / ack-reminder / claim / release. On create/update, field values may be passed as a `fields` OBJECT (the same shape reads return, e.g. `{\"status\":\"done\",\"effort\":\"l\"}`) — it merges into the same path as the dedicated params and `field: [\"key=value\"]`; the same key in two places with CONFLICTING values is refused, not silently resolved. `list` accepts `unparented: true` to keep items with no parent or implements relationship (mutually exclusive with `parent`). `list` results are SUMMARY-shaped by default on both transports — no content bodies; pass `full: true` for complete bodies (token-expensive), or prefer `get` for a single item's body. `update` field writes are a server-side field-level merge (only the keys you set change); pass `expected_seq` for optimistic concurrency — the `seq` you last read, which is on every item AND every list summary — and a stale value fails with a structured 409 `update_conflict` whose details carry `actual_seq` to retry with. PREFER IT over `expected_updated_at`, which cannot tell two writes inside one second apart; that one still works, and if you send both, seq decides. A token cannot see a browser tab's unsaved edits, so `content` sent WITH one is refused with 409 `content_pending_flush` while any exist; re-reading will not clear it. Wait for the editor to save, or pass `overwrite_pending_edits: true` to replace them. `content_state: \"superseded_set_aside\"` is different and wins when both apply: an editor upgrade set aside edits no tab can restore, so EVERY content write is refused (token or not; `details.set_aside_rows`) and waiting never clears it. Tell the user, or pass `overwrite_pending_edits: true` to discard them. Writing `content` while a browser tab has the item open routes the markdown through that live document, and the stored copy is updated only by a later collab-snapshot flush — usually from the tab that applied it, though any such write updates the row, and nothing guarantees one happens: the 200 echoes the content you SENT and carries `warnings.content_outcome: \"applied_pending_flush\"`, which describes that WRITE — the content went to the document, not the row — rather than being a live reading of the row, which a concurrent flush may already have updated. A `get` (or a `list` with `full: true` — a default list carries no content at all) before such a flush lands reads the stored copy and answers with the PREVIOUS content, which is the lag rather than a failed write, so do not re-send on the strength of it. You do not have to infer that from the write, and you will also see it when someone ELSE's tab holds the unflushed edits and you wrote nothing: a read that serves a body the server knows is behind the live document carries `content_state: \"applied_pending_flush\"` — same value as the write warning, different question (what the ROW is, not what your REQUEST did). Treat it as re-read later, never as re-send, and do not wait on it in a loop: the server normally recovers the stored copy itself about a minute after the last tab leaves the item, but that is not guaranteed (a recovery can fail, and an operator can turn it off), and otherwise the row catches up only when a tab next opens the item. The stored form may also not be byte-identical to what you sent, since the markdown round-trips through the editor (bullet markers, emphasis characters, list numbering and blank-line runs normalise; already-canonical markdown survives unchanged), so compare on meaning rather than bytes. `move` changes an item's COLLECTION within its workspace: system metadata (implementation notes, decision log, linked PR) survives it, and any field value the target schema has no home for is dropped AND reported in the move's activity entry — check there rather than assuming a move is lossless. A carried unique-field collision is dropped, named in `warnings.not_unique`. Three system keys — `implementation_notes`, `decision_log`, `convention` — cannot be set through `field` on update or move; the call is refused with `validation_failed`, naming the key and the write path that does maintain it (`note`, `decide`, and library activation respectively). `github_pr` is refused as well (BUG-2696): a `field` value arrived as a string, so the PR data was stored double-encoded and no link appeared. Its writer is `pad github link` / `unlink`, which needs a local git checkout, and there is no PR-link action on this surface yet, so treat linking a PR as something to hand to a human rather than a call to retry. On `create` they are refused too (BUG-3163), with the same `validation_failed`: create the item first, then use `note` / `decide` on it; convention metadata comes from library activation. `history` returns read-only item version metadata (newest-first), bounded to the NEWEST 50 versions by default (max 300 — pass `limit` to change the window); pass `full: true` to include each version's resolved content body (token-expensive). There is no `offset`: versions are stored as reverse patches, so only a newest-end window is cheap to reconstruct. Unassign or detach with the `clear_assigned_user` / `clear_agent_role` / `clear_parent` booleans; an empty `assign` / `role` / `parent` does NOT clear. `remind` / `ack-reminder`: a fired reminder shows in `pad_project` next / ready (the delivery path on an instance with no webhook) until acknowledged, and one on a completed item is hidden there. `claim` / `release`: a loser's 409 is `lease_held`, and a lease bumps neither `updated_at` nor `seq`. `edit-comment` / `delete-comment` take a `comment_id` from `list-comments` (whose rows carry `edited`); only a comment's author may edit it. Correct a comment in place rather than appending a correction. Deleting a comment that has replies leaves a placeholder (`deleted: true`, empty body) so the replies keep their parent; editing, replying to or reacting to it answers 409 `comment_deleted`. (Two older forms still work and are not deprecated: `field: [\"assigned_user_id=\"]` on either transport, and a direct `assigned_user_id: \"\"` param over remote `/mcp` only — prefer the boolean, which is the only one this schema advertises.)\n\n**App attribution (v0.65).** An item an installed app created, a comment an app wrote and a history row an app's write made carry `via_app` (the install id) and `via_app_name` (the app's name). On an item they name the app that CREATED it, not the last writer. They are absent for anything a person or an agent wrote.\n\n**Relation fields (v0.31).** A `relation` value may be a UUID, an issue ref (`COLO-3`), or the target item's EXACT TITLE — in that order, so an item literally titled `COLO-3` is unreachable by title while the ref resolves. Title matching is scoped to the collection the field declares: a title that is unique only workspace-wide is REFUSED, naming the collection searched, and a title matching two or more items inside the declared collection is refused as `ambiguous` rather than `not_found` — it matched too much, not too little. Only items YOU can see count towards any of that, so a match you have no access to never changes your answer.\n\nReads carry a `relation_targets` member beside `fields`: field key → `{id, ref, title}`, so you can render a relation without a request per value. `fields` still holds the canonical id, which is what you write back. An entry with an `id` and NO `ref`/`title` means the target is gone **or** you may not see it — the two are deliberately indistinguishable, so do not render it as either \"deleted\" or \"hidden\"; \"unavailable\" is the honest word. An entry with `stored_as_text: true` differs: the stored value is not UUID-shaped, so it was never an id (an imported title, legacy text). Render it as text, not as unavailable.\n\n**`multi_relation` fields (v0.33).** A field declared `multi_relation` holds an ORDERED LIST of references — `[\"\u003cuuid\u003e\", \"COLO-3\", \"Red\"]` — and every element resolves through the same UUID → ref → exact-title ladder, with the same collection scoping. Declare one through the `fields` DSL as `owners:multi_relation:people`: the third part is the TARGET COLLECTION, and omitting it is refused at parse time rather than building a field no write can ever satisfy.\n\nFour rules differ from a scalar `relation`, and none of them is guessable from the scalar behaviour:\n\n- **There is exactly one stored form for \"no references\": the key ABSENT.** Send `[]` and the write normalises it to that. A `required` `multi_relation` means at least one element that RESOLVED, so `[]` and an absent key both refuse.\n- **An empty or whitespace-only ELEMENT is refused**, naming its index — it is not skipped the way an empty scalar value is. Remove the element rather than blanking it.\n- **Order is part of the value.** `[\"a\",\"b\"]` and `[\"b\",\"a\"]` are different values, and a write preserves the order you sent.\n- **Duplicates are refused**, with reason `duplicate_referent`, naming the SECOND occurrence. Two elements can be different strings naming one item — a UUID and a ref, a ref and a title — so this is decided after resolution, and the two forms you sent may look nothing alike.\n\nIf ANY element fails, the whole write is refused and nothing is stored — never a partial list. On a cross-workspace copy the value is dropped WHOLE and reported once in `warnings.dropped_fields`, so an import can never change an element count.\n\nIn `relation_targets`, a `multi_relation` key carries a JSON **ARRAY** of the same `{id, ref, title}` objects, in stored order and one per stored element — including an `id`-only entry for an element that names nothing, so a position in the list lines up with the same position in `fields`. A scalar key is unchanged: still a single object, byte-identical to v0.31. Decide which shape to expect from the field's declared type, not by probing.\n\n- `pad_workspace` — Workspaces: list / members / invite / storage / audit-log / create / claim / deleted / restore.\n- `pad_collection` — Collections: list / create / update / delete.\n- `pad_project` — Project intelligence: dashboard / next / ready / stale / standup / changelog / report / activity. Use `ready` for the actionable backlog and `stale` for items needing attention; `activity` to catch up on what other agents/users changed since you last worked (non-streaming feed with item refs + change details).\n- `pad_role` — Agent roles: list / create / update / delete.\n- `pad_search` — Full-text search across items: query.\n- `pad_playbook` — Invokable procedures: list / get / run / match. Use `run` to bind args against a playbook's declared spec and get the rendered body back; side-effect-free. `run` refuses a playbook whose status isn't `active` (a draft still being authored) with a `playbook_not_active` error — pass `allow_draft: true` to override. Both `run` and `get` echo the playbook's `status`. Both may also carry `content_state` (`\"applied_pending_flush\"`, or `\"superseded_set_aside\"` as under `pad_item`), meaning the BODY you are about to execute is behind the item's live collaborative document — an editor holds unflushed edits, so the steps may be superseded. It is a reason to re-read or to ask, not a reason to refuse to run; the body is still the best the server has. `match` finds a playbook from text.\n- `pad_library` — Convention + playbook library (the global catalog of pre-built entries workspaces activate): list / get / activate.\n- `pad_attachment` — Read-only attachment metadata: list / show. `list` enumerates a workspace's attachments (filter by item / category / collection / attached / unattached); `show` returns one attachment's MIME, size, filename, and ETag via a HEAD request. Uploading and general file downloads stay CLI-only; bounded image bytes are available through the attachment resource below.\n- `pad_meta` — Server introspection: server-info / version / tool-surface / bootstrap. The `bootstrap` action returns one-shot workspace context (user + collections + always-on conventions + a metadata-only `convention_index` of every active convention + roles + playbook metadata + dashboard + recent activity).\n- `pad_set_workspace` — Load workspace context; response embeds the bootstrap blob so you load context in one call. Whether it also pins a session default: see Workspace context below. Takes `workspace: \u003cslug\u003e` only (no `action`).\n\nFor the ten resource × action tools, always pass `action` as a top-level field. Per-action required parameters are documented in each tool's description.\n\n## Resources are cheaper than tool calls\n\nRead these directly when you need workspace state:\n\n- `pad://workspace/{ws}/dashboard` — computed project overview (active items, plans, attention, suggested next).\n- `pad://workspace/{ws}/collections` — collection types + schemas.\n- `pad://workspace/{ws}/items` — list of all items (use `pad_item.action: list` for filtering).\n- `pad://workspace/{ws}/items/{ref}` — single item rendered as markdown.\n- `pad://workspace/{ws}/attachments/{id}` — image attachment as a bounded base64 `thumb-md` resource; rejects non-images and image bytes over 1 MiB (pre-base64).\n- `pad://workspace/{ws}/bootstrap` — one-shot workspace context (same payload as `pad_meta.action: bootstrap` and `pad_set_workspace`'s embedded response).\n- `pad://_meta/version` — server version + stability tiers.\n\nResources support host-side prefetch — if the host can fetch them once at session start, you don't pay per turn.\n\nLoad the bootstrap resource once per workspace and reuse its collections, conventions, roles, and playbook metadata. Refresh it after switching workspaces, after changing those configuration surfaces, when Pad reports stale schema/context, or when the user asks. For changing work state, read the affected item or use a targeted project tool instead of fetching the whole bootstrap again.\n\n## Workspace context\n\nEvery action that operates within a workspace accepts an optional `workspace` parameter. Resolution order:\n\n1. Explicit `workspace` argument on the call (always wins).\n2. On a single-user local server only: the session default set via `pad_set_workspace`.\n3. On a single-user local server only: the CWD-linked workspace from `.pad.toml`.\n\nA multi-user/remote server does **not** persist a session default — pass `workspace` explicitly on every call. If none resolves, the action returns a structured `no_workspace` error with `available_workspaces`.\n\n## Update flow: read first, then patch\n\nFor `pad_item.action: update`, the server merges your patch with the item's current state. Pass only the fields you want to change. When changing `status`, ALWAYS include a `comment` explaining why — it builds the audit trail that helps the team understand history.\n\n## One error code you must not retry\n\nErrors come back as a structured envelope with a `code`. Most failures are worth a retry after you fix something, and a `server_error` may be transient. `stored_state_unreadable` is neither.\n\nIt means the ITEM'S STORED DATA cannot be decoded — your input was fine. Today it fires when `note` / `decide` would append to an `implementation_notes` / `decision_log` value that is not a list of entries. The operation is refused precisely because completing it would overwrite that value. The refusal is fully deterministic, so retrying — immediately or after a backoff — fails identically every time and changes nothing.\n\nWhen you see it: tell the user which item is affected, and stop. Whether you can inspect the bad value depends on which layer is broken — if the item's whole `fields` blob fails to parse, `action: get` hands it back as a raw string; if one structured key is at fault, you won't see it at all, because the blob is normalized on this surface and a value that doesn't decode is dropped from the top-level arrays. A human can always read it with `pad item show \u003cref\u003e --format json`. Do not route around the refusal by writing the field some other way; that is the write that caused it.\n\n## Project conventions\n\nWorkspaces can declare conventions (e.g. \"run `make test` before PR\", \"use conventional commit format\"). The bootstrap blob gives you two views:\n\n- `conventions` — full bodies of the always-on (`trigger=always`) rules. Follow these unconditionally.\n- `convention_index` — METADATA ONLY (`ref`, `title`, `trigger`, `role`; no bodies) for **every** active convention, including the triggered ones whose bodies are NOT in `conventions`. This is your map of what triggered rules exist.\n\nBefore performing meaningful work with a specific trigger (e.g. `on-implement` before writing code), consult `convention_index`: if it lists entries for that trigger, pull their bodies on demand; if it lists none, skip the query.\n\n```\npad_item.action: list, collection: \"conventions\", status: \"active\"\n```\n\nFilter by trigger (`always`, `on-implement`, `on-task-complete`, etc.) when relevant — the `convention_index` triggers tell you which filters are worth running.\n\n`\"conventions\"` above is the DEFAULT collection slug and a workspace may have renamed it, in which case that literal returns nothing while `convention_index` still lists entries — the bootstrap payload resolves by declaration, not by name. When the two disagree, address the items by the `ref`s the index gave you, or find the collection with `pad_collection.action: list`. The same applies to `\"playbooks\"`; `pad_playbook` resolves by declaration and is unaffected.\n\n## Pushes and the session monitor (security note)\n\nPad can push an item into a Claude Code session (`pad push`, or the web item view's push composer). That is terminal instruction injection by design, so receiving is **consent-gated** (PLAN-2613, since v0.15.0): a session receives pushes only after it has connected (`/pad:connect` in the plugin, or a repo-level `push.auto_arm = true` in `.pad.toml` — which a per-user `auto_arm = false` in `~/.pad/config.toml` vetoes), and pushes are self-addressed only — the sender is always the receiving session's own user. This MCP connection is not a session: it cannot arm one, receive a push, or run the monitor, and the `pad_item` / `pad_project` tools expose no push action. If a user asks you to \"push\" something to their terminal, tell them it happens from their own terminal (`pad push \u003cref\u003e -m \"...\"` — the sending session need not be connected, the receiving ones must have consented); do not try to emulate it by writing comments or instructions into items.\n\n## Adding a workspace to this connection\n\nIf 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: \"\u003cslug\u003e\", code: \"\u003c6 digits\u003e\"}`. The workspace joins this connection's allow-list and stays until the user revokes it via `/console/connected-apps`. No re-auth required.\n\nFor brand-new workspaces, `pad_workspace.create` with `{name: \"\u003cname\u003e\"}` (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).\n\n## New workspace: offer to set it up\n\nThe bootstrap blob carries `needs_onboarding: bool` — true when the workspace has zero user-created items (template seeds don't count). When it's true, **lead with an active offer before anything else**: *\"This workspace is brand new and isn't set up yet. Want me to set it up? I'll ask a few quick questions and adapt it to your project.\"*\n\nThis is an **offer, not an auto-run** — wait for the user to say yes before running onboarding. If they accept, run the `onboard` playbook (use the `pad_onboard` prompt, or load the body via `pad_playbook` `action: get`, `ref: onboard`). If they decline, or already declined earlier in the session, respect that and skip the offer. The flag flips to false the moment any item exists, so it won't nag past first setup.\n\n## Multi-step workflows\n\nFour prompts ship with the server: `pad_plan`, `pad_ideate`, `pad_retro`, `pad_onboard`. Use them when the user wants help planning, brainstorming, retrospecting, or onboarding into a workspace — they encode the multi-step Pad-aware playbook for each.\n" } diff --git a/cmd/pad/testdata/mcp_wire/stdio_initialize.json b/cmd/pad/testdata/mcp_wire/stdio_initialize.json index d836ea095..8e9faef1f 100644 --- a/cmd/pad/testdata/mcp_wire/stdio_initialize.json +++ b/cmd/pad/testdata/mcp_wire/stdio_initialize.json @@ -8,7 +8,7 @@ }, "padToolSurface": { "tool_surface_stable": true, - "version": "0.65" + "version": "0.66" } }, "prompts": {}, @@ -21,5 +21,5 @@ "name": "pad-mcp", "version": "" }, - "instructions": "Pad is a project tracker for developers and AI agents — issues (TASK, BUG), plans (PLAN), ideas (IDEA), docs (DOC), conventions, comments, and dependencies. Use this server when a user mentions:\n\n- Issue refs like `TASK-5`, `BUG-12`, `PLAN-3`, `IDEA-8` — they are stable, human-readable IDs and the canonical way to address items. Use them, never slugs; they are what audit trails and PR titles show.\n- Tasks / issues / items / plans / progress / \"what's on my plate\" / \"what to work on next\" / standup / changelog / retrospective.\n- Project conventions, decision records, or \"how should this team do X.\"\n\nIf the user is asking general code questions with no project-management thread, you don't need this server.\n\n## Tool surface (v0.65)\n\nTen resource × action tools, plus `pad_set_workspace`. Eleven tools total.\n\nInputs are validated strictly: an undeclared top-level key is rejected with a structured `validation_failed` naming it, never accepted and silently dropped.\n\n- `pad_item` — Items: create / update / delete / get / list / move / restore / link / unlink / deps / star / unstar / starred / comment / list-comments / edit-comment / delete-comment / backlinks / bulk-update / note / decide / export / import / history / remind / ack-reminder / claim / release. On create/update, field values may be passed as a `fields` OBJECT (the same shape reads return, e.g. `{\"status\":\"done\",\"effort\":\"l\"}`) — it merges into the same path as the dedicated params and `field: [\"key=value\"]`; the same key in two places with CONFLICTING values is refused, not silently resolved. `list` accepts `unparented: true` to keep items with no parent or implements relationship (mutually exclusive with `parent`). `list` results are SUMMARY-shaped by default on both transports — no content bodies; pass `full: true` for complete bodies (token-expensive), or prefer `get` for a single item's body. `update` field writes are a server-side field-level merge (only the keys you set change); pass `expected_seq` for optimistic concurrency — the `seq` you last read, which is on every item AND every list summary — and a stale value fails with a structured 409 `update_conflict` whose details carry `actual_seq` to retry with. PREFER IT over `expected_updated_at`, which cannot tell two writes inside one second apart; that one still works, and if you send both, seq decides. A token cannot see a browser tab's unsaved edits, so `content` sent WITH one is refused with 409 `content_pending_flush` while any exist; re-reading will not clear it. Wait for the editor to save, or pass `overwrite_pending_edits: true` to replace them. `content_state: \"superseded_set_aside\"` is different and wins when both apply: an editor upgrade set aside edits no tab can restore, so EVERY content write is refused (token or not; `details.set_aside_rows`) and waiting never clears it. Tell the user, or pass `overwrite_pending_edits: true` to discard them. Writing `content` while a browser tab has the item open routes the markdown through that live document, and the stored copy is updated only by a later collab-snapshot flush — usually from the tab that applied it, though any such write updates the row, and nothing guarantees one happens: the 200 echoes the content you SENT and carries `warnings.content_outcome: \"applied_pending_flush\"`, which describes that WRITE — the content went to the document, not the row — rather than being a live reading of the row, which a concurrent flush may already have updated. A `get` (or a `list` with `full: true` — a default list carries no content at all) before such a flush lands reads the stored copy and answers with the PREVIOUS content, which is the lag rather than a failed write, so do not re-send on the strength of it. You do not have to infer that from the write, and you will also see it when someone ELSE's tab holds the unflushed edits and you wrote nothing: a read that serves a body the server knows is behind the live document carries `content_state: \"applied_pending_flush\"` — same value as the write warning, different question (what the ROW is, not what your REQUEST did). Treat it as re-read later, never as re-send, and do not wait on it in a loop: the server normally recovers the stored copy itself about a minute after the last tab leaves the item, but that is not guaranteed (a recovery can fail, and an operator can turn it off), and otherwise the row catches up only when a tab next opens the item. The stored form may also not be byte-identical to what you sent, since the markdown round-trips through the editor (bullet markers, emphasis characters, list numbering and blank-line runs normalise; already-canonical markdown survives unchanged), so compare on meaning rather than bytes. `move` changes an item's COLLECTION within its workspace: system metadata (implementation notes, decision log, linked PR) survives it, and any field value the target schema has no home for is dropped AND reported in the move's activity entry — check there rather than assuming a move is lossless. A carried unique-field collision is dropped, named in `warnings.not_unique`. Three system keys — `implementation_notes`, `decision_log`, `convention` — cannot be set through `field` on update or move; the call is refused with `validation_failed`, naming the key and the write path that does maintain it (`note`, `decide`, and library activation respectively). `github_pr` is refused as well (BUG-2696): a `field` value arrived as a string, so the PR data was stored double-encoded and no link appeared. Its writer is `pad github link` / `unlink`, which needs a local git checkout, and there is no PR-link action on this surface yet, so treat linking a PR as something to hand to a human rather than a call to retry. On `create` they are refused too (BUG-3163), with the same `validation_failed`: create the item first, then use `note` / `decide` on it; convention metadata comes from library activation. `history` returns read-only item version metadata (newest-first), bounded to the NEWEST 50 versions by default (max 300 — pass `limit` to change the window); pass `full: true` to include each version's resolved content body (token-expensive). There is no `offset`: versions are stored as reverse patches, so only a newest-end window is cheap to reconstruct. Unassign or detach with the `clear_assigned_user` / `clear_agent_role` / `clear_parent` booleans; an empty `assign` / `role` / `parent` does NOT clear. `remind` / `ack-reminder`: a fired reminder shows in `pad_project` next / ready (the delivery path on an instance with no webhook) until acknowledged, and one on a completed item is hidden there. `claim` / `release`: a loser's 409 is `lease_held`, and a lease bumps neither `updated_at` nor `seq`. `edit-comment` / `delete-comment` take a `comment_id` from `list-comments` (whose rows carry `edited`); only a comment's author may edit it. Correct a comment in place rather than appending a correction. Deleting a comment that has replies leaves a placeholder (`deleted: true`, empty body) so the replies keep their parent; editing, replying to or reacting to it answers 409 `comment_deleted`. (Two older forms still work and are not deprecated: `field: [\"assigned_user_id=\"]` on either transport, and a direct `assigned_user_id: \"\"` param over remote `/mcp` only — prefer the boolean, which is the only one this schema advertises.)\n\n**App attribution (v0.65).** An item an installed app created, a comment an app wrote and a history row an app's write made carry `via_app` (the install id) and `via_app_name` (the app's name). On an item they name the app that CREATED it, not the last writer. They are absent for anything a person or an agent wrote.\n\n**Relation fields (v0.31).** A `relation` value may be a UUID, an issue ref (`COLO-3`), or the target item's EXACT TITLE — in that order, so an item literally titled `COLO-3` is unreachable by title while the ref resolves. Title matching is scoped to the collection the field declares: a title that is unique only workspace-wide is REFUSED, naming the collection searched, and a title matching two or more items inside the declared collection is refused as `ambiguous` rather than `not_found` — it matched too much, not too little. Only items YOU can see count towards any of that, so a match you have no access to never changes your answer.\n\nReads carry a `relation_targets` member beside `fields`: field key → `{id, ref, title}`, so you can render a relation without a request per value. `fields` still holds the canonical id, which is what you write back. An entry with an `id` and NO `ref`/`title` means the target is gone **or** you may not see it — the two are deliberately indistinguishable, so do not render it as either \"deleted\" or \"hidden\"; \"unavailable\" is the honest word. An entry with `stored_as_text: true` differs: the stored value is not UUID-shaped, so it was never an id (an imported title, legacy text). Render it as text, not as unavailable.\n\n**`multi_relation` fields (v0.33).** A field declared `multi_relation` holds an ORDERED LIST of references — `[\"\u003cuuid\u003e\", \"COLO-3\", \"Red\"]` — and every element resolves through the same UUID → ref → exact-title ladder, with the same collection scoping. Declare one through the `fields` DSL as `owners:multi_relation:people`: the third part is the TARGET COLLECTION, and omitting it is refused at parse time rather than building a field no write can ever satisfy.\n\nFour rules differ from a scalar `relation`, and none of them is guessable from the scalar behaviour:\n\n- **There is exactly one stored form for \"no references\": the key ABSENT.** Send `[]` and the write normalises it to that. A `required` `multi_relation` means at least one element that RESOLVED, so `[]` and an absent key both refuse.\n- **An empty or whitespace-only ELEMENT is refused**, naming its index — it is not skipped the way an empty scalar value is. Remove the element rather than blanking it.\n- **Order is part of the value.** `[\"a\",\"b\"]` and `[\"b\",\"a\"]` are different values, and a write preserves the order you sent.\n- **Duplicates are refused**, with reason `duplicate_referent`, naming the SECOND occurrence. Two elements can be different strings naming one item — a UUID and a ref, a ref and a title — so this is decided after resolution, and the two forms you sent may look nothing alike.\n\nIf ANY element fails, the whole write is refused and nothing is stored — never a partial list. On a cross-workspace copy the value is dropped WHOLE and reported once in `warnings.dropped_fields`, so an import can never change an element count.\n\nIn `relation_targets`, a `multi_relation` key carries a JSON **ARRAY** of the same `{id, ref, title}` objects, in stored order and one per stored element — including an `id`-only entry for an element that names nothing, so a position in the list lines up with the same position in `fields`. A scalar key is unchanged: still a single object, byte-identical to v0.31. Decide which shape to expect from the field's declared type, not by probing.\n\n- `pad_workspace` — Workspaces: list / members / invite / storage / audit-log / create / claim / deleted / restore.\n- `pad_collection` — Collections: list / create / update / delete.\n- `pad_project` — Project intelligence: dashboard / next / ready / stale / standup / changelog / report / activity. Use `ready` for the actionable backlog and `stale` for items needing attention; `activity` to catch up on what other agents/users changed since you last worked (non-streaming feed with item refs + change details).\n- `pad_role` — Agent roles: list / create / update / delete.\n- `pad_search` — Full-text search across items: query.\n- `pad_playbook` — Invokable procedures: list / get / run / match. Use `run` to bind args against a playbook's declared spec and get the rendered body back; side-effect-free. `run` refuses a playbook whose status isn't `active` (a draft still being authored) with a `playbook_not_active` error — pass `allow_draft: true` to override. Both `run` and `get` echo the playbook's `status`. Both may also carry `content_state` (`\"applied_pending_flush\"`, or `\"superseded_set_aside\"` as under `pad_item`), meaning the BODY you are about to execute is behind the item's live collaborative document — an editor holds unflushed edits, so the steps may be superseded. It is a reason to re-read or to ask, not a reason to refuse to run; the body is still the best the server has. `match` finds a playbook from text.\n- `pad_library` — Convention + playbook library (the global catalog of pre-built entries workspaces activate): list / get / activate.\n- `pad_attachment` — Read-only attachment metadata: list / show. `list` enumerates a workspace's attachments (filter by item / category / collection / attached / unattached); `show` returns one attachment's MIME, size, filename, and ETag via a HEAD request. Uploading and general file downloads stay CLI-only; bounded image bytes are available through the attachment resource below.\n- `pad_meta` — Server introspection: server-info / version / tool-surface / bootstrap. The `bootstrap` action returns one-shot workspace context (user + collections + always-on conventions + a metadata-only `convention_index` of every active convention + roles + playbook metadata + dashboard + recent activity).\n- `pad_set_workspace` — Load workspace context; response embeds the bootstrap blob so you load context in one call. Whether it also pins a session default: see Workspace context below. Takes `workspace: \u003cslug\u003e` only (no `action`).\n\nFor the ten resource × action tools, always pass `action` as a top-level field. Per-action required parameters are documented in each tool's description.\n\n## Resources are cheaper than tool calls\n\nRead these directly when you need workspace state:\n\n- `pad://workspace/{ws}/dashboard` — computed project overview (active items, plans, attention, suggested next).\n- `pad://workspace/{ws}/collections` — collection types + schemas.\n- `pad://workspace/{ws}/items` — list of all items (use `pad_item.action: list` for filtering).\n- `pad://workspace/{ws}/items/{ref}` — single item rendered as markdown.\n- `pad://workspace/{ws}/attachments/{id}` — image attachment as a bounded base64 `thumb-md` resource; rejects non-images and image bytes over 1 MiB (pre-base64).\n- `pad://workspace/{ws}/bootstrap` — one-shot workspace context (same payload as `pad_meta.action: bootstrap` and `pad_set_workspace`'s embedded response).\n- `pad://_meta/version` — server version + stability tiers.\n\nResources support host-side prefetch — if the host can fetch them once at session start, you don't pay per turn.\n\nLoad the bootstrap resource once per workspace and reuse its collections, conventions, roles, and playbook metadata. Refresh it after switching workspaces, after changing those configuration surfaces, when Pad reports stale schema/context, or when the user asks. For changing work state, read the affected item or use a targeted project tool instead of fetching the whole bootstrap again.\n\n## Workspace context\n\nEvery action that operates within a workspace accepts an optional `workspace` parameter. Resolution order:\n\n1. Explicit `workspace` argument on the call (always wins).\n2. On a single-user local server only: the session default set via `pad_set_workspace`.\n3. On a single-user local server only: the CWD-linked workspace from `.pad.toml`.\n\nA multi-user/remote server does **not** persist a session default — pass `workspace` explicitly on every call. If none resolves, the action returns a structured `no_workspace` error with `available_workspaces`.\n\n## Update flow: read first, then patch\n\nFor `pad_item.action: update`, the server merges your patch with the item's current state. Pass only the fields you want to change. When changing `status`, ALWAYS include a `comment` explaining why — it builds the audit trail that helps the team understand history.\n\n## One error code you must not retry\n\nErrors come back as a structured envelope with a `code`. Most failures are worth a retry after you fix something, and a `server_error` may be transient. `stored_state_unreadable` is neither.\n\nIt means the ITEM'S STORED DATA cannot be decoded — your input was fine. Today it fires when `note` / `decide` would append to an `implementation_notes` / `decision_log` value that is not a list of entries. The operation is refused precisely because completing it would overwrite that value. The refusal is fully deterministic, so retrying — immediately or after a backoff — fails identically every time and changes nothing.\n\nWhen you see it: tell the user which item is affected, and stop. Whether you can inspect the bad value depends on which layer is broken — if the item's whole `fields` blob fails to parse, `action: get` hands it back as a raw string; if one structured key is at fault, you won't see it at all, because the blob is normalized on this surface and a value that doesn't decode is dropped from the top-level arrays. A human can always read it with `pad item show \u003cref\u003e --format json`. Do not route around the refusal by writing the field some other way; that is the write that caused it.\n\n## Project conventions\n\nWorkspaces can declare conventions (e.g. \"run `make test` before PR\", \"use conventional commit format\"). The bootstrap blob gives you two views:\n\n- `conventions` — full bodies of the always-on (`trigger=always`) rules. Follow these unconditionally.\n- `convention_index` — METADATA ONLY (`ref`, `title`, `trigger`, `role`; no bodies) for **every** active convention, including the triggered ones whose bodies are NOT in `conventions`. This is your map of what triggered rules exist.\n\nBefore performing meaningful work with a specific trigger (e.g. `on-implement` before writing code), consult `convention_index`: if it lists entries for that trigger, pull their bodies on demand; if it lists none, skip the query.\n\n```\npad_item.action: list, collection: \"conventions\", status: \"active\"\n```\n\nFilter by trigger (`always`, `on-implement`, `on-task-complete`, etc.) when relevant — the `convention_index` triggers tell you which filters are worth running.\n\n`\"conventions\"` above is the DEFAULT collection slug and a workspace may have renamed it, in which case that literal returns nothing while `convention_index` still lists entries — the bootstrap payload resolves by declaration, not by name. When the two disagree, address the items by the `ref`s the index gave you, or find the collection with `pad_collection.action: list`. The same applies to `\"playbooks\"`; `pad_playbook` resolves by declaration and is unaffected.\n\n## Pushes and the session monitor (security note)\n\nPad can push an item into a Claude Code session (`pad push`, or the web item view's push composer). That is terminal instruction injection by design, so receiving is **consent-gated** (PLAN-2613, since v0.15.0): a session receives pushes only after it has connected (`/pad:connect` in the plugin, or a repo-level `push.auto_arm = true` in `.pad.toml` — which a per-user `auto_arm = false` in `~/.pad/config.toml` vetoes), and pushes are self-addressed only — the sender is always the receiving session's own user. This MCP connection is not a session: it cannot arm one, receive a push, or run the monitor, and the `pad_item` / `pad_project` tools expose no push action. If a user asks you to \"push\" something to their terminal, tell them it happens from their own terminal (`pad push \u003cref\u003e -m \"...\"` — the sending session need not be connected, the receiving ones must have consented); do not try to emulate it by writing comments or instructions into items.\n\n## Adding a workspace to this connection\n\nIf 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: \"\u003cslug\u003e\", code: \"\u003c6 digits\u003e\"}`. The workspace joins this connection's allow-list and stays until the user revokes it via `/console/connected-apps`. No re-auth required.\n\nFor brand-new workspaces, `pad_workspace.create` with `{name: \"\u003cname\u003e\"}` (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).\n\n## New workspace: offer to set it up\n\nThe bootstrap blob carries `needs_onboarding: bool` — true when the workspace has zero user-created items (template seeds don't count). When it's true, **lead with an active offer before anything else**: *\"This workspace is brand new and isn't set up yet. Want me to set it up? I'll ask a few quick questions and adapt it to your project.\"*\n\nThis is an **offer, not an auto-run** — wait for the user to say yes before running onboarding. If they accept, run the `onboard` playbook (use the `pad_onboard` prompt, or load the body via `pad_playbook` `action: get`, `ref: onboard`). If they decline, or already declined earlier in the session, respect that and skip the offer. The flag flips to false the moment any item exists, so it won't nag past first setup.\n\n## Multi-step workflows\n\nFour prompts ship with the server: `pad_plan`, `pad_ideate`, `pad_retro`, `pad_onboard`. Use them when the user wants help planning, brainstorming, retrospecting, or onboarding into a workspace — they encode the multi-step Pad-aware playbook for each.\n" + "instructions": "Pad is a project tracker for developers and AI agents — issues (TASK, BUG), plans (PLAN), ideas (IDEA), docs (DOC), conventions, comments, and dependencies. Use this server when a user mentions:\n\n- Issue refs like `TASK-5`, `BUG-12`, `PLAN-3`, `IDEA-8` — they are stable, human-readable IDs and the canonical way to address items. Use them, never slugs; they are what audit trails and PR titles show.\n- Tasks / issues / items / plans / progress / \"what's on my plate\" / \"what to work on next\" / standup / changelog / retrospective.\n- Project conventions, decision records, or \"how should this team do X.\"\n\nIf the user is asking general code questions with no project-management thread, you don't need this server.\n\n## Tool surface (v0.66)\n\nTen resource × action tools, plus `pad_set_workspace`. Eleven tools total.\n\nInputs are validated strictly: an undeclared top-level key is rejected with a structured `validation_failed` naming it, never accepted and silently dropped.\n\n- `pad_item` — Items: create / update / delete / get / list / move / restore / link / unlink / deps / star / unstar / starred / comment / list-comments / edit-comment / delete-comment / backlinks / bulk-update / note / decide / export / import / history / remind / ack-reminder / claim / release. On create/update, field values may be passed as a `fields` OBJECT (the same shape reads return, e.g. `{\"status\":\"done\",\"effort\":\"l\"}`) — it merges into the same path as the dedicated params and `field: [\"key=value\"]`; the same key in two places with CONFLICTING values is refused, not silently resolved. `list` accepts `unparented: true` to keep items with no parent or implements relationship (mutually exclusive with `parent`). `list` results are SUMMARY-shaped by default on both transports — no content bodies; pass `full: true` for complete bodies (token-expensive), or prefer `get` for a single item's body. `update` field writes are a server-side field-level merge (only the keys you set change); pass `expected_seq` for optimistic concurrency — the `seq` you last read, which is on every item AND every list summary — and a stale value fails with a structured 409 `update_conflict` whose details carry `actual_seq` to retry with. PREFER IT over `expected_updated_at`, which cannot tell two writes inside one second apart; that one still works, and if you send both, seq decides. A token cannot see a browser tab's unsaved edits, so `content` sent WITH one is refused with 409 `content_pending_flush` while any exist; re-reading will not clear it. Wait for the editor to save, or pass `overwrite_pending_edits: true` to replace them. `content_state: \"superseded_set_aside\"` is different and wins when both apply: an editor upgrade set aside edits no tab can restore, so EVERY content write is refused (token or not; `details.set_aside_rows`) and waiting never clears it. Tell the user, or pass `overwrite_pending_edits: true` to discard them. Writing `content` while a browser tab has the item open routes the markdown through that live document, and the stored copy is updated only by a later collab-snapshot flush — usually from the tab that applied it, though any such write updates the row, and nothing guarantees one happens: the 200 echoes the content you SENT and carries `warnings.content_outcome: \"applied_pending_flush\"`, which describes that WRITE — the content went to the document, not the row — rather than being a live reading of the row, which a concurrent flush may already have updated. A `get` (or a `list` with `full: true` — a default list carries no content at all) before such a flush lands reads the stored copy and answers with the PREVIOUS content, which is the lag rather than a failed write, so do not re-send on the strength of it. You do not have to infer that from the write, and you will also see it when someone ELSE's tab holds the unflushed edits and you wrote nothing: a read that serves a body the server knows is behind the live document carries `content_state: \"applied_pending_flush\"` — same value as the write warning, different question (what the ROW is, not what your REQUEST did). Treat it as re-read later, never as re-send, and do not wait on it in a loop: the server normally recovers the stored copy itself about a minute after the last tab leaves the item, but that is not guaranteed (a recovery can fail, and an operator can turn it off), and otherwise the row catches up only when a tab next opens the item. The stored form may also not be byte-identical to what you sent, since the markdown round-trips through the editor (bullet markers, emphasis characters, list numbering and blank-line runs normalise; already-canonical markdown survives unchanged), so compare on meaning rather than bytes. `move` changes an item's COLLECTION within its workspace: system metadata (implementation notes, decision log, linked PR) survives it, and any field value the target schema has no home for is dropped AND reported in the move's activity entry — check there rather than assuming a move is lossless. A carried unique-field collision is dropped, named in `warnings.not_unique`. Three system keys — `implementation_notes`, `decision_log`, `convention` — cannot be set through `field` on update or move; the call is refused with `validation_failed`, naming the key and the write path that does maintain it (`note`, `decide`, and library activation respectively). `github_pr` is refused as well (BUG-2696): a `field` value arrived as a string, so the PR data was stored double-encoded and no link appeared. Its writer is `pad github link` / `unlink`, which needs a local git checkout, and there is no PR-link action on this surface yet, so treat linking a PR as something to hand to a human rather than a call to retry. On `create` they are refused too (BUG-3163), with the same `validation_failed`: create the item first, then use `note` / `decide` on it; convention metadata comes from library activation. `history` returns read-only item version metadata (newest-first), bounded to the NEWEST 50 versions by default (max 300 — pass `limit` to change the window); pass `full: true` to include each version's resolved content body (token-expensive). There is no `offset`: versions are stored as reverse patches, so only a newest-end window is cheap to reconstruct. Unassign or detach with the `clear_assigned_user` / `clear_agent_role` / `clear_parent` booleans; an empty `assign` / `role` / `parent` does NOT clear. `remind` / `ack-reminder`: a fired reminder shows in `pad_project` next / ready (the delivery path on an instance with no webhook) until acknowledged, and one on a completed item is hidden there. `claim` / `release`: a loser's 409 is `lease_held`, and a lease bumps neither `updated_at` nor `seq`. `edit-comment` / `delete-comment` take a `comment_id` from `list-comments` (whose rows carry `edited`); only a comment's author may edit it. Correct a comment in place rather than appending a correction. Deleting a comment that has replies leaves a placeholder (`deleted: true`, empty body) so the replies keep their parent; editing, replying to or reacting to it answers 409 `comment_deleted`. (Two older forms still work and are not deprecated: `field: [\"assigned_user_id=\"]` on either transport, and a direct `assigned_user_id: \"\"` param over remote `/mcp` only — prefer the boolean, which is the only one this schema advertises.)\n\n**App attribution (v0.65).** An item an installed app created, a comment an app wrote and a history row an app's write made carry `via_app` (the install id) and `via_app_name` (the app's name). On an item they name the app that CREATED it, not the last writer. They are absent for anything a person or an agent wrote.\n\n**Relation fields (v0.31).** A `relation` value may be a UUID, an issue ref (`COLO-3`), or the target item's EXACT TITLE — in that order, so an item literally titled `COLO-3` is unreachable by title while the ref resolves. Title matching is scoped to the collection the field declares: a title that is unique only workspace-wide is REFUSED, naming the collection searched, and a title matching two or more items inside the declared collection is refused as `ambiguous` rather than `not_found` — it matched too much, not too little. Only items YOU can see count towards any of that, so a match you have no access to never changes your answer.\n\nReads carry a `relation_targets` member beside `fields`: field key → `{id, ref, title}`, so you can render a relation without a request per value. `fields` still holds the canonical id, which is what you write back. An entry with an `id` and NO `ref`/`title` means the target is gone **or** you may not see it — the two are deliberately indistinguishable, so do not render it as either \"deleted\" or \"hidden\"; \"unavailable\" is the honest word. An entry with `stored_as_text: true` differs: the stored value is not UUID-shaped, so it was never an id (an imported title, legacy text). Render it as text, not as unavailable.\n\n**`multi_relation` fields (v0.33).** A field declared `multi_relation` holds an ORDERED LIST of references — `[\"\u003cuuid\u003e\", \"COLO-3\", \"Red\"]` — and every element resolves through the same UUID → ref → exact-title ladder, with the same collection scoping. Declare one through the `fields` DSL as `owners:multi_relation:people`: the third part is the TARGET COLLECTION, and omitting it is refused at parse time rather than building a field no write can ever satisfy.\n\nFour rules differ from a scalar `relation`, and none of them is guessable from the scalar behaviour:\n\n- **There is exactly one stored form for \"no references\": the key ABSENT.** Send `[]` and the write normalises it to that. A `required` `multi_relation` means at least one element that RESOLVED, so `[]` and an absent key both refuse.\n- **An empty or whitespace-only ELEMENT is refused**, naming its index — it is not skipped the way an empty scalar value is. Remove the element rather than blanking it.\n- **Order is part of the value.** `[\"a\",\"b\"]` and `[\"b\",\"a\"]` are different values, and a write preserves the order you sent.\n- **Duplicates are refused**, with reason `duplicate_referent`, naming the SECOND occurrence. Two elements can be different strings naming one item — a UUID and a ref, a ref and a title — so this is decided after resolution, and the two forms you sent may look nothing alike.\n\nIf ANY element fails, the whole write is refused and nothing is stored — never a partial list. On a cross-workspace copy the value is dropped WHOLE and reported once in `warnings.dropped_fields`, so an import can never change an element count.\n\nIn `relation_targets`, a `multi_relation` key carries a JSON **ARRAY** of the same `{id, ref, title}` objects, in stored order and one per stored element — including an `id`-only entry for an element that names nothing, so a position in the list lines up with the same position in `fields`. A scalar key is unchanged: still a single object, byte-identical to v0.31. Decide which shape to expect from the field's declared type, not by probing.\n\n- `pad_workspace` — Workspaces: list / members / invite / storage / audit-log / create / claim / deleted / restore.\n- `pad_collection` — Collections: list / create / update / delete.\n- `pad_project` — Project intelligence: dashboard / next / ready / stale / standup / changelog / report / activity. Use `ready` for the actionable backlog and `stale` for items needing attention; `activity` to catch up on what other agents/users changed since you last worked (non-streaming feed with item refs + change details).\n- `pad_role` — Agent roles: list / create / update / delete.\n- `pad_search` — Full-text search across items: query.\n- `pad_playbook` — Invokable procedures: list / get / run / match. Use `run` to bind args against a playbook's declared spec and get the rendered body back; side-effect-free. `run` refuses a playbook whose status isn't `active` (a draft still being authored) with a `playbook_not_active` error — pass `allow_draft: true` to override. Both `run` and `get` echo the playbook's `status`. Both may also carry `content_state` (`\"applied_pending_flush\"`, or `\"superseded_set_aside\"` as under `pad_item`), meaning the BODY you are about to execute is behind the item's live collaborative document — an editor holds unflushed edits, so the steps may be superseded. It is a reason to re-read or to ask, not a reason to refuse to run; the body is still the best the server has. `match` finds a playbook from text.\n- `pad_library` — Convention + playbook library (the global catalog of pre-built entries workspaces activate): list / get / activate.\n- `pad_attachment` — Read-only attachment metadata: list / show. `list` enumerates a workspace's attachments (filter by item / category / collection / attached / unattached); `show` returns one attachment's MIME, size, filename, and ETag via a HEAD request. Uploading and general file downloads stay CLI-only; bounded image bytes are available through the attachment resource below.\n- `pad_meta` — Server introspection: server-info / version / tool-surface / bootstrap. The `bootstrap` action returns one-shot workspace context (user + collections + always-on conventions + a metadata-only `convention_index` of every active convention + roles + playbook metadata + dashboard + recent activity).\n- `pad_set_workspace` — Load workspace context; response embeds the bootstrap blob so you load context in one call. Whether it also pins a session default: see Workspace context below. Takes `workspace: \u003cslug\u003e` only (no `action`).\n\nFor the ten resource × action tools, always pass `action` as a top-level field. Per-action required parameters are documented in each tool's description.\n\n## Resources are cheaper than tool calls\n\nRead these directly when you need workspace state:\n\n- `pad://workspace/{ws}/dashboard` — computed project overview (active items, plans, attention, suggested next).\n- `pad://workspace/{ws}/collections` — collection types + schemas.\n- `pad://workspace/{ws}/items` — list of all items (use `pad_item.action: list` for filtering).\n- `pad://workspace/{ws}/items/{ref}` — single item rendered as markdown.\n- `pad://workspace/{ws}/attachments/{id}` — image attachment as a bounded base64 `thumb-md` resource; rejects non-images and image bytes over 1 MiB (pre-base64).\n- `pad://workspace/{ws}/bootstrap` — one-shot workspace context (same payload as `pad_meta.action: bootstrap` and `pad_set_workspace`'s embedded response).\n- `pad://_meta/version` — server version + stability tiers.\n\nResources support host-side prefetch — if the host can fetch them once at session start, you don't pay per turn.\n\nLoad the bootstrap resource once per workspace and reuse its collections, conventions, roles, and playbook metadata. Refresh it after switching workspaces, after changing those configuration surfaces, when Pad reports stale schema/context, or when the user asks. For changing work state, read the affected item or use a targeted project tool instead of fetching the whole bootstrap again.\n\n## Workspace context\n\nEvery action that operates within a workspace accepts an optional `workspace` parameter. Resolution order:\n\n1. Explicit `workspace` argument on the call (always wins).\n2. On a single-user local server only: the session default set via `pad_set_workspace`.\n3. On a single-user local server only: the CWD-linked workspace from `.pad.toml`.\n\nA multi-user/remote server does **not** persist a session default — pass `workspace` explicitly on every call. If none resolves, the action returns a structured `no_workspace` error with `available_workspaces`.\n\n## Update flow: read first, then patch\n\nFor `pad_item.action: update`, the server merges your patch with the item's current state. Pass only the fields you want to change. When changing `status`, ALWAYS include a `comment` explaining why — it builds the audit trail that helps the team understand history.\n\n## One error code you must not retry\n\nErrors come back as a structured envelope with a `code`. Most failures are worth a retry after you fix something, and a `server_error` may be transient. `stored_state_unreadable` is neither.\n\nIt means the ITEM'S STORED DATA cannot be decoded — your input was fine. Today it fires when `note` / `decide` would append to an `implementation_notes` / `decision_log` value that is not a list of entries. The operation is refused precisely because completing it would overwrite that value. The refusal is fully deterministic, so retrying — immediately or after a backoff — fails identically every time and changes nothing.\n\nWhen you see it: tell the user which item is affected, and stop. Whether you can inspect the bad value depends on which layer is broken — if the item's whole `fields` blob fails to parse, `action: get` hands it back as a raw string; if one structured key is at fault, you won't see it at all, because the blob is normalized on this surface and a value that doesn't decode is dropped from the top-level arrays. A human can always read it with `pad item show \u003cref\u003e --format json`. Do not route around the refusal by writing the field some other way; that is the write that caused it.\n\n## Project conventions\n\nWorkspaces can declare conventions (e.g. \"run `make test` before PR\", \"use conventional commit format\"). The bootstrap blob gives you two views:\n\n- `conventions` — full bodies of the always-on (`trigger=always`) rules. Follow these unconditionally.\n- `convention_index` — METADATA ONLY (`ref`, `title`, `trigger`, `role`; no bodies) for **every** active convention, including the triggered ones whose bodies are NOT in `conventions`. This is your map of what triggered rules exist.\n\nBefore performing meaningful work with a specific trigger (e.g. `on-implement` before writing code), consult `convention_index`: if it lists entries for that trigger, pull their bodies on demand; if it lists none, skip the query.\n\n```\npad_item.action: list, collection: \"conventions\", status: \"active\"\n```\n\nFilter by trigger (`always`, `on-implement`, `on-task-complete`, etc.) when relevant — the `convention_index` triggers tell you which filters are worth running.\n\n`\"conventions\"` above is the DEFAULT collection slug and a workspace may have renamed it, in which case that literal returns nothing while `convention_index` still lists entries — the bootstrap payload resolves by declaration, not by name. When the two disagree, address the items by the `ref`s the index gave you, or find the collection with `pad_collection.action: list`. The same applies to `\"playbooks\"`; `pad_playbook` resolves by declaration and is unaffected.\n\n## Pushes and the session monitor (security note)\n\nPad can push an item into a Claude Code session (`pad push`, or the web item view's push composer). That is terminal instruction injection by design, so receiving is **consent-gated** (PLAN-2613, since v0.15.0): a session receives pushes only after it has connected (`/pad:connect` in the plugin, or a repo-level `push.auto_arm = true` in `.pad.toml` — which a per-user `auto_arm = false` in `~/.pad/config.toml` vetoes), and pushes are self-addressed only — the sender is always the receiving session's own user. This MCP connection is not a session: it cannot arm one, receive a push, or run the monitor, and the `pad_item` / `pad_project` tools expose no push action. If a user asks you to \"push\" something to their terminal, tell them it happens from their own terminal (`pad push \u003cref\u003e -m \"...\"` — the sending session need not be connected, the receiving ones must have consented); do not try to emulate it by writing comments or instructions into items.\n\n## Adding a workspace to this connection\n\nIf 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: \"\u003cslug\u003e\", code: \"\u003c6 digits\u003e\"}`. The workspace joins this connection's allow-list and stays until the user revokes it via `/console/connected-apps`. No re-auth required.\n\nFor brand-new workspaces, `pad_workspace.create` with `{name: \"\u003cname\u003e\"}` (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).\n\n## New workspace: offer to set it up\n\nThe bootstrap blob carries `needs_onboarding: bool` — true when the workspace has zero user-created items (template seeds don't count). When it's true, **lead with an active offer before anything else**: *\"This workspace is brand new and isn't set up yet. Want me to set it up? I'll ask a few quick questions and adapt it to your project.\"*\n\nThis is an **offer, not an auto-run** — wait for the user to say yes before running onboarding. If they accept, run the `onboard` playbook (use the `pad_onboard` prompt, or load the body via `pad_playbook` `action: get`, `ref: onboard`). If they decline, or already declined earlier in the session, respect that and skip the offer. The flag flips to false the moment any item exists, so it won't nag past first setup.\n\n## Multi-step workflows\n\nFour prompts ship with the server: `pad_plan`, `pad_ideate`, `pad_retro`, `pad_onboard`. Use them when the user wants help planning, brainstorming, retrospecting, or onboarding into a workspace — they encode the multi-step Pad-aware playbook for each.\n" } diff --git a/internal/cli/client.go b/internal/cli/client.go index b5d7b4ce4..28ed5a5ac 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -2495,3 +2495,48 @@ func (c *Client) AckReminder(wsSlug, reminderID string) (*models.Reminder, error func (c *Client) DeleteReminder(wsSlug, reminderID string) error { return c.delete("/workspaces/" + wsSlug + "/reminders/" + reminderID) } + +// MyInvitation is one pending invitation addressed to the signed-in account +// (GET /api/v1/me/invitations, BUG-2136). +type MyInvitation struct { + ID string `json:"id"` + Role string `json:"role"` + WorkspaceSlug string `json:"workspace_slug"` + WorkspaceName string `json:"workspace_name"` + WorkspaceOwnerUsername string `json:"workspace_owner_username"` + InvitedByName string `json:"invited_by_name"` + CreatedAt time.Time `json:"created_at"` + ExpiresAt *time.Time `json:"expires_at,omitempty"` +} + +// ListMyInvitations returns the pending invitations addressed to the caller, +// and whether the caller's email is verified (only a verified email is +// matched). +func (c *Client) ListMyInvitations() ([]MyInvitation, bool, error) { + var body struct { + Invitations []MyInvitation `json:"invitations"` + EmailVerified bool `json:"email_verified"` + } + if err := c.get("/me/invitations", &body); err != nil { + return nil, false, err + } + return body.Invitations, body.EmailVerified, nil +} + +// AcceptMyInvitation accepts a pending invitation by id and returns the role +// held and the workspace slug. +func (c *Client) AcceptMyInvitation(id string) (role, workspaceSlug string, err error) { + var body struct { + Role string `json:"role"` + WorkspaceSlug string `json:"workspace_slug"` + } + if err := c.post("/me/invitations/"+url.PathEscape(id)+"/accept", nil, &body); err != nil { + return "", "", err + } + return body.Role, body.WorkspaceSlug, nil +} + +// DeclineMyInvitation declines (deletes) a pending invitation by id. +func (c *Client) DeclineMyInvitation(id string) error { + return c.post("/me/invitations/"+url.PathEscape(id)+"/decline", nil, nil) +} diff --git a/internal/mcp/instructions.md b/internal/mcp/instructions.md index 946d9240d..1b6eaeea5 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.65) +## Tool surface (v0.66) Ten resource × action tools, plus `pad_set_workspace`. Eleven tools total. diff --git a/internal/mcp/version.go b/internal/mcp/version.go index 2f27bec10..6dfb30c56 100644 --- a/internal/mcp/version.go +++ b/internal/mcp/version.go @@ -1738,6 +1738,19 @@ const CmdhelpVersion = "0.1" // caller cannot read embeds nothing; a failed fetch stays non-fatal, // as on stdio. Stdio is unchanged. // +// 0.66 — BUG-2136 (lead ruling, day 87). BEHAVIOR bump on the v0.52 / +// v0.43 grounds: `pad_workspace.action=invite` no longer ADDS an existing +// account to the workspace. Every invite now creates a pending +// invitation that the invitee accepts or declines (in the web app, or +// with `pad workspace invitations` / `accept` / `decline`, CLI-only by +// ruling), on both transports. The result loses the direct-add shape +// (`added`, `user_id`, `name`) and is always `{invited, code, email, +// role, join_url?}`, so it also stops saying whether the address has an +// account. A re-invite of an address with a pending invitation replaces +// it (a fresh code, role and 14-day expiry; the old code stops working). +// An agent that invited a user and then acted on them as a member must +// now wait for their acceptance. No name, enum or param moved. +// // 0.65 — TASK-3413 (SPEC-6 U9c). ADDITIVE bump on the v0.53 grounds: // an item an installed APP created, a comment an app wrote and a version // row an app's write made gain `omitempty` `via_app` (the install id) and @@ -1779,7 +1792,7 @@ const CmdhelpVersion = "0.1" // this surface can receive it; the entry exists so a future action does // not collapse it to permission_denied. When an action that can reach // it is added, that addition is the contract change and owns the bump. -const ToolSurfaceVersion = "0.65" +const ToolSurfaceVersion = "0.66" // 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/models/activity.go b/internal/models/activity.go index 81015a60e..d8694555b 100644 --- a/internal/models/activity.go +++ b/internal/models/activity.go @@ -12,25 +12,28 @@ var ValidActions = []string{ // Audit action constants for auth/admin events const ( - ActionLogin = "login" - ActionLoginFailed = "login_failed" - ActionLogout = "logout" - ActionBootstrap = "bootstrap" - ActionRegister = "register" - ActionPasswordChanged = "password_changed" - ActionPasswordReset = "password_reset" - ActionTokenCreated = "token_created" - ActionTokenRevoked = "token_revoked" - ActionTokenRotated = "token_rotated" - ActionTOTPEnabled = "totp_enabled" - ActionTOTPDisabled = "totp_disabled" - ActionMemberInvited = "member_invited" - ActionMemberRemoved = "member_removed" - ActionRoleChanged = "role_changed" - ActionSettingsChanged = "settings_changed" - ActionOAuthLogin = "oauth_login" - ActionOAuthLoginFailed = "oauth_login_failed" - ActionPlanChanged = "plan_changed" + ActionLogin = "login" + ActionLoginFailed = "login_failed" + ActionLogout = "logout" + ActionBootstrap = "bootstrap" + ActionRegister = "register" + ActionPasswordChanged = "password_changed" + ActionPasswordReset = "password_reset" + ActionTokenCreated = "token_created" + ActionTokenRevoked = "token_revoked" + ActionTokenRotated = "token_rotated" + ActionTOTPEnabled = "totp_enabled" + ActionTOTPDisabled = "totp_disabled" + ActionMemberInvited = "member_invited" + // ActionMemberInviteDeclined: the invitee declined a pending invitation + // (BUG-2136). The invitation row is deleted; this is its trail. + ActionMemberInviteDeclined = "member_invite_declined" + ActionMemberRemoved = "member_removed" + ActionRoleChanged = "role_changed" + ActionSettingsChanged = "settings_changed" + ActionOAuthLogin = "oauth_login" + ActionOAuthLoginFailed = "oauth_login_failed" + ActionPlanChanged = "plan_changed" // ActionPlanOverridesChanged is logged when an admin updates a // user's plan_overrides JSON via the admin user-detail page. // Surfaces per-user storage / workspace / API-token quota diff --git a/internal/server/bug2136_invite_consent_test.go b/internal/server/bug2136_invite_consent_test.go new file mode 100644 index 000000000..853c924f2 --- /dev/null +++ b/internal/server/bug2136_invite_consent_test.go @@ -0,0 +1,261 @@ +package server + +import ( + "fmt" + "net/http" + "sync/atomic" + "testing" + "time" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" +) + +// BUG-2136 (lead ruling, day 87): an existing user invited to a workspace is +// asked, not added. Every invite creates a pending invitation the invitee can +// accept or decline; a re-invite replaces the pending one; decline deletes it +// and leaves an audit event. + +type inviteBody struct { + Added bool `json:"added"` + Invited bool `json:"invited"` + Code string `json:"code"` + UserID string `json:"user_id"` + Name string `json:"name"` +} + +func myInvitationIDs(t *testing.T, f *accessFixture, tok string) []string { + t.Helper() + rr := f.do("GET", "/api/v1/me/invitations", tok, nil) + f.must(rr, http.StatusOK, "list my invitations") + var body struct { + Invitations []struct { + ID string `json:"id"` + } `json:"invitations"` + } + parseJSON(t, rr, &body) + ids := []string{} + for _, inv := range body.Invitations { + ids = append(ids, inv.ID) + } + return ids +} + +func TestBUG2136_InviteAsksAVerifiedExistingUser(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newAccessFixture(t, d) + invitee := mkUser(t, f.srv, "existing@example.com") + + rr := f.do("POST", "/api/v1/workspaces/"+f.wsSlug+"/members/invite", f.ownerTok, + map[string]any{"email": "existing@example.com", "role": "editor"}) + f.must(rr, http.StatusCreated, "invite existing user") + var body inviteBody + parseJSON(t, rr, &body) + if body.Added || !body.Invited || body.Code == "" { + t.Fatalf("invite of an existing user: got %s, want a pending invitation", rr.Body.String()) + } + // The response has the invitation's shape, with no direct-add keys. + var keys map[string]any + parseJSON(t, rr, &keys) + for _, k := range []string{"added", "user_id", "name"} { + if _, ok := keys[k]; ok { + t.Errorf("invite response carries the direct-add key %q: %s", k, rr.Body.String()) + } + } + if member, err := f.srv.store.IsWorkspaceMember(f.wsID, invitee.ID); err != nil || member { + t.Fatalf("the invitee became a member without accepting (member=%v err=%v)", member, err) + } + ids := myInvitationIDs(t, f, f.token(invitee)) + if len(ids) != 1 { + t.Fatalf("the invitee sees %d pending invitations, want 1", len(ids)) + } + // Accepting is what joins. + rr = f.do("POST", "/api/v1/me/invitations/"+ids[0]+"/accept", f.token(invitee), nil) + f.must(rr, http.StatusOK, "accept") + if member, _ := f.srv.store.IsWorkspaceMember(f.wsID, invitee.ID); !member { + t.Fatalf("accept did not add the member") + } + }) +} + +func TestBUG2136_ReinviteReplacesThePendingInvitation(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newAccessFixture(t, d) + invitee := mkUser(t, f.srv, "again@example.com") + invite := func(role string) inviteBody { + t.Helper() + rr := f.do("POST", "/api/v1/workspaces/"+f.wsSlug+"/members/invite", f.ownerTok, + map[string]any{"email": "again@example.com", "role": role}) + f.must(rr, http.StatusCreated, "invite") + var b inviteBody + parseJSON(t, rr, &b) + return b + } + first := invite("viewer") + second := invite("editor") + pending, err := f.srv.store.ListWorkspaceInvitations(f.wsID) + if err != nil { + t.Fatal(err) + } + n := 0 + for _, inv := range pending { + if inv.Email == "again@example.com" { + n++ + if inv.Role != "editor" { + t.Errorf("the surviving invitation has role %q, want the re-invite's editor", inv.Role) + } + } + } + if n != 1 { + t.Fatalf("%d pending invitations for one address after a re-invite, want 1", n) + } + if rr := f.do("POST", "/api/v1/invitations/"+first.Code+"/accept", f.token(invitee), nil); rr.Code != http.StatusNotFound { + t.Errorf("the replaced invitation's code still works: %d %s", rr.Code, rr.Body.String()) + } + rr := f.do("POST", "/api/v1/invitations/"+second.Code+"/accept", f.token(invitee), nil) + f.must(rr, http.StatusOK, "accept the re-invite") + }) +} + +func TestBUG2136_DeclineByID(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newAccessFixture(t, d) + invitee := mkUser(t, f.srv, "decliner@example.com") + other := mkUser(t, f.srv, "someone-else@example.com") + rr := f.do("POST", "/api/v1/workspaces/"+f.wsSlug+"/members/invite", f.ownerTok, + map[string]any{"email": "decliner@example.com"}) + f.must(rr, http.StatusCreated, "invite") + ids := myInvitationIDs(t, f, f.token(invitee)) + if len(ids) != 1 { + t.Fatalf("want 1 pending invitation, got %d", len(ids)) + } + // Another account cannot decline it, and is told nothing exists. + unknown := f.do("POST", "/api/v1/me/invitations/no-such-id/decline", f.token(other), nil) + f.must(unknown, http.StatusNotFound, "decline unknown id") + if rr := f.do("POST", "/api/v1/me/invitations/"+ids[0]+"/decline", f.token(other), nil); rr.Code != http.StatusNotFound || rr.Body.String() != unknown.Body.String() { + t.Fatalf("another account's decline: %d %s, want the unknown-id 404", rr.Code, rr.Body.String()) + } + + rr = f.do("POST", "/api/v1/me/invitations/"+ids[0]+"/decline", f.token(invitee), nil) + if rr.Code != http.StatusNoContent && rr.Code != http.StatusOK { + t.Fatalf("decline: %d %s", rr.Code, rr.Body.String()) + } + if left := myInvitationIDs(t, f, f.token(invitee)); len(left) != 0 { + t.Errorf("a declined invitation is still listed: %v", left) + } + if pending, _ := f.srv.store.ListWorkspaceInvitations(f.wsID); len(pending) != 0 { + t.Errorf("the owner still sees the declined invitation as pending: %d", len(pending)) + } + if rr := f.do("POST", "/api/v1/me/invitations/"+ids[0]+"/accept", f.token(invitee), nil); rr.Code != http.StatusNotFound { + t.Errorf("a declined invitation can still be accepted: %d", rr.Code) + } + if member, _ := f.srv.store.IsWorkspaceMember(f.wsID, invitee.ID); member { + t.Errorf("declining made the invitee a member") + } + acts, err := f.srv.store.ListWorkspaceActivity(f.wsID, models.ActivityListParams{Limit: 50}) + if err != nil { + t.Fatal(err) + } + found := false + for _, a := range acts { + found = found || a.Action == models.ActionMemberInviteDeclined + } + if !found { + t.Errorf("no %s audit event", models.ActionMemberInviteDeclined) + } + }) +} + +func TestBUG2136_DeclineByCode(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newAccessFixture(t, d) + invitee := mkUser(t, f.srv, "code-decliner@example.com") + other := mkUser(t, f.srv, "not-the-invitee@example.com") + rr := f.do("POST", "/api/v1/workspaces/"+f.wsSlug+"/members/invite", f.ownerTok, + map[string]any{"email": "code-decliner@example.com"}) + f.must(rr, http.StatusCreated, "invite") + var b inviteBody + parseJSON(t, rr, &b) + + if rr := f.do("POST", "/api/v1/invitations/"+b.Code+"/decline", f.token(other), nil); rr.Code != http.StatusForbidden { + t.Fatalf("a different account declined by code: %d %s", rr.Code, rr.Body.String()) + } + rr = f.do("POST", "/api/v1/invitations/"+b.Code+"/decline", f.token(invitee), nil) + if rr.Code != http.StatusNoContent && rr.Code != http.StatusOK { + t.Fatalf("decline by code: %d %s", rr.Code, rr.Body.String()) + } + if rr := f.do("POST", "/api/v1/invitations/"+b.Code+"/accept", f.token(invitee), nil); rr.Code != http.StatusNotFound { + t.Errorf("a declined invitation's code still accepts: %d", rr.Code) + } + }) +} + +// Two sibling invitations for one address (a concurrent re-invite can leave +// both on Postgres without the lock): accepting both is one membership and no +// error, because a second accept by a member is idempotent (BUG-3281). +func TestBUG2136_SiblingInvitationsAcceptCleanly(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newAccessFixture(t, d) + invitee := mkUser(t, f.srv, "twin@example.com") + a, err := f.srv.store.CreateInvitation(f.wsID, "twin@example.com", "editor", f.owner.ID) + if err != nil { + t.Fatal(err) + } + // Insert the sibling directly, bypassing the replace, to model the race. + b := insertRawInvitation(t, f, d, "twin@example.com", "viewer") + for _, id := range []string{a.ID, b} { + rr := f.do("POST", "/api/v1/me/invitations/"+id+"/accept", f.token(invitee), nil) + if rr.Code != http.StatusOK && rr.Code != http.StatusNotFound { + t.Fatalf("accepting a sibling invitation: %d %s", rr.Code, rr.Body.String()) + } + } + members, err := f.srv.store.ListWorkspaceMembers(f.wsID) + if err != nil { + t.Fatal(err) + } + n := 0 + for _, m := range members { + if m.UserID == invitee.ID { + n++ + } + } + if n != 1 { + t.Fatalf("%d memberships after accepting two sibling invitations, want 1", n) + } + }) +} + +// Inviting an existing member is still refused. +func TestBUG2136_InvitingAMemberIsRefused(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newAccessFixture(t, d) + m := mkUser(t, f.srv, "already@example.com") + if err := f.srv.store.AddWorkspaceMember(f.wsID, m.ID, "editor"); err != nil { + t.Fatal(err) + } + rr := f.do("POST", "/api/v1/workspaces/"+f.wsSlug+"/members/invite", f.ownerTok, + map[string]any{"email": "already@example.com"}) + f.must(rr, http.StatusConflict, "invite a member") + }) +} + +var rawInvitationSeq atomic.Int64 + +// insertRawInvitation inserts a pending invitation row directly, bypassing +// CreateInvitation's replace, to model two invites that raced past it. +func insertRawInvitation(t *testing.T, f *accessFixture, d store.DriverType, email, role string) string { + t.Helper() + id := fmt.Sprintf("raw-%d-%s", rawInvitationSeq.Add(1), email) + q := `INSERT INTO workspace_invitations (id, workspace_id, email, role, invited_by, code, code_hash, created_at, expires_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)` + if d == store.DriverPostgres { + q = `INSERT INTO workspace_invitations (id, workspace_id, email, role, invited_by, code, code_hash, created_at, expires_at) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)` + } + ts := time.Now().UTC().Format(time.RFC3339) + exp := time.Now().UTC().Add(time.Hour).Format(time.RFC3339) + if _, err := f.srv.store.DB().Exec(q, id, f.wsID, email, role, f.owner.ID, id, "hash-"+id, ts, exp); err != nil { + t.Fatalf("insert raw invitation: %v", err) + } + return id +} diff --git a/internal/server/bug3348_unverified_email_access_test.go b/internal/server/bug3348_unverified_email_access_test.go index 53ad719e1..5e05ec487 100644 --- a/internal/server/bug3348_unverified_email_access_test.go +++ b/internal/server/bug3348_unverified_email_access_test.go @@ -44,15 +44,24 @@ func TestBUG3348_InviteDoesNotDirectAddUnverifiedAccount(t *testing.T) { t.Fatalf("unverified squatter read the workspace: %d %s", rr.Code, rr.Body.String()) } - // Control: a verified account is still added directly. - mkUser(t, f.srv, "real@example.com") + // Control: a verified account gets the same pending invitation (since + // BUG-2136 no account is added directly), and as the owner of the + // address it can accept it from its own in-app list, which the + // squatter cannot. + real := mkUser(t, f.srv, "real@example.com") rr = f.do("POST", "/api/v1/workspaces/"+f.wsSlug+"/members/invite", f.ownerTok, map[string]any{"email": "real@example.com", "role": "editor"}) f.must(rr, http.StatusCreated, "invite verified") + body.Added, body.Invited = false, false parseJSON(t, rr, &body) - if !body.Added { - t.Fatalf("verified account was not added directly: %s", rr.Body.String()) + if body.Added || !body.Invited { + t.Fatalf("verified account: got %s, want a pending invitation (BUG-2136)", rr.Body.String()) } + ids := myInvitationIDs(t, f, f.token(real)) + if len(ids) != 1 { + t.Fatalf("the verified invitee lists %d invitations, want 1", len(ids)) + } + f.must(f.do("POST", "/api/v1/me/invitations/"+ids[0]+"/accept", f.token(real), nil), http.StatusOK, "verified accept") }) } diff --git a/internal/server/handlers_members.go b/internal/server/handlers_members.go index 9172f023e..d4b8420e6 100644 --- a/internal/server/handlers_members.go +++ b/internal/server/handlers_members.go @@ -144,38 +144,27 @@ func (s *Server) handleInviteMember(w http.ResponseWriter, r *http.Request) { return } - // BUG-3348: only an account that has PROVEN this address is added - // directly. Anyone can register an address on a self-serve instance, so - // an unverified account matching the invite is not evidence of who will - // read the workspace; it gets the ordinary invitation below, whose code - // goes to the address itself. Accepting it verifies the account. - if existingUser != nil && existingUser.IsEmailVerified() { - // User exists and owns the address — add them directly - alreadyMember, _ := s.store.IsWorkspaceMember(workspaceID, existingUser.ID) + // BUG-2136 (lead ruling, day 87): an existing account is ASKED, never + // added. Every invite is a pending invitation the invitee accepts or + // declines, in-app (GET /me/invitations) or from the email when one is + // sent. The response has the same SHAPE whether or not the address has + // an account. It is not an account-existence guarantee: a code holder, + // the inviter included, can still read has_account from the public + // invitation preview, which the join page needs (pre-existing). BUG-3348's + // direct add of a verified account is gone with it. + if existingUser != nil { + alreadyMember, merr := s.store.IsWorkspaceMember(workspaceID, existingUser.ID) + if merr != nil { + writeInternalError(w, merr) + return + } if alreadyMember { writeError(w, http.StatusConflict, "conflict", "User is already a member of this workspace") return } - if err := s.store.AddWorkspaceMember(workspaceID, existingUser.ID, input.Role, s.workspaceLimitMintOpts()...); err != nil { - if writeStorePlanLimitError(w, r, err, "") { - return - } - writeInternalError(w, err) - return - } - s.logWorkspaceAuditEvent(workspaceID, models.ActionMemberInvited, r, auditMeta(map[string]string{"email": existingUser.Email, "role": input.Role, "added_directly": "true"})) - s.publishWorkspaceAccessChangedFromRequest(r, workspaceID, watchevents.AccessGained, existingUser.ID) - writeJSON(w, http.StatusCreated, map[string]interface{}{ - "added": true, - "user_id": existingUser.ID, - "email": existingUser.Email, - "name": existingUser.Name, - "role": input.Role, - }) - return } - // User doesn't exist — create an invitation + // Create (or, for a re-invite, replace) the pending invitation. inv, err := s.store.CreateInvitation(workspaceID, input.Email, input.Role, inviterID) if writeAccountDisabledIf(w, err) { return @@ -465,6 +454,51 @@ func (s *Server) handleAcceptInvitation(w http.ResponseWriter, r *http.Request) s.writeInvitationAccepted(w, inv, role) } +// handleDeclineInvitation answers POST /api/v1/invitations/{code}/decline +// (BUG-2136): declining from the join link. The caller must be signed in as +// the invited address, exactly as accepting by code requires, so a forwarded +// code cannot be used to decline on someone else's behalf. +func (s *Server) handleDeclineInvitation(w http.ResponseWriter, r *http.Request) { + inv, err := s.store.GetInvitationByCode(chi.URLParam(r, "code")) + if err != nil { + writeInternalError(w, err) + return + } + if inv == nil { + writeError(w, http.StatusNotFound, "not_found", "Invitation not found or already accepted") + return + } + user := currentUser(r) + if user == nil || user.IsApp() { + writeError(w, http.StatusUnauthorized, "unauthorized", "You must be logged in to decline an invitation") + return + } + if !invitationEmailMatches(user.Email, inv.Email) { + writeError(w, http.StatusForbidden, "invitation_email_mismatch", + "This invitation was sent to a different email address. Sign in with that account to decline.") + return + } + s.declineInvitation(w, r, inv) +} + +// declineInvitation is what declining means behind both doors: the pending +// invitation is deleted and a member_invite_declined audit event records it. +// A row gone in between (accepted, cancelled or declined concurrently) is the +// same 404 an unknown invitation gets. +func (s *Server) declineInvitation(w http.ResponseWriter, r *http.Request, inv *models.WorkspaceInvitation) { + if err := s.store.DeleteInvitationAdmin(inv.ID); err != nil { + if errors.Is(err, sql.ErrNoRows) { + writeError(w, http.StatusNotFound, "not_found", "Invitation not found or already accepted") + return + } + writeInternalError(w, err) + return + } + s.logWorkspaceAuditEvent(inv.WorkspaceID, models.ActionMemberInviteDeclined, r, + auditMeta(map[string]string{"email": inv.Email, "role": inv.Role})) + w.WriteHeader(http.StatusNoContent) +} + // acceptInvitationCore is the one accept path behind both invitation doors: // POST /invitations/{code}/accept and POST /me/invitations/{id}/accept // (TASK-3277). Each door decides first whether the caller may accept inv; @@ -484,6 +518,13 @@ func (s *Server) acceptInvitationCore(w http.ResponseWriter, r *http.Request, in // key direct adds take. A refusal rolls the transaction back, so the // invitation stays pending and can be accepted once there is room. added, role, err := s.store.AcceptWorkspaceInvitation(inv.ID, inv.WorkspaceID, user.ID, inv.Role, s.workspaceLimitMintOpts()...) + // Declined, replaced or cancelled after the door read it (BUG-2136): the + // store rolled the accept back, so this is the not-found every door gives + // an invitation that is gone. No reconcile: nothing was written. + if errors.Is(err, store.ErrInvitationGone) { + writeError(w, http.StatusNotFound, "not_found", "Invitation not found or already accepted") + return "", false + } if err != nil { // RECONCILE BEFORE REFUSING (BUG-3026). The store returns the raw // commit error, so a lost acknowledgement lands the membership and the diff --git a/internal/server/handlers_my_invitations.go b/internal/server/handlers_my_invitations.go index 57756ec01..3557640ec 100644 --- a/internal/server/handlers_my_invitations.go +++ b/internal/server/handlers_my_invitations.go @@ -88,3 +88,33 @@ func (s *Server) handleAcceptMyInvitation(w http.ResponseWriter, r *http.Request } s.writeInvitationAccepted(w, inv, role) } + +// handleDeclineMyInvitation answers POST /api/v1/me/invitations/{id}/decline +// (BUG-2136): the invitee says no from the in-app list. Same admission as the +// accept beside it: a signed-in person with a VERIFIED email, and an id that is +// not a pending invitation addressed to them answers 404 identical to an +// unknown id. Declining deletes the invitation, as an owner's cancel does, so +// every pending list drops it and its code stops working; the audit event is +// its trail. An expired invitation may be declined too, which only tidies it. +func (s *Server) handleDeclineMyInvitation(w http.ResponseWriter, r *http.Request) { + user := currentUser(r) + if user == nil || user.IsApp() { + writeError(w, http.StatusUnauthorized, "unauthorized", "You must be logged in to decline an invitation") + return + } + if !user.IsEmailVerified() { + writeError(w, http.StatusForbidden, "email_not_verified", + "Verify your email address to decline invitations from this list, or use the link in the invitation email.") + return + } + inv, err := s.store.GetPendingInvitationForEmail(chi.URLParam(r, "id"), user.Email) + if err != nil { + writeInternalError(w, err) + return + } + if inv == nil { + writeError(w, http.StatusNotFound, "not_found", "Invitation not found or already accepted") + return + } + s.declineInvitation(w, r, inv) +} diff --git a/internal/server/handlers_plan_limit_race_test.go b/internal/server/handlers_plan_limit_race_test.go index 44c085aeb..d81bf35e7 100644 --- a/internal/server/handlers_plan_limit_race_test.go +++ b/internal/server/handlers_plan_limit_race_test.go @@ -491,15 +491,6 @@ func (e *planLimitEnv) collectionID(t *testing.T, slug string) string { return coll.ID } -func (e *planLimitEnv) newUser(t *testing.T, email string) *models.User { - t.Helper() - u, err := e.srv.store.CreateUser(models.UserCreate{Email: email, Name: email, Password: "pw-limit-12345"}) - if err != nil { - t.Fatalf("CreateUser(%s): %v", email, err) - } - return u -} - func (e *planLimitEnv) itemRace(t *testing.T, door func(t *testing.T) *httptest.ResponseRecorder) workspaceRace { tasks := e.collectionID(t, "tasks") return workspaceRace{ @@ -542,27 +533,14 @@ func (e *planLimitEnv) itemImportRace(t *testing.T) workspaceRace { }) } -// W3: the invite door's direct add of an existing user. -func (e *planLimitEnv) memberAddRace(t *testing.T) workspaceRace { - invitee := e.newUser(t, "invitee@example.com") - rival := e.newUser(t, "rival@example.com") - return workspaceRace{ - feature: "members_per_workspace", - count: func(t *testing.T) int { - return e.countIn(t, `SELECT COUNT(*) FROM workspace_members WHERE workspace_id = ?`) - }, - compete: func(t *testing.T) { - if err := e.srv.store.AddWorkspaceMember(e.home.ID, rival.ID, "editor"); err != nil { - t.Errorf("competing AddWorkspaceMember: %v", err) - } - }, - door: func(t *testing.T) *httptest.ResponseRecorder { - body, _ := json.Marshal(map[string]any{"email": invitee.Email, "role": "editor"}) - return e.do("POST", "/api/v1/workspaces/"+e.home.Slug+"/members/invite", "application/json", body, "") - }, - admitted: http.StatusCreated, - } -} +// The members door is gone from this harness (BUG-2136). The invite still runs +// its members_per_workspace pre-check (refusing an invitation at the cap, as it +// always did for code invitations), but it no longer creates a membership, so +// this harness's member count cannot measure it. The door that creates the +// membership is the accept, whose cap the store decides alone; it is covered +// by handlers_invitation_accept_limit_test.go (BUG-3098), including the +// register race and the self-hosted leg. The invite's refusal at the cap is +// handlers_workspace_cap_test.go. // W4: the webhook create door. Literal IPs keep ValidateWebhookURL off DNS. func (e *planLimitEnv) webhookRace(t *testing.T) workspaceRace { @@ -604,16 +582,6 @@ func TestPlanLimitRace_ItemsImport_NoCompetitor_Admitted(t *testing.T) { e.runWorkspaceRace(t, e.itemImportRace(t), false) } -func TestPlanLimitRace_Members_CompetingAddInWindow_Refused(t *testing.T) { - e := newPlanLimitEnv(t) - e.runWorkspaceRace(t, e.memberAddRace(t), true) -} - -func TestPlanLimitRace_Members_NoCompetitor_Admitted(t *testing.T) { - e := newPlanLimitEnv(t) - e.runWorkspaceRace(t, e.memberAddRace(t), false) -} - func TestPlanLimitRace_Webhooks_CompetingCreateInWindow_Refused(t *testing.T) { e := newPlanLimitEnv(t) e.runWorkspaceRace(t, e.webhookRace(t), true) @@ -653,7 +621,6 @@ func TestPlanLimit_SelfHosted_NotEnforced(t *testing.T) { }{ {"Items", (*planLimitEnv).itemCreateRace}, {"ItemsImport", (*planLimitEnv).itemImportRace}, - {"Members", (*planLimitEnv).memberAddRace}, {"Webhooks", (*planLimitEnv).webhookRace}, } { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/server/handlers_workspace_cap_test.go b/internal/server/handlers_workspace_cap_test.go index 524eac4c1..a353cf996 100644 --- a/internal/server/handlers_workspace_cap_test.go +++ b/internal/server/handlers_workspace_cap_test.go @@ -242,7 +242,9 @@ func TestPlanLimitError_ResponseShape(t *testing.T) { wsSlug := wsResp.Slug // Add existing users until we're at the member cap (3 for free tier). - // The owner themselves count as 1, so invite 2 more real users. + // The owner themselves count as 1, so add 2 more. Added through the store: + // since BUG-2136 an invite only asks, and this test's subject is the + // invite door's refusal AT the cap. for i := 2; i <= 3; i++ { member, err := srv.store.CreateUser(models.UserCreate{ Email: fmt.Sprintf("member%d@test.com", i), @@ -253,10 +255,12 @@ func TestPlanLimitError_ResponseShape(t *testing.T) { if err != nil { t.Fatalf("CreateUser(member%d): %v", i, err) } - invRR := doRequestWithCookie(srv, "POST", "/api/v1/workspaces/"+wsSlug+"/members/invite", - map[string]string{"email": member.Email, "role": "editor"}, ownerToken) - if invRR.Code != http.StatusCreated { - t.Fatalf("invite member %d: expected 201, got %d: %s", i, invRR.Code, invRR.Body.String()) + ws, err := srv.store.GetWorkspaceBySlug(wsSlug) + if err != nil || ws == nil { + t.Fatalf("GetWorkspaceBySlug: %v", err) + } + if err := srv.store.AddWorkspaceMember(ws.ID, member.ID, "editor"); err != nil { + t.Fatalf("AddWorkspaceMember(member%d): %v", i, err) } } diff --git a/internal/server/middleware_verified_email.go b/internal/server/middleware_verified_email.go index fecd26a6b..3f4b87bc8 100644 --- a/internal/server/middleware_verified_email.go +++ b/internal/server/middleware_verified_email.go @@ -80,11 +80,17 @@ func isMutatingMethod(method string) bool { // callers have no currentUser, so emailUnverifiedBlocked already // returned false. func verifiedEmailExemptPath(path string) bool { - // Invitation accept — matched structurally so any {code} passes, - // while /invitations/{code}/preview (GET, already public) and any - // other /invitations subpath are unaffected. - if strings.HasPrefix(path, "/api/v1/invitations/") && strings.HasSuffix(path, "/accept") { - return true + // Invitation accept and decline (BUG-2136): the two answers to one + // invitation, matched structurally so any {code} passes, while + // /invitations/{code}/preview (GET, already public) and any other + // /invitations subpath are unaffected. + // Exactly two segments after the prefix (codex r2), so no nested path + // that happens to end in /accept or /decline is exempted. + if rest, ok := strings.CutPrefix(path, "/api/v1/invitations/"); ok { + if code, action, found := strings.Cut(rest, "/"); found && code != "" && + (action == "accept" || action == "decline") { + return true + } } switch path { case "/api/v1/auth/logout", diff --git a/internal/server/middleware_verified_email_test.go b/internal/server/middleware_verified_email_test.go index 85471d3a3..8edf22129 100644 --- a/internal/server/middleware_verified_email_test.go +++ b/internal/server/middleware_verified_email_test.go @@ -499,3 +499,42 @@ func TestVerifiedEmail_OAuthAuthorize_BlocksUnverified(t *testing.T) { rr := doAuthedRequest(srv, "GET", "/oauth/authorize?"+q.Encode(), nil, sessionToken) assertBlocked(t, rr, "unverified oauth authorize render") } + +// BUG-2136, codex r1: declining by code is the other answer to the same +// invitation, so it passes the gate exactly as accepting does. +func TestVerifiedEmail_InvitationDecline_CarveOut(t *testing.T) { + f := newVerifiedEmailFixture(t, true) + admin, err := f.srv.store.GetUserByEmail("admin@ve.test") + if err != nil || admin == nil { + t.Fatalf("GetUserByEmail admin: %v", err) + } + otherWS, err := f.srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "Decline Target", OwnerID: admin.ID}) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + inv, err := f.srv.store.CreateInvitation(otherWS.ID, f.unv.email, "editor", admin.ID) + if err != nil { + t.Fatalf("CreateInvitation: %v", err) + } + rr := doRequestWithCookie(f.srv, "POST", "/api/v1/invitations/"+inv.Code+"/decline", nil, f.unv.session) + assertNotEmailBlocked(t, rr, "unverified invitation-decline") + if rr.Code != http.StatusNoContent { + t.Fatalf("unverified invitation-decline: expected 204, got %d (body: %s)", rr.Code, rr.Body.String()) + } +} + +func TestVerifiedEmailExemptPath_InvitationAnswersOnly(t *testing.T) { + for path, want := range map[string]bool{ + "/api/v1/invitations/abc/accept": true, + "/api/v1/invitations/abc/decline": true, + "/api/v1/invitations/abc/preview": false, + "/api/v1/invitations/abc/nested/accept": false, + "/api/v1/invitations/x/y/decline": false, + "/api/v1/invitations//accept": false, + "/api/v1/invitations/abc": false, + } { + if got := verifiedEmailExemptPath(path); got != want { + t.Errorf("verifiedEmailExemptPath(%q) = %v, want %v", path, got, want) + } + } +} diff --git a/internal/server/my_invitations_test.go b/internal/server/my_invitations_test.go index fbed93154..55db217ad 100644 --- a/internal/server/my_invitations_test.go +++ b/internal/server/my_invitations_test.go @@ -94,13 +94,17 @@ func TestMyInvitations_ListFilters(t *testing.T) { f.owner.Username = "ownerhandle" mine := f.invite("me@example.com", "editor") - // Case differs only in how the inviter typed it; stored lowercased. + // Case differs only in how the inviter typed it; stored lowercased, + // so this is a RE-INVITE of the same address and replaces `mine` + // (BUG-2136: one live invitation per workspace and address). mineUpper := f.invite(" ME@Example.com ", "viewer") f.invite("other@example.com", "editor") - expired := f.invite("me@example.com", "editor") - expireInvitation(t, f.srv, expired.ID) - accepted := f.invite("me@example.com", "editor") - if err := f.srv.store.AcceptInvitation(accepted.ID); err != nil { + // The expired and accepted rows are inserted directly: CreateInvitation + // would replace the live one, and these legs test the LIST's filters. + expired := insertRawInvitation(t, f, d, "me@example.com", "editor") + expireInvitation(t, f.srv, expired) + accepted := insertRawInvitation(t, f, d, "me@example.com", "editor") + if err := f.srv.store.AcceptInvitation(accepted); err != nil { t.Fatalf("AcceptInvitation: %v", err) } @@ -108,9 +112,9 @@ func TestMyInvitations_ListFilters(t *testing.T) { if !got.EmailVerified { t.Fatalf("verified caller reported email_verified=false") } - if want := sortedIDs(mine.ID, mineUpper.ID); !reflect.DeepEqual(invitationIDs(got.Invitations), want) { - t.Fatalf("listed %v, want %v (not expired %s, not accepted %s, not other's)", - invitationIDs(got.Invitations), want, expired.ID, accepted.ID) + if want := sortedIDs(mineUpper.ID); !reflect.DeepEqual(invitationIDs(got.Invitations), want) { + t.Fatalf("listed %v, want %v (not the replaced %s, not expired %s, not accepted %s, not other's)", + invitationIDs(got.Invitations), want, mine.ID, expired, accepted) } for _, inv := range got.Invitations { if inv.WorkspaceSlug != f.wsSlug || inv.WorkspaceName != "Access WS" || inv.WorkspaceOwnerUsername != f.owner.Username { diff --git a/internal/server/server.go b/internal/server/server.go index 211d775b9..4732553fa 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -1915,6 +1915,7 @@ func (s *Server) setupRouter() { // Invitations (outside workspace scope) r.Post("/invitations/{code}/accept", s.handleAcceptInvitation) + r.Post("/invitations/{code}/decline", s.handleDeclineInvitation) // Non-consuming invitation preview (BUG-1934). Public/pre-auth // (exempted in isPublicAPIPath) so the logged-out /join page can @@ -1975,6 +1976,7 @@ func (s *Server) setupRouter() { // client only. r.Get("/me/invitations", s.handleListMyInvitations) r.Post("/me/invitations/{id}/accept", s.handleAcceptMyInvitation) + r.Post("/me/invitations/{id}/decline", s.handleDeclineMyInvitation) // Workspaces r.Route("/workspaces", func(r chi.Router) { diff --git a/internal/server/workspace_access_notify_test.go b/internal/server/workspace_access_notify_test.go index c6e98b675..2b2c31c69 100644 --- a/internal/server/workspace_access_notify_test.go +++ b/internal/server/workspace_access_notify_test.go @@ -203,10 +203,18 @@ func TestWorkspaceAccessChanged_ImportJSONAndBundle(t *testing.T) { func TestWorkspaceAccessChanged_InviteExistingUser(t *testing.T) { forEachDialect(t, func(t *testing.T, d store.DriverType) { f := newAccessFixture(t, d) - mkUser(t, f.srv, "invitee@example.com") + invitee := mkUser(t, f.srv, "invitee@example.com") since := f.mark() f.must(f.do("POST", "/api/v1/workspaces/"+f.wsSlug+"/members/invite", f.ownerTok, map[string]any{"email": "invitee@example.com", "role": "editor"}), http.StatusCreated, "invite") + // BUG-2136: inviting an existing account asks it; nothing is gained + // until it accepts. + f.expect(since) + ids := myInvitationIDs(t, f, f.token(invitee)) + if len(ids) != 1 { + t.Fatalf("the invitee lists %d invitations, want 1", len(ids)) + } + f.must(f.do("POST", "/api/v1/me/invitations/"+ids[0]+"/accept", f.token(invitee), nil), http.StatusOK, "accept") f.expect(since, f.line("gained", "invitee@example.com")) }) } @@ -455,6 +463,12 @@ func TestWorkspaceAccessChanged_StreamOptInAndAddressing(t *testing.T) { f.must(f.do("POST", "/api/v1/workspaces/"+f.wsSlug+"/members/invite", f.ownerTok, map[string]any{"email": "target@example.com", "role": "viewer"}), http.StatusCreated, "invite target") + // BUG-2136: the gain happens on accept, not on invite. + tids := myInvitationIDs(t, f, ttok) + if len(tids) != 1 { + t.Fatalf("the target lists %d invitations, want 1", len(tids)) + } + f.must(f.do("POST", "/api/v1/me/invitations/"+tids[0]+"/accept", ttok, nil), http.StatusOK, "target accepts") ev := waitForWatchEvent(t, targetAccess, 3*time.Second) var p watchEventPayload if err := json.Unmarshal([]byte(ev.Data), &p); err != nil { diff --git a/internal/store/bug2136_reinvite_concurrency_test.go b/internal/store/bug2136_reinvite_concurrency_test.go new file mode 100644 index 000000000..a54d92021 --- /dev/null +++ b/internal/store/bug2136_reinvite_concurrency_test.go @@ -0,0 +1,95 @@ +package store + +import ( + "errors" + "sync" + "testing" +) + +// BUG-2136: a re-invite replaces the pending invitation for its (workspace, +// address). Concurrent invites of one address must still leave exactly one +// live invitation: CreateInvitation takes the workspace lock before its +// delete-then-insert, so two of them cannot both see "nothing pending" and +// both insert. On SQLite the write lock serializes them anyway; the leg that +// can fail without the workspace lock is Postgres (make test-pg). +func TestCreateInvitation_ConcurrentReinvitesLeaveOnePending(t *testing.T) { + s := testStore(t) + owner := createTestUser(t, s, "owner-2136@example.com", "Owner", "password123") + ws := createTestWorkspace(t, s, "Reinvite race") + if err := s.AddWorkspaceMember(ws.ID, owner.ID, "owner"); err != nil { + t.Fatal(err) + } + + const n = 8 + var wg sync.WaitGroup + errs := make(chan error, n) + start := make(chan struct{}) + for i := 0; i < n; i++ { + wg.Add(1) + go func() { + defer wg.Done() + <-start + if _, err := s.CreateInvitation(ws.ID, "raced@example.com", "editor", owner.ID); err != nil { + errs <- err + } + }() + } + close(start) + wg.Wait() + close(errs) + for err := range errs { + t.Errorf("CreateInvitation: %v", err) + } + + pending, err := s.ListWorkspaceInvitations(ws.ID) + if err != nil { + t.Fatal(err) + } + if len(pending) != 1 { + t.Fatalf("%d pending invitations after %d concurrent invites of one address, want exactly 1", len(pending), n) + } +} + +// Codex r1: accept must not land after the invitation was declined (or +// replaced). The handler reads the invitation before the store transaction, +// so a decline can delete the row in between; the accept must then add no +// membership and report the invitation gone. Modelled by deleting the row +// between the read and AcceptWorkspaceInvitation. +func TestAcceptWorkspaceInvitation_RowGoneAddsNoMembership(t *testing.T) { + s := testStore(t) + owner := createTestUser(t, s, "owner-2136b@example.com", "Owner", "password123") + invitee := createTestUser(t, s, "late@example.com", "Late", "password123") + ws := createTestWorkspace(t, s, "Accept after decline") + if err := s.AddWorkspaceMember(ws.ID, owner.ID, "owner"); err != nil { + t.Fatal(err) + } + inv, err := s.CreateInvitation(ws.ID, "late@example.com", "editor", owner.ID) + if err != nil { + t.Fatal(err) + } + // Control: a live invitation accepts. + ctl, err := s.CreateInvitation(ws.ID, "control@example.com", "editor", owner.ID) + if err != nil { + t.Fatal(err) + } + ctlUser := createTestUser(t, s, "control@example.com", "Control", "password123") + if _, _, err := s.AcceptWorkspaceInvitation(ctl.ID, ws.ID, ctlUser.ID, "editor"); err != nil { + t.Fatalf("control accept: %v", err) + } + + if err := s.DeleteInvitationAdmin(inv.ID); err != nil { // the decline lands first + t.Fatal(err) + } + _, _, err = s.AcceptWorkspaceInvitation(inv.ID, ws.ID, invitee.ID, "editor") + if !errors.Is(err, ErrInvitationGone) { + t.Fatalf("accept of a declined invitation: err = %v, want ErrInvitationGone", err) + } + if member, _ := s.IsWorkspaceMember(ws.ID, invitee.ID); member { + t.Errorf("an accept that lost to a decline still added the member") + } + // A second accept of an ALREADY ACCEPTED invitation stays idempotent + // (BUG-3281): the loser of two concurrent accepts. + if _, _, err := s.AcceptWorkspaceInvitation(ctl.ID, ws.ID, ctlUser.ID, "editor"); err != nil { + t.Errorf("re-accepting an accepted invitation must stay idempotent, got %v", err) + } +} diff --git a/internal/store/workspace_members.go b/internal/store/workspace_members.go index 78f3efaf5..712d1918e 100644 --- a/internal/store/workspace_members.go +++ b/internal/store/workspace_members.go @@ -145,6 +145,11 @@ func (s *Store) commitWorkspaceMemberTx(tx *sql.Tx) error { return tx.Commit() } +// ErrInvitationGone: the invitation an accept was admitted on no longer +// exists, because it was declined, replaced or cancelled after the caller read +// it (BUG-2136). The accept is rolled back. +var ErrInvitationGone = errors.New("invitation no longer pending") + // AcceptWorkspaceInvitation accepts an invitation in one transaction: the // membership through addWorkspaceMemberTx, then the invitation's accepted_at. // An existing membership is an idempotent success (BUG-3281, lead ruling): @@ -170,9 +175,25 @@ func (s *Store) AcceptWorkspaceInvitation(invitationID, workspaceID, userID, rol return false, "", err } // The first accept's timestamp stands; a concurrent loser leaves it alone. - if _, err := tx.Exec(s.q(`UPDATE workspace_invitations SET accepted_at = ? WHERE id = ? AND accepted_at IS NULL`), now(), invitationID); err != nil { + res, err := tx.Exec(s.q(`UPDATE workspace_invitations SET accepted_at = ? WHERE id = ? AND accepted_at IS NULL`), now(), invitationID) + if err != nil { return false, "", fmt.Errorf("accept workspace invitation: %w", err) } + // Nothing updated is one of two things (BUG-2136). The row is still there, + // accepted: a concurrent accept won, and this one is the idempotent loser + // (BUG-3281). The row is GONE: it was declined, replaced or cancelled after + // the caller read it, and this accept must not land, so the membership + // written above is rolled back. On Postgres the UPDATE holds the row's lock + // until commit, so a decline racing it waits and then finds nothing pending. + if n, rerr := res.RowsAffected(); rerr == nil && n == 0 { + var one int + switch qerr := tx.QueryRow(s.q(`SELECT 1 FROM workspace_invitations WHERE id = ?`), invitationID).Scan(&one); { + case errors.Is(qerr, sql.ErrNoRows): + return false, "", ErrInvitationGone + case qerr != nil: + return false, "", fmt.Errorf("accept workspace invitation: recheck: %w", qerr) + } + } if err := s.commitWorkspaceMemberTx(tx); err != nil { // added and effectiveRole describe what this transaction wrote, so they // are returned with a commit error: if the caller finds the commit @@ -963,11 +984,23 @@ func (s *Store) CreateInvitation(workspaceID, email, role, invitedBy string) (*m return nil, fmt.Errorf("insert invitation: begin: %w", err) } defer func() { _ = tx.Rollback() }() + // One live invitation per (workspace, address) (BUG-2136): a re-invite + // REPLACES the pending one, so a fresh code, role and expiry win and the + // old code stops working. The workspace lock serializes two concurrent + // invites of one address on Postgres (SQLite's write lock already does); + // it is taken first, the order the other workspace writers use. + if err := s.acquireWorkspaceSeqLock(tx, workspaceID); err != nil { + return nil, fmt.Errorf("insert invitation: lock: %w", err) + } if invitedBy != "" { if err := s.requireActiveUserTx(tx, invitedBy); err != nil { return nil, err } } + if _, err := tx.Exec(s.q(`DELETE FROM workspace_invitations WHERE workspace_id = ? AND email = ? AND accepted_at IS NULL`), + workspaceID, strings.ToLower(strings.TrimSpace(email))); err != nil { + return nil, fmt.Errorf("insert invitation: replace pending: %w", err) + } if _, err := tx.Exec(s.q(` INSERT INTO workspace_invitations (id, workspace_id, email, role, invited_by, code, code_hash, created_at, expires_at, proof_hash) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) diff --git a/web/e2e/lib/permission-walk.ts b/web/e2e/lib/permission-walk.ts index edbbb8b2c..f65e1cb80 100644 --- a/web/e2e/lib/permission-walk.ts +++ b/web/e2e/lib/permission-walk.ts @@ -151,6 +151,24 @@ async function createItems( return items; } +/** + * Accept, as the invitee, the pending invitation to workspace `ws` (BUG-2136: + * an invite no longer adds an existing account, it asks). + */ +async function acceptInvitationTo(baseURL: string, token: string, ws: string, key: string): Promise { + const invitee = await bearer(baseURL, token); + try { + const list = (await ( + await expectOk(await invitee.get('/api/v1/me/invitations'), `list invitations for ${key}`) + ).json()) as { invitations: { id: string; workspace_slug: string }[] }; + const inv = list.invitations.find((i) => i.workspace_slug === ws); + if (!inv) throw new Error(`no pending invitation to ${ws} for ${key}`); + await expectOk(await invitee.post(`/api/v1/me/invitations/${inv.id}/accept`), `accept for ${key}`); + } finally { + await invitee.dispose(); + } +} + /** Seed one walk world. Each call builds a fresh, independent one. */ export async function seedPermissionWalk(): Promise { const { baseURL, adminSessionToken } = suiteFixture(); @@ -195,6 +213,8 @@ export async function seedPermissionWalk(): Promise { }), `invite ${key}` ); + // BUG-2136: an invite asks; the account joins when it accepts. + await acceptInvitationTo(baseURL, accounts[key].token, ws, key); } const tasksColl = (await ( diff --git a/web/e2e/workspace-access-live.spec.ts b/web/e2e/workspace-access-live.spec.ts index b1d1a5517..809c088bb 100644 --- a/web/e2e/workspace-access-live.spec.ts +++ b/web/e2e/workspace-access-live.spec.ts @@ -112,6 +112,13 @@ async function seed(fixture: SuiteFixture): Promise { await owner.api.post(`/api/v1/workspaces/${shared}/members/invite`, { data: { email: member.email, role: 'editor' } }), 'invite' ); + // BUG-2136: an invite asks; the member joins when they accept. + const list = (await (await ok(await member.api.get('/api/v1/me/invitations'), 'list invitations')).json()) as { + invitations: { id: string; workspace_slug: string }[]; + }; + const inv = list.invitations.find((i) => i.workspace_slug === shared); + if (!inv) throw new Error(`no pending invitation to ${shared}`); + await ok(await member.api.post(`/api/v1/me/invitations/${inv.id}/accept`), 'accept'); await setOpenSet(member, [own, shared]); return { owner, member, own, shared }; }