fix: Review queue approvals (CODE_DERIVED + stale list + bulk errors)

AGENTS.md

@@ -191,6 +191,11 @@ only covers cloud-specific, non-obvious caveats.
   `sudo -u postgres psql -f docker/postgres/init/11_create_acme_erp.sql` then
   `bash scripts/seed-acme-erp.sh` (registers `ACME ERP (Multi-Schema)` when backend auth
   is disabled or you have an admin session cookie).
+- **Company Knowledge → Review queue** (code-scan suggestions): seed without an LLM via
+  `python3 scripts/self-host/seed-review-suggestions.py --count 50`, then exercise approve/
+  reject/bulk edge cases with `python3 scripts/self-host/e2e-review-approvals.py`.
+  Approving `SCHEMA_DOC` rows needs `CODE_DERIVED` on `schema_documentation_source_check`
+  (startup initializer repairs this; Hibernate `ddl-auto` does not).
 
 ### Non-obvious setup caveats (each cost real debugging time)
 

backend/src/main/java/com/dbaagent/config/SchemaDocumentationSourceCompatibilityInitializer.java

@@ -0,0 +1,74 @@
+package com.dbaagent.config;
+
+import com.dbaagent.model.DocumentationSource;
+import lombok.extern.slf4j.Slf4j;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.DependsOn;
+import org.springframework.jdbc.core.JdbcTemplate;
+
+import javax.sql.DataSource;
+import java.util.Arrays;
+import java.util.stream.Collectors;
+
+/**
+ * Keeps {@code schema_documentation.source} CHECK aligned with
+ * {@link DocumentationSource}.
+ *
+ * <p>Hibernate {@code ddl-auto=update} does not rewrite CHECK constraints when an
+ * enum gains a value. Self-host installs that predate V90 therefore reject
+ * {@code CODE_DERIVED} rows written by code-scan suggestion approve — the Review
+ * queue shows {@code APPROVED 0 OF N} while every SCHEMA_DOC decide is swallowed
+ * by bulk-decide. Mirrors {@link BrainInitSchemaCompatibilityInitializer}.
+ */
+@Configuration
+@Slf4j
+public class SchemaDocumentationSourceCompatibilityInitializer {
+
+    private static final String TABLE = "schema_documentation";
+    private static final String COLUMN = "source";
+    private static final String CONSTRAINT = "schema_documentation_source_check";
+
+    @Bean("schemaDocumentationSourceCompatibilityBootstrap")
+    @DependsOn("entityManagerFactory")
+    public Object schemaDocumentationSourceCompatibilityBootstrap(DataSource dataSource) {
+        JdbcTemplate jdbc = new JdbcTemplate(dataSource);
+        if (!tableExists(jdbc, TABLE) || !columnExists(jdbc, TABLE, COLUMN)) {
+            return new Object();
+        }
+
+        String allowed = Arrays.stream(DocumentationSource.values())
+            .map(DocumentationSource::name)
+            .map(v -> "'" + v + "'")
+            .collect(Collectors.joining(", "));
+
+        jdbc.execute("ALTER TABLE " + TABLE + " DROP CONSTRAINT IF EXISTS " + CONSTRAINT);
+        jdbc.execute(
+            "ALTER TABLE " + TABLE
+                + " ADD CONSTRAINT " + CONSTRAINT
+                + " CHECK ((" + COLUMN + ")::text = ANY (ARRAY[" + allowed + "]::text[]))"
+        );
+        log.info("Ensured {} allows DocumentationSource values: {}", CONSTRAINT, allowed);
+        return new Object();
+    }
+
+    private boolean tableExists(JdbcTemplate jdbc, String tableName) {
+        Integer count = jdbc.queryForObject("""
+                SELECT COUNT(*)
+                FROM information_schema.tables
+                WHERE table_schema = 'public' AND table_name = ?
+                """, Integer.class, tableName);
+        return count != null && count > 0;
+    }
+
+    private boolean columnExists(JdbcTemplate jdbc, String tableName, String columnName) {
+        Integer count = jdbc.queryForObject("""
+                SELECT COUNT(*)
+                FROM information_schema.columns
+                WHERE table_schema = 'public'
+                  AND table_name = ?
+                  AND column_name = ?
+                """, Integer.class, tableName, columnName);
+        return count != null && count > 0;
+    }
+}

backend/src/main/java/com/dbaagent/controller/CodeScanController.java

