fix(vault): preserve team-scoped docs on team delete (#1356)

Deleting a team failed with SQLSTATE 23514 whenever it owned a
team-scoped vault document. The vault_documents.team_id FK is
ON DELETE SET NULL; when team_id became NULL the
vault_docs_team_null_scope_fix() trigger (migration 000043)
unconditionally set scope='personal'. Team docs have agent_id IS NULL,
so the resulting (personal, agent_id NULL) row violates the
vault_documents_scope_consistency CHECK and aborts the whole delete.

PostgreSQL: migration 000089 replaces the trigger function to pick a
valid target scope by ownership, and only rewrite genuinely team-scoped
rows so scope='custom' docs that merely carry a team_id are preserved:
  agent_id IS NOT NULL -> 'personal' (defensive: legacy dirty rows)
  agent_id IS NULL     -> 'shared'   (normal team docs)

SQLite: the same class of bug exists (FK SET NULL leaves scope='team'
with a NULL team_id and aborts on the CHECK), but SQLite fires no
trigger during the FK action, so a DB-level fix is impossible.
SQLiteTeamStore.DeleteTeam now converts team-scoped docs in the same
transaction before removing the team, scoped by tenant_id in the
tenant path to keep tenant isolation.

Adds regression tests on both engines and bumps RequiredSchemaVersion
to 89. SQLite needs no schema migration since the fix is in Go code.

internal/store/sqlitestore/teams.go

@@ -91,16 +91,46 @@ func (s *SQLiteTeamStore) UpdateTeam(ctx context.Context, teamID uuid.UUID, upda
 }
 
 func (s *SQLiteTeamStore) DeleteTeam(ctx context.Context, teamID uuid.UUID) error {
-	if store.IsCrossTenant(ctx) {
-		_, err := s.db.ExecContext(ctx, `DELETE FROM agent_teams WHERE id = ?`, teamID)
-		return err
-	}
+	crossTenant := store.IsCrossTenant(ctx)
 	tid := store.TenantIDFromContext(ctx)
-	if tid == uuid.Nil {
+	if !crossTenant && tid == uuid.Nil {
 		return fmt.Errorf("tenant_id required for delete")
 	}
-	_, err := s.db.ExecContext(ctx, `DELETE FROM agent_teams WHERE id = ? AND tenant_id = ?`, teamID, tid)
-	return err
+
+	// Team deletion must not fail when the team owns vault documents (issue #1077).
+	// The team_id FK is ON DELETE SET NULL, but SQLite fires no scope-fixing trigger
+	// during the FK action, so a team-scoped doc would be left as scope='team' with a
+	// NULL team_id and abort the delete on the vault_documents_scope_consistency CHECK.
+	// Convert team-scoped docs first (agent_id IS NULL -> 'shared'; the CASE guards
+	// any legacy 'team' rows that carry an agent_id). Custom-scope docs are left as-is;
+	// the FK SET NULL clears their team_id and 'custom' has no consistency constraint.
+	tx, err := s.db.BeginTx(ctx, nil)
+	if err != nil {
+		return err
+	}
+	defer func() { _ = tx.Rollback() }()
+
+	const fixScopeSQL = `UPDATE vault_documents
+		SET scope = CASE WHEN agent_id IS NOT NULL THEN 'personal' ELSE 'shared' END,
+		    team_id = NULL
+		WHERE team_id = ? AND scope = 'team'`
+	if crossTenant {
+		if _, err := tx.ExecContext(ctx, fixScopeSQL, teamID); err != nil {
+			return err
+		}
+		if _, err := tx.ExecContext(ctx, `DELETE FROM agent_teams WHERE id = ?`, teamID); err != nil {
+			return err
+		}
+		return tx.Commit()
+	}
+
+	if _, err := tx.ExecContext(ctx, fixScopeSQL+` AND tenant_id = ?`, teamID, tid); err != nil {
+		return err
+	}
+	if _, err := tx.ExecContext(ctx, `DELETE FROM agent_teams WHERE id = ? AND tenant_id = ?`, teamID, tid); err != nil {
+		return err
+	}
+	return tx.Commit()
 }
 
 func (s *SQLiteTeamStore) ListTeams(ctx context.Context) ([]store.TeamData, error) {

internal/store/sqlitestore/teams_delete_vault_scope_test.go

@@ -0,0 +1,94 @@
+//go:build sqlite || sqliteonly
+
+package sqlitestore
+
+import (
+	"context"
+	"database/sql"
+	"path/filepath"
+	"testing"
+
+	"github.com/google/uuid"
+
+	"github.com/nextlevelbuilder/goclaw/internal/store"
+)
+
+// TestSQLiteDeleteTeam_PreservesTeamVaultDocs is a regression test for issue #1077.
+//
+// Deleting a team must not fail when the team owns vault documents. Team-scoped
+// docs (scope='team', agent_id IS NULL) must be preserved by converting them to
+// scope='shared' with team_id NULL. Without the fix, the FK ON DELETE SET NULL
+// leaves scope='team' with a NULL team_id, which violates the
+// vault_documents_scope_consistency CHECK and aborts the whole delete.
+func TestSQLiteDeleteTeam_PreservesTeamVaultDocs(t *testing.T) {
+	db := newTeamVaultScopeDB(t)
+	tenantID := store.GenNewID()
+	mustExec(t, db, `INSERT INTO tenants (id, name, slug, status, settings, created_at, updated_at)
+		VALUES (?, 'test-tenant', 'test-tenant', 'active', '{}', datetime('now'), datetime('now'))`, tenantID)
+	agentID := store.GenNewID()
+	mustExec(t, db, `INSERT INTO agents (id, agent_key, owner_id, model, tenant_id)
+		VALUES (?, 'lead', 'owner', 'gpt', ?)`, agentID, tenantID)
+
+	teamID := store.GenNewID()
+	mustExec(t, db, `INSERT INTO agent_teams (id, name, lead_agent_id, created_by, tenant_id)
+		VALUES (?, 'team-a', ?, 'owner', ?)`, teamID, agentID, tenantID)
+
+	// Team-scoped doc (agent_id NULL) — the exact shape that triggers the bug.
+	teamDocID := store.GenNewID()
+	mustExec(t, db, `INSERT INTO vault_documents (id, tenant_id, team_id, scope, path)
+		VALUES (?, ?, ?, 'team', 'teams/a/note.md')`, teamDocID, tenantID, teamID)
+
+	// Custom-scope doc carrying team_id + agent_id — must NOT be clobbered.
+	customDocID := store.GenNewID()
+	mustExec(t, db, `INSERT INTO vault_documents (id, tenant_id, agent_id, team_id, scope, path)
+		VALUES (?, ?, ?, ?, 'custom', 'teams/a/custom.md')`, customDocID, tenantID, agentID, teamID)
+
+	teamStore := NewSQLiteTeamStore(db)
+	ctx := store.WithTenantID(context.Background(), tenantID)
+
+	if err := teamStore.DeleteTeam(ctx, teamID); err != nil {
+		t.Fatalf("DeleteTeam failed: %v", err)
+	}
+
+	// Team was actually deleted.
+	var count int
+	if err := db.QueryRow(`SELECT COUNT(*) FROM agent_teams WHERE id = ?`, teamID).Scan(&count); err != nil {
+		t.Fatalf("count teams: %v", err)
+	}
+	if count != 0 {
+		t.Fatalf("team not deleted, count=%d", count)
+	}
+
+	// Team doc preserved as 'shared'.
+	assertVaultScope(t, db, teamDocID, "shared", false)
+	// Custom doc keeps its scope; team_id cleared by FK SET NULL.
+	assertVaultScope(t, db, customDocID, "custom", false)
+}
+
+func assertVaultScope(t *testing.T, db *sql.DB, docID uuid.UUID, wantScope string, wantTeam bool) {
+	t.Helper()
+	var scope string
+	var teamID sql.NullString
+	if err := db.QueryRow(`SELECT scope, team_id FROM vault_documents WHERE id = ?`, docID).Scan(&scope, &teamID); err != nil {
+		t.Fatalf("query doc %s: %v", docID, err)
+	}
+	if scope != wantScope {
+		t.Errorf("doc %s scope = %q, want %q", docID, scope, wantScope)
+	}
+	if teamID.Valid != wantTeam {
+		t.Errorf("doc %s team_id present = %v, want %v (value=%q)", docID, teamID.Valid, wantTeam, teamID.String)
+	}
+}
+
+func newTeamVaultScopeDB(t *testing.T) *sql.DB {
+	t.Helper()
+	db, err := OpenDB(filepath.Join(t.TempDir(), "team_vault_scope.db"))
+	if err != nil {
+		t.Fatalf("OpenDB: %v", err)
+	}
+	t.Cleanup(func() { _ = db.Close() })
+	if err := EnsureSchema(db); err != nil {
+		t.Fatalf("EnsureSchema: %v", err)
+	}
+	return db
+}

internal/upgrade/version.go

@@ -2,4 +2,4 @@ package upgrade
 
 // RequiredSchemaVersion is the schema migration version this binary requires.
 // Bump this whenever adding a new SQL migration file.
-const RequiredSchemaVersion uint = 88
+const RequiredSchemaVersion uint = 89

migrations/000089_vault_team_delete_scope_fix.down.sql

@@ -0,0 +1,13 @@
+-- Revert to the migration 000043 behavior: unconditionally coerce scope to
+-- 'personal' when a team_id is cleared. NOTE: this reintroduces issue #1077 —
+-- team-scoped docs (agent_id IS NULL) will again violate the scope-consistency
+-- CHECK on team deletion.
+CREATE OR REPLACE FUNCTION vault_docs_team_null_scope_fix()
+RETURNS TRIGGER AS $$
+BEGIN
+    IF NEW.team_id IS NULL AND OLD.team_id IS NOT NULL THEN
+        NEW.scope := 'personal';
+    END IF;
+    RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;

migrations/000089_vault_team_delete_scope_fix.up.sql

@@ -0,0 +1,22 @@
+-- Fix issue #1077: deleting a team fails when it owns team-scoped vault docs.
+--
+-- vault_documents.team_id is a FK with ON DELETE SET NULL. When team_id becomes
+-- NULL, the trg_vault_docs_team_null_scope trigger auto-corrects scope. The original
+-- function (migration 000043) set scope='personal' unconditionally, but team-scoped
+-- docs have agent_id IS NULL, so the resulting (scope='personal', agent_id IS NULL)
+-- row violates vault_documents_scope_consistency (migration 000055/000056) and aborts
+-- the whole delete with SQLSTATE 23514.
+--
+-- Pick a scope that keeps the ownership invariant, and only rewrite genuinely
+-- team-scoped rows so scope='custom' docs that merely carry a team_id are left intact:
+--   agent_id IS NOT NULL -> 'personal' (defensive: tolerate legacy dirty rows)
+--   agent_id IS NULL     -> 'shared'   (normal team docs; the common case)
+CREATE OR REPLACE FUNCTION vault_docs_team_null_scope_fix()
+RETURNS TRIGGER AS $$
+BEGIN
+    IF NEW.team_id IS NULL AND OLD.team_id IS NOT NULL AND OLD.scope = 'team' THEN
+        NEW.scope := CASE WHEN NEW.agent_id IS NOT NULL THEN 'personal' ELSE 'shared' END;
+    END IF;
+    RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;

tests/integration/v3_vault_scope_check_test.go

@@ -3,6 +3,7 @@
 package integration
 
 import (
+	"database/sql"
 	"testing"
 
 	"github.com/google/uuid"
@@ -79,3 +80,63 @@ func TestStoreVault_ScopeCheck(t *testing.T) {
 		})
 	}
 }
+
+// TestStoreVault_TeamDeletePreservesDocs is a regression test for issue #1077.
+//
+// Deleting a team that owns vault documents must succeed. Team-scoped docs
+// (scope='team', agent_id IS NULL) are preserved as scope='shared' via the
+// vault_docs_team_null_scope_fix() trigger. Before the fix the trigger forced
+// scope='personal' unconditionally, which — combined with agent_id IS NULL —
+// violated vault_documents_scope_consistency and aborted the delete (SQLSTATE 23514).
+func TestStoreVault_TeamDeletePreservesDocs(t *testing.T) {
+	db := testDB(t)
+	tenantID, agentID := seedTenantAgent(t, db)
+	teamID, _ := seedTeam(t, db, tenantID, agentID)
+
+	suffix := uuid.New().String()[:8]
+	teamDocID := uuid.New().String()
+	customDocID := uuid.New().String()
+	t.Cleanup(func() {
+		db.Exec(`DELETE FROM vault_documents WHERE id IN ($1, $2)`, teamDocID, customDocID)
+	})
+
+	// Team-scoped doc (agent_id NULL) — the exact shape that triggers the bug.
+	if _, err := db.Exec(`INSERT INTO vault_documents
+		(id, tenant_id, team_id, scope, path, title, doc_type, content_hash)
+		VALUES ($1, $2, $3, 'team', $4, 'team-doc', 'note', 'h1')`,
+		teamDocID, tenantID, teamID, "team-del/"+suffix+"-team.md"); err != nil {
+		t.Fatalf("insert team doc: %v", err)
+	}
+	// Custom-scope doc carrying team_id + agent_id — must NOT be clobbered.
+	if _, err := db.Exec(`INSERT INTO vault_documents
+		(id, tenant_id, agent_id, team_id, scope, path, title, doc_type, content_hash)
+		VALUES ($1, $2, $3, $4, 'custom', $5, 'custom-doc', 'note', 'h2')`,
+		customDocID, tenantID, agentID, teamID, "team-del/"+suffix+"-custom.md"); err != nil {
+		t.Fatalf("insert custom doc: %v", err)
+	}
+
+	// Delete the team: exercises FK ON DELETE SET NULL → scope-fix trigger → CHECK.
+	if _, err := db.Exec(`DELETE FROM agent_teams WHERE id = $1`, teamID); err != nil {
+		t.Fatalf("delete team failed (issue #1077 regression): %v", err)
+	}
+
+	// Team doc preserved as 'shared', team_id cleared.
+	assertPGVaultScope(t, db, teamDocID, "shared")
+	// Custom doc keeps its scope, team_id cleared by FK SET NULL.
+	assertPGVaultScope(t, db, customDocID, "custom")
+}
+
+func assertPGVaultScope(t *testing.T, db *sql.DB, docID, wantScope string) {
+	t.Helper()
+	var scope string
+	var teamID sql.NullString
+	if err := db.QueryRow(`SELECT scope, team_id FROM vault_documents WHERE id = $1`, docID).Scan(&scope, &teamID); err != nil {
+		t.Fatalf("query doc %s: %v", docID, err)
+	}
+	if scope != wantScope {
+		t.Errorf("doc %s scope = %q, want %q", docID, scope, wantScope)
+	}
+	if teamID.Valid {
+		t.Errorf("doc %s team_id should be NULL after team delete, got %q", docID, teamID.String)
+	}
+}