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