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" + )