diff --git a/CLAUDE.md b/CLAUDE.md index 32e711444..80029c845 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -283,7 +283,7 @@ Collection names accept singular forms: `task`→`tasks`, `idea`→`ideas`, `doc ## MCP server -Pad runs as a local Model Context Protocol server so Claude Desktop / Cursor / Windsurf can call non-interactive `pad` commands as tools. The tool surface is a **hand-curated catalog** (currently v0.30) in `internal/mcp/catalog_*.go` — one ToolDef per resource (`pad_item`, `pad_workspace`, `pad_collection`, `pad_project`, `pad_role`, `pad_search`, `pad_meta`, `pad_playbook`, `pad_library`, `pad_attachment`) with an `action` enum dispatching to underlying CLI commands. v0.30 (BUG-2870) makes one `--field key=value` entry mean ONE thing at every door. Six sites parsed that entry independently — `item create`, `item list`, `item update`, `item move` and `item copy` in `cmd/pad`, plus `ingestFieldKVP` on the remote door — in four spellings, so `field:[" effort=l"]` stored an undeclared field literally named `" effort"` through the CLI while the remote door trimmed and wrote `effort`: the same call storing two different keys, decided by the transport. All six now call `items.SplitFieldEntry`, under two deliberately asymmetric rules — a padded KEY is REFUSED everywhere (trimming silently retargets the write to a field the caller did not type), a VALUE is carried VERBATIM everywhere (trimming reinterprets a caller's bytes, and on a text field the padding is content). The catalog's conflict pass is re-grounded on the same change, since its rules were derived from that trimming: comparisons are RAW, a padded entry is refused in the pass rather than skipped, `parseFieldArray`'s refusal is PROPAGATED rather than swallowed (it had no second owner on the no-`fields` path, where four existing refusals were landing as successes), and canonicalization and the re-emission path are gone — nothing rewrites a caller's key any more. v0.29 (PLAN-2857 / TASK-2878) makes a `relation` field value have to NAME A LIVE ITEM in the collection that field declares. `internal/items` only ever checked the SHAPE of a relation ("must be a string") because that package is DB-free, so any string at all was accepted and stored and no client could render it honestly. Every write door now refuses a value that names nothing, names an item in the WRONG collection, sits in a field declaring no target collection, or is a SLUG (deliberate divergence from `ResolveItem` — a slug is neither an ID nor stable). A CARRIED value, already on the item and asserted by nobody, is never refused: within a workspace it resolves and survives; across a boundary it is dropped without a lookup and reported in `warnings.dropped_fields`. v0.28 (IDEA-2641) adds two ADDITIVE `pad_item` actions — `remind` (arm a one-shot reminder at an RFC3339 `remind_at` instant) and `ack-reminder` (acknowledge a fired one by `reminder_id`). Agents already RECEIVED reminders, since the poll surface is `pad_project.next` / `ready`; what was missing is the other half — deferring work is exactly when an agent knows it wants to be asked again. A bare date is refused rather than read as midnight. Re-arm and disarm stay CLI-only until a listing action exists to discover an id. v0.27 (BUG-2850) typed field values server-side, carried the `fields` object with its JSON types intact, named undeclared keys back in `warnings.undeclared_fields`, and replaced the per-site conflict guards with one check that refuses two names for one target in a single call. v0.26 (IDEA-2756) makes `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant carries `may_create_workspaces=false` — that consent checkbox previously gated only the post-creation auto-add, so a connection whose user declined it created workspaces anyway — invisible to it when the connection carried an explicit allow-list, visible when it carried the `all_current_workspaces` wildcard; the consent mismatch is the defect in both cases. The same gate covers `POST /workspaces/import` (a second door onto `store.ImportWorkspace` → `CreateWorkspace`, with no MCP action today). No escape-hatch param, deliberately: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps`. v0.25 (TASK-2657 / BUG-2702) makes `pad_library.activate` resolve its destination collection from the target's declared artifact kind rather than the literal `conventions` / `playbooks` slugs, and surfaces a lookup ERROR instead of falling back. v0.24 (#1066) makes the `pad_item` `fields` OBJECT a real write form on create/update — reads return `fields` as a native object (BUG-991 normalization), and writing that shape back was a silent no-op: not a declared param, no `additionalProperties`, so it was accepted, never mapped by `BuildCLIArgs`, and dropped while the PATCH still bumped `updated_at`. The alias merges into the same path as `field: ["key=value"]` / the dedicated params (`catalog_item_fields.go`), refusing the same key in two places with conflicting values; and input validation is now STRICT across all catalog tools — an undeclared top-level key fails with a structured `validation_failed` naming it, instead of being silently dropped (a small documented compat list survives: pad_item's v0.16 `assigned_user_id` / `agent_role_id` remote clear form). One bump covers both halves — they are one contract change. v0.23 (BUG-2627 part 2 + BUG-2675, PR #1166) refuses raw `field` setters naming system-metadata keys in `fields_patch` on every transport (`github_pr` exempt on UPDATE only — the sole remote writer, itself broken: BUG-2696) and adds the retry-hostile `stored_state_unreadable` error code. v0.22 (BUG-2674, PR #1165) makes reserved metadata survive a move and refuses `field` setters naming those keys on move/copy — see `internal/mcp/version.go` for both full entries. v0.21 (BUG-2608) bounds `pad_item.action=history`, which was unbounded on every surface: the `limit` param now covers it (default 50, max 300 — the NEWEST N versions, with no `offset`, because reverse-patch storage makes only a newest-end window cheap to reconstruct), applied in the CATALOG action so it lands on both transports, and summary mode now asks the server to skip patch resolution (`?summary=true`) instead of resolving every body and discarding it. Additive param bump — `limit` already existed and nothing changed shape. v0.20 (BUG-2302 + BUG-2305, one bump) adds explicit MCP tool annotations (`readOnlyHint`/`destructiveHint`/`idempotentHint` derived from the catalog's own write-shape knowledge, fixing read-only tools that advertised `destructiveHint:true`) and makes `pad_item.list` summary-shaped on the REMOTE /mcp transport too (the hand-written `dispatchItemList` projects via `cli.ToItemSummaries`; `full=true` opts back into complete bodies) — see `internal/mcp/version.go` for the authoritative per-version changelog. Post-0.20 without a bump (BUG-2304): `item backlinks` / `item history` / `project report` gained HTTP route coverage — they were advertised but answered "not yet implemented over HTTP transport" — and a catalog↔route parity test (`dispatch_http_parity_test.go`) now drives every catalog action and fails on any future advertised-but-unrouted action; no names, enums, or shapes changed, hence no bump. v0.19 adds a `clear_parent` boolean to `pad_item` — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` bareword flag on `pad item update` (BUG-2078). v0.18 adds `clear_assigned_user` / `clear_agent_role` booleans to `pad_item` — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` bareword flags on `pad item update` (IDEA-2584). Update-only, deliberately asymmetric with create. v0.17 carries the empty-string clear to the LOCAL STDIO transport, which shells out to the CLI — `cmd/pad/cmd_item.go` now lifts `assigned_user_id` / `agent_role_id` onto their columns instead of into the fields blob, on create and update (BUG-2583). v0.16 makes an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an MCP agent can finally unassign an item (TASK-2571). v0.15 adds the `pad_item.list` `unparented` boolean, mutually exclusive with `parent`, for items with no parent or implements relationship (TASK-2096). v0.2 introduced the catalog (PLAN-969 / TASK-981); v0.3 added `pad_playbook`, `pad_meta.action: bootstrap`, `pad_set_workspace`'s embedded-bootstrap response, and the `pad://workspace/{ws}/bootstrap` resource (PLAN-1377 / TASK-1380); v0.4 trimmed the bootstrap payload by ~40% (PLAN-1410) — slim `BootstrapCollection` + `BootstrapRole` projections (no UUIDs/timestamps/settings; nested `schema` object; redundant labels omitted), removed top-level `recent_activity` duplicate, dropped convention `slug`, and added a `BootstrapDashboard` wrapper that caps five sub-arrays (`attention`, `recent_activity`, `active_items`, `active_plans`, `by_role`) at 5 entries each with parallel `*_overflow_count` fields. The pre-catalog v0.1 cmdhelp leaf walker is retired. +Pad runs as a local Model Context Protocol server so Claude Desktop / Cursor / Windsurf can call non-interactive `pad` commands as tools. The tool surface is a **hand-curated catalog** (currently v0.31) in `internal/mcp/catalog_*.go` — one ToolDef per resource (`pad_item`, `pad_workspace`, `pad_collection`, `pad_project`, `pad_role`, `pad_search`, `pad_meta`, `pad_playbook`, `pad_library`, `pad_attachment`) with an `action` enum dispatching to underlying CLI commands. v0.31 (PLAN-2857 U6 / TASK-2996) lets a `relation` value be an EXACT TITLE, scoped to the collection that field declares, alongside the UUID and ref v0.29 accepted; adds a `relation_targets` member to reads hydrating each stored id to `{id, ref, title}`; and makes the `fields` DSL take the target collection slug as a relation's third part. The first two are additive — a title is a THIRD spelling, and `relation_targets` is `omitempty` — so the bump is owed by the DSL: `fields="owner:relation"` used to be ACCEPTED and is now REFUSED, because the old parser put the third part into `Options` for every type and therefore built a relation with NO target, a field every subsequent write refused with `target_missing`. Scoping is the point: a title unique only workspace-wide refuses and names the collection searched, and two matches inside the declared collection get their own `ambiguous` reason rather than `not_found`, which would state the opposite of what happened. The ladder is UUID, then ref, then title, so an item literally TITLED `COLO-3` is unreachable by title while the ref resolves. An id-only `relation_targets` entry means the target is GONE **or** the caller may not SEE it — the two are made indistinguishable deliberately, matching the write side's `wrong_collection`→`not_found` collapse, so no consumer may render it as either. v0.30 (BUG-2870) makes one `--field key=value` entry mean ONE thing at every door. Six sites parsed that entry independently — `item create`, `item list`, `item update`, `item move` and `item copy` in `cmd/pad`, plus `ingestFieldKVP` on the remote door — in four spellings, so `field:[" effort=l"]` stored an undeclared field literally named `" effort"` through the CLI while the remote door trimmed and wrote `effort`: the same call storing two different keys, decided by the transport. All six now call `items.SplitFieldEntry`, under two deliberately asymmetric rules — a padded KEY is REFUSED everywhere (trimming silently retargets the write to a field the caller did not type), a VALUE is carried VERBATIM everywhere (trimming reinterprets a caller's bytes, and on a text field the padding is content). The catalog's conflict pass is re-grounded on the same change, since its rules were derived from that trimming: comparisons are RAW, a padded entry is refused in the pass rather than skipped, `parseFieldArray`'s refusal is PROPAGATED rather than swallowed (it had no second owner on the no-`fields` path, where four existing refusals were landing as successes), and canonicalization and the re-emission path are gone — nothing rewrites a caller's key any more. v0.29 (PLAN-2857 / TASK-2878) makes a `relation` field value have to NAME A LIVE ITEM in the collection that field declares. `internal/items` only ever checked the SHAPE of a relation ("must be a string") because that package is DB-free, so any string at all was accepted and stored and no client could render it honestly. Every write door now refuses a value that names nothing, names an item in the WRONG collection, sits in a field declaring no target collection, or is a SLUG (deliberate divergence from `ResolveItem` — a slug is neither an ID nor stable). A CARRIED value, already on the item and asserted by nobody, is never refused: within a workspace it resolves and survives; across a boundary it is dropped without a lookup and reported in `warnings.dropped_fields`. v0.28 (IDEA-2641) adds two ADDITIVE `pad_item` actions — `remind` (arm a one-shot reminder at an RFC3339 `remind_at` instant) and `ack-reminder` (acknowledge a fired one by `reminder_id`). Agents already RECEIVED reminders, since the poll surface is `pad_project.next` / `ready`; what was missing is the other half — deferring work is exactly when an agent knows it wants to be asked again. A bare date is refused rather than read as midnight. Re-arm and disarm stay CLI-only until a listing action exists to discover an id. v0.27 (BUG-2850) typed field values server-side, carried the `fields` object with its JSON types intact, named undeclared keys back in `warnings.undeclared_fields`, and replaced the per-site conflict guards with one check that refuses two names for one target in a single call. v0.26 (IDEA-2756) makes `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant carries `may_create_workspaces=false` — that consent checkbox previously gated only the post-creation auto-add, so a connection whose user declined it created workspaces anyway — invisible to it when the connection carried an explicit allow-list, visible when it carried the `all_current_workspaces` wildcard; the consent mismatch is the defect in both cases. The same gate covers `POST /workspaces/import` (a second door onto `store.ImportWorkspace` → `CreateWorkspace`, with no MCP action today). No escape-hatch param, deliberately: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps`. v0.25 (TASK-2657 / BUG-2702) makes `pad_library.activate` resolve its destination collection from the target's declared artifact kind rather than the literal `conventions` / `playbooks` slugs, and surfaces a lookup ERROR instead of falling back. v0.24 (#1066) makes the `pad_item` `fields` OBJECT a real write form on create/update — reads return `fields` as a native object (BUG-991 normalization), and writing that shape back was a silent no-op: not a declared param, no `additionalProperties`, so it was accepted, never mapped by `BuildCLIArgs`, and dropped while the PATCH still bumped `updated_at`. The alias merges into the same path as `field: ["key=value"]` / the dedicated params (`catalog_item_fields.go`), refusing the same key in two places with conflicting values; and input validation is now STRICT across all catalog tools — an undeclared top-level key fails with a structured `validation_failed` naming it, instead of being silently dropped (a small documented compat list survives: pad_item's v0.16 `assigned_user_id` / `agent_role_id` remote clear form). One bump covers both halves — they are one contract change. v0.23 (BUG-2627 part 2 + BUG-2675, PR #1166) refuses raw `field` setters naming system-metadata keys in `fields_patch` on every transport (`github_pr` exempt on UPDATE only — the sole remote writer, itself broken: BUG-2696) and adds the retry-hostile `stored_state_unreadable` error code. v0.22 (BUG-2674, PR #1165) makes reserved metadata survive a move and refuses `field` setters naming those keys on move/copy — see `internal/mcp/version.go` for both full entries. v0.21 (BUG-2608) bounds `pad_item.action=history`, which was unbounded on every surface: the `limit` param now covers it (default 50, max 300 — the NEWEST N versions, with no `offset`, because reverse-patch storage makes only a newest-end window cheap to reconstruct), applied in the CATALOG action so it lands on both transports, and summary mode now asks the server to skip patch resolution (`?summary=true`) instead of resolving every body and discarding it. Additive param bump — `limit` already existed and nothing changed shape. v0.20 (BUG-2302 + BUG-2305, one bump) adds explicit MCP tool annotations (`readOnlyHint`/`destructiveHint`/`idempotentHint` derived from the catalog's own write-shape knowledge, fixing read-only tools that advertised `destructiveHint:true`) and makes `pad_item.list` summary-shaped on the REMOTE /mcp transport too (the hand-written `dispatchItemList` projects via `cli.ToItemSummaries`; `full=true` opts back into complete bodies) — see `internal/mcp/version.go` for the authoritative per-version changelog. Post-0.20 without a bump (BUG-2304): `item backlinks` / `item history` / `project report` gained HTTP route coverage — they were advertised but answered "not yet implemented over HTTP transport" — and a catalog↔route parity test (`dispatch_http_parity_test.go`) now drives every catalog action and fails on any future advertised-but-unrouted action; no names, enums, or shapes changed, hence no bump. v0.19 adds a `clear_parent` boolean to `pad_item` — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` bareword flag on `pad item update` (BUG-2078). v0.18 adds `clear_assigned_user` / `clear_agent_role` booleans to `pad_item` — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` bareword flags on `pad item update` (IDEA-2584). Update-only, deliberately asymmetric with create. v0.17 carries the empty-string clear to the LOCAL STDIO transport, which shells out to the CLI — `cmd/pad/cmd_item.go` now lifts `assigned_user_id` / `agent_role_id` onto their columns instead of into the fields blob, on create and update (BUG-2583). v0.16 makes an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an MCP agent can finally unassign an item (TASK-2571). v0.15 adds the `pad_item.list` `unparented` boolean, mutually exclusive with `parent`, for items with no parent or implements relationship (TASK-2096). v0.2 introduced the catalog (PLAN-969 / TASK-981); v0.3 added `pad_playbook`, `pad_meta.action: bootstrap`, `pad_set_workspace`'s embedded-bootstrap response, and the `pad://workspace/{ws}/bootstrap` resource (PLAN-1377 / TASK-1380); v0.4 trimmed the bootstrap payload by ~40% (PLAN-1410) — slim `BootstrapCollection` + `BootstrapRole` projections (no UUIDs/timestamps/settings; nested `schema` object; redundant labels omitted), removed top-level `recent_activity` duplicate, dropped convention `slug`, and added a `BootstrapDashboard` wrapper that caps five sub-arrays (`attention`, `recent_activity`, `active_items`, `active_plans`, `by_role`) at 5 entries each with parallel `*_overflow_count` fields. The pre-catalog v0.1 cmdhelp leaf walker is retired. cmdhelp is still consumed at dispatch time — `BuildCLIArgs` reads individual command schemas to translate the catalog's snake_case input map into CLI args. cmdhelp no longer drives tool naming or count. @@ -297,7 +297,7 @@ pad mcp status # Install state across supported clients ``` Surface: -- **Tools:** the v0.26 catalog — ten resource × action tools (`pad_item`, `pad_workspace`, `pad_collection`, `pad_project`, `pad_role`, `pad_search`, `pad_meta`, `pad_playbook`, `pad_library`, `pad_attachment`) plus `pad_set_workspace` (takes a `workspace` slug only — no action enum). The ten resource × action tools take `action: ` to choose what they do. `pad_item` (v0.19) exposes `clear_parent` as the canonical parent-detach (update only); (v0.18) exposes `clear_assigned_user` / `clear_agent_role` booleans as the canonical unassign (update only); (v0.17) treats an empty-string `assigned_user_id` / `agent_role_id` as a clear on BOTH transports via `field: ["assigned_user_id="]` — the direct param form is remote-only, since it isn't schema-declared and stdio's BuildCLIArgs drops unknown keys (IDEA-2584); v0.16 fixed remote only; (v0.15) adds the `unparented` list parameter; v0.14 added `history` + `expected_updated_at`. `pad_project` (v0.13) adds `ready` (actionable backlog) + `stale` (items needing attention); `pad_project.activity` (v0.12) is the non-streaming, bounded activity feed — catch up on what other agents/users changed since you last worked. `pad_attachment` is the read-only attachment-metadata surface — `list`/`show` (upload/download/view stay CLI-only). `pad_library` is the convention+playbook library surface — `list`/`get`/`activate`. `pad_playbook` is the playbook surface from PLAN-1377 — `list`/`get`/`run` mirror the CLI's `pad playbook` subcommands; `run` is side-effect-free and returns the body + bound args for the agent to execute. v0.4 (PLAN-1410) didn't change the tool/action surface; it trimmed the bootstrap JSON those tools/resources return — see the Stability contract subsection below for details. +- **Tools:** the v0.31 catalog — ten resource × action tools (`pad_item`, `pad_workspace`, `pad_collection`, `pad_project`, `pad_role`, `pad_search`, `pad_meta`, `pad_playbook`, `pad_library`, `pad_attachment`) plus `pad_set_workspace` (takes a `workspace` slug only — no action enum). The ten resource × action tools take `action: ` to choose what they do. `pad_item` (v0.19) exposes `clear_parent` as the canonical parent-detach (update only); (v0.18) exposes `clear_assigned_user` / `clear_agent_role` booleans as the canonical unassign (update only); (v0.17) treats an empty-string `assigned_user_id` / `agent_role_id` as a clear on BOTH transports via `field: ["assigned_user_id="]` — the direct param form is remote-only, since it isn't schema-declared and stdio's BuildCLIArgs drops unknown keys (IDEA-2584); v0.16 fixed remote only; (v0.15) adds the `unparented` list parameter; v0.14 added `history` + `expected_updated_at`. `pad_project` (v0.13) adds `ready` (actionable backlog) + `stale` (items needing attention); `pad_project.activity` (v0.12) is the non-streaming, bounded activity feed — catch up on what other agents/users changed since you last worked. `pad_attachment` is the read-only attachment-metadata surface — `list`/`show` (upload/download/view stay CLI-only). `pad_library` is the convention+playbook library surface — `list`/`get`/`activate`. `pad_playbook` is the playbook surface from PLAN-1377 — `list`/`get`/`run` mirror the CLI's `pad playbook` subcommands; `run` is side-effect-free and returns the body + bound args for the agent to execute. v0.4 (PLAN-1410) didn't change the tool/action surface; it trimmed the bootstrap JSON those tools/resources return — see the Stability contract subsection below for details. - **Resources:** `pad://workspace/{ws}/items/{ref}`, `pad://workspace/{ws}/items`, `pad://workspace/{ws}/dashboard`, `pad://workspace/{ws}/collections`, `pad://workspace/{ws}/attachments/{id}` (bounded base64 image via `thumb-md`; non-images and image bytes over 1 MiB (pre-base64) rejected), `pad://workspace/{ws}/bootstrap` (one-shot workspace overview — user + collections + always-on conventions + roles + playbook metadata + dashboard + recent activity), plus the server-wide `pad://_meta/version`. - **Prompts:** `pad_plan`, `pad_ideate`, `pad_retro`, `pad_onboard` — multi-step workflows lifted from `skills/pad/SKILL.md`. @@ -305,7 +305,7 @@ Surface: **Stability contract.** Two version constants live in `internal/mcp/version.go`, advertised in the handshake under `capabilities.experimental.padCmdhelp` and `capabilities.experimental.padToolSurface`: - `CmdhelpVersion` (currently `"0.1"`) — the cmdhelp CLI help-tree contract. Bump when CLI flag/arg schemas change incompatibly. -- `ToolSurfaceVersion` (currently `"0.30"`) — the MCP tool catalog contract. Bump when tool names, action enums, or parameter shapes change incompatibly. **v0.30** (BUG-2870) is a BEHAVIOR bump on the v0.29/v0.27/v0.26/v0.16/v0.10/v0.9 grounds — no tool name, action enum or param shape changed. **Every door refuses a padded KEY now**, and each was accepting it differently: /mcp trimmed it and wrote the declared field, the CLI stored a ghost field beside it — so both refuse something they used to accept. What is /mcp-only is the VALUE half, which it used to trim and type and now passes through to the same validation the CLI has always applied. A caller writing canonical entries sees no difference at either door. A second, separately-noticeable fix rides with it: `detectFieldConflicts` swallowed `parseFieldArray`'s refusal as "the caller owns this error surface", which held only while the sole possible error was a shape error — `reshapeItemFields` returns early with no `fields` object, so on the no-`fields` path (this bug's own path) four existing refusals were landing as successes. No escape hatch, for v0.29's reason: there is no legitimate call this refuses, only calls whose two readings a door used to choose between silently. **v0.29** (PLAN-2857 U1 / TASK-2878) is a BEHAVIOR bump on the v0.27/v0.26/v0.16/v0.10/v0.9 grounds (NOT v0.28's, which was purely additive) — no tool name, action enum, or param shape changed, but a `relation` field value must now name a live item in the collection that field declares, so every write door refuses values it used to store. Refused: names nothing (`not_found`), names an item in the wrong collection, the field declares no target collection (`target_missing`), or the value is a SLUG — a deliberate divergence from `ResolveItem`'s UUID→ref→slug ladder, since a slug is neither an ID nor stable and free text like "red" resolving to whatever is slugged `red` today is exactly the corruption this closes. Ordinary `validation_error`, no new code and no new details key, because stdio classifies errors by matching CLI stderr prose. A CARRIED value — already on the item, asserted by nobody — is never refused, since refusing would make every legacy item un-updatable, un-movable and un-copyable: within a workspace it resolves and SURVIVES, across a workspace boundary it is dropped without a lookup (a source-workspace id cannot mean anything in the destination) and reported through the same `warnings.dropped_fields` channel BUG-2674 established. No escape hatch, deliberately: unlike v0.10's `allow_draft` there is no legitimate call this refuses, and the case with a real claim to leniency is already exempt by provenance rather than by a flag. **v0.28** (IDEA-2641 / GitHub #1010) adds two ADDITIVE `pad_item` actions and two optional params: `remind` arms a one-shot reminder at an RFC3339 `remind_at` INSTANT, and `ack-reminder` acknowledges a fired one by `reminder_id`. Purely additive — nothing existing moved, and a v0.27 consumer that enumerates neither action is unaffected; same disposition as v0.13 / v0.11 / v0.8, which likewise wired existing CLI verbs onto the catalog. Agents already RECEIVED reminders (the poll surface is `pad_project.next` / `ready`, long exposed); what was missing is the half where an agent that defers work can say when it wants to be asked again. `remind_at` REFUSES a bare date rather than reading it as midnight — the `date` schema type accepts `YYYY-MM-DD` so a caller will try it, but a bare date names a 24-hour span and choosing an hour inside it would fire at a time nobody picked. Re-arm and disarm stay CLI-only: both address a reminder by an id the agent would have to list first, and no listing action exists on this surface yet — a door with no handle. **v0.27** (BUG-2850) types field values SERVER-SIDE at all eight validate sites, carries the `fields` object to the remote door with its JSON types intact, accepts undeclared keys while NAMING them in `warnings.undeclared_fields`, and replaces five accreted conflict guards with one canonical pass; the merge refuses several ambiguities it used to resolve silently. (This entry was missing from CLAUDE.md — the 0.27 unit swept `instructions.md` and `README.md` and not this file.) **v0.27** (BUG-2850) types field values server-side at all eight `Validate*` call sites so a declared number/json field is writable from the remote transport at all, carries the `fields` OBJECT with its JSON types intact, names undeclared keys back in `warnings.undeclared_fields` (accepted rather than refused — a census of 1012 items found 14 such keys across 168 live values, so refusing would have broken read-modify-write on items nobody had edited wrongly), and replaces the accreted per-site conflict guards with ONE check over a canonical view of every source; that check refuses ambiguities v0.26 resolved silently, chiefly two names for one target in a single call (`parent`/`plan`, `assign`/`assigned_user_id`, `role`/`agent_role_id`), refused even when the values match because the names address one thing through incomparable vocabularies and the two doors resolved them differently. **v0.26** (IDEA-2756) is a BEHAVIOR bump on the v0.9/v0.16/v0.25 grounds — no tool name, action enum, or param shape changed, but `pad_workspace.create` now refuses a call it used to permit. Closest precedent is v0.10, which likewise turned a server-side gate into a structured refusal; unlike v0.10 there is no `allow_draft`-style override, because the gate encodes a decision the USER made at consent time and a bypass param would be the app overriding its own grant. `POST /workspaces/import` is gated by the same shared helper (import mints a workspace through `store.ImportWorkspace`), though it has no MCP action today. **v0.25** (TASK-2657 / BUG-2702) resolves `pad_library.activate`'s destination collection from the target's declared artifact kind rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly; a lookup ERROR is surfaced rather than silently falling back. **v0.24** (#1066) adds the `fields` OBJECT param to `pad_item` create/update — an alias merging into the same path as `field`/the dedicated params, so the shape reads return is finally a valid write shape; the same key supplied twice with conflicting values is REFUSED (refuse-on-ambiguity, the v0.18/v0.19 disposition), equal duplicates collapse to one write, and non-writer actions refuse a `fields` param loudly. It also makes input validation STRICT for every catalog tool: undeclared top-level keys are rejected with a structured error naming them, instead of being accepted and silently dropped by `BuildCLIArgs` — which is the mechanism that made the `fields` object a session-scoped silent no-op in the first place. Compat carve-out: `pad_item`'s v0.16 `assigned_user_id` / `agent_role_id` remote-transport clear form stays accepted (documented, undeprecated, deliberately never schema-declared). The strict half changes behaviour for inputs that previously "succeeded", but that reliance was indistinguishable from a caller bug (the key never did anything), so the break is the fix; one bump covers both halves. **v0.23** (BUG-2627 part 2 + BUG-2675) refuses system-metadata keys through `fields_patch` on all three doors at once, `github_pr` exempt on update (move/copy still refuse it), and adds the retry-hostile `stored_state_unreadable` code. **v0.22** (BUG-2674) stops `pad_item.action=move` destroying system metadata and refuses `field` setters naming the reserved keys there. **v0.21** bounds `pad_item.action=history` (BUG-2608): the `limit` param now covers it, default 50 / max 300, applied in the CATALOG action so it reaches both transports (HTTP reads the input; stdio gets the CLI's new `--limit` via BuildCLIArgs). The window is the NEWEST N and there is deliberately no `offset` — versions are reverse patches, so only a newest-end window is cheap to reconstruct. Additive param bump; a v0.20 consumer sending no limit now receives the newest 50 rather than every version, which is the fix. Summary mode additionally asks the server to skip patch resolution rather than resolving bodies the dispatcher discards. **v0.19** adds a `clear_parent` boolean to `pad_item` (BUG-2078) — an ADDITIVE param bump, same grounds as v0.18; nothing existing changed shape. The server has supported clearing a parent since BUG-2013 (`extractParentLink` treats a present-but-empty `parent` key in `fields_patch` as detach), but neither client surface could reach it — `--parent ""` was a silent no-op on the CLI and the MCP `parent` param has the same "empty means not provided" convention every other declared string on the tool has. Boolean rather than overloading the empty string, same two reasons as v0.18: keeps that invariant intact for every other param, and only a boolean reaches LOCAL STDIO via `BuildCLIArgs`, mapping to a new `--clear-parent` bareword flag exactly as `clear_assigned_user` maps to `--clear-assigned-user`. Update-only, same asymmetry as v0.18. A simultaneous `parent` + `clear_parent` — including via `field: ["parent=..."]` or the `plan` alias `extractParentLink` also accepts — is REFUSED on both transports, not silently resolved (codex round 1). Also refused, not silently applied: `clear_parent` against a collection whose schema declares its own `parent`/`plan` field — `extractParentLink` skips hierarchy handling entirely for a schema-shadowed key and lets it fall through as an ordinary field write, so the wire shape `{"parent":""}` can no longer distinguish clear-hierarchy intent from a legitimate blank-a-real-field write once it reaches the server; the ambiguity is created at the client surface that accepted `clear_parent`, so that surface refuses rather than guessing (codex round 2). **v0.18** adds `clear_assigned_user` / `clear_agent_role` booleans to `pad_item` (IDEA-2584) — an ADDITIVE param bump (v0.5/v0.6 precedent); nothing existing changed shape and v0.16/v0.17's empty-string forms still work, undeprecated. v0.16 and v0.17 made the clear WORK; nothing advertised it, because the params that do it were never in the catalog, so an agent reading the schema reached for `assign: ""` (a no-op, and it stays one). Booleans rather than declaring the string params, for two reasons: an empty DECLARED string is inert everywhere else on the tool, so giving one a destructive meaning would let a param-padding client silently unassign everything; and only a boolean can reach LOCAL STDIO, since `BuildCLIArgs` emits the CLI's real flags and a param with no flag behind it is dropped — these map to new `--clear-assigned-user` / `--clear-agent-role` bareword flags, exactly as `allow_draft` maps to `--allow-draft`. Update-only, deliberately asymmetric with create (clearing at create has no honest behaviour but a no-op; a test fails if someone adds them there). Server-side it is wiring, not new semantics: `models.ItemUpdate.ClearAssignedUser`/`ClearAgentRole` already existed with store support since BUG-2566. **v0.17** closes the transport gap v0.16 documented: local stdio MCP shells out to the CLI, which wrote `--field assigned_user_id=` into the item's FIELDS BLOB while the column stayed stale and then printed "Updated TASK-9". `cmd/pad/cmd_item.go` now lifts `columnFieldKeys` onto the columns on create AND update, mirroring `liftFieldsToColumns` and its INVARIANT. Two compat changes, ruled separately: non-empty values move to the column and stop writing the blob key (relying on the old behaviour is relying on a shadowing defect), and empty values clear (falls out of the lift, inherits BUG-2566). Existing stray blob keys are left alone — the fix stops minting new ones. Another behaviour-only bump (BUG-2583). **v0.16** lets an MCP agent UNASSIGN an item over the REMOTE transport (TASK-2571). No tool/action/param shape changed — this is a BEHAVIOR bump on the same grounds as v0.9: an empty-string `assigned_user_id` / `agent_role_id`, passed at the top level or as `field: ["assigned_user_id="]`, was silently dropped by two dispatch-path filters (`mapItemUpdate`, `liftFieldsToColumns`) and is now forwarded as a clear-to-NULL. The store has had defined clear semantics for exactly these two columns since BUG-2566 and HTTP inherited them, so this is uniformity restoration — MCP was the only surface with no way to unassign. Compat posture accepted deliberately: today's `""` senders get a no-op, and a no-op is the surprising reading. The empty-string filter on `tags` at the same call site STAYS (codex #547 r3 P2) — `tags: ""` is a corrupt JSONB/TEXT write, not a clear; same-looking guard, opposite justification. `clear_assigned_user` / `clear_agent_role` schema flags (option (b)) deliberately skipped as additive sugar, though codex review reopened the case — the catalog exposes `assign` / `role`, NOT the ID params, so an agent reading the schema still can't discover the clear (IDEA-2584); an empty `assign` is deliberately left inert because every other schema-declared string on that mapper treats empty as not-provided. **Transport scope:** v0.16 fixed the REMOTE /mcp transport only; v0.17 (BUG-2583) closed the local-stdio half at the CLI. **v0.15** adds the `unparented` boolean to `pad_item.list`, mutually exclusive with `parent`, for structural loose-item filtering (TASK-2096). **v0.14** added a `history` action to `pad_item` (read-only item version history — newest-first metadata; content body omitted for token thrift) and an `expected_updated_at` param for optimistic concurrency on `update` (round-trip the `updated_at` you last read; a stale value fails with a structured 409 `code=update_conflict`). The `update` action's field writes are now a server-side field-level MERGE (only the keys you set change) rather than a full-blob replace, closing the concurrent-update lost-write race (IDEA-1480 / TASK-2022) — pure addition to the action enum + param vocabulary; existing `pad_item` actions/params are unchanged and backwards-compatible. **v0.13** adds `ready` + `stale` actions to `pad_project`, mirroring the existing CLI `pad project ready` / `pad project stale` (TASK-2019): `ready` (read-only) returns the actionable backlog — the query-oriented counterpart to `next`, reusing the dashboard's suggested-next logic; `stale` (read-only) lists items needing attention (stalled, blocked, overdue, or out of the active workflow). Both HTTP dispatchers already existed (`dispatch_http_project.go`); this just wires them onto the catalog. `pad project reconcile` stays CLI-only (shells out to `gh` for live PR state — a local-git dependency MCP agents lack). Pure addition of two read-only actions — existing actions unchanged; backwards-compatible for v0.12 consumers that don't enumerate the new actions. **v0.12** adds an `activity` action to `pad_project`, mirroring the new CLI `pad project activity [--limit N] [--actor user|agent] [--since DATE]` (TASK-2018) — the non-streaming, bounded query counterpart to the CLI-only `pad project watch` SSE stream. Read-only snapshot of the workspace's enriched activity feed (item refs, titles, field-level change details) backed by the existing `GET /workspaces/{ws}/activity` endpoint (previously web-UI-only, now extended with a server-side `since` date filter so `limit`/`actor`/`since` behave identically across CLI, stdio MCP, and cloud HTTP), so agents can catch up on what other agents/users did since they last worked. Adds `actor` + `limit` params to the `pad_project` vocabulary (`since` already existed for changelog); pure addition — existing actions unchanged; backwards-compatible for v0.11 consumers that don't enumerate the new action. **v0.11** adds the read-only `pad_attachment` tool (the tenth resource × action tool) with `list` + `show` actions, mirroring the CLI `pad attachment list` / `pad attachment show` (TASK-2017): `list` enumerates a workspace's attachments (optional filters: item / category / collection / attached / unattached / sort / limit / offset); `show` returns one attachment's metadata (MIME, size, filename, ETag, last-modified) via a HEAD request without transferring bytes. Both HTTP dispatchers already existed (`dispatch_http_attachments.go`); this just wires them onto the catalog. Upload / download / view stay CLI-only (filesystem-bound, excluded per the catalog's exclusion rules). Pure addition — existing tools/actions unchanged; backwards-compatible for v0.10 consumers that don't enumerate the new tool. The base64 image RESOURCE for multimodal agents (`pad://workspace/{ws}/attachments/{id}`) shipped later in TASK-2077 (PR #930) as a bounded, image-only resource; TASK-2101 brought it — and the full read-only resource set — to the remote /mcp transport via the in-process `HTTPResourceFetcher`, so resources are no longer local-stdio-only. **v0.10** enforces the draft-playbook gate server-side: `pad_playbook.run` (and the underlying `POST /playbooks/{ref}/run`) now refuses a playbook whose `status` isn't `active` with a structured `playbook_not_active` error, adds an `allow_draft` boolean param (bareword `--allow-draft` on the CLI) as the escape hatch, and echoes the playbook `status` on both the `run` and `get` responses (BUG-2020). **v0.9** makes `pad_item.list` summary-shaped by default (drops item `content`, adds a default result limit of 50 / hard max 300 on MCP; CLI `--full` restores the complete shape) — a behavior change to the tool's return shape, hence the bump, though tool names, action enums, and parameter shapes are unchanged (TASK-2000). **v0.8** adds `restore` + `deleted` actions to `pad_workspace`, mirroring the CLI `pad workspace restore` / `pad workspace deleted` (TASK-1972): `deleted` (read-only) lists the caller's soft-deleted workspaces still inside the 30-day restore window; `restore` (mutating, not destructive, owner-only) un-soft-deletes a workspace by `slug` while it's still restorable. Both reuse the existing `slug` param — no new params; pure addition. **v0.7** adds `export` + `import` actions to `pad_item`, mirroring the CLI `pad item export` / `pad item import` (covers playbooks AND conventions). `export` (read-only) takes `ref` and returns the portable artifact text — it forces the CLI's stdout sink (`-o -`) so the bytes come back as the result instead of a file. `import` (mutating, not destructive) takes a new `artifact` param (the full artifact text) and returns `{ref, slug, warnings}`; the ExecDispatcher can't pipe stdin, so it spills the artifact to a temp file and dispatches `item import `. v0.6 added the `pad_item.backlinks` action; v0.5 added `pad_library`. v0.3 (PLAN-1377 / TASK-1380) introduced `pad_meta.action: bootstrap`, `pad_set_workspace`'s embedded-bootstrap response, and the `pad://workspace/{ws}/bootstrap` resource. **v0.4 (PLAN-1410)** is a comprehensive bootstrap-payload trim — same tool catalog, slimmer JSON shape inside bootstrap responses: `BootstrapCollection` projection drops `id`/`workspace_id`/timestamps/`settings` and emits `schema` as a nested object; `BootstrapRole` projection drops UUIDs/timestamps/`tools`; convention `slug` dropped; top-level `recent_activity` (a duplicate of `dashboard.recent_activity`) removed; new `BootstrapDashboard` wrapper caps five sub-arrays (`attention`, `recent_activity`, `active_items`, `active_plans`, `by_role`) at 5 entries each with parallel `*_overflow_count` fields; redundant schema labels omitted when `label == TitleCase(key)`. Cumulative size reduction: ~40% on a representative workspace, ~54% on the fixture (see PLAN-1410's Result section for per-section deltas). Compatibility: most changes are subtractive (dropped fields) or additive (overflow counts), but **one type change is breaking**: `collections[].schema` went from a JSON-encoded string to a nested JSON object — clients that JSON.parse()'d the string need to consume it directly as an object now. The dropped fields (UUIDs, timestamps, settings, duplicate `recent_activity`, convention `slug`) have canonical alternatives (slugs for addressing; `pad collection list` / `pad role list` for the full models when needed). +- `ToolSurfaceVersion` (currently `"0.31"`) — the MCP tool catalog contract. Bump when tool names, action enums, or parameter shapes change incompatibly. **v0.30** (BUG-2870) is a BEHAVIOR bump on the v0.29/v0.27/v0.26/v0.16/v0.10/v0.9 grounds — no tool name, action enum or param shape changed. **Every door refuses a padded KEY now**, and each was accepting it differently: /mcp trimmed it and wrote the declared field, the CLI stored a ghost field beside it — so both refuse something they used to accept. What is /mcp-only is the VALUE half, which it used to trim and type and now passes through to the same validation the CLI has always applied. A caller writing canonical entries sees no difference at either door. A second, separately-noticeable fix rides with it: `detectFieldConflicts` swallowed `parseFieldArray`'s refusal as "the caller owns this error surface", which held only while the sole possible error was a shape error — `reshapeItemFields` returns early with no `fields` object, so on the no-`fields` path (this bug's own path) four existing refusals were landing as successes. No escape hatch, for v0.29's reason: there is no legitimate call this refuses, only calls whose two readings a door used to choose between silently. **v0.29** (PLAN-2857 U1 / TASK-2878) is a BEHAVIOR bump on the v0.27/v0.26/v0.16/v0.10/v0.9 grounds (NOT v0.28's, which was purely additive) — no tool name, action enum, or param shape changed, but a `relation` field value must now name a live item in the collection that field declares, so every write door refuses values it used to store. Refused: names nothing (`not_found`), names an item in the wrong collection, the field declares no target collection (`target_missing`), or the value is a SLUG — a deliberate divergence from `ResolveItem`'s UUID→ref→slug ladder, since a slug is neither an ID nor stable and free text like "red" resolving to whatever is slugged `red` today is exactly the corruption this closes. Ordinary `validation_error`, no new code and no new details key, because stdio classifies errors by matching CLI stderr prose. A CARRIED value — already on the item, asserted by nobody — is never refused, since refusing would make every legacy item un-updatable, un-movable and un-copyable: within a workspace it resolves and SURVIVES, across a workspace boundary it is dropped without a lookup (a source-workspace id cannot mean anything in the destination) and reported through the same `warnings.dropped_fields` channel BUG-2674 established. No escape hatch, deliberately: unlike v0.10's `allow_draft` there is no legitimate call this refuses, and the case with a real claim to leniency is already exempt by provenance rather than by a flag. **v0.28** (IDEA-2641 / GitHub #1010) adds two ADDITIVE `pad_item` actions and two optional params: `remind` arms a one-shot reminder at an RFC3339 `remind_at` INSTANT, and `ack-reminder` acknowledges a fired one by `reminder_id`. Purely additive — nothing existing moved, and a v0.27 consumer that enumerates neither action is unaffected; same disposition as v0.13 / v0.11 / v0.8, which likewise wired existing CLI verbs onto the catalog. Agents already RECEIVED reminders (the poll surface is `pad_project.next` / `ready`, long exposed); what was missing is the half where an agent that defers work can say when it wants to be asked again. `remind_at` REFUSES a bare date rather than reading it as midnight — the `date` schema type accepts `YYYY-MM-DD` so a caller will try it, but a bare date names a 24-hour span and choosing an hour inside it would fire at a time nobody picked. Re-arm and disarm stay CLI-only: both address a reminder by an id the agent would have to list first, and no listing action exists on this surface yet — a door with no handle. **v0.27** (BUG-2850) types field values SERVER-SIDE at all eight validate sites, carries the `fields` object to the remote door with its JSON types intact, accepts undeclared keys while NAMING them in `warnings.undeclared_fields`, and replaces five accreted conflict guards with one canonical pass; the merge refuses several ambiguities it used to resolve silently. (This entry was missing from CLAUDE.md — the 0.27 unit swept `instructions.md` and `README.md` and not this file.) **v0.27** (BUG-2850) types field values server-side at all eight `Validate*` call sites so a declared number/json field is writable from the remote transport at all, carries the `fields` OBJECT with its JSON types intact, names undeclared keys back in `warnings.undeclared_fields` (accepted rather than refused — a census of 1012 items found 14 such keys across 168 live values, so refusing would have broken read-modify-write on items nobody had edited wrongly), and replaces the accreted per-site conflict guards with ONE check over a canonical view of every source; that check refuses ambiguities v0.26 resolved silently, chiefly two names for one target in a single call (`parent`/`plan`, `assign`/`assigned_user_id`, `role`/`agent_role_id`), refused even when the values match because the names address one thing through incomparable vocabularies and the two doors resolved them differently. **v0.26** (IDEA-2756) is a BEHAVIOR bump on the v0.9/v0.16/v0.25 grounds — no tool name, action enum, or param shape changed, but `pad_workspace.create` now refuses a call it used to permit. Closest precedent is v0.10, which likewise turned a server-side gate into a structured refusal; unlike v0.10 there is no `allow_draft`-style override, because the gate encodes a decision the USER made at consent time and a bypass param would be the app overriding its own grant. `POST /workspaces/import` is gated by the same shared helper (import mints a workspace through `store.ImportWorkspace`), though it has no MCP action today. **v0.25** (TASK-2657 / BUG-2702) resolves `pad_library.activate`'s destination collection from the target's declared artifact kind rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly; a lookup ERROR is surfaced rather than silently falling back. **v0.24** (#1066) adds the `fields` OBJECT param to `pad_item` create/update — an alias merging into the same path as `field`/the dedicated params, so the shape reads return is finally a valid write shape; the same key supplied twice with conflicting values is REFUSED (refuse-on-ambiguity, the v0.18/v0.19 disposition), equal duplicates collapse to one write, and non-writer actions refuse a `fields` param loudly. It also makes input validation STRICT for every catalog tool: undeclared top-level keys are rejected with a structured error naming them, instead of being accepted and silently dropped by `BuildCLIArgs` — which is the mechanism that made the `fields` object a session-scoped silent no-op in the first place. Compat carve-out: `pad_item`'s v0.16 `assigned_user_id` / `agent_role_id` remote-transport clear form stays accepted (documented, undeprecated, deliberately never schema-declared). The strict half changes behaviour for inputs that previously "succeeded", but that reliance was indistinguishable from a caller bug (the key never did anything), so the break is the fix; one bump covers both halves. **v0.23** (BUG-2627 part 2 + BUG-2675) refuses system-metadata keys through `fields_patch` on all three doors at once, `github_pr` exempt on update (move/copy still refuse it), and adds the retry-hostile `stored_state_unreadable` code. **v0.22** (BUG-2674) stops `pad_item.action=move` destroying system metadata and refuses `field` setters naming the reserved keys there. **v0.21** bounds `pad_item.action=history` (BUG-2608): the `limit` param now covers it, default 50 / max 300, applied in the CATALOG action so it reaches both transports (HTTP reads the input; stdio gets the CLI's new `--limit` via BuildCLIArgs). The window is the NEWEST N and there is deliberately no `offset` — versions are reverse patches, so only a newest-end window is cheap to reconstruct. Additive param bump; a v0.20 consumer sending no limit now receives the newest 50 rather than every version, which is the fix. Summary mode additionally asks the server to skip patch resolution rather than resolving bodies the dispatcher discards. **v0.19** adds a `clear_parent` boolean to `pad_item` (BUG-2078) — an ADDITIVE param bump, same grounds as v0.18; nothing existing changed shape. The server has supported clearing a parent since BUG-2013 (`extractParentLink` treats a present-but-empty `parent` key in `fields_patch` as detach), but neither client surface could reach it — `--parent ""` was a silent no-op on the CLI and the MCP `parent` param has the same "empty means not provided" convention every other declared string on the tool has. Boolean rather than overloading the empty string, same two reasons as v0.18: keeps that invariant intact for every other param, and only a boolean reaches LOCAL STDIO via `BuildCLIArgs`, mapping to a new `--clear-parent` bareword flag exactly as `clear_assigned_user` maps to `--clear-assigned-user`. Update-only, same asymmetry as v0.18. A simultaneous `parent` + `clear_parent` — including via `field: ["parent=..."]` or the `plan` alias `extractParentLink` also accepts — is REFUSED on both transports, not silently resolved (codex round 1). Also refused, not silently applied: `clear_parent` against a collection whose schema declares its own `parent`/`plan` field — `extractParentLink` skips hierarchy handling entirely for a schema-shadowed key and lets it fall through as an ordinary field write, so the wire shape `{"parent":""}` can no longer distinguish clear-hierarchy intent from a legitimate blank-a-real-field write once it reaches the server; the ambiguity is created at the client surface that accepted `clear_parent`, so that surface refuses rather than guessing (codex round 2). **v0.18** adds `clear_assigned_user` / `clear_agent_role` booleans to `pad_item` (IDEA-2584) — an ADDITIVE param bump (v0.5/v0.6 precedent); nothing existing changed shape and v0.16/v0.17's empty-string forms still work, undeprecated. v0.16 and v0.17 made the clear WORK; nothing advertised it, because the params that do it were never in the catalog, so an agent reading the schema reached for `assign: ""` (a no-op, and it stays one). Booleans rather than declaring the string params, for two reasons: an empty DECLARED string is inert everywhere else on the tool, so giving one a destructive meaning would let a param-padding client silently unassign everything; and only a boolean can reach LOCAL STDIO, since `BuildCLIArgs` emits the CLI's real flags and a param with no flag behind it is dropped — these map to new `--clear-assigned-user` / `--clear-agent-role` bareword flags, exactly as `allow_draft` maps to `--allow-draft`. Update-only, deliberately asymmetric with create (clearing at create has no honest behaviour but a no-op; a test fails if someone adds them there). Server-side it is wiring, not new semantics: `models.ItemUpdate.ClearAssignedUser`/`ClearAgentRole` already existed with store support since BUG-2566. **v0.17** closes the transport gap v0.16 documented: local stdio MCP shells out to the CLI, which wrote `--field assigned_user_id=` into the item's FIELDS BLOB while the column stayed stale and then printed "Updated TASK-9". `cmd/pad/cmd_item.go` now lifts `columnFieldKeys` onto the columns on create AND update, mirroring `liftFieldsToColumns` and its INVARIANT. Two compat changes, ruled separately: non-empty values move to the column and stop writing the blob key (relying on the old behaviour is relying on a shadowing defect), and empty values clear (falls out of the lift, inherits BUG-2566). Existing stray blob keys are left alone — the fix stops minting new ones. Another behaviour-only bump (BUG-2583). **v0.16** lets an MCP agent UNASSIGN an item over the REMOTE transport (TASK-2571). No tool/action/param shape changed — this is a BEHAVIOR bump on the same grounds as v0.9: an empty-string `assigned_user_id` / `agent_role_id`, passed at the top level or as `field: ["assigned_user_id="]`, was silently dropped by two dispatch-path filters (`mapItemUpdate`, `liftFieldsToColumns`) and is now forwarded as a clear-to-NULL. The store has had defined clear semantics for exactly these two columns since BUG-2566 and HTTP inherited them, so this is uniformity restoration — MCP was the only surface with no way to unassign. Compat posture accepted deliberately: today's `""` senders get a no-op, and a no-op is the surprising reading. The empty-string filter on `tags` at the same call site STAYS (codex #547 r3 P2) — `tags: ""` is a corrupt JSONB/TEXT write, not a clear; same-looking guard, opposite justification. `clear_assigned_user` / `clear_agent_role` schema flags (option (b)) deliberately skipped as additive sugar, though codex review reopened the case — the catalog exposes `assign` / `role`, NOT the ID params, so an agent reading the schema still can't discover the clear (IDEA-2584); an empty `assign` is deliberately left inert because every other schema-declared string on that mapper treats empty as not-provided. **Transport scope:** v0.16 fixed the REMOTE /mcp transport only; v0.17 (BUG-2583) closed the local-stdio half at the CLI. **v0.15** adds the `unparented` boolean to `pad_item.list`, mutually exclusive with `parent`, for structural loose-item filtering (TASK-2096). **v0.14** added a `history` action to `pad_item` (read-only item version history — newest-first metadata; content body omitted for token thrift) and an `expected_updated_at` param for optimistic concurrency on `update` (round-trip the `updated_at` you last read; a stale value fails with a structured 409 `code=update_conflict`). The `update` action's field writes are now a server-side field-level MERGE (only the keys you set change) rather than a full-blob replace, closing the concurrent-update lost-write race (IDEA-1480 / TASK-2022) — pure addition to the action enum + param vocabulary; existing `pad_item` actions/params are unchanged and backwards-compatible. **v0.13** adds `ready` + `stale` actions to `pad_project`, mirroring the existing CLI `pad project ready` / `pad project stale` (TASK-2019): `ready` (read-only) returns the actionable backlog — the query-oriented counterpart to `next`, reusing the dashboard's suggested-next logic; `stale` (read-only) lists items needing attention (stalled, blocked, overdue, or out of the active workflow). Both HTTP dispatchers already existed (`dispatch_http_project.go`); this just wires them onto the catalog. `pad project reconcile` stays CLI-only (shells out to `gh` for live PR state — a local-git dependency MCP agents lack). Pure addition of two read-only actions — existing actions unchanged; backwards-compatible for v0.12 consumers that don't enumerate the new actions. **v0.12** adds an `activity` action to `pad_project`, mirroring the new CLI `pad project activity [--limit N] [--actor user|agent] [--since DATE]` (TASK-2018) — the non-streaming, bounded query counterpart to the CLI-only `pad project watch` SSE stream. Read-only snapshot of the workspace's enriched activity feed (item refs, titles, field-level change details) backed by the existing `GET /workspaces/{ws}/activity` endpoint (previously web-UI-only, now extended with a server-side `since` date filter so `limit`/`actor`/`since` behave identically across CLI, stdio MCP, and cloud HTTP), so agents can catch up on what other agents/users did since they last worked. Adds `actor` + `limit` params to the `pad_project` vocabulary (`since` already existed for changelog); pure addition — existing actions unchanged; backwards-compatible for v0.11 consumers that don't enumerate the new action. **v0.11** adds the read-only `pad_attachment` tool (the tenth resource × action tool) with `list` + `show` actions, mirroring the CLI `pad attachment list` / `pad attachment show` (TASK-2017): `list` enumerates a workspace's attachments (optional filters: item / category / collection / attached / unattached / sort / limit / offset); `show` returns one attachment's metadata (MIME, size, filename, ETag, last-modified) via a HEAD request without transferring bytes. Both HTTP dispatchers already existed (`dispatch_http_attachments.go`); this just wires them onto the catalog. Upload / download / view stay CLI-only (filesystem-bound, excluded per the catalog's exclusion rules). Pure addition — existing tools/actions unchanged; backwards-compatible for v0.10 consumers that don't enumerate the new tool. The base64 image RESOURCE for multimodal agents (`pad://workspace/{ws}/attachments/{id}`) shipped later in TASK-2077 (PR #930) as a bounded, image-only resource; TASK-2101 brought it — and the full read-only resource set — to the remote /mcp transport via the in-process `HTTPResourceFetcher`, so resources are no longer local-stdio-only. **v0.10** enforces the draft-playbook gate server-side: `pad_playbook.run` (and the underlying `POST /playbooks/{ref}/run`) now refuses a playbook whose `status` isn't `active` with a structured `playbook_not_active` error, adds an `allow_draft` boolean param (bareword `--allow-draft` on the CLI) as the escape hatch, and echoes the playbook `status` on both the `run` and `get` responses (BUG-2020). **v0.9** makes `pad_item.list` summary-shaped by default (drops item `content`, adds a default result limit of 50 / hard max 300 on MCP; CLI `--full` restores the complete shape) — a behavior change to the tool's return shape, hence the bump, though tool names, action enums, and parameter shapes are unchanged (TASK-2000). **v0.8** adds `restore` + `deleted` actions to `pad_workspace`, mirroring the CLI `pad workspace restore` / `pad workspace deleted` (TASK-1972): `deleted` (read-only) lists the caller's soft-deleted workspaces still inside the 30-day restore window; `restore` (mutating, not destructive, owner-only) un-soft-deletes a workspace by `slug` while it's still restorable. Both reuse the existing `slug` param — no new params; pure addition. **v0.7** adds `export` + `import` actions to `pad_item`, mirroring the CLI `pad item export` / `pad item import` (covers playbooks AND conventions). `export` (read-only) takes `ref` and returns the portable artifact text — it forces the CLI's stdout sink (`-o -`) so the bytes come back as the result instead of a file. `import` (mutating, not destructive) takes a new `artifact` param (the full artifact text) and returns `{ref, slug, warnings}`; the ExecDispatcher can't pipe stdin, so it spills the artifact to a temp file and dispatches `item import `. v0.6 added the `pad_item.backlinks` action; v0.5 added `pad_library`. v0.3 (PLAN-1377 / TASK-1380) introduced `pad_meta.action: bootstrap`, `pad_set_workspace`'s embedded-bootstrap response, and the `pad://workspace/{ws}/bootstrap` resource. **v0.4 (PLAN-1410)** is a comprehensive bootstrap-payload trim — same tool catalog, slimmer JSON shape inside bootstrap responses: `BootstrapCollection` projection drops `id`/`workspace_id`/timestamps/`settings` and emits `schema` as a nested object; `BootstrapRole` projection drops UUIDs/timestamps/`tools`; convention `slug` dropped; top-level `recent_activity` (a duplicate of `dashboard.recent_activity`) removed; new `BootstrapDashboard` wrapper caps five sub-arrays (`attention`, `recent_activity`, `active_items`, `active_plans`, `by_role`) at 5 entries each with parallel `*_overflow_count` fields; redundant schema labels omitted when `label == TitleCase(key)`. Cumulative size reduction: ~40% on a representative workspace, ~54% on the fixture (see PLAN-1410's Result section for per-section deltas). Compatibility: most changes are subtractive (dropped fields) or additive (overflow counts), but **one type change is breaking**: `collections[].schema` went from a JSON-encoded string to a nested JSON object — clients that JSON.parse()'d the string need to consume it directly as an object now. The dropped fields (UUIDs, timestamps, settings, duplicate `recent_activity`, convention `slug`) have canonical alternatives (slugs for addressing; `pad collection list` / `pad role list` for the full models when needed). Post-0.30 without a bump (BUG-2995): a successful content write through the designated applier used to answer with the item's PREVIOUS content — on that path the markdown goes to a live tab's Y.Doc and the row write runs with `Content` nil, so the response described the item as it stood before the request. It now carries the content as SENT plus an additive `omitempty` `warnings.content_outcome: "applied_pending_flush"`. No bump, on the BUG-2304 rather than the v0.9 grounds: v0.9 bumped because list rows LOST fields consumers read, whereas here nothing is removed or retyped — what changed is the VALUE of one field on one path, which a caller does observe. The claim is that no DOCUMENTED OR SUPPORTED reliance breaks, not that none can exist: a consumer could have detected the applier path by noticing the response echoed something other than what it sent, and that stops working — but it was never documented, a genuinely lost write produced the same mismatch, and `warnings.content_outcome` answers that question properly in the same change. Reads are unchanged: during the window a `get` (or `list` with `full: true`) still answers from the row, so the warning means re-read later, not re-send. diff --git a/README.md b/README.md index 0b9c6b4f4..3d16f59bd 100644 --- a/README.md +++ b/README.md @@ -394,7 +394,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.30)** — ten resource × action tools plus `pad_set_workspace` (eleven total), no flat verb explosion. Undeclared input keys are rejected with a structured error rather than silently dropped. `pad_item` create/update accept field values as a `fields` object (the same shape reads return) as an equivalent to the dedicated params / `field: ["key=value"]`, and its values keep their JSON types where the transport can carry them. Field values are typed against the collection schema server-side, so a declared number or json field is writable from the remote transport (which sends every value as a string). Keys the schema does not declare are stored and NAMED back in `warnings.undeclared_fields`. One key supplied through two doors is adjudicated once: differing values are refused, equal ones collapse, and two names for the same target — `parent`/`plan`, `assign`/`assigned_user_id`, `role`/`agent_role_id` — are refused even when the values match. `pad_item.list` accepts `unparented: true` (mutually exclusive with `parent`) to select items with no parent or implements relationship, and is summary-shaped by default on both transports (`full: true` opts into complete content bodies): +**Tool catalog (v0.31)** — ten resource × action tools plus `pad_set_workspace` (eleven total), no flat verb explosion. Undeclared input keys are rejected with a structured error rather than silently dropped. `pad_item` create/update accept field values as a `fields` object (the same shape reads return) as an equivalent to the dedicated params / `field: ["key=value"]`, and its values keep their JSON types where the transport can carry them. Field values are typed against the collection schema server-side, so a declared number or json field is writable from the remote transport (which sends every value as a string). Keys the schema does not declare are stored and NAMED back in `warnings.undeclared_fields`. One key supplied through two doors is adjudicated once: differing values are refused, equal ones collapse, and two names for the same target — `parent`/`plan`, `assign`/`assigned_user_id`, `role`/`agent_role_id` — are refused even when the values match. `pad_item.list` accepts `unparented: true` (mutually exclusive with `parent`) to select items with no parent or implements relationship, and is summary-shaped by default on both transports (`full: true` opts into complete content bodies): | Tool | Actions | |---|---| @@ -423,7 +423,7 @@ initialize handshake under `capabilities.experimental.padCmdhelp` and `pad://_meta/version`): - `cmdhelp_version: "0.1"` — CLI help-tree contract (used at dispatch time) -- `tool_surface_version: "0.30"` — MCP tool catalog contract (v0.5 added `pad_library`; v0.6 `pad_item.backlinks`; v0.7 `pad_item` `export`/`import`; v0.8 `pad_workspace` `deleted`/`restore`; v0.9 made `pad_item.list` summary-shaped by default with a default+max result cap; v0.10 enforced the draft-playbook gate server-side on `pad_playbook.run` with an `allow_draft` escape hatch; v0.11 added the read-only `pad_attachment` tool (`list`/`show`); v0.12 added `pad_project.activity` (agent-accessible non-streaming activity feed); v0.13 added `pad_project` `ready`/`stale` (agent-oriented backlog + attention queries); v0.14 added `pad_item` `history` + optimistic concurrency (TASK-2022); v0.15 added the `pad_item.list` `unparented` parameter (TASK-2096); v0.16 made an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an agent can finally unassign an item (TASK-2571); v0.17 carried that to the LOCAL STDIO transport by teaching the CLI to lift those keys onto their columns instead of into the fields blob (BUG-2583); v0.18 added `clear_assigned_user` / `clear_agent_role` booleans — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` flags on `pad item update` (IDEA-2584); v0.19 added a `clear_parent` boolean — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` flag on `pad item update` (BUG-2078); v0.20 gave every tool an explicit annotation block derived from the catalog’s read-only knowledge — fully-read-only tools advertise `readOnlyHint: true` / `destructiveHint: false`, all-additive-write tools (`pad_workspace`, `pad_library`) drop `destructiveHint`, overwrite/delete-capable tools stay conservatively destructive, `openWorldHint: false` everywhere — replacing mcp-go’s defaults that marked every tool destructive (BUG-2302), and made `pad_item.list` summary-shaped on the remote HTTP transport too, with a declared `full` boolean as the opt-in for complete bodies on both transports (BUG-2305); v0.21 bounded `pad_item.history`, which was unbounded on every surface — `limit` now covers it (default 50, max 300, the NEWEST N; no `offset`, because reverse-patch storage makes only a newest-end window cheap), applied in the catalog action so it lands on both transports, and summary mode now asks the server to skip patch resolution rather than resolving bodies the dispatcher discards (BUG-2608); v0.22 stopped `pad_item.move` destroying an item’s system metadata — implementation notes, decision log, linked PR and convention data now survive a move, any field the destination schema has no home for is REPORTED in the move’s activity entry rather than vanishing, and a `field` setter naming one of those reserved keys is refused with `malformed_override` instead of writing it (BUG-2674); v0.23 closed the same door on the ordinary update — a `field` setter naming `implementation_notes`, `decision_log` or `convention` is now refused on every transport at once (`validation_error` on HTTP, surfaced to MCP clients as `validation_failed`); the one gate covers the CLI, remote MCP and stdio MCP at once because all three lower a `field` setter into the same `fields_patch`; `github_pr` is deliberately exempt ON UPDATE (move and copy still refuse it), since `pad github link` cannot run on remote MCP and refusing it would leave those agents with no door at all (that door is itself broken — BUG-2696); item CREATE stays open, deliberately, because its full-`fields` payload is shared with Pad’s own writers. v0.23 also added the retry-hostile `stored_state_unreadable` error code so an agent told its target item’s stored data is unreadable stops instead of retrying a permanent failure (BUG-2627 / BUG-2675); v0.24 made the `pad_item` `fields` object a real write form on create/update — reads return `fields` as a native object, and writing that shape back was a silent no-op (accepted, never mapped, dropped while the PATCH still bumped `updated_at`) — merging it into the same path as `field`/the dedicated params with conflicting duplicate keys refused, and made input validation strict across all catalog tools: undeclared top-level keys now fail with a structured error instead of being silently dropped (#1066); v0.25 made `pad_library.activate` resolve its DESTINATION collection from the target’s declared artifact kind (SPEC-5 collection traits) rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly instead of failing not-found with the collection sitting right there (BUG-2702); a lookup ERROR is now surfaced rather than silently falling back to the canonical slug, because falling back on an error means writing to a slug nothing was confirmed about (TASK-2657); v0.26 made `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant has `may_create_workspaces=false` — that checkbox previously gated only the post-creation auto-add, so a connection whose user declined it could still create workspaces — and on a connection with an explicit workspace allow-list, could not then see them (a wildcard `all_current_workspaces` connection could, which is why the consent mismatch rather than the invisibility is the defect); the same gate covers `POST /workspaces/import`, which mints a workspace through a second door. There is deliberately no escape-hatch parameter: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps` (IDEA-2756); v0.27 typed field values server-side so a declared number/json field is writable from the remote transport at all, carried the `fields` object with its JSON types intact, named undeclared keys back in `warnings.undeclared_fields` (accepted rather than refused — a census of 1012 items found 14 such keys across 168 live values, so refusing would have broken read-modify-write on items nobody had edited wrongly), and replaced the accreted per-site conflict guards with ONE check over a canonical view of every source; that check refuses several ambiguities v0.26 resolved silently, chiefly two names for one target in a single call (`parent`/`plan`, `assign`/`assigned_user_id`, `role`/`agent_role_id`), refused even when the values match because the names address one thing through incomparable vocabularies and the two doors resolved them differently (BUG-2850); v0.28 added two ADDITIVE `pad_item` actions — `remind`, which arms a one-shot reminder at an RFC3339 instant (`remind_at`), and `ack-reminder`, which acknowledges a fired one by id (`reminder_id`); a bare `YYYY-MM-DD` is refused rather than read as midnight, since a date names a 24-hour span and picking an hour inside it would be the server choosing a time nobody did (IDEA-2641); v0.29 made a `relation` field value have to NAME A LIVE ITEM in the collection that field declares — `internal/items` only ever checked the SHAPE of a relation ("must be a string"), because deciding whether a string names an item is a database question and that package is DB-free, so any string at all was accepted and stored and no client could render it honestly; every write door now refuses a value that names nothing, names an item in the WRONG collection, sits in a field whose schema declares no target collection, or is a SLUG (a deliberate divergence from `ResolveItem`: a slug is neither an ID nor stable, and free text like "red" resolving to whatever is slugged `red` today is exactly the corruption this closes). A CARRIED value — one already on the item, asserted by nobody — is never refused, because refusing would make every legacy item un-updatable, un-movable and un-copyable: within a workspace it resolves and survives, across a workspace boundary it is dropped without a lookup and reported in `warnings.dropped_fields`, so `pad_item.action=copy` now names a drop where v0.28 silently landed a dangling reference (PLAN-2857 / TASK-2878); ; v0.30 made one `--field key=value` entry mean ONE thing at every door — six sites parsed it independently (`item create`, `item list`, `item update`, `item move`, `item copy`, and the remote door’s own ingest) in four spellings, so `field:[" effort=l"]` stored an undeclared field literally named " effort" through the CLI and wrote `effort` through /mcp, the same call storing two different keys depending only on the transport. All six now share one parse, with two deliberately asymmetric rules: a padded KEY is REFUSED everywhere rather than trimmed anywhere (trimming silently retargets the write to a field the caller did not type), and a VALUE is carried VERBATIM everywhere (trimming reinterprets a caller’s bytes, and on a text field the padding is content) — a padded value against a typed field is refused one layer down by validation, naming the field, which is the same answer at both doors since v0.27 types declared fields server-side. Both doors refuse something they used to accept, and they were accepting it differently — /mcp trimmed the padded key and wrote the declared field, the CLI stored a ghost field beside it; what is /mcp-only is the value half, which it used to trim and type and now passes through to the same validation the CLI has always applied. A caller writing canonical entries sees no difference at either door (BUG-2870); see `internal/mcp/version.go` for the full changelog) +- `tool_surface_version: "0.31"` — MCP tool catalog contract (v0.5 added `pad_library`; v0.6 `pad_item.backlinks`; v0.7 `pad_item` `export`/`import`; v0.8 `pad_workspace` `deleted`/`restore`; v0.9 made `pad_item.list` summary-shaped by default with a default+max result cap; v0.10 enforced the draft-playbook gate server-side on `pad_playbook.run` with an `allow_draft` escape hatch; v0.11 added the read-only `pad_attachment` tool (`list`/`show`); v0.12 added `pad_project.activity` (agent-accessible non-streaming activity feed); v0.13 added `pad_project` `ready`/`stale` (agent-oriented backlog + attention queries); v0.14 added `pad_item` `history` + optimistic concurrency (TASK-2022); v0.15 added the `pad_item.list` `unparented` parameter (TASK-2096); v0.16 made an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an agent can finally unassign an item (TASK-2571); v0.17 carried that to the LOCAL STDIO transport by teaching the CLI to lift those keys onto their columns instead of into the fields blob (BUG-2583); v0.18 added `clear_assigned_user` / `clear_agent_role` booleans — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` flags on `pad item update` (IDEA-2584); v0.19 added a `clear_parent` boolean — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` flag on `pad item update` (BUG-2078); v0.20 gave every tool an explicit annotation block derived from the catalog’s read-only knowledge — fully-read-only tools advertise `readOnlyHint: true` / `destructiveHint: false`, all-additive-write tools (`pad_workspace`, `pad_library`) drop `destructiveHint`, overwrite/delete-capable tools stay conservatively destructive, `openWorldHint: false` everywhere — replacing mcp-go’s defaults that marked every tool destructive (BUG-2302), and made `pad_item.list` summary-shaped on the remote HTTP transport too, with a declared `full` boolean as the opt-in for complete bodies on both transports (BUG-2305); v0.21 bounded `pad_item.history`, which was unbounded on every surface — `limit` now covers it (default 50, max 300, the NEWEST N; no `offset`, because reverse-patch storage makes only a newest-end window cheap), applied in the catalog action so it lands on both transports, and summary mode now asks the server to skip patch resolution rather than resolving bodies the dispatcher discards (BUG-2608); v0.22 stopped `pad_item.move` destroying an item’s system metadata — implementation notes, decision log, linked PR and convention data now survive a move, any field the destination schema has no home for is REPORTED in the move’s activity entry rather than vanishing, and a `field` setter naming one of those reserved keys is refused with `malformed_override` instead of writing it (BUG-2674); v0.23 closed the same door on the ordinary update — a `field` setter naming `implementation_notes`, `decision_log` or `convention` is now refused on every transport at once (`validation_error` on HTTP, surfaced to MCP clients as `validation_failed`); the one gate covers the CLI, remote MCP and stdio MCP at once because all three lower a `field` setter into the same `fields_patch`; `github_pr` is deliberately exempt ON UPDATE (move and copy still refuse it), since `pad github link` cannot run on remote MCP and refusing it would leave those agents with no door at all (that door is itself broken — BUG-2696); item CREATE stays open, deliberately, because its full-`fields` payload is shared with Pad’s own writers. v0.23 also added the retry-hostile `stored_state_unreadable` error code so an agent told its target item’s stored data is unreadable stops instead of retrying a permanent failure (BUG-2627 / BUG-2675); v0.24 made the `pad_item` `fields` object a real write form on create/update — reads return `fields` as a native object, and writing that shape back was a silent no-op (accepted, never mapped, dropped while the PATCH still bumped `updated_at`) — merging it into the same path as `field`/the dedicated params with conflicting duplicate keys refused, and made input validation strict across all catalog tools: undeclared top-level keys now fail with a structured error instead of being silently dropped (#1066); v0.25 made `pad_library.activate` resolve its DESTINATION collection from the target’s declared artifact kind (SPEC-5 collection traits) rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly instead of failing not-found with the collection sitting right there (BUG-2702); a lookup ERROR is now surfaced rather than silently falling back to the canonical slug, because falling back on an error means writing to a slug nothing was confirmed about (TASK-2657); v0.26 made `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant has `may_create_workspaces=false` — that checkbox previously gated only the post-creation auto-add, so a connection whose user declined it could still create workspaces — and on a connection with an explicit workspace allow-list, could not then see them (a wildcard `all_current_workspaces` connection could, which is why the consent mismatch rather than the invisibility is the defect); the same gate covers `POST /workspaces/import`, which mints a workspace through a second door. There is deliberately no escape-hatch parameter: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps` (IDEA-2756); v0.27 typed field values server-side so a declared number/json field is writable from the remote transport at all, carried the `fields` object with its JSON types intact, named undeclared keys back in `warnings.undeclared_fields` (accepted rather than refused — a census of 1012 items found 14 such keys across 168 live values, so refusing would have broken read-modify-write on items nobody had edited wrongly), and replaced the accreted per-site conflict guards with ONE check over a canonical view of every source; that check refuses several ambiguities v0.26 resolved silently, chiefly two names for one target in a single call (`parent`/`plan`, `assign`/`assigned_user_id`, `role`/`agent_role_id`), refused even when the values match because the names address one thing through incomparable vocabularies and the two doors resolved them differently (BUG-2850); v0.28 added two ADDITIVE `pad_item` actions — `remind`, which arms a one-shot reminder at an RFC3339 instant (`remind_at`), and `ack-reminder`, which acknowledges a fired one by id (`reminder_id`); a bare `YYYY-MM-DD` is refused rather than read as midnight, since a date names a 24-hour span and picking an hour inside it would be the server choosing a time nobody did (IDEA-2641); v0.29 made a `relation` field value have to NAME A LIVE ITEM in the collection that field declares — `internal/items` only ever checked the SHAPE of a relation ("must be a string"), because deciding whether a string names an item is a database question and that package is DB-free, so any string at all was accepted and stored and no client could render it honestly; every write door now refuses a value that names nothing, names an item in the WRONG collection, sits in a field whose schema declares no target collection, or is a SLUG (a deliberate divergence from `ResolveItem`: a slug is neither an ID nor stable, and free text like "red" resolving to whatever is slugged `red` today is exactly the corruption this closes). A CARRIED value — one already on the item, asserted by nobody — is never refused, because refusing would make every legacy item un-updatable, un-movable and un-copyable: within a workspace it resolves and survives, across a workspace boundary it is dropped without a lookup and reported in `warnings.dropped_fields`, so `pad_item.action=copy` now names a drop where v0.28 silently landed a dangling reference (PLAN-2857 / TASK-2878); v0.30 made one `--field key=value` entry mean ONE thing at every door — six sites parsed it independently (`item create`, `item list`, `item update`, `item move`, `item copy`, and the remote door’s own ingest) in four spellings, so `field:[" effort=l"]` stored an undeclared field literally named " effort" through the CLI and wrote `effort` through /mcp, the same call storing two different keys depending only on the transport. All six now share one parse, with two deliberately asymmetric rules: a padded KEY is REFUSED everywhere rather than trimmed anywhere (trimming silently retargets the write to a field the caller did not type), and a VALUE is carried VERBATIM everywhere (trimming reinterprets a caller’s bytes, and on a text field the padding is content) — a padded value against a typed field is refused one layer down by validation, naming the field, which is the same answer at both doors since v0.27 types declared fields server-side. Both doors refuse something they used to accept, and they were accepting it differently — /mcp trimmed the padded key and wrote the declared field, the CLI stored a ghost field beside it; what is /mcp-only is the value half, which it used to trim and type and now passes through to the same validation the CLI has always applied. A caller writing canonical entries sees no difference at either door (BUG-2870); v0.31 let a `relation` field value be an EXACT TITLE, scoped to the collection that field declares, alongside the UUID and ref v0.29 accepted — a THIRD accepted spelling, so nothing a v0.30 caller sends stops working — added a `relation_targets` member to reads hydrating each stored id to `{id, ref, title}` so a client can render a relation without a request per value, and made the `fields` DSL take the target collection slug as a relation's third part. The bump is owed by that last part: `fields="owner:relation"` used to be ACCEPTED and is now REFUSED, because the old parser put the third part into Options for every type and so built a relation with no target — a field every subsequent write then refused with `target_missing`, which is to say a field that could never accept a value. Title resolution is COLLECTION-SCOPED on purpose: a title unique only workspace-wide refuses and names the collection searched, and two matches inside the declared collection get their own `ambiguous` reason rather than `not_found`, which would state the opposite of what happened. The ladder is UUID, then ref, then title, so an item literally TITLED "COLO-3" is unreachable by title while the ref resolves. An id-only `relation_targets` entry means the target is gone OR the caller may not see it, and consumers must not render it as either — the two are made indistinguishable deliberately (PLAN-2857 / TASK-2996); see `internal/mcp/version.go` for the full changelog) External agents pin against these so a future rename doesn't break them silently. Errors come back as structured envelopes (`{error: {code, diff --git a/cmd/pad/cmd_collection.go b/cmd/pad/cmd_collection.go index e34839b4d..e27375d94 100644 --- a/cmd/pad/cmd_collection.go +++ b/cmd/pad/cmd_collection.go @@ -168,8 +168,13 @@ Two ways to define the schema: --fields Compact DSL for the simple case: key:type[:option1,option2,...] Separate multiple fields with semicolons. Does not support - terminal_options, custom defaults, computed fields, suffixes, - or relation collections. + terminal_options, custom defaults, computed fields, or suffixes. + + For a relation the third part is the TARGET COLLECTION slug + rather than an options list, and it is required: + owner:relation:people + A relation with no target is refused, because such a field + cannot accept any value. --schema Full CollectionSchema JSON for everything else. Accepts: inline JSON: --schema '{"fields":[...]}' @@ -181,6 +186,7 @@ Two ways to define the schema: Examples: pad collection create "Bugs" --fields "status:select:new,triaged,fixing,resolved;severity:select:low,medium,high,critical;component:text" pad collection create "Decisions" --icon "⚖️" --fields "status:select:proposed,accepted,rejected;impact:select:low,medium,high" + pad collection create "Reviews" --fields "status:select:open,done;reviewer:relation:people" pad collection create "Marketing" --schema '{"fields":[{"key":"status","label":"Status","type":"select","options":["idea","drafting","review","published","archived"],"terminal_options":["published","archived"],"default":"idea","required":true}]}' pad collection create "Marketing" --schema @./marketing-schema.json cat schema.json | pad collection create "Marketing" --schema - diff --git a/cmd/pad/cmd_item.go b/cmd/pad/cmd_item.go index 2efb0873f..9bcc7c710 100644 --- a/cmd/pad/cmd_item.go +++ b/cmd/pad/cmd_item.go @@ -776,6 +776,15 @@ func showCmd() *cobra.Command { if item.Convention != nil && (k == "category" || k == "trigger" || k == "scope" || k == "priority" || k == "enforcement" || k == "surfaces" || k == "commands") { continue // shown in dedicated section below } + // U6: a relation's stored value is an item id, which + // tells a reader nothing. The response already carries + // the hydrated target, so this is a render, not a + // lookup — and it falls back to the raw value when the + // field is not a relation or was not hydrated. + if target, ok := item.RelationTargets[k]; ok { + fmt.Printf("%-12s %s\n", k+":", cli.RenderRelationValue(target)) + continue + } fmt.Printf("%-12s %v\n", k+":", v) } fmt.Println("---") diff --git a/internal/cli/item_summary.go b/internal/cli/item_summary.go index ff53e593e..a346b0248 100644 --- a/internal/cli/item_summary.go +++ b/internal/cli/item_summary.go @@ -20,6 +20,10 @@ import ( // `tags` are emitted as nested JSON (not escaped strings) so agents can read // them without a second parse. // +// The one id-bearing member it KEEPS is `relation_targets` (PLAN-2857 U6) — +// see the field's own comment for why a relation is the exception to the +// dropped-UUID rule. +// // The full shape (raw models.Item) is still available via `item list --full`. type ItemSummary struct { Ref string `json:"ref,omitempty"` @@ -35,8 +39,23 @@ type ItemSummary struct { AssignedUser string `json:"assigned_user,omitempty"` AgentRole string `json:"agent_role,omitempty"` HasChildren bool `json:"has_children,omitempty"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` + + // RelationTargets carries the hydrated `relation` values (PLAN-2857 U6). + // + // It survives the summary projection while the other UUID plumbing does + // not, because it is the ONE case where the id in `fields` is useless on + // its own: a relation stores an item id, and without this member a client + // reading a list has a UUID and no way to render it short of a request per + // value. That is the defect U6 exists to close, and dropping it here would + // leave it open on the agent doors — `pad item list --format json` and + // `pad_item.list` on both transports — which are the doors that matter + // most. + // + // Small by construction: one entry per relation field the item actually + // carries, and absent entirely on the items that carry none. + RelationTargets map[string]models.RelationTarget `json:"relation_targets,omitempty"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` } // contentPreviewLimit caps the content_preview at a small, agent-friendly @@ -97,21 +116,22 @@ func rawJSONOrNil(s string) json.RawMessage { // ToItemSummary projects a single models.Item into the summary shape. func ToItemSummary(item models.Item) ItemSummary { return ItemSummary{ - Ref: item.Ref, - Title: item.Title, - Slug: item.Slug, - CollectionSlug: item.CollectionSlug, - ItemNumber: item.ItemNumber, - Fields: rawJSONOrNil(item.Fields), - Tags: rawJSONOrNil(item.Tags), - Pinned: item.Pinned, - ContentPreview: contentPreview(item.Content), - ParentRef: item.ParentRef, - AssignedUser: item.AssignedUserName, - AgentRole: item.AgentRoleSlug, - HasChildren: item.HasChildren, - CreatedAt: item.CreatedAt, - UpdatedAt: item.UpdatedAt, + Ref: item.Ref, + Title: item.Title, + Slug: item.Slug, + CollectionSlug: item.CollectionSlug, + ItemNumber: item.ItemNumber, + Fields: rawJSONOrNil(item.Fields), + Tags: rawJSONOrNil(item.Tags), + Pinned: item.Pinned, + ContentPreview: contentPreview(item.Content), + ParentRef: item.ParentRef, + AssignedUser: item.AssignedUserName, + AgentRole: item.AgentRoleSlug, + HasChildren: item.HasChildren, + RelationTargets: item.RelationTargets, + CreatedAt: item.CreatedAt, + UpdatedAt: item.UpdatedAt, } } diff --git a/internal/cli/relation_render.go b/internal/cli/relation_render.go new file mode 100644 index 000000000..7216d31e5 --- /dev/null +++ b/internal/cli/relation_render.go @@ -0,0 +1,39 @@ +package cli + +import ( + "fmt" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// RenderRelationValue is the ONE place a hydrated `relation` value becomes text +// for a human (PLAN-2857 U6). +// +// A relation stores an item id, so the raw stored value is a UUID and reading +// it tells a person nothing. `relation_targets` already carries what is needed +// to render it, so this is a formatter over data the response holds — no +// lookup, and nothing here can fail. +// +// resolved -> "COLO-3 · Red" +// id-only -> "6f2c… (unavailable)" +// +// WHY "unavailable" AND NOT "deleted". An id-only entry has TWO causes that +// this layer cannot tell apart: the target is gone, or the requester may not +// see it. "(deleted)" would assert non-existence for both, which is false for +// half of them and is an existence claim the response does not support — the +// same defect codex round 12 found in the copy dialog, where the UI read +// `not_found` as "does not exist" when the server also emits it for a target +// the caller merely cannot see. The server works to make those two +// indistinguishable; the renderer must not undo that in the last inch. +func RenderRelationValue(target models.RelationTarget) string { + if target.Ref == "" { + return fmt.Sprintf("%s (unavailable)", target.ID) + } + if target.Title == "" { + // Ref without a title should not happen — they come from the same row — + // but rendering "COLO-3 · " would look like a bug in the data rather + // than in this function. + return target.Ref + } + return fmt.Sprintf("%s · %s", target.Ref, target.Title) +} diff --git a/internal/cli/relation_render_test.go b/internal/cli/relation_render_test.go new file mode 100644 index 000000000..7b47e643a --- /dev/null +++ b/internal/cli/relation_render_test.go @@ -0,0 +1,52 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// TestRenderRelationValue covers the three shapes a hydrated relation can +// arrive in, and one thing the renderer must NOT say. +func TestRenderRelationValue(t *testing.T) { + t.Parallel() + + t.Run("resolved renders ref and title", func(t *testing.T) { + t.Parallel() + got := RenderRelationValue(models.RelationTarget{ID: "abc", Ref: "COLO-3", Title: "Red"}) + if got != "COLO-3 · Red" { + t.Errorf("got %q, want %q", got, "COLO-3 · Red") + } + if strings.Contains(got, "abc") { + t.Error("the raw id leaked into a fully-resolved render, which is the noise U6 exists to remove") + } + }) + + t.Run("id-only keeps the id and claims nothing about it", func(t *testing.T) { + t.Parallel() + got := RenderRelationValue(models.RelationTarget{ID: "6f2c-dead"}) + if !strings.Contains(got, "6f2c-dead") { + t.Errorf("got %q, want the stored id retained — it is all the caller has", got) + } + // THE POINT OF THIS LEG. An id-only entry means either the target is + // gone or the requester may not see it, and this layer cannot tell + // which. Saying "deleted" asserts non-existence for a target that may + // be alive and merely hidden — the existence oracle the server's + // collapse exists to prevent, and the exact defect codex round 12 + // found in the copy dialog's not_found wording. + for _, forbidden := range []string{"deleted", "removed", "does not exist", "missing"} { + if strings.Contains(strings.ToLower(got), forbidden) { + t.Errorf("render %q claims %q; an id-only entry covers BOTH a gone target and a hidden one, so a non-existence claim is false for half the cases", got, forbidden) + } + } + }) + + t.Run("ref without a title does not render a dangling separator", func(t *testing.T) { + t.Parallel() + got := RenderRelationValue(models.RelationTarget{ID: "abc", Ref: "COLO-3"}) + if got != "COLO-3" { + t.Errorf("got %q, want %q", got, "COLO-3") + } + }) +} diff --git a/internal/collections/dsl.go b/internal/collections/dsl.go index 605162d62..ca8095a81 100644 --- a/internal/collections/dsl.go +++ b/internal/collections/dsl.go @@ -47,7 +47,31 @@ func ParseFieldsDSL(fieldsDSL string) (models.CollectionSchema, error) { Label: cases.Title(language.English).String(strings.ReplaceAll(parts[0], "_", " ")), Type: parts[1], } - if len(parts) == 3 && parts[2] != "" { + if fd.Type == "relation" { + // PLAN-2857 U6. For a relation the third part is the TARGET + // COLLECTION slug, not an options list. + // + // It used to fall into Options like every other type, which did not + // merely fail to declare the target: it produced a relation field + // with Options=["colors"] and NO target, and every write to that + // field was then refused with `target_missing`. A DSL that builds a + // field nothing can ever write to is worse than one that refuses, + // so a bare `owner:relation` is now an error at parse time. + target := "" + if len(parts) == 3 { + target = strings.TrimSpace(parts[2]) + } + if target == "" { + return schema, fmt.Errorf("invalid field definition: %q — a relation needs its target collection (expected %s:relation:)", f, fd.Key) + } + if strings.Contains(target, ",") { + // A comma here means the caller wrote an options list, which is + // the pre-U6 shape. Refusing names the mistake; accepting the + // first item would silently pick a target they did not choose. + return schema, fmt.Errorf("invalid field definition: %q — a relation takes ONE target collection, not a list", f) + } + fd.Collection = target + } else if len(parts) == 3 && parts[2] != "" { fd.Options = strings.Split(parts[2], ",") } if fd.Type == "select" && fd.Key == "status" { diff --git a/internal/collections/dsl_relation_test.go b/internal/collections/dsl_relation_test.go new file mode 100644 index 000000000..4ec4a01c4 --- /dev/null +++ b/internal/collections/dsl_relation_test.go @@ -0,0 +1,81 @@ +package collections + +import ( + "strings" + "testing" +) + +// The `--fields` DSL and relation targets (PLAN-2857 U6). +// +// The gap this closes was not "the DSL cannot declare a target". It was worse: +// `owner:relation:colors` put "colors" into Options like every other type, so +// it built a relation field with NO target collection — and every write to that +// field was then refused with `target_missing`. The DSL silently produced a +// field nothing could ever write to. + +func TestParseFieldsDSL_RelationTakesTheTargetCollection(t *testing.T) { + t.Parallel() + schema, err := ParseFieldsDSL("owner:relation:people") + if err != nil { + t.Fatalf("ParseFieldsDSL: %v", err) + } + if len(schema.Fields) != 1 { + t.Fatalf("want 1 field, got %d", len(schema.Fields)) + } + fd := schema.Fields[0] + if fd.Collection != "people" { + t.Errorf("Collection = %q, want %q", fd.Collection, "people") + } + if len(fd.Options) != 0 { + t.Errorf("Options = %v, want empty — putting the target in Options is the defect this closes, and it leaves the field unwritable", fd.Options) + } +} + +// A bare `owner:relation` is REFUSED rather than producing the unusable field +// the old parser built. The error names the expected shape, because a caller +// who typed the old form needs to know what to type instead. +func TestParseFieldsDSL_BareRelationIsRefused(t *testing.T) { + t.Parallel() + for _, in := range []string{"owner:relation", "owner:relation:", "owner:relation: "} { + schema, err := ParseFieldsDSL(in) + if err == nil { + t.Errorf("%q: expected an error, got schema %+v — a relation with no target is a field no write can ever satisfy", in, schema) + continue + } + if !strings.Contains(err.Error(), "relation") || !strings.Contains(err.Error(), "collection") { + t.Errorf("%q: error %q should name what is missing and the expected shape", in, err.Error()) + } + } +} + +// A comma means the caller wrote an OPTIONS list — the pre-U6 shape. Refusing +// names the mistake; taking the first entry would silently pick a target they +// did not choose. +func TestParseFieldsDSL_RelationRefusesAnOptionsList(t *testing.T) { + t.Parallel() + _, err := ParseFieldsDSL("owner:relation:people,teams") + if err == nil { + t.Fatal("expected an error for a comma-separated relation target") + } + if !strings.Contains(err.Error(), "ONE target collection") { + t.Errorf("error %q should say a relation takes one target", err.Error()) + } +} + +// CONTROL: the change must not have altered how every OTHER type reads its +// third part. Without this, moving the options branch could break select and +// nothing in this file would notice. +func TestParseFieldsDSL_NonRelationStillTakesOptions(t *testing.T) { + t.Parallel() + schema, err := ParseFieldsDSL("size:select:small,large") + if err != nil { + t.Fatalf("ParseFieldsDSL: %v", err) + } + fd := schema.Fields[0] + if len(fd.Options) != 2 || fd.Options[0] != "small" || fd.Options[1] != "large" { + t.Errorf("Options = %v, want [small large]", fd.Options) + } + if fd.Collection != "" { + t.Errorf("Collection = %q, want empty on a non-relation", fd.Collection) + } +} diff --git a/internal/mcp/catalog_collection.go b/internal/mcp/catalog_collection.go index 53827e0e3..5e462cefc 100644 --- a/internal/mcp/catalog_collection.go +++ b/internal/mcp/catalog_collection.go @@ -40,7 +40,7 @@ var padCollectionTool = ToolDef{ { Name: "fields", Type: "string", - Description: "Compact field DSL: \"key:type[:options]; ...\". Optional for action=create. Example: \"status:select:open,done; priority:select:high,medium,low\". Use `schema` instead when you need terminal_options, custom defaults, computed fields, suffixes, or relation collections — the DSL cannot express those. Mutually exclusive with `schema`.", + Description: "Compact field DSL: \"key:type[:options]; ...\". Optional for action=create. Example: \"status:select:open,done; priority:select:high,medium,low\". For a relation the third part is the TARGET COLLECTION slug rather than an options list, and it is required: \"reviewer:relation:people\" — a relation with no target is refused, since such a field cannot accept any value. Use `schema` instead when you need terminal_options, custom defaults, computed fields, or suffixes — the DSL cannot express those. Mutually exclusive with `schema`.", }, { Name: "schema", @@ -104,11 +104,15 @@ Actions: Optional: fields OR schema (mutually exclusive), icon, description, layout, default_view, board_group_by. DSL example: fields="status:select:open,done; priority:select:high,medium,low" + Relation in the DSL: the third part is the TARGET COLLECTION slug, + not an options list, and it is required — + fields="reviewer:relation:people". A relation with no target is + refused rather than built, because such a field cannot accept any + value. Schema example: schema={"fields":[{"key":"status","type":"select","options":["new","done"],"terminal_options":["done"]}]} Prefer schema when terminal_options matters (driving dashboard active/complete counts) or when fields need defaults, computed - flags, suffixes, or relation collections — the DSL cannot express - those. + flags, or suffixes — the DSL cannot express those. update — Update an existing collection's name, icon, description, prefix, schema, or sort order. Workspace owner only. Required: workspace, slug. diff --git a/internal/mcp/dispatch_http_relation_targets_test.go b/internal/mcp/dispatch_http_relation_targets_test.go new file mode 100644 index 000000000..2c404192a --- /dev/null +++ b/internal/mcp/dispatch_http_relation_targets_test.go @@ -0,0 +1,157 @@ +package mcp + +// PLAN-2857 U6 — a relation value survives the REMOTE transport in both +// directions: an exact TITLE written through it resolves to the canonical id, +// and the hydrated `relation_targets` reaches the agent through the summary +// projection. +// +// WHY THIS IS NOT CEREMONIAL, given the server doors are already tested. The +// remote list path does not forward the handler's response — it re-projects it +// through cli.ToItemSummaries, and that projection drops UUID plumbing on +// purpose. relation_targets was dropped by it during this very unit, which is +// exactly the failure a handler-level test cannot see: the server was correct +// and the agent still received a bare id with nothing to render it. Same +// rationale as the summary-shape tests above, and the same fixture style — +// real server, real store, because a stubbed handler can encode a response the +// endpoint never produces. + +import ( + "context" + "encoding/json" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/server" + "github.com/PerpetualSoftware/pad/internal/store" + "github.com/PerpetualSoftware/pad/internal/store/storetest" +) + +type relationTargetsFixture struct { + d *HTTPHandlerDispatcher + store *store.Store + ws *models.Workspace + cars *models.Collection + target *models.Item +} + +func newRelationTargetsFixture(t *testing.T) *relationTargetsFixture { + t.Helper() + s := storetest.NewSQLite(t) + srv := server.New(s) + t.Cleanup(srv.Stop) + + owner, err := s.CreateUser(models.UserCreate{ + Email: "rel-owner@example.com", Name: "Owner", Password: "correct-horse-battery-staple", + }) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Rel WS", Slug: "rel-ws", OwnerID: owner.ID}) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + if err := s.AddWorkspaceMember(ws.ID, owner.ID, "owner"); err != nil { + t.Fatalf("AddWorkspaceMember: %v", err) + } + colors, err := s.CreateCollection(ws.ID, models.CollectionCreate{ + Name: "Colors", Slug: "colors", Prefix: "COLO", + Schema: `{"fields":[{"key":"status","type":"select","options":["open","done"],"default":"open"}]}`, + }) + if err != nil { + t.Fatalf("CreateCollection(colors): %v", err) + } + cars, err := s.CreateCollection(ws.ID, models.CollectionCreate{ + Name: "Cars", Slug: "cars", Prefix: "CAR", + Schema: `{"fields":[{"key":"status","type":"select","options":["open","done"],"default":"open"},{"key":"color","type":"relation","collection":"colors"}]}`, + }) + if err != nil { + t.Fatalf("CreateCollection(cars): %v", err) + } + red, err := s.CreateItem(ws.ID, colors.ID, models.ItemCreate{Title: "Red", CreatedBy: owner.ID}) + if err != nil { + t.Fatalf("CreateItem(red): %v", err) + } + + return &relationTargetsFixture{ + d: &HTTPHandlerDispatcher{ + Handler: srv, + UserResolver: func(context.Context) *models.User { return owner }, + }, + store: s, ws: ws, cars: cars, target: red, + } +} + +// TestHTTPRelation_TitleWrittenOverTheRemoteDoorStoresTheCanonicalID: the write +// half. The title is resolved SERVER-side, so the remote door must carry the +// caller's string through untouched and store what the server resolved. +func TestHTTPRelation_TitleWrittenOverTheRemoteDoorStoresTheCanonicalID(t *testing.T) { + f := newRelationTargetsFixture(t) + + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "rel-ws", + "collection": "cars", + "title": "By title", + "field": []any{"color=Red"}, + }) + res, err := f.d.Dispatch(ctx, []string{"item", "create"}, nil) + if err != nil { + t.Fatalf("Dispatch(item create): %v", err) + } + if res.IsError { + t.Fatalf("create refused an exact title: %s", textOf(res)) + } + + // Read the STORED value back, not the response: a door that echoed the + // right thing while writing the wrong thing would pass a response check. + items, err := f.store.ListItems(f.ws.ID, models.ItemListParams{CollectionSlug: "cars"}) + if err != nil { + t.Fatalf("ListItems: %v", err) + } + if len(items) != 1 { + t.Fatalf("expected 1 car, got %d", len(items)) + } + var fields map[string]any + if err := json.Unmarshal([]byte(items[0].Fields), &fields); err != nil { + t.Fatalf("decode fields: %v", err) + } + if fields["color"] != f.target.ID { + t.Errorf("stored color = %v, want the canonical id %s — the title did not resolve through this transport", fields["color"], f.target.ID) + } +} + +// TestHTTPRelation_ListCarriesHydratedTargets: the read half, and the one that +// caught a real strip. The summary projection must keep relation_targets. +func TestHTTPRelation_ListCarriesHydratedTargets(t *testing.T) { + f := newRelationTargetsFixture(t) + + if _, err := f.store.CreateItem(f.ws.ID, f.cars.ID, models.ItemCreate{ + Title: "Ruby car", + Fields: `{"status":"open","color":"` + f.target.ID + `"}`, + }); err != nil { + t.Fatalf("CreateItem(car): %v", err) + } + + text := dispatchList(t, f.d, map[string]any{"workspace": "rel-ws", "collection": "cars"}) + + var items []struct { + Ref string `json:"ref"` + RelationTargets map[string]struct { + ID string `json:"id"` + Ref string `json:"ref"` + Title string `json:"title"` + } `json:"relation_targets"` + } + if err := json.Unmarshal([]byte(text), &items); err != nil { + t.Fatalf("decode summaries: %v\n%s", err, text) + } + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d\n%s", len(items), text) + } + got, ok := items[0].RelationTargets["color"] + if !ok { + t.Fatalf("the summary projection dropped relation_targets, so an agent gets a bare id with nothing to render it — the exact defect U6 exists to close:\n%s", text) + } + if got.ID != f.target.ID || got.Ref != f.target.Ref || got.Title != "Red" { + t.Errorf("hydrated as %+v, want {id:%s ref:%s title:Red}", got, f.target.ID, f.target.Ref) + } +} diff --git a/internal/mcp/instructions.md b/internal/mcp/instructions.md index f69a85612..5bd67ef05 100644 --- a/internal/mcp/instructions.md +++ b/internal/mcp/instructions.md @@ -6,13 +6,18 @@ 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.30) +## Tool surface (v0.31) Ten resource × action tools, plus `pad_set_workspace` (which takes a `workspace` slug only — no action enum). Eleven tools total. Inputs are validated strictly: an undeclared top-level key is rejected with a structured `validation_failed` naming it, never accepted and silently dropped. - `pad_item` — Items: create / update / delete / get / list / move / restore / link / unlink / deps / star / unstar / starred / comment / list-comments / backlinks / bulk-update / note / decide / export / import / history / remind / ack-reminder. 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_updated_at` for optimistic concurrency (a stale value fails with a structured 409 `update_conflict`). 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. 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. 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 the exception on UPDATE only — a move or copy still refuses it — because `pad github link` needs a local git checkout you don't have and an update would be your only way in. Be aware that it does not currently work either: a `field` value arrives as a string, so the PR data is stored double-encoded and no link appears (BUG-2696). Treat linking a PR as something to hand to a human for now, rather than a call to retry. On `create` none of them are blocked, since that door is shared with Pad's own writers — but don't hand-write `implementation_notes` / `decision_log` there either. Doing so does not merely fail to help: it stores something Pad cannot read back, which hides the existing entries on every surface and makes `note` / `decide` refuse on that item until it is repaired. `history` returns read-only item version metadata (newest-first), bounded to the NEWEST 50 versions by default (max 300 — pass `limit` to change the window); pass `full: true` to include each version's resolved content body (token-expensive). There is no `offset`: versions are stored as reverse patches, so only a newest-end window is cheap to reconstruct. To UNASSIGN an item, pass `clear_assigned_user: true` (or `clear_agent_role: true`) — the canonical form, works on both transports. To DETACH an item from its parent, pass `clear_parent: true` — same canonical shape, works on both transports. Setting and clearing the same field in one call is refused, not silently resolved, so don't pair `clear_assigned_user`/`clear_agent_role`/`clear_parent` with `assign`/`role`/`parent` respectively. An empty `assign` / `role` / `parent` does NOT clear: those name a person, a slug, or a ref, so an empty value reads as "not provided", exactly like every other optional string here. `remind` arms a one-shot reminder on an item: pass `remind_at` as an RFC3339 INSTANT (`2026-08-01T09:00:00Z`; an offset is stored as the same moment in UTC). A bare date is REFUSED rather than read as midnight, because it names a 24-hour span and picking an hour inside it would fire at a time nobody chose. When it fires the item appears in `pad_project.action: next` / `ready` until you acknowledge it with `ack-reminder` (which takes a `reminder_id` — carried on the fired suggestion itself as `reminder_id`, so a poller that never armed it can still retire it, and also returned when you arm one) — that poll surface is the delivery path on any instance without a webhook configured, so it is where you will actually see it. Nothing else acknowledges a reminder: completing the item does NOT, because a reminder may have been armed precisely to fire after the work was done. A reminder on a completed item is hidden from next/ready and left untouched in place. (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.) + +**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. + +Reads 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. + - `pad_workspace` — Workspaces: list / members / invite / storage / audit-log / create / claim / deleted / restore. - `pad_collection` — Collections: list / create / update / delete. - `pad_project` — Project intelligence: dashboard / next / ready / stale / standup / changelog / report / activity. Use `ready` for the actionable backlog and `stale` for items needing attention; `activity` to catch up on what other agents/users changed since you last worked (non-streaming feed with item refs + change details). diff --git a/internal/mcp/version.go b/internal/mcp/version.go index de05e0203..69db61ec7 100644 --- a/internal/mcp/version.go +++ b/internal/mcp/version.go @@ -937,6 +937,47 @@ const CmdhelpVersion = "0.1" // case, which is the one with a real claim to leniency, is // already exempt by provenance rather than by a flag. // +// 0.31 — PLAN-2857 U6 / TASK-2996. A `relation` field value may now +// be an EXACT TITLE, scoped to the collection that field declares, +// alongside the UUID and ref U1 accepted; reads carry a new +// `relation_targets` member hydrating each stored id to +// {id, ref, title}; and `pad_collection.create`'s `fields` DSL takes +// the target collection slug as a relation's third part. +// +// A BEHAVIOR bump on the v0.30/v0.29 grounds — no tool name, action +// enum or param shape changed. Two of the three parts are purely +// additive: a title is a THIRD accepted spelling, so nothing a 0.30 +// consumer sends stops working, and `relation_targets` is additive +// and omitempty. The bump is owed by the DSL: `fields="owner:relation"` +// used to be ACCEPTED and is now REFUSED. +// +// That refusal is the fix, not a regression. The old parser put the +// third part into Options for every type, so a relation declared +// through the DSL got Options=["colors"] and NO target collection — +// and every subsequent write to that field was refused with +// `target_missing`. The DSL was building a field that could never +// accept a value, and a caller relying on that was relying on a +// field that did not work. Same disposition as v0.24's strict-input +// half: the reliance was indistinguishable from a caller bug, so the +// break is the fix. +// +// Title resolution is COLLECTION-SCOPED, and that is the whole +// point rather than an implementation detail. A title unique only +// workspace-wide REFUSES, naming the collection searched; resolving +// it workspace-wide would let free text land on whatever happens to +// carry that title today. Two or more matches inside the declared +// collection get a new `ambiguous` reason rather than `not_found`, +// which would state the opposite of what happened. The ladder is +// UUID, then ref, then title, so an item literally TITLED "COLO-3" +// is unreachable by title while the ref COLO-3 resolves — deliberate, +// since refs are canonical and the ambiguity is the title's doing. +// +// An id-only `relation_targets` entry means the target is gone OR +// the caller may not see it, and consumers must not render it as +// either: the two are made indistinguishable on purpose, and the +// write side collapses `wrong_collection` to `not_found` for the +// same reason. +// // - POST-0.30, NO BUMP — BUG-2995. A successful content write through // the designated applier used to answer with the item's PREVIOUS // content: on that path the markdown goes to a live browser tab's @@ -975,7 +1016,7 @@ const CmdhelpVersion = "0.1" // is ever updated is not established — the flush belongs to a // browser tab and BUG-3000 carries the open half — so no surface // here states a duration. -const ToolSurfaceVersion = "0.30" +const ToolSurfaceVersion = "0.31" // 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/item.go b/internal/models/item.go index b55f8a080..aa50bfed7 100644 --- a/internal/models/item.go +++ b/internal/models/item.go @@ -109,6 +109,20 @@ type ItemWriteWarnings struct { ContentOutcome string `json:"content_outcome,omitempty"` } +// RelationTarget is one hydrated `relation` value: the item an ID points at, +// in the smallest shape a caller needs to RENDER it without a second request +// (PLAN-2857 U6). +// +// `ID` is always set — it is the stored value, and it is all there is when the +// target is gone or invisible. `Ref` and `Title` are present only when the +// target resolved and the requester may see it, so a consumer must treat them +// as optional rather than assuming a hydrated entry is complete. +type RelationTarget struct { + ID string `json:"id"` + Ref string `json:"ref,omitempty"` + Title string `json:"title,omitempty"` +} + // ContentOutcomeAppliedPendingFlush is the only ContentOutcome value that rides on // a 2xx: the content reached the collaborative document and items.content has not // caught up. It lives here rather than beside the error-path outcome constants @@ -222,6 +236,25 @@ type Item struct { ParentSlug string `json:"parent_slug,omitempty"` ParentCollectionSlug string `json:"parent_collection_slug,omitempty"` + // RelationTargets hydrates `relation` field values for READ responses + // (PLAN-2857 U6): field key -> the item that value names. + // + // It sits BESIDE `fields` rather than inside it, by lead ruling. `fields` is + // a write shape as well as a read shape — a caller reads an item, changes + // one key and writes the object back — so replacing a relation's ID string + // with an object there would break that round trip for every existing + // client. Here it is additive and omitempty: a consumer that does not know + // the key ignores it. + // + // A value that cannot be hydrated appears as an ID-ONLY entry rather than + // being omitted. Omitting it would read as "this item has no relation", + // which is a different and false statement; the honest one is "it points at + // this id and I cannot tell you more". Two cases produce that: a DANGLING + // value whose target is gone, and a target the requester may not see — + // hydrating a ref or title for the latter would leak exactly what the + // write-side visibility collapse exists to withhold. + RelationTargets map[string]RelationTarget `json:"relation_targets,omitempty"` + // Warnings is populated on WRITE responses only (create / update), never // on reads, and is never stored. It carries things the write did that the // caller might not have meant, or could not otherwise learn: field keys the diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index 1b3fffb23..f63c0f0dc 100644 --- a/internal/server/handlers_items.go +++ b/internal/server/handlers_items.go @@ -78,7 +78,7 @@ func (s *Server) handleListItems(w http.ResponseWriter, r *http.Request) { if result == nil { result = []models.Item{} } - s.enrichItemsWithParent(workspaceID, result, visibleIDs) + s.enrichItemsWithParent(r, workspaceID, result, visibleIDs) writeJSON(w, http.StatusOK, result) } @@ -245,7 +245,7 @@ func (s *Server) handleListItemsIndex(w http.ResponseWriter, r *http.Request) { if result == nil { result = []models.Item{} } - s.enrichItemsWithParent(workspaceID, result, visibleIDs) + s.enrichItemsWithParent(r, workspaceID, result, visibleIDs) // Cursor: max(pre-list workspace floor, MAX(returned-rows.seq)). // See the long comment above MaxItemSeq for why the floor is read @@ -389,7 +389,7 @@ func (s *Server) handleListItemsChanges(w http.ResponseWriter, r *http.Request) // /items-index payload shape. Soft-deleted parents are skipped by // the underlying GetItem (deleted_at IS NULL), so we don't leak // parent title / ref after the parent itself has been archived. - s.enrichItemsWithParent(workspaceID, rows, visibleIDs) + s.enrichItemsWithParent(r, workspaceID, rows, visibleIDs) changes := make([]itemChangeRow, 0, len(rows)) for _, it := range rows { @@ -586,7 +586,7 @@ func (s *Server) handleListCollectionItems(w http.ResponseWriter, r *http.Reques if result == nil { result = []models.Item{} } - s.enrichItemsWithParent(workspaceID, result, visibleIDs) + s.enrichItemsWithParent(r, workspaceID, result, visibleIDs) writeJSON(w, http.StatusOK, result) } @@ -735,7 +735,7 @@ func (s *Server) handleCreateItem(w http.ResponseWriter, r *http.Request) { } createVisIDs, _ := s.visibleCollectionIDs(r, workspaceID) - if err := s.enrichItemForResponse(item, createVisIDs); err != nil { + if err := s.enrichItemForResponse(r, item, createVisIDs); err != nil { writeInternalError(w, err) return } @@ -1003,7 +1003,7 @@ func (s *Server) handleGetItem(w http.ResponseWriter, r *http.Request) { } enrichVisIDs, _ := s.visibleCollectionIDs(r, workspaceID) - if err := s.enrichItemForResponse(item, enrichVisIDs); err != nil { + if err := s.enrichItemForResponse(r, item, enrichVisIDs); err != nil { writeInternalError(w, err) return } @@ -2147,7 +2147,7 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { } updateVisIDs, _ := s.visibleCollectionIDs(r, workspaceID) - if err := s.enrichItemForResponse(updated, updateVisIDs); err != nil { + if err := s.enrichItemForResponse(r, updated, updateVisIDs); err != nil { writeInternalError(w, err) return } @@ -2283,7 +2283,7 @@ func (s *Server) handleRestoreItem(w http.ResponseWriter, r *http.Request) { s.publishItemEventWithName(sseItemRestored, workspaceID, restored.ID, restored.Title, restored.CollectionSlug, actor, actorNameFromRequest(r), source, restored.Seq) restoreVisIDs, _ := s.visibleCollectionIDs(r, workspaceID) - if err := s.enrichItemForResponse(restored, restoreVisIDs); err != nil { + if err := s.enrichItemForResponse(r, restored, restoreVisIDs); err != nil { writeInternalError(w, err) return } @@ -2457,6 +2457,7 @@ func (s *Server) handleMoveItem(w http.ResponseWriter, r *http.Request) { return } relRefusals, relDropped, relErr := s.store.MigrateRelationReferents( + s.relationVisibility(r, workspaceRole(r)), workspaceID, items.SchemaForMigratedFields(targetSchema), result.Fields, input.FieldOverrides, store.CarriedSourceValues(currentFields, result.Dropped), store.RelationCarryWithinWorkspace) @@ -2499,6 +2500,7 @@ func (s *Server) handleMoveItem(w http.ResponseWriter, r *http.Request) { // the required-field check has to see a value referent resolution dropped, // which is why the main pass runs first — see ResolveLateRelationDefaults. lateDropped, lateErr := s.store.ResolveLateRelationDefaults( + s.relationVisibility(r, workspaceRole(r)), workspaceID, items.SchemaForMigratedFields(targetSchema), result.Fields, relBefore) if lateErr != nil { writeInternalError(w, lateErr) @@ -2651,7 +2653,7 @@ func (s *Server) handleMoveItem(w http.ResponseWriter, r *http.Request) { s.publishWatchNotifications(workspaceID, moved, actor, actorNameForMove) moveVisIDs, _ := s.visibleCollectionIDs(r, workspaceID) - if err := s.enrichItemForResponse(moved, moveVisIDs); err != nil { + if err := s.enrichItemForResponse(r, moved, moveVisIDs); err != nil { writeInternalError(w, err) return } @@ -3039,7 +3041,7 @@ func (s *Server) handleGetItemChildren(w http.ResponseWriter, r *http.Request) { children = filtered } - s.enrichItemsWithParent(workspaceID, children, visibleIDs) + s.enrichItemsWithParent(r, workspaceID, children, visibleIDs) if visibleIDs != nil { // Visibility-aware has_children: only count visible grandchildren for i := range children { diff --git a/internal/server/handlers_items_bulk.go b/internal/server/handlers_items_bulk.go index 2e2d886e8..1d101be8a 100644 --- a/internal/server/handlers_items_bulk.go +++ b/internal/server/handlers_items_bulk.go @@ -563,7 +563,7 @@ func (s *Server) bulkFieldUpdate(r *http.Request, workspaceID string, item *mode for k, v := range suppliedRelations { fieldMap[k] = v } - lateDropped, lateErr := s.store.ResolveLateRelationDefaults(workspaceID, schema, fieldMap, relBefore) + lateDropped, lateErr := s.store.ResolveLateRelationDefaults(s.relationVisibility(r, workspaceRole(r)), workspaceID, schema, fieldMap, relBefore) if lateErr != nil { return nil, &bulkOpError{message: "Failed to resolve relation references", code: "internal_error"} } @@ -822,6 +822,7 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m return nil, &bulkOpError{message: relationIssuesMessage(invisible), code: "validation_error"} } relRefusals, relDropped, relErr := s.store.MigrateRelationReferents( + s.relationVisibility(r, workspaceRole(r)), workspaceID, items.SchemaForMigratedFields(targetSchema), result.Fields, suppliedByCaller, store.CarriedSourceValues(currentFields, result.Dropped), store.RelationCarryWithinWorkspace) @@ -851,6 +852,7 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m // Relation defaults ValidateFields just injected (codex round 2). After // validation for the reason ResolveLateRelationDefaults documents. lateDropped, lateErr := s.store.ResolveLateRelationDefaults( + s.relationVisibility(r, workspaceRole(r)), workspaceID, items.SchemaForMigratedFields(targetSchema), result.Fields, relBefore) if lateErr != nil { return nil, &bulkOpError{message: "Failed to resolve relation references", code: "internal_error"} diff --git a/internal/server/handlers_items_copy_preflight.go b/internal/server/handlers_items_copy_preflight.go index dde691b2f..400a97428 100644 --- a/internal/server/handlers_items_copy_preflight.go +++ b/internal/server/handlers_items_copy_preflight.go @@ -811,6 +811,7 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) return } relRefusals, relDropped, relErr := s.store.MigrateRelationReferents( + s.relationVisibility(r, dst.Role), dst.WorkspaceID(), items.SchemaForMigratedFields(targetSchema), final, input.FieldOverrides, carriedSource, relMode) if relErr != nil { @@ -866,6 +867,7 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) // reported an unresolved default as carrying while the copy dropped it // would be the DR-6 divergence in a new place. lateDropped, lateErr := s.store.ResolveLateRelationDefaults( + s.relationVisibility(r, dst.Role), dst.WorkspaceID(), items.SchemaForMigratedFields(targetSchema), final, relBefore) if lateErr != nil { writeInternalError(w, fmt.Errorf("copy preflight: resolve relation defaults: %w", lateErr)) diff --git a/internal/server/handlers_project_intel.go b/internal/server/handlers_project_intel.go index 6c3707965..85dfea9ec 100644 --- a/internal/server/handlers_project_intel.go +++ b/internal/server/handlers_project_intel.go @@ -441,7 +441,7 @@ func (s *Server) handleGetProjectChangelog(w http.ResponseWriter, r *http.Reques // (codex R1 P1, TASK-1916: using collIDs here dropped parent // fields for items whose parent lives in an item-grant-only // collection, silently breaking ?parent= for that guest). - s.enrichItemsWithParent(workspaceID, items, navVisibleIDs) + s.enrichItemsWithParent(r, workspaceID, items, navVisibleIDs) filtered := items[:0] for _, item := range items { if itemMatchesParentFilter(item, parent) { diff --git a/internal/server/handlers_stars.go b/internal/server/handlers_stars.go index 386108c6a..6446b14da 100644 --- a/internal/server/handlers_stars.go +++ b/internal/server/handlers_stars.go @@ -204,7 +204,7 @@ func (s *Server) handleListStarredItems(w http.ResponseWriter, r *http.Request) items = filtered // Hydrate items with parent links and computed refs - s.enrichItemsWithParent(workspaceID, items, visibleIDs) + s.enrichItemsWithParent(r, workspaceID, items, visibleIDs) if len(items) == 0 { items = []models.Item{} diff --git a/internal/server/item_lineage.go b/internal/server/item_lineage.go index 6f51b852f..e68747efc 100644 --- a/internal/server/item_lineage.go +++ b/internal/server/item_lineage.go @@ -2,6 +2,7 @@ package server import ( "fmt" + "net/http" "strings" "github.com/PerpetualSoftware/pad/internal/models" @@ -10,7 +11,7 @@ import ( // enrichItemsWithParent batch-populates parent link info on a slice of items. // Used by list endpoints where calling enrichItemForResponse per-item is too expensive. // visibleIDs controls which parent collections are allowed; nil means all visible. -func (s *Server) enrichItemsWithParent(workspaceID string, items []models.Item, visibleIDs ...[]string) { +func (s *Server) enrichItemsWithParent(r *http.Request, workspaceID string, items []models.Item, visibleIDs ...[]string) { if len(items) == 0 { return } @@ -22,6 +23,11 @@ func (s *Server) enrichItemsWithParent(workspaceID string, items []models.Item, hasVis = true } + // U6 hydration runs BEFORE the parent-map early return below. The two are + // unrelated decorations, and a workspace with no parent links at all would + // otherwise return here and silently ship every list read unhydrated. + s.hydrateRelationTargets(r, workspaceID, items) + parentMap, err := s.store.GetParentMap(workspaceID) if err != nil || len(parentMap) == 0 { return @@ -77,7 +83,7 @@ func (s *Server) enrichItemsWithParent(workspaceID string, items []models.Item, // enrichItemForResponse populates derived closure and parent info on a single item. // An optional visibleIDs slice filters related items so hidden-collection metadata // is not leaked. Pass nil (or omit) for full access. -func (s *Server) enrichItemForResponse(item *models.Item, visibleIDs ...[]string) error { +func (s *Server) enrichItemForResponse(r *http.Request, item *models.Item, visibleIDs ...[]string) error { if item == nil { return nil } @@ -94,6 +100,13 @@ func (s *Server) enrichItemForResponse(item *models.Item, visibleIDs ...[]string } item.DerivedClosure = closure + // U6: one item is the degenerate batch. Going through the same helper means + // a single read and a list read cannot disagree about what `relation_targets` + // says for the same item — including which targets collapse to id-only. + one := []models.Item{*item} + s.hydrateRelationTargets(r, item.WorkspaceID, one) + item.RelationTargets = one[0].RelationTargets + // Populate parent link info — skip if parent is in a hidden collection parentLink, err := s.store.GetParentForItem(item.ID) if err != nil { diff --git a/internal/server/item_moved_to_test.go b/internal/server/item_moved_to_test.go index 3ceb69b1c..7488c073f 100644 --- a/internal/server/item_moved_to_test.go +++ b/internal/server/item_moved_to_test.go @@ -797,7 +797,7 @@ func TestMovedTo_MutationResponsesDoNotCarryPointer(t *testing.T) { if fresh.MovedTo != nil { t.Fatal("store-level item carries MovedTo; it must only ever be set by handleGetItem") } - if err := f.srv.enrichItemForResponse(fresh, nil); err != nil { + if err := f.srv.enrichItemForResponse(httptest.NewRequest("GET", "/", nil), fresh, nil); err != nil { t.Fatalf("enrichItemForResponse: %v", err) } if fresh.MovedTo != nil { diff --git a/internal/server/relation_hydration.go b/internal/server/relation_hydration.go new file mode 100644 index 000000000..ef9005675 --- /dev/null +++ b/internal/server/relation_hydration.go @@ -0,0 +1,136 @@ +package server + +import ( + "encoding/json" + "net/http" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// hydrateRelationTargets fills `relation_targets` on a batch of items +// (PLAN-2857 U6): for each `relation` field the collection declares, the item +// the stored id names, as {id, ref, title}. +// +// It is the READ-side counterpart to the write-side resolver, and it inherits +// that resolver's division of labour: the store answers what the items say, and +// this layer — the one that knows who is asking — decides what may be said back. +// +// TWO THINGS COLLAPSE A TARGET TO ID-ONLY, and neither omits the key: +// +// - the value resolves to nothing live in this workspace (dangling, or an id +// naming another workspace, which the store declines to resolve); +// - the requester cannot see the target ITEM. +// +// THAT SECOND CHECK IS PER-ITEM, NOT PER-COLLECTION, and the difference is a +// leak rather than a nicety. `store.VisibleCollectionIDs` is deliberately +// NAV-LENIENT: it folds in a collection reachable only through an ITEM-level +// grant "so the collection appears in navigation", and leaves item-level +// filtering to the handlers — requireCollectionFullyVisible's comment says so, +// and exists because BUG-1920 walked into it. Filtering hydration on that set +// would hand a member with a grant on ONE item in a collection the ref and +// title of EVERY item any relation points at there. `checkItemVisible` is the +// authority, and it is asked once per DISTINCT TARGET, not once per item. +// +// Omitting the key instead would say "this item has no relation here", which is +// a different statement and a false one. An id-only entry says the true thing: +// the value is stored, and there is nothing more this requester may be told. +// The write side already collapses `wrong_collection` to `not_found` for an +// invisible target; hydrating that item's ref and title on the read would hand +// back exactly what that collapse withholds. +// +// BEST-EFFORT. A failure here leaves items unhydrated rather than failing the +// read — the same posture enrichItemsWithParent takes for parent info, and for +// the same reason: `relation_targets` is additive convenience, and a list read +// that 500s because a convenience field could not be built is a worse answer +// than one without it. +func (s *Server) hydrateRelationTargets(r *http.Request, workspaceID string, items []models.Item) { + if len(items) == 0 || r == nil { + return + } + + schemas, err := s.relationSchemasForWorkspace(workspaceID) + if err != nil || len(schemas) == 0 { + return + } + + targets, err := s.store.HydrateRelationTargetsQ(s.store.DB(), workspaceID, items, schemas) + if err != nil || len(targets) == 0 { + return + } + + // workspaceRole(r) is the right role here, unlike in the cross-workspace + // copy: every item being enriched belongs to the workspace in the URL, + // which is the workspace that role was stashed for. + user, role, bearer := currentUser(r), workspaceRole(r), isBearerAuth(r) + + // One decision per DISTINCT target, reused across every item pointing at + // it. A list page of 50 items sharing one target asks once. + allowed := map[string]bool{} + visible := func(id string) bool { + if seen, done := allowed[id]; done { + return seen + } + item, err := s.store.GetItem(id) + if err != nil || item == nil { + allowed[id] = false + return false + } + seen, err := s.checkItemVisible(workspaceID, item, user, role, bearer) + if err != nil { + // Fail CLOSED. An error here is not a licence to disclose. + seen = false + } + allowed[id] = seen + return seen + } + + for i := range items { + perField, ok := targets[items[i].ID] + if !ok { + continue + } + out := make(map[string]models.RelationTarget, len(perField)) + for key, target := range perField { + if target.Ref != "" && !visible(target.ID) { + // Resolved, but not for these eyes. Keep the id — the value IS + // stored — and drop what would disclose the target. + target = models.RelationTarget{ID: target.ID} + } + out[key] = target + } + items[i].RelationTargets = out + } +} + +// relationSchemasForWorkspace maps collection ID -> schema for every collection +// in the workspace that DECLARES a relation field. +// +// Narrowed to relation-declaring collections on purpose: the map is handed to +// the store's hydrator, which skips a collection it cannot find, so including +// the rest would only make a larger map for it to walk past. A collection with +// no relation field contributes no work either way. +func (s *Server) relationSchemasForWorkspace(workspaceID string) (map[string]models.CollectionSchema, error) { + colls, err := s.store.ListCollections(workspaceID) + if err != nil { + return nil, err + } + out := map[string]models.CollectionSchema{} + for i := range colls { + if colls[i].Schema == "" { + continue + } + var schema models.CollectionSchema + if err := json.Unmarshal([]byte(colls[i].Schema), &schema); err != nil { + // A schema that will not parse is a different defect, reported + // elsewhere; hydration declines to be the second voice. + continue + } + for _, def := range schema.Fields { + if def.Type == "relation" { + out[colls[i].ID] = schema + break + } + } + } + return out, nil +} diff --git a/internal/server/relation_referents.go b/internal/server/relation_referents.go index 983ecc06d..6deb83956 100644 --- a/internal/server/relation_referents.go +++ b/internal/server/relation_referents.go @@ -85,7 +85,7 @@ func (s *Server) resolveRelationReferentsAs( return canonical } - issues, err := s.store.ResolveRelationReferents(workspaceID, schema, fieldMap) + issues, err := s.store.ResolveRelationReferents(workspaceID, schema, fieldMap, s.relationVisibility(r, role)) if err != nil { return nil, err } @@ -120,9 +120,21 @@ func (s *Server) resolveRelationReferentsAs( // where a person belongs" is the useful half of this reason. // // Any other issue is already `not_found`-shaped and needs nothing. + // An `ambiguous` reason arrives already decided: since the + // resolver takes this requester's visibility predicate, its count + // is the VISIBLE count and there is nothing left to narrow here. + // It used to be re-decided in this layer, which left the copy + // transaction — where no server pass runs — leaking. if ri.Reason != store.RelationTargetWrongCollection { continue } + if ri.VisibilityChecked { + // The resolver judged this one against this requester already + // (title path). Re-resolving it here would run a title through + // a UUID-or-ref ladder, find nothing, and collapse a reason the + // caller is entitled to see. + continue + } target, terr := s.store.ResolveRelationTarget(workspaceID, ri.Value) if terr != nil { return nil, terr @@ -215,9 +227,13 @@ func collapseIssue(issues []store.RelationIssue, key string, reason store.Relati // Reviewer named ONE site; this is applied at all five late-default sites, // per CONVE-18 — the class is "a store-resolved issue reaching a caller // without passing the visibility collapse", not the one call it was spotted at. +// ONLY the ref path reaches here now. A title-derived issue is decided inside +// the resolver, which takes this requester's visibility predicate — so an +// invisible title match never becomes a `wrong_collection` in the first place, +// and there is nothing for this pass to collapse. func (s *Server) collapseInvisibleRelationIssues(r *http.Request, workspaceID, role string, issues []store.RelationIssue) error { for i := range issues { - if issues[i].Reason != store.RelationTargetWrongCollection { + if issues[i].Reason != store.RelationTargetWrongCollection || issues[i].VisibilityChecked { continue } target, terr := s.store.ResolveRelationTarget(workspaceID, issues[i].Value) @@ -452,7 +468,7 @@ func (s *Server) resolveRelationsForWrite( // — and reporting none of it. The refusals are still returned; they are // the carry report, not a stop. - lateDropped, err := s.store.ResolveLateRelationDefaults(workspaceID, schema, fieldMap, presentBefore) + lateDropped, err := s.store.ResolveLateRelationDefaults(s.relationVisibility(r, role), workspaceID, schema, fieldMap, presentBefore) if err != nil { return nil, nil, err } diff --git a/internal/server/relation_titles_copy_test.go b/internal/server/relation_titles_copy_test.go new file mode 100644 index 000000000..a7c0b6ffd --- /dev/null +++ b/internal/server/relation_titles_copy_test.go @@ -0,0 +1,116 @@ +package server + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// The copy door and the ruled visibility property (PLAN-2857 U6). +// +// These are the legs codex round 3 named as missing, and they are the reason +// the property had to move INTO the resolver. The cross-workspace copy runs the +// resolver inside its own transaction, so a server-layer second pass could not +// reach it: a title override narrowed correctly and then had the narrowed id +// thrown away, and the copy's late defaults were never narrowed at all — which +// made PREFLIGHT and COPY disagree, the DR-6 divergence class in a new place. + +// twinsInDest creates two same-titled live items in the destination target +// collection and returns them, plus an editor who can see the FIRST only. +// +// The editor's access to wsB is collection-level on the tasks collection and +// item-level on one twin. That combination is the point: it is what makes the +// visible count differ from the live count. +func twinsInDest(t *testing.T, f *relationFixture, title string) (*models.Item, *models.Item, *models.User) { + t.Helper() + visible, err := f.srv.store.CreateItem(f.wsB.ID, f.targetsB.ID, models.ItemCreate{Title: title, CreatedBy: f.owner.ID}) + if err != nil { + t.Fatalf("CreateItem(visible twin): %v", err) + } + hidden, err := f.srv.store.CreateItem(f.wsB.ID, f.targetsB.ID, models.ItemCreate{Title: title, CreatedBy: f.owner.ID}) + if err != nil { + t.Fatalf("CreateItem(hidden twin): %v", err) + } + u := f.restrictedEditor("copy-twin@example.com", "copytwin", + []string{f.collA.ID}, []string{f.collB.ID}) + if _, err := f.srv.store.CreateItemGrant(f.wsB.ID, visible.ID, u.ID, "read", f.owner.ID); err != nil { + t.Fatalf("CreateItemGrant: %v", err) + } + return visible, hidden, u +} + +// TestRelationTitleCopy_OverrideResolvesForACallerWhoSeesOneTwin is round 3's +// first P1: the copy re-resolved the raw title inside its transaction and saw +// `ambiguous`, so a hidden twin changed the copy's STATUS. +func TestRelationTitleCopy_OverrideResolvesForACallerWhoSeesOneTwin(t *testing.T) { + f := newCopyRelationFixture(t) + visible, hidden, u := twinsInDest(t, f, "Twin In B") + + body := f.baseBody() + body["field_overrides"] = map[string]any{"owner_ref": "Twin In B"} + + rr := f.callCopy(u, reqOpts{}, body) + if rr.Code != http.StatusCreated && rr.Code != http.StatusOK { + t.Fatalf("a caller who can see exactly ONE match must have the override resolve — the twin they cannot see is not their ambiguity: %d %s", rr.Code, rr.Body.String()) + } + var res ItemCopyResult + decodeInto(t, rr, &res) + got := f.persistedFields(res.Item.ID)["owner_ref"] + if got != visible.ID { + t.Errorf("stored owner_ref = %#v, want the VISIBLE twin %s (hidden twin is %s)", got, visible.ID, hidden.ID) + } +} + +// TestRelationTitleCopy_PreflightAndCopyAgreeOnANarrowedTitle is round 3's +// second P1, and it is the one a per-door test cannot make: agreement between +// two doors is a claim about the PAIR. +// +// Preflight applied the narrowing and the copy did not, so the preview promised +// a value the copy then refused or dropped. +func TestRelationTitleCopy_PreflightAndCopyAgreeOnANarrowedTitle(t *testing.T) { + f := newCopyRelationFixture(t) + visible, _, u := twinsInDest(t, f, "Agreed Twin") + + body := f.baseBody() + body["field_overrides"] = map[string]any{"owner_ref": "Agreed Twin"} + + // PREFLIGHT first, as a user would. + preRR := f.call(u, reqOpts{}, body) + if preRR.Code != http.StatusOK { + t.Fatalf("preflight: expected 200, got %d: %s", preRR.Code, preRR.Body.String()) + } + var pre ItemCopyPreflight + decodeInto(t, preRR, &pre) + promised, carried := carriedValue(pre, "owner_ref") + if !carried { + reason, dropped := droppedReason(pre, "owner_ref") + t.Fatalf("preflight did not promise the narrowed value (dropped=%v reason=%q)", dropped, reason) + } + if promised != visible.ID { + t.Errorf("preflight promised %#v, want the visible twin %s", promised, visible.ID) + } + + // Then the COPY, with the identical body. The two must agree. + copyRR := f.callCopy(u, reqOpts{}, body) + if copyRR.Code != http.StatusCreated && copyRR.Code != http.StatusOK { + t.Fatalf("the preflight promised a value and the copy refused: %d %s", copyRR.Code, copyRR.Body.String()) + } + var res ItemCopyResult + decodeInto(t, copyRR, &res) + stored := f.persistedFields(res.Item.ID)["owner_ref"] + if stored != promised { + t.Errorf("preflight promised %#v and the copy stored %#v — a preview that does not predict the copy is the DR-6 divergence", promised, stored) + } +} + +// decodeInto unmarshals a recorder body, failing with the body on error so a +// decode failure names what came back rather than only that it did not parse. +func decodeInto(t *testing.T, rr *httptest.ResponseRecorder, out any) { + t.Helper() + if err := json.Unmarshal(rr.Body.Bytes(), out); err != nil { + t.Fatalf("decode: %v\nbody: %s", err, rr.Body.String()) + } +} diff --git a/internal/server/relation_titles_doors_test.go b/internal/server/relation_titles_doors_test.go new file mode 100644 index 000000000..b1f08f9a3 --- /dev/null +++ b/internal/server/relation_titles_doors_test.go @@ -0,0 +1,586 @@ +package server + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// Door-level legs for exact-title resolution and read hydration (PLAN-2857 U6). +// +// internal/store already tests the title ladder and the hydrator directly, so +// those vouch for the COMPONENTS. They say nothing about whether any door is +// BOUND to them — a create handler that never reaches the title rung, or a read +// handler that never hydrates, passes every store test. These are the binding +// claims, driven through the handlers a client actually reaches. Same +// separation, and same reason, as the U1 table above. + +// TestRelationTitleDoors_ThreeSpellingsAgreeThroughTheCreateDoor is the design +// row's proving test at the door: uuid, ref and exact title, each through +// handleCreateItem, must land on an identical STORED id. +// +// Read back out of the database rather than off the response, for the reason +// storedRelation documents: a handler that echoed the right thing while writing +// the wrong thing would pass a response-only check. +func TestRelationTitleDoors_ThreeSpellingsAgreeThroughTheCreateDoor(t *testing.T) { + f := newDoorFixture(t) + path := "/api/v1/workspaces/" + f.ws.Slug + "/collections/" + f.tasks.Slug + "/items" + params := map[string]string{"collSlug": f.tasks.Slug} + + for _, supplied := range []string{f.target.ID, f.target.Ref, f.target.Title} { + body := map[string]any{"title": "By " + supplied, "fields": map[string]any{"owner_ref": supplied}} + rr := f.call(f.srv.handleCreateItem, "POST", path, params, body) + if rr.Code != http.StatusCreated { + t.Fatalf("supplied %q: expected 201, got %d: %s", supplied, rr.Code, rr.Body.String()) + } + var created models.Item + if err := json.Unmarshal(rr.Body.Bytes(), &created); err != nil { + t.Fatalf("supplied %q: decode: %v", supplied, err) + } + stored, ok := f.storedRelation(created.ID) + if !ok { + t.Fatalf("supplied %q: no owner_ref stored", supplied) + } + if stored != f.target.ID { + t.Errorf("supplied %q stored %v, want the canonical id %s — the door must agree with the other two spellings", supplied, stored, f.target.ID) + } + } +} + +// TestRelationTitleDoors_TitleWrongCollectionDoesNotDiscloseExistence is +// ruling (3) on the TITLE path, and it is the leg the addendum was written for. +// +// The ref-path equivalent (TestRelationDoors_WrongCollectionDoesNotDisclose- +// Existence) collapses by RE-RESOLVING the value through ResolveRelationTarget. +// That ladder does not speak titles, so a title-derived wrong_collection would +// re-resolve to nil, take the not-found arm, and collapse for EVERY caller — +// visible and invisible alike. The invisible leg below would still pass. Only +// the VISIBLE leg catches it, which is why both are here and why the fix +// carries the matched id on the issue instead of looking it up twice. +func TestRelationTitleDoors_TitleWrongCollectionDoesNotDiscloseExistence(t *testing.T) { + f := newDoorFixture(t) + // A live item in a collection that is NOT the relation's declared target, + // addressed by its TITLE. + other := mustSchemaCollection(t, f.srv, f.ws.ID, "Vaults", `{"fields":[]}`) + if _, err := f.srv.store.CreateItem(f.ws.ID, other.ID, models.ItemCreate{ + Title: "Ledger", CreatedBy: f.owner.ID, + }); err != nil { + t.Fatalf("CreateItem(ledger): %v", err) + } + + body := map[string]any{"title": "New", "fields": map[string]any{"owner_ref": "Ledger"}} + path := "/api/v1/workspaces/" + f.ws.Slug + "/collections/" + f.tasks.Slug + "/items" + params := map[string]string{"collSlug": f.tasks.Slug} + + // VISIBLE: the owner can see Vaults, so they keep the specific reason. This + // is the leg that dies if the collapse re-resolves the title. + seeing := f.call(f.srv.handleCreateItem, "POST", path, params, body) + if seeing.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", seeing.Code, seeing.Body.String()) + } + if !strings.Contains(seeing.Body.String(), "is not an item in collection") { + t.Fatalf("a caller who CAN see the target lost the wrong_collection reason — the collapse judged the wrong item, or could not find it at all: %s", seeing.Body.String()) + } + + // INVISIBLE: an editor with no access to Vaults must not learn that the + // title names anything. A title is guessable in a way a ref is not, which + // is why this leg exists at all. + blind := mustUser(t, f.srv, "blind-title@example.com", "blindtitle", "") + if err := f.srv.store.AddWorkspaceMember(f.ws.ID, blind.ID, "editor"); err != nil { + t.Fatalf("AddWorkspaceMember: %v", err) + } + if err := f.srv.store.SetMemberCollectionAccess(f.ws.ID, blind.ID, "specific", + []string{f.tasks.ID, f.people.ID}); err != nil { + t.Fatalf("SetMemberCollectionAccess: %v", err) + } + hidden := f.callAs(blind, "editor", f.srv.handleCreateItem, "POST", path, params, body) + if hidden.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", hidden.Code, hidden.Body.String()) + } + if strings.Contains(hidden.Body.String(), "is not an item in collection") { + t.Fatalf("the refusal tells a caller who cannot see the target that the TITLE names something: %s", hidden.Body.String()) + } + if !strings.Contains(hidden.Body.String(), "does not name an item") { + t.Fatalf("expected the not_found phrasing, got: %s", hidden.Body.String()) + } +} + +// TestRelationTitleDoors_ReadHydratesTheTarget is the hydration half's binding +// claim: the read door returns relation_targets, and a dangling value comes +// back ID-ONLY and PRESENT rather than omitted. +func TestRelationTitleDoors_ReadHydratesTheTarget(t *testing.T) { + f := newDoorFixture(t) + + live := f.seed(`{"owner_ref":"` + f.target.ID + `"}`) + dangling := f.seed(`{"owner_ref":"00000000-0000-4000-8000-00000000dead"}`) + + get := func(it *models.Item) models.Item { + t.Helper() + rr := f.call(f.srv.handleGetItem, "GET", + "/api/v1/workspaces/"+f.ws.Slug+"/items/"+it.Slug, + map[string]string{"itemSlug": it.Slug}, nil) + if rr.Code != http.StatusOK { + t.Fatalf("get %s: expected 200, got %d: %s", it.Slug, rr.Code, rr.Body.String()) + } + var out models.Item + if err := json.Unmarshal(rr.Body.Bytes(), &out); err != nil { + t.Fatalf("decode %s: %v", it.Slug, err) + } + return out + } + + got := get(live) + target, ok := got.RelationTargets["owner_ref"] + if !ok { + t.Fatal("the read door returned no relation_targets — the hydrator is not bound to it, which every store-level test would still pass") + } + if target.ID != f.target.ID || target.Ref != f.target.Ref || target.Title != f.target.Title { + t.Errorf("hydrated as %+v, want {id:%s ref:%s title:%s}", target, f.target.ID, f.target.Ref, f.target.Title) + } + + got = get(dangling) + target, ok = got.RelationTargets["owner_ref"] + if !ok { + t.Fatal("a dangling value was OMITTED; omission reads as \"this item has no relation\", which is a different and false statement") + } + if target.Ref != "" || target.Title != "" { + t.Errorf("a dangling value hydrated as %+v, want id-only", target) + } + if target.ID == "" { + t.Error("a dangling entry lost the stored id, which is the only thing the caller had") + } +} + +// TestRelationTitleDoors_ReadDoesNotHydrateAnInvisibleTarget is the read-side +// mirror of the disclosure rule above. +// +// The write side collapses `wrong_collection` to `not_found` for a target the +// caller cannot see. Hydrating that same item's ref and title on the READ would +// hand back exactly what the collapse withholds — a second door onto the fact +// the first one closes. +func TestRelationTitleDoors_ReadDoesNotHydrateAnInvisibleTarget(t *testing.T) { + f := newDoorFixture(t) + + it := f.seed(`{"owner_ref":"` + f.target.ID + `"}`) + + blind := mustUser(t, f.srv, "blind-read@example.com", "blindread", "") + if err := f.srv.store.AddWorkspaceMember(f.ws.ID, blind.ID, "editor"); err != nil { + t.Fatalf("AddWorkspaceMember: %v", err) + } + // Can see the item itself, but NOT the collection its relation points into. + if err := f.srv.store.SetMemberCollectionAccess(f.ws.ID, blind.ID, "specific", + []string{f.tasks.ID}); err != nil { + t.Fatalf("SetMemberCollectionAccess: %v", err) + } + + rr := f.callAs(blind, "editor", f.srv.handleGetItem, "GET", + "/api/v1/workspaces/"+f.ws.Slug+"/items/"+it.Slug, + map[string]string{"itemSlug": it.Slug}, nil) + if rr.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + var out models.Item + if err := json.Unmarshal(rr.Body.Bytes(), &out); err != nil { + t.Fatalf("decode: %v", err) + } + target, ok := out.RelationTargets["owner_ref"] + if !ok { + t.Fatal("the key was omitted entirely; id-only is the honest answer, omission says the item has no relation") + } + if target.Ref != "" || target.Title != "" { + t.Errorf("hydrated an invisible target as %+v — that is the ref and title the write-side collapse exists to withhold", target) + } + + // CONTROL, same fixture: the owner CAN see People and gets the full triple. + // Without it, a hydrator that returned id-only for everything would pass. + ownerRR := f.call(f.srv.handleGetItem, "GET", + "/api/v1/workspaces/"+f.ws.Slug+"/items/"+it.Slug, + map[string]string{"itemSlug": it.Slug}, nil) + var ownerOut models.Item + if err := json.Unmarshal(ownerRR.Body.Bytes(), &ownerOut); err != nil { + t.Fatalf("decode owner: %v", err) + } + if ownerOut.RelationTargets["owner_ref"].Ref != f.target.Ref { + t.Errorf("control: the owner should see the full triple, got %+v", ownerOut.RelationTargets["owner_ref"]) + } +} + +// --- title resolution is scoped to what the caller can SEE ---------------- +// +// Lead ruling on PLAN-2857 U6, one step past "collapse ambiguous when nothing +// is visible": among live exact-title matches in the declared collection, only +// VISIBLE ones count — zero is not_found, one RESOLVES, two or more is +// ambiguous. A hidden match never changes the answer a caller gets. +// +// The rule lives in the RESOLVER, which takes the requester's visibility +// predicate (store.RelationVisibilityFunc). It was a server-side second pass +// first, and that left the cross-workspace copy — which runs the resolver +// inside its own transaction — deciding without it. + +// restrictedTitleFixture builds two same-titled colours and a member who can +// see the Doors collection plus, through an ITEM-LEVEL grant, exactly the +// colours named. Item grants are the point: they are what makes +// VisibleCollectionIDs nav-lenient, and therefore what a collection-level +// filter gets wrong. +func restrictedTitleFixture(t *testing.T, f *doorFixture, email string, grantItems ...*models.Item) *models.User { + t.Helper() + u := mustUser(t, f.srv, email, strings.ReplaceAll(email, "@example.com", ""), "") + if err := f.srv.store.AddWorkspaceMember(f.ws.ID, u.ID, "editor"); err != nil { + t.Fatalf("AddWorkspaceMember: %v", err) + } + // Explicit access to the collection under test only. People is reachable + // ONLY through the item grants below — the nav-lenient case. + if err := f.srv.store.SetMemberCollectionAccess(f.ws.ID, u.ID, "specific", []string{f.tasks.ID}); err != nil { + t.Fatalf("SetMemberCollectionAccess: %v", err) + } + for _, it := range grantItems { + if _, err := f.srv.store.CreateItemGrant(f.ws.ID, it.ID, u.ID, "read", f.owner.ID); err != nil { + t.Fatalf("CreateItemGrant(%s): %v", it.Title, err) + } + } + return u +} + +// createByTitle drives the create door as `user` with a relation supplied BY +// TITLE, returning the recorder so each leg can assert on the outcome. +func (f *doorFixture) createByTitle(user *models.User, title, itemTitle string) *httptest.ResponseRecorder { + f.t.Helper() + return f.callAs(user, "editor", f.srv.handleCreateItem, "POST", + "/api/v1/workspaces/"+f.ws.Slug+"/collections/"+f.tasks.Slug+"/items", + map[string]string{"collSlug": f.tasks.Slug}, + map[string]any{"title": itemTitle, "fields": map[string]any{"owner_ref": title}}) +} + +func TestRelationTitleDoors_AmbiguityIsCountedOverVISIBLEMatchesOnly(t *testing.T) { + f := newDoorFixture(t) + // Two live people share a title. The OWNER sees both. + twinA, err := f.srv.store.CreateItem(f.ws.ID, f.people.ID, models.ItemCreate{Title: "Robin", CreatedBy: f.owner.ID}) + if err != nil { + t.Fatalf("CreateItem(twinA): %v", err) + } + twinB, err := f.srv.store.CreateItem(f.ws.ID, f.people.ID, models.ItemCreate{Title: "Robin", CreatedBy: f.owner.ID}) + if err != nil { + t.Fatalf("CreateItem(twinB): %v", err) + } + + t.Run("two visible matches stay ambiguous", func(t *testing.T) { + both := restrictedTitleFixture(t, f, "sees-both@example.com", twinA, twinB) + rr := f.createByTitle(both, "Robin", "Both") + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", rr.Code, rr.Body.String()) + } + if !strings.Contains(rr.Body.String(), "more than one") && !strings.Contains(rr.Body.String(), "ambiguous") { + t.Errorf("a caller who can see BOTH matches must be told the title is ambiguous, got: %s", rr.Body.String()) + } + }) + + t.Run("one visible match resolves, and the hidden twin changes nothing", func(t *testing.T) { + one := restrictedTitleFixture(t, f, "sees-one@example.com", twinA) + rr := f.createByTitle(one, "Robin", "Only one") + if rr.Code != http.StatusCreated { + t.Fatalf("a caller who can see exactly ONE match must have it resolve — the other is not ambiguity for them: %d %s", rr.Code, rr.Body.String()) + } + var created models.Item + if err := json.Unmarshal(rr.Body.Bytes(), &created); err != nil { + t.Fatalf("decode: %v", err) + } + stored, ok := f.storedRelation(created.ID) + if !ok { + t.Fatal("no owner_ref stored") + } + if stored != twinA.ID { + t.Errorf("stored %v, want the VISIBLE twin %s", stored, twinA.ID) + } + }) + + t.Run("no visible match is not_found, never ambiguous", func(t *testing.T) { + none := restrictedTitleFixture(t, f, "sees-none@example.com") + rr := f.createByTitle(none, "Robin", "Neither") + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", rr.Code, rr.Body.String()) + } + body := rr.Body.String() + if strings.Contains(body, "more than one") || strings.Contains(body, "ambiguous") { + t.Errorf("telling a caller the title matches SEVERAL items they cannot see is the oracle this rule closes: %s", body) + } + if !strings.Contains(body, "does not name an item") { + t.Errorf("expected the not_found phrasing, got: %s", body) + } + }) +} + +// TestRelationTitleDoors_HydrationUsesITEMVisibilityNotCollectionVisibility is +// the read-side half of the same trap, and it is a leak a collection-level +// filter cannot catch. +// +// store.VisibleCollectionIDs is deliberately NAV-LENIENT: it folds in a +// collection reachable only through an ITEM grant "so the collection appears in +// navigation", leaving item-level filtering to handlers. +// requireCollectionFullyVisible's comment says so, and exists because BUG-1920 +// walked into it. So a member holding a grant on ONE person gets People in that +// set — and a hydration filtered on it hands them the ref and title of EVERY +// person any item points at. +func TestRelationTitleDoors_HydrationUsesITEMVisibilityNotCollectionVisibility(t *testing.T) { + f := newDoorFixture(t) + + granted, err := f.srv.store.CreateItem(f.ws.ID, f.people.ID, models.ItemCreate{Title: "Granted Person", CreatedBy: f.owner.ID}) + if err != nil { + t.Fatalf("CreateItem(granted): %v", err) + } + + pointsAtGranted := f.seed(`{"owner_ref":"` + granted.ID + `"}`) + pointsAtSecret := f.seed(`{"owner_ref":"` + f.target.ID + `"}`) + + // The grant is on `granted` ONLY. f.target (Ada) is a sibling in the same + // collection, with no grant. + member := restrictedTitleFixture(t, f, "item-grant@example.com", granted, pointsAtGranted, pointsAtSecret) + + read := func(it *models.Item) models.RelationTarget { + t.Helper() + rr := f.callAs(member, "editor", f.srv.handleGetItem, "GET", + "/api/v1/workspaces/"+f.ws.Slug+"/items/"+it.Slug, + map[string]string{"itemSlug": it.Slug}, nil) + if rr.Code != http.StatusOK { + t.Fatalf("get %s: expected 200, got %d: %s", it.Slug, rr.Code, rr.Body.String()) + } + var out models.Item + if err := json.Unmarshal(rr.Body.Bytes(), &out); err != nil { + t.Fatalf("decode: %v", err) + } + return out.RelationTargets["owner_ref"] + } + + // THE LEAK: a sibling in a nav-lenient collection, with no grant of its own. + secret := read(pointsAtSecret) + if secret.Ref != "" || secret.Title != "" { + t.Errorf("hydrated an UNGRANTED sibling as %+v — the collection is only reachable through an item grant, and filtering on collection visibility hands over every item in it", secret) + } + if secret.ID == "" { + t.Error("the stored id was dropped; id-only is the honest answer, omission is not") + } + + // CONTROL, same member and same request shape: the item they DO hold a + // grant on still hydrates fully. Without this a hydrator that returned + // id-only for everything would pass the assertion above. + ok := read(pointsAtGranted) + if ok.Ref != granted.Ref || ok.Title != "Granted Person" { + t.Errorf("control: the GRANTED target should hydrate fully, got %+v", ok) + } +} + +// TestRelationTitleDoors_VisibilityNarrowingHasNoCountBoundary is the +// regression test for codex round 2's P1: the first fix bounded the candidate +// list at a constant and answered `ambiguous` past it, which made the BOUND an +// oracle — a caller could tell "one visible plus cap hidden" (resolves) from +// "one visible plus cap+1 hidden" (ambiguous), and the difference is entirely +// about items they cannot see. +// +// The fix pages through candidates and stops at the SECOND VISIBLE one, so no +// match count changes the answer. This drives well past the page size with a +// single visible match: it must still resolve. +func TestRelationTitleDoors_VisibilityNarrowingHasNoCountBoundary(t *testing.T) { + f := newDoorFixture(t) + + // One visible match. Creation ORDER does not place it in the walk — the + // walk goes by `id` and ids are random UUIDs — so this is not "created + // first, therefore visited last". What forces the walk past the page + // boundary is the COUNT: 251 matches against a page size of 100, so the + // single visible one cannot be decided without paging wherever it lands. + visible, err := f.srv.store.CreateItem(f.ws.ID, f.people.ID, models.ItemCreate{Title: "Crowd", CreatedBy: f.owner.ID}) + if err != nil { + t.Fatalf("CreateItem(visible): %v", err) + } + // Comfortably past relationTitleCandidatePage (100), so the walk pages. + const hidden = 250 + for i := 0; i < hidden; i++ { + if _, err := f.srv.store.CreateItem(f.ws.ID, f.people.ID, models.ItemCreate{ + Title: "Crowd", CreatedBy: f.owner.ID, + }); err != nil { + t.Fatalf("CreateItem(hidden %d): %v", i, err) + } + } + + member := restrictedTitleFixture(t, f, "one-in-a-crowd@example.com", visible) + rr := f.createByTitle(member, "Crowd", "Found in the crowd") + if rr.Code != http.StatusCreated { + t.Fatalf("a caller who can see exactly ONE of %d matches must have it resolve, whatever the total: %d %s", hidden+1, rr.Code, rr.Body.String()) + } + var created models.Item + if err := json.Unmarshal(rr.Body.Bytes(), &created); err != nil { + t.Fatalf("decode: %v", err) + } + stored, ok := f.storedRelation(created.ID) + if !ok { + t.Fatal("no owner_ref stored") + } + if stored != visible.ID { + t.Errorf("stored %v, want the one visible match %s", stored, visible.ID) + } + + // CONTROL: the OWNER sees all 251 and is told it is ambiguous. Without + // this, a narrowing that resolved to the first match for everybody would + // pass the leg above. + ownerRR := f.call(f.srv.handleCreateItem, "POST", + "/api/v1/workspaces/"+f.ws.Slug+"/collections/"+f.tasks.Slug+"/items", + map[string]string{"collSlug": f.tasks.Slug}, + map[string]any{"title": "Owner tries", "fields": map[string]any{"owner_ref": "Crowd"}}) + if ownerRR.Code != http.StatusBadRequest { + t.Fatalf("control: a caller who sees every match must be refused, got %d: %s", ownerRR.Code, ownerRR.Body.String()) + } +} + +// TestRelationTitleDoors_UpdateDoorAppliesTheSameNarrowing answers codex round +// 2's P2 on coverage: the three ambiguity legs only drove CREATE, and each door +// reaches the resolver by its own route. +func TestRelationTitleDoors_UpdateDoorAppliesTheSameNarrowing(t *testing.T) { + f := newDoorFixture(t) + twinA, err := f.srv.store.CreateItem(f.ws.ID, f.people.ID, models.ItemCreate{Title: "Sam", CreatedBy: f.owner.ID}) + if err != nil { + t.Fatalf("CreateItem(twinA): %v", err) + } + if _, err := f.srv.store.CreateItem(f.ws.ID, f.people.ID, models.ItemCreate{Title: "Sam", CreatedBy: f.owner.ID}); err != nil { + t.Fatalf("CreateItem(twinB): %v", err) + } + + subject := f.seed(`{"status":"open"}`) + member := restrictedTitleFixture(t, f, "update-sees-one@example.com", twinA, subject) + + rr := f.callAs(member, "editor", f.srv.handleUpdateItem, "PATCH", + "/api/v1/workspaces/"+f.ws.Slug+"/items/"+subject.Slug, + map[string]string{"itemSlug": subject.Slug}, + map[string]any{"fields_patch": map[string]any{"owner_ref": "Sam"}}) + if rr.Code != http.StatusOK { + t.Fatalf("the update door must narrow the same way create does: %d %s", rr.Code, rr.Body.String()) + } + stored, ok := f.storedRelation(subject.ID) + if !ok { + t.Fatal("no owner_ref stored") + } + if stored != twinA.ID { + t.Errorf("stored %v, want the VISIBLE twin %s", stored, twinA.ID) + } +} + +// TestRelationTitleDoors_LateDefaultResolvesForACallerWhoSeesOne is codex round +// 2's other P1, and it is a leak through the OUTCOME rather than the message. +// +// A relation DEFAULT declared as a title goes through the late pass, which runs +// after validation injects it. The first fix could only downgrade a narrowed +// ambiguity to not_found there — ResolveLateRelationDefaults has already +// deleted the key and the collapse had no field map to restore it into. So a +// caller who was the twin's only visible match got the default APPLIED when +// they were the sole match and DROPPED when a twin they cannot see existed. The +// reason said nothing, and the outcome said everything. +func TestRelationTitleDoors_LateDefaultResolvesForACallerWhoSeesOne(t *testing.T) { + f := newDoorFixture(t) + + visible, err := f.srv.store.CreateItem(f.ws.ID, f.people.ID, models.ItemCreate{Title: "Morgan", CreatedBy: f.owner.ID}) + if err != nil { + t.Fatalf("CreateItem(visible): %v", err) + } + if _, err := f.srv.store.CreateItem(f.ws.ID, f.people.ID, models.ItemCreate{Title: "Morgan", CreatedBy: f.owner.ID}); err != nil { + t.Fatalf("CreateItem(hidden twin): %v", err) + } + + // The default is a TITLE, and it is ambiguous in the store's own counting. + defaulted := mustSchemaCollection(t, f.srv, f.ws.ID, "TitleDefaulted", fmt.Sprintf(`{"fields":[ + {"key":"status","label":"Status","type":"select","options":["open","done"]}, + {"key":"priority","label":"Priority","type":"select","options":["low","high"]}, + {"key":"owner_ref","label":"Owner","type":"relation","collection":%q,"default":"Morgan"} + ]}`, f.people.Slug)) + + subject, err := f.srv.store.CreateItem(f.ws.ID, defaulted.ID, models.ItemCreate{ + Title: "Gets a defaulted owner", Fields: `{"status":"open"}`, CreatedBy: f.owner.ID, + }) + if err != nil { + t.Fatalf("CreateItem(subject): %v", err) + } + + member := restrictedTitleFixture(t, f, "late-default@example.com", visible, subject) + if err := f.srv.store.SetMemberCollectionAccess(f.ws.ID, member.ID, "specific", + []string{f.tasks.ID, defaulted.ID}); err != nil { + t.Fatalf("SetMemberCollectionAccess: %v", err) + } + + // The BULK door, not fields_patch. The patch route resolves only the keys + // the caller named, so a defaulted relation never reaches the late pass + // there at all — I aimed this at fields_patch first and it proved nothing. + // `set-priority` is the same route TestRelationDoors_BulkUpdateResolves- + // InjectedRelationDefault uses, and it does run the injection. + rr := f.callAs(member, "editor", f.srv.handleBulkItems, "POST", + "/api/v1/workspaces/"+f.ws.Slug+"/items/bulk", nil, + map[string]any{"op": "set-priority", "ids": []string{subject.ID}, "priority": "high"}) + if rr.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + + stored, ok := f.storedRelationKey(subject.ID, "owner_ref") + if !ok { + t.Fatal("the defaulted relation was DROPPED for a caller who can see exactly one match; the drop itself tells them a twin they cannot see exists") + } + if stored != visible.ID { + t.Errorf("stored %v, want the one VISIBLE match %s", stored, visible.ID) + } +} + +// TestRelationTitleDoors_ProbeFindsAVisibleMatchNotAnArbitraryOne is codex +// round 4's P2, and it is the property failing in the other direction. +// +// When nothing in the declared collection carries the title, the resolver +// probes the workspace so it can answer `wrong_collection` instead of +// not_found. That probe took ONE arbitrary row and visibility-filtered it — so +// if the row it happened to pick was hidden, the caller got not_found even +// though a DIFFERENT live match they can see would have earned the specific +// reason. A hidden match changed the answer again. +func TestRelationTitleDoors_ProbeFindsAVisibleMatchNotAnArbitraryOne(t *testing.T) { + f := newDoorFixture(t) + + // Two collections, NEITHER of them the relation's declared target, each + // holding an item with the same title. The caller can see one. + collOne := mustSchemaCollection(t, f.srv, f.ws.ID, "Vault One", `{"fields":[]}`) + collTwo := mustSchemaCollection(t, f.srv, f.ws.ID, "Vault Two", `{"fields":[]}`) + itemOne, err := f.srv.store.CreateItem(f.ws.ID, collOne.ID, models.ItemCreate{Title: "Split Title", CreatedBy: f.owner.ID}) + if err != nil { + t.Fatalf("CreateItem(one): %v", err) + } + itemTwo, err := f.srv.store.CreateItem(f.ws.ID, collTwo.ID, models.ItemCreate{Title: "Split Title", CreatedBy: f.owner.ID}) + if err != nil { + t.Fatalf("CreateItem(two): %v", err) + } + + // DETERMINISM. The probe walks by `id`, and ids are random UUIDs, so which + // row it reaches first is chance — a test that hid an arbitrary one would + // pass roughly half the time against the BROKEN code and look fine. Hide + // the collection holding the LEXICALLY SMALLER id, so the first row the + // probe sees is always the hidden one and the leg always exercises the + // case it names. + hiddenColl, openColl := collOne, collTwo + if itemTwo.ID < itemOne.ID { + hiddenColl, openColl = collTwo, collOne + } + + member := mustUser(t, f.srv, "probe-split@example.com", "probesplit", "") + if err := f.srv.store.AddWorkspaceMember(f.ws.ID, member.ID, "editor"); err != nil { + t.Fatalf("AddWorkspaceMember: %v", err) + } + // Can see the collection under test and openColl; NOT hiddenColl, whose + // item the probe reaches first. + if err := f.srv.store.SetMemberCollectionAccess(f.ws.ID, member.ID, "specific", + []string{f.tasks.ID, openColl.ID}); err != nil { + t.Fatalf("SetMemberCollectionAccess: %v", err) + } + + rr := f.createByTitle(member, "Split Title", "Probing") + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", rr.Code, rr.Body.String()) + } + body := rr.Body.String() + if !strings.Contains(body, "is not an item in collection") { + t.Errorf("the caller CAN see the match in %q, so they are owed the specific reason; the probe reaches %q first and must keep walking rather than filtering one arbitrary row: %s", openColl.Name, hiddenColl.Name, body) + } +} diff --git a/internal/server/relation_titles_import_test.go b/internal/server/relation_titles_import_test.go new file mode 100644 index 000000000..a24aa5709 --- /dev/null +++ b/internal/server/relation_titles_import_test.go @@ -0,0 +1,145 @@ +package server + +import ( + "encoding/json" + "fmt" + "net/http" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/artifact" + "github.com/PerpetualSoftware/pad/internal/models" +) + +// TestArtifactImport_DropsFieldKeysTheKindDoesNotDeclare pins the assumption +// that makes the artifact door's relation-visibility question MOOT +// (PLAN-2857 U6, codex round 6 P1). +// +// Round 6 raised a real-sounding P1: handleImportArtifact uses the +// `relationsCarry` posture, so a carried relation title would be judged by the +// IMPORTER's visibility and two people importing one artifact would store +// different bytes. The lead ruled that import KEEPS visibility — an import has +// no prior truth, and exempting it would let a crafted title resolve to a UUID +// the importer cannot see but can then read straight back off their own item. +// +// The reasoning is sound and the path does not exist. An artifact's frontmatter +// carries a FIXED ALLOW-LIST of field keys per kind, and anything else is +// dropped at DECODE, before any relation code runs — so no relation value can +// ride in an artifact, whatever the destination collection's schema has been +// reshaped to. +// +// I wrote the ruled test first and it passed while asserting nothing: the field +// never arrived, so the leg could not tell the visibility rule from its own +// absence, and a mutation removing the predicate entirely SURVIVED it. +// +// WHAT THIS TEST DOES AND DOES NOT ESTABLISH, because the same trap is easy to +// fall into twice. It asserts the OBSERVABLE invariant end to end: a +// frontmatter key the kind does not declare never reaches the stored blob, even +// when the destination schema declares it as a relation and the artifact bytes +// really carry it (they are hand-built below, because artifact.Encode filters +// on the way OUT and an Encode-built fixture never puts the key on the wire). +// The control asserts declared keys DO land, so it is not passing by storing +// nothing. +// +// It does NOT identify which layer drops the key, and it is not a guard on the +// decode allow-list specifically: widening playbookFieldKeys does not make this +// test fail, so at least one further filter sits behind it that I did not +// isolate. The claim is "no relation value reaches the import door", verified; +// not "here is the single mechanism that stops it". +func TestArtifactImport_DropsFieldKeysTheKindDoesNotDeclare(t *testing.T) { + srv := testServer(t) + wsSlug := createWSForTest(t, srv) + ws, err := srv.store.GetWorkspaceBySlug(wsSlug) + if err != nil || ws == nil { + t.Fatalf("GetWorkspaceBySlug(%s): %v", wsSlug, err) + } + + // Reshape the destination so the schema WOULD accept the key. If the + // artifact format ever stops filtering, this is what lets the value land. + secrets := mustSchemaCollection(t, srv, ws.ID, "Secrets", `{"fields":[]}`) + if _, err := srv.store.CreateItem(ws.ID, secrets.ID, models.ItemCreate{Title: "Hidden Owner"}); err != nil { + t.Fatalf("CreateItem(target): %v", err) + } + colls, err := srv.store.ListCollections(ws.ID) + if err != nil { + t.Fatalf("ListCollections: %v", err) + } + var playbooks *models.Collection + for i := range colls { + if colls[i].Slug == "playbooks" { + playbooks = &colls[i] + } + } + if playbooks == nil { + t.Fatalf("no playbooks collection in the seeded workspace (have %d)", len(colls)) + } + var schema map[string]any + if err := json.Unmarshal([]byte(playbooks.Schema), &schema); err != nil { + t.Fatalf("decode playbooks schema: %v", err) + } + fields, _ := schema["fields"].([]any) + schema["fields"] = append(fields, map[string]any{ + "key": "owner_ref", "label": "Owner", "type": "relation", "collection": secrets.Slug, + }) + reshaped, err := json.Marshal(schema) + if err != nil { + t.Fatalf("encode schema: %v", err) + } + reshapedStr := string(reshaped) + if _, err := srv.store.UpdateCollection(playbooks.ID, models.CollectionUpdate{Schema: &reshapedStr}); err != nil { + t.Fatalf("UpdateCollection: %v", err) + } + + // The reshape must actually have taken, or the assertion below passes + // because the field is undeclared rather than because the door filters it. + reread, rerr := srv.store.GetCollection(playbooks.ID) + if rerr != nil { + t.Fatalf("re-read playbooks: %v", rerr) + } + if !strings.Contains(reread.Schema, "owner_ref") { + t.Fatalf("the schema reshape did not take, so this test would prove nothing: %s", reread.Schema) + } + + // HAND-BUILT BYTES, not artifact.Encode. Encode writes only the kind's + // known keys into frontmatter, so an artifact built through it never + // carries the extra key and the decode-side filter this test is about is + // never exercised — my first version did exactly that and a mutation + // widening the decode allow-list SURVIVED it. + data := []byte("---\n" + + "pad_artifact: playbook\n" + + fmt.Sprintf("format_version: %v\n", artifact.FormatVersion) + + "title: Artifact With An Undeclared Key\n" + + "status: active\n" + + "trigger: manual\n" + + "scope: all\n" + + "owner_ref: Hidden Owner\n" + + "---\n\nbody\n") + + rr := doArtifactRequest(srv, "POST", "/api/v1/workspaces/"+wsSlug+"/import-artifact", data) + if rr.Code != http.StatusCreated { + t.Fatalf("import: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var envelope struct { + Slug string `json:"slug"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &envelope); err != nil { + t.Fatalf("decode: %v\nbody: %s", err, rr.Body.String()) + } + stored, err := srv.store.GetItemBySlug(ws.ID, envelope.Slug) + if err != nil || stored == nil { + t.Fatalf("GetItemBySlug(%s): %v", envelope.Slug, err) + } + var blob map[string]any + if err := json.Unmarshal([]byte(stored.Fields), &blob); err != nil { + t.Fatalf("decode fields: %v", err) + } + + if v, present := blob["owner_ref"]; present { + t.Errorf("the artifact door carried %q = %#v into a RELATION field. The frontmatter allow-list used to drop it before any relation code ran, which is why PLAN-2857 U6's visibility rule was never implemented for this door. It is now reachable and the lead's ruling needs applying there", "owner_ref", v) + } + // CONTROL: the DECLARED keys did land, so the assertion above is about the + // allow-list filtering rather than about the import storing nothing. + if blob["trigger"] != "manual" || blob["scope"] != "all" { + t.Errorf("declared frontmatter keys did not land (%#v) — the leg above proves nothing without this", blob) + } +} diff --git a/internal/store/items_cross_workspace_copy.go b/internal/store/items_cross_workspace_copy.go index 737255d24..d88cc75ea 100644 --- a/internal/store/items_cross_workspace_copy.go +++ b/internal/store/items_cross_workspace_copy.go @@ -1143,7 +1143,7 @@ func (s *Store) migrateCopyFields(q Queryer, destWorkspaceID, sourceFieldsJSON, if scope == items.CrossWorkspace { mode = RelationCarryCrossWorkspace } - relRefusals, relDropped, relErr := s.MigrateRelationReferentsQ(q, destWorkspaceID, + relRefusals, relDropped, relErr := s.MigrateRelationReferentsQ(q, canSee, destWorkspaceID, items.SchemaForMigratedFields(targetSchema), migrated.Fields, overrides, CarriedSourceValues(currentFields, migrated.Dropped), mode) if relErr != nil { @@ -1179,7 +1179,7 @@ func (s *Store) migrateCopyFields(q Queryer, destWorkspaceID, sourceFieldsJSON, } // Relation defaults ValidateFields just injected, which the pass above // could not have seen (codex round 2). - lateDropped, lateErr := s.ResolveLateRelationDefaultsQ(q, destWorkspaceID, + lateDropped, lateErr := s.ResolveLateRelationDefaultsQ(q, canSee, destWorkspaceID, items.SchemaForMigratedFields(targetSchema), migrated.Fields, relBefore) if lateErr != nil { return nil, nil, fmt.Errorf("copy item across workspaces: resolve relation defaults: %w", lateErr) diff --git a/internal/store/relation_referents.go b/internal/store/relation_referents.go index 423611de5..97ed34f07 100644 --- a/internal/store/relation_referents.go +++ b/internal/store/relation_referents.go @@ -2,7 +2,9 @@ package store import ( "database/sql" + "encoding/json" "fmt" + "sort" "strings" "github.com/PerpetualSoftware/pad/internal/models" @@ -48,6 +50,14 @@ const ( // so nothing can be checked against it. A schema problem, surfaced rather // than treated as permission to store anything. RelationTargetMissing RelationIssueReason = "target_missing" + // RelationTargetAmbiguous — the value is an exact TITLE matching more than + // one live item in the target collection, so it names no single referent + // (PLAN-2857 U6). Distinct from `not_found` deliberately: telling a caller + // their title matched nothing, when it matched twice, sends them to write + // the item that already exists. Titles are not unique in a collection and + // nothing in this codebase makes them so, which is why this reason exists + // rather than the lookup taking the first row. + RelationTargetAmbiguous RelationIssueReason = "ambiguous" // RelationTargetInvalidShape — the value is not a string at all, so it // cannot name anything. Normally `ValidateFields` catches this and the // resolver deliberately stays out of it (one error per defect), but an @@ -66,6 +76,24 @@ type RelationIssue struct { Value string Target string Reason RelationIssueReason + + // VisibilityChecked is set when the RESOLVER already judged this issue + // against a requester — i.e. it was given a RelationVisibilityFunc. + // + // Only the TITLE path sets it, and only because the server's collapse pass + // must then leave it alone. That pass re-resolves the value through + // `ResolveRelationTarget`, which speaks a UUID-or-ref ladder and cannot + // resolve a title: it would find nothing, take the vanished-target arm, and + // collapse a `wrong_collection` the caller is entitled to see. Removing an + // earlier marker field reintroduced exactly that, and the VISIBLE leg of + // the door test caught it — the invisible leg passes either way. + // + // The ref path deliberately keeps its own collapse. Retrofitting it is a + // change to working code and belongs to BUG-3012, not here. + // + // Internal only: this type has no JSON tags and reaches callers through + // Message(). + VisibilityChecked bool } // Message renders the issue the way every door reports it. One function so the @@ -80,6 +108,12 @@ func (ri RelationIssue) Message() string { // earlier wording named only the first and misdiagnosed the second as // a schema that says nothing when it says something broken. return fmt.Sprintf("field %q has no valid target collection, so %q cannot be resolved", ri.Key, ri.Value) + case RelationTargetAmbiguous: + // Names the collection searched, like wrong_collection does, because + // the caller's next move is either to disambiguate by ref or to look + // at the duplicates — and both need to know where the search ran. + return fmt.Sprintf("field %q: %q matches more than one item in collection %q; use a ref (e.g. PREFIX-3) to say which", + ri.Key, ri.Value, ri.Target) case RelationTargetInvalidShape: return fmt.Sprintf("field %q has a default that is not a reference", ri.Key) default: @@ -102,6 +136,7 @@ func RelationIssueReasons() []RelationIssueReason { RelationTargetMissing, RelationTargetNotPortable, RelationTargetInvalidShape, + RelationTargetAmbiguous, } } @@ -135,7 +170,11 @@ func RelationIssuesMessage(issues []RelationIssue) string { // targets are out of PLAN-2857 v1. // - Empty and absent values are not issues. Clearing a relation is a // legitimate write, and a required-field check is `ValidateFields`'s job. -// - Exact-title resolution is U6. A ref or a UUID resolve; nothing else does. +// - A UUID, a ref, or an EXACT TITLE resolve; nothing else does. The title +// rung is U6 (PLAN-2857) and is scoped to the field's declared collection, +// unlike `ResolveItem`'s workspace-wide ladder. This line used to say +// "exact-title resolution is U6. A ref or a UUID resolve; nothing else +// does" — it was U1's scope note and U6 falsified it. // - Soft-deleted targets do not resolve (`ResolveItem` and `GetItem` exclude // them), so writing a reference to a deleted item is refused while an // ALREADY-STORED one still renders honestly on read — the read half U2 @@ -144,8 +183,9 @@ func (s *Store) ResolveRelationReferents( workspaceID string, schema models.CollectionSchema, fieldMap map[string]any, + canSee RelationVisibilityFunc, ) ([]RelationIssue, error) { - return s.ResolveRelationReferentsQ(s.Q(), workspaceID, schema, fieldMap) + return s.ResolveRelationReferentsQ(s.Q(), workspaceID, schema, fieldMap, canSee) } // ResolveRelationReferentsQ is ResolveRelationReferents parameterised over its @@ -163,6 +203,7 @@ func (s *Store) ResolveRelationReferentsQ( workspaceID string, schema models.CollectionSchema, fieldMap map[string]any, + canSee RelationVisibilityFunc, ) ([]RelationIssue, error) { var issues []RelationIssue // Cache per target slug: a schema with several relations aimed at one @@ -222,12 +263,52 @@ func (s *Store) ResolveRelationReferentsQ( return nil, err } if item == nil { - issues = append(issues, RelationIssue{ - Key: def.Key, Value: value, Target: def.Collection, Reason: RelationTargetNotFound, - }) - continue + // U6: the value is neither a UUID nor a ref, so try it as an exact + // TITLE — scoped to the declared collection, which is the whole + // distinction from ResolveItem's workspace-wide ladder (R11). + titled, outside, ambiguous, terr := s.resolveRelationTitleQ(q, workspaceID, targetID, value, canSee) + if terr != nil { + return nil, terr + } + switch { + case ambiguous: + issues = append(issues, RelationIssue{ + Key: def.Key, Value: value, Target: def.Collection, + Reason: RelationTargetAmbiguous, + }) + continue + case titled != nil: + // Resolved by title. Falls through to the canonicalisation + // below by taking the same shape a ref would have. + item = titled + case outside != nil: + // The title names a live item the requester CAN see, just not + // one in this collection — the probe above already applied + // visibility, so an invisible match never reaches here. + // VisibilityChecked tells the server's collapse to leave this + // alone: that pass re-resolves through a UUID-or-ref ladder + // which cannot speak titles, so it would find nothing and + // collapse a reason the caller is entitled to see. + issues = append(issues, RelationIssue{ + Key: def.Key, Value: value, Target: def.Collection, + Reason: RelationTargetWrongCollection, VisibilityChecked: canSee != nil, + }) + continue + default: + issues = append(issues, RelationIssue{ + Key: def.Key, Value: value, Target: def.Collection, Reason: RelationTargetNotFound, + }) + continue + } } if item.CollectionID != targetID { + // VisibilityChecked is deliberately NOT set here. A ref-derived + // issue is re-resolvable by the server through the same ladder that + // produced it, so it keeps its own collapse — judging it here would + // silently change the ref path's behaviour (no second lookup, no + // TOCTOU window) inside a unit whose scope says not to touch it. + // That window is real and is filed as BUG-3012 rather than fixed in + // passing. issues = append(issues, RelationIssue{ Key: def.Key, Value: value, Target: def.Collection, Reason: RelationTargetWrongCollection, }) @@ -324,6 +405,304 @@ func (s *Store) resolveRelationTargetQ(q Queryer, workspaceID, value string) (*m return item, nil } +// resolveRelationTitleQ resolves an exact TITLE to an item, scoped to the +// declared target collection (PLAN-2857 U6, lead ruling day 55). +// +// It returns at most one of: +// +// - item — exactly one live item in `targetCollectionID` carries this +// title. This is the only case that resolves. +// - outside — nothing in the target collection carries it, but a live item +// elsewhere in the workspace does. Returned so the caller can +// refuse with `wrong_collection` AND record which item it was; +// it never resolves. +// - ambiguous — two or more live items in the TARGET collection carry it, so +// the title names no single referent. +// +// All three empty means nothing in the workspace carries the title. +// +// WHY THE SCOPE IS THE WHOLE POINT. `ResolveItem`'s ladder is workspace-wide +// and ends in a SLUG, so `--field color=red` would resolve a `red` item in any +// collection — R11 in the design pass, and the reason this is new code instead +// of a reuse. The workspace-wide query below exists ONLY to turn "not here" +// into a better refusal; it is never allowed to produce a resolution. +// +// EXACTNESS, and precisely what is exact about it. `=` on the STORED title, +// case-sensitive on both engines, with no folding and no trimming of the stored +// side: the alternative invents a matching rule nobody ratified and makes two +// visibly different titles collide. +// +// The SUPPLIED value is trimmed before it gets here, by the same TrimSpace the +// whole resolver applies to every relation value (a relation names a reference, +// not content, and a whitespace-only one has meant "no reference" since U1). So +// `" Red "` does match a stored `"Red"`, while a stored `" Red "` is reachable +// only by a value that trims to it. Worth stating, because "exact" on its own +// implies neither. +// +// The ORDER of the ladder above this matters too: UUID, then ref, then title. +// An item literally titled "TASK-5" is therefore unreachable by title while a +// ref TASK-5 resolves. That is the correct precedence — refs are the canonical +// form — but it is a real edge, so it is written down here rather than found. +func (s *Store) resolveRelationTitleQ( + q Queryer, + workspaceID, targetCollectionID, title string, + canSee RelationVisibilityFunc, +) (item *models.Item, outside *models.Item, ambiguous bool, err error) { + // THE COUNT THAT DECIDES IS THE VISIBLE COUNT (lead ruling, PLAN-2857 U6). + // Among live exact-title matches in the target collection, only ones the + // requester can SEE count: zero is not_found, exactly one resolves, two or + // more is ambiguous. A hidden match never changes the answer. + // + // canSee == nil means there is NO requester — import, and any migrate that + // runs outside a request — and then every live match counts, which is right + // because nobody is being told anything. + // + // This lives HERE rather than as a second pass in the server layer, and the + // difference is not tidiness. The cross-workspace copy runs the resolver + // inside its own transaction, where no server-layer pass can reach it, so a + // server-side re-decision left the copy door leaking and made preflight and + // copy disagree. `RelationVisibilityFunc` already existed for exactly this + // shape, and resolveRelationsForWrite's own comment named the gap: + // "Unifying the two families needs a caller-supplied visibility predicate + // on the store API." + // + // TWO COSTS, stated because they are real, and stated more carefully than + // the first version of this comment (codex rounds 4 and 5 — round 5 was + // right that both claims were overstated): + // + // The walk loads each candidate and asks canSee about it, so it is O(live + // items sharing this exact title) queries — in this collection here, and + // ANYWHERE IN THE WORKSPACE for the wrong_collection probe below, which is + // the larger set. Normally that set is ONE. It grows when many items share + // a title, and the walk stops at the SECOND VISIBLE match, so the shape + // that actually scans is many HIDDEN same-titled items. + // + // An earlier version of this comment claimed that shape requires write + // access to a collection the walker cannot read. That is FALSE: an + // item-level grant lets a caller write the destination while seeing only + // selected items in the target collection, which the fixtures in + // relation_titles_doors_test.go build directly. The honest bound is the + // page size on memory and early termination on the common cases, not an + // argument that the bad case is unreachable. + // + // The cursor is not snapshot-stable. Rows are ordered by `id` and the + // cursor only advances, so an UNCHANGED, still-matching row that existed + // when the walk started is visited. That is weaker than "every row that + // existed is visited", which is what this used to say: each page is its own + // snapshot, so a row deleted or retitled before its page is reached is + // skipped, and a later insert may be observed. None of that is a defect — + // it is inherent to any non-snapshot read, equally true of the LIMIT 2 + // count this replaced, and a row that changed mid-request is not one the + // answer owed anything to. + // + // PAGED ON `id`, not item_number: item_number is NULLABLE (migration 006 + // adds it with no constraint and nothing since makes it NOT NULL), and + // `item_number > ?` silently excludes every NULL row — so a legacy + // visible match could be skipped while the unpaged count still saw it, and + // the two disagreed. `id` is the primary key and cannot be null; the order + // is arbitrary but STABLE, which is all a cursor needs. Nothing observable + // depends on WHICH order, because the only case that resolves has exactly + // one visible match. + seen := 0 + var only *models.Item + anyMatch := false + cursor := "" + for { + ids, cerr := s.relationTitleIDPageQ(q, workspaceID, targetCollectionID, title, cursor, relationTitlePageSize) + if cerr != nil { + return nil, nil, false, cerr + } + if len(ids) == 0 { + break + } + anyMatch = true + for _, id := range ids { + cursor = id + if canSee == nil { + // Load BEFORE counting. Counting first meant a row deleted + // between the id page and this fetch still counted, so one + // remaining live match could answer `ambiguous` (codex round + // 5). A vanished row is not a match. + found, gerr := s.GetItemQ(q, id) + if gerr != nil { + return nil, nil, false, gerr + } + if found == nil { + continue + } + seen++ + if seen > 1 { + return nil, nil, true, nil + } + only = found + continue + } + found, gerr := s.GetItemQ(q, id) + if gerr != nil { + return nil, nil, false, gerr + } + if found == nil { + // Vanished between the id read and this one; not a match. + continue + } + ok, verr := canSee(q, workspaceID, found) + if verr != nil { + return nil, nil, false, verr + } + if !ok { + continue + } + seen++ + if seen > 1 { + // Nothing later can change this, so the rest is never read. + return nil, nil, true, nil + } + only = found + } + if len(ids) < relationTitlePageSize { + break + } + } + if seen == 1 { + // `only` can be nil if the row vanished between the two reads; the + // caller then refuses, which is the honest answer. + return only, nil, false, nil + } + if anyMatch { + // The title DOES match inside the declared collection — the requester + // just cannot see any of them. Answer not_found and do NOT fall + // through to the workspace-wide probe: that probe has no collection + // filter, so it would rediscover one of these very items and report + // `wrong_collection`, which is both untrue (it is in the right + // collection) and the disclosure this rule exists to prevent. + return nil, nil, false, nil + } + + // Nothing in the target collection. One workspace-wide probe, for the + // refusal only. + // + // It walks for a VISIBLE match rather than taking an arbitrary one and + // filtering it. `LIMIT 1` plus a filter was the first shape and it broke + // the property in the other direction: if the row it happened to pick was + // hidden the caller got not_found, while a DIFFERENT live match they can + // see would have earned `wrong_collection` — so a hidden match changed the + // answer again (codex round 4). + other, perr := s.firstVisibleTitleMatchQ(q, workspaceID, title, canSee) + if perr != nil { + return nil, nil, false, perr + } + // A `wrong_collection` refusal names a LIVE item, so raising it for a + // target the requester cannot see is the existence oracle the server + // collapse closes for refs. Answering not_found here means the collapse has + // nothing left to do on the title path. + return nil, other, false, nil +} + +// firstVisibleTitleMatchQ returns the first live item anywhere in the workspace +// with this exact title that the requester may SEE, or nil. +// +// canSee == nil means no requester, and then the first live match is the +// answer — nobody is being told anything. +func (s *Store) firstVisibleTitleMatchQ( + q Queryer, + workspaceID, title string, + canSee RelationVisibilityFunc, +) (*models.Item, error) { + cursor := "" + for { + rows, err := q.Query(s.q(` + SELECT id FROM items + WHERE workspace_id = ? AND title = ? AND deleted_at IS NULL AND id > ? + ORDER BY id + LIMIT ? + `), workspaceID, title, cursor, relationTitlePageSize) + if err != nil { + return nil, fmt.Errorf("relation title probe: %w", err) + } + var ids []string + for rows.Next() { + var id string + if scanErr := rows.Scan(&id); scanErr != nil { + rows.Close() + return nil, fmt.Errorf("relation title probe scan: %w", scanErr) + } + ids = append(ids, id) + } + if rowsErr := rows.Err(); rowsErr != nil { + rows.Close() + return nil, fmt.Errorf("relation title probe rows: %w", rowsErr) + } + rows.Close() + if len(ids) == 0 { + return nil, nil + } + for _, id := range ids { + cursor = id + item, gerr := s.GetItemQ(q, id) + if gerr != nil { + return nil, gerr + } + if item == nil { + continue + } + if canSee == nil { + return item, nil + } + ok, verr := canSee(q, workspaceID, item) + if verr != nil { + return nil, verr + } + if ok { + return item, nil + } + } + if len(ids) < relationTitlePageSize { + return nil, nil + } + } +} + +// relationTitlePageSize bounds MEMORY for the walk above, not the answer: the +// loop stops at the second VISIBLE match or at the end of the rows, so no match +// count changes what a caller is told. A cap on the ANSWER was an earlier shape +// and it was an oracle in itself — "one visible plus cap hidden" read +// differently from "one visible plus cap+1 hidden". +const relationTitlePageSize = 100 + +// relationTitleIDPageQ returns one page of ids of live items with this exact +// title in this collection, ordered by id, starting after `afterID`. +// +// Ids only: the caller decides what it may look at before loading anything. +func (s *Store) relationTitleIDPageQ( + q Queryer, + workspaceID, collectionID, title, afterID string, + limit int, +) ([]string, error) { + rows, err := q.Query(s.q(` + SELECT id FROM items + WHERE workspace_id = ? AND collection_id = ? AND title = ? + AND deleted_at IS NULL AND id > ? + ORDER BY id + LIMIT ? + `), workspaceID, collectionID, title, afterID, limit) + if err != nil { + return nil, fmt.Errorf("relation title page: %w", err) + } + defer rows.Close() + var ids []string + for rows.Next() { + var id string + if scanErr := rows.Scan(&id); scanErr != nil { + return nil, fmt.Errorf("relation title page scan: %w", scanErr) + } + ids = append(ids, id) + } + if rowsErr := rows.Err(); rowsErr != nil { + return nil, fmt.Errorf("relation title page rows: %w", rowsErr) + } + return ids, nil +} + // collectionIDBySlugQ returns the collection's ID, or "" when the workspace // has no live collection with that slug. ID only: the referent check compares // `item.CollectionID`, and loading the whole model would pull in per-collection @@ -374,6 +753,14 @@ func (s *Store) itemByRefQ(q Queryer, workspaceID, prefix string, number int) (* return s.GetItemQ(q, id) } +// relationWant is one stored relation value plus the collection slug its field +// DECLARES, carried together so hydration can check the second against the +// first without re-walking the schemas. +type relationWant struct { + value string + targetSlug string +} + // RelationTargetNotPortable — a carried relation value that cannot cross a // workspace boundary. Not a defect in the value: it names a live item in the // SOURCE workspace, and PLAN-2857 v1 excludes cross-workspace relation @@ -476,6 +863,7 @@ func (o RelationOrigin) Refuses() bool { // Mutates fieldMap: dropped keys are deleted, and values that survive are // canonicalised to their target's ID. func (s *Store) MigrateRelationReferents( + canSee RelationVisibilityFunc, workspaceID string, schema models.CollectionSchema, fieldMap map[string]any, @@ -483,13 +871,14 @@ func (s *Store) MigrateRelationReferents( sourceFields map[string]any, mode RelationCarryMode, ) (refusals []RelationIssue, dropped []RelationIssue, err error) { - return s.MigrateRelationReferentsQ(s.Q(), workspaceID, schema, fieldMap, supplied, sourceFields, mode) + return s.MigrateRelationReferentsQ(s.Q(), canSee, workspaceID, schema, fieldMap, supplied, sourceFields, mode) } // MigrateRelationReferentsQ is MigrateRelationReferents on a caller-supplied // read executor — see ResolveRelationReferentsQ for why the copy door needs it. func (s *Store) MigrateRelationReferentsQ( q Queryer, + canSee RelationVisibilityFunc, workspaceID string, schema models.CollectionSchema, fieldMap map[string]any, @@ -530,7 +919,7 @@ func (s *Store) MigrateRelationReferentsQ( // Supplied values are ordinary writes. if suppliedRelations := byOrigin[RelationOriginSupplied]; len(suppliedRelations) > 0 { - issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, suppliedRelations) + issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, suppliedRelations, canSee) if resolveErr != nil { return nil, nil, resolveErr } @@ -608,7 +997,7 @@ func (s *Store) MigrateRelationReferentsQ( delete(fieldMap, def.Key) delete(defaults, def.Key) } - issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, defaults) + issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, defaults, canSee) if resolveErr != nil { return nil, nil, resolveErr } @@ -729,7 +1118,21 @@ func (s *Store) MigrateRelationReferentsQ( // below, which exists to keep dangling referents out of the blob. So // this comment is also a warning: if you ever change that rule, you // are changing this too, in the other direction. - issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, carried) + // nil, NOT canSee, and the argument is the twenty lines above this + // one. A carried value was asserted by nobody, so no caller is probing + // with it; and judging it by the MOVER's visibility is precisely what + // "would make the STORED BYTES depend on who performed the move". + // + // U6 made that reachable for the first time. Before it a carried value + // that happened to match a title never resolved at all, so there was + // nothing for visibility to change. Passing the predicate here — which + // I did, against this file's own standing argument — meant a carried + // title resolved for one mover and was DROPPED for another, and the + // item's stored fields depended on who touched it (codex round 5). + // + // The disclosure the predicate exists to prevent needs a caller who + // TYPED the title. Supplied values and injected defaults still get it. + issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, carried, nil) if resolveErr != nil { return nil, nil, resolveErr } @@ -789,18 +1192,20 @@ func RelationKeysPresent(schema models.CollectionSchema, fieldMap map[string]any // MigrateRelationReferents gives RelationOriginDestinationDefault, which is // what this is: the same origin, arriving late. func (s *Store) ResolveLateRelationDefaults( + canSee RelationVisibilityFunc, workspaceID string, schema models.CollectionSchema, fieldMap map[string]any, before map[string]bool, ) (dropped []RelationIssue, err error) { - return s.ResolveLateRelationDefaultsQ(s.Q(), workspaceID, schema, fieldMap, before) + return s.ResolveLateRelationDefaultsQ(s.Q(), canSee, workspaceID, schema, fieldMap, before) } // ResolveLateRelationDefaultsQ is ResolveLateRelationDefaults on a // caller-supplied read executor — the copy door runs inside a transaction. func (s *Store) ResolveLateRelationDefaultsQ( q Queryer, + canSee RelationVisibilityFunc, workspaceID string, schema models.CollectionSchema, fieldMap map[string]any, @@ -849,7 +1254,7 @@ func (s *Store) ResolveLateRelationDefaultsQ( // silently discarded them. return dropped, nil } - issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, late) + issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, late, canSee) if resolveErr != nil { return nil, resolveErr } @@ -1034,3 +1439,182 @@ func (s *Store) DropInvisibleRelationDefaultsQ( } return dropped, nil } + +// HydrateRelationTargetsQ resolves the stored `relation` values on a batch of +// items into {id, ref, title} entries (PLAN-2857 U6). +// +// BATCHED ON PURPOSE. A list read of 50 items with two relation fields each is +// 100 values; resolving them one at a time is 100 queries behind one page. This +// collects every distinct id across every item first and asks once. +// +// It answers only what the ITEMS say. Visibility is not its business — the same +// division the resolver documents at the top of this file — so a caller that +// serves a request filters the result before sending it. What it does do is +// leave an unresolvable value as an ID-ONLY entry rather than dropping it, +// because dropping it says "no relation here" and that is false. +// +// `schemas` maps a collection ID to that collection's schema, since a batch can +// span collections; an item whose collection is absent from the map is skipped +// rather than guessed at. +// +// SCOPED TO ONE WORKSPACE, and that is load-bearing rather than tidy. A stored +// relation value is just an id in a blob: nothing stops a legacy row, or a +// value that predates U1's validation, from naming an item in ANOTHER +// workspace. Resolving it would put that item's ref and title in a response to +// someone who cannot see its workspace at all. The resolver guards the same +// hole on its UUID branch and says so; this is the read-side half. A foreign id +// falls through to the ID-only path below, which is the honest answer: the +// value is stored, and there is nothing here to tell the caller about it. +func (s *Store) HydrateRelationTargetsQ( + q Queryer, + workspaceID string, + items []models.Item, + schemas map[string]models.CollectionSchema, +) (map[string]map[string]models.RelationTarget, error) { + // item ID -> field key -> the stored value and the collection its field + // DECLARES as the target. + perItem := map[string]map[string]relationWant{} + wanted := map[string]struct{}{} + + for i := range items { + schema, ok := schemas[items[i].CollectionID] + if !ok { + continue + } + fieldMap := map[string]any{} + if items[i].Fields != "" { + if err := json.Unmarshal([]byte(items[i].Fields), &fieldMap); err != nil { + // A blob that will not parse is a different defect and other + // code reports it; hydration declines to be the second voice. + continue + } + } + for _, def := range schema.Fields { + if def.Type != "relation" { + continue + } + raw, exists := fieldMap[def.Key] + if !exists || raw == nil { + continue + } + value, isStr := raw.(string) + if !isStr { + continue + } + value = strings.TrimSpace(value) + if value == "" { + continue + } + if perItem[items[i].ID] == nil { + perItem[items[i].ID] = map[string]relationWant{} + } + perItem[items[i].ID][def.Key] = relationWant{value: value, targetSlug: def.Collection} + wanted[value] = struct{}{} + } + } + if len(wanted) == 0 { + return nil, nil + } + + ids := make([]string, 0, len(wanted)) + for id := range wanted { + ids = append(ids, id) + } + // Deterministic order so a query plan and any test on it are stable. + sort.Strings(ids) + + resolved := map[string]models.RelationTarget{} + // The collection each resolved target actually lives in, for the + // declared-collection check below. + resolvedIn := map[string]string{} + // Chunked: some drivers cap placeholders per statement, and a workspace + // with a long list read can exceed it. + const chunk = 200 + for start := 0; start < len(ids); start += chunk { + end := start + chunk + if end > len(ids) { + end = len(ids) + } + batch := ids[start:end] + placeholders := strings.TrimSuffix(strings.Repeat("?,", len(batch)), ",") + args := make([]any, 0, len(batch)) + for _, id := range batch { + args = append(args, id) + } + rows, err := q.Query(s.q(` + SELECT i.id, i.title, i.item_number, i.collection_id, c.prefix + FROM items i + JOIN collections c ON c.id = i.collection_id + WHERE i.workspace_id = ? AND i.id IN (`+placeholders+`) AND i.deleted_at IS NULL + `), append([]any{workspaceID}, args...)...) + if err != nil { + return nil, fmt.Errorf("hydrate relation targets: %w", err) + } + for rows.Next() { + var id, title, collectionID, prefix string + // NULLABLE. item_number is added by migration 006 with no + // constraint and nothing since makes it NOT NULL, so a pre-006 row + // carries NULL — and scanning NULL into an int ERRORS. This + // function is best-effort and its caller drops the whole map on + // error, so one legacy row would have silently removed + // `relation_targets` from the ENTIRE response (codex round 7). + var number sql.NullInt64 + if err := rows.Scan(&id, &title, &number, &collectionID, &prefix); err != nil { + rows.Close() + return nil, fmt.Errorf("hydrate relation targets scan: %w", err) + } + target := models.RelationTarget{ID: id, Title: title} + if number.Valid { + target.Ref = fmt.Sprintf("%s-%d", prefix, number.Int64) + } + resolvedIn[id] = collectionID + resolved[id] = target + } + if err := rows.Err(); err != nil { + rows.Close() + return nil, fmt.Errorf("hydrate relation targets rows: %w", err) + } + rows.Close() + } + + // Declared target slug -> collection id, resolved once per distinct slug. + declaredIDs := map[string]string{} + + out := map[string]map[string]models.RelationTarget{} + for itemID, fields := range perItem { + for key, want := range fields { + value := want.value + target, ok := resolved[value] + if !ok { + // Dangling: the stored value names nothing live. ID-only, so + // the read stays honest instead of erroring or pretending. + target = models.RelationTarget{ID: value} + } else if want.targetSlug != "" { + // THE VALUE MUST LIVE WHERE THE FIELD SAYS IT DOES. The write + // side refuses a relation pointing outside the declared + // collection, but legacy and corrupt rows predate that, and + // rendering one as `{ref, title}` presents a Task as though it + // were the Colour the field promised. ID-only is the honest + // shape — the same one a dangling value gets, for the same + // reason (codex round 7). + declaredID, known := declaredIDs[want.targetSlug] + if !known { + var derr error + declaredID, derr = s.collectionIDBySlugQ(q, workspaceID, want.targetSlug) + if derr != nil { + return nil, derr + } + declaredIDs[want.targetSlug] = declaredID + } + if declaredID == "" || resolvedIn[value] != declaredID { + target = models.RelationTarget{ID: value} + } + } + if out[itemID] == nil { + out[itemID] = map[string]models.RelationTarget{} + } + out[itemID][key] = target + } + } + return out, nil +} diff --git a/internal/store/relation_referents_test.go b/internal/store/relation_referents_test.go index 1fe73993e..9c3bc06c5 100644 --- a/internal/store/relation_referents_test.go +++ b/internal/store/relation_referents_test.go @@ -58,7 +58,7 @@ func TestResolveRelationReferents_AcceptsIDAndRefAndCanonicalises(t *testing.T) // leg a resolver that refused everything would pass the whole suite. for _, supplied := range []string{red.ID, red.Ref} { fields := map[string]any{"color": supplied} - issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), fields) + issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil) if err != nil { t.Fatalf("supplied %q: %v", supplied, err) } @@ -109,7 +109,7 @@ func TestResolveRelationReferents_RejectsUnresolvableValues(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { fields := map[string]any{"color": tc.value} - issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), fields) + issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -158,7 +158,7 @@ func TestResolveRelationReferents_LeavesNonWritesAlone(t *testing.T) { case "explicit null": fields["color"] = nil } - issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), fields) + issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -183,7 +183,7 @@ func TestResolveRelationReferents_SoftDeletedTargetDoesNotResolve(t *testing.T) // two halves are what make "deleted" distinguishable from "never resolved" // in the UI, so this must not quietly become not_found's twin. fields := map[string]any{"color": gone.ID} - issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), fields) + issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -201,7 +201,7 @@ func TestResolveRelationReferents_TargetCollectionProblems(t *testing.T) { // A relation field with no target cannot be checked against anything. // Surfaced rather than treated as permission to store whatever. fields := map[string]any{"color": red.ID} - issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema(""), fields) + issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema(""), fields, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -212,7 +212,7 @@ func TestResolveRelationReferents_TargetCollectionProblems(t *testing.T) { t.Run("target names no collection", func(t *testing.T) { fields := map[string]any{"color": red.ID} - issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("nonexistent"), fields) + issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("nonexistent"), fields, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -238,7 +238,7 @@ func TestResolveRelationReferents_IsDeterministicAndBatched(t *testing.T) { var first string for i := 0; i < 8; i++ { fields := map[string]any{"accent": "nope-a", "color": "nope-b"} - issues, err := s.ResolveRelationReferents(ws.ID, schema, fields) + issues, err := s.ResolveRelationReferents(ws.ID, schema, fields, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -270,7 +270,7 @@ func TestMigrateRelationReferents_SameWorkspaceKeepsWhatResolves(t *testing.T) { // relation must survive. Dropping it would lose data on every move of a // correctly-related item. fields := map[string]any{"color": red.Ref, "status": "open"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryWithinWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(nil, ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryWithinWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -291,7 +291,7 @@ func TestMigrateRelationReferents_SameWorkspaceDropsWhatDoesNot(t *testing.T) { // accepted any string for a relation all along, so an item carrying "red" // must stay MOVABLE. Dropped and reported, never refused. fields := map[string]any{"color": "red", "status": "open"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryWithinWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(nil, ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryWithinWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -319,7 +319,7 @@ func TestMigrateRelationReferents_CrossWorkspaceDropsEveryCarriedRelation(t *tes // so there is nothing in the destination it could mean. Same reason // github_pr uses, because it is the same fact about the same kind of value. fields := map[string]any{"color": red.ID, "status": "open"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryCrossWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(nil, ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryCrossWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -345,7 +345,7 @@ func TestMigrateRelationReferents_SuppliedOverrideRefusesOnEitherMode(t *testing for _, mode := range []RelationCarryMode{RelationCarryWithinWorkspace, RelationCarryCrossWorkspace} { fields := map[string]any{"color": "nope"} supplied := map[string]any{"color": "nope"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, supplied, carriedFrom(fields), mode) + refusals, dropped, err := s.MigrateRelationReferents(nil, ws.ID, u1RelationSchema("colors"), fields, supplied, carriedFrom(fields), mode) if err != nil { t.Fatalf("mode %v: %v", mode, err) } @@ -366,7 +366,7 @@ func TestMigrateRelationReferents_SuppliedOverrideRefusesOnEitherMode(t *testing // on a copy at all. fields := map[string]any{"color": red.ID} supplied := map[string]any{"color": red.ID} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, supplied, carriedFrom(fields), RelationCarryCrossWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(nil, ws.ID, u1RelationSchema("colors"), fields, supplied, carriedFrom(fields), RelationCarryCrossWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -407,12 +407,12 @@ func TestResolveRelationReferents_OverflowingRefDoesNotResolve(t *testing.T) { // Sanity: the honest ref for that item still resolves, so a failure below // is about the overflow and not about the fixture. ok := map[string]any{"color": red.Ref} - if issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), ok); err != nil || len(issues) != 0 { + if issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), ok, nil); err != nil || len(issues) != 0 { t.Fatalf("control: %v %+v", err, issues) } overflowed := map[string]any{"color": "COLO-18446744073709551617"} - issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), overflowed) + issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), overflowed, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/internal/store/relation_titles_test.go b/internal/store/relation_titles_test.go new file mode 100644 index 000000000..68b559ecf --- /dev/null +++ b/internal/store/relation_titles_test.go @@ -0,0 +1,480 @@ +package store + +import ( + "encoding/json" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// Exact-title resolution and read-side hydration for `relation` values +// (PLAN-2857 U6). +// +// U1 accepted a UUID or a ref. U6 adds a THIRD spelling — an exact title, +// scoped to the field's declared collection — and the read side that lets a +// client render any of them. The legs below are the design row's proving test +// at the store level; the door-level equivalents live beside the U1 door tests. +// +// Every negative leg here is paired with a positive one that would break if the +// resolver simply refused everything (CONVE-34). That is not decoration: the +// first draft of a "title in the wrong collection must refuse" test passes +// against a resolver with no title support at all. + +// resolveOne runs one supplied value through the resolver and returns the +// stored result plus any issues, so a leg reads as value-in / outcome-out. +func resolveOne(t *testing.T, s *Store, ws *models.Workspace, schema models.CollectionSchema, supplied string) (string, []RelationIssue) { + t.Helper() + fields := map[string]any{"color": supplied} + issues, err := s.ResolveRelationReferents(ws.ID, schema, fields, nil) + if err != nil { + t.Fatalf("resolve %q: %v", supplied, err) + } + stored, _ := fields["color"].(string) + return stored, issues +} + +// TestRelationTitle_ThreeSpellingsOfOneItemStoreTheSameID is the design row's +// proving test: the same relation written by ref, by exact title and by raw +// UUID must land on an identical stored ID. +// +// It is also the CONTROL for every refusal leg below. A resolver that refused +// every title would pass those legs and fail only this one. +func TestRelationTitle_ThreeSpellingsOfOneItemStoreTheSameID(t *testing.T) { + t.Parallel() + s := testStore(t) + ws, _, _, red := relationFixture(t, s) + schema := u1RelationSchema("colors") + + for _, supplied := range []string{red.ID, red.Ref, red.Title} { + stored, issues := resolveOne(t, s, ws, schema, supplied) + if len(issues) != 0 { + t.Fatalf("%q: unexpected issues %v", supplied, issues) + } + if stored != red.ID { + t.Errorf("%q stored %q, want the canonical id %q — three spellings of one item must not store three different values", supplied, stored, red.ID) + } + } +} + +// TestRelationTitle_ScopeIsTheDeclaredCollection is ruling (3)'s store half and +// the R11 guard: a title unique in the WORKSPACE but living outside the +// field's declared collection must refuse, not resolve. +// +// The in-collection control runs in the same test so a uniformly-refusing +// resolver cannot pass it. +func TestRelationTitle_ScopeIsTheDeclaredCollection(t *testing.T) { + t.Parallel() + s := testStore(t) + ws, _, cars, _ := relationFixture(t, s) + schema := u1RelationSchema("colors") + + // "Sedan" exists, is unique workspace-wide, and is in the WRONG collection. + sedan := createTestItem(t, s, ws.ID, cars.ID, "Sedan", "") + + stored, issues := resolveOne(t, s, ws, schema, "Sedan") + if len(issues) != 1 { + t.Fatalf("a title outside the declared collection must produce exactly one issue, got %d: %v", len(issues), issues) + } + if issues[0].Reason != RelationTargetWrongCollection { + t.Errorf("reason = %q, want %q — resolving it workspace-wide is the R11 defect this guards", issues[0].Reason, RelationTargetWrongCollection) + } + if stored == sedan.ID { + t.Error("the out-of-collection item was stored anyway") + } + + // CONTROL, same run: a title that IS in the declared collection resolves. + // Without this leg, a resolver with no title support passes the assertions + // above by refusing everything. + blue := createTestItem(t, s, ws.ID, mustCollectionID(t, s, ws.ID, "colors"), "Blue", "") + stored, issues = resolveOne(t, s, ws, schema, "Blue") + if len(issues) != 0 { + t.Fatalf("control: an in-collection title must resolve, got issues %v", issues) + } + if stored != blue.ID { + t.Errorf("control: stored %q, want %q", stored, blue.ID) + } +} + +// TestRelationTitle_AmbiguousRefusesWithItsOwnReason covers ruling (2): two +// live items in the TARGET collection sharing a title name no single one. +// +// It must NOT collapse into not_found, which would tell the caller the opposite +// of what happened — the title matched too much, not too little. +func TestRelationTitle_AmbiguousRefusesWithItsOwnReason(t *testing.T) { + t.Parallel() + s := testStore(t) + ws, colors, _, _ := relationFixture(t, s) + schema := u1RelationSchema("colors") + + createTestItem(t, s, ws.ID, colors.ID, "Teal", "") + createTestItem(t, s, ws.ID, colors.ID, "Teal", "") + + stored, issues := resolveOne(t, s, ws, schema, "Teal") + if len(issues) != 1 { + t.Fatalf("want exactly one issue, got %d: %v", len(issues), issues) + } + if issues[0].Reason != RelationTargetAmbiguous { + t.Errorf("reason = %q, want %q", issues[0].Reason, RelationTargetAmbiguous) + } + // The supplied value is left as the caller sent it, not blanked and not + // canonicalised to either twin — an ambiguous title names no single item, + // so there is nothing to canonicalise to. + if stored != "Teal" { + t.Errorf("stored = %q, want the supplied title back untouched", stored) + } + + // The vocabulary must be enumerable, since the copy preflight puts these on + // the wire and a cross-stack gate checks the client against this list. + found := false + for _, r := range RelationIssueReasons() { + if r == RelationTargetAmbiguous { + found = true + } + } + if !found { + t.Error("RelationTargetAmbiguous is missing from RelationIssueReasons(), so no consumer can enumerate it") + } +} + +// TestRelationTitle_RefWinsOverAnItemTitledLikeARef pins the ladder's order and +// the shadowing case the resolver's doc comment names: UUID, then ref, then +// title. An item literally titled "COLO-3" is unreachable BY TITLE while a ref +// COLO-3 resolves. +// +// This is deliberate — refs are the canonical spelling and the ambiguity is +// created by the title — but it is a real edge, so it is pinned rather than +// left to be rediscovered as a bug. +func TestRelationTitle_RefWinsOverAnItemTitledLikeARef(t *testing.T) { + t.Parallel() + s := testStore(t) + ws, colors, _, red := relationFixture(t, s) + schema := u1RelationSchema("colors") + + // A second colour whose TITLE is the first colour's REF. + createTestItem(t, s, ws.ID, colors.ID, red.Ref, "") + // A third with an ordinary title, for the control below. + plain := createTestItem(t, s, ws.ID, colors.ID, "Cerulean", "") + + stored, issues := resolveOne(t, s, ws, schema, red.Ref) + if len(issues) != 0 { + t.Fatalf("unexpected issues: %v", issues) + } + if stored != red.ID { + t.Errorf("stored %q, want the REF's target %q — the ref rung must win over an item merely titled like one", stored, red.ID) + } + + // CONTROL, and the reason this test needs one (codex round 1 nit): the + // assertions above supply a REF and exercise only the pre-existing ref + // rung, so they pass with exact-title resolution removed entirely and the + // impostor is decoration. The control resolves an ORDINARY title in the + // same run, so the test fails if the title rung goes away — which is what + // makes the shadowing assertion above a statement about PRECEDENCE rather + // than about a rung that might not exist. + byTitle, issues := resolveOne(t, s, ws, schema, "Cerulean") + if len(issues) != 0 { + t.Fatalf("control: an ordinary title must resolve, got %v", issues) + } + if byTitle != plain.ID { + t.Errorf("control: stored %q, want %q", byTitle, plain.ID) + } +} + +// TestRelationTitle_SoftDeletedTitleDoesNotResolve: a deleted item's title is +// not a handle. The control is the same title on a live item, so the leg +// cannot pass by refusing every title. +func TestRelationTitle_SoftDeletedTitleDoesNotResolve(t *testing.T) { + t.Parallel() + s := testStore(t) + ws, colors, _, _ := relationFixture(t, s) + schema := u1RelationSchema("colors") + + gone := createTestItem(t, s, ws.ID, colors.ID, "Ochre", "") + if err := s.DeleteItem(gone.ID); err != nil { + t.Fatalf("delete: %v", err) + } + + _, issues := resolveOne(t, s, ws, schema, "Ochre") + if len(issues) != 1 || issues[0].Reason != RelationTargetNotFound { + t.Fatalf("a soft-deleted title must be not_found, got %v", issues) + } + + // CONTROL: the same title, live again, resolves. + live := createTestItem(t, s, ws.ID, colors.ID, "Ochre", "") + stored, issues := resolveOne(t, s, ws, schema, "Ochre") + if len(issues) != 0 { + t.Fatalf("control: a live title must resolve, got %v", issues) + } + if stored != live.ID { + t.Errorf("control: stored %q, want %q", stored, live.ID) + } +} + +// TestHydrateRelationTargets_ResolvesDanglesAndRefusesForeignWorkspaces covers +// the read side: what a caller gets back for each kind of stored value. +// +// The foreign-workspace leg is the one worth the fixture cost. A stored +// relation value is just an id in a blob, and nothing stops a legacy row from +// naming an item in ANOTHER workspace; hydrating it would put that item's ref +// and title in a response to someone who cannot see its workspace at all. +func TestHydrateRelationTargets_ResolvesDanglesAndRefusesForeignWorkspaces(t *testing.T) { + t.Parallel() + s := testStore(t) + ws, colors, cars, red := relationFixture(t, s) + + // A live colour in a DIFFERENT workspace, reachable only by its raw id. + otherWS, otherColors, _, _ := relationFixture(t, s) + foreign := createTestItem(t, s, otherWS.ID, otherColors.ID, "Foreign Red", "") + _ = colors + + mk := func(value string) models.Item { + it := createTestItem(t, s, ws.ID, cars.ID, "Car "+value[:6], "") + blob, _ := json.Marshal(map[string]any{"status": "open", "color": value}) + if _, err := s.UpdateItem(it.ID, models.ItemUpdate{Fields: strPtr(string(blob))}); err != nil { + t.Fatalf("seed fields: %v", err) + } + got, err := s.GetItem(it.ID) + if err != nil { + t.Fatalf("reload: %v", err) + } + return *got + } + + resolvable := mk(red.ID) + dangling := mk("00000000-0000-4000-8000-00000000dead") + crossWS := mk(foreign.ID) + + schemas := map[string]models.CollectionSchema{cars.ID: u1RelationSchema("colors")} + out, err := s.HydrateRelationTargetsQ(s.DB(), ws.ID, []models.Item{resolvable, dangling, crossWS}, schemas) + if err != nil { + t.Fatalf("hydrate: %v", err) + } + + // Resolvable: the full triple. + got := out[resolvable.ID]["color"] + if got.ID != red.ID || got.Ref != red.Ref || got.Title != red.Title { + t.Errorf("resolvable hydrated as %+v, want {id:%s ref:%s title:%s}", got, red.ID, red.Ref, red.Title) + } + + // Dangling: ID-ONLY, and PRESENT. Omitting the key would say "this item has + // no relation", which is a different and false statement. + got = out[dangling.ID]["color"] + if _, present := out[dangling.ID]["color"]; !present { + t.Fatal("a dangling value was omitted rather than hydrated id-only") + } + if got.Ref != "" || got.Title != "" { + t.Errorf("dangling hydrated as %+v, want id-only", got) + } + + // Cross-workspace: also ID-ONLY. This is the disclosure leg — a ref or a + // title here is another workspace's data. + got = out[crossWS.ID]["color"] + if got.Ref != "" || got.Title != "" { + t.Errorf("a foreign-workspace id hydrated as %+v — that is %s's ref/title in this workspace's response", got, otherWS.Slug) + } + if got.ID != foreign.ID { + t.Errorf("cross-workspace entry lost its stored id: %+v", got) + } +} + +// mustCollectionID looks a collection up by slug, failing the test rather than +// returning an empty string that would make a later assertion fail for the +// wrong reason. +func mustCollectionID(t *testing.T, s *Store, workspaceID, slug string) string { + t.Helper() + colls, err := s.ListCollections(workspaceID) + if err != nil { + t.Fatalf("list collections: %v", err) + } + for i := range colls { + if colls[i].Slug == slug { + return colls[i].ID + } + } + t.Fatalf("no collection %q in workspace %s (have %d)", slug, workspaceID, len(colls)) + return "" +} + +// TestRelationTitle_ANullItemNumberCandidateStillCounts is codex round 3's +// third P1, and it is the reason the candidate walk pages on `id`. +// +// The first paging shape used `item_number > ?` as its cursor. item_number is +// NULLABLE — migration 006 adds it with no constraint and nothing since makes +// it NOT NULL — and `item_number > ?` excludes every NULL row, while the count +// that decided ambiguity did not. So the two disagreed about the candidate set, +// and a live legacy row could be skipped: the walk would see one match where +// there were two, and RESOLVE a title that is genuinely ambiguous. +// +// `id` is the primary key and cannot be null. +func TestRelationTitle_ANullItemNumberCandidateStillCounts(t *testing.T) { + t.Parallel() + s := testStore(t) + ws, colors, _, _ := relationFixture(t, s) + schema := u1RelationSchema("colors") + + ordinary := createTestItem(t, s, ws.ID, colors.ID, "Legacy Hue", "") + legacy := createTestItem(t, s, ws.ID, colors.ID, "Legacy Hue", "") + // A pre-006 row: live, titled, and carrying no item_number at all. + if _, err := s.db.Exec(s.q("UPDATE items SET item_number = NULL WHERE id = ?"), legacy.ID); err != nil { + t.Fatalf("null the item_number: %v", err) + } + + _, issues := resolveOne(t, s, ws, schema, "Legacy Hue") + if len(issues) != 1 || issues[0].Reason != RelationTargetAmbiguous { + t.Fatalf("two live items carry this title — one of them with a NULL item_number — so it is ambiguous, got %v", issues) + } + + // CONTROL: with the NULL-numbered row gone the same title resolves, so the + // leg above is about the NULL row counting rather than about the resolver + // refusing everything. + if err := s.DeleteItem(legacy.ID); err != nil { + t.Fatalf("delete legacy: %v", err) + } + stored, issues := resolveOne(t, s, ws, schema, "Legacy Hue") + if len(issues) != 0 { + t.Fatalf("control: one live match must resolve, got %v", issues) + } + if stored != ordinary.ID { + t.Errorf("control: stored %q, want %q", stored, ordinary.ID) + } +} + +// TestMigrateRelationTitle_CarriedValueIgnoresTheMoversVisibility is codex +// round 5's P1, and this file's own standing argument, as a test. +// +// A CARRIED value was asserted by nobody, so no caller is probing with it — +// and judging it by the MOVER's visibility is exactly what +// MigrateRelationReferentsQ's comment calls out as making "the STORED BYTES +// depend on who performed the move". U6 made that reachable for the first +// time: before it, a carried value that happened to match a title never +// resolved at all, so there was nothing for visibility to change. +// +// Both legs use a predicate that hides EVERYTHING. If the carried path +// consulted it, the relation would be dropped; it must survive identically. +func TestMigrateRelationTitle_CarriedValueIgnoresTheMoversVisibility(t *testing.T) { + t.Parallel() + s := testStore(t) + ws, _, _, red := relationFixture(t, s) + + blind := func(Queryer, string, *models.Item) (bool, error) { return false, nil } + + // The carried value is a TITLE — the U6 spelling, and the one that was + // never resolvable before this unit. + fields := map[string]any{"color": red.Title, "status": "open"} + refusals, dropped, err := s.MigrateRelationReferents(blind, ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryWithinWorkspace) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(refusals) != 0 || len(dropped) != 0 { + t.Fatalf("a carried title was judged by the mover's visibility, so the stored bytes now depend on WHO moved the item: refusals=%+v dropped=%+v", refusals, dropped) + } + if fields["color"] != red.ID { + t.Errorf("carried title resolved to %v, want the canonical id %s", fields["color"], red.ID) + } + + // PARITY: the same carried title with a permissive predicate must produce + // the identical stored value. Without this leg, a migrate that ignored the + // value entirely would pass the assertions above. + seeing := func(Queryer, string, *models.Item) (bool, error) { return true, nil } + fields2 := map[string]any{"color": red.Title, "status": "open"} + if _, _, err := s.MigrateRelationReferents(seeing, ws.ID, u1RelationSchema("colors"), fields2, nil, carriedFrom(fields2), RelationCarryWithinWorkspace); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if fields2["color"] != fields["color"] { + t.Errorf("two movers stored different values for one carried title: %v vs %v", fields["color"], fields2["color"]) + } +} + +// TestRelationTitle_SuppliedValueStillObeysVisibility is the other half, and it +// is what stops the fix above from being "visibility was switched off". +// +// A SUPPLIED title — one the caller typed — must still be judged, because that +// is the probe the whole rule exists to answer. +func TestRelationTitle_SuppliedValueStillObeysVisibility(t *testing.T) { + t.Parallel() + s := testStore(t) + ws, _, _, red := relationFixture(t, s) + + blind := func(Queryer, string, *models.Item) (bool, error) { return false, nil } + fields := map[string]any{"color": red.Title} + issues, err := s.ResolveRelationReferents(ws.ID, u1RelationSchema("colors"), fields, blind) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(issues) != 1 || issues[0].Reason != RelationTargetNotFound { + t.Fatalf("a SUPPLIED title naming an item the caller cannot see must be not_found — the carried carve-out must not reach it: %v", issues) + } +} + +// TestHydrateRelationTargets_WrongCollectionAndNullNumberStayHonest covers +// codex round 7's two read-side P2s. +// +// Both are about legacy rows: the write side has refused wrong-collection +// relation values since U1 and item_number has been populated since migration +// 006, but rows predating either still exist and the READ has to hold up. +func TestHydrateRelationTargets_WrongCollectionAndNullNumberStayHonest(t *testing.T) { + t.Parallel() + s := testStore(t) + ws, colors, cars, red := relationFixture(t, s) + + // A live item in the WRONG collection for the `color` field. + sedan := createTestItem(t, s, ws.ID, cars.ID, "Sedan", "") + // A legitimate target whose item_number is NULL, as a pre-006 row is. + legacy := createTestItem(t, s, ws.ID, colors.ID, "Legacy Blue", "") + if _, err := s.db.Exec(s.q("UPDATE items SET item_number = NULL WHERE id = ?"), legacy.ID); err != nil { + t.Fatalf("null the item_number: %v", err) + } + + mk := func(label, value string) models.Item { + it := createTestItem(t, s, ws.ID, cars.ID, label, "") + blob, _ := json.Marshal(map[string]any{"status": "open", "color": value}) + fields := string(blob) + if _, err := s.UpdateItem(it.ID, models.ItemUpdate{Fields: &fields}); err != nil { + t.Fatalf("seed %s: %v", label, err) + } + got, err := s.GetItem(it.ID) + if err != nil { + t.Fatalf("reload %s: %v", label, err) + } + return *got + } + + good := mk("Points at a colour", red.ID) + wrong := mk("Points at a car", sedan.ID) + nulled := mk("Points at a legacy colour", legacy.ID) + + schemas := map[string]models.CollectionSchema{cars.ID: u1RelationSchema("colors")} + out, err := s.HydrateRelationTargetsQ(s.DB(), ws.ID, []models.Item{good, wrong, nulled}, schemas) + if err != nil { + t.Fatalf("hydrate: %v", err) + } + + // A NULL item_number must not take the whole response down with it. This + // function is best-effort and its caller drops the entire map on error, so + // scanning NULL into an int would have removed relation_targets from every + // item in the response, not just this one. + if len(out) == 0 { + t.Fatal("hydration returned nothing at all; one legacy row with a NULL item_number erased the whole response") + } + if got := out[good.ID]["color"]; got.Ref != red.Ref || got.Title != red.Title { + t.Errorf("the ordinary target did not hydrate: %+v", got) + } + + // Wrong collection: id-only. Rendering it fully would present a Car as + // though it were the Colour the field promised. + if got := out[wrong.ID]["color"]; got.Ref != "" || got.Title != "" { + t.Errorf("a value pointing OUTSIDE the declared collection hydrated as %+v; the field says colors and this is a car", got) + } else if got.ID != sedan.ID { + t.Errorf("the wrong-collection entry lost its stored id: %+v", got) + } + + // NULL item_number: the row is a legitimate target in the right + // collection, so it hydrates — with a title and no ref, because there is + // no number to build one from. Not id-only, and not an error. + got := out[nulled.ID]["color"] + if got.Title != "Legacy Blue" { + t.Errorf("a NULL-numbered target lost its title: %+v", got) + } + if got.Ref != "" { + t.Errorf("a NULL-numbered target invented a ref: %+v", got) + } +} diff --git a/web/src/lib/items/copyDropReasons.ts b/web/src/lib/items/copyDropReasons.ts index 9c464114a..1209844f4 100644 --- a/web/src/lib/items/copyDropReasons.ts +++ b/web/src/lib/items/copyDropReasons.ts @@ -25,8 +25,9 @@ * * KEPT IN THE SAME ORDER as the Go declarations so a diff of one against the * other is readable. The first five are the migrate-level reasons in - * `handlers_items_copy_preflight.go`; the last five are the relation-level - * `RelationIssueReason` constants in `internal/store/relation_referents.go`. + * `handlers_items_copy_preflight.go`; the rest are the relation-level + * `RelationIssueReason` constants in `internal/store/relation_referents.go` + * (five at TASK-2878, plus `ambiguous` at PLAN-2857 U6). * * This list is the CONTRACT the completeness test checks the map against. It * is duplicated from Go rather than generated, so it can go stale — which is @@ -47,6 +48,7 @@ export const COPY_DROP_REASONS = [ 'wrong_collection', 'target_missing', 'invalid_shape', + 'ambiguous', ] as const; export type CopyDropReason = (typeof COPY_DROP_REASONS)[number]; @@ -81,6 +83,13 @@ const MESSAGES: Record = { target_missing: 'the field has no valid collection to link to', invalid_shape: 'the destination field’s default is not a valid reference', + + // PLAN-2857 U6. Reachable only from a TITLE — a UUID and a ref are unique + // by construction — and it says the title matched TOO MUCH, which is the + // opposite of `not_found`. The sentence names the fix the user can act on + // (be more specific) without claiming how many matched, which the response + // does not say. + ambiguous: 'more than one item in that collection has this title', }; /** diff --git a/web/src/lib/types/index.ts b/web/src/lib/types/index.ts index 0d4b21c6b..8668051b5 100644 --- a/web/src/lib/types/index.ts +++ b/web/src/lib/types/index.ts @@ -852,6 +852,15 @@ export interface ItemCopyPreflightDropped { | 'not_found' | 'wrong_collection' | 'target_missing' + /** + * An exact TITLE matched two or more live items inside the field's + * declared collection, so it names no single one (PLAN-2857 U6). Only + * the title path can produce it — a UUID and a ref are unique by + * construction. It is its own reason rather than `not_found` because + * that would tell the caller the opposite of what happened: the title + * matched too much, not too little. + */ + | 'ambiguous' /** * The destination schema's default for this field is not a reference at * all. Injected defaults are never type-checked, so this is the one