MOD-BACKFILL — fill the sweep's reference fields, and refuse the rest out loud - #113
Open
ibuilder wants to merge 1 commit into
Open
MOD-BACKFILL — fill the sweep's reference fields, and refuse the rest out loud#113ibuilder wants to merge 1 commit into
ibuilder wants to merge 1 commit into
Conversation
…refuse the rest out loud
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 <noreply@anthropic.com>
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
PR Summary by QodoMOD-BACKFILL: Admin backfill of additive reference fields via exact, safe matching
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. test_ref_backfill sets AEC_RBAC
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+17
to
+25
| 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" | ||
|
|
There was a problem hiding this comment.
1. test_ref_backfill sets aec_rbac 📜 Skill insight ≡ Correctness
The new DB-backed test in services/api/test_ref_backfill.py violates the test-environment isolation rules by hard-setting environment variables at import/startup: it sets os.environ["AEC_RBAC"] = "1" instead of removing it and also unconditionally overrides DATABASE_URL and STORAGE_DIR. This breaks the run_tests.py per-test sandbox/cleanup contract and can cause RBAC-dependent behavior and persistent DB/storage state to leak across runs in CI or on developer machines.
Agent Prompt
## Issue description
`services/api/test_ref_backfill.py` is not respecting the test runner’s isolation contract and compliance requirements for DB-backed tests: it sets `AEC_RBAC` instead of removing it and unconditionally overrides `DATABASE_URL` and `STORAGE_DIR`, which can leak RBAC behavior and persistent DB/storage state across runs.
## Issue Context
- PR Compliance ID 2374337 requires DB-backed API tests to remove `AEC_RBAC` from the environment (use `os.environ.pop("AEC_RBAC", None)`) and to use `test_*-` prefixed local paths.
- `services/api/run_tests.py` provides per-test `DATABASE_URL` and `STORAGE_DIR` values and cleans up those paths after each test; unconditionally overriding them in the test bypasses that isolation/cleanup and can reduce determinism.
- Prefer deriving any cleanup from the effective environment values so the test behaves correctly both when run via `run_tests.py` and when run standalone.
## Fix Focus Areas
- services/api/test_ref_backfill.py[15-28]
- services/api/run_tests.py[81-96]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+49
to
+69
| _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() |
There was a problem hiding this comment.
2. normalize() regex lacks caps 📜 Skill insight ⛨ Security
normalize() applies regexes with unbounded quantifiers to free-text input without capping input length before matching/substitution. This violates the ReDoS prevention requirement for bounded regex quantifiers and capped scan length.
Agent Prompt
## Issue description
`services/api/src/aec_api/ref_backfill.py` uses regexes with unbounded quantifiers (e.g., `\s+`, `[\s,]+`) and does not cap the input length before applying them in `normalize()`, which violates the ReDoS compliance rule.
## Issue Context
Even if the current patterns are relatively simple, the compliance rule requires both bounded quantifiers and a maximum input length (e.g., `text[:20_000]`) before regex processing.
## Fix Focus Areas
- services/api/src/aec_api/ref_backfill.py[49-69]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+65
to
+83
| @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) | ||
|
|
There was a problem hiding this comment.
4. Unknown module returns success 🐞 Bug ☼ Reliability
The new /modules/backfill-references endpoint accepts any module string and passes it through; unknown module keys are silently skipped and return HTTP 200 with an empty report. This can make an operator believe a backfill ran (or was safely reviewed) when nothing happened.
Agent Prompt
### Issue description
When the caller provides `module=<typo>`, `backfill()` will process that key, but `pairs_for()` returns an empty list for unknown modules and the tool returns a normal-looking 200 response with no explicit error.
### Issue Context
The endpoint is an admin operational tool meant to be "refusal-forward"; a typo should fail loudly.
### Fix Focus Areas
- services/api/src/aec_api/routers/modules.py[65-83]
- services/api/src/aec_api/ref_backfill.py[72-88]
- services/api/src/aec_api/ref_backfill.py[119-129]
### Suggested fix
- In the router, if `module` is provided and `module not in mod_engine.REGISTRY` (or `ref_backfill.REGISTRY`), raise `HTTPException(404, "unknown module ...")`.
- Alternatively (or additionally), in `backfill()` validate all `module_keys` up front and include an explicit `unknown_module` entry in the report (but do not silently skip).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+48
to
+51
| #: 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) |
There was a problem hiding this comment.
5. Suffix stripping overly broad 🐞 Bug ≡ Correctness
normalize() strips a trailing "company" token as a legal suffix, which can conflate real names where “Company” is part of the identity (e.g., “The Company” vs “The”). That increases the chance of creating a wrong but plausible unique match.
Agent Prompt
### Issue description
`_LEGAL_SUFFIX` includes `company`, so `normalize("Acme Company") == normalize("Acme")`. Because backfill links on *exact normalized equality*, this broadens the match surface and can create incorrect unique matches.
### Issue Context
This backfill explicitly optimizes for “refuse rather than guess”; broad normalization rules increase guessing.
### Fix Focus Areas
- services/api/src/aec_api/ref_backfill.py[48-69]
### Suggested fix
- Remove `company` from `_LEGAL_SUFFIX` (keeping `co`/`corp`/`inc`/etc.), **or**
- Add a stricter rule (e.g., only strip `company` when it appears as part of a known legal form pattern), **and**
- Add a focused test case demonstrating the desired behavior for names ending in "Company" so future changes don’t reintroduce accidental conflation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Base
6c07a068(i.e. on top of the merged #109),git merge-treeCLEAN as of that SHA. Item 3 of the field sweep's "not done" list. Surface is one new engine file, one route, one test — nomodule.jsonedits, so it doesn't stack conflicts on #111 or #112.The problem
The sweep added 54 reference fields beside their text twins 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 design is about which matches it refuses
The failure modes are not symmetric:
So the matcher is deliberately timid, and each limit is a decision rather than an omission:
Acme Co., Inc.==ACME). Nothing else — no edit distance, no abbreviation expansion, no prefix match.Acme ElectricalandAcme Electricare different companiescreated_atis a coin flip wearing a suitapply=Falsereturns exactly what it would do and writes nothingAdmin-gated (it writes across every register in one call).
party=Noneon the write, so it cannot advance a workflow state or satisfy a party gate; actorsystem:backfillkeeps it identifiable in the audit trail.Two hollow greens caught in my own test
Neither would have failed — both are the "check with no input" shape:
pairs_for()at import time returned[].REGISTRYpopulates when the app starts, so every assertion under it — includingall(t == "company" for ...)— passed over an empty list. Moved inside the TestClient block.AEC_RBACunset, so role enforcement was off 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 with how many records were indexed, a blank source is not reported (nothing to match on 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.
Verification
test_ref_backfillPASS. The one failure istest_schema_diagand it is not from this branch — see below.TESTS: 451, manifest guard clean, three suites PASS, ruff clean), not by grepping. MOD-FILTER — per-field filters + server-side sort · MOD-PERCENT — the form layer didn't knowpercentexisted #109 taught that a marker check can pass on a file that doesn't compile.test_schema_diagfails on any clean checkoutReported separately to its author.
test_shipped_samples_have_zero_structural_errorsneeds five.ifcfiles that are not tracked —git ls-files samples/returns three entries (README.md+ two.mass), andgit archive HEAD samples/confirms that's all a fresh clone gets. They exist only as ignored files on this machine.It's the same defect
.gitignoreline 49 records:samples/was ignored outright until v0.3.769, and the oldtest_samples— "whose comment read 'samples/ is committed'" — passed locally because the directory happened to exist. Its own anti-vacuous guard is what made this visible instead of iterating zero files and reporting success.🤖 Generated with Claude Code