SCALE-SEAM (98) — detailing carriers, and a field map total over one module but not the codebase - #431
Merged
Merged
Conversation
…trix loses
Moves `lodSummary`/`setLod` and `phasing`/`setPhase` out of `client.ts` into
`api/model.ts`. They answer one question — *what state are the model's elements
in, and set it?* — and `client.ts` goes 727 -> 711, 76 methods above the STAYING
banner.
The grouping is derived, not asserted:
- identical return shape `{ total, <x>ed, prop, counts: Record<...> }`;
- both writers are `(pid, guids, <enum>, publish) -> editIfc`;
- both readers are consumed by `viewer/tools/modelStatePanels.ts` (251, 316);
- both writers sit unwired and *adjacent* on `clientCallers.test.ts`'s UNCALLED
allowlist;
- `model.ts` already owned `/model/lod/census`, `/lod/handover-readiness` and
`/lod/assessment`, while the base distribution `/projects/{pid}/lod` was left
behind in `client.ts` — `lodSummary` was a sibling separated from its family.
`authoring_matrix.py` DISAGREES and is recorded as the losing vote rather than
elided. It files `set_lod` under `data` and `set_phase` under `lifecycle`,
because it categorises by the IFC output each recipe writes — an LOD stage tag
against `Massing_Phasing.Status`. Different property sets, same question, which
is (89)'s "storage is a HOW" trap. This is the first slice where the matrix has
been wrong after being right three running.
It also MEETS (94)'s objection rather than overriding it: that slice declined
`setPhase` because taking the writer alone would have stranded `phasing()`, the
reader/writer split (87) had to undo. Both halves move together here.
The four names are added to `surface.test.ts` because its floor is a slack
ratchet (788 actual vs 751 floor) — the count alone would not notice a loss —
and because the UNCALLED allowlist is about call sites, not the surface.
Verified: tsc 0, lint 0, `vitest run src/api` 27 files / 119 tests, build 0,
`test_file_sizes.py` / `test_claude_md_gates.py` / `test_roadmap_status.py` /
ruff all 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tt2XKB83wwNt2nrMbK6eEA
…ness that actually bounds Moves `lod500`, `setManufacturerInfo` and `attachOmDocument` out of `client.ts` into `api/model.ts`, rejoining `verifyAsbuilt` and `recordAsbuiltDimension` which (94) moved. `client.ts` goes 711 -> 698, 73 methods above the STAYING banner. TWO BOUNDING WITNESSES, derived independently, agreeing: - `openAsBuiltPanel` in `viewer/tools/modelStatePanels.ts` calls exactly five API methods — read off that function's brace closure rather than grepped for, so it is the closure of a scope and not a sample. Two of the five were already in `model.ts`. - The reader's own response type names its writer set field by field: `verified`/`by_method` from `verify_asbuilt`; `with_dimensions`/`dimensions_out_of_tolerance` from `record_asbuilt_dimension`; `with_manufacturer`/`with_serial` from `set_manufacturer_info`; `with_om_docs`/`om_documents` from `attach_om_document`. The backend route says it in prose: "Stamp elements with the `verify_asbuilt` recipe." A reader whose response type enumerates its writers is a DERIVED population. After eight slices of sampling, that is the first grouping witness here that bounds a set instead of illustrating one. WHAT CORROBORATES IS NOT WHAT BOUNDS. `test_lod500.py` reaches exactly three recipes — `attach_om_document`, `set_manufacturer_info`, `verify_asbuilt` — and OMITS `record_asbuilt_dimension`, which is unambiguously in this family. It agrees with the answer without being able to establish it. Three slices claimed "and no others" off a test file and were wrong all three times. NOT CLAIMED, and said in the header, the pin, the changelog and the roadmap rather than only in review: `attachDocument` stays in `client.ts`, takes a `purpose` parameter, and `asbuilt_summary` counts ANY purpose-tagged document reference — so "every writer of `with_om_docs` moves here" is false. The field map is of the recipes each field was designed around, not of everything that can set it. Two sources disagree and lose for the same reason: `attach_om_document` is a purpose-tagged wrapper of `detailing.attach_document`, and `authoring_matrix.py` files it and `set_manufacturer_info` under `data`. The first is a shared HELPER, the second a STORAGE bucket — the groupings (89) and (90) each had to reject. The matrix is the losing vote twice running after being right three times. Also fixes the extraction helper: it terminated its brace count on the method signature line, so a signature wrapping across two lines with no opening brace on the first read as a 3-line method. `attachOmDocument` is 6. Verified: tsc 0, lint 0, `vitest run src/api` 27 files / 119 tests, build 0, ruff (the CI command, from `services/api`) 0, and `test_file_sizes.py`, `test_claude_md_gates.py`, `test_roadmap_status.py`, `test_ruff_scope.py`, `test_reachable.py` all 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tt2XKB83wwNt2nrMbK6eEA
PR #412 was squash-merged, so origin/main carries (95) as b0e8281 while this branch still pointed at the pre-squash 44a6ed4. The content diff between the remote branch and origin/main was empty before this merge, so the branch held no unmerged work; -s ours keeps this branch's tree (origin/main plus (96)) and reconciles the histories without rewriting the remote branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tt2XKB83wwNt2nrMbK6eEA
… and was not
Moves `editHistory`, `editUndo` and `editRedo` out of `client.ts` into
`api/authoring.ts`. `client.ts` goes 698 -> 687, 70 methods above the STAYING
banner.
WHY THAT MIXIN: `editIfc`, already there, is the PUSH they pop. `authoring.py`
records the pre-edit version on every `/edit` call "so this edit can be undone",
`_restore_version` pops that stack, and `edit_history.state()` reads its depths.
One stack, and the operation that fills it was already in this file.
The types agree: both writers return `{restored, state: {can_undo, can_redo}}`,
and `state` is `editHistory`'s own return type minus the depths — the writers
hand back the reader's answer. That is a type-level relation, not the shared
`/edit/` prefix. The prefix is real and is deliberately not the argument, since
a route prefix is exactly what the verification slice was caught grouping on.
A HYPOTHESIS TESTED AND WITHDRAWN, which is the part worth keeping. "Undo
restores the prior model version" makes `model.ts` the obvious home — it owns
`modelVersions`, `versionDiff`, `versionCostDelta`. It is the wrong home: those
read `/projects/{pid}/versions` out of `bim.py`, while undo pops a DIFFERENT
stack, the `edit_history` sidecar, which `recipe_log.py` describes as a list of
file paths with "No recipe, no parameters, no actor". Two stacks, one word — and
the word is what made the wrong answer look obvious. Second withdrawal of a
plausible destination after checking it, after (93) withdrew (92)'s `HttpCore`
forecast.
THE BOUND IS WEAKER THAN (96)'s AND IS STATED THAT WAY. `app.ts`'s S4 block
wires `refreshUndo` (calling only `editHistory`) and `doUndoRedo` (calling only
`editUndo`/`editRedo`), so the union is exactly these three — but that unit is a
block delimited by reading, not a closure the braces define. It corroborates; it
does not bound. Not every set has a witness as strong as the last one's, and
promoting a block to a closure would be this sequence's own recurring defect.
Verified: tsc 0, lint 0, `vitest run src/api` 27 files / 119 tests, build 0,
ruff (the CI command, from `services/api`) 0, and `test_file_sizes.py`,
`test_claude_md_gates.py`, `test_roadmap_status.py` all 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tt2XKB83wwNt2nrMbK6eEA
PR #413 was squash-merged, so origin/main carries (96) as ffb7b9f while this branch still held the pre-squash commits. The content diff between the remote branch and origin/main was empty before this merge, so the branch held no unmerged work; -s ours keeps this branch's tree (origin/main plus (97)) and reconciles the histories without rewriting the remote branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tt2XKB83wwNt2nrMbK6eEA
…flagged one CodeRabbit flagged `editUndo`'s docstring on #414: it says "restore the prior model version + republish" unconditionally, but `publish` defaults to true and can be false. Verified against the backend — `_restore_version` guards the republish with `if publish:` — so the claim is wrong whenever a caller passes `publish: false`. Grepping every site of that wording found the same unconditional claim in THREE live places, not one: - `apps/web/src/api/authoring.ts` — `editUndo` (the flagged one) - `services/api/src/aec_api/routers/authoring.py` — the `/edit/undo` route - `services/api/src/aec_api/routers/authoring.py` — the `/edit/redo` route All three now say republishing happens only when `publish`. The remaining hits are UI button labels, where the call site never passes the flag so republish really is unconditional, and historical CHANGELOG entries; both are correct as written and are left alone. Also fixes the same defect's other half in the same five lines: the declared return type omitted `publish`, which the backend adds as `"running"` when it republishes. Correcting the prose while leaving the type silent about the same conditional would be the half-applied fix this sequence has twice been caught making. Verified: tsc 0, lint 0, `vitest run src/api` 27 files / 119 tests, build 0, ruff (the CI command, from `services/api`) 0, `test_file_sizes.py` 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tt2XKB83wwNt2nrMbK6eEA
…module but not the codebase Moves `elementDetailing`, `classify`, `applyDetailingRules`, `validateDetailing` and `attachDocument` out of `client.ts` into a new `api/detailing.ts`. `client.ts` goes 687 -> 665, 65 methods above the STAYING banner. WHAT THEY ANSWER: what informational carriers are attached to this element, write them, and which are missing? THE WITNESS IS A 1:1 AND TOTAL FIELD-TO-WRITER MAP, the shape (96) established. `element_detailing` walks `HasAssociations` and branches on exactly two relationship types, and `detailing.py` holds exactly two writers, one per response array: classifications[] <- classify (IfcRelAssociatesClassification) documents[] <- attachDocument (IfcRelAssociatesDocument) The map is read out of the reader's own body, not matched on names. The other two methods are those same writes automated and audited: `applyDetailingRules` runs the condition-to-content rule set and writes both carrier kinds, `validateDetailing` reports elements a rule applies to that lack the code. TOTAL OVER THE MODULE, NOT THE CODEBASE, and the difference is the claim. `attachOmDocument` — moved to `model.ts` in (96) — wraps the SAME `detailing.attach_document` and also lands in `documents[]`, so "these are all the writers of this reader's fields" is false. That overlap was recorded when it moved, which is why it was on hand to qualify this slice rather than being found in review. ADJACENCY AGREED WITH THE ANSWER AND IS NOT EVIDENCE FOR IT. These five were contiguous in `client.ts`; unlike (95), where non-contiguity was the whole argument, a positional split would have found this set too. Stated precisely because it looks like support. ALSO RECORDED: `api.classify()` has no call site — `detailingSection.ts` drives the recipe through the generic `authorAndReload` path, bypassing the typed method, and `clientCallers.test.ts` counts it reached because it matches bare string literals as well as calls. That looseness is deliberate per that file's own docstring, so it is a limit of the gate rather than a defect in it. The new mixin needs `editIfc`, so it declares `NeedsEditIfc` and composes outside `withAuthoring`; `compositionOrder.test.ts` gains a fourth assertion. MUTATION-CHECKED: relaxing the constraint to `Ctor<any>` produces "TS2578: Unused '@ts-expect-error' directive" on exactly the new line, so it fails for the reason claimed rather than merely passing. The extraction removed 27 lines and the banner plus import added 5 back — caught by the ratchet, not by me, the same way (94) found its own banner growth. Verified: tsc 0, lint 0, `vitest run src/api` 27 files / 119 tests, build 0, ruff (the CI command, from `services/api`) 0, and `test_file_sizes.py`, `test_claude_md_gates.py`, `test_roadmap_status.py` all 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tt2XKB83wwNt2nrMbK6eEA
PR #414 was squash-merged, so origin/main carries (97) as 3faae36 while this branch still held the pre-squash commits. The content diff between the remote branch and origin/main was empty before this merge, so the branch held no unmerged work; -s ours keeps this branch's tree (origin/main plus (98)) and reconciles the histories without rewriting the remote branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tt2XKB83wwNt2nrMbK6eEA
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change extracts five detailing methods from ChangesDetailing API extraction
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Detailing API methods were reorganized into a dedicated mixin while retaining the ApiClient surface and mutation capability requirements. The supplied checks and documentation updates support merge readiness with no identified production risk. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 5 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review Generated by Claude Code |
|
✅ Action performedReview finished.
|
ibuilder
marked this pull request as ready for review
September 4, 2026 06:14
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. So far, Strix has reviewed 28 pull requests, surfaced 3 security issues (1 critical/high) and blocked 2 risky merges across this workspace. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
ibuilder
pushed a commit
that referenced
this pull request
Sep 4, 2026
PR #431 was squash-merged, so origin/main carries (98) as ae4bc0e while this branch still held the pre-squash commits. The content diff between the remote branch and origin/main was empty before this merge, so the branch held no unmerged work; -s ours keeps this branch's tree (origin/main plus (99)) and reconciles the histories without rewriting the remote branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tt2XKB83wwNt2nrMbK6eEA
7 tasks
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
2 participants
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.
What & why
apps/web/src/api/client.tsgoes 687 → 665 (65 methods above the STAYING banner, 4 below).Five methods move to a new
apps/web/src/api/detailing.ts:elementDetailing,classify,applyDetailingRules,validateDetailing,attachDocument. What they answer: what informationalcarriers are attached to this element, write them, and which are missing? Continues SCALE-SEAM in
docs/roadmap.md.A 1:1 and total field-to-writer map
The witness is the shape (96) established, at its strongest yet. The reader's response has exactly
two carrier arrays, and
services/data/src/aec_data/detailing.pyholds exactly two writers, one perarray:
classifications[]classifyIfcRelAssociatesClassificationdocuments[]attachDocumentIfcRelAssociatesDocumentelement_detailingwalksHasAssociationsand branches on precisely those two relationship types —nothing else contributes a field — so the map is read out of the reader's own body rather than
matched on names. The other two methods are those same two writes automated and audited:
applyDetailingRulesruns the condition-to-content rule set and writes both carrier kinds, andvalidateDetailingreports elements a rule applies to that lack the required code.Total over the module, not over the codebase
attachOmDocument— moved tomodel.tsin (96) — is a purpose-tagged wrapper of the samedetailing.attach_document, so it also writesIfcRelAssociatesDocumentand its output lands indocuments[]. So "these are all the writers of this reader's fields" is false, and is notclaimed. The supported claim is narrower: the map is 1:1 and total over
detailing.py.That overlap was named when
attachOmDocumentmoved, which is why it was on hand to qualify thisslice instead of being found by a reviewer — recording a limit pays off in the slice after.
Adjacency agreed with the answer, and is not evidence for it
These five were contiguous in
client.ts(119–145). Unlike (95), where non-contiguity was thewhole argument for grouping by what methods answer, a positional split would have found this set
too. Worth stating precisely because it looks like support: a grouping that happens to coincide
with adjacency is not thereby better evidenced.
Also recorded:
api.classify()has no call siteapps/web/src/viewer/tools/detailingSection.tsdrives the recipe through the genericauthorAndReload("classify", …)path, bypassing the typed method.apps/web/src/api/clientCallers.test.tscounts it as reached because it matches bare stringliterals as well as calls — a looseness that file's own docstring declares deliberate, preferring a
higher ceiling to a false unreachability report. So it's a limit of that gate, not a defect in it.
Noted in the new file's header so the next reader doesn't assume the method is live.
Composition
The new mixin needs
editIfc, so it declaresNeedsEditIfcand composes outsidewithAuthoring;apps/web/src/api/compositionOrder.test.tsgains a fourth assertion. That assertion wasmutation-checked: relaxing the constraint to
Ctor<any>producesTS2578: Unused '@ts-expect-error' directiveon exactly the new line, so it fails for the reason claimed ratherthan merely passing.
The extraction removed 27 lines and the banner plus import added 5 back — caught by the size
ratchet, not by me, the same way (94) found its own banner growth.
Note on this branch's history
#414 was squash-merged, so
origin/maincarries (97) as3faae36awhile this branch still held thepre-squash commits. The content diff between the remote branch and
origin/mainwas verified emptyfirst, so the branch held no unmerged work; the histories were reconciled with a
-s oursmergerather than a force-push.
Checklist (mirrors CONTRIBUTING.md)
cd services/api && python -m ruff check ../..cleantest_file_sizes.py,test_claude_md_gates.py,test_roadmap_status.pyexit 0;full suite running as a cross-check
npx tsc --noEmit0,npm run lint0,npm run build0;npx vitest run src/api27 files / 119 testsCHANGELOG.mdentry added (newest at top);docs/roadmap.mdupdated🤖 Generated with Claude Code
https://claude.ai/code/session_01Tt2XKB83wwNt2nrMbK6eEA
Generated by Claude Code
Summary by CodeRabbit
Refactor
Documentation
Tests