Skip to content

Commit 0899d37

Browse files
geekypunkclaude
geekypunk
and
claude
committed
fix(brain): unwedge suggestion approval and refresh knowledge counts live
Approving code-scan suggestions reported "Approved 0 of 2" and, once approving worked, the knowledge counts stayed stale until a page reload while freshly approved items sorted to the bottom of the list. Approval was permanently wedged ------------------------------ `schema_documentation` carried no unique constraint on (connection_id, object_type, object_name, parent_object, source) and `CodeSuggestionApplier.approve` took no row lock, so one bulk approve submitted twice concurrently wrote 219 duplicate pairs. Every later approve touching one of those keys threw `IncorrectResultSizeDataAccessException: Query did not return a unique result` out of the Optional-returning upsert finder, which bulk-decide swallowed per item. The duplicate never self-heals, so all 198 pending SCHEMA_DOC suggestions were unapprovable. - Those finders now return List; `SchemaDocumentationDeduplicator` collapses matches, keeps the newest row, repoints any `applied_doc_id` off the rows it deletes (a loose reference, not an FK, so a dangling value fails silently) and drops their RAG embeddings. Applied at all four call sites, including `SchemaDriftListener`. - `V116__dedupe_schema_documentation.sql` + `SchemaDocumentationDedupeInitializer` remove the duplicates and add `ux_schema_doc_target`. There is no Flyway here, so the initializer is what actually applies it; it is idempotent and skips once the index exists. On the reporting install: 219 rows and 219 orphaned embeddings removed, 0 duplicate groups left. - approve/reject load the suggestion `FOR UPDATE`, so the concurrent double-submit that created the duplicates blocks instead of racing. Counts did not refresh ---------------------- An approval also writes `schema_documentation`, served by `brain/notes`, which backs the Write-notes tab and its coverage counts. The decide hooks invalidated only codeScan + companyKnowledge, so those counts were stale until reload. `invalidateAfterDecision` now covers brain and schemaContext too. Newest items sorted last ------------------------ - `@PreUpdate` does not fire on insert, so a new note has a null `updatedAt`; sorting on it alone with nulls last sent every brand-new note to the bottom. `BrainNoteService` sorts on COALESCE(updatedAt, createdAt), matching the company-knowledge repo. - `listSuggestions` sorted every status by confidence, scattering a fresh approval among hundreds of older ones. PENDING stays confidence-first (a work queue); decided statuses sort by `decidedAt DESC NULLS LAST`. Test tooling ------------ Both self-host scripts hardcoded `sudo -u postgres psql`, which does not exist on the Compose deployment install.sh produces, so the documented verify command failed before testing anything; they now resolve the path via `scripts/self-host/vaultdb.py`. The e2e suite also parked every real CODE_DERIVED row by rewriting source to USER and never restored it, silently relabelling approved docs on a live install — it now copies rows to a scratch table and restores them, and only deletes its planted row while nothing references it. Added cases for duplicate collapse, applied_doc_id repointing, the unique index, and two genuinely concurrent approves writing exactly one row: 30/30 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018mG2xj9gWJ8WzfDP2fDePP
1 parent b4d62d2 commit 0899d37

20 files changed

Lines changed: 1028 additions & 83 deletions

‎.gitignore‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,3 +104,7 @@ optd-sidecar/target/
104104
*.iml
105105
.local-admin-credentials
106106
.local-mcp-token
107+
108+
# Python bytecode from scripts/
109+
__pycache__/
110+
*.pyc

‎CLAUDE.md‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,16 @@ broken. Assert the *outcome*, never the attempt:
287287
- **Mocks hide SDK breaks.** `tests/tools/test_mcp_structured_content.py` uses a
288288
`_FakeCallToolResult` with a hardcoded `.isError`, so it kept passing precisely when
289289
the real SDK stopped matching. Pin the dependency; a fake cannot catch this.
290+
- **A self-host verification script must reach the DB the way the install does.**
291+
`seed-review-suggestions.py` / `e2e-review-approvals.py` hardcoded
292+
`sudo -u postgres psql`, which only exists on a bare-metal install — on the Compose
293+
deployment `install.sh` actually produces, the documented verify command died before
294+
testing anything. Both now resolve the path through `scripts/self-host/vaultdb.py`.
295+
- **A test that mutates shared state must restore it, and only what it created.** The
296+
same e2e suite parked every real `CODE_DERIVED` row by rewriting `source` to `USER`
297+
and never restored it, so a run against a live install silently relabelled the user's
298+
approved docs. It now copies rows to a scratch table and restores them, and its
299+
cleanup deletes the planted row only while nothing references it.
290300
- **Never claim a check you did not run.** `install.sh` reported "up to date" when it
291301
could not reach npm; it now says it could not check.
292302
- **`set -e` + `read` at EOF aborts silently.** Prompts in `install.sh` use
@@ -375,6 +385,53 @@ it against a real database — not a theoretical hardening pass.
375385
`POST /users/admin/reset` on every install that had run `setup-agent.sh`, since that
376386
mints an admin MCP token on each run.
377387

388+
- **An `Optional`-returning derived finder is an assertion that the key is unique.**
389+
Spring Data throws `IncorrectResultSizeDataAccessException` ("Query did not return a
390+
unique result: N results were returned") the moment it is not, and the row that broke
391+
it never repairs itself, so the failure is permanent rather than transient.
392+
`schema_documentation` had no unique constraint on
393+
`(connection_id, object_type, object_name, parent_object, source)` and
394+
`CodeSuggestionApplier.approve` took no row lock, so one bulk approve submitted twice
395+
concurrently wrote 219 duplicate pairs. Every later approve touching one of those keys
396+
threw, `CodeScanService.bulkDecide` swallowed it per item, and the Review queue
397+
reported "Approved 0 of 2" — with all 198 pending SCHEMA_DOC suggestions wedged.
398+
Three-part fix, and all three are load-bearing:
399+
1. `V116__dedupe_schema_documentation.sql` + `SchemaDocumentationDedupeInitializer`
400+
(no Flyway here, so the initializer is what actually applies it) collapse the
401+
duplicates and add `ux_schema_doc_target`, keyed on
402+
`coalesce(parent_object,'')` because Postgres treats NULLs as distinct.
403+
2. Those finders now return `List`, and `SchemaDocumentationDeduplicator.collapse`
404+
keeps the newest row, repoints any `applied_doc_id` off the rows it deletes, and
405+
drops their RAG embeddings. Do not restore an `Optional` variant — legacy installs
406+
still carry duplicates until the initializer runs.
407+
3. `approve`/`reject` load the suggestion via `findByIdForUpdate` (`PESSIMISTIC_WRITE`)
408+
so the concurrent double-submit that created the duplicates blocks instead of racing.
409+
- **`applied_doc_id` is a loose reference, not an FK.** Deleting a `schema_documentation`
410+
row it points at raises nothing and dangles silently — repoint before deleting.
411+
- **Approve *updates* the row an earlier scan wrote**, so a "freshly approved" doc row
412+
carries a historical `created_at`. A test that plants an "old" duplicate with a
413+
hardcoded past date can easily plant the *newer* of the two and assert nothing; anchor
414+
fixture timestamps to the real row's `created_at`.
415+
- **A write's blast radius decides what to invalidate, not the endpoint you called.**
416+
Approving a code-scan suggestion writes `code_knowledge_suggestion` *and*
417+
`schema_documentation` (served by `brain/notes`, which backs the Write-notes tab
418+
and its coverage counts) *and* `rag_documents` *and*, for KNOWLEDGE_ENTRY, a
419+
company knowledge entry. The decide hooks invalidated only `codeScan` +
420+
`companyKnowledge`, so every schema-doc-derived count stayed stale until the user
421+
reloaded the page. `invalidateAfterDecision` in `useCodeScan.js` is the single
422+
place that lists them; add to it when an approval starts writing something new.
423+
- **`@PreUpdate` does not fire on insert, so `updatedAt` is null on a brand-new row.**
424+
Sorting "newest first" on `updatedAt` alone with nulls last therefore sends every
425+
freshly created row to the *bottom* — which is why a just-approved note did not
426+
appear at the top of the Write-notes list. Sort on
427+
`COALESCE(updatedAt, createdAt)` (`BrainNoteService.touchedAt`,
428+
`CompanyKnowledgeEntryRepository.findByConnectionIdOrderByRecency`).
429+
- **Suggestion list order depends on the status being viewed.** PENDING is a work
430+
queue → `confidence DESC`. APPROVED/REJECTED are history → `decidedAt DESC NULLS
431+
LAST` so the decision you just made is at the top; confidence-sorting a decided
432+
list scattered fresh approvals among hundreds of older ones
433+
(`CodeScanService.sortFor`).
434+
378435
### Endpoint Authorization Rules
379436

380437
- **Authentication is not authorization.** `SecurityConfig` only asserts

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

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
package com.dbaagent.config;
2+
3+
import lombok.extern.slf4j.Slf4j;
4+
import org.springframework.context.annotation.Bean;
5+
import org.springframework.context.annotation.Configuration;
6+
import org.springframework.context.annotation.DependsOn;
7+
import org.springframework.jdbc.core.JdbcTemplate;
8+
import org.springframework.transaction.support.TransactionTemplate;
9+
import org.springframework.transaction.PlatformTransactionManager;
10+
11+
import javax.sql.DataSource;
12+
13+
/**
14+
* Applies {@code V116__dedupe_schema_documentation.sql} at startup: collapses
15+
* duplicate {@code schema_documentation} rows and adds the unique index on the
16+
* logical key.
17+
*
18+
* <p>This repo has no Flyway runtime — {@code db/migration} is a hand-maintained
19+
* changelog and Hibernate {@code ddl-auto=update} never adds an index the entity
20+
* does not declare. Without this, self-host installs carrying duplicates from a
21+
* double-submitted bulk approve stay wedged: every SCHEMA_DOC approve throws
22+
* {@code Query did not return a unique result}. Mirrors
23+
* {@link SchemaDocumentationSourceCompatibilityInitializer}.
24+
*
25+
* <p>Idempotent and cheap on a clean install: the index exists, so it returns
26+
* before touching a row.
27+
*/
28+
@Configuration
29+
@Slf4j
30+
public class SchemaDocumentationDedupeInitializer {
31+
32+
private static final String TABLE = "schema_documentation";
33+
private static final String INDEX = "ux_schema_doc_target";
34+
35+
@Bean("schemaDocumentationDedupeBootstrap")
36+
@DependsOn("entityManagerFactory")
37+
public Object schemaDocumentationDedupeBootstrap(DataSource dataSource,
38+
PlatformTransactionManager txManager) {
39+
JdbcTemplate jdbc = new JdbcTemplate(dataSource);
40+
if (!tableExists(jdbc, TABLE)) {
41+
return new Object();
42+
}
43+
if (indexExists(jdbc, INDEX)) {
44+
return new Object();
45+
}
46+
47+
// One transaction: a half-applied dedupe (rows deleted, index missing)
48+
// would silently re-accumulate duplicates until the next boot.
49+
new TransactionTemplate(txManager).executeWithoutResult(status -> {
50+
int repointed = jdbc.update("""
51+
UPDATE code_knowledge_suggestion s
52+
SET applied_doc_id = l.keep_id
53+
FROM (%s) l
54+
WHERE s.applied_doc_id = l.id
55+
""".formatted(LOSERS));
56+
57+
int embeddings = jdbc.update(
58+
"DELETE FROM rag_documents WHERE id IN (SELECT id FROM (%s) l)".formatted(LOSERS));
59+
60+
int removed = jdbc.update(
61+
"DELETE FROM schema_documentation WHERE id IN (SELECT id FROM (%s) l)".formatted(LOSERS));
62+
63+
jdbc.execute("""
64+
CREATE UNIQUE INDEX IF NOT EXISTS %s
65+
ON %s (connection_id, object_type, object_name, coalesce(parent_object, ''), source)
66+
""".formatted(INDEX, TABLE));
67+
68+
if (removed > 0) {
69+
log.warn("Deduped {}: removed {} duplicate rows, {} orphaned embeddings, "
70+
+ "repointed {} applied_doc_id references",
71+
TABLE, removed, embeddings, repointed);
72+
}
73+
log.info("Ensured unique index {} on {}", INDEX, TABLE);
74+
});
75+
return new Object();
76+
}
77+
78+
/**
79+
* Every row but the newest within each logical key. Newest wins because it is
80+
* the row existing {@code applied_doc_id} references point at; {@code id}
81+
* breaks ties for rows written in the same clock tick. {@code coalesce} on
82+
* {@code parent_object} because Postgres treats NULLs as distinct, so TABLE
83+
* rows would otherwise never group together.
84+
*/
85+
private static final String LOSERS = """
86+
SELECT id, keep_id FROM (
87+
SELECT id,
88+
first_value(id) OVER w AS keep_id,
89+
row_number() OVER w AS rn
90+
FROM schema_documentation
91+
WINDOW w AS (
92+
PARTITION BY connection_id, object_type, object_name,
93+
coalesce(parent_object, ''), source
94+
ORDER BY created_at DESC NULLS LAST, id DESC
95+
)
96+
) ranked WHERE rn > 1
97+
""";
98+
99+
private boolean tableExists(JdbcTemplate jdbc, String tableName) {
100+
Integer count = jdbc.queryForObject("""
101+
SELECT COUNT(*)
102+
FROM information_schema.tables
103+
WHERE table_schema = 'public' AND table_name = ?
104+
""", Integer.class, tableName);
105+
return count != null && count > 0;
106+
}
107+
108+
private boolean indexExists(JdbcTemplate jdbc, String indexName) {
109+
Integer count = jdbc.queryForObject("""
110+
SELECT COUNT(*)
111+
FROM pg_indexes
112+
WHERE schemaname = 'public' AND indexname = ?
113+
""", Integer.class, indexName);
114+
return count != null && count > 0;
115+
}
116+
}

‎backend/src/main/java/com/dbaagent/repository/CodeKnowledgeSuggestionRepository.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,44 @@
33
import com.dbaagent.model.code.CodeKnowledgeSuggestion;
44
import org.springframework.data.domain.Page;
55
import org.springframework.data.domain.Pageable;
6+
import jakarta.persistence.LockModeType;
67
import org.springframework.data.jpa.repository.JpaRepository;
8+
import org.springframework.data.jpa.repository.Lock;
9+
import org.springframework.data.jpa.repository.Modifying;
10+
import org.springframework.data.jpa.repository.Query;
11+
import org.springframework.data.repository.query.Param;
712
import org.springframework.stereotype.Repository;
813

14+
import java.util.Collection;
915
import java.util.List;
16+
import java.util.Optional;
1017

1118
@Repository
1219
public interface CodeKnowledgeSuggestionRepository extends JpaRepository<CodeKnowledgeSuggestion, String> {
1320

21+
/**
22+
* Row-locking load used by approve/reject. Without it two concurrent bulk
23+
* decides both read the same suggestion as PENDING and both materialize a
24+
* {@code schema_documentation} row — the duplicate-row bug that
25+
* {@code V116__dedupe_schema_documentation.sql} had to clean up. The second
26+
* caller now blocks, then sees APPROVED and returns early.
27+
*/
28+
@Lock(LockModeType.PESSIMISTIC_WRITE)
29+
@Query("SELECT s FROM CodeKnowledgeSuggestion s WHERE s.id = :id")
30+
Optional<CodeKnowledgeSuggestion> findByIdForUpdate(@Param("id") String id);
31+
32+
/**
33+
* Repoints approvals at the surviving row when duplicate schema_documentation
34+
* rows are collapsed. {@code applied_doc_id} is a loose reference, not an FK,
35+
* so deleting a duplicate would otherwise leave a suggestion pointing at a row
36+
* that no longer exists — silently, since nothing enforces it.
37+
*/
38+
@Modifying(flushAutomatically = true)
39+
@Query("UPDATE CodeKnowledgeSuggestion s SET s.appliedDocId = :keepId "
40+
+ "WHERE s.appliedDocId IN :staleIds")
41+
int repointAppliedDocId(@Param("keepId") String keepId,
42+
@Param("staleIds") Collection<String> staleIds);
43+
1444
Page<CodeKnowledgeSuggestion> findByConnectionIdAndStatus(
1545
String connectionId,
1646
CodeKnowledgeSuggestion.Status status,

‎backend/src/main/java/com/dbaagent/repository/SchemaDocumentationRepository.java‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,15 @@ List<SchemaDocumentation> findByConnectionIdAndObjectType(
2121
SchemaDocumentation.DocumentationType objectType
2222
);
2323

24-
Optional<SchemaDocumentation> findByConnectionIdAndObjectTypeAndObjectName(
24+
/**
25+
* Returns a {@link List}, never an {@link Optional} — the logical key is not
26+
* unique in data written before {@code V116__dedupe_schema_documentation.sql}
27+
* added the constraint, and an {@code Optional} finder throws
28+
* {@code IncorrectResultSizeDataAccessException} on a legacy duplicate rather
29+
* than letting the caller repair it. Collapse matches with
30+
* {@link com.dbaagent.service.SchemaDocumentationDeduplicator}.
31+
*/
32+
List<SchemaDocumentation> findByConnectionIdAndObjectTypeAndObjectName(
2533
String connectionId,
2634
SchemaDocumentation.DocumentationType objectType,
2735
String objectName
@@ -51,12 +59,13 @@ AND TRIM(d.businessTerms) <> ''
5159
""")
5260
long countWithBusinessTerms(@Param("connectionId") String connectionId);
5361

54-
// Upsert support: find existing AI doc to update instead of creating duplicates
55-
Optional<SchemaDocumentation> findByConnectionIdAndObjectTypeAndObjectNameAndSource(
62+
// Upsert support: find existing doc to update instead of creating duplicates.
63+
// List-returning for the same reason as findByConnectionIdAndObjectTypeAndObjectName above.
64+
List<SchemaDocumentation> findByConnectionIdAndObjectTypeAndObjectNameAndSource(
5665
String connectionId, SchemaDocumentation.DocumentationType objectType,
5766
String objectName, DocumentationSource source);
5867

59-
Optional<SchemaDocumentation> findByConnectionIdAndObjectTypeAndObjectNameAndParentObjectAndSource(
68+
List<SchemaDocumentation> findByConnectionIdAndObjectTypeAndObjectNameAndParentObjectAndSource(
6069
String connectionId, SchemaDocumentation.DocumentationType objectType,
6170
String objectName, String parentObject, DocumentationSource source);
6271

‎backend/src/main/java/com/dbaagent/service/SchemaDescriptionService.java‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ public class SchemaDescriptionService {
3434
private final ColumnProfileRepository columnProfileRepo;
3535
private final InferredTableRelationshipRepository inferredRelationshipRepository;
3636
private final TrainingService trainingService;
37+
private final SchemaDocumentationDeduplicator schemaDocDeduplicator;
3738
private final ConnectionService connectionService;
3839
private final DatabaseProviderRegistry providerRegistry;
3940
private final ObjectMapper objectMapper = new ObjectMapper();
@@ -71,6 +72,7 @@ public SchemaDescriptionService(
7172
ColumnProfileRepository columnProfileRepo,
7273
InferredTableRelationshipRepository inferredRelationshipRepository,
7374
TrainingService trainingService,
75+
SchemaDocumentationDeduplicator schemaDocDeduplicator,
7476
ConnectionService connectionService,
7577
DatabaseProviderRegistry providerRegistry,
7678
@Value("${brain.description.ai-concurrency:4}") int aiConcurrency) {
@@ -80,6 +82,7 @@ public SchemaDescriptionService(
8082
this.columnProfileRepo = columnProfileRepo;
8183
this.inferredRelationshipRepository = inferredRelationshipRepository;
8284
this.trainingService = trainingService;
85+
this.schemaDocDeduplicator = schemaDocDeduplicator;
8386
this.connectionService = connectionService;
8487
this.providerRegistry = providerRegistry;
8588
this.aiConcurrency = Math.max(1, aiConcurrency);
@@ -417,13 +420,14 @@ private int saveTableDescription(String connectionId, TableDescription desc,
417420
: desc.getTableName();
418421

419422
// Upsert table-level doc (find existing AI doc or create new)
420-
var existingTableDoc = schemaDocRepo
421-
.findByConnectionIdAndObjectTypeAndObjectNameAndSource(
423+
var existingTableDoc = schemaDocDeduplicator.collapse(
424+
schemaDocRepo.findByConnectionIdAndObjectTypeAndObjectNameAndSource(
422425
connectionId, DocumentationType.TABLE, objectName,
423-
DocumentationSource.AI_GENERATED);
426+
DocumentationSource.AI_GENERATED),
427+
objectName + " (TABLE, AI_GENERATED)");
424428
SchemaDocumentation tableDoc;
425-
if (existingTableDoc.isPresent()) {
426-
tableDoc = existingTableDoc.get();
429+
if (existingTableDoc != null) {
430+
tableDoc = existingTableDoc;
427431
tableDoc.setDescription(desc.getTableDescription());
428432
tableDoc.setBusinessTerms(desc.getBusinessTerms());
429433
tableDoc.setConfidence(desc.getConfidence());
@@ -445,13 +449,14 @@ private int saveTableDescription(String connectionId, TableDescription desc,
445449
// Upsert column-level docs
446450
for (var col : desc.getColumns()) {
447451
if (col.getDescription() == null || col.getDescription().isBlank()) continue;
448-
var existingColDoc = schemaDocRepo
449-
.findByConnectionIdAndObjectTypeAndObjectNameAndParentObjectAndSource(
452+
var existingColDoc = schemaDocDeduplicator.collapse(
453+
schemaDocRepo.findByConnectionIdAndObjectTypeAndObjectNameAndParentObjectAndSource(
450454
connectionId, DocumentationType.COLUMN, col.getName(),
451-
objectName, DocumentationSource.AI_GENERATED);
455+
objectName, DocumentationSource.AI_GENERATED),
456+
objectName + "." + col.getName() + " (COLUMN, AI_GENERATED)");
452457
SchemaDocumentation colDoc;
453-
if (existingColDoc.isPresent()) {
454-
colDoc = existingColDoc.get();
458+
if (existingColDoc != null) {
459+
colDoc = existingColDoc;
455460
colDoc.setDescription(col.getDescription());
456461
colDoc.setConfidence(col.getConfidence());
457462
} else {

0 commit comments

Comments
 (0)