From 1f183b17eb8e7640a25d5a43bd3d0b1d1c5735d9 Mon Sep 17 00:00:00 2001 From: NGHTBOY Date: Thu, 27 Aug 2026 03:17:54 +0200 Subject: [PATCH] fix(knowledge): the guard did not apply on the way in, so the store never cleaned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deploying the guard was not enough, and measuring after the deploy is how that was found. A full re-index ran in production WITH the guard live — 9 981 files detected, 8 644 parsed, graph rebuilt to 25 491 symbols — and project_cache.knowledge_json, written at 01:00:16, still held 48 implausible names: CASCADE CURRENT_TIMESTAMP GET IF Our SI 0 200 1328 ases bs … The casing and the bare integers gave it away: the guard rejects all of those on write, so they were not produced. They were LOADED. `ProjectKnowledge.from_json` restored table_usage with no filter, and `_incremental_update` carried the cache forward the same way, so a name recorded before the guard existed came back every run, was re-saved, and outlived any number of clean extractions. Clearing it would have needed a manual purge nobody schedules. The guard applies on both boundaries now. One run cleans the store. Verified: loading 13 names — 10 noise, 3 real — yields exactly the three real ones, and a real name survives a round trip with its readers intact. Second change — the map is part of a repo index, not an ingestion automation: `auto_sync_after_index` defaults ON and leaves the ingestion-automation family. That family's rule is "nothing calls out on a schedule unasked" and the flag never qualified: code_db_sync_pipeline.py has zero references to adapter, connector, execute_query, introspect, httpx or aiohttp. The sync opens no connection to anything — its inputs are the stored DbIndex and the code knowledge, both local, and its output is the code↔DB map, which is what a repo index produces. The mis-grouping's consequence appeared the moment it was tested: unset on 2026-08-25 with eight genuine ingestion flags, it meant the re-index above rebuilt the graph while the map kept the previous night's updated_at. The flag stays, for the reason the old grouping never named: the sync runs an LLM (CodeDbSyncAnalyzer), so it costs tokens per run. One of the new tests fails if the sync ever grows an outward call, because that would make the classification wrong again. Production has the flag unset, so the new default applies without a config change and `make config-drift` stays clean. The config_drift fixture that listed it among the ten 2026-08-23 divergences now records WHY it left rather than being edited silently. Suite: 6922 passed, 4 skipped, 1 xfailed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XtEZboEDbEXLYrLf1owRXB --- CHANGELOG.md | 45 +++++++ CLAUDE.md | 10 +- backend/app/config.py | 24 +++- backend/app/knowledge/entity_extractor.py | 16 +++ .../unit/docs/test_config_drift_script.py | 18 ++- .../test_table_name_is_not_invented.py | 67 ++++++++++ .../test_the_map_is_part_of_the_index.py | 119 ++++++++++++++++++ 7 files changed, 293 insertions(+), 6 deletions(-) create mode 100644 backend/tests/unit/knowledge/test_the_map_is_part_of_the_index.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cc2c89d..717ce5ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,51 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed — the table-name guard did not apply on the way in, so the store never cleaned + +Deploying the guard was not enough, and measuring after the deploy is how that was found. +A full re-index ran in production **with the guard live** — 9 981 files detected, 8 644 +parsed, graph rebuilt to 25 491 symbols — and `project_cache.knowledge_json`, written at +01:00:16, still held **48** implausible names: + + CASCADE CURRENT_TIMESTAMP GET IF Our SI 0 200 1328 ases bs … + +The casing and the bare integers gave it away: the guard rejects all of those on write, so +they were not produced. They were **loaded**. `ProjectKnowledge.from_json` restored +`table_usage` with no filter, and `_incremental_update` carried the cache forward the same +way — so a name recorded before the guard existed came back on every run, was re-saved, +and outlived any number of clean extractions. Clearing it would have needed a manual purge +nobody schedules. + +The guard now applies on both boundaries. One run cleans the store. Verified: loading 13 +names — 10 noise, 3 real — yields exactly the three real ones, and a real name survives a +full round trip with its readers intact. + +### Changed — the code↔DB map is part of a repo index, not an ingestion automation + +`auto_sync_after_index` **defaults ON** and has left the ingestion-automation family. + +The family's rule is *nothing calls out on a schedule unasked*, and the flag never +qualified. Measured against `code_db_sync_pipeline.py`: zero references to `adapter`, +`connector`, `execute_query`, `introspect`, `httpx` or `aiohttp`. The sync opens no +connection to anything — its inputs are the stored `DbIndex` and the code knowledge, both +already local, and its output is the code↔DB map, which is what a repo index *produces*. + +The mis-grouping had a consequence, and it appeared the moment it was tested: unset in +production on 2026-08-25 alongside eight genuine ingestion flags, it meant the full +re-index above rebuilt the graph while the map kept the previous night's `updated_at`. +"The index ran and the map did not" is the wrong default for a product whose value is that +map's freshness. + +**The flag stays, for the reason the old grouping never named.** The sync runs an LLM +(`CodeDbSyncAnalyzer`), so it costs tokens per run. Turning it off now switches off a cost +rather than a phantom outward call — and one of the new tests fails if the sync ever grows +an outward call, because that would make the classification wrong again. + +Production has the flag unset, so the new default applies there without a config change, +and `make config-drift` stays clean. The `config_drift` fixture that listed it among the +ten 2026-08-23 divergences now records *why* it left rather than being edited silently. + ### Fixed — the code↔DB map invented 33 of the 39 tables it named This is the product's core promise for a repository added to a dashboard: the agent goes diff --git a/CLAUDE.md b/CLAUDE.md index 66e0d7bc..cae7b97e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -378,9 +378,15 @@ A timeout also gets **exactly one** LLM repair, prompted to narrow scope rather **Intelligence remediation W5 landmarks** (code↔DB trust signals, SYNC-L2/L3/L5/L6/L7/L8/L9 + low-batch L11–L14): `classify_freshness()` in `git_tracker.py` uses `iter_commits` for exact AHEAD/BEHIND/DIVERGED states (L3); `KnowledgeFreshnessService` maps each state to a distinct warning + severity (DIVERGED=critical); `EntityExtractor` attributes SQL column refs per-statement with noise-token stripping (L2) — **and that stripping was insufficient for table NAMES until 2026-08-27**: it blanked comments and string literals but left six SQL keywords as the only filter, so `TABLE_REF_SQL` (`FROM|JOIN|INTO|UPDATE|TABLE` + a word) also matched prose, and `_model_name_to_table` pluralised one- and two-character tokens into plausible names. Production's code↔DB map for the one real customer named 39 tables of which **six existed** — see `is_plausible_table_name` and `tables_declared_in_migration`; `_compute_column_drift()` produces deterministic sorted set-diff overriding LLM `sync_status` when both sides are known (L5); migration `c9b8a7f6e5d4` adds `CodeDbSync.column_mismatch_json`; sync loaders match on `(schema,table)` pair for schema-qualified ORM models (L6); bare-suffix keying normalised across all loaders (L7); empty-graph warning gated on `lineage_enabled OR clustering_enabled` (L8); op-kind uses word-boundary regex (L9); `_coerce_confidence` rounds floats before clamping (L11); `CallerRef.depth_estimated` sentinel replaces fabricated depth (L12); enum-table link uses word-boundary token matching (L13); DB-index TTL reads from `settings.db_index_ttl_hours` (L14). New config keys: `git_freshness_fetch_origin` (off — gates remote fetch for cross-machine accuracy; env `GIT_FRESHNESS_FETCH_ORIGIN`), `db_index_ttl_hours` (24; env `DB_INDEX_TTL_HOURS`). -**Ingestion automation (all off by default):** +**Ingestion automation (all off by default)** — the rule is *nothing calls out on a schedule unasked*, so membership is decided by whether the thing reaches outward, not by whether it is automatic: -`git_webhook_enabled`, `git_poll_enabled`, `auto_sync_after_index`, `freshness_reconciler_enabled`, `schema_change_alerts_enabled`. +`git_webhook_enabled`, `git_poll_enabled`, `freshness_reconciler_enabled`, `schema_change_alerts_enabled`. + +**`auto_sync_after_index` left this family on 2026-08-27 and now defaults ON.** It never belonged: measured against `code_db_sync_pipeline.py`, the sync has zero references to `adapter`, `connector`, `execute_query`, `introspect`, `httpx` or `aiohttp` — it opens no connection to anything. Its inputs are the stored `DbIndex` and the code knowledge, both already local, and its output is the code↔DB map, which is **what a repo index produces** rather than a separate ingestion. + +The grouping had a consequence. Unset in production on 2026-08-25 alongside eight genuine ingestion flags, it meant a full re-index on 2026-08-27 rebuilt the graph (25 491 symbols, 2 134 edges) while the map kept the previous night's `updated_at`. "The index ran and the map did not" is the wrong default for a product whose value is that map's freshness. + +The flag itself stays, for the reason the old grouping never named: the sync runs an LLM (`CodeDbSyncAnalyzer`), so it costs **tokens per run**. Turning it off now switches off a cost rather than a phantom outward call. **Analytics sources** (`docs/ANALYTICS_SOURCES.md`; all six read from `backend/app/config.py`, all validated at boot — a non-positive value raises rather than silently idling the collector): diff --git a/backend/app/config.py b/backend/app/config.py index ae0404a3..10655bf2 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -459,9 +459,27 @@ def _fix_database_url(self) -> "Settings": git_poll_enabled: bool = False git_poll_interval_minutes: int = 15 - # Auto-chain code↔DB sync after a successful repo index completes (closes the - # index→sync gap so lineage never silently lags the freshly indexed code). - auto_sync_after_index: bool = False + # Auto-chain code↔DB sync after a successful repo index completes. + # + # DEFAULT ON, and moved out of the ingestion-automation group above (2026-08-27). + # It sat there under the house rule "nothing calls out on a schedule unasked", and + # that rule has no claim on it: measured against `code_db_sync_pipeline.py`, the sync + # has zero references to `adapter`, `connector`, `execute_query`, `introspect`, + # `httpx` or `aiohttp`. It opens no connection to anything. Its inputs are the stored + # `DbIndex` and the code knowledge — both already local — and its output is the + # code↔DB map, which is *what a repo index produces*, not a separate ingestion. + # + # The grouping had a consequence. Unset in production on 2026-08-25 with eight + # genuine ingestion flags, it meant a full re-index on 2026-08-27 rebuilt the graph + # (25 491 symbols, 2 134 edges) and left the map carrying the previous night's + # `updated_at`. For a product whose value is that map's freshness, "the index ran and + # the map did not" is the wrong default. + # + # The flag stays, because a switch does earn its place — for a reason the old grouping + # never named. The sync runs an LLM (`CodeDbSyncAnalyzer.analyze_table`, + # `analyze_table_batch`, `generate_summary`), so it costs TOKENS per run. An operator + # who wants that off now turns off a cost, not a phantom outward call. + auto_sync_after_index: bool = True # FreshnessReconciler: when stale knowledge crosses the staleness threshold, # enqueue a background re-index instead of waiting for a user. Runs inside the diff --git a/backend/app/knowledge/entity_extractor.py b/backend/app/knowledge/entity_extractor.py index 7f03bcd8..30b4737d 100644 --- a/backend/app/knowledge/entity_extractor.py +++ b/backend/app/knowledge/entity_extractor.py @@ -156,6 +156,18 @@ def from_json(cls, raw: str) -> ProjectKnowledge: cols = [ColumnInfo(**c) for c in edata.pop("columns", [])] knowledge.entities[name] = EntityInfo(**edata, columns=cols) for tbl, udata in data.get("table_usage", {}).items(): + # The guard belongs on the READ side too, and that is not symmetry for its + # own sake. Table usage is persisted in `project_cache.knowledge_json` and + # restored on every run, so a name recorded before the guard existed comes + # back, gets re-saved, and outlives any number of clean extractions. + # + # Measured 2026-08-27: a full re-index ran WITH the guard deployed and the + # cache still held 48 implausible names — `CASCADE`, `CURRENT_TIMESTAMP`, + # `GET`, `IF`, `0`, `200`, `1328`, `ases`, `bs` — because they were loaded + # rather than produced. Filtering on load is what lets one run clean the + # store instead of needing a manual purge. + if not is_plausible_table_name(tbl): + continue knowledge.table_usage[tbl] = TableUsage(**udata) for edef in data.get("enums", []): knowledge.enums.append(EnumDefinition(**edef)) @@ -487,6 +499,10 @@ def _incremental_update( stale_set = set(changed_files) | set(deleted_files or []) for tbl, usage in cached.table_usage.items(): + # Same reason as `from_json`: an incremental run carries the cache forward, so + # without this the guard only ever applies to files that happened to change. + if not is_plausible_table_name(tbl): + continue new_usage = knowledge.table_usage.setdefault( tbl, TableUsage(table_name=tbl), diff --git a/backend/tests/unit/docs/test_config_drift_script.py b/backend/tests/unit/docs/test_config_drift_script.py index 3c3371fa..80c2d81a 100644 --- a/backend/tests/unit/docs/test_config_drift_script.py +++ b/backend/tests/unit/docs/test_config_drift_script.py @@ -96,9 +96,25 @@ def test_the_ten_that_were_found_are_reported_as_drift(drift) -> None: "RERANKER_ENABLED": "true", } drifted, recorded, unparseable = drift.compare(found_in_prod, drift.code_defaults()) - assert sorted(k for k, _, _ in drifted) == sorted(found_in_prod) + + # `AUTO_SYNC_AFTER_INDEX` left the set on 2026-08-27, and the reason is the point of + # keeping this fixture historical rather than editing it: the flag was not drifting, + # it was mis-grouped. It sat under the ingestion-automation rule ("nothing calls out + # on a schedule unasked") while the sync it gates calls out to nothing — zero + # references to `adapter`, `connector`, `execute_query`, `introspect`, `httpx` or + # `aiohttp` in `code_db_sync_pipeline.py`. Unsetting it made a full re-index leave its + # own code↔DB map carrying the previous night's timestamp, so the default is now on + # and `true` in production is agreement, not divergence. + # + # The other nine were genuine drift and still are. + was_misgrouped = {"AUTO_SYNC_AFTER_INDEX"} + assert sorted(k for k, _, _ in drifted) == sorted(set(found_in_prod) - was_misgrouped) assert recorded == [] assert unparseable == [] + assert drift.code_defaults()["AUTO_SYNC_AFTER_INDEX"] is True, ( + "if this default goes back to False, the flag is drift again and this test " + "should be the thing that says so" + ) def test_a_recorded_divergence_is_not_drift(drift) -> None: diff --git a/backend/tests/unit/knowledge/test_table_name_is_not_invented.py b/backend/tests/unit/knowledge/test_table_name_is_not_invented.py index ff67dc81..6c56c7dc 100644 --- a/backend/tests/unit/knowledge/test_table_name_is_not_invented.py +++ b/backend/tests/unit/knowledge/test_table_name_is_not_invented.py @@ -165,3 +165,70 @@ def test_an_alias_is_not_a_table(self) -> None: sql = "SELECT e.name FROM employees e WHERE e.id = 1" found = [t for t in TABLE_REF_SQL.findall(sql) if is_plausible_table_name(t)] assert found == ["employees"], found + + +class TestTheGuardAppliesOnTheWayInToo: + """A guard on production does not clean a store. + + `table_usage` is persisted in `project_cache.knowledge_json` and restored on every + run. Measured 2026-08-27: a full re-index ran with the production-side guard deployed + — 9 981 files detected, 8 644 parsed, graph rebuilt — and the cache still held **48** + implausible names afterwards. They were not produced; they were *loaded*, then + re-saved. Without a filter on `from_json` the noise outlives any number of clean + extractions, and clearing it needs a manual purge nobody schedules. + """ + + #: Verbatim from `project_cache.knowledge_json`, 2026-08-27 01:00:16, after the + #: rebuild. Note the casing and the bare integers: both are shapes the guard already + #: rejects, which is how it is known these were loaded rather than extracted. + LOADED_NOISE = [ + "0", + "1328", + "2", + "200", + "3", + "31", + "CASCADE", + "CURRENT_TIMESTAMP", + "GET", + "IF", + "Our", + "SI", + "a", + "an", + "and", + "any", + "ases", + "bs", + ] + + def test_deserialising_drops_them(self) -> None: + import json + + from app.knowledge.entity_extractor import ProjectKnowledge + + payload = { + "table_usage": { + name: {"table_name": name, "readers": [], "writers": [], "orm_refs": []} + for name in [*self.LOADED_NOISE, *REAL] + } + } + loaded = ProjectKnowledge.from_json(json.dumps(payload)) + assert sorted(loaded.table_usage) == sorted(REAL), sorted(loaded.table_usage) + + def test_the_real_names_survive_the_round_trip(self) -> None: + """Filtering on load is only safe if it is the same rule as on write.""" + import json + + from app.knowledge.entity_extractor import ProjectKnowledge + + payload = { + "table_usage": { + n: {"table_name": n, "readers": ["a.php"], "writers": [], "orm_refs": []} + for n in REAL + } + } + loaded = ProjectKnowledge.from_json(json.dumps(payload)) + again = ProjectKnowledge.from_json(loaded.to_json()) + assert sorted(again.table_usage) == sorted(REAL) + assert again.table_usage[REAL[0]].readers == ["a.php"], "payload must survive" diff --git a/backend/tests/unit/knowledge/test_the_map_is_part_of_the_index.py b/backend/tests/unit/knowledge/test_the_map_is_part_of_the_index.py new file mode 100644 index 00000000..7f98d31c --- /dev/null +++ b/backend/tests/unit/knowledge/test_the_map_is_part_of_the_index.py @@ -0,0 +1,119 @@ +"""The code↔DB map is what a repo index produces, not an ingestion automation. + +`auto_sync_after_index` sat in the ingestion-automation group in `config.py`, under this +project's house rule: *nothing calls out on a schedule unasked*. It was therefore `False` +by default and was unset in production on 2026-08-25 along with eight other flags of that +family. + +The consequence surfaced on 2026-08-27: a full re-index rebuilt the code graph — 25 491 +symbols, 2 134 edges — and the code↔DB map **did not move**. Its rows still carried +`updated_at` from the previous night. For a product whose value is that map's freshness, +"the index ran and the map did not" is the wrong default, and grouping decided it. + +**The grouping was wrong on the facts.** Measured against +`app/knowledge/code_db_sync_pipeline.py`: zero references to `adapter`, `connector`, +`execute_query`, `introspect`, `httpx`, `requests.` or `aiohttp`. The sync opens no +connection to anything. It reads the stored `DbIndex` and the code knowledge — both +already in this project's own database — and derives a map. It calls out to nowhere, so +the rule that exists to stop unasked outward calls has no claim on it. + +**A switch still earns its place, for a different reason.** The sync runs an LLM +(`CodeDbSyncAnalyzer` — `analyze_table`, `analyze_table_batch`, `generate_summary`), so it +costs tokens per run. That is a real reason an operator might want it off, and it is not +the reason it was off. The flag stays; what changes is its default, its group, and the +sentence next to it. +""" + +from __future__ import annotations + +from pathlib import Path + +from app.config import Settings + +PIPELINE = Path(__file__).resolve().parents[3] / "app" / "knowledge" / "code_db_sync_pipeline.py" +REPOS_ROUTE = Path(__file__).resolve().parents[3] / "app" / "api" / "routes" / "repos.py" + + +class TestTheSyncCallsOutToNothing: + """The measurement that moves it out of the ingestion-automation family. If this ever + fails, the sync has grown an outward call and the classification must be revisited — + which is the point of asserting it rather than asserting the flag alone.""" + + def test_it_opens_no_connection(self) -> None: + source = PIPELINE.read_text(encoding="utf-8") + outward = { + token: source.count(token) + for token in ("adapter", "connector", "execute_query", "introspect", "httpx", "aiohttp") + } + assert all(n == 0 for n in outward.values()), outward + + def test_it_reads_what_is_already_stored(self) -> None: + """`load_db_index` and `load_code_knowledge` are the inputs — both local.""" + source = PIPELINE.read_text(encoding="utf-8") + assert "load_db_index" in source + assert "load_code_knowledge" in source + + +class TestTheChainRunsByDefault: + def test_the_flag_defaults_on(self) -> None: + """A map that lags the index it belongs to is the failure this default prevents.""" + assert Settings.model_fields["auto_sync_after_index"].default is True + + def test_the_flag_still_exists(self) -> None: + """Removing it would be the other mistake: the sync costs LLM tokens per run, and + an operator who wants that off has a real reason the previous grouping never + named.""" + assert "auto_sync_after_index" in Settings.model_fields + + def test_the_default_is_documented_by_its_real_cost(self) -> None: + """The comment beside a flag is what the next person reads instead of measuring. + It said "closes the index→sync gap"; it did not say why the flag was off, and the + reason recorded elsewhere — ingestion automation — was wrong.""" + source = (Path(__file__).resolve().parents[3] / "app" / "config.py").read_text("utf-8") + idx = source.index("auto_sync_after_index") + window = source[max(0, idx - 1400) : idx] + lowered = window.lower() + assert "llm" in lowered or "token" in lowered, ( + "the flag's real cost — LLM tokens per sync — is not stated next to it" + ) + assert "calls out" in lowered or "outward" in lowered or "local" in lowered, ( + "nothing next to the flag records that the sync reaches nothing outward, " + "which is why it is not ingestion automation" + ) + + +class TestTheGateStillGates: + def test_the_chain_reads_the_flag(self) -> None: + """Default-on is not the same as ungated. An operator setting it false must still + get an index with no sync.""" + source = REPOS_ROUTE.read_text(encoding="utf-8") + chain = source[source.index("async def _maybe_autostart_sync_chain") :][:1600] + assert "auto_sync_after_index" in chain + + def test_a_disabled_chain_says_so_rather_than_failing_quietly(self) -> None: + source = REPOS_ROUTE.read_text(encoding="utf-8") + chain = source[source.index("async def _maybe_autostart_sync_chain") :][:1600] + assert "flag_off" in chain, "a skipped chain must be visible in the log" + + +def test_the_ingestion_automation_family_no_longer_claims_it() -> None: + """`CLAUDE.md` lists the family by name. Leaving this flag in that list would keep + the wrong reason on the record even after the default changed — and the record is what + the next audit reads.""" + claude_md = Path(__file__).resolve().parents[4] / "CLAUDE.md" + text = claude_md.read_text(encoding="utf-8") + marker = "**Ingestion automation" + assert marker in text, "the family heading moved; this check needs re-pointing" + # Membership is the LIST, not the prose around it. A first attempt read a + # character window and tripped on the paragraph that explains the flag *left* the + # family — which is exactly the text that should be there. + after = text[text.index(marker) :] + listing = next(line for line in after.splitlines()[1:] if line.strip().startswith("`")) + assert "auto_sync_after_index" not in listing, ( + "the flag is still listed as ingestion automation, which is the classification " + f"that made an index leave its own map stale: {listing}" + ) + # And the departure has to be recorded, or the next reader re-adds it. + assert "auto_sync_after_index" in after[: len(marker) + 1200], ( + "nothing near the family says the flag left it, or why" + )