MOD-FIELDSET — 58 registers stop being one undifferentiated column of inputs - #112
Merged
Conversation
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
PR Summary by QodoMOD-FIELDSET: Add fieldsets to module forms and enforce contiguity in tests
AI Description
Diagram
High-Level Assessment
Files changed (62)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Ungrouped cap masks regressions
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+113
to
+126
| ungrouped = [] | ||
| for k, mod in mods.items(): | ||
| seq = [f.get("fieldset") for f in mod["fields"] if f["type"] != "rollup"] | ||
| if not any(seq): | ||
| ungrouped.append(k) | ||
| continue | ||
| assert all(seq), f"{k}: some fields have a fieldset and some do not: {seq}" | ||
| runs = [fs for i, fs in enumerate(seq) if i == 0 or fs != seq[i - 1]] | ||
| assert len(runs) == len(set(runs)), ( | ||
| f"{k}: fieldsets must be contiguous, got {runs} — the renderer emits one header per run, " | ||
| "so an interleaved fieldset draws its header twice") | ||
| # A floor, not an equality: new small modules may legitimately join this set, but the 58 grouped by | ||
| # the sweep must not quietly drift back to a single undifferentiated column of inputs. | ||
| assert len(ungrouped) <= 8, f"{len(ungrouped)} modules have no fieldsets at all: {sorted(ungrouped)}" |
There was a problem hiding this comment.
1. Ungrouped cap masks regressions 🐞 Bug ⚙ Maintainability
The new global fieldset gate only enforces len(ungrouped) <= 8, which means several non-lookup modules could lose all fieldsets and still pass CI as long as the total stays under the cap. This undermines the stated intent (“58 grouped … must not quietly drift back”) because it doesn’t constrain *which* modules are allowed to be ungrouped or why.
Agent Prompt
## Issue description
`test_modules.py` introduces a new global check that allows up to 8 modules to have no `fieldset` at all. This is a weak regression guard because any module can become ungrouped (lose all fieldsets) as long as the total ungrouped count remains under 8.
## Issue Context
The surrounding comment explains the intent: allow small lookup-table modules to omit fieldsets, but prevent real “forms” from drifting back to an undifferentiated column. A simple count cap doesn’t encode that intent.
## Fix Focus Areas
- services/api/test_modules.py[113-126]
## Suggested fix
Replace (or augment) the `len(ungrouped) <= 8` assertion with a rule that encodes *why* a module may be ungrouped, without needing a hand-maintained list. Examples:
1) **Size-based heuristic** (low maintenance):
- If a module has no fieldsets, assert it has at most N non-rollup fields (e.g., `<= 4`) and/or has no `reference` fields.
2) **Explicit allowed set** (strongest):
- Keep an allowlist for ungrouped modules (`{"labor_rate", "material_rate", "equipment_rate", "location"}`) and assert `set(ungrouped) <= allowed`.
Either approach prevents large real modules from silently losing fieldsets while still permitting legitimate lookup-table modules.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This was referenced Jul 30, 2026
…iated column of inputs 62 of 133 modules had NO fieldsets, so their record form rendered every field in one flat list: `comparable` 21 fields, `listing` 20, `zoning` 19, `lease` 15. That is the clearest paper-form tell at the layer a user actually touches — a form with no sections is a form nobody can scan. Nine groups, assigned by what the field IS rather than by where it happened to sit: Identity · Classification · Parties · Dates · Money · Quantities · Status · Links · Notes. Fieldsets must be CONTIGUOUS (the renderer emits one header per run, so an interleaved fieldset draws its header twice), so this is a STABLE SORT by group — relative order inside a group is the author's original order, not a reshuffle. THREE THINGS THE AUTOMATED PASS GOT WRONG, ALL CAUGHT BY READING THE OUTPUT OR BY A GATE: 1. `percent` is not money. The first pass mapped every percent to Money and filed **zoning's lot coverage under a Money header**. `retainage_pct` and `fee_pct` are money; `lot_coverage_pct`, `efficiency_pct` and `diversion_pct` are proportions of area, time and mass. Now decided by name, and zoning reads Identity · Classification · Quantities · Notes. 2. The sort SPLIT the sweep's text+reference pairs — `responsible` classifies as Parties and `responsible_contact` as Links, so a naive sort separates them every time. `test_module_fields` caught it on the first run, exactly as written. The reference now INHERITS its twin's group and sorts immediately after it. 3. A `continue` bound to the inner loop instead of the outer, so single-group modules had their fieldsets stripped and then sorted by a key that no longer existed. FOUR MODULES LEFT UNGROUPED ON PURPOSE: `labor_rate`, `material_rate`, `equipment_rate` and `location` are 3-4 field lookup tables. A section header on three fields is decoration. AND A PRE-EXISTING DEFECT THE WORK SURFACED. `pm_schedule`, `staffing` and `work_order` have had non-contiguous fieldsets since before the sweep — each rendering a duplicate header — because `test_modules` checked contiguity only for an ENUMERATED list of thirteen tier-1 modules. The check was real; its scope was the bug. Same hand-maintained-list shape as `_PROTECTED_PREFIXES` and the `/modules` key allowlist: the ones without a completeness check are the ones that failed. Contiguity is a property of the RENDERER, so the gate now asserts it over EVERY module that uses fieldsets, which costs nothing and needs no upkeep. It deliberately does not require every module to HAVE fieldsets — a three-field lookup legitimately has none — but a module that has them and interleaves them is never legitimate. The count of ungrouped modules is a floor, not an equality, so a new small module can join the set while the 58 cannot quietly drift back. Mutation-checked, and the FIRST mutation was a no-op worth recording: moving zoning's only `Notes` field to the front merely relocates that run and leaves every run unique, so the gate passed and proved nothing. A real interleave — moving a Quantities field into the middle of Identity — fails it with the runs printed. A mutation that cannot express the defect is not evidence. No version files or CHANGELOG — the release lane batches those. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ibuilder
force-pushed
the
feat/mod-fieldsets
branch
from
July 30, 2026 22:40
fe9aa4d to
9bb3564
Compare
Owner
Author
|
Rebased onto The
|
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
23f02cb7,git merge-treeCLEAN as of that SHA. Item 6 of the field sweep's "not done" list. Touches onlymodule.jsonfiles plustest_modules.py— no engine, no renderer, no API.The gap
62 of 133 modules had no fieldsets, so their record form rendered every field in one flat list:
comparable21 fields,listing20,zoning19,lease15. That's the clearest paper-form tell at the layer a user actually touches — a form with no sections is a form nobody can scan.Nine groups, assigned by what the field is: Identity · Classification · Parties · Dates · Money · Quantities · Status · Links · Notes. Fieldsets must be contiguous (the renderer emits one header per run), so this is a stable sort by group — relative order inside a group is the author's original order, not a reshuffle.
58 grouped. 4 left alone on purpose —
labor_rate,material_rate,equipment_rate,locationare 3–4 field lookup tables where a section header is decoration.Three things the automated pass got wrong
percentis not moneyretainage_pctandfee_pctare money;lot_coverage_pct,efficiency_pct,diversion_pctare proportions of area, time and mass. Caught by reading the output; now decided by nameresponsibleclassifies as Parties,responsible_contactas Links.test_module_fields(from #108) failed on the first run, exactly as written. The reference now inherits its twin's group and sorts immediately after itcontinuebound to the inner loopA pre-existing defect this surfaced
pm_schedule,staffingandwork_orderhave had non-contiguous fieldsets since before the sweep — each rendering a duplicate header — becausetest_moduleschecked contiguity only for an enumerated list of thirteen tier-1 modules. The check was real; its scope was the bug. Same hand-maintained-list shape as_PROTECTED_PREFIXESand the/moduleskey allowlist: the ones without a completeness check are the ones that failed.Contiguity is a property of the renderer, so the gate now asserts it over every module that uses fieldsets — costs nothing, needs no upkeep. It deliberately does not require every module to have fieldsets (a three-field lookup legitimately doesn't), but having them and interleaving them is never legitimate. The ungrouped count is a floor, not an equality.
The first mutation was a no-op, which is the part worth recording
Moving zoning's only
Notesfield to the front and re-running: the gate passed. That looks like a mutation surviving. It isn't — relocating a single-field run leaves every run unique, so the mutation could not express the defect at all. A genuine interleave (a Quantities field into the middle of Identity) fails it with the runs printed:A mutation that cannot express the defect is not evidence, and a passing test against one proves nothing.
Verification
node_modulesinto the worktree as a junction, so Vite deniesC:/Server/modelmaker/node_modules/pdfjs-dist/...as outside the project root. CI has realnode_modulesand won't see it.No version files or CHANGELOG — the release lane batches those.
🤖 Generated with Claude Code