From 29bb6e2c9e01458c1049e5ade93a6cdcf3c57e64 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 11 Sep 2026 00:20:40 +0000 Subject: [PATCH] fix(relations): hydration survives a NULL item_number and honours the declared collection (PLAN-2857 U6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 7's two read-side P2s, both about legacy rows the write side has refused since U1 but which still exist. NULL ITEM_NUMBER TOOK DOWN THE WHOLE RESPONSE. The hydration query scanned items.item_number into an `int`, and that column is nullable — migration 006 adds it with no constraint and nothing since makes it NOT NULL. Scanning NULL into an int ERRORS, this function is best-effort, and its caller drops the entire map on error: so ONE pre-006 row anywhere in a page silently removed `relation_targets` from every item in the response. Scanned as sql.NullInt64 now; such a target hydrates with its title and no ref, because there is no number to build one from — not id-only, and not an error. THE DECLARED COLLECTION WAS NOT CHECKED. Any visible same-workspace id hydrated to {ref, title}, including one pointing at an item in a different collection than the field declares. That renders a Car as though it were the Colour the field promised. It is id-only now, the same shape a dangling value gets and for the same reason: the honest answer is "this points somewhere I will not dress up". Not everything round 7 raised is a defect. It called the store signature changes a P1 on the grounds that "existing callers outside this diff cannot compile". internal/store is an INTERNAL package — nothing outside this module can import it — so the compiler has already proved every caller was updated, which is exactly why a required parameter was the right shape rather than a variadic that would let a caller silently omit the predicate. instructions.md now DOCUMENTS the feature rather than only bumping its heading: the three accepted spellings and their order, collection-scoped matching, `ambiguous` versus `not_found`, that only items you can see count, and that an id-only `relation_targets` entry means gone OR hidden and must not be rendered as either. MUTATION: M20 item_number scanned as int again KILLED M21 declared-collection check removed KILLED Gates: go test ./... 0, gofmt clean, lint 0 issues. Claude-Session: https://claude.ai/code/session_01PRcuviXka4Un1FGrBrX8jc --- internal/mcp/instructions.md | 5 ++ internal/store/relation_referents.go | 69 +++++++++++++++++++----- internal/store/relation_titles_test.go | 74 ++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 13 deletions(-) diff --git a/internal/mcp/instructions.md b/internal/mcp/instructions.md index 512a9b36b..5bd67ef05 100644 --- a/internal/mcp/instructions.md +++ b/internal/mcp/instructions.md @@ -13,6 +13,11 @@ Ten resource × action tools, plus `pad_set_workspace` (which takes a `workspace 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/store/relation_referents.go b/internal/store/relation_referents.go index e9b23f445..97ed34f07 100644 --- a/internal/store/relation_referents.go +++ b/internal/store/relation_referents.go @@ -753,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 @@ -1463,8 +1471,9 @@ func (s *Store) HydrateRelationTargetsQ( items []models.Item, schemas map[string]models.CollectionSchema, ) (map[string]map[string]models.RelationTarget, error) { - // item ID -> field key -> stored value - perItem := map[string]map[string]string{} + // 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 { @@ -1497,9 +1506,9 @@ func (s *Store) HydrateRelationTargetsQ( continue } if perItem[items[i].ID] == nil { - perItem[items[i].ID] = map[string]string{} + perItem[items[i].ID] = map[string]relationWant{} } - perItem[items[i].ID][def.Key] = value + perItem[items[i].ID][def.Key] = relationWant{value: value, targetSlug: def.Collection} wanted[value] = struct{}{} } } @@ -1515,6 +1524,9 @@ func (s *Store) HydrateRelationTargetsQ( 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 @@ -1530,7 +1542,7 @@ func (s *Store) HydrateRelationTargetsQ( args = append(args, id) } rows, err := q.Query(s.q(` - SELECT i.id, i.title, i.item_number, c.prefix + 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 @@ -1539,17 +1551,24 @@ func (s *Store) HydrateRelationTargetsQ( return nil, fmt.Errorf("hydrate relation targets: %w", err) } for rows.Next() { - var id, title, prefix string - var number int - if err := rows.Scan(&id, &title, &number, &prefix); err != nil { + 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) } - resolved[id] = models.RelationTarget{ - ID: id, - Ref: fmt.Sprintf("%s-%d", prefix, number), - Title: title, + 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() @@ -1558,14 +1577,38 @@ func (s *Store) HydrateRelationTargetsQ( 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, value := range fields { + 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{} diff --git a/internal/store/relation_titles_test.go b/internal/store/relation_titles_test.go index 8c35c3c37..68b559ecf 100644 --- a/internal/store/relation_titles_test.go +++ b/internal/store/relation_titles_test.go @@ -404,3 +404,77 @@ func TestRelationTitle_SuppliedValueStillObeysVisibility(t *testing.T) { 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) + } +}