MOD-SWEEP — field-level sweep of all 133 registers - #108
Merged
Conversation
The field vocabulary in use across 1171 fields was eleven attributes long — name, label, type, fieldset, options, required, module, source_module, source_field, op, help — every one structural or presentational. Nothing said what a number MEASURED, nothing distinguished a percentage from a count, and half the concepts that already had their own register were held as loose strings. Units lived in field NAMES (elevation_ft, expected_life_years, ld_per_day), where only a human can read them: a name cannot be rendered beside an input, appended to a cell, converted, or checked. TWO LIVE DEFECTS IN THE REFERENCE RENDERER, and the first gates everything else. A reference column resolved ids against a bounded fetch, then fell back to `String(v).slice(0, 8)` AS A CLICKABLE LINK. Three unrelated situations took that path and all three were indistinguishable from a resolved reference: the target was deleted, the target lies past the 500-record resolve bound, or the value is legacy free text. Eight characters is the specific harm — short enough to look like an id, long enough to look deliberate. The same truncation existed a second time in `fmtCell`, on the path taken when a reference is not a resolved column. `refCell` now separates the three, because they ask different things of the reader: resolved -> the link; a UUID absent from the map -> the short id, NOT a link, marked unresolved; not an id at all -> the value verbatim at full length, marked as unlinked text, because that string is the only handle anyone has for re-linking it. The bound is now a named constant, since it is a correctness boundary rather than a tuning parameter. RELATIONSHIPS — 74 text fields named a concept that already had its own register (vendor, location, spec_section, system, inspector, bidder, tenant, carrier). None is converted in place. Converting one would have turned "Acme Electrical Inc" into a link reading "Acme Ele" that opens nothing, across 56 modules, with every gate still green — which is precisely why the renderer fix had to land first. The codebase already held the safe pattern in three places (investor+investor_company, lease+tenant_company, subcontract+vendor_company), so this follows that precedent rather than inventing one: 54 references added BESIDE their text twins, each adjacent and sharing its fieldset, both asserted — a pair rendered in two different places is a pair nobody recognises as a pair. 20 candidates were rejected and the reasons are the useful part: an AHJ is not a project company (permit.authority, entitlement.agency); a manufacturer is not a project party; risk.owner is a PERSON, and whether that means contact or company genuinely differs per module; and every plural (photos, deficiencies, spec_sections, assumptions) is a child LIST needing the table type, where a reference would be wrong in a way that looks right. FOUR AUTOMATED INFERENCES WERE WRONG AND READING THE OUTPUT CAUGHT THEM. capital_plan.planned_year, fca_element.recommended_year and market_assumption.construction_start_year are calendar years, not durations — the suffix rule labelled all three "yr". prime_contract.ld_per_day is dollars PER day. The gate now names those three as unit-less so a future bulk pass cannot re-lose the distinction. reference fields 98 -> 152 registers with <3 columns 55 -> 17 modules with no reference at all 69 -> 49 a reference AS a column 2 -> 32 percent-typed fields 2 -> 22 fields declaring a unit 0 -> 67 list_columns was widened APPEND-ONLY. The first pass chose by type priority and silently dropped human-chosen columns such as bid_package.trade; it was redone to preserve every existing column and append to it. FORMATTING, STATED RATHER THAN SMUGGLED: the edits were applied programmatically, so the 90 touched module.json files are normalised to the expanded one-key-per-line style already used by the majority (rfi, coi, submittal). That accounts for roughly 900 of the 1366 added lines. Eight further files whose ONLY change was that reformatting have been reverted — a diff with no semantic content is noise a reviewer has to read. test_module_fields.py holds each figure as a FLOOR, not an equality: adding a module never requires editing it, undoing the work fails it. Analysis in docs/internal/module-field-sweep.md — internal, because docs/ is the Pages web root and an earlier audit of mine was published from it for two releases before another session caught it. Reference material: eManagerNYC/django_emanager, a Django CM app the user built previously. Its value is the structural contrast — ForeignKeys throughout where these modules used strings — plus four ideas still missing here, chief among them that its `ticket` composes the labor/material/equipment rate libraries into totals, while `eticket` has six fields and links to no rate library at all. Not done, and listed in the doc rather than half-built: per-field filters and server-side sort (filtering is q+state only), the table field type, backfilling the 54 new references, and eticket -> the rate libraries. 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-SWEEP: add units/percent types, safer reference cells, and module field ratchets
AI Description
Diagram
High-Level Assessment
Files changed (98)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Unresolved UUIDs not openable
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1811
to
+1817
| const span = document.createElement("span"); | ||
| if (UUID_RE.test(v)) { | ||
| span.className = "ref-unresolved"; | ||
| span.textContent = `${v.slice(0, 8)}…`; | ||
| span.title = `This ${target} could not be resolved — it may have been deleted, or lie beyond ` | ||
| + `the first ${REF_RESOLVE_LIMIT} records of ${target}. Not a working link.`; | ||
| } else { |
There was a problem hiding this comment.
2. Unresolved uuids not openable 🐞 Bug ≡ Correctness
PortalUI.refCell renders UUID-shaped values that are absent from the resolve map as a non-clickable span, which blocks opening a referenced record even when the UUID is a valid record id (e.g., when it falls outside REF_RESOLVE_LIMIT or the resolve fetch fails). The API and UI already support opening records directly by id (openByBrief→openRecord), so this removes a functional navigation path rather than only avoiding “fake labels”.
Agent Prompt
### Issue description
`refCell()` intentionally avoids rendering a link when a reference value isn’t in the id→label map, but it treats all UUID-shaped values as “not a working link”. UUID-shaped values can still be valid record ids (e.g. beyond `REF_RESOLVE_LIMIT` or when the resolve fetch fails), so making them non-clickable blocks users from navigating to a real linked record.
### Issue Context
- The UI can open records by id via `openByBrief()`/`openRecord()`.
- The API endpoint used by `openRecord()` fetches a record by `id` directly and is not bounded by the 500-record resolve limit.
- The “legacy free-text” case is real and should remain non-link; the fix should preserve that distinction while still allowing UUIDs to be opened.
### Fix Focus Areas
- apps/web/src/portal/portal.ts[1795-1825]
- apps/web/src/portal/portal.ts[2681-2685]
- services/api/src/aec_api/modules.py[220-225]
### Suggested fix
- For `UUID_RE.test(v)` branch, render an `<a>` (or a button/icon) styled with `.ref-unresolved` (so it still *reads* unresolved) but allow click to attempt `openByBrief(c.module!, v)`.
- Add error handling for failed opens (404/permission) to avoid unhandled promise rejections (toast a concise message like “Linked record not found (may be deleted)” and keep the unresolved styling/tooltip).
- Keep the non-UUID branch as plain text (unlinked).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
1829
to
1838
| if (v == null || v === "") return ""; | ||
| if (f.type === "currency") return `$${Number(v).toLocaleString(undefined, { maximumFractionDigits: 0 })}`; | ||
| if (f.type === "percent") return `${Number(v).toLocaleString(undefined, { maximumFractionDigits: 2 })}%`; | ||
| if (f.type === "multiselect" && Array.isArray(v)) return (v as string[]).join(", "); | ||
| if (f.type === "reference") return String(v).slice(0, 8); | ||
| // Same defect as the old refCell fallback, in the path taken when a reference is NOT a resolved | ||
| // column: eight characters of a value, indistinguishable from a resolved label. A UUID at least | ||
| // announces itself as an id; anything else is shown as the text it is. | ||
| if (f.type === "reference") return UUID_RE.test(String(v)) ? `${String(v).slice(0, 8)}…` : String(v); | ||
| if (f.unit) return `${String(v)} ${f.unit}`; | ||
| return String(v).slice(0, 40); |
There was a problem hiding this comment.
3. Currency units never shown 🐞 Bug ≡ Correctness
fmtCell() returns early for currency (and percent) before the generic unit append logic, so units declared on currency fields (e.g. prime_contract.ld_per_day with unit: "$/day") are silently ignored in table cells. Additionally, for number fields with units the renderer uses String(v) (not locale formatting), causing inconsistent numeric display once units are introduced.
Agent Prompt
### Issue description
`PortalUI.fmtCell()` adds support for `f.unit`, but because `currency` and `percent` return before the `if (f.unit)` branch, units on those types are never displayed. This makes newly-added units on currency fields ineffective, and number+unit values lose `toLocaleString()` formatting.
### Issue Context
- The schema explicitly allows `unit` on `currency` (`_NUMERIC_TYPES` includes `currency`).
- At least one module declares a unit on a currency field: `prime_contract.ld_per_day` has `"type": "currency"` and `"unit": "$/day"`.
### Fix Focus Areas
- apps/web/src/portal/portal.ts[1828-1838]
- services/api/modules/prime_contract/module.json[107-113]
- services/api/src/aec_api/module_schema.py[22-29]
### Suggested fix
- Refactor `fmtCell()` to compute a base formatted value first (currency/percent/number), then append `unit` if present and non-redundant.
- Example approach:
- `let s = ...` where currency uses `$${Number(v).toLocaleString(...)}` and number uses `Number(v).toLocaleString(...)`.
- Append `unit` for all numeric types, but avoid double symbols:
- If `f.type === 'percent'` and `f.unit === '%'`, don’t append.
- For currency, consider storing units like `/day` (not `$/day`) or strip a leading `$` from `f.unit` when the `$` prefix is already in `s`.
- Keep reference/text truncation logic unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Owner
Author
|
Reviewed and merging. The Verified, not assumed:
One real gap, which this PR amplifies 11x rather than causesThe form layer does not know
This is pre-existing — Neither I am taking the fix in the release lane rather than asking for a branch revision. Merging. |
ibuilder
added a commit
that referenced
this pull request
Jul 30, 2026
Found by Massing Core reviewing #108, and it is a regression that PR made live. Three places asked `f.type === "number" || f.type === "currency"` and `percent` was in none of them. That was survivable while TWO fields in the whole system were typed `percent`; #108 converted 21 more across 16 registers — every retainage, fee, overhead and contingency percentage — and each site failed differently and quietly: 1. `EDITABLE` omitted it, so those 21 fields stopped being inline-editable overnight. A capability removed with no error and no visible cause. 2. The record form rendered `<input type="text">`: no numeric keypad on a phone, no step, no browser validation. 3. The save path skipped `Number(v)` and stored the STRING "5". `validate_record` calls `float(value)` so it passed, and every consumer coerces, so nothing ever failed — the column just accumulated a mixture of `5` and `"5"` depending on which form last touched the record. The quietest of the three, and the one that corrupts data rather than UX. That third one also feeds the bug MOD-FILTER's numeric cast exists to survive: numbers stored as text are exactly what makes a SQL comparison go lexicographic, where `'9' >= 10` is true. Fixed as ONE idea rather than three ternaries: `NUMERIC_FIELD_TYPES` is named once and referenced at all three sites, so the next numeric type is one edit instead of three that can be done two-thirds of the way — which is precisely how this happened. Also: a declared `unit` now renders BESIDE the input, not only in table cells. Half of what declaring a unit is for is the moment somebody is typing the number. Read structurally rather than via `ModuleField.unit`, because that property is declared on the sweep branch which is merged to main but cannot be merged into this one right now without git overwriting two files another session is holding dirty. WHY IT SURVIVED, WHICH IS THE PART WORTH KEEPING. It was invisible from both directions: `test_module_fields.py` reads config SHAPE and says nothing about whether a UI can render it, and the web suite never built a form containing a percent field. Two green suites, one type nothing handled. So the new gate is a COVERAGE test, not a DOM test. Rendering a form and asserting one input is `type="number"` would prove `percent` works and nothing about the next type; what failed here was completeness — a hand-maintained list with no check that it covered the types in use. `fieldTypeCoverage.test.ts` asserts the partition instead: every type the 133 shipped modules declare is either editable or deliberately read-only, no type is in both lists, every magnitude-holding type is treated as numeric, and nothing in `module_schema.FIELD_TYPES` is unrenderable. `READ_ONLY_FIELD_TYPES` carries a stated reason per entry, so "not editable" is a decision on the record rather than an omission nobody noticed. Mutation-checked: removing `percent` from `NUMERIC_FIELD_TYPES` — the exact pre-fix state — fails 4 of the 6 assertions, from four independent angles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ibuilder
added a commit
that referenced
this pull request
Jul 30, 2026
…wback that admits it cannot tell Three R22 items, all Lane C engines plus their routes (Lane G) and one register field (Lane H). No version files. No module added — the register count stays 133/30. --- R22-OPTION-OBJECT ------------------------------------------------------------- `design_option` carried `hard_cost` and `irr_pct` as plain typed fields with ZERO reference to a deal, while proforma/solve.py sizes debt and returns.py computes the levered IRR right next door. A typed 19.7% and a solved 19.7% rendered identically on the card, and the typed one is the one that goes stale. `option_economics.py` gives every figure a basis: `derived` (the option names a scenario and solve() ran), `declared` (typed, used as given, LABELLED), `unlinked`, `unavailable`. `unlinked` is the one that matters. Pricing an option from the project's other scenario would assert that this massing was underwritten under that deal — provenance nobody recorded. It is refused and the candidate scenarios are named instead. A declared figure is never promoted to derived and a derived one never overwrites what was typed, so `declared_disagrees_with_derived` can exist: a scheme whose typed IRR sits 4 points above what its own scenario solves to is the most useful output here, and it is only visible because both numbers are kept. --- R22-ITP-NCR (2) --------------------------------------------------------------- The registers were never the gap and the entry's estimate was wrong in a specific direction. Verified by reading the files: `itp` carries hold/witness points (`point_type`, `acceptance_criteria`, planned->active->verified), `ncr` a real open->dispositioned->closed lifecycle, and quality.py rolls all three up. But zero GUID fields across all three modules, and turnover.py referenced none of them — so a project could be certified substantially complete with the quality evidence never attached to what it inspected. NO new field and NO migration. Records attach through `element_guids`, the column every module record already has, already routed as POST .../elements. `data.element` has one reader in the codebase; `element_guids` has the module engine. Adding a bespoke field would have created a second convention beside the platform's own. The rule it enforces: an element with no quality record is `unrecorded`, never `clear`. "0 open NCRs" across a building nobody inspected is literally true and completely misleading. `any_attached` and `coverage_pct` separate "this building has a problem" from "nobody has linked records yet". --- clawback status --------------------------------------------------------------- `lp_irr is None` means XIRR did not converge — 26% of shapes in a 729-pattern sweep — not "no restitution owed". That case silently returned the same payload as a healthy deal, so a GP kept money a clawback might have recovered. Now reported as `clawback.status` = applied / not_owed / unavailable / not_requested, with the reason. No fallback is picked, deliberately: inventing a restitution figure out of a failed solve is how a plausible dollar amount gets cited while a missing one gets chased. Whether a non-converging IRR is a zero-clawback case is a deal-terms decision. --- also ------------------------------------------------------------------------- test_proforma: the CPI-fallback forecast is value-checked, not just its label. The old assertion was `forecast_at_completion < 40_000_000`, an upper bound — flipping the branch's `max` to `min` collapsed 27,133,231.54 to 9,000,000 (spend-to-date, i.e. claiming a month-6-of-17 job is finished) and passed. Found by mutation testing. Rebased onto b44ddbf and re-applied on top of #108's field sweep rather than over it — `gross_area_sf` keeps `unit: sf`, `efficiency_pct` and `irr_pct` keep `type: percent`, asserted before the edit was written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ibuilder
added a commit
that referenced
this pull request
Jul 30, 2026
…et had gone slack again `/procurement` moves to `api/procurement.ts` as `withProcurement`: 9 methods, 86 lines, `client.ts` 4,026 -> 3,940. Reaches only `json` on HttpCore, so no shared private helper crosses this boundary — unlike ③ (`liveStream` had to come down) and ④ (`reviewPost` had to become protected). The nine sat in **six separate regions** of the file (~1297, ~2152, ~2634, ~3185, ~3242, ~3252). That is the concrete form of the roadmap's claim that the `// --- section ---` comments no longer delimit anything: they label where a run *starts* and the file then carries on into other domains. Groups located by the route each method calls; bodies by brace matching; each doc comment claimed by scanning BACKWARDS to its opening `/**`, because a forward scan takes the next method's comment and a single-line-comment assumption broke ③ outright. ## The ratchet had drifted back into slack, and only mutation showed it Mutation-checking the extraction is routine here. This time it found something: M1 delete `procurementGate` (one method) -> **6 passed** <- should have failed M2 unwire the whole mixin (-9 methods) -> failed at 689 vs 696 Losing a single method was **invisible**. The floor said `>= 696`, and the live surface had grown to **698** across #108-#111, so -1 still cleared it. That is exactly the drift that took this number from 685 to 696 one day earlier, recurring within 24 hours — and the mechanism is now clear enough to write down: **every merge that adds an endpoint silently converts this ratchet back into a slack floor.** A ratchet is only a ratchet on the day it is set. Raised to **698**, read from the gate's own `surfaceOf` rather than a probe of my own — my counter has disagreed with it before, and a threshold from a different reader is a threshold for a different question. Re-ran M1 against the tightened floor: **fails at 697 vs 698**, naming the number. The comment now says the count must be re-read after any batch of merges, not only when extracting. ## The roadmap's remaining-groups list was wrong, so it is re-measured rather than edited It named `/procurement`, `/auth` and `/elements` and omitted several larger groups. Counted on `main` by route literals per first path segment: /auth 20 · /proforma 15 · /connections 11 · /drawing-set 11 · /drawings 11 · /elements 11 · /models 9 · /documents 9 · /contracts 8 · /sync 7 · /pdf 7 · /mep 7 ⑦ is `/auth` (20) — the one group that owns **token state** rather than only calling routes, so it needs care. `/proforma` (15) is the larger easy one and the better next cut if ⑦ stalls. Verified: web **1012/1012**, `tsc --noEmit` clean, eslint clean, doc + lane gates green. No version files — batched into the next release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Field-level sweep of every module: fields, fieldsets, relationships, and CRUD affordances. Full analysis in
docs/internal/module-field-sweep.md.Two live defects on
main, and the first gates everything elseA reference column resolved ids against a bounded fetch, then fell back to
String(v).slice(0, 8)as a clickable link. Three unrelated situations took that path and all three were indistinguishable from a resolved reference:Eight characters is the specific harm — short enough to look like an id, long enough to look deliberate. The same truncation existed a second time in
fmtCell, on the path taken when a reference is not a resolved column.refCellnow separates the three, because they ask different things of the reader: resolved → the link · a UUID absent from the map → the short id, not a link, marked unresolved · not an id at all → the value verbatim at full length, marked as unlinked text, because that string is the only handle anyone has for re-linking it.Relationships — 74 strings that name a register
74 text fields named a concept that already had its own register (
vendor,location,spec_section,system,inspector,bidder,tenant,carrier), while only 98 reference fields existed across 133 modules and 69 modules had none at all.None is converted in place. Converting one would have turned
"Acme Electrical Inc"into a link readingAcme Elethat opens nothing, across 56 modules, with every gate still green — which is why the renderer fix had to land first. The codebase already held the safe pattern in three places (investor+investor_company,lease+tenant_company,subcontract+vendor_company), so this follows that precedent: 54 references added beside their text twins, each adjacent and sharing its fieldset, both asserted.20 candidates were rejected and the reasons are the useful part: an AHJ is not a project company (
permit.authority); a manufacturer is not a project party;risk.owneris a person and whether that meanscontactorcompanygenuinely differs per module; and every plural (photos,deficiencies,spec_sections) is a child list needing the table type, where a reference would be wrong in a way that looks right.Four automated inferences were wrong, and reading the output caught them
capital_plan.planned_year,fca_element.recommended_yearandmarket_assumption.construction_start_yearare calendar years, not durations — the suffix rule labelled all threeyr. Andprime_contract.ld_per_dayis dollars per day. The gate now names those three as unit-less so a future bulk pass cannot re-lose the distinction.Result
percent-typed fieldslist_columnswas widened append-only — the first pass chose by type priority and silently dropped human-chosen columns likebid_package.trade, so it was redone to preserve every existing column.Reviewing this diff
Roughly 900 of the 1771 added lines are formatting. The edits were applied programmatically, so the 90 touched
module.jsonfiles are normalised to the expanded one-key-per-line style already used by the majority (rfi,coi,submittal). Eight further files whose only change was that reformatting have been reverted — a diff with no semantic content is noise a reviewer has to read.test_module_fields.pyholds each figure as a floor, not an equality: adding a module never requires editing it, undoing the work fails it.Verification
TESTSlength 447,test_module_fieldsregistered, manifest guard clean)tsc --noEmitclean, ruff + eslint cleanmain(3f376fff, v0.3.804)No version files or CHANGELOG — the release lane batches those.
Not done, listed rather than half-built
q+stateonly, so on a 20-field register you cannot filter by discipline, vendor, cost code or date range. An API change, not a config change, and the largest remaining CRUD gap.tablefield type — still the blocker for 22 list-in-a-textarea fields and forsov/estimate/bid_submissionbeing documents rather than rows.eticket→ the rate libraries. Reference material for this sweep was eManagerNYC/django_emanager, whoseticketcomposes labor/material/equipment rates into totals;etickethas six fields and links to no rate library.🤖 Generated with Claude Code