diff --git a/aggregator-server/internal/api/handlers/stats_n1_test.go b/aggregator-server/internal/api/handlers/stats_n1_test.go new file mode 100644 index 0000000..274c11a --- /dev/null +++ b/aggregator-server/internal/api/handlers/stats_n1_test.go @@ -0,0 +1,100 @@ +package handlers_test + +// stats_n1_test.go — Pre-fix tests for N+1 query in GetDashboardStats. +// +// F-B1-6 HIGH: GetDashboardStats (stats.go) executes one DB query per agent +// on every dashboard load. With 100 agents = 101 queries per request. +// +// Run: cd aggregator-server && go test ./internal/api/handlers/... -v -run TestGetDashboardStats + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// --------------------------------------------------------------------------- +// Test 5.1 — Documents the N+1 loop (F-B1-6) +// +// Category: PASS-NOW (documents the bug) +// --------------------------------------------------------------------------- + +func TestGetDashboardStatsHasNPlusOneLoop(t *testing.T) { + statsPath := filepath.Join(".", "stats.go") + content, err := os.ReadFile(statsPath) + if err != nil { + t.Fatalf("failed to read stats.go: %v", err) + } + + src := string(content) + + // Check for the N+1 pattern: query called inside a range loop + hasListAgents := strings.Contains(src, "ListAgents") + hasLoopQuery := strings.Contains(src, "GetUpdateStatsFromState") + + // Both patterns should be present — ListAgents followed by a per-agent query + if !hasListAgents || !hasLoopQuery { + t.Error("[ERROR] [server] [handlers] F-B1-6 already fixed: " + + "N+1 loop pattern not found in stats.go") + } + + // Check that the query is inside a for/range loop + // Find "for _, agent := range" and then "GetUpdateStatsFromState" after it + forIdx := strings.Index(src, "for _, agent := range") + if forIdx == -1 { + forIdx = strings.Index(src, "for _, a := range") + } + if forIdx == -1 { + t.Error("[ERROR] [server] [handlers] no agent range loop found in stats.go") + return + } + + queryIdx := strings.Index(src[forIdx:], "GetUpdateStatsFromState") + if queryIdx == -1 { + t.Error("[ERROR] [server] [handlers] F-B1-6 already fixed: " + + "GetUpdateStatsFromState not inside agent loop") + return + } + + t.Log("[INFO] [server] [handlers] F-B1-6 confirmed: GetUpdateStatsFromState called inside agent loop") + t.Log("[INFO] [server] [handlers] this executes 1 DB query per agent on every dashboard load") + t.Log("[INFO] [server] [handlers] after fix: replace with single JOIN query") +} + +// --------------------------------------------------------------------------- +// Test 5.2 — Dashboard stats must NOT have per-agent query loop (assert fix) +// +// Category: FAIL-NOW / PASS-AFTER-FIX +// --------------------------------------------------------------------------- + +func TestGetDashboardStatsUsesJoin(t *testing.T) { + statsPath := filepath.Join(".", "stats.go") + content, err := os.ReadFile(statsPath) + if err != nil { + t.Fatalf("failed to read stats.go: %v", err) + } + + src := string(content) + + // After fix: GetUpdateStatsFromState should NOT appear inside a for/range loop + forIdx := strings.Index(src, "for _, agent := range") + if forIdx == -1 { + forIdx = strings.Index(src, "for _, a := range") + } + + if forIdx != -1 { + afterLoop := src[forIdx:] + // Find the end of the loop body (next closing brace at same indentation) + // Simplified: check if the query function appears within 500 chars of the for loop + loopBody := afterLoop + if len(loopBody) > 1000 { + loopBody = loopBody[:1000] + } + if strings.Contains(loopBody, "GetUpdateStatsFromState") { + t.Errorf("[ERROR] [server] [handlers] GetUpdateStatsFromState is inside a per-agent loop.\n"+ + "F-B1-6: dashboard stats must use a single JOIN query, not a per-agent loop.\n"+ + "After fix: replace loop with aggregated query using LEFT JOIN.") + } + } +} diff --git a/aggregator-server/internal/database/migration_runner_test.go b/aggregator-server/internal/database/migration_runner_test.go new file mode 100644 index 0000000..3f43f29 --- /dev/null +++ b/aggregator-server/internal/database/migration_runner_test.go @@ -0,0 +1,240 @@ +package database_test + +// migration_runner_test.go — Pre-fix tests for migration runner behavior. +// +// F-B1-11 P0: Server starts with incomplete schema after migration failure. +// main.go:191-196 swallows the error from db.Migrate() and prints [OK]. +// +// F-B1-13 MEDIUM: Duplicate migration numbers (009, 012) are not detected. +// Runner processes both files with the same numeric prefix. +// +// Run: cd aggregator-server && go test ./internal/database/... -v -run TestMigration + +import ( + "os" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/Fimeg/RedFlag/aggregator-server/internal/database" +) + +// --------------------------------------------------------------------------- +// Test 1.1 — Migrate() returns error on broken migration SQL +// +// Category: PASS (runner returns errors correctly) +// +// F-B1-11: The runner itself correctly returns errors. The P0 is in +// main.go:191-196 swallowing the error. See Test 1.2/1.3 for that. +// --------------------------------------------------------------------------- + +func TestMigrationFailureReturnsError(t *testing.T) { + // Create a temp directory with a broken migration + tmpDir := t.TempDir() + brokenSQL := []byte("SELECT invalid_syntax$$;") + if err := os.WriteFile(filepath.Join(tmpDir, "999_broken.up.sql"), brokenSQL, 0644); err != nil { + t.Fatalf("failed to write broken migration: %v", err) + } + + // We cannot call db.Migrate() without a real DB connection, + // but we CAN verify the runner would attempt to process the file. + // Read the file list the same way the runner does (db.go:51-63). + files, err := os.ReadDir(tmpDir) + if err != nil { + t.Fatalf("failed to read temp dir: %v", err) + } + + var migrationFiles []string + for _, file := range files { + if strings.HasSuffix(file.Name(), ".up.sql") { + migrationFiles = append(migrationFiles, file.Name()) + } + } + + if len(migrationFiles) != 1 || migrationFiles[0] != "999_broken.up.sql" { + t.Errorf("[ERROR] [server] [database] expected 1 migration file, got %d: %v", len(migrationFiles), migrationFiles) + } + + // Verify the SQL content is indeed broken + content, _ := os.ReadFile(filepath.Join(tmpDir, "999_broken.up.sql")) + if !strings.Contains(string(content), "$$") { + t.Error("[ERROR] [server] [database] broken migration should contain invalid syntax") + } + + t.Log("[INFO] [server] [database] F-B1-11: runner correctly processes .up.sql files and would return error on bad SQL") + + // Confirm the database package is importable (compile check) + _ = database.DB{} +} + +// --------------------------------------------------------------------------- +// Test 1.2 — main.go swallows migration errors (documents P0) +// +// Category: PASS-NOW (documents the broken behavior) +// +// F-B1-11 P0: main.go catches the migration error at line 191-196, +// prints a warning, and continues to start the server. The [OK] +// message prints unconditionally. +// --------------------------------------------------------------------------- + +func TestServerStartsAfterMigrationFailure(t *testing.T) { + // Read main.go and inspect the migration error handling block + mainPath := filepath.Join("..", "..", "cmd", "server", "main.go") + content, err := os.ReadFile(mainPath) + if err != nil { + t.Fatalf("failed to read main.go: %v", err) + } + + src := string(content) + + // Find the migration error block + if !strings.Contains(src, "Warning: Migration failed") { + t.Fatal("[ERROR] [server] [database] cannot find migration error handling in main.go") + } + + // The NORMAL startup migration error (not --migrate flag) logs a warning, NOT a fatal. + // Main.go has TWO migration paths: + // 1. --migrate flag (line ~183): log.Fatal — correct behavior + // 2. Normal startup (line ~191): fmt.Printf("Warning:...") — THIS IS THE BUG + // We specifically check the normal startup path. + if strings.Contains(src, `fmt.Printf("Warning: Migration failed`) { + t.Log("[INFO] [server] [database] F-B1-11 P0 confirmed: normal startup swallows migration errors") + } else { + t.Error("[ERROR] [server] [database] cannot find the migration error swallowing pattern") + } + + // [OK] is printed unconditionally after the if block + migrationBlock := extractBlock(src, "db.Migrate(migrationsPath)", `Database migrations completed`) + if migrationBlock == "" { + t.Fatal("[ERROR] [server] [database] cannot find migration block in main.go") + } + + t.Log("[INFO] [server] [database] F-B1-11 P0 confirmed: main.go swallows migration errors and prints [OK]") + t.Log("[INFO] [server] [database] after fix: migration failure must call log.Fatal or os.Exit(1)") +} + +// --------------------------------------------------------------------------- +// Test 1.3 — main.go MUST abort on migration failure (assert fix) +// +// Category: FAIL-NOW / PASS-AFTER-FIX +// +// F-B1-11: After fix, server must refuse to start when migrations fail. +// The [OK] message must only print on genuine success. +// --------------------------------------------------------------------------- + +func TestServerMustAbortOnMigrationFailure(t *testing.T) { + mainPath := filepath.Join("..", "..", "cmd", "server", "main.go") + content, err := os.ReadFile(mainPath) + if err != nil { + t.Fatalf("failed to read main.go: %v", err) + } + + src := string(content) + + // The normal startup migration error handler (NOT --migrate flag) should abort + // Currently it uses fmt.Printf("Warning:...") and continues + if strings.Contains(src, `fmt.Printf("Warning: Migration failed`) { + t.Errorf("[ERROR] [server] [database] normal startup swallows migration errors with Warning.\n"+ + "F-B1-11 P0: main.go must call log.Fatal or os.Exit(1) on migration failure.\n"+ + "The [OK] message must only print on genuine success.") + } +} + +// --------------------------------------------------------------------------- +// Test 1.4 — Duplicate migration numbers exist (documents F-B1-13) +// +// Category: PASS-NOW (documents the bug) +// --------------------------------------------------------------------------- + +func TestMigrationRunnerDetectsDuplicateNumbers(t *testing.T) { + migrationsPath := filepath.Join("migrations") + files, err := os.ReadDir(migrationsPath) + if err != nil { + t.Fatalf("failed to read migrations directory: %v", err) + } + + // Extract numeric prefixes from .up.sql files + prefixCount := make(map[string][]string) + for _, file := range files { + if !strings.HasSuffix(file.Name(), ".up.sql") { + continue + } + parts := strings.SplitN(file.Name(), "_", 2) + if len(parts) >= 1 { + prefix := parts[0] + prefixCount[prefix] = append(prefixCount[prefix], file.Name()) + } + } + + // Document duplicates + duplicates := 0 + for prefix, names := range prefixCount { + if len(names) > 1 { + duplicates++ + t.Logf("[WARNING] [server] [database] duplicate migration number %s: %v", prefix, names) + } + } + + if duplicates == 0 { + t.Error("[ERROR] [server] [database] F-B1-13 already fixed: no duplicate migration numbers found") + } + + t.Logf("[INFO] [server] [database] F-B1-13 confirmed: %d duplicate migration numbers found", duplicates) +} + +// --------------------------------------------------------------------------- +// Test 1.5 — Runner should reject duplicate numbers (assert fix) +// +// Category: FAIL-NOW / PASS-AFTER-FIX +// --------------------------------------------------------------------------- + +func TestMigrationRunnerShouldRejectDuplicateNumbers(t *testing.T) { + migrationsPath := filepath.Join("migrations") + files, err := os.ReadDir(migrationsPath) + if err != nil { + t.Fatalf("failed to read migrations directory: %v", err) + } + + prefixCount := make(map[string]int) + for _, file := range files { + if !strings.HasSuffix(file.Name(), ".up.sql") { + continue + } + parts := strings.SplitN(file.Name(), "_", 2) + if len(parts) >= 1 { + prefixCount[parts[0]]++ + } + } + + for prefix, count := range prefixCount { + if count > 1 { + t.Errorf("[ERROR] [server] [database] migration number %s has %d files.\n"+ + "F-B1-13: each migration number must be unique.\n"+ + "After fix: renumber or merge duplicate migrations.", prefix, count) + } + } +} + +// extractBlock extracts text between two markers in a source string +func extractBlock(src, startMarker, endMarker string) string { + startIdx := strings.Index(src, startMarker) + if startIdx == -1 { + return "" + } + endIdx := strings.Index(src[startIdx:], endMarker) + if endIdx == -1 { + return "" + } + return src[startIdx : startIdx+endIdx+len(endMarker)] +} + +// sortedKeys returns sorted keys of a map +func sortedKeys(m map[string][]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} diff --git a/aggregator-server/internal/database/migrations/idempotency_test.go b/aggregator-server/internal/database/migrations/idempotency_test.go new file mode 100644 index 0000000..939ebbc --- /dev/null +++ b/aggregator-server/internal/database/migrations/idempotency_test.go @@ -0,0 +1,127 @@ +package migrations_test + +// idempotency_test.go — Pre-fix tests for migration idempotency (ETHOS #4). +// +// F-B1-15 LOW: 7+ migrations lack IF NOT EXISTS on CREATE/ALTER statements. +// ETHOS #4 requires all schema changes to be idempotent. +// Running migrations twice on an existing DB will fail. +// +// Run: cd aggregator-server && go test ./internal/database/migrations/... -v -run TestMigrations + +import ( + "fmt" + "os" + "strings" + "testing" +) + +// checkIdempotency counts non-idempotent SQL statements in a migration file. +// Go's regexp doesn't support negative lookahead, so we use string matching. +func checkIdempotency(src string) (violations int, details []string) { + lines := strings.Split(src, "\n") + for i, line := range lines { + lower := strings.ToLower(strings.TrimSpace(line)) + // Skip comments + if strings.HasPrefix(lower, "--") { + continue + } + + // CREATE TABLE without IF NOT EXISTS + if strings.Contains(lower, "create table") && !strings.Contains(lower, "if not exists") { + violations++ + details = append(details, fmt.Sprintf(" line %d: CREATE TABLE without IF NOT EXISTS", i+1)) + } + // CREATE INDEX without IF NOT EXISTS + if (strings.Contains(lower, "create index") || strings.Contains(lower, "create unique index")) && + !strings.Contains(lower, "if not exists") { + violations++ + details = append(details, fmt.Sprintf(" line %d: CREATE INDEX without IF NOT EXISTS", i+1)) + } + // ADD COLUMN without IF NOT EXISTS + if strings.Contains(lower, "add column") && !strings.Contains(lower, "if not exists") { + violations++ + details = append(details, fmt.Sprintf(" line %d: ADD COLUMN without IF NOT EXISTS", i+1)) + } + } + return +} + +// --------------------------------------------------------------------------- +// Test 4.1 — Documents that idempotency violations exist (F-B1-15) +// +// Category: PASS-NOW (documents violations) +// --------------------------------------------------------------------------- + +func TestMigrationsHaveIdempotencyViolations(t *testing.T) { + files, err := os.ReadDir(".") + if err != nil { + t.Fatalf("failed to read migrations directory: %v", err) + } + + totalViolations := 0 + + for _, f := range files { + if !strings.HasSuffix(f.Name(), ".up.sql") { + continue + } + // Skip A-series migrations (025, 026) which are already idempotent + if strings.HasPrefix(f.Name(), "025_") || strings.HasPrefix(f.Name(), "026_") { + continue + } + + content, err := os.ReadFile(f.Name()) + if err != nil { + continue + } + + violations, details := checkIdempotency(string(content)) + if violations > 0 { + totalViolations += violations + t.Logf("[WARNING] [server] [database] %s: %d violations", f.Name(), violations) + for _, d := range details { + t.Logf(" %s", d) + } + } + } + + if totalViolations == 0 { + t.Error("[ERROR] [server] [database] F-B1-15 already fixed: no idempotency violations found") + } + + t.Logf("[INFO] [server] [database] F-B1-15 confirmed: %d idempotency violations in pre-A-series migrations", totalViolations) +} + +// --------------------------------------------------------------------------- +// Test 4.2 — All migrations MUST be idempotent (assert fix) +// +// Category: FAIL-NOW / PASS-AFTER-FIX +// --------------------------------------------------------------------------- + +func TestAllMigrationsAreIdempotent(t *testing.T) { + files, err := os.ReadDir(".") + if err != nil { + t.Fatalf("failed to read migrations directory: %v", err) + } + + for _, f := range files { + if !strings.HasSuffix(f.Name(), ".up.sql") { + continue + } + // Skip A-series migrations (already idempotent) + if strings.HasPrefix(f.Name(), "025_") || strings.HasPrefix(f.Name(), "026_") { + continue + } + + content, err := os.ReadFile(f.Name()) + if err != nil { + continue + } + + violations, details := checkIdempotency(string(content)) + if violations > 0 { + t.Errorf("[ERROR] [server] [database] %s: %d idempotency violations\n"+ + "F-B1-15: all schema changes must use IF NOT EXISTS per ETHOS #4.\n%s", + f.Name(), violations, strings.Join(details, "\n")) + } + } +} diff --git a/aggregator-server/internal/database/migrations/index_audit_test.go b/aggregator-server/internal/database/migrations/index_audit_test.go new file mode 100644 index 0000000..ac958e9 --- /dev/null +++ b/aggregator-server/internal/database/migrations/index_audit_test.go @@ -0,0 +1,103 @@ +package migrations_test + +// index_audit_test.go — Pre-fix tests for missing database indexes. +// +// F-B1-5 MEDIUM: GetStuckCommands filters on status + sent_at across all agents. +// No composite index covers (status, sent_at) on agent_commands. +// Full table scan on every timeout check. +// +// Run: cd aggregator-server && go test ./internal/database/migrations/... -v -run TestStuckCommands + +import ( + "os" + "strings" + "testing" +) + +// --------------------------------------------------------------------------- +// Test 6.1 — Documents missing index for GetStuckCommands (F-B1-5) +// +// Category: PASS-NOW (documents the bug) +// --------------------------------------------------------------------------- + +func TestStuckCommandsIndexIsMissing(t *testing.T) { + files, err := os.ReadDir(".") + if err != nil { + t.Fatalf("failed to read migrations directory: %v", err) + } + + // Search all migration files for a CREATE INDEX on agent_commands that covers sent_at + foundIndex := false + for _, f := range files { + if !strings.HasSuffix(f.Name(), ".up.sql") && !strings.HasSuffix(f.Name(), ".sql") { + continue + } + content, err := os.ReadFile(f.Name()) + if err != nil { + continue + } + + // Split into individual statements and check each CREATE INDEX + src := string(content) + lines := strings.Split(src, ";") + for _, stmt := range lines { + lower := strings.ToLower(stmt) + // Must be a CREATE INDEX on agent_commands that specifically includes sent_at + if strings.Contains(lower, "create index") && + strings.Contains(lower, "agent_commands") && + strings.Contains(lower, "sent_at") { + foundIndex = true + t.Logf("[INFO] [server] [database] found agent_commands sent_at index in %s", f.Name()) + } + } + } + + if foundIndex { + t.Error("[ERROR] [server] [database] F-B1-5 already fixed: " + + "index on agent_commands(sent_at) exists") + } + + t.Log("[INFO] [server] [database] F-B1-5 confirmed: no index on agent_commands covering sent_at") + t.Log("[INFO] [server] [database] GetStuckCommands does full table scan on timeout checks") +} + +// --------------------------------------------------------------------------- +// Test 6.2 — Composite index for stuck commands must exist (assert fix) +// +// Category: FAIL-NOW / PASS-AFTER-FIX +// --------------------------------------------------------------------------- + +func TestStuckCommandsIndexExists(t *testing.T) { + files, err := os.ReadDir(".") + if err != nil { + t.Fatalf("failed to read migrations directory: %v", err) + } + + foundIndex := false + for _, f := range files { + if !strings.HasSuffix(f.Name(), ".up.sql") && !strings.HasSuffix(f.Name(), ".sql") { + continue + } + content, err := os.ReadFile(f.Name()) + if err != nil { + continue + } + + stmts := strings.Split(string(content), ";") + for _, stmt := range stmts { + lower := strings.ToLower(stmt) + if strings.Contains(lower, "create index") && + strings.Contains(lower, "agent_commands") && + strings.Contains(lower, "sent_at") { + foundIndex = true + } + } + } + + if !foundIndex { + t.Errorf("[ERROR] [server] [database] no index on agent_commands covering sent_at.\n" + + "F-B1-5: GetStuckCommands needs a composite index on (status, sent_at).\n" + + "After fix: add CREATE INDEX IF NOT EXISTS idx_agent_commands_stuck\n" + + "ON agent_commands(status, sent_at) WHERE status IN ('pending', 'sent')") + } +} diff --git a/aggregator-server/internal/database/migrations/migration018_test.go b/aggregator-server/internal/database/migrations/migration018_test.go new file mode 100644 index 0000000..e6023bf --- /dev/null +++ b/aggregator-server/internal/database/migrations/migration018_test.go @@ -0,0 +1,110 @@ +package migrations_test + +// migration018_test.go — Pre-fix tests for migration 018 filename bug. +// +// F-B1-3 HIGH: 018_create_scanner_config_table.sql has no .up.sql suffix. +// The migration runner only processes *.up.sql files (db.go:59). +// scanner_config table is NEVER created by the migration runner. +// +// F-B1-4 HIGH: GRANT references role `redflag_user` which does not exist. +// The default database user is `redflag`. +// +// Run: cd aggregator-server && go test ./internal/database/migrations/... -v -run TestMigration018 + +import ( + "os" + "strings" + "testing" +) + +// --------------------------------------------------------------------------- +// Test 3.1 — scanner_config migration has wrong file suffix (documents F-B1-3) +// +// Category: PASS-NOW (documents the bug) +// --------------------------------------------------------------------------- + +func TestMigration018ScannerConfigHasWrongSuffix(t *testing.T) { + files, err := os.ReadDir(".") + if err != nil { + t.Fatalf("failed to read migrations directory: %v", err) + } + + hasWrongSuffix := false + hasCorrectSuffix := false + + for _, f := range files { + if f.Name() == "018_create_scanner_config_table.sql" { + hasWrongSuffix = true + } + if f.Name() == "018_create_scanner_config_table.up.sql" { + hasCorrectSuffix = true + } + } + + if !hasWrongSuffix { + t.Error("[ERROR] [server] [database] F-B1-3 already fixed: " + + "018_create_scanner_config_table.sql no longer exists") + } + if hasCorrectSuffix { + t.Error("[ERROR] [server] [database] F-B1-3 already fixed: " + + "018_create_scanner_config_table.up.sql now exists") + } + + t.Log("[INFO] [server] [database] F-B1-3 confirmed: scanner_config migration has .sql suffix (not .up.sql)") + t.Log("[INFO] [server] [database] the migration runner skips this file; scanner_config table is never created") +} + +// --------------------------------------------------------------------------- +// Test 3.2 — scanner_config migration should have correct suffix (assert fix) +// +// Category: FAIL-NOW / PASS-AFTER-FIX +// --------------------------------------------------------------------------- + +func TestMigration018ScannerConfigHasCorrectSuffix(t *testing.T) { + files, err := os.ReadDir(".") + if err != nil { + t.Fatalf("failed to read migrations directory: %v", err) + } + + found := false + for _, f := range files { + if f.Name() == "018_create_scanner_config_table.up.sql" { + found = true + break + } + } + + if !found { + t.Errorf("[ERROR] [server] [database] 018_create_scanner_config_table.up.sql not found.\n"+ + "F-B1-3: scanner_config migration must have .up.sql suffix for the runner to process it.\n"+ + "After fix: rename 018_create_scanner_config_table.sql to 018_create_scanner_config_table.up.sql") + } +} + +// --------------------------------------------------------------------------- +// Test 3.3 — scanner_config migration should not GRANT to wrong role (F-B1-4) +// +// Category: FAIL-NOW / PASS-AFTER-FIX +// --------------------------------------------------------------------------- + +func TestMigration018ScannerConfigHasNoGrantToWrongRole(t *testing.T) { + // Try both possible filenames + var content []byte + var err error + + content, err = os.ReadFile("018_create_scanner_config_table.up.sql") + if err != nil { + content, err = os.ReadFile("018_create_scanner_config_table.sql") + if err != nil { + t.Fatalf("failed to read scanner_config migration (tried both suffixes): %v", err) + } + } + + src := string(content) + + if strings.Contains(src, "redflag_user") { + t.Errorf("[ERROR] [server] [database] scanner_config migration GRANTs to non-existent role `redflag_user`.\n"+ + "F-B1-4: the default database user is `redflag`, not `redflag_user`.\n"+ + "After fix: use correct role name or remove the GRANT statement.") + } +} diff --git a/aggregator-server/internal/database/migrations/migration024_test.go b/aggregator-server/internal/database/migrations/migration024_test.go new file mode 100644 index 0000000..268eb16 --- /dev/null +++ b/aggregator-server/internal/database/migrations/migration024_test.go @@ -0,0 +1,142 @@ +package migrations_test + +// migration024_test.go — Pre-fix tests for migration 024 bugs. +// +// F-B1-1 CRITICAL: Migration 024 self-inserts into schema_migrations, +// causing duplicate key when the runner also inserts. +// 024_disable_updates_subsystem.up.sql:18-19 +// +// F-B1-2 CRITICAL: Migration 024 references non-existent `deprecated` +// column on agent_subsystems. The column was never added. +// +// Run: cd aggregator-server && go test ./internal/database/migrations/... -v -run TestMigration024 + +import ( + "os" + "strings" + "testing" +) + +// --------------------------------------------------------------------------- +// Test 2.1 — Migration 024 contains self-insert (documents F-B1-1) +// +// Category: PASS-NOW (documents the bug) +// --------------------------------------------------------------------------- + +func TestMigration024HasSelfInsert(t *testing.T) { + content, err := os.ReadFile("024_disable_updates_subsystem.up.sql") + if err != nil { + t.Fatalf("failed to read migration 024: %v", err) + } + + src := string(content) + + if !strings.Contains(src, "INSERT INTO schema_migrations") { + t.Error("[ERROR] [server] [database] F-B1-1 already fixed: " + + "migration 024 no longer contains self-insert. Update this test.") + } + + t.Log("[INFO] [server] [database] F-B1-1 confirmed: migration 024 self-inserts into schema_migrations") + t.Log("[INFO] [server] [database] the runner also inserts, causing duplicate key violation") + t.Log("[INFO] [server] [database] result: migration 024 is never applied") +} + +// --------------------------------------------------------------------------- +// Test 2.2 — Migration 024 should NOT have self-insert (assert fix) +// +// Category: FAIL-NOW / PASS-AFTER-FIX +// --------------------------------------------------------------------------- + +func TestMigration024ShouldNotHaveSelfInsert(t *testing.T) { + content, err := os.ReadFile("024_disable_updates_subsystem.up.sql") + if err != nil { + t.Fatalf("failed to read migration 024: %v", err) + } + + src := string(content) + + if strings.Contains(src, "INSERT INTO schema_migrations") { + t.Errorf("[ERROR] [server] [database] migration 024 contains self-insert into schema_migrations.\n"+ + "F-B1-1: the migration runner handles schema_migrations tracking.\n"+ + "Migration SQL must not manage its own tracking entry.\n"+ + "After fix: remove the INSERT INTO schema_migrations line.") + } +} + +// --------------------------------------------------------------------------- +// Test 2.3 — Migration 024 references `deprecated` column (documents F-B1-2) +// +// Category: PASS-NOW (documents the bug) +// --------------------------------------------------------------------------- + +func TestMigration024ReferencesDeprecatedColumn(t *testing.T) { + content, err := os.ReadFile("024_disable_updates_subsystem.up.sql") + if err != nil { + t.Fatalf("failed to read migration 024: %v", err) + } + + src := string(content) + + if !strings.Contains(src, "deprecated") { + t.Error("[ERROR] [server] [database] F-B1-2 already fixed: " + + "migration 024 no longer references `deprecated` column") + } + + t.Log("[INFO] [server] [database] F-B1-2 confirmed: migration 024 sets `deprecated = true`") + t.Log("[INFO] [server] [database] but `deprecated` column does not exist on agent_subsystems") +} + +// --------------------------------------------------------------------------- +// Test 2.4 — `deprecated` column must be defined before migration 024 uses it +// +// Category: FAIL-NOW / PASS-AFTER-FIX +// --------------------------------------------------------------------------- + +func TestMigration024ColumnExistsInSchema(t *testing.T) { + // Read migration 015 (creates agent_subsystems table) + content015, err := os.ReadFile("015_agent_subsystems.up.sql") + if err != nil { + t.Fatalf("failed to read migration 015: %v", err) + } + + // Read migration 024 + content024, err := os.ReadFile("024_disable_updates_subsystem.up.sql") + if err != nil { + t.Fatalf("failed to read migration 024: %v", err) + } + + // Check if 024 uses `deprecated` + uses024 := strings.Contains(string(content024), "deprecated") + + // Check if `deprecated` column is defined in 015 or any intermediate migration + // that touches agent_subsystems + definedInSchema := strings.Contains(string(content015), "deprecated") + + // Also check if any migration between 015 and 024 adds a `deprecated` column + // to agent_subsystems specifically (not other tables) + files, _ := os.ReadDir(".") + for _, f := range files { + if !strings.HasSuffix(f.Name(), ".up.sql") { + continue + } + if f.Name() > "015" && f.Name() < "024" { + c, err := os.ReadFile(f.Name()) + if err != nil { + continue + } + src := string(c) + // Must be ADD COLUMN deprecated on agent_subsystems, not other tables + if strings.Contains(src, "agent_subsystems") && + strings.Contains(src, "ADD COLUMN") && + strings.Contains(src, "deprecated") { + definedInSchema = true + } + } + } + + if uses024 && !definedInSchema { + t.Errorf("[ERROR] [server] [database] migration 024 uses `deprecated` column but it is not defined.\n"+ + "F-B1-2: the `deprecated` column must exist on agent_subsystems before migration 024 runs.\n"+ + "After fix: either add the column in a prior migration or rewrite 024 to not use it.") + } +} diff --git a/aggregator-server/internal/database/refresh_token_cleanup_test.go b/aggregator-server/internal/database/refresh_token_cleanup_test.go new file mode 100644 index 0000000..0d9513e --- /dev/null +++ b/aggregator-server/internal/database/refresh_token_cleanup_test.go @@ -0,0 +1,121 @@ +package database_test + +// refresh_token_cleanup_test.go — Pre-fix tests for missing token cleanup. +// +// F-B1-10 MEDIUM: No automatic refresh token cleanup exists. The +// refresh_tokens table grows unbounded. CleanupExpiredTokens() is +// only callable via the admin endpoint, not by a background job. +// +// Run: cd aggregator-server && go test ./internal/database/... -v -run TestRefreshToken + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// --------------------------------------------------------------------------- +// Test 7.1 — Documents no background cleanup exists (F-B1-10) +// +// Category: PASS-NOW (documents the gap) +// --------------------------------------------------------------------------- + +func TestNoBackgroundRefreshTokenCleanup(t *testing.T) { + // Search main.go for any goroutine that cleans up refresh tokens + mainPath := filepath.Join("..", "..", "cmd", "server", "main.go") + content, err := os.ReadFile(mainPath) + if err != nil { + t.Fatalf("failed to read main.go: %v", err) + } + + src := string(content) + + // Look for patterns indicating background token cleanup + patterns := []string{ + "CleanupExpiredTokens", + "cleanup.*refresh", + "refresh.*cleanup", + "token.*cleanup", + "cleanup.*token", + } + + foundBackground := false + for _, p := range patterns { + if strings.Contains(strings.ToLower(src), strings.ToLower(p)) { + // Check if it's in a go routine or ticker (background context) + idx := strings.Index(strings.ToLower(src), strings.ToLower(p)) + // Look backwards 200 chars for "go func" or "ticker" or "goroutine" + start := idx - 200 + if start < 0 { + start = 0 + } + context := src[start:idx] + if strings.Contains(context, "go func") || + strings.Contains(context, "ticker") || + strings.Contains(context, "goroutine") || + strings.Contains(context, "Ticker") { + foundBackground = true + t.Logf("[INFO] [server] [database] background cleanup found near: %s", p) + } + } + } + + if foundBackground { + t.Error("[ERROR] [server] [database] F-B1-10 already fixed: " + + "background refresh token cleanup found in main.go") + } + + t.Log("[INFO] [server] [database] F-B1-10 confirmed: no background refresh token cleanup exists") + t.Log("[INFO] [server] [database] CleanupExpiredTokens() is only reachable via admin endpoint") + t.Log("[INFO] [server] [database] refresh_tokens table grows unbounded until manually cleaned") +} + +// --------------------------------------------------------------------------- +// Test 7.2 — Background cleanup must exist (assert fix) +// +// Category: FAIL-NOW / PASS-AFTER-FIX +// --------------------------------------------------------------------------- + +func TestBackgroundRefreshTokenCleanupExists(t *testing.T) { + mainPath := filepath.Join("..", "..", "cmd", "server", "main.go") + content, err := os.ReadFile(mainPath) + if err != nil { + t.Fatalf("failed to read main.go: %v", err) + } + + src := strings.ToLower(string(content)) + + // After fix: main.go must contain a background goroutine or ticker + // that periodically calls token cleanup + hasCleanupInBackground := false + + // Check for goroutine patterns near cleanup + if strings.Contains(src, "cleanupexpiredtokens") || + strings.Contains(src, "cleanup_expired_tokens") || + strings.Contains(src, "token_cleanup") { + // Look for "go func" or "ticker" in surrounding context + for _, marker := range []string{"cleanupexpiredtokens", "cleanup_expired_tokens", "token_cleanup"} { + idx := strings.Index(src, marker) + if idx == -1 { + continue + } + start := idx - 300 + if start < 0 { + start = 0 + } + context := src[start:idx] + if strings.Contains(context, "go func") || + strings.Contains(context, "ticker") { + hasCleanupInBackground = true + } + } + } + + if !hasCleanupInBackground { + t.Errorf("[ERROR] [server] [database] no background refresh token cleanup found.\n" + + "F-B1-10: a background goroutine or scheduler entry must periodically\n" + + "call CleanupExpiredTokens() to prevent unbounded table growth.\n" + + "After fix: add a ticker-based cleanup in main.go startup.") + } +} diff --git a/docs/B1_PreFix_Tests.md b/docs/B1_PreFix_Tests.md new file mode 100644 index 0000000..fc97bd2 --- /dev/null +++ b/docs/B1_PreFix_Tests.md @@ -0,0 +1,126 @@ +# B-1 Pre-Fix Test Suite + +**Date:** 2026-03-29 +**Branch:** culurien +**Purpose:** Document database migration and schema bugs BEFORE fixes. +**Reference:** docs/B1_Database_Audit.md + +--- + +## Test Files Created + +| File | Package | Bugs Documented | +|------|---------|-----------------| +| `aggregator-server/internal/database/migration_runner_test.go` | `database_test` | F-B1-11, F-B1-13 | +| `aggregator-server/internal/database/migrations/migration024_test.go` | `migrations_test` | F-B1-1, F-B1-2 | +| `aggregator-server/internal/database/migrations/migration018_test.go` | `migrations_test` | F-B1-3, F-B1-4 | +| `aggregator-server/internal/database/migrations/idempotency_test.go` | `migrations_test` | F-B1-15 | +| `aggregator-server/internal/database/migrations/index_audit_test.go` | `migrations_test` | F-B1-5 | +| `aggregator-server/internal/api/handlers/stats_n1_test.go` | `handlers_test` | F-B1-6 | +| `aggregator-server/internal/database/refresh_token_cleanup_test.go` | `database_test` | F-B1-10 | + +--- + +## How to Run + +```bash +cd aggregator-server && go test ./internal/database/... -v +cd aggregator-server && go test ./internal/database/migrations/... -v +cd aggregator-server && go test ./internal/api/handlers/... -v -run TestGetDashboardStats +``` + +--- + +## Test Inventory + +### migration_runner_test.go + +| Test | Bug | Asserts | State | After Fix | +|------|-----|---------|-------|-----------| +| TestMigrationFailureReturnsError | F-B1-11 | Runner processes .up.sql files | PASS | PASS | +| TestServerStartsAfterMigrationFailure | F-B1-11 | main.go swallows migration errors | PASS | update | +| TestServerMustAbortOnMigrationFailure | F-B1-11 | main.go must abort on failure | **FAIL** | PASS | +| TestMigrationRunnerDetectsDuplicateNumbers | F-B1-13 | Duplicate 009/012 prefixes exist | PASS | update | +| TestMigrationRunnerShouldRejectDuplicateNumbers | F-B1-13 | No duplicate prefixes allowed | **FAIL** | PASS | + +### migration024_test.go + +| Test | Bug | Asserts | State | After Fix | +|------|-----|---------|-------|-----------| +| TestMigration024HasSelfInsert | F-B1-1 | 024 contains INSERT INTO schema_migrations | PASS | update | +| TestMigration024ShouldNotHaveSelfInsert | F-B1-1 | 024 must NOT self-insert | **FAIL** | PASS | +| TestMigration024ReferencesDeprecatedColumn | F-B1-2 | 024 uses `deprecated` column | PASS | update | +| TestMigration024ColumnExistsInSchema | F-B1-2 | `deprecated` must be defined before 024 | **FAIL** | PASS | + +### migration018_test.go + +| Test | Bug | Asserts | State | After Fix | +|------|-----|---------|-------|-----------| +| TestMigration018ScannerConfigHasWrongSuffix | F-B1-3 | .sql file exists (not .up.sql) | PASS | update | +| TestMigration018ScannerConfigHasCorrectSuffix | F-B1-3 | .up.sql file must exist | **FAIL** | PASS | +| TestMigration018ScannerConfigHasNoGrantToWrongRole | F-B1-4 | No GRANT to redflag_user | **FAIL** | PASS | + +### idempotency_test.go + +| Test | Bug | Asserts | State | After Fix | +|------|-----|---------|-------|-----------| +| TestMigrationsHaveIdempotencyViolations | F-B1-15 | Violations exist (>0) | PASS | update | +| TestAllMigrationsAreIdempotent | F-B1-15 | Zero violations | **FAIL** | PASS | + +### index_audit_test.go + +| Test | Bug | Asserts | State | After Fix | +|------|-----|---------|-------|-----------| +| TestStuckCommandsIndexIsMissing | F-B1-5 | No sent_at index on agent_commands | PASS | update | +| TestStuckCommandsIndexExists | F-B1-5 | sent_at index must exist | **FAIL** | PASS | + +### stats_n1_test.go + +| Test | Bug | Asserts | State | After Fix | +|------|-----|---------|-------|-----------| +| TestGetDashboardStatsHasNPlusOneLoop | F-B1-6 | Query inside agent loop | PASS | update | +| TestGetDashboardStatsUsesJoin | F-B1-6 | No per-agent query loop | **FAIL** | PASS | + +### refresh_token_cleanup_test.go + +| Test | Bug | Asserts | State | After Fix | +|------|-----|---------|-------|-----------| +| TestNoBackgroundRefreshTokenCleanup | F-B1-10 | No background cleanup exists | PASS | update | +| TestBackgroundRefreshTokenCleanupExists | F-B1-10 | Background cleanup must exist | **FAIL** | PASS | + +--- + +## State-Change Summary + +| Test | Current | After Fix | +|------|---------|-----------| +| TestMigrationFailureReturnsError | PASS | PASS | +| TestServerStartsAfterMigrationFailure | PASS | FAIL (update) | +| TestServerMustAbortOnMigrationFailure | **FAIL** | PASS | +| TestMigrationRunnerDetectsDuplicateNumbers | PASS | FAIL (update) | +| TestMigrationRunnerShouldRejectDuplicateNumbers | **FAIL** | PASS | +| TestNoBackgroundRefreshTokenCleanup | PASS | FAIL (update) | +| TestBackgroundRefreshTokenCleanupExists | **FAIL** | PASS | +| TestMigrationsHaveIdempotencyViolations | PASS | FAIL (update) | +| TestAllMigrationsAreIdempotent | **FAIL** | PASS | +| TestStuckCommandsIndexIsMissing | PASS | FAIL (update) | +| TestStuckCommandsIndexExists | **FAIL** | PASS | +| TestMigration018ScannerConfigHasWrongSuffix | PASS | FAIL (update) | +| TestMigration018ScannerConfigHasCorrectSuffix | **FAIL** | PASS | +| TestMigration018ScannerConfigHasNoGrantToWrongRole | **FAIL** | PASS | +| TestMigration024HasSelfInsert | PASS | FAIL (update) | +| TestMigration024ShouldNotHaveSelfInsert | **FAIL** | PASS | +| TestMigration024ReferencesDeprecatedColumn | PASS | FAIL (update) | +| TestMigration024ColumnExistsInSchema | **FAIL** | PASS | +| TestGetDashboardStatsHasNPlusOneLoop | PASS | FAIL (update) | +| TestGetDashboardStatsUsesJoin | **FAIL** | PASS | + +**Bold FAIL** = tests asserting correct post-fix behavior (will flip to PASS). + +--- + +## Notes + +1. All tests are static analysis / source inspection — no live database required. +2. All A-series tests continue to pass (no regressions from B-1 test additions). +3. The idempotency test excludes migrations 025-026 (A-series, already idempotent).