Spaces:
Runtime error
Runtime error
| # RICS Hardcoded-Constant Inventory | |
| **Status: RESOLVED** β all three constants relocated/extracted; `test_no_rics_hardcodes` | |
| now passes (suite stepped 3 β 2 β 1 β 0, one constant at a time). The sections | |
| below retain the original pre-refactor analysis and append the post-refactor | |
| source-of-truth location and its guard test. | |
| Scope: the three production modules that previously failed `test_no_rics_hardcodes` | |
| because they hardcoded RICS-domain rating vocabulary. | |
| > The refactor was a **pure relocation / schema-driven extraction**: no constant | |
| > value and no runtime behaviour changed. Byte-for-byte identity (2.2) and exact | |
| > set identity (2.1) are pinned by dedicated guard tests. | |
| --- | |
| ## 1. The guard test | |
| `backend/tests/test_no_rics_hardcodes.py` | |
| ```python | |
| _FORBIDDEN = re.compile(r"Condition Rating|CR[123]|_RICS_LETTER", re.IGNORECASE) | |
| ``` | |
| Parametrised over every `backend/**/*.py` **except**: | |
| - anything under a `tests/` path | |
| - anything under a `prompts/` path | |
| - `rics_canonical_l3.py` | |
| - `__init__.py` | |
| **Architectural intent encoded by the exemptions:** RICS-specific literals are | |
| permitted only in (a) the canonical schema seed (`rics_canonical_l3.py`), | |
| (b) prompt templates (`backend/prompts/`), and (c) tests. Everywhere else, | |
| domain vocabulary must be **derived from the discovered `TemplateSchema`**, not | |
| inlined β so the engine stays firm-/template-agnostic. | |
| --- | |
| ## 2. The three violations | |
| ### 2.1 `backend/core/pii_scrubber.py` β `_STATUS_TERMS` (line ~58) | |
| ```python | |
| _STATUS_TERMS = frozenset({ | |
| "condition rating", "defect", "satisfactory", "serviceable", "deflection", | |
| "distortion", "cracking", "spalled", "damp", "moisture", "insulation", | |
| }) | |
| ``` | |
| - **Match that trips the test:** `"condition rating"`. | |
| - **Role:** part of `PROPTECH_SAFE_WHITELIST` (Pass-2 guardrail). Whitelisted | |
| spans are **never masked** as PII. Removing/renaming the term risks the | |
| scrubber redacting the legitimate phrase "condition rating" out of reference | |
| prose β re-introducing exactly the over-redaction class we just fixed. | |
| - **Blast radius:** every REFERENCE-tier ingest (`RagStore.ingest_document` β | |
| `scrub_reference_for_ingest`) and every `assert_no_pii` gate (DOCX export). | |
| - **Load-bearing:** YES. Touching this changes what survives redaction globally. | |
| - **RESOLVED β** `pii_scrubber._BASE_STATUS_TERMS` (the 10 non-rating terms) plus | |
| the rating term derived as `DEFAULT_RATING_SYSTEM_NAME.lower()` imported from | |
| `rics_canonical_l3.py`. `_STATUS_TERMS = _BASE_STATUS_TERMS | {rating}` β the | |
| effective set is unchanged. No literal `"condition rating"` remains in the file. | |
| Guard: `backend/tests/test_pii_whitelist_source.py` (exact set match + | |
| canonical-derivation + the phrase still survives a scrub). | |
| ### 2.2 `backend/core/reference_mapper.py` β `_RICS_DOMAIN_RULES` (lines ~26β32) | |
| ```python | |
| _RICS_DOMAIN_RULES = """ | |
| RICS LEVEL 3 DOMAIN RULES (mandatory): | |
| - Condition ratings may only use: "1", "2", "3", "NI", "NA". Never emit an empty rating token. | |
| - If a note or baseline fragment is ambiguous or corrupted, preserve it verbatim inside [AMBIGUOUS: <text>]. | |
| - Output pure continuous prose only. No markdown fences, bullet lists, or chat preamble. | |
| - British English throughout. | |
| """ | |
| ``` | |
| - **Matches that trip the test:** `"Condition ratings"` (line 28) and | |
| `"condition rating field"` (line ~88, the per-call rating hint). | |
| - **Role:** system-prompt fragment injected into the in-place mapping LLM call. | |
| The allowed rating tokens (`1/2/3/NI/NA`) are RICS-L3-specific. | |
| - **Blast radius:** the grounding/mapping LLM output contract. Changing the | |
| allowed-values text changes what the model is told it may emit per section. | |
| - **Load-bearing:** YES (prompt contract). NOTE: this is arguably mis-located β | |
| prompt text belongs under `backend/prompts/` (which the test exempts), so a | |
| large part of the fix is simply **relocation**, not redesign. | |
| - **RESOLVED β** both fragments relocated **verbatim** to | |
| `backend/prompts/mapping_prompt.py` as `RICS_DOMAIN_RULES` (the rules block) and | |
| `RATING_HINT_TEMPLATE` (the line-88 hint). `reference_mapper._RICS_DOMAIN_RULES` | |
| is now an alias to the relocated object; the hint is built via | |
| `RATING_HINT_TEMPLATE.format(rating_value=...)`. Done as **pure relocation only** | |
| (the `1/2/3/NI/NA` token parameterisation in Β§3 below was deliberately *not* | |
| performed β it would break the byte-for-byte requirement; see Β§3 note). Guard: | |
| `backend/tests/test_grounding_prompt_relocation.py` pins SHA-256 | |
| `75b4d183β¦b06c` (len 363) and the hint string. | |
| ### 2.3 `backend/core/hybrid_discovery_compiler.py` β rating-name default (line ~65) | |
| ```python | |
| schema.rating_system = RatingSystem( | |
| detected=True, | |
| name=schema.rating_system.name or "Condition Rating", # <-- hardcoded fallback | |
| type=schema.rating_system.type or "text", | |
| values=rating_values, | |
| ... | |
| ) | |
| ``` | |
| - **Match that trips the test:** `"Condition Rating"`. | |
| - **Role:** fallback label for the rating system when discovery found rating | |
| *legends/values* but no explicit *name*. | |
| - **Blast radius:** the user-facing rating label in DOCX headings | |
| (`docx_builder._rating_label`) and previews. Cosmetic but visible. | |
| - **Load-bearing:** PARTIAL (display only; no logic branches on the literal). | |
| - **RESOLVED β** `rics_canonical_l3.DEFAULT_RATING_SYSTEM_NAME = "Condition Rating"`, | |
| imported into `hybrid_discovery_compiler` (`name=schema.rating_system.name or | |
| DEFAULT_RATING_SYSTEM_NAME`). **Value preserved exactly.** Note this fallback | |
| (`"Condition Rating"`) is intentionally distinct from the canonical schema's | |
| fully-qualified `rating_system.name` (`"RICS Condition Rating"`, line ~284) β | |
| the two were **not** collapsed, to avoid changing a value. | |
| --- | |
| ## 3. Target schema-driven shape | |
| The relevant model already exists β `backend/models/schema.py::RatingSystem`: | |
| ```python | |
| class RatingSystem(BaseModel): | |
| detected: bool = False | |
| name: str = "" # discovered label from the template | |
| type: str | None = None | |
| values: list[RatingValue] = [] # discovered legend (e.g. 1/2/3/NI/NA) | |
| format_template: str | None = None | |
| inline_example: str | None = None | |
| ``` | |
| Intended end-state per violation: | |
| | # | Module | Move the literal to | Mechanism | | |
| |---|--------|---------------------|-----------| | |
| | 2.1 | `pii_scrubber._STATUS_TERMS` | schema/canonical-derived whitelist | Build the status-term whitelist from `schema.rating_system.name` + `RatingValue` labels (+ a small canonical seed in `rics_canonical_l3.py`). Whitelist becomes data, not an inline literal. | | |
| | 2.2 | `reference_mapper._RICS_DOMAIN_RULES` | `backend/prompts/` (exempt) | Relocate the prompt fragment to a `prompts/` builder; parameterise allowed rating tokens from `schema.rating_system.values` instead of the hardcoded `1/2/3/NI/NA`. | | |
| | 2.3 | `hybrid_discovery_compiler` default name | `rics_canonical_l3.py` (exempt) | Replace the `or "Condition Rating"` literal with a canonical default constant imported from `rics_canonical_l3`, or leave `name` empty and let `_rating_label` fall back to the canonical default. | | |
| Net effect: the only files that mention "Condition Rating" become the two the | |
| test already exempts (`rics_canonical_l3.py`, `backend/prompts/*`), so the engine | |
| core is genuinely template-agnostic and the test passes without weakening it. | |
| **Deviation from the original mechanism column (as built):** | |
| - **2.2** was executed as **pure verbatim relocation**. The proposed | |
| "parameterise allowed rating tokens from `schema.rating_system.values`" was | |
| deliberately **not** done β it would alter the prompt string and break the | |
| byte-for-byte / SHA-256 guard. Token parameterisation remains a *separate, | |
| future* enhancement (it changes behaviour and must be specced/tested on its own). | |
| - **2.1** uses the **canonical seed** (`DEFAULT_RATING_SYSTEM_NAME`) rather than a | |
| live `schema.rating_system.name` lookup, because `PROPTECH_SAFE_WHITELIST` is a | |
| module-level constant with no per-request schema in scope. Net set is identical. | |
| --- | |
| ## 4. Safe refactor ordering (when undertaken separately) | |
| 1. **2.3 first** (lowest risk, display-only). Verify DOCX rating headings still | |
| render the expected label for a RICS-L3 template and for a template with an | |
| explicit custom rating name. | |
| 2. **2.2 next** (prompt relocation + parameterisation). Verify with a live/mock | |
| mapping run that allowed rating tokens still match `schema.rating_system.values`; | |
| confirm `test_minimum_weave` / `test_maximum_compose` / `test_medium_expand`. | |
| 3. **2.1 last** (highest risk β PII). Build the whitelist from schema + canonical | |
| seed; verify reference ingest does **not** redact "condition rating" and that | |
| `assert_no_pii` still blocks genuine PII. Re-run the full PII suite and the | |
| reference-ingest E2E (`test_section_photos`, `test_source_attribution_e2e`). | |
| Each step is independently shippable and independently revertible. Do **not** | |
| batch 2.1 with 2.2 β PII and prompt-contract regressions must be bisectable. | |
| ### Verification gate for the whole refactor | |
| Status: **PASSING** β full `backend/tests` suite green (0 failures); the three | |
| `test_no_rics_hardcodes` cases now pass, plus the two new relocation guards. | |
| ``` | |
| python -m pytest backend/tests/test_no_rics_hardcodes.py \ | |
| backend/tests/test_grounding_prompt_relocation.py \ | |
| backend/tests/test_pii_whitelist_source.py \ | |
| backend/tests/test_pii_scrubber.py \ | |
| backend/tests/test_section_photos.py \ | |
| backend/tests/test_source_attribution_e2e.py \ | |
| backend/tests/test_minimum_weave.py backend/tests/test_maximum_compose.py \ | |
| backend/tests/test_medium_expand.py --no-cov | |
| ``` | |