From 0bf710eea539f2937526306698b77fb2364cd0fe Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 27 Apr 2026 10:57:24 -0400 Subject: [PATCH] fix: hide item_links pointing to soft-deleted items (BUG-734) (#259) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(store): hide item_links pointing to soft-deleted items (BUG-734) Item-link queries that JOIN against `items` now also filter on `deleted_at IS NULL` for both source and target. This prevents `pad item related`, the lineage breadcrumb, and dashboard enrichment from surfacing dangling endpoints when one side has been archived. Affected queries in internal/store/items.go: - GetItemLinks (powers `pad item related`, lineage, dashboard) - GetItemLink (singular; fixed for consistency) - GetParentForItem (breadcrumb / lineage; archived parent reads as none) Other item_links queries already filtered on deleted_at; export.go deliberately keeps all rows for backup correctness — left unchanged. The link rows themselves are preserved on disk, so restoring a soft-deleted item resurrects its relationships automatically. Tests: - TestItemLinks_HidesSoftDeletedEndpoints — delete + restore round-trip on both source-side and target-side - TestGetParentForItem_HidesSoftDeletedParent — parent breadcrumb path Manually verified: PLAN + TASK with `implements` link, soft-delete the TASK, `pad item related ` correctly returns no implementers. * fix(store): address Codex review findings on PR #259 (BUG-734) Three follow-ups from Codex's review of the soft-delete filter on item-link queries: 1. MEDIUM — GetParentMap now JOINs items on both sides and filters on deleted_at IS NULL. handlers_dashboard.go uses this map directly to detect orphaned tasks (items not present in the map are flagged), so without the filter a task whose parent had been soft-deleted would silently fail to appear as orphaned. 2. LOW — Revert the deleted_at filter on getItemLink (lowercase, private). Its only caller is the post-insert readback in CreateItemLink, which means filtering buys nothing user-facing and introduces a delete-race window where a successful INSERT returns nil. SetParentLink's readback was switched from GetItemLinks to getItemLink for the same reason. User-facing surfaces still go through GetItemLinks (plural) and GetParentForItem, both of which retain the filter. 3. LOW — Add an explicit comment in export.go documenting that item_links are exported in full (including links to soft-deleted items), and why that intentionally diverges from the user-facing query behavior. Tests: TestGetParentMap_ExcludesSoftDeletedEndpoints exercises the dashboard regression path on both source-side and target-side soft-delete, plus the restore round-trip. * fix(store): reject soft-deleted parent in ListItems UUID parent filter (BUG-734) Codex review on 288283b flagged that ListItems(parent=) at items.go:534 runs an EXISTS subquery against item_links without checking whether the target parent is soft-deleted. Slug/ref input rejects deleted parents upstream via GetItem/GetItemBySlug, but raw-UUID input bypasses that path and would still return active children of an archived parent. Fix: JOIN items into the EXISTS subquery and require deleted_at IS NULL on the parent. Test: TestListItems_ParentFilter_RespectsSoftDeletedParent — covers the delete + restore round-trip on the parent. * fix(store): apply parent-filter in FTS path so search+parent enforces deleted-parent rejection (BUG-734) Codex's 3rd review pass on PR #259 caught that listItemsFTS does not re-apply ParentLinkID. Combining `parent=&search=` therefore silently dropped the parent constraint — and, by extension, the deleted-parent rejection introduced earlier in this PR. Fix: replay the same EXISTS-with-deleted_at-IS-NULL predicate in the FTS branch. Test: TestListItems_ParentFilter_FTS_RespectsSoftDeletedParent covers the delete + restore round-trip on the search path. The wider FTS filter-bypass (Tags, AssignedUserID, AgentRoleID, Fields, ParentID are all silently dropped when search is set) is pre-existing behavior outside BUG-734's scope; tracked as BUG-812. --- internal/store/export.go | 8 +- internal/store/items.go | 67 +++++--- internal/store/items_test.go | 291 +++++++++++++++++++++++++++++++++++ 3 files changed, 346 insertions(+), 20 deletions(-) diff --git a/internal/store/export.go b/internal/store/export.go index 46b9d77f..7058cfe9 100644 --- a/internal/store/export.go +++ b/internal/store/export.go @@ -97,7 +97,13 @@ func (s *Store) ExportWorkspace(slug string) (*models.WorkspaceExport, error) { return nil, err } - // Item links + // Item links — exported in full, including links whose source or target item + // is soft-deleted. This is intentional and differs from user-facing reads + // (GetItemLinks/GetParentForItem/GetParentMap, which all filter on + // items.deleted_at IS NULL — see BUG-734). Backups need to round-trip the + // raw graph so that re-importing into a workspace where the deleted items + // are restored preserves the original relationships. The import path + // already silently skips links whose endpoints are missing entirely. linkRows, err := s.db.Query(s.q(` SELECT id, source_id, target_id, link_type, created_by, created_at FROM item_links WHERE workspace_id = ? diff --git a/internal/store/items.go b/internal/store/items.go index 028de3ca..ad1d824e 100644 --- a/internal/store/items.go +++ b/internal/store/items.go @@ -529,9 +529,12 @@ func (s *Store) ListItems(workspaceID string, params models.ItemListParams) ([]m args = append(args, params.AgentRoleID, params.AgentRoleID) } - // Parent link filter via item_links + // Parent link filter via item_links. Joins items so we ignore links pointing + // to a soft-deleted parent — slug/ref filtering already rejects deleted + // parents upstream, but raw-UUID input bypasses that path. See BUG-734 / + // Codex review on PR #259. if params.ParentLinkID != "" { - query += " AND EXISTS (SELECT 1 FROM item_links il WHERE il.source_id = i.id AND il.link_type = 'parent' AND il.target_id = ?)" + query += " AND EXISTS (SELECT 1 FROM item_links il JOIN items p ON p.id = il.target_id AND p.deleted_at IS NULL WHERE il.source_id = i.id AND il.link_type = 'parent' AND il.target_id = ?)" args = append(args, params.ParentLinkID) } @@ -634,6 +637,17 @@ func (s *Store) listItemsFTS(workspaceID string, params models.ItemListParams) ( args = append(args, params.CollectionSlug) } + // Parent link filter — must mirror the non-FTS path so combining + // `parent=&search=` doesn't silently drop the parent constraint + // (and, by extension, the soft-deleted-parent rejection from BUG-734). + // See Codex review on PR #259. Note: other filter kinds (tags, assignee, + // role, fields, ParentID) are also currently ignored in the FTS path — + // pre-existing behavior outside BUG-734's scope; tracked separately. + if params.ParentLinkID != "" { + query += " AND EXISTS (SELECT 1 FROM item_links il JOIN items p ON p.id = il.target_id AND p.deleted_at IS NULL WHERE il.source_id = i.id AND il.link_type = 'parent' AND il.target_id = ?)" + args = append(args, params.ParentLinkID) + } + if len(params.CollectionIDs) > 0 && len(params.ItemIDs) > 0 { collPlaceholders := make([]string, len(params.CollectionIDs)) for i, id := range params.CollectionIDs { @@ -987,6 +1001,13 @@ func (s *Store) CreateItemLink(workspaceID string, input models.ItemLinkCreate, return s.getItemLink(id) } +// getItemLink is the unfiltered post-insert readback used by CreateItemLink to +// hydrate the freshly-inserted row with collection/source/target metadata. It +// intentionally does NOT filter on items.deleted_at IS NULL: the only caller +// is the immediate readback after INSERT, and a delete race against either +// endpoint would otherwise cause the just-successful insert to return nil +// (Codex review on PR #259). User-facing surfaces all read links via +// GetItemLinks (plural) or GetParentForItem, both of which DO filter. func (s *Store) getItemLink(id string) (*models.ItemLink, error) { var link models.ItemLink var createdAt string @@ -1040,6 +1061,12 @@ func (s *Store) getItemLink(id string) (*models.ItemLink, error) { return &link, nil } +// GetItemLinks returns links where the given item is either source or target. +// Links pointing to or from soft-deleted items are filtered out so callers (e.g. +// `pad item related`, the lineage panel, the dashboard enrichment pass) don't +// surface dangling endpoints. The link rows themselves are preserved on disk — +// restoring a soft-deleted item resurrects its relationships automatically. See +// BUG-734. func (s *Store) GetItemLinks(itemID string) ([]models.ItemLink, error) { srcStatusExpr := s.dialect.JSONExtractText("s.fields", "status") tgtStatusExpr := s.dialect.JSONExtractText("t.fields", "status") @@ -1049,8 +1076,8 @@ func (s *Store) GetItemLinks(itemID string) ([]models.ItemLink, error) { s.item_number, t.item_number, %s, %s FROM item_links l - JOIN items s ON s.id = l.source_id - JOIN items t ON t.id = l.target_id + JOIN items s ON s.id = l.source_id AND s.deleted_at IS NULL + JOIN items t ON t.id = l.target_id AND t.deleted_at IS NULL JOIN collections sc ON sc.id = s.collection_id JOIN collections tc ON tc.id = t.collection_id WHERE l.source_id = ? OR l.target_id = ? @@ -1165,17 +1192,10 @@ func (s *Store) SetParentLink(workspaceID, itemID, parentID, createdBy string) ( return nil, fmt.Errorf("commit parent link: %w", err) } - // Return the full link with enriched fields - links, err := s.GetItemLinks(itemID) - if err != nil { - return nil, err - } - for _, link := range links { - if link.ID == id { - return &link, nil - } - } - return nil, fmt.Errorf("parent link created but not found") + // Return the full link with enriched fields. Use the unfiltered readback + // helper so that a delete race against either endpoint between commit and + // readback doesn't cause the successful insert to surface as nil. + return s.getItemLink(id) } // checkParentCycle walks the ancestor chain from parentID and returns an error @@ -1213,6 +1233,8 @@ func (s *Store) ClearParentLink(itemID string) error { } // GetParentForItem returns the parent link for an item, or nil if it has no parent. +// A parent link pointing to a soft-deleted item is treated as no parent — the +// breadcrumb / lineage UI shouldn't show a deleted ancestor. See BUG-734. func (s *Store) GetParentForItem(itemID string) (*models.ItemLink, error) { sStatusExpr := s.dialect.JSONExtractText("s.fields", "status") tStatusExpr := s.dialect.JSONExtractText("t.fields", "status") @@ -1222,8 +1244,8 @@ func (s *Store) GetParentForItem(itemID string) (*models.ItemLink, error) { s.item_number, t.item_number, %s, %s FROM item_links l - JOIN items s ON s.id = l.source_id - JOIN items t ON t.id = l.target_id + JOIN items s ON s.id = l.source_id AND s.deleted_at IS NULL + JOIN items t ON t.id = l.target_id AND t.deleted_at IS NULL JOIN collections sc ON sc.id = s.collection_id JOIN collections tc ON tc.id = t.collection_id WHERE l.source_id = ? AND l.link_type IN (%s) @@ -1272,10 +1294,17 @@ func (s *Store) GetParentForItem(itemID string) (*models.ItemLink, error) { // GetParentMap returns a map of item ID -> parent item ID for all parent links // in a workspace. Used for efficient batch lookups (e.g., dashboard, list enrichment). +// +// Links whose source or target item is soft-deleted are excluded so that +// dashboard orphan-detection (handlers_dashboard.go) and similar enrichment +// passes don't treat a task whose parent has been archived as still parented. +// See BUG-734. func (s *Store) GetParentMap(workspaceID string) (map[string]string, error) { rows, err := s.db.Query(s.q(fmt.Sprintf(` - SELECT source_id, target_id FROM item_links - WHERE workspace_id = ? AND link_type IN (%s) + SELECT il.source_id, il.target_id FROM item_links il + JOIN items s ON s.id = il.source_id AND s.deleted_at IS NULL + JOIN items t ON t.id = il.target_id AND t.deleted_at IS NULL + WHERE il.workspace_id = ? AND il.link_type IN (%s) `, childLinkTypeSQL())), workspaceID) if err != nil { return nil, fmt.Errorf("get parent map: %w", err) diff --git a/internal/store/items_test.go b/internal/store/items_test.go index fb4aacff..f9c1bdc9 100644 --- a/internal/store/items_test.go +++ b/internal/store/items_test.go @@ -954,6 +954,297 @@ func TestItemLinks(t *testing.T) { } } +// TestItemLinks_HidesSoftDeletedEndpoints exercises BUG-734: when an item that +// is the source or target of a link gets soft-deleted, GetItemLinks should not +// surface the link from the surviving endpoint's perspective. Restoring the +// deleted item should resurrect the link automatically — the row is preserved +// on disk; only the query layer filters it. +func TestItemLinks_HidesSoftDeletedEndpoints(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "Test") + col := createTestCollection(t, s, ws.ID, "Tasks") + + plan := createTestItem(t, s, ws.ID, col.ID, "Plan", "") + implementer := createTestItem(t, s, ws.ID, col.ID, "Implementer task", "") + + // implementer --implements--> plan + if _, err := s.CreateItemLink(ws.ID, models.ItemLinkCreate{ + TargetID: plan.ID, + LinkType: "implements", + }, implementer.ID); err != nil { + t.Fatalf("CreateItemLink: %v", err) + } + + // Sanity: link visible from both endpoints. + if links, _ := s.GetItemLinks(plan.ID); len(links) != 1 { + t.Fatalf("expected 1 link from plan side before delete, got %d", len(links)) + } + if links, _ := s.GetItemLinks(implementer.ID); len(links) != 1 { + t.Fatalf("expected 1 link from implementer side before delete, got %d", len(links)) + } + + // Soft-delete the implementer (the BUG-734 scenario: source side gone). + if err := s.DeleteItem(implementer.ID); err != nil { + t.Fatalf("DeleteItem: %v", err) + } + + // From the plan's perspective, the dangling implementer must not surface. + links, err := s.GetItemLinks(plan.ID) + if err != nil { + t.Fatalf("GetItemLinks after delete: %v", err) + } + if len(links) != 0 { + t.Errorf("expected 0 links from plan side after implementer deleted, got %d (orphan leak — BUG-734)", len(links)) + } + + // Restore the implementer — the link row was never deleted, so the + // relationship should reappear automatically. + if _, err := s.RestoreItem(implementer.ID); err != nil { + t.Fatalf("RestoreItem: %v", err) + } + links, err = s.GetItemLinks(plan.ID) + if err != nil { + t.Fatalf("GetItemLinks after restore: %v", err) + } + if len(links) != 1 { + t.Errorf("expected 1 link from plan side after restore, got %d (link should be preserved across soft-delete/restore)", len(links)) + } + + // Now soft-delete the plan side instead (target side gone) and verify the + // implementer's view also drops the dangling link. + if err := s.DeleteItem(plan.ID); err != nil { + t.Fatalf("DeleteItem plan: %v", err) + } + links, err = s.GetItemLinks(implementer.ID) + if err != nil { + t.Fatalf("GetItemLinks after target delete: %v", err) + } + if len(links) != 0 { + t.Errorf("expected 0 links from implementer side after plan deleted, got %d (target-side orphan leak)", len(links)) + } +} + +// TestGetParentForItem_HidesSoftDeletedParent ensures lineage / breadcrumb +// queries don't surface a soft-deleted ancestor. See BUG-734. +func TestGetParentForItem_HidesSoftDeletedParent(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "Test") + col := createTestCollection(t, s, ws.ID, "Tasks") + + parent := createTestItem(t, s, ws.ID, col.ID, "Parent", "") + child := createTestItem(t, s, ws.ID, col.ID, "Child", "") + + if _, err := s.SetParentLink(ws.ID, child.ID, parent.ID, "user"); err != nil { + t.Fatalf("SetParentLink: %v", err) + } + + // Before delete: parent visible. + if link, err := s.GetParentForItem(child.ID); err != nil { + t.Fatalf("GetParentForItem: %v", err) + } else if link == nil { + t.Fatal("expected parent link before delete, got nil") + } + + // Soft-delete parent. + if err := s.DeleteItem(parent.ID); err != nil { + t.Fatalf("DeleteItem: %v", err) + } + + // After delete: must read as no parent (don't render a deleted breadcrumb). + link, err := s.GetParentForItem(child.ID) + if err != nil { + t.Fatalf("GetParentForItem after delete: %v", err) + } + if link != nil { + t.Errorf("expected nil parent link after soft-delete, got %+v", link) + } + + // After restore: parent visible again. + if _, err := s.RestoreItem(parent.ID); err != nil { + t.Fatalf("RestoreItem: %v", err) + } + if link, err := s.GetParentForItem(child.ID); err != nil { + t.Fatalf("GetParentForItem after restore: %v", err) + } else if link == nil { + t.Error("expected parent link to reappear after restore") + } +} + +// TestGetParentMap_ExcludesSoftDeletedEndpoints covers the dashboard +// orphan-detection path: a task whose parent has been soft-deleted should +// NOT appear in GetParentMap, so handlers_dashboard.go correctly flags the +// task as orphaned. See BUG-734 / Codex review on PR #259. +func TestGetParentMap_ExcludesSoftDeletedEndpoints(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "Test") + col := createTestCollection(t, s, ws.ID, "Tasks") + + parent := createTestItem(t, s, ws.ID, col.ID, "Parent", "") + child := createTestItem(t, s, ws.ID, col.ID, "Child", "") + if _, err := s.SetParentLink(ws.ID, child.ID, parent.ID, "user"); err != nil { + t.Fatalf("SetParentLink: %v", err) + } + + // Sanity: child→parent mapping present. + m, err := s.GetParentMap(ws.ID) + if err != nil { + t.Fatalf("GetParentMap: %v", err) + } + if m[child.ID] != parent.ID { + t.Fatalf("expected parent map %s→%s, got %s→%s", child.ID, parent.ID, child.ID, m[child.ID]) + } + + // Soft-delete the parent. The child must now look "parentless" so the + // dashboard orphan detector flags it. + if err := s.DeleteItem(parent.ID); err != nil { + t.Fatalf("DeleteItem: %v", err) + } + m, err = s.GetParentMap(ws.ID) + if err != nil { + t.Fatalf("GetParentMap after parent delete: %v", err) + } + if _, hasEntry := m[child.ID]; hasEntry { + t.Errorf("expected child to drop from parent map after parent soft-deleted (orphan-detection regression)") + } + + // Restoring the parent should bring the mapping back. + if _, err := s.RestoreItem(parent.ID); err != nil { + t.Fatalf("RestoreItem: %v", err) + } + m, err = s.GetParentMap(ws.ID) + if err != nil { + t.Fatalf("GetParentMap after parent restore: %v", err) + } + if m[child.ID] != parent.ID { + t.Errorf("expected parent map to be restored to %s→%s, got %s→%s", child.ID, parent.ID, child.ID, m[child.ID]) + } + + // Soft-deleting the child side should also drop the entry. + if err := s.DeleteItem(child.ID); err != nil { + t.Fatalf("DeleteItem child: %v", err) + } + m, err = s.GetParentMap(ws.ID) + if err != nil { + t.Fatalf("GetParentMap after child delete: %v", err) + } + if _, hasEntry := m[child.ID]; hasEntry { + t.Errorf("expected child to drop from parent map after the child itself was soft-deleted") + } +} + +// TestListItems_ParentFilter_FTS_RespectsSoftDeletedParent covers the +// `parent=&search=` combination. The search path routes through +// listItemsFTS, which the non-FTS parent filter doesn't touch; the FTS +// path needs to enforce the same deleted-parent rejection. See BUG-734 / +// Codex review on PR #259 (3rd pass). +func TestListItems_ParentFilter_FTS_RespectsSoftDeletedParent(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "Test") + col := createTestCollection(t, s, ws.ID, "Tasks") + + parent := createTestItem(t, s, ws.ID, col.ID, "Parent", "") + // Use a distinctive title so the FTS match is unambiguous. + child := createTestItem(t, s, ws.ID, col.ID, "Distinctivekeyword child", "") + if _, err := s.SetParentLink(ws.ID, child.ID, parent.ID, "user"); err != nil { + t.Fatalf("SetParentLink: %v", err) + } + + // Sanity: search + parent finds the child while parent is live. + items, err := s.ListItems(ws.ID, models.ItemListParams{ + ParentLinkID: parent.ID, + Search: "Distinctivekeyword", + }) + if err != nil { + t.Fatalf("ListItems (FTS+parent): %v", err) + } + if len(items) != 1 || items[0].ID != child.ID { + t.Fatalf("expected to find 1 child via FTS+parent before delete, got %d", len(items)) + } + + // Soft-delete the parent. The FTS path must also reject the now-deleted + // parent, otherwise `?parent=&search=foo` continues to leak + // active children of an archived parent (the gap Codex flagged). + if err := s.DeleteItem(parent.ID); err != nil { + t.Fatalf("DeleteItem: %v", err) + } + items, err = s.ListItems(ws.ID, models.ItemListParams{ + ParentLinkID: parent.ID, + Search: "Distinctivekeyword", + }) + if err != nil { + t.Fatalf("ListItems (FTS+parent) after delete: %v", err) + } + if len(items) != 0 { + t.Errorf("expected 0 children via FTS+parent after parent soft-deleted, got %d (FTS-path parent-filter regression)", len(items)) + } + + // Restore brings the child back through the FTS+parent path. + if _, err := s.RestoreItem(parent.ID); err != nil { + t.Fatalf("RestoreItem: %v", err) + } + items, err = s.ListItems(ws.ID, models.ItemListParams{ + ParentLinkID: parent.ID, + Search: "Distinctivekeyword", + }) + if err != nil { + t.Fatalf("ListItems (FTS+parent) after restore: %v", err) + } + if len(items) != 1 || items[0].ID != child.ID { + t.Errorf("expected child to reappear via FTS+parent after restoring parent, got %d", len(items)) + } +} + +// TestListItems_ParentFilter_RespectsSoftDeletedParent ensures the +// `parent=` query filter doesn't return children of a soft-deleted +// parent. Slug/ref filters already reject deleted parents upstream via +// GetItem/GetItemBySlug, but raw-UUID input bypasses that path. See +// BUG-734 / Codex review on PR #259. +func TestListItems_ParentFilter_RespectsSoftDeletedParent(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "Test") + col := createTestCollection(t, s, ws.ID, "Tasks") + + parent := createTestItem(t, s, ws.ID, col.ID, "Parent", "") + child := createTestItem(t, s, ws.ID, col.ID, "Child", "") + if _, err := s.SetParentLink(ws.ID, child.ID, parent.ID, "user"); err != nil { + t.Fatalf("SetParentLink: %v", err) + } + + // Sanity: child is reachable via the parent filter. + items, err := s.ListItems(ws.ID, models.ItemListParams{ParentLinkID: parent.ID}) + if err != nil { + t.Fatalf("ListItems: %v", err) + } + if len(items) != 1 || items[0].ID != child.ID { + t.Fatalf("expected to find 1 child via parent filter before delete, got %+v", items) + } + + // Soft-delete the parent. Filter must now return no children — no + // caller should be able to list children of a deleted parent by UUID. + if err := s.DeleteItem(parent.ID); err != nil { + t.Fatalf("DeleteItem: %v", err) + } + items, err = s.ListItems(ws.ID, models.ItemListParams{ParentLinkID: parent.ID}) + if err != nil { + t.Fatalf("ListItems after parent delete: %v", err) + } + if len(items) != 0 { + t.Errorf("expected 0 children after parent soft-deleted, got %d (parent-filter regression)", len(items)) + } + + // Restoring the parent should bring the child back into the filter. + if _, err := s.RestoreItem(parent.ID); err != nil { + t.Fatalf("RestoreItem: %v", err) + } + items, err = s.ListItems(ws.ID, models.ItemListParams{ParentLinkID: parent.ID}) + if err != nil { + t.Fatalf("ListItems after restore: %v", err) + } + if len(items) != 1 || items[0].ID != child.ID { + t.Errorf("expected 1 child after restoring parent, got %d", len(items)) + } +} + func TestItemLinkDefaultType(t *testing.T) { s := testStore(t) ws := createTestWorkspace(t, s, "Test")