Skip to content

fix(brain): unwedge suggestion approval and refresh knowledge counts live - #77

Merged
venkateshsakamuri-lab merged 2 commits into
mainfrom
fix/schema-doc-duplicate-upsert
Aug 22, 2026
Merged

fix(brain): unwedge suggestion approval and refresh knowledge counts live#77
venkateshsakamuri-lab merged 2 commits into
mainfrom
fix/schema-doc-duplicate-upsert

Conversation

@geekypunk

Copy link
Copy Markdown
Contributor

Follow-up to #74. That PR made the failure visible (bulk decide now returns failures[], and the UI renders a partial/zero success as an error) but the approval itself still threw — its diagnosis, a schema_documentation_source_check missing CODE_DERIVED, did not apply to the reporting install, where the CHECK was already correct and the log said something else entirely.

What was actually failing

WARN CodeScanService : bulk decide skipped e2011614-…: Query did not return a unique result: 2 results were returned
WARN CodeScanService : bulk decide skipped f3689e0e-…: Query did not return a unique result: 2 results were returned

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 (pairs ~3s apart; in every group the older row is an orphan and the newer one holds the applied_doc_id). Every later approve landing on such a key threw out of the Optional-returning upsert finder, and bulk-decide swallowed it per item. The duplicate never self-heals, so all 198 pending SCHEMA_DOC suggestions were permanently unapprovable.

Fix

1. Duplicate-tolerant upsert. The three finders now return List, so the compiler forces every caller to handle N matches. SchemaDocumentationDeduplicator keeps the newest row, repoints any applied_doc_id off the rows it deletes (a loose reference, not an FK — a dangling value fails silently), and drops their RAG embeddings. Applied at all four call sites, including SchemaDriftListener, which would have thrown identically the first time one of 17 duplicated tables was dropped.

2. Data repair + constraint. V116__dedupe_schema_documentation.sql, applied by SchemaDocumentationDedupeInitializer — there is no Flyway runtime here, so a SQL file alone would never run. Idempotent: it returns before touching a row once the index exists. coalesce(parent_object,'') in the key because Postgres treats NULLs as distinct.

3. The root cause. approve/reject load the suggestion FOR UPDATE, so the concurrent double-submit blocks and the second caller sees APPROVED.

Also fixed (reported after the first fix landed)

  • Counts stale until reload. 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. invalidateAfterDecision now covers brain and schemaContext too.
  • Newest notes sorted last. @PreUpdate never fires 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. Now COALESCE(updatedAt, createdAt), matching CompanyKnowledgeEntryRepository.
  • Fresh approvals buried in the Approved view. listSuggestions sorted every status by confidence. PENDING stays confidence-first (it is a work queue); decided statuses now sort by decidedAt DESC NULLS LAST.

Two bugs in #74's own test tooling

  • Both scripts hardcoded sudo -u postgres psql, which does not exist on the Compose deployment install.sh produces — the verify command in fix: Review queue approvals (CODE_DERIVED + stale list + bulk errors) #74's description failed before testing anything. Now resolved through scripts/self-host/vaultdb.py.
  • e2e-review-approvals.py step 10 rewrote every real CODE_DERIVED row to source='USER' and never restored it. Running it against a live install silently relabelled 339 approved docs, and post-V116 it would collide with the unique index. It now parks rows in a scratch table and restores them with a verified count, and its cleanup deletes the planted row only while nothing references it.

Verification

Run against the live self-host stack, not just unit tests.

  • Initializer on the affected install: removed 219 duplicate rows, 219 orphaned embeddings; 7068 → 6849 doc rows, 8190 → 7971 RAG rows, 0 duplicate groups, 0 dangling applied_doc_id. Skipped cleanly on restart.
  • The two originally stuck suggestions: {"requested": 2, "succeeded": 2, "failed": 0}.
  • Ordering, approving the three lowest-confidence items so the two sorts disagree — all three land above older conf=0.97 rows.
  • brain/notes 4607 → 4608 on a first-time doc (the count that never refreshed), and the just-approved crm.customers now sorts first.
  • 21 backend unit tests green (4 new on collapse, 2 new on note ordering). 184 related tests run; the 6 failures in TrainingServiceBusinessTermTest / BrainInitStageExecutorTest reproduce identically on pristine main and are unrelated.
  • E2E suite: 30/30 pass, including duplicate collapse, applied_doc_id repointing, the unique index rejecting a second row, and two genuinely concurrent approves writing exactly one row.
python3 scripts/self-host/seed-review-suggestions.py <connectionId> --count 20
python3 scripts/self-host/e2e-review-approvals.py <connectionId>
# ✓ All review-approval edge cases passed

Note: the suite consumes its own fixtures, so re-run the seed before each run.

🤖 Generated with Claude Code

https://claude.ai/code/session_018mG2xj9gWJ8WzfDP2fDePP

…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
@geekypunk
geekypunk requested a review from a team as a code owner August 22, 2026 17:50
@venkateshsakamuri-lab
venkateshsakamuri-lab merged commit ecf954f into main Aug 22, 2026
9 checks passed
@venkateshsakamuri-lab
venkateshsakamuri-lab deleted the fix/schema-doc-duplicate-upsert branch August 22, 2026 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants