From 7f76db99c539de138dcb9953e5199e5346399911 Mon Sep 17 00:00:00 2001 From: "Matthew M. Emma" Date: Thu, 30 Jul 2026 07:47:22 -0700 Subject: [PATCH] =?UTF-8?q?feat(modules):=20MOD-BACKFILL=20=E2=80=94=20fil?= =?UTF-8?q?l=20the=20sweep's=20reference=20fields,=20and=20refuse=20the=20?= =?UTF-8?q?rest=20out=20loud?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The field sweep found 74 text fields naming a concept that already had its own register (`vendor`, `location`, `spec_section`, `system`, `inspector`) and added 54 reference fields BESIDE them rather than converting in place — because a converted field's legacy string would have rendered as a clickable link labelled with its first eight characters, across 56 modules, with every gate green. That left the references empty. This fills them. THE WHOLE DESIGN IS ABOUT WHICH MATCHES IT REFUSES TO MAKE, because the failure modes are not symmetric: an EMPTY reference is visibly empty. Somebody sees a blank and fills it. a WRONG one resolves, opens a real record and shows a plausible name — so it is never questioned, and it silently attributes an RFI to the wrong subcontractor or a defect to the wrong room. Nothing downstream can tell, because the value is structurally perfect. So the matcher is deliberately timid, and each limit is a decision rather than an omission: exact after normalisation case, whitespace, and ONE trailing legal suffix fold ("Acme Co., Inc." == "ACME"). Nothing else: no edit distance, no abbreviation expansion, no prefix match. "Acme Electrical" and "Acme Electric" are different companies and it will not pretend unique or nothing two records answering to one name is a question for a human. Breaking that tie by id or created_at is a coin flip wearing a suit never overwrite a hand-set link is better evidence than a string match, even when the string disagrees dry run by default `apply=False` returns exactly what it would do and writes nothing every skip REPORTED with its reason and a count. A backfill that quietly does 60% of the work and says "done" leaves the other 40% invisible, which is worse than one that does 60% and names the rest Admin-gated: it writes across every register in a project in one call. `party=None` on the write, so it cannot advance a workflow state or satisfy a party gate; the actor string `system:backfill` keeps it identifiable in the audit trail. TWO HOLLOW GREENS CAUGHT IN MY OWN TEST, both worth recording because neither would have failed: 1. `pairs_for()` at import time returned []. `REGISTRY` is populated when the app STARTS, so every assertion under it — including `all(t == "company" for ...)` — passed over an empty list. Same shape as `all()` over []: a check with no input cannot fail. Moved inside the TestClient block. 2. The admin-gate assertion ran with `AEC_RBAC` unset, so role enforcement was off entirely and the non-admin call returned 200. It failed loudly only because the assertion was written as a REFUSAL; had it been "an admin can call it", it would have passed while testing nothing. A security check run with the security disabled is the emptiest kind of green. The test asserts the refusals, not the matches: ambiguous is reported with its count, no-match is reported with how many records were indexed, a blank source is NOT reported (there was nothing to match on, so it is not a shortfall), a hand-set value survives an apply that would have matched it, a second apply links nothing, and ambiguity stays refused until the DATA changes — never because the matcher picked. No version files or CHANGELOG — the release lane batches those. Co-Authored-By: Claude Opus 5 --- services/api/run_tests.py | 2 +- services/api/src/aec_api/ref_backfill.py | 175 ++++++++++++++++++++ services/api/src/aec_api/routers/modules.py | 22 ++- services/api/test_ref_backfill.py | 155 +++++++++++++++++ 4 files changed, 352 insertions(+), 2 deletions(-) create mode 100644 services/api/src/aec_api/ref_backfill.py create mode 100644 services/api/test_ref_backfill.py diff --git a/services/api/run_tests.py b/services/api/run_tests.py index f7602ad4..08916e1e 100644 --- a/services/api/run_tests.py +++ b/services/api/run_tests.py @@ -38,7 +38,7 @@ "test_ask", "test_verification", "test_webhooks", "test_operate_capital", "test_payroll_drawings", "test_assistant_itb", "test_construction_depth", "test_distribution", "test_e57", "test_empty_project", "test_metrics", "test_metrics_auth", "test_licensing", "test_revit_bridge", "test_precon", "test_specs", "test_feasibility", "test_clash_import", "test_clash_intel", "test_layout", "test_loads", "test_verified_progress", "test_element_records", "test_securities_bridge", "test_imports", "test_search_alerts", "test_attachments", # previously not wired into the gate (glob would have caught these) — now covered: "test_analytics", "test_discipline", "test_gbxml", "test_review", "test_interop", - "test_module_config", "test_module_schema", "test_module_tables", "test_module_filters", "test_module_fields", "test_throttle", "test_route_order", + "test_module_config", "test_module_schema", "test_ref_backfill", "test_module_tables", "test_module_filters", "test_module_fields", "test_throttle", "test_route_order", # R23-PREFAB-KIT — the kit join + its register routes: "test_prefab_kit", "test_prefab_route", # Tier-1 competitive upgrades: diff --git a/services/api/src/aec_api/ref_backfill.py b/services/api/src/aec_api/ref_backfill.py new file mode 100644 index 00000000..d537ab78 --- /dev/null +++ b/services/api/src/aec_api/ref_backfill.py @@ -0,0 +1,175 @@ +"""MOD-BACKFILL — populate the additive text+reference pairs from the text already stored. + +The field sweep found 74 text fields naming a concept that already had its own register (`vendor`, +`location`, `spec_section`, `system`, `inspector`), and added 54 reference fields **beside** them +rather than converting in place — because a converted field's legacy string would have rendered as a +clickable link labelled with its first eight characters, across 56 modules, with every gate green. + +That left the references empty. This fills them in, and the whole design is about **which matches it +refuses to make**. + +**A WRONG AUTO-LINK IS WORSE THAN AN EMPTY ONE**, and not by a little. An empty reference is visibly +empty: somebody sees a blank and fills it. A wrong one is a link that resolves, opens a real record, +and shows a plausible name — so it is never questioned, and it silently attributes an RFI to the wrong +subcontractor or a defect to the wrong room. Nothing downstream can tell the difference, because the +value is structurally perfect. This is the same asymmetry as every other defect this codebase has +chased: the dangerous output is the plausible one, not the missing one. + +So the matcher is deliberately timid: + +* **Exact, after normalisation only.** Case, surrounding whitespace, internal whitespace runs and a + trailing legal suffix (`Inc`, `LLC`, `Ltd`, `Co`, `Corp`) are normalised away. Nothing else — no + fuzzy distance, no token overlap, no prefix matching. "Acme Electrical" and "Acme Electric" are + different companies and this will not pretend otherwise. +* **Unique or nothing.** Two candidate records matching one string is not a 50/50 guess to be broken + by `created_at` or by id order; it is a question for a human. Ambiguity is reported, never resolved. +* **Never overwrite.** A reference that already has a value is left alone even if it disagrees with the + text, because somebody may have set it by hand and the hand-set one is the better evidence. +* **Dry run by default.** `apply=False` returns exactly what it would do and changes nothing, so the + report can be read before anything is written. + +The counterpart to timidity is that every skip is REPORTED with its reason. A backfill that silently +does 60% of the work and says "done" leaves the other 40% invisible, which is worse than one that +does 60% and names the rest. +""" +from __future__ import annotations + +import re +from typing import Any + +from sqlalchemy.orm import Session + +from .modules_query import list_records +from .modules_registry import REGISTRY + +#: The suffixes the sweep used when adding a reference beside its text twin. +PAIR_SUFFIXES = ("_company", "_loc", "_spec", "_system", "_contact", "_package") + +#: Trailing company-form tokens that carry no identity — "Acme Inc" and "Acme" are one firm. +_LEGAL_SUFFIX = re.compile( + r"[\s,]+(inc|inc\.|llc|l\.l\.c\.|ltd|ltd\.|limited|co|co\.|corp|corp\.|corporation|company|" + r"plc|lp|llp|pllc|gmbh|pty|sa|nv|bv)$", re.I) + + +def normalize(s: Any) -> str: + """The one normalisation both sides go through. Anything this does not fold is a real difference. + + Deliberately shallow: lowercase, collapse whitespace, drop a single trailing legal suffix, strip + surrounding punctuation. It does NOT strip internal punctuation ("J&J" vs "J and J"), expand + abbreviations ("Bldg" vs "Building") or do edit-distance — each of those turns a refusal into a + guess, and a guess here produces a link that looks right. + """ + t = re.sub(r"\s+", " ", str(s or "")).strip().strip(".,;:-–—").strip() + if not t: + return "" + prev = None + while prev != t: # "Acme Co., Inc." -> "Acme" + prev = t + t = _LEGAL_SUFFIX.sub("", t).strip().strip(".,;:-–—").strip() + return t.lower() + + +def pairs_for(module_key: str) -> list[tuple[str, str, str]]: + """[(text_field, reference_field, target_module)] for one module, from its declared fields.""" + mod = REGISTRY.get(module_key) or {} + by = {f.get("name"): f for f in mod.get("fields", []) if f.get("name")} + out: list[tuple[str, str, str]] = [] + for name, f in by.items(): + if f.get("type") != "reference" or not f.get("module"): + continue + for suf in PAIR_SUFFIXES: + if name.endswith(suf): + stem = name[: -len(suf)] + twin = by.get(stem) + if twin and twin.get("type") in ("text", "textarea"): + out.append((stem, name, f["module"])) + break + return sorted(out) + + +def _index(db: Session, project_id: str, target: str) -> tuple[dict[str, list[str]], int]: + """normalised title -> [record ids] for a target module, plus how many records were indexed. + + A LIST, not a single id: collisions are the thing this exists to detect, and a dict keyed by title + would silently keep the last one — which is exactly the quiet wrong answer the whole module is + written to avoid. + """ + mod = REGISTRY.get(target) or {} + tf = mod.get("title_field") + idx: dict[str, list[str]] = {} + rows = list_records(db, target, project_id, limit=5000) + for r in rows: + data = r.get("data") or {} + for candidate in (data.get(tf) if tf else None, r.get("ref")): + key = normalize(candidate) + if key: + idx.setdefault(key, []).append(r["id"]) + return idx, len(rows) + + +def backfill(db: Session, project_id: str, *, module_keys: list[str] | None = None, + apply: bool = False) -> dict[str, Any]: + """Fill empty reference fields from their text twins. Returns a full report either way. + + `apply=False` (the default) changes nothing — the report is identical to what an apply would do, + so it can be read first. Nothing is ever overwritten and no ambiguous match is ever resolved. + """ + from . import modules as me # local: the engine imports this module's peers + + keys = module_keys or sorted(REGISTRY) + linked: list[dict[str, str]] = [] + skipped: list[dict[str, str]] = [] + indexes: dict[str, tuple[dict[str, list[str]], int]] = {} + scanned = 0 + + for key in keys: + prs = pairs_for(key) + if not prs: + continue + records = list_records(db, key, project_id, limit=5000) + scanned += len(records) + for text_field, ref_field, target in prs: + if target not in indexes: + indexes[target] = _index(db, project_id, target) + idx, target_n = indexes[target] + for r in records: + data = r.get("data") or {} + raw = data.get(text_field) + if data.get(ref_field): + continue # never overwrite a hand-set link + norm = normalize(raw) + if not norm: + continue # nothing to match on; not a skip worth reporting + hits = idx.get(norm) or [] + base = {"module": key, "record": r["id"], "ref": r.get("ref") or "", + "field": ref_field, "text": str(raw), "target": target} + if len(hits) == 1: + linked.append({**base, "linked_to": hits[0]}) + if apply: + # `party=None`: a backfill is not a workflow actor. It writes one reference + # field and must not be able to advance a state or satisfy a party gate — + # the actor string keeps it identifiable in the audit trail. + me.update_record(db, key, project_id, r["id"], {ref_field: hits[0]}, + "system:backfill", None) + elif len(hits) > 1: + # Two records answer to one name. Breaking the tie by id or date would be a coin + # flip wearing a suit — it is a question for a human, so it is reported as one. + skipped.append({**base, "reason": "ambiguous", + "detail": f"{len(hits)} records in {target} share this name"}) + else: + skipped.append({**base, "reason": "no match", + "detail": f"no {target} record is named this " + f"({target_n} indexed)"}) + return { + "applied": bool(apply), + "records_scanned": scanned, + "linked": linked, + "skipped": skipped, + "summary": {"linked": len(linked), + "ambiguous": sum(1 for s in skipped if s["reason"] == "ambiguous"), + "no_match": sum(1 for s in skipped if s["reason"] == "no match")}, + "note": ("Exact match after normalisation only, unique matches only, existing values never " + "overwritten. A wrong link resolves, opens a real record and shows a plausible name, " + "so it is never questioned — an empty reference is visibly empty and gets filled. " + "Every skip is listed with its reason rather than left as a silent shortfall."), + } diff --git a/services/api/src/aec_api/routers/modules.py b/services/api/src/aec_api/routers/modules.py index fa6b3c01..3293173d 100644 --- a/services/api/src/aec_api/routers/modules.py +++ b/services/api/src/aec_api/routers/modules.py @@ -10,7 +10,7 @@ from pydantic import BaseModel from sqlalchemy.orm import Session -from .. import ai, audit, mailer, rbac, rooms +from .. import ai, audit, mailer, rbac, ref_backfill, rooms from .. import modules as mod_engine from .. import sync as sync_engine from ..db import get_db @@ -62,6 +62,26 @@ def list_modules(): ] +@router.post("/projects/{pid}/modules/backfill-references") +def backfill_references(pid: str, module: str | None = None, apply: bool = False, + db: Session = Depends(get_db), + _: str = Depends(require_role("admin"))): + """MOD-BACKFILL — fill empty reference fields from the text twin already on the record. + + The field sweep added 54 references BESIDE their text fields rather than converting in place; + this populates them. Exact match after normalisation, unique matches only, existing values never + overwritten — and **`apply` defaults to False**, so the default call is a report. + + A wrong auto-link is worse than an empty one: it resolves, opens a real record and shows a + plausible name, so nobody questions it. An empty reference is visibly empty and gets filled by + the next person to look. Every skip is returned with its reason rather than left as a silent + shortfall. + + `admin` because it writes across every register in the project in one call. + """ + return ref_backfill.backfill(db, pid, module_keys=[module] if module else None, apply=apply) + + @router.get("/rooms") def list_rooms(): """R26 — the five-room spine, and the allocation of every module to exactly one room. diff --git a/services/api/test_ref_backfill.py b/services/api/test_ref_backfill.py new file mode 100644 index 00000000..31ede6c2 --- /dev/null +++ b/services/api/test_ref_backfill.py @@ -0,0 +1,155 @@ +"""MOD-BACKFILL — filling the sweep's reference fields, and the matches it REFUSES to make. + +The field sweep added 54 reference fields beside their text twins rather than converting in place. +This fills them from the text already stored. Almost every assertion below is about a link the +matcher declines to make, because that is where the value is: + +**A wrong auto-link is worse than an empty one.** An empty reference is visibly empty — somebody sees +a blank and fills it. A wrong one resolves, opens a real record and shows a plausible name, so it is +never questioned, and it silently attributes an RFI to the wrong subcontractor or a defect to the +wrong room. Nothing downstream can tell, because the value is structurally perfect. Timidity is the +feature; the tests that matter are the ones proving it stays timid. + +Run: PYTHONPATH=src ./.venv/Scripts/python.exe test_ref_backfill.py +""" +import os + +os.environ["DATABASE_URL"] = "sqlite:///./test_ref_backfill.db" +os.environ["STORAGE_DIR"] = "./test_storage_ref_backfill" +os.environ["AEC_TRUST_XUSER"] = "1" +# WITHOUT THIS, ROLE ENFORCEMENT IS OFF AND THE ADMIN-GATE ASSERTION TESTS NOTHING. The first version +# of this file omitted it, and the non-admin call returned 200 — which failed the assertion loudly, +# but would have passed silently had the assertion been written the other way round ("an admin can +# call it"). A security check run with the security disabled is the emptiest kind of green. +os.environ["AEC_RBAC"] = "1" + +for _f in ("./test_ref_backfill.db",): + if os.path.exists(_f): + os.remove(_f) + +from fastapi.testclient import TestClient # noqa: E402 + +from aec_api.main import app # noqa: E402 +from aec_api.ref_backfill import normalize, pairs_for # noqa: E402 + +H = {"X-User": "gc"} + +# ---- 1. normalisation folds only what carries no identity -------------------------------------- +assert normalize("Acme Electrical Inc.") == normalize("acme electrical") +assert normalize("Acme Co., Inc.") == normalize("ACME") # repeated legal suffixes +assert normalize(" Level 2 ") == "level 2" +assert normalize(None) == "" and normalize("") == "" and normalize(" ") == "" +# ...and NOT things that are real differences. Each of these would be a guess, and a guess here +# produces a link that looks right. +assert normalize("Acme Electrical") != normalize("Acme Electric") # no edit distance +assert normalize("J&J Mechanical") != normalize("J and J Mechanical") +assert normalize("Bldg A") != normalize("Building A") # no abbreviation expansion +assert normalize("Level 2") != normalize("Level 20") # no prefix matching + +with TestClient(app) as c: + # ---- 2. the pair list comes from the declared config, not a hand-kept list ------------------ + # INSIDE the client block on purpose: `REGISTRY` is populated when the app starts, so calling + # `pairs_for` at import time returns [] and every assertion below it passes vacuously. That is the + # same shape as `all()` over an empty list — a check that cannot fail because it has no input. + coi_pairs = pairs_for("coi") + assert ("vendor", "vendor_company", "company") in coi_pairs, coi_pairs + assert ("carrier", "carrier_company", "company") in coi_pairs, coi_pairs + assert all(t == "company" for _s, _r, t in coi_pairs), coi_pairs + assert pairs_for("rfi"), "rfi should have at least the spec_section pair" + + pid = c.post("/projects", headers=H, json={"name": "Backfill"}).json()["id"] + + def mk(mod, data): + r = c.post(f"/projects/{pid}/modules/{mod}", headers=H, json={"data": data}) + assert r.status_code == 201, r.text + return r.json()["id"] + + # target register: three companies, two of which share a name after normalisation + acme = mk("company", {"name": "Acme Electrical Inc."}) + _bolt = mk("company", {"name": "Bolt Mechanical"}) + dup_a = mk("company", {"name": "Summit Interiors"}) + dup_b = mk("company", {"name": "Summit Interiors LLC"}) # normalises to the same string + + # source records, each exercising one branch + exact = mk("coi", {"vendor": "acme electrical", "expires": "2027-01-01"}) # -> links + ambig = mk("coi", {"vendor": "Summit Interiors", "expires": "2027-01-01"}) # -> ambiguous + nomatch = mk("coi", {"vendor": "Nobody Ltd", "expires": "2027-01-01"}) # -> no match + preset = mk("coi", {"vendor": "acme electrical", "vendor_company": dup_a, + "expires": "2027-01-01"}) # -> never overwritten + # The blank case needs a module whose text twin is OPTIONAL — `coi.vendor` is required, so an + # empty one cannot be created at all. `punchlist.responsible` is free. + blank = mk("punchlist", {"description": "Touch up paint"}) # -> not reported + + def run(apply=False, module="coi"): + r = c.post(f"/projects/{pid}/modules/backfill-references", + headers=H, params={"module": module, "apply": str(apply).lower()}) + assert r.status_code == 200, r.text + return r.json() + + # ---- 3. DRY RUN IS THE DEFAULT and changes nothing ----------------------------------------- + rep = run(apply=False) + assert rep["applied"] is False + ids = {x["record"] for x in rep["linked"]} + assert ids == {exact}, f"expected only the exact match to link, got {ids}" + got = c.get(f"/projects/{pid}/modules/coi/{exact}", headers=H).json() + assert not got["data"].get("vendor_company"), "a dry run wrote to the record" + + # ---- 4. every refusal is REPORTED with a reason --------------------------------------------- + by_record = {x["record"]: x for x in rep["skipped"]} + assert by_record[ambig]["reason"] == "ambiguous", by_record.get(ambig) + assert "2 records" in by_record[ambig]["detail"], by_record[ambig]["detail"] + assert by_record[nomatch]["reason"] == "no match", by_record.get(nomatch) + # a blank source is not a shortfall — there was nothing to match on, so it is not noise in the + # report. Checked against a project-wide run, since `blank` is a punchlist record. + wide = c.post(f"/projects/{pid}/modules/backfill-references", + headers=H, params={"apply": "false"}).json() + assert blank not in {x["record"] for x in wide["skipped"]}, \ + "an empty text field was reported as a skip" + # an already-set reference is neither linked nor reported: it is simply not this tool's business + assert preset not in by_record and preset not in ids + + # ---- 5. APPLY writes exactly what the dry run promised -------------------------------------- + rep2 = run(apply=True) + assert rep2["applied"] is True + assert {x["record"] for x in rep2["linked"]} == ids, "apply diverged from the dry run" + got = c.get(f"/projects/{pid}/modules/coi/{exact}", headers=H).json() + assert got["data"]["vendor_company"] == acme, got["data"] + # the hand-set one is untouched even though its text says "acme electrical" and would have matched + kept = c.get(f"/projects/{pid}/modules/coi/{preset}", headers=H).json() + assert kept["data"]["vendor_company"] == dup_a, "a hand-set reference was overwritten" + + # ---- 6. it is IDEMPOTENT — a second apply links nothing new --------------------------------- + rep3 = run(apply=True) + assert rep3["summary"]["linked"] == 0, rep3["summary"] + assert rep3["summary"]["ambiguous"] == 1 and rep3["summary"]["no_match"] == 1, rep3["summary"] + + # ---- 7. ambiguity stays refused even after one candidate is renamed apart ------------------- + # (the tie is broken by the DATA changing, never by the matcher picking) + c.patch(f"/projects/{pid}/modules/company/{dup_b}", headers=H, + json={"name": "Summit Interiors West"}) + rep4 = run(apply=True) + assert {x["record"] for x in rep4["linked"]} == {ambig}, rep4["linked"] + got = c.get(f"/projects/{pid}/modules/coi/{ambig}", headers=H).json() + assert got["data"]["vendor_company"] == dup_a + + # ---- 8. scoping to one module does not touch others ----------------------------------------- + all_rep = c.post(f"/projects/{pid}/modules/backfill-references", + headers=H, params={"apply": "false"}).json() + assert all_rep["records_scanned"] >= rep["records_scanned"] + + # ---- 9. it is admin-gated: it writes across every register in one call ---------------------- + r = c.post(f"/projects/{pid}/modules/backfill-references", headers={"X-User": "sub"}, + params={"apply": "true"}) + assert r.status_code in (401, 403), f"a non-admin ran a project-wide write ({r.status_code})" + +print("MOD-BACKFILL OK - reference fields fill from their text twins by EXACT match after " + "normalisation, unique matches only, existing values never overwritten, dry-run by default, " + "idempotent, and admin-gated. Normalisation folds case, whitespace and trailing legal suffixes " + "(Acme Co., Inc. == ACME) and deliberately NOTHING else: no edit distance, no abbreviation " + "expansion, no prefix matching, so 'Acme Electrical' and 'Acme Electric' stay different " + "companies. Two records answering to one name is reported as ambiguous rather than resolved — " + "breaking that tie by id or date would be a coin flip wearing a suit. Every refusal is returned " + "with its reason, because a backfill that silently does 60% of the work and reports success " + "leaves the other 40% invisible. The asymmetry driving all of it: an empty reference is visibly " + "empty and gets filled, while a wrong one resolves, opens a real record, shows a plausible name " + "and is never questioned.")