@@ -15,6 +15,7 @@
 import org.springframework.web.servlet.mvc.method.annotation.SseEmitter;
 
 import java.io.IOException;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -156,16 +157,18 @@ public ResponseEntity<Map<String, Object>> bulkDecide(
         if (body == null || body.ids() == null || body.ids().isEmpty()) {
             return ResponseEntity.badRequest().body(Map.of("error", "ids required"));
         }
-        var processed = codeScanService.bulkDecide(
+        var result = codeScanService.bulkDecide(
             body.ids(),
             body.decision(),
             accessControlService.getCurrentUsername(),
             body.note()
         );
-        return ResponseEntity.ok(Map.of(
-            "requested", body.ids().size(),
-            "succeeded", processed.size()
-        ));
+        Map<String, Object> payload = new LinkedHashMap<>();
+        payload.put("requested", body.ids().size());
+        payload.put("succeeded", result.succeeded().size());
+        payload.put("failed", result.failures().size());
+        payload.put("failures", result.failures());
+        return ResponseEntity.ok(payload);
     }
 
     private static CodeKnowledgeSuggestion.Status parseStatus(String s) {
@@ -192,4 +195,9 @@ public ResponseEntity<Map<String, String>> handleIO(IOException e) {
     public ResponseEntity<Map<String, String>> handleBadInput(IllegalArgumentException e) {
         return ResponseEntity.badRequest().body(Map.of("error", e.getMessage()));
     }
+
+    @ExceptionHandler(IllegalStateException.class)
+    public ResponseEntity<Map<String, String>> handleIllegalState(IllegalStateException e) {
+        return ResponseEntity.badRequest().body(Map.of("error", e.getMessage()));
+    }
 }

backend/src/main/java/com/dbaagent/service/codescan/CodeScanService.java

@@ -579,24 +579,46 @@ public CodeKnowledgeSuggestion decide(String suggestionId,
      * transaction inside applier.approve/reject, so one bad row cannot mark a
      * shared transaction rollback-only and break every subsequent item.
      */
-    public List<CodeKnowledgeSuggestion> bulkDecide(List<String> ids,
-                                                    String decision,
-                                                    String decidedBy,
-                                                    String note) {
+    public BulkDecideResult bulkDecide(List<String> ids,
+                                       String decision,
+                                       String decidedBy,
+                                       String note) {
         List<CodeKnowledgeSuggestion> out = new ArrayList<>();
-        int failures = 0;
+        List<Map<String, String>> failures = new ArrayList<>();
         for (String id : ids) {
             try {
                 out.add(decide(id, decision, decidedBy, note));
             } catch (Exception e) {
-                failures++;
-                log.warn("bulk decide skipped {}: {}", id, e.getMessage());
+                String message = rootMessage(e);
+                failures.add(Map.of("id", id, "error", message));
+                log.warn("bulk decide skipped {}: {}", id, message);
             }
         }
-        if (failures > 0) {
-            log.info("bulk decide: {} succeeded, {} failed", out.size(), failures);
+        if (!failures.isEmpty()) {
+            log.info("bulk decide: {} succeeded, {} failed", out.size(), failures.size());
         }
-        return out;
+        return new BulkDecideResult(out, failures);
+    }
+
+    public record BulkDecideResult(
+        List<CodeKnowledgeSuggestion> succeeded,
+        List<Map<String, String>> failures
+    ) {}
+
+    private static String rootMessage(Throwable e) {
+        Throwable cur = e;
+        String best = e.getMessage() == null ? e.getClass().getSimpleName() : e.getMessage();
+        while (cur != null) {
+            if (cur.getMessage() != null && !cur.getMessage().isBlank()) {
+                best = cur.getMessage();
+            }
+            cur = cur.getCause();
+        }
+        // Keep API payloads short — full stack stays in logs.
+        if (best.length() > 400) {
+            return best.substring(0, 397) + "...";
+        }
+        return best;
     }
 
     // ---- Helpers ----

backend/src/main/java/com/dbaagent/service/codescan/CodeSuggestionApplier.java

@@ -82,15 +82,20 @@ private void applySchemaDoc(CodeKnowledgeSuggestion suggestion, String decidedBy
         String objectName;
         String parentObject = null;
         String target = suggestion.getTargetObject();
+        // Never silently skip — bulk-decide would report succeeded while nothing
+        // was written, and the Review UI would look like a no-op approval.
         if (target == null || target.isBlank()) {
-            log.warn("SCHEMA_DOC suggestion {} has no targetObject; skipping", suggestion.getId());
-            return;
+            throw new IllegalArgumentException(
+                "SCHEMA_DOC suggestion " + suggestion.getId() + " has no targetObject"
+            );
         }
         if (objectType == SchemaDocumentation.DocumentationType.COLUMN) {
             int dot = target.indexOf('.');
             if (dot <= 0 || dot >= target.length() - 1) {
-                log.warn("SCHEMA_DOC column suggestion {} has malformed target '{}'", suggestion.getId(), target);
-                return;
+                throw new IllegalArgumentException(
+                    "SCHEMA_DOC column suggestion " + suggestion.getId()
+                        + " has malformed target '" + target + "'"
+                );
             }
             parentObject = target.substring(0, dot);
             objectName = target.substring(dot + 1);

backend/src/test/java/com/dbaagent/service/codescan/CodeSuggestionApplierTest.java

@@ -0,0 +1,130 @@
+package com.dbaagent.service.codescan;
+
+import com.dbaagent.model.DocumentationSource;
+import com.dbaagent.model.SchemaDocumentation;
+import com.dbaagent.model.SchemaMetadata;
+import com.dbaagent.model.code.CodeKnowledgeSuggestion;
+import com.dbaagent.repository.CodeKnowledgeSuggestionRepository;
+import com.dbaagent.repository.SchemaDocumentationRepository;
+import com.dbaagent.service.CompanyKnowledgeService;
+import com.dbaagent.service.SchemaScannerService;
+import com.dbaagent.service.TrainingService;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@ExtendWith(MockitoExtension.class)
+class CodeSuggestionApplierTest {
+
+    @Mock private CodeKnowledgeSuggestionRepository suggestionRepository;
+    @Mock private SchemaDocumentationRepository schemaDocRepository;
+    @Mock private CompanyKnowledgeService companyKnowledgeService;
+    @Mock private TrainingService trainingService;
+    @Mock private SchemaScannerService schemaScannerService;
+
+    private CodeSuggestionApplier applier;
+
+    @BeforeEach
+    void setUp() {
+        applier = new CodeSuggestionApplier(
+            suggestionRepository,
+            schemaDocRepository,
+            companyKnowledgeService,
+            trainingService,
+            schemaScannerService
+        );
+    }
+
+    @Test
+    void approveSchemaDocColumn_writesCodeDerivedRow() throws Exception {
+        CodeKnowledgeSuggestion suggestion = baseSuggestion();
+        suggestion.setTargetKind(CodeKnowledgeSuggestion.TargetKind.SCHEMA_DOC);
+        suggestion.setTargetObject("fct_ashram_visit.party_size");
+        suggestion.setPayload(Map.of("objectKind", "COLUMN", "businessTerms", List.of("party size")));
+
+        when(suggestionRepository.findById("s1")).thenReturn(Optional.of(suggestion));
+        when(suggestionRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
+        when(schemaDocRepository.findByConnectionIdAndObjectTypeAndObjectNameAndParentObjectAndSource(
+            eq("conn-1"),
+            eq(SchemaDocumentation.DocumentationType.COLUMN),
+            eq("party_size"),
+            eq("acme_erp.fct_ashram_visit"),
+            eq(DocumentationSource.CODE_DERIVED)
+        )).thenReturn(Optional.empty());
+        SchemaMetadata schema = new SchemaMetadata();
+        schema.setDatabaseName("acme_erp");
+        when(schemaScannerService.scanSchema("conn-1")).thenReturn(schema);
+        when(schemaDocRepository.save(any())).thenAnswer(inv -> {
+            SchemaDocumentation doc = inv.getArgument(0);
+            doc.setId("doc-1");
+            return doc;
+        });
+
+        CodeKnowledgeSuggestion approved = applier.approve("s1", "admin", null);
+
+        assertThat(approved.getStatus()).isEqualTo(CodeKnowledgeSuggestion.Status.APPROVED);
+        assertThat(approved.getAppliedDocId()).isEqualTo("doc-1");
+
+        ArgumentCaptor<SchemaDocumentation> captor = ArgumentCaptor.forClass(SchemaDocumentation.class);
+        verify(schemaDocRepository).save(captor.capture());
+        SchemaDocumentation saved = captor.getValue();
+        assertThat(saved.getSource()).isEqualTo(DocumentationSource.CODE_DERIVED);
+        assertThat(saved.getObjectName()).isEqualTo("party_size");
+        assertThat(saved.getParentObject()).isEqualTo("acme_erp.fct_ashram_visit");
+        assertThat(saved.getBusinessTerms()).isEqualTo("party size");
+        verify(trainingService).upsertDocumentationEmbedding(saved);
+    }
+
+    @Test
+    void approveSchemaDoc_blankTarget_throwsInsteadOfSilentSkip() {
+        CodeKnowledgeSuggestion suggestion = baseSuggestion();
+        suggestion.setTargetKind(CodeKnowledgeSuggestion.TargetKind.SCHEMA_DOC);
+        suggestion.setTargetObject("  ");
+        when(suggestionRepository.findById("s1")).thenReturn(Optional.of(suggestion));
+
+        assertThatThrownBy(() -> applier.approve("s1", "admin", null))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessageContaining("no targetObject");
+        verify(schemaDocRepository, never()).save(any());
+        verify(suggestionRepository, never()).save(any());
+    }
+
+    @Test
+    void approveRejectedSuggestion_isRejected() {
+        CodeKnowledgeSuggestion suggestion = baseSuggestion();
+        suggestion.setStatus(CodeKnowledgeSuggestion.Status.REJECTED);
+        when(suggestionRepository.findById("s1")).thenReturn(Optional.of(suggestion));
+
+        assertThatThrownBy(() -> applier.approve("s1", "admin", null))
+            .isInstanceOf(IllegalStateException.class)
+            .hasMessageContaining("cannot be approved");
+    }
+
+    private static CodeKnowledgeSuggestion baseSuggestion() {
+        return CodeKnowledgeSuggestion.builder()
+            .id("s1")
+            .jobId("job-1")
+            .connectionId("conn-1")
+            .title("Visit party size")
+            .content("Number of people in the visiting party.")
+            .confidence(0.99)
+            .status(CodeKnowledgeSuggestion.Status.PENDING)
+            .sourceFiles(List.of(Map.of("path", "src/Seed.java", "startLine", 1, "endLine", 10)))
+            .build();
+    }
+}