fix(taxonomy): 404 node records on a tenant mismatch (ENG-1887)

internal/repository/taxonomy_repository.go

@@ -1038,6 +1038,8 @@ func (r *TaxonomyRepository) RemoveNode(
 }
 
 // ListNodeRecords returns feedback records assigned to a visible taxonomy node or descendants.
+// The node must be visible and belong to the tenant, otherwise a not-found error is returned —
+// the same contract as every other node-scoped operation.
 func (r *TaxonomyRepository) ListNodeRecords(
 	ctx context.Context,
 	nodeID uuid.UUID,
@@ -1048,7 +1050,29 @@ func (r *TaxonomyRepository) ListNodeRecords(
 		limit = defaultTaxonomyNodeRecordLimit
 	}
 
-	rows, err := r.db.Query(ctx, `
+	// The records query below is already tenant-safe on its own (it filters on the run's tenant, so a
+	// foreign node simply matches no rows). The ownership guard is about the contract, not isolation:
+	// without it a foreign or removed node is indistinguishable from one that genuinely holds no
+	// records, which is what CountNodeRecords, GetTree, RenameNode and RemoveNode all avoid by
+	// resolving ownership first.
+	//
+	// Both reads share one REPEATABLE READ snapshot. On separate statements a RemoveNode committing
+	// between them would pass the guard and then match no rows in the recursive CTE, handing back the
+	// ambiguous 200-empty this contract exists to remove. Under one snapshot the node is either
+	// visible to both reads or to neither, so a removal mid-request lands as a 404. Read-only, so the
+	// deferred rollback is the only exit.
+	dbTx, err := r.db.BeginTx(ctx, pgx.TxOptions{IsoLevel: pgx.RepeatableRead, AccessMode: pgx.ReadOnly})
+	if err != nil {
+		return nil, 0, fmt.Errorf("begin taxonomy node records tx: %w", err)
+	}
+
+	defer rollbackQuietly(ctx, dbTx, "list taxonomy node records: rollback failed")
+
+	if _, err := getNodeForTenant(ctx, dbTx, nodeID, tenantID); err != nil {
+		return nil, 0, err
+	}
+
+	rows, err := dbTx.Query(ctx, `
 		WITH RECURSIVE visible_nodes AS (
 			SELECT id, run_id, cluster_id
 			FROM taxonomy_nodes
@@ -1342,6 +1366,41 @@ func scanTaxonomyNode(row scanner) (*models.TaxonomyNode, error) {
 	return &node, nil
 }
 
+// A node is addressable by a tenant when it is visible and its run belongs to that tenant. Shared by
+// the locking write path and the read path so the two can never drift apart on what "yours" means.
+// $1 is the node id, $2 the tenant id.
+const taxonomyNodeForTenantWhere = `
+		WHERE id = $1 AND removed_at IS NULL
+		  AND EXISTS (
+		    SELECT 1 FROM taxonomy_runs
+		    WHERE taxonomy_runs.id = taxonomy_nodes.run_id AND taxonomy_runs.tenant_id = $2
+		  )`
+
+// getNodeForTenant returns a visible taxonomy node addressable by the tenant, or a not-found error.
+// The read-only counterpart of getNodeForUpdate: same ownership predicate, no row lock. It takes a
+// queryer so the caller decides the snapshot — ListNodeRecords passes its transaction so the check
+// and the records read cannot disagree about whether the node is still there.
+func getNodeForTenant(
+	ctx context.Context,
+	q queryer,
+	nodeID uuid.UUID,
+	tenantID string,
+) (*models.TaxonomyNode, error) {
+	node, err := queryTaxonomyNode(ctx, q, taxonomyNodeSelect+`
+		FROM taxonomy_nodes`+taxonomyNodeForTenantWhere,
+		nodeID, tenantID,
+	)
+	if err != nil {
+		if errors.Is(err, pgx.ErrNoRows) {
+			return nil, huberrors.NewNotFoundError("taxonomy_node", "taxonomy node not found")
+		}
+
+		return nil, fmt.Errorf("get taxonomy node for tenant: %w", err)
+	}
+
+	return node, nil
+}
+
 // getNodeForUpdate takes a tenantWriteTx (not the narrower queryer) so the
 // compiler enforces that the SELECT ... FOR UPDATE row lock is held for the
 // life of a transaction; outside one, the lock would release at statement end.
@@ -1354,12 +1413,7 @@ func getNodeForUpdate(
 	// The tenant predicate keeps the row lock tenant-scoped: a caller can never
 	// lock another tenant's node row, even transiently.
 	node, err := queryTaxonomyNode(ctx, transaction, taxonomyNodeSelect+`
-		FROM taxonomy_nodes
-		WHERE id = $1 AND removed_at IS NULL
-		  AND EXISTS (
-		    SELECT 1 FROM taxonomy_runs
-		    WHERE taxonomy_runs.id = taxonomy_nodes.run_id AND taxonomy_runs.tenant_id = $2
-		  )
+		FROM taxonomy_nodes`+taxonomyNodeForTenantWhere+`
 		FOR UPDATE`,
 		nodeID, tenantID,
 	)

openapi.yaml

@@ -2080,7 +2080,9 @@ paths:
             summary: List feedback records for a taxonomy node
             description: |
                 Returns the feedback records assigned to a node and all of its (visible) descendant nodes, via the
-                clusters those nodes reference. Tenant-scoped. The `limit` in the response reflects the applied cap.
+                clusters those nodes reference. Tenant-scoped; returns 404 if the node does not belong to the tenant
+                or has been removed. An empty `data` therefore means the node genuinely holds no records. The `limit`
+                in the response reflects the applied cap.
             operationId: list-taxonomy-node-records
             parameters:
                 - name: node_id
@@ -2129,6 +2131,12 @@ paths:
                         application/problem+json:
                             schema:
                                 $ref: '#/components/schemas/ErrorModel'
+                "404":
+                    description: Not Found – no node with this ID for the tenant.
+                    content:
+                        application/problem+json:
+                            schema:
+                                $ref: '#/components/schemas/ErrorModel'
                 default:
                     description: Error
                     content:

tests/taxonomy_api_test.go

@@ -223,12 +223,11 @@ func TestTaxonomyAPI_TenantIsolation(t *testing.T) {
 			taxonomyURL(harness.server.URL, "/v1/taxonomy/nodes/"+ids.BranchID.String(), removeQuery), harness.apiKey, nil, http.StatusNotFound, nil)
 	})
 
-	t.Run("node records return nothing for another tenant", func(t *testing.T) {
+	t.Run("node records 404 for another tenant", func(t *testing.T) {
+		// Not 200-with-empty: that would make a foreign node indistinguishable from one of your own
+		// that holds no records. Same contract as the run, tree, rename and remove cases above.
 		recordsURL := taxonomyURL(harness.server.URL, "/v1/taxonomy/nodes/"+ids.RootID.String()+"/records", otherQuery)
-
-		var resp models.TaxonomyNodeRecordsResponse
-		requestTaxonomyJSON(ctx, t, http.MethodGet, recordsURL, harness.apiKey, nil, http.StatusOK, &resp)
-		require.Empty(t, resp.Data)
+		requestTaxonomyJSON(ctx, t, http.MethodGet, recordsURL, harness.apiKey, nil, http.StatusNotFound, nil)
 	})
 }
 

tests/taxonomy_persistence_test.go

@@ -466,10 +466,27 @@ func TestTaxonomyRepository_ListNodeRecords(t *testing.T) {
 	require.Len(t, records, 1)
 	require.Equal(t, ids.FeedbackRecordID, records[0].ID)
 
-	// A different tenant sees nothing for the same node id.
-	otherTenantRecords, _, err := repo.ListNodeRecords(ctx, ids.RootID, "other-tenant-"+uuid.NewString(), 50)
+	// A different tenant cannot address the node at all — not even to be told it is empty.
+	_, _, err = repo.ListNodeRecords(ctx, ids.RootID, "other-tenant-"+uuid.NewString(), 50)
+	require.ErrorIs(t, err, huberrors.ErrNotFound, "node records must be tenant-scoped")
+
+	// An unknown node id is the same not-found, so ownership is never enumerable.
+	_, _, err = repo.ListNodeRecords(ctx, uuid.New(), scope.TenantID, 50)
+	require.ErrorIs(t, err, huberrors.ErrNotFound)
+
+	// A soft-removed node is not addressable either, matching rename and remove: the tree has already
+	// dropped it, so a caller asking for its records is working from stale state.
+	_, err = repo.RemoveNode(ctx, ids.LeafID, scope.TenantID, "actor-remove")
+	require.NoError(t, err)
+
+	_, _, err = repo.ListNodeRecords(ctx, ids.LeafID, scope.TenantID, 50)
+	require.ErrorIs(t, err, huberrors.ErrNotFound)
+
+	// The root survives and now genuinely holds no records, because the removed leaf carried the only
+	// cluster membership. That empty result is only meaningful because the cases above are 404s.
+	rootRecords, _, err := repo.ListNodeRecords(ctx, ids.RootID, scope.TenantID, 50)
 	require.NoError(t, err)
-	require.Empty(t, otherTenantRecords, "node records must be tenant-scoped")
+	require.Empty(t, rootRecords)
 }
 
 // TestTaxonomyRepository_TenantIsolation proves every tenant-scoped read and mutation refuses
@@ -501,6 +518,11 @@ func TestTaxonomyRepository_TenantIsolation(t *testing.T) {
 		require.ErrorIs(t, err, huberrors.ErrNotFound)
 	})
 
+	t.Run("node records refuse another tenant", func(t *testing.T) {
+		_, _, err := repo.ListNodeRecords(ctx, ids.RootID, otherTenant, 50)
+		require.ErrorIs(t, err, huberrors.ErrNotFound)
+	})
+
 	t.Run("rename and remove refuse another tenant", func(t *testing.T) {
 		_, err := repo.RenameNode(ctx, ids.BranchID, otherTenant, "attacker", "Hijacked")
 		require.ErrorIs(t, err, huberrors.ErrNotFound)