fix(store): search, tags, children and progress exclude soft-deleted collections (BUG-3428, phase 1) (#1824)

* test(store): search, tags, children and progress must exclude soft-deleted collections (BUG-3428, red)

* fix(store): global search, tags, children and progress exclude soft-deleted collections; the open-children guard keeps them (BUG-3428)

* test(store): bare-number search excludes soft-deleted collections (BUG-3428)

* test(store): has_children agrees with the children list; tighter progress and facet assertions (BUG-3428, codex r1, red)

* fix(store): has_children leaves out children in soft-deleted collections (BUG-3428, codex r1)

internal/store/bug3428_deleted_collection_reads_test.go

@@ -0,0 +1,217 @@
+package store
+
+import (
+	"strconv"
+	"testing"
+
+	"github.com/PerpetualSoftware/pad/internal/models"
+)
+
+// BUG-3428 phase 1: the reads BUG-3425 left out. Global search, workspace tags,
+// and child items / progress still included the items of a soft-deleted
+// collection. Each test keeps a live item beside the doomed one, so a read
+// that returns nothing cannot pass. Reuses bug3425World (same package).
+
+func searchIDs(t *testing.T, s *Store, ws *models.Workspace, q string) (map[string]bool, *SearchResponse) {
+	t.Helper()
+	resp, err := s.Search(SearchParams{Query: q, Workspace: ws.Slug, Unrestricted: true})
+	if err != nil {
+		t.Fatalf("Search(%q): %v", q, err)
+	}
+	ids := map[string]bool{}
+	for _, r := range resp.Results {
+		ids[r.Item.ID] = true
+	}
+	return ids, resp
+}
+
+func TestSearch_ExcludesSoftDeletedCollections(t *testing.T) {
+	t.Parallel()
+	w := newBug3425World(t)
+
+	ids, _ := searchIDs(t, w.s, w.ws, "Zephyr")
+	if !ids[w.doomed.ID] || !ids[w.live.ID] {
+		t.Fatalf("control: text search should find both items before the delete")
+	}
+	if ref, _ := searchIDs(t, w.s, w.ws, refOf(w.doomed)); !ref[w.doomed.ID] {
+		t.Fatalf("control: a ref search should find the doomed item before the delete")
+	}
+	num := strconv.Itoa(*w.doomed.ItemNumber)
+	if byNum, _ := searchIDs(t, w.s, w.ws, num); !byNum[w.doomed.ID] {
+		t.Fatalf("control: a bare-number search should find the doomed item before the delete")
+	}
+
+	w.deleteDoomed(t)
+	ids, resp := searchIDs(t, w.s, w.ws, "Zephyr")
+	if ids[w.doomed.ID] {
+		t.Errorf("text search still finds an item from a soft-deleted collection")
+	}
+	if !ids[w.live.ID] {
+		t.Errorf("text search lost the live item")
+	}
+	if resp.Total != len(resp.Results) {
+		t.Errorf("total %d disagrees with %d results", resp.Total, len(resp.Results))
+	}
+	// The facet query swallows its own errors into empty maps, so the live
+	// collection's count is asserted too: an empty map cannot pass.
+	if resp.Facets == nil || resp.Facets.Collections[w.liveColl.Slug] != 1 {
+		t.Errorf("facets lost the live collection: %+v", resp.Facets)
+	} else if resp.Facets.Collections[w.doomColl.Slug] != 0 {
+		t.Errorf("facets still count the soft-deleted collection: %v", resp.Facets.Collections)
+	}
+	if ref, _ := searchIDs(t, w.s, w.ws, refOf(w.doomed)); ref[w.doomed.ID] {
+		t.Errorf("a ref search still finds an item from a soft-deleted collection")
+	}
+	if byNum, _ := searchIDs(t, w.s, w.ws, num); byNum[w.doomed.ID] {
+		t.Errorf("a bare-number search still finds an item from a soft-deleted collection")
+	}
+	if byNum, _ := searchIDs(t, w.s, w.ws, strconv.Itoa(*w.live.ItemNumber)); !byNum[w.live.ID] {
+		t.Errorf("a bare-number search lost the live item")
+	}
+}
+
+func TestListWorkspaceTags_ExcludesSoftDeletedCollections(t *testing.T) {
+	t.Parallel()
+	w := newBug3425World(t)
+	tag := func(it *models.Item, tags string) {
+		t.Helper()
+		if _, err := w.s.UpdateItem(it.ID, models.ItemUpdate{Tags: &tags}); err != nil {
+			t.Fatalf("tag %s: %v", it.Title, err)
+		}
+	}
+	tag(w.live, `["shared"]`)
+	tag(w.doomed, `["shared","doomed-only"]`)
+	counts := func() map[string]int {
+		t.Helper()
+		tc, err := w.s.ListWorkspaceTags(w.ws.ID, nil, nil)
+		if err != nil {
+			t.Fatalf("ListWorkspaceTags: %v", err)
+		}
+		m := map[string]int{}
+		for _, c := range tc {
+			m[c.Tag] = c.Count
+		}
+		return m
+	}
+	if c := counts(); c["shared"] != 2 || c["doomed-only"] != 1 {
+		t.Fatalf("control: want shared=2 doomed-only=1 before the delete, got %v", c)
+	}
+	w.deleteDoomed(t)
+	c := counts()
+	if c["shared"] != 1 {
+		t.Errorf("shared counts the soft-deleted collection's item: %d, want 1", c["shared"])
+	}
+	if _, ok := c["doomed-only"]; ok {
+		t.Errorf("a tag only a soft-deleted collection's item carries still lists: %v", c)
+	}
+}
+
+func TestChildrenAndProgress_ExcludeSoftDeletedCollections(t *testing.T) {
+	t.Parallel()
+	w := newBug3425World(t)
+	s := w.s
+	parentColl := createTestCollection(t, s, w.ws.ID, "Plans")
+	parent := createTestItem(t, s, w.ws.ID, parentColl.ID, "Parent", "")
+	for _, child := range []*models.Item{w.live, w.doomed} {
+		if _, err := s.CreateItemLink(w.ws.ID, models.ItemLinkCreate{TargetID: parent.ID, LinkType: "parent"}, child.ID); err != nil {
+			t.Fatalf("link %s: %v", child.Title, err)
+		}
+	}
+	check := func(when string, wantDoomed bool, wantTotal int) {
+		t.Helper()
+		kids, err := s.GetChildItems(parent.ID)
+		if err != nil {
+			t.Fatalf("%s GetChildItems: %v", when, err)
+		}
+		if !hasItem(kids, w.live.ID) || hasItem(kids, w.doomed.ID) != wantDoomed {
+			t.Errorf("%s GetChildItems: live=%v doomed=%v, want doomed=%v", when, hasItem(kids, w.live.ID), hasItem(kids, w.doomed.ID), wantDoomed)
+		}
+		batch, err := s.GetChildItemsForParents([]string{parent.ID})
+		if err != nil {
+			t.Fatalf("%s GetChildItemsForParents: %v", when, err)
+		}
+		if !hasItem(batch[parent.ID], w.live.ID) || hasItem(batch[parent.ID], w.doomed.ID) != wantDoomed {
+			t.Errorf("%s GetChildItemsForParents disagrees: doomed=%v, want %v", when, hasItem(batch[parent.ID], w.doomed.ID), wantDoomed)
+		}
+		total, _, err := s.GetItemProgress(parent.ID)
+		if err != nil {
+			t.Fatalf("%s GetItemProgress: %v", when, err)
+		}
+		if total != wantTotal {
+			t.Errorf("%s GetItemProgress total = %d, want %d", when, total, wantTotal)
+		}
+		all, err := s.GetAllItemProgress(w.ws.ID, parentColl.Slug, false)
+		if err != nil {
+			t.Fatalf("%s GetAllItemProgress: %v", when, err)
+		}
+		found := false
+		for _, p := range all {
+			if p.ItemID == parent.ID {
+				found = true
+				if p.Total != wantTotal {
+					t.Errorf("%s GetAllItemProgress total = %d, want %d", when, p.Total, wantTotal)
+				}
+			}
+		}
+		if !found {
+			t.Errorf("%s GetAllItemProgress has no row for the parent", when)
+		}
+	}
+	check("before the delete (control)", true, 2)
+	w.deleteDoomed(t)
+	check("after the delete", false, 1)
+}
+
+// The open-children guard reads children through GetChildItemsTx and is NOT
+// narrowed by BUG-3428: whether an open child in a soft-deleted collection
+// should still block closing its parent is a write-path rule, left for the
+// lead's ruling. This pins the current behaviour so changing it is a decision.
+func TestGetChildItemsTx_StillIncludesSoftDeletedCollections(t *testing.T) {
+	t.Parallel()
+	w := newBug3425World(t)
+	s := w.s
+	parentColl := createTestCollection(t, s, w.ws.ID, "Plans")
+	parent := createTestItem(t, s, w.ws.ID, parentColl.ID, "Parent", "")
+	if _, err := s.CreateItemLink(w.ws.ID, models.ItemLinkCreate{TargetID: parent.ID, LinkType: "parent"}, w.doomed.ID); err != nil {
+		t.Fatalf("link: %v", err)
+	}
+	w.deleteDoomed(t)
+	tx, err := s.DB().Begin()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer tx.Rollback()
+	kids, err := s.GetChildItemsTx(tx, parent.ID)
+	if err != nil {
+		t.Fatalf("GetChildItemsTx: %v", err)
+	}
+	if !hasItem(kids, w.doomed.ID) {
+		t.Errorf("the guard's child read no longer sees a soft-deleted collection's child; that changes what blocks closing a parent")
+	}
+}
+
+// Codex r1: has_children must agree with the children list. A child whose only
+// child lives in a soft-deleted collection has none to list, so it must not
+// claim to have any.
+func TestPopulateHasChildren_ExcludesSoftDeletedCollections(t *testing.T) {
+	t.Parallel()
+	w := newBug3425World(t)
+	s := w.s
+	middle := w.live
+	if _, err := s.CreateItemLink(w.ws.ID, models.ItemLinkCreate{TargetID: middle.ID, LinkType: "parent"}, w.doomed.ID); err != nil {
+		t.Fatalf("link: %v", err)
+	}
+	flag := func() bool {
+		t.Helper()
+		items := []models.Item{*middle}
+		s.PopulateHasChildren(items)
+		return items[0].HasChildren
+	}
+	if !flag() {
+		t.Fatalf("control: has_children should be true before the delete")
+	}
+	w.deleteDoomed(t)
+	if flag() {
+		t.Errorf("has_children still true when the only child is in a soft-deleted collection")
+	}
+}

internal/store/done_field_test.go

@@ -165,10 +165,11 @@ func TestGetItemProgress_DefaultsToStatusWithoutSettings(t *testing.T) {
 }
 
 // TestGetItemProgress_HonorsSoftDeletedChildCollections verifies a
-// regression Codex flagged: soft-deleted child collections must still
-// contribute done filters, otherwise their items can never satisfy the
-// per-collection `collection_id = ? AND ...` clause and would always be
-// counted as active.
+// regression Codex flagged: a child whose collection was soft-deleted must
+// never be counted as ACTIVE because its collection's done rule went
+// missing. Since BUG-3428 such a child is left out of progress entirely, as
+// every other item read leaves it out, which closes that miscount at the
+// source: it is neither open nor done.
 func TestGetItemProgress_HonorsSoftDeletedChildCollections(t *testing.T) {
 	t.Parallel()
 	s := testStore(t)
@@ -211,18 +212,14 @@ func TestGetItemProgress_HonorsSoftDeletedChildCollections(t *testing.T) {
 		t.Fatalf("soft delete tasks: %v", err)
 	}
 
-	// Expect the same done count — soft-deleted collections still
-	// contribute their done rules.
-	_, doneAfter, err := s.GetItemProgress(plan.ID)
+	// Both children were in the deleted collection: out of BOTH counts, so
+	// the open one is not counted as active work and the plan reads 0/0.
+	totalAfter, doneAfter, err := s.GetItemProgress(plan.ID)
 	if err != nil {
 		t.Fatalf("GetItemProgress after soft-delete: %v", err)
 	}
-	if doneAfter != 1 {
-		t.Errorf(
-			"expected done count to remain 1 after collection soft-delete, got %d — "+
-				"soft-deleted collections must still contribute done filters",
-			doneAfter,
-		)
+	if totalAfter != 0 || doneAfter != 0 {
+		t.Errorf("after the collection's soft-delete want 0/0 (its children leave progress, BUG-3428), got %d/%d", doneAfter, totalAfter)
 	}
 }
 

internal/store/items.go

@@ -1437,7 +1437,8 @@ func (s *Store) ListWorkspaceTags(workspaceID string, collectionIDs, itemIDs []s
 	query := `
 		SELECT ` + valueExpr + ` AS tag, COUNT(DISTINCT i.id) AS cnt
 		FROM items i, ` + fromExpr + `
-		WHERE i.workspace_id = ? AND i.deleted_at IS NULL`
+		WHERE i.workspace_id = ? AND i.deleted_at IS NULL
+		  AND EXISTS (SELECT 1 FROM collections c WHERE c.id = i.collection_id AND c.deleted_at IS NULL)`
 	args := []interface{}{workspaceID}
 
 	if len(collectionIDs) > 0 && len(itemIDs) > 0 {
@@ -4610,6 +4611,7 @@ func (s *Store) GetItemProgress(parentItemID string) (total int, done int, err e
 		SELECT COUNT(CASE WHEN NOT %s THEN 1 END),
 		       COUNT(CASE WHEN %s AND NOT %s THEN 1 END)
 		FROM items i
+		JOIN collections c ON c.id = i.collection_id AND c.deleted_at IS NULL
 		JOIN item_links il ON il.source_id = i.id AND il.link_type IN (%s) AND il.target_id = ?
 		WHERE i.deleted_at IS NULL
 	`, abandonedExpr, doneExpr, abandonedExpr, childLinkTypeSQL())), args...).Scan(&total, &done)
@@ -4909,6 +4911,7 @@ func (s *Store) GetAllItemProgress(workspaceID, collectionSlug string, includeAr
 		LEFT JOIN item_links il ON il.link_type IN (%s) AND il.target_id = p.id
 		LEFT JOIN items t ON t.id = il.source_id
 		                  AND t.deleted_at IS NULL
+		                  AND EXISTS (SELECT 1 FROM collections tc WHERE tc.id = t.collection_id AND tc.deleted_at IS NULL)
 		WHERE p.workspace_id = ?
 		  AND pc.slug = ?
 		  %s
@@ -4935,8 +4938,11 @@ func (s *Store) GetAllItemProgress(workspaceID, collectionSlug string, includeAr
 
 // GetChildItems returns all non-deleted child items linked to the given parent
 // via item_links. Returns children from any collection.
+//
+// A child in a soft-deleted collection is left out (BUG-3428), as every other
+// item read leaves it out.
 func (s *Store) GetChildItems(parentItemID string) ([]models.Item, error) {
-	return s.getChildItems(s.db, parentItemID)
+	return s.getChildItems(s.db, parentItemID, false)
 }
 
 // GetChildItemsTx is the in-transaction variant of GetChildItems. The
@@ -4962,10 +4968,13 @@ func (s *Store) GetChildItems(parentItemID string) ([]models.Item, error) {
 // `SELECT DISTINCT … FOR UPDATE`. The advisory-lock pattern sidesteps
 // that constraint while still giving us a serialized snapshot.
 func (s *Store) GetChildItemsTx(tx *sql.Tx, parentItemID string) ([]models.Item, error) {
+	// The open-children guard is this function's caller, and it still counts
+	// a child in a soft-deleted collection as open work (BUG-3428 leaves that
+	// write rule to a ruling), so this read keeps those children.
 	if tx == nil {
-		return s.GetChildItems(parentItemID)
+		return s.getChildItems(s.db, parentItemID, true)
 	}
-	return s.getChildItems(tx, parentItemID)
+	return s.getChildItems(tx, parentItemID, true)
 }
 
 // acquireParentChildrenLocksForUpdate is the in-tx helper UpdateItem
@@ -5198,7 +5207,11 @@ type childQueryer interface {
 	Query(query string, args ...any) (*sql.Rows, error)
 }
 
-func (s *Store) getChildItems(q childQueryer, parentItemID string) ([]models.Item, error) {
+func (s *Store) getChildItems(q childQueryer, parentItemID string, includeDeletedCollections bool) ([]models.Item, error) {
+	collFilter := " AND c.deleted_at IS NULL"
+	if includeDeletedCollections {
+		collFilter = ""
+	}
 	rows, err := q.Query(s.q(fmt.Sprintf(`
 		SELECT DISTINCT i.id, i.workspace_id, i.collection_id, i.title, i.slug, i.content, `+contentStateSQL+`, i.fields, i.tags,
 		       i.pinned, i.sort_order, i.parent_id, i.assigned_user_id, i.agent_role_id, i.role_sort_order,
@@ -5212,9 +5225,9 @@ func (s *Store) getChildItems(q childQueryer, parentItemID string) ([]models.Ite
 		JOIN item_links il ON il.source_id = i.id AND il.link_type IN (%s) AND il.target_id = ?
 		LEFT JOIN users au ON au.id = i.assigned_user_id
 		LEFT JOIN agent_roles ar ON ar.id = i.agent_role_id
-		WHERE i.deleted_at IS NULL
+		WHERE i.deleted_at IS NULL%s
 		ORDER BY i.sort_order ASC, i.created_at ASC
-	`, childLinkTypeSQL())), parentItemID)
+	`, childLinkTypeSQL(), collFilter)), parentItemID)
 	if err != nil {
 		return nil, fmt.Errorf("get child items: %w", err)
 	}
@@ -5263,7 +5276,7 @@ func (s *Store) GetChildItemsForParents(parentIDs []string) (map[string][]models
 		JOIN item_links il ON il.source_id = i.id AND il.link_type IN (%s) AND il.target_id IN (%s)
 		LEFT JOIN users au ON au.id = i.assigned_user_id
 		LEFT JOIN agent_roles ar ON ar.id = i.agent_role_id
-		WHERE i.deleted_at IS NULL
+		WHERE i.deleted_at IS NULL AND c.deleted_at IS NULL
 		ORDER BY i.sort_order ASC, i.created_at ASC
 	`, childLinkTypeSQL(), strings.Join(placeholders, ","))), args...)
 	if err != nil {
@@ -5371,6 +5384,7 @@ func (s *Store) PopulateHasChildren(items []models.Item) {
 	query := fmt.Sprintf(`
 		SELECT DISTINCT il.target_id FROM item_links il
 		JOIN items child ON child.id = il.source_id AND child.deleted_at IS NULL
+		JOIN collections cc ON cc.id = child.collection_id AND cc.deleted_at IS NULL
 		WHERE il.link_type IN (%s) AND il.target_id IN (%s)
 	`, childLinkTypeSQL(), strings.Join(placeholders, ","))
 

internal/store/search.go

@@ -147,6 +147,7 @@ func (s *Store) Search(params SearchParams) (*SearchResponse, error) {
 		}
 
 		refQuery, refArgs = appendSearchPermissionFilter(refQuery, refArgs, params)
+		refQuery = appendLiveCollectionFilter(refQuery)
 
 		// Apply content filters to ref lookup too
 		refQuery, refArgs = appendSearchCollectionFilter(refQuery, refArgs, params)
@@ -240,6 +241,7 @@ func (s *Store) Search(params SearchParams) (*SearchResponse, error) {
 		}
 
 		numQuery, numArgs = appendSearchPermissionFilter(numQuery, numArgs, params)
+		numQuery = appendLiveCollectionFilter(numQuery)
 
 		// Apply content filters to numeric lookup too
 		numQuery, numArgs = appendSearchCollectionFilter(numQuery, numArgs, params)
@@ -406,6 +408,7 @@ func (s *Store) Search(params SearchParams) (*SearchResponse, error) {
 	}
 
 	query, args = appendSearchPermissionFilter(query, args, params)
+	query = appendLiveCollectionFilter(query)
 
 	// Collection filter — the resolved per-workspace set, or a literal slug.
 	query, args = appendSearchCollectionFilter(query, args, params)
@@ -618,6 +621,7 @@ func (s *Store) appendSearchFilters(query string, args []interface{}, params Sea
 	}
 
 	query, args = appendSearchPermissionFilter(query, args, params)
+	query = appendLiveCollectionFilter(query)
 
 	query, args = appendSearchCollectionFilter(query, args, params)
 
@@ -790,6 +794,14 @@ func appendSearchPermissionFilter(query string, args []interface{}, params Searc
 	return query, args
 }
 
+// appendLiveCollectionFilter leaves out the items of a soft-deleted collection
+// (BUG-3428), for every caller alike. Every search query joins collections as
+// c and calls it beside appendSearchPermissionFilter, so the results, the
+// total and the facets agree.
+func appendLiveCollectionFilter(query string) string {
+	return query + ` AND c.deleted_at IS NULL`
+}
+
 func appendSearchCollectionFilter(query string, args []interface{}, params SearchParams) (string, []interface{}) {
 	if params.CollectionFilterIDs != nil {
 		if len(params.CollectionFilterIDs) == 0 {