Skip to content

Commit b4d62d2

Browse files
venkateshsakamuri-labcursoragent
venkateshsakamuri-lab
and
cursoragent
authored
fix: Review queue approvals (CODE_DERIVED + stale list + bulk errors) (#74)
<!-- CURSOR_AGENT_PR_BODY_BEGIN --> ## Problem (customer screenshot) Step 3 showed **198 awaiting sign-off**, the table showed **2** pending SCHEMA rows, and bulk approve displayed **APPROVED 0 OF 2**. ## Root causes (reproduced) 1. **Approve fails on self-host** — `schema_documentation_source_check` often still lacks `CODE_DERIVED` (Hibernate `ddl-auto=update` never rewrites CHECKs). SCHEMA_DOC approve inserts `source=CODE_DERIVED` → constraint violation → bulk-decide swallows the error → UI shows `Approved 0 of N` as a success banner. 2. **Count mismatch** — badge uses a fresh `totalElements` probe; the Review list used `useAllCodeScanSuggestions` with `staleTime: 30s` and only invalidated on scan *start*, not on COMPLETED. 3. **Silent failures** — bulk API returned only `{requested, succeeded}`; UI treated `succeeded: 0` as success. ## Fix - Startup initializer (`SchemaDocumentationSourceCompatibilityInitializer`) keeps the source CHECK aligned with `DocumentationSource` (same pattern as Brain init stages). - Bulk decide returns `failed` + `failures[{id,error}]`; Review UI treats partial/zero success as an error banner with details. - Invalidate code-scan queries when a scan reaches COMPLETED/FAILED/CANCELLED; drop list `staleTime`. - Malformed SCHEMA_DOC targets throw instead of silently skipping. - `CodeSuggestionApplierTest` + seed/E2E scripts: - `scripts/self-host/seed-review-suggestions.py` - `scripts/self-host/e2e-review-approvals.py` (15 edge cases, all green) ## Verify ```bash python3 scripts/self-host/seed-review-suggestions.py --count 20 python3 scripts/self-host/e2e-review-approvals.py # expect: ✓ All review-approval edge cases passed ``` Please merge the AGENTS.md updates so future agents remember the Review seed/E2E path and the CODE_DERIVED CHECK footgun. <!-- CURSOR_AGENT_PR_BODY_END --> <div><a href="https://cursor.com/agents/bc-8ce91e70-c67b-48c6-84b3-05bb9d06231a?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a>&nbsp;<a href="https://cursor.com/background-agent?bcId=bc-8ce91e70-c67b-48c6-84b3-05bb9d06231a&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a>&nbsp;</div> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
1 parent ab4ab8f commit b4d62d2

11 files changed

Lines changed: 765 additions & 25 deletions

File tree

‎AGENTS.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,11 @@ only covers cloud-specific, non-obvious caveats.
191191
`sudo -u postgres psql -f docker/postgres/init/11_create_acme_erp.sql` then
192192
`bash scripts/seed-acme-erp.sh` (registers `ACME ERP (Multi-Schema)` when backend auth
193193
is disabled or you have an admin session cookie).
194+
- **Company Knowledge → Review queue** (code-scan suggestions): seed without an LLM via
195+
`python3 scripts/self-host/seed-review-suggestions.py --count 50`, then exercise approve/
196+
reject/bulk edge cases with `python3 scripts/self-host/e2e-review-approvals.py`.
197+
Approving `SCHEMA_DOC` rows needs `CODE_DERIVED` on `schema_documentation_source_check`
198+
(startup initializer repairs this; Hibernate `ddl-auto` does not).
194199

195200
### Non-obvious setup caveats (each cost real debugging time)
196201

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

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
package com.dbaagent.config;
2+
3+
import com.dbaagent.model.DocumentationSource;
4+
import lombok.extern.slf4j.Slf4j;
5+
import org.springframework.context.annotation.Bean;
6+
import org.springframework.context.annotation.Configuration;
7+
import org.springframework.context.annotation.DependsOn;
8+
import org.springframework.jdbc.core.JdbcTemplate;
9+
10+
import javax.sql.DataSource;
11+
import java.util.Arrays;
12+
import java.util.stream.Collectors;
13+
14+
/**
15+
* Keeps {@code schema_documentation.source} CHECK aligned with
16+
* {@link DocumentationSource}.
17+
*
18+
* <p>Hibernate {@code ddl-auto=update} does not rewrite CHECK constraints when an
19+
* enum gains a value. Self-host installs that predate V90 therefore reject
20+
* {@code CODE_DERIVED} rows written by code-scan suggestion approve — the Review
21+
* queue shows {@code APPROVED 0 OF N} while every SCHEMA_DOC decide is swallowed
22+
* by bulk-decide. Mirrors {@link BrainInitSchemaCompatibilityInitializer}.
23+
*/
24+
@Configuration
25+
@Slf4j
26+
public class SchemaDocumentationSourceCompatibilityInitializer {
27+
28+
private static final String TABLE = "schema_documentation";
29+
private static final String COLUMN = "source";
30+
private static final String CONSTRAINT = "schema_documentation_source_check";
31+
32+
@Bean("schemaDocumentationSourceCompatibilityBootstrap")
33+
@DependsOn("entityManagerFactory")
34+
public Object schemaDocumentationSourceCompatibilityBootstrap(DataSource dataSource) {
35+
JdbcTemplate jdbc = new JdbcTemplate(dataSource);
36+
if (!tableExists(jdbc, TABLE) || !columnExists(jdbc, TABLE, COLUMN)) {
37+
return new Object();
38+
}
39+
40+
String allowed = Arrays.stream(DocumentationSource.values())
41+
.map(DocumentationSource::name)
42+
.map(v -> "'" + v + "'")
43+
.collect(Collectors.joining(", "));
44+
45+
jdbc.execute("ALTER TABLE " + TABLE + " DROP CONSTRAINT IF EXISTS " + CONSTRAINT);
46+
jdbc.execute(
47+
"ALTER TABLE " + TABLE
48+
+ " ADD CONSTRAINT " + CONSTRAINT
49+
+ " CHECK ((" + COLUMN + ")::text = ANY (ARRAY[" + allowed + "]::text[]))"
50+
);
51+
log.info("Ensured {} allows DocumentationSource values: {}", CONSTRAINT, allowed);
52+
return new Object();
53+
}
54+
55+
private boolean tableExists(JdbcTemplate jdbc, String tableName) {
56+
Integer count = jdbc.queryForObject("""
57+
SELECT COUNT(*)
58+
FROM information_schema.tables
59+
WHERE table_schema = 'public' AND table_name = ?
60+
""", Integer.class, tableName);
61+
return count != null && count > 0;
62+
}
63+
64+
private boolean columnExists(JdbcTemplate jdbc, String tableName, String columnName) {
65+
Integer count = jdbc.queryForObject("""
66+
SELECT COUNT(*)
67+
FROM information_schema.columns
68+
WHERE table_schema = 'public'
69+
AND table_name = ?
70+
AND column_name = ?
71+
""", Integer.class, tableName, columnName);
72+
return count != null && count > 0;
73+
}
74+
}

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

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.springframework.web.servlet.mvc.method.annotation.SseEmitter;
1616

1717
import java.io.IOException;
18+
import java.util.LinkedHashMap;
1819
import java.util.List;
1920
import java.util.Locale;
2021
import java.util.Map;
@@ -156,16 +157,18 @@ public ResponseEntity<Map<String, Object>> bulkDecide(
156157
if (body == null || body.ids() == null || body.ids().isEmpty()) {
157158
return ResponseEntity.badRequest().body(Map.of("error", "ids required"));
158159
}
159-
var processed = codeScanService.bulkDecide(
160+
var result = codeScanService.bulkDecide(
160161
body.ids(),
161162
body.decision(),
162163
accessControlService.getCurrentUsername(),
163164
body.note()
164165
);
165-
return ResponseEntity.ok(Map.of(
166-
"requested", body.ids().size(),
167-
"succeeded", processed.size()
168-
));
166+
Map<String, Object> payload = new LinkedHashMap<>();
167+
payload.put("requested", body.ids().size());
168+
payload.put("succeeded", result.succeeded().size());
169+
payload.put("failed", result.failures().size());
170+
payload.put("failures", result.failures());
171+
return ResponseEntity.ok(payload);
169172
}
170173

171174
private static CodeKnowledgeSuggestion.Status parseStatus(String s) {
@@ -192,4 +195,9 @@ public ResponseEntity<Map<String, String>> handleIO(IOException e) {
192195
public ResponseEntity<Map<String, String>> handleBadInput(IllegalArgumentException e) {
193196
return ResponseEntity.badRequest().body(Map.of("error", e.getMessage()));
194197
}
198+
199+
@ExceptionHandler(IllegalStateException.class)
200+
public ResponseEntity<Map<String, String>> handleIllegalState(IllegalStateException e) {
201+
return ResponseEntity.badRequest().body(Map.of("error", e.getMessage()));
202+
}
195203
}

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

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -579,24 +579,46 @@ public CodeKnowledgeSuggestion decide(String suggestionId,
579579
* transaction inside applier.approve/reject, so one bad row cannot mark a
580580
* shared transaction rollback-only and break every subsequent item.
581581
*/
582-
public List<CodeKnowledgeSuggestion> bulkDecide(List<String> ids,
583-
String decision,
584-
String decidedBy,
585-
String note) {
582+
public BulkDecideResult bulkDecide(List<String> ids,
583+
String decision,
584+
String decidedBy,
585+
String note) {
586586
List<CodeKnowledgeSuggestion> out = new ArrayList<>();
587-
int failures = 0;
587+
List<Map<String, String>> failures = new ArrayList<>();
588588
for (String id : ids) {
589589
try {
590590
out.add(decide(id, decision, decidedBy, note));
591591
} catch (Exception e) {
592-
failures++;
593-
log.warn("bulk decide skipped {}: {}", id, e.getMessage());
592+
String message = rootMessage(e);
593+
failures.add(Map.of("id", id, "error", message));
594+
log.warn("bulk decide skipped {}: {}", id, message);
594595
}
595596
}
596-
if (failures > 0) {
597-
log.info("bulk decide: {} succeeded, {} failed", out.size(), failures);
597+
if (!failures.isEmpty()) {
598+
log.info("bulk decide: {} succeeded, {} failed", out.size(), failures.size());
598599
}
599-
return out;
600+
return new BulkDecideResult(out, failures);
601+
}
602+
603+
public record BulkDecideResult(
604+
List<CodeKnowledgeSuggestion> succeeded,
605+
List<Map<String, String>> failures
606+
) {}
607+
608+
private static String rootMessage(Throwable e) {
609+
Throwable cur = e;
610+
String best = e.getMessage() == null ? e.getClass().getSimpleName() : e.getMessage();
611+
while (cur != null) {
612+
if (cur.getMessage() != null && !cur.getMessage().isBlank()) {
613+
best = cur.getMessage();
614+
}
615+
cur = cur.getCause();
616+
}
617+
// Keep API payloads short — full stack stays in logs.
618+
if (best.length() > 400) {
619+
return best.substring(0, 397) + "...";
620+
}
621+
return best;
600622
}
601623

602624
// ---- Helpers ----

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

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -82,15 +82,20 @@ private void applySchemaDoc(CodeKnowledgeSuggestion suggestion, String decidedBy
8282
String objectName;
8383
String parentObject = null;
8484
String target = suggestion.getTargetObject();
85+
// Never silently skip — bulk-decide would report succeeded while nothing
86+
// was written, and the Review UI would look like a no-op approval.
8587
if (target == null || target.isBlank()) {
86-
log.warn("SCHEMA_DOC suggestion {} has no targetObject; skipping", suggestion.getId());
87-
return;
88+
throw new IllegalArgumentException(
89+
"SCHEMA_DOC suggestion " + suggestion.getId() + " has no targetObject"
90+
);
8891
}
8992
if (objectType == SchemaDocumentation.DocumentationType.COLUMN) {
9093
int dot = target.indexOf('.');
9194
if (dot <= 0 || dot >= target.length() - 1) {
92-
log.warn("SCHEMA_DOC column suggestion {} has malformed target '{}'", suggestion.getId(), target);
93-
return;
95+
throw new IllegalArgumentException(
96+
"SCHEMA_DOC column suggestion " + suggestion.getId()
97+
+ " has malformed target '" + target + "'"
98+
);
9499
}
95100
parentObject = target.substring(0, dot);
96101
objectName = target.substring(dot + 1);

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

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
package com.dbaagent.service.codescan;
2+
3+
import com.dbaagent.model.DocumentationSource;
4+
import com.dbaagent.model.SchemaDocumentation;
5+
import com.dbaagent.model.SchemaMetadata;
6+
import com.dbaagent.model.code.CodeKnowledgeSuggestion;
7+
import com.dbaagent.repository.CodeKnowledgeSuggestionRepository;
8+
import com.dbaagent.repository.SchemaDocumentationRepository;
9+
import com.dbaagent.service.CompanyKnowledgeService;
10+
import com.dbaagent.service.SchemaScannerService;
11+
import com.dbaagent.service.TrainingService;
12+
import org.junit.jupiter.api.BeforeEach;
13+
import org.junit.jupiter.api.Test;
14+
import org.junit.jupiter.api.extension.ExtendWith;
15+
import org.mockito.ArgumentCaptor;
16+
import org.mockito.Mock;
17+
import org.mockito.junit.jupiter.MockitoExtension;
18+
19+
import java.util.List;
20+
import java.util.Map;
21+
import java.util.Optional;
22+
23+
import static org.assertj.core.api.Assertions.assertThat;
24+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
25+
import static org.mockito.ArgumentMatchers.any;
26+
import static org.mockito.ArgumentMatchers.eq;
27+
import static org.mockito.Mockito.never;
28+
import static org.mockito.Mockito.verify;
29+
import static org.mockito.Mockito.when;
30+
31+
@ExtendWith(MockitoExtension.class)
32+
class CodeSuggestionApplierTest {
33+
34+
@Mock private CodeKnowledgeSuggestionRepository suggestionRepository;
35+
@Mock private SchemaDocumentationRepository schemaDocRepository;
36+
@Mock private CompanyKnowledgeService companyKnowledgeService;
37+
@Mock private TrainingService trainingService;
38+
@Mock private SchemaScannerService schemaScannerService;
39+
40+
private CodeSuggestionApplier applier;
41+
42+
@BeforeEach
43+
void setUp() {
44+
applier = new CodeSuggestionApplier(
45+
suggestionRepository,
46+
schemaDocRepository,
47+
companyKnowledgeService,
48+
trainingService,
49+
schemaScannerService
50+
);
51+
}
52+
53+
@Test
54+
void approveSchemaDocColumn_writesCodeDerivedRow() throws Exception {
55+
CodeKnowledgeSuggestion suggestion = baseSuggestion();
56+
suggestion.setTargetKind(CodeKnowledgeSuggestion.TargetKind.SCHEMA_DOC);
57+
suggestion.setTargetObject("fct_ashram_visit.party_size");
58+
suggestion.setPayload(Map.of("objectKind", "COLUMN", "businessTerms", List.of("party size")));
59+
60+
when(suggestionRepository.findById("s1")).thenReturn(Optional.of(suggestion));
61+
when(suggestionRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
62+
when(schemaDocRepository.findByConnectionIdAndObjectTypeAndObjectNameAndParentObjectAndSource(
63+
eq("conn-1"),
64+
eq(SchemaDocumentation.DocumentationType.COLUMN),
65+
eq("party_size"),
66+
eq("acme_erp.fct_ashram_visit"),
67+
eq(DocumentationSource.CODE_DERIVED)
68+
)).thenReturn(Optional.empty());
69+
SchemaMetadata schema = new SchemaMetadata();
70+
schema.setDatabaseName("acme_erp");
71+
when(schemaScannerService.scanSchema("conn-1")).thenReturn(schema);
72+
when(schemaDocRepository.save(any())).thenAnswer(inv -> {
73+
SchemaDocumentation doc = inv.getArgument(0);
74+
doc.setId("doc-1");
75+
return doc;
76+
});
77+
78+
CodeKnowledgeSuggestion approved = applier.approve("s1", "admin", null);
79+
80+
assertThat(approved.getStatus()).isEqualTo(CodeKnowledgeSuggestion.Status.APPROVED);
81+
assertThat(approved.getAppliedDocId()).isEqualTo("doc-1");
82+
83+
ArgumentCaptor<SchemaDocumentation> captor = ArgumentCaptor.forClass(SchemaDocumentation.class);
84+
verify(schemaDocRepository).save(captor.capture());
85+
SchemaDocumentation saved = captor.getValue();
86+
assertThat(saved.getSource()).isEqualTo(DocumentationSource.CODE_DERIVED);
87+
assertThat(saved.getObjectName()).isEqualTo("party_size");
88+
assertThat(saved.getParentObject()).isEqualTo("acme_erp.fct_ashram_visit");
89+
assertThat(saved.getBusinessTerms()).isEqualTo("party size");
90+
verify(trainingService).upsertDocumentationEmbedding(saved);
91+
}
92+
93+
@Test
94+
void approveSchemaDoc_blankTarget_throwsInsteadOfSilentSkip() {
95+
CodeKnowledgeSuggestion suggestion = baseSuggestion();
96+
suggestion.setTargetKind(CodeKnowledgeSuggestion.TargetKind.SCHEMA_DOC);
97+
suggestion.setTargetObject(" ");
98+
when(suggestionRepository.findById("s1")).thenReturn(Optional.of(suggestion));
99+
100+
assertThatThrownBy(() -> applier.approve("s1", "admin", null))
101+
.isInstanceOf(IllegalArgumentException.class)
102+
.hasMessageContaining("no targetObject");
103+
verify(schemaDocRepository, never()).save(any());
104+
verify(suggestionRepository, never()).save(any());
105+
}
106+
107+
@Test
108+
void approveRejectedSuggestion_isRejected() {
109+
CodeKnowledgeSuggestion suggestion = baseSuggestion();
110+
suggestion.setStatus(CodeKnowledgeSuggestion.Status.REJECTED);
111+
when(suggestionRepository.findById("s1")).thenReturn(Optional.of(suggestion));
112+
113+
assertThatThrownBy(() -> applier.approve("s1", "admin", null))
114+
.isInstanceOf(IllegalStateException.class)
115+
.hasMessageContaining("cannot be approved");
116+
}
117+
118+
private static CodeKnowledgeSuggestion baseSuggestion() {
119+
return CodeKnowledgeSuggestion.builder()
120+
.id("s1")
121+
.jobId("job-1")
122+
.connectionId("conn-1")
123+
.title("Visit party size")
124+
.content("Number of people in the visiting party.")
125+
.confidence(0.99)
126+
.status(CodeKnowledgeSuggestion.Status.PENDING)
127+
.sourceFiles(List.of(Map.of("path", "src/Seed.java", "startLine", 1, "endLine", 10)))
128+
.build();
129+
}
130+
}

0 commit comments

Comments
 (0)