diff --git a/internal/store/sqlitestore/teams.go b/internal/store/sqlitestore/teams.go index 5fa13a94..0d8f20b2 100644 --- a/internal/store/sqlitestore/teams.go +++ b/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) { diff --git a/internal/store/sqlitestore/teams_delete_vault_scope_test.go b/internal/store/sqlitestore/teams_delete_vault_scope_test.go new file mode 100644 index 00000000..8be700ba --- /dev/null +++ b/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 +} diff --git a/internal/upgrade/version.go b/internal/upgrade/version.go index ae995221..aa4dbc98 100644 --- a/internal/upgrade/version.go +++ b/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 diff --git a/migrations/000089_vault_team_delete_scope_fix.down.sql b/migrations/000089_vault_team_delete_scope_fix.down.sql new file mode 100644 index 00000000..7d17c5bd --- /dev/null +++ b/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; diff --git a/migrations/000089_vault_team_delete_scope_fix.up.sql b/migrations/000089_vault_team_delete_scope_fix.up.sql new file mode 100644 index 00000000..a6a629af --- /dev/null +++ b/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; diff --git a/tests/integration/v3_vault_scope_check_test.go b/tests/integration/v3_vault_scope_check_test.go index c02c584d..e9d6e9ff 100644 --- a/tests/integration/v3_vault_scope_check_test.go +++ b/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) + } +}