Merge branch 'main' into kaushik-IDE

Author: venkateshsakamuri-lab

.gitignore

@@ -104,3 +104,7 @@ optd-sidecar/target/
 *.iml
 .local-admin-credentials
 .local-mcp-token
+
+# Python bytecode from scripts/
+__pycache__/
+*.pyc

AGENTS.md

@@ -212,6 +212,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)
 

CLAUDE.md

@@ -388,6 +388,15 @@ The Agent tab must not inherit the admin MCP token. `/api/agent/session` mints a
    resilience but only ever handled a *moved ref*, re-issuing the identical refused
    request against a 429. A fallback that fails the same way as the thing it backs
    up is not a fallback.
+7. **Never offer a write the caller cannot enforce.** `SOUL.md` once asked
+   "Should everyone on this database see this?" after every good answer, so
+   Agent chat offered "save this as a shared DeepSQL brain note" to users
+   without `canManageContent` and then 403'd. `get_brain_context` now stamps
+   `callerCapabilities`; if `doNotOffer` includes `save_brain_note`, the
+   agent must not mention it. MCP `save_brain_note` also fail-closes before
+   the POST. Admins get a non-blocking suggestion bubble only after they
+   correct or teach the Agent (`POST /brain/notes/propose` + accept) — a
+   clean first answer stays quiet. Overlaps merge into one intent.
 
 ### Verification Anti-Patterns (do not repeat)
 
@@ -406,6 +415,16 @@ broken. Assert the *outcome*, never the attempt:
 - **Mocks hide SDK breaks.** `tests/tools/test_mcp_structured_content.py` uses a
   `_FakeCallToolResult` with a hardcoded `.isError`, so it kept passing precisely when
   the real SDK stopped matching. Pin the dependency; a fake cannot catch this.
+- **A self-host verification script must reach the DB the way the install does.**
+  `seed-review-suggestions.py` / `e2e-review-approvals.py` hardcoded
+  `sudo -u postgres psql`, which only exists on a bare-metal install — on the Compose
+  deployment `install.sh` actually produces, the documented verify command died before
+  testing anything. Both now resolve the path through `scripts/self-host/vaultdb.py`.
+- **A test that mutates shared state must restore it, and only what it created.** The
+  same e2e suite parked every real `CODE_DERIVED` row by rewriting `source` to `USER`
+  and never restored it, so a run against a live install silently relabelled the user's
+  approved docs. It now copies rows to a scratch table and restores them, and its
+  cleanup deletes the planted row only while nothing references it.
 - **Never claim a check you did not run.** `install.sh` reported "up to date" when it
   could not reach npm; it now says it could not check.
 - **`set -e` + `read` at EOF aborts silently.** Prompts in `install.sh` use
@@ -494,6 +513,53 @@ it against a real database — not a theoretical hardening pass.
   `POST /users/admin/reset` on every install that had run `setup-agent.sh`, since that
   mints an admin MCP token on each run.
 
+- **An `Optional`-returning derived finder is an assertion that the key is unique.**
+  Spring Data throws `IncorrectResultSizeDataAccessException` ("Query did not return a
+  unique result: N results were returned") the moment it is not, and the row that broke
+  it never repairs itself, so the failure is permanent rather than transient.
+  `schema_documentation` had 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, `CodeScanService.bulkDecide` swallowed it per item, and the Review queue
+  reported "Approved 0 of 2" — with all 198 pending SCHEMA_DOC suggestions wedged.
+  Three-part fix, and all three are load-bearing:
+  1. `V116__dedupe_schema_documentation.sql` + `SchemaDocumentationDedupeInitializer`
+     (no Flyway here, so the initializer is what actually applies it) collapse the
+     duplicates and add `ux_schema_doc_target`, keyed on
+     `coalesce(parent_object,'')` because Postgres treats NULLs as distinct.
+  2. Those finders now return `List`, and `SchemaDocumentationDeduplicator.collapse`
+     keeps the newest row, repoints any `applied_doc_id` off the rows it deletes, and
+     drops their RAG embeddings. Do not restore an `Optional` variant — legacy installs
+     still carry duplicates until the initializer runs.
+  3. `approve`/`reject` load the suggestion via `findByIdForUpdate` (`PESSIMISTIC_WRITE`)
+     so the concurrent double-submit that created the duplicates blocks instead of racing.
+- **`applied_doc_id` is a loose reference, not an FK.** Deleting a `schema_documentation`
+  row it points at raises nothing and dangles silently — repoint before deleting.
+- **Approve *updates* the row an earlier scan wrote**, so a "freshly approved" doc row
+  carries a historical `created_at`. A test that plants an "old" duplicate with a
+  hardcoded past date can easily plant the *newer* of the two and assert nothing; anchor
+  fixture timestamps to the real row's `created_at`.
+- **A write's blast radius decides what to invalidate, not the endpoint you called.**
+  Approving a code-scan suggestion writes `code_knowledge_suggestion` *and*
+  `schema_documentation` (served by `brain/notes`, which backs the Write-notes tab
+  and its coverage counts) *and* `rag_documents` *and*, for KNOWLEDGE_ENTRY, a
+  company knowledge entry. The decide hooks invalidated only `codeScan` +
+  `companyKnowledge`, so every schema-doc-derived count stayed stale until the user
+  reloaded the page. `invalidateAfterDecision` in `useCodeScan.js` is the single
+  place that lists them; add to it when an approval starts writing something new.
+- **`@PreUpdate` does not fire on insert, so `updatedAt` is null on a brand-new row.**
+  Sorting "newest first" on `updatedAt` alone with nulls last therefore sends every
+  freshly created row to the *bottom* — which is why a just-approved note did not
+  appear at the top of the Write-notes list. Sort on
+  `COALESCE(updatedAt, createdAt)` (`BrainNoteService.touchedAt`,
+  `CompanyKnowledgeEntryRepository.findByConnectionIdOrderByRecency`).
+- **Suggestion list order depends on the status being viewed.** PENDING is a work
+  queue → `confidence DESC`. APPROVED/REJECTED are history → `decidedAt DESC NULLS
+  LAST` so the decision you just made is at the top; confidence-sorting a decided
+  list scattered fresh approvals among hundreds of older ones
+  (`CodeScanService.sortFor`).
+
 ### Endpoint Authorization Rules
 
 - **Authentication is not authorization.** `SecurityConfig` only asserts

agent/SOUL.md

@@ -2,7 +2,7 @@ You are **DeepSQL DBA**, an AI database performance assistant. You answer questi
 
 **Lead with the answer.** You ground thoroughly with the tools, but you do **not** narrate that work in your reply. No "I checked / I joined…", no "Grounding used", no "Filters applied", no "Used:" footnotes, no column/filter walkthroughs. Answer with just the result — a number, a short ranked table, or a one-line sentence — and apply business rules silently. Tool steps already show what ran; don't repeat that in the bubble.
 
-After the answer you may offer **one short follow-up question** (a single line) when it helps the user go deeper. Do not stack multiple offers. If the user wants the SQL, the tables, or how you got there, they'll ask, and then you show it. Admit uncertainty instead of guessing; prefer one correct answer over a verbose survey.
+After the answer you may offer **one short follow-up question** (a single line) when it helps the user go deeper — a question they can answer, not an action they cannot take. Do not stack multiple offers. If the user wants the SQL, the tables, or how you got there, they'll ask, and then you show it. Admit uncertainty instead of guessing; prefer one correct answer over a verbose survey.
 
 (Exception: the schema-consult flow in rule 8 — when proposing a table/migration you DO briefly state what already exists, because that's the point of the consult.)
 
@@ -24,25 +24,35 @@ After the answer you may offer **one short follow-up question** (a single line)
 
 8. **Consult before you commit schema.** When the user says "add a table / track X / write a migration," STOP and run the brain consult (`get_brain_context` → `get_schema` → `list_business_rules` → `get_relationships` → `get_anti_patterns`). There is almost always an existing table or column to extend instead of duplicate. Narrate what you found before proposing DDL.
 
+9. **Never offer an action the caller cannot enforce.** `get_brain_context` and
+   `list_connections` carry `callerCapabilities`. If `doNotOffer` lists an
+   action — especially `save_brain_note` — do not mention it, do not ask
+   "should I save this", and do not render a Yes button. Answering a metric
+   is not a request to persist it. The product UI may show a non-blocking
+   save bubble after the user corrects or teaches a definition; leave that
+   to the UI. Never volunteer it yourself.
+
 ## Remembering things — two different places
 
-There are TWO planes of memory. Route every "remember this" to the right one:
+There are TWO planes of memory. Route a remember request only when the user
+explicitly asked to remember / pin / save a definition:
 
 1. **Company brain context (shared).** Durable facts about the *data* — what a
    column means, a join path, a business definition, an accepted recommendation.
    These ground EVERYONE's answers on this connection. Save them with
-   **`save_brain_note(connectionId, tableName, noteText, columnName?)`**.
-   - "Accept this recommendation" / "remember this for the team" → review with
-     **`list_brain_recommendations`**, then `save_brain_note` for each good one.
-   - This is **admin-only** (manage-content) and audited. If the user lacks
-     permission, the backend rejects it — say so, don't work around it.
+   **`save_brain_note(connectionId, tableName, noteText, columnName?)`**
+   **only if** `callerCapabilities.canWriteSharedBrainNotes` is true
+   (`list_connections.canManageContent`).
+   - If they asked to remember and they cannot write: tell them an admin with
+     manage-content on this connection has to save it. Do not call the tool.
 2. **Individual preference (yours alone).** How *this* user likes answers
    formatted, a private shortcut, a personal default. That is a **DeepSQL
    skill** on the user's own profile — it does NOT belong in the shared brain.
    Never push a personal preference into `save_brain_note`.
 
-When unsure which plane a request belongs to, ask: "Should everyone on this
-database see this, or just you?" Shared → brain note. Just you → DeepSQL skill.
+Do not volunteer a shared-brain save after answering a data question. Do not
+ask "should everyone on this database see this?" unless the user already asked
+to remember something **and** they can write shared notes.
 
 ## Skills
 

agent/skills/bi-query/SKILL.md

@@ -27,7 +27,7 @@ Use when the user asks a question whose answer is **in the data** ("how many boo
 
 6. **Run it** with `execute_sql(connectionId, sql, limit=…)`. Remember: default 100 rows, max 1000. For a total, `SELECT COUNT(*)` rather than counting a truncated result set.
 
-7. **Answer only.** Reply with just the result — the number or a short ranked table — then optionally **one** short follow-up question. Apply business rules silently; do NOT append "Grounding used" / "Filters applied" / "Used:" / tool-narration / column-mapping sections. Only if the user asks how you got it do you show the tables, joins, and filters.
+7. **Answer only.** Reply with just the result — the number or a short ranked table — then optionally **one** short follow-up question the user can actually act on. Do **not** offer to save a shared brain note, apply an index, or run DDL/DML unless `get_brain_context.callerCapabilities` says they can. Apply business rules silently; do NOT append "Grounding used" / "Filters applied" / "Used:" / tool-narration / column-mapping sections. Only if the user asks how you got it do you show the tables, joins, and filters.
 
 ## Guardrails
 

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

@@ -0,0 +1,116 @@
+package com.dbaagent.config;
+
+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 org.springframework.transaction.support.TransactionTemplate;
+import org.springframework.transaction.PlatformTransactionManager;
+
+import javax.sql.DataSource;
+
+/**
+ * Applies {@code V116__dedupe_schema_documentation.sql} at startup: collapses
+ * duplicate {@code schema_documentation} rows and adds the unique index on the
+ * logical key.
+ *
+ * <p>This repo has no Flyway runtime — {@code db/migration} is a hand-maintained
+ * changelog and Hibernate {@code ddl-auto=update} never adds an index the entity
+ * does not declare. Without this, self-host installs carrying duplicates from a
+ * double-submitted bulk approve stay wedged: every SCHEMA_DOC approve throws
+ * {@code Query did not return a unique result}. Mirrors
+ * {@link SchemaDocumentationSourceCompatibilityInitializer}.
+ *
+ * <p>Idempotent and cheap on a clean install: the index exists, so it returns
+ * before touching a row.
+ */
+@Configuration
+@Slf4j
+public class SchemaDocumentationDedupeInitializer {
+
+    private static final String TABLE = "schema_documentation";
+    private static final String INDEX = "ux_schema_doc_target";
+
+    @Bean("schemaDocumentationDedupeBootstrap")
+    @DependsOn("entityManagerFactory")
+    public Object schemaDocumentationDedupeBootstrap(DataSource dataSource,
+                                                     PlatformTransactionManager txManager) {
+        JdbcTemplate jdbc = new JdbcTemplate(dataSource);
+        if (!tableExists(jdbc, TABLE)) {
+            return new Object();
+        }
+        if (indexExists(jdbc, INDEX)) {
+            return new Object();
+        }
+
+        // One transaction: a half-applied dedupe (rows deleted, index missing)
+        // would silently re-accumulate duplicates until the next boot.
+        new TransactionTemplate(txManager).executeWithoutResult(status -> {
+            int repointed = jdbc.update("""
+                UPDATE code_knowledge_suggestion s
+                SET applied_doc_id = l.keep_id
+                FROM (%s) l
+                WHERE s.applied_doc_id = l.id
+                """.formatted(LOSERS));
+
+            int embeddings = jdbc.update(
+                "DELETE FROM rag_documents WHERE id IN (SELECT id FROM (%s) l)".formatted(LOSERS));
+
+            int removed = jdbc.update(
+                "DELETE FROM schema_documentation WHERE id IN (SELECT id FROM (%s) l)".formatted(LOSERS));
+
+            jdbc.execute("""
+                CREATE UNIQUE INDEX IF NOT EXISTS %s
+                ON %s (connection_id, object_type, object_name, coalesce(parent_object, ''), source)
+                """.formatted(INDEX, TABLE));
+
+            if (removed > 0) {
+                log.warn("Deduped {}: removed {} duplicate rows, {} orphaned embeddings, "
+                        + "repointed {} applied_doc_id references",
+                    TABLE, removed, embeddings, repointed);
+            }
+            log.info("Ensured unique index {} on {}", INDEX, TABLE);
+        });
+        return new Object();
+    }
+
+    /**
+     * Every row but the newest within each logical key. Newest wins because it is
+     * the row existing {@code applied_doc_id} references point at; {@code id}
+     * breaks ties for rows written in the same clock tick. {@code coalesce} on
+     * {@code parent_object} because Postgres treats NULLs as distinct, so TABLE
+     * rows would otherwise never group together.
+     */
+    private static final String LOSERS = """
+        SELECT id, keep_id FROM (
+            SELECT id,
+                   first_value(id) OVER w AS keep_id,
+                   row_number()    OVER w AS rn
+            FROM schema_documentation
+            WINDOW w AS (
+                PARTITION BY connection_id, object_type, object_name,
+                             coalesce(parent_object, ''), source
+                ORDER BY created_at DESC NULLS LAST, id DESC
+            )
+        ) ranked WHERE rn > 1
+        """;
+
+    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 indexExists(JdbcTemplate jdbc, String indexName) {
+        Integer count = jdbc.queryForObject("""
+                SELECT COUNT(*)
+                FROM pg_indexes
+                WHERE schemaname = 'public' AND indexname = ?
+                """, Integer.class, indexName);
+        return count != null && count > 0;
+    }
+}