feat(ifc-reference-reader): publish the filter receipt on the progress channel - #410
Conversation
…s channel (#407) A consumer drawing `batch-size` segments as they arrive cannot tell two runs apart while it draws: a bridge that honoured `storeys: [L2]` and is streaming one floor, and a pre-1.2.0 bridge that ignored the filter, exited zero, and is streaming the whole building. Both are just geometry turning up. The `selected` receipt settles it, but it rides on the node output — available only once the walk is over, which is the wait progressive delivery exists to remove. So a progressive consumer had no honest option but to refuse to draw filtered reads, which is why floless.app ships progressive import for unfiltered reads only. Publish the same receipt on the `tessellate` record: once, after the filter has resolved to expressIDs and before the first segment — the last moment a consumer can still decide not to draw. Absent `selected` keeps its meaning (this bridge cannot answer), so an unfiltered read publishes none and no existing consumer sees a change. The selection is now resolved before the material and property-set maps rather than after. Those cost real time on the files this command exists for and the selection needs neither; leaving them in front would have made the receipt arrive that much later, which is dead air a progressive consumer waits through. Both maps are read only from `onMesh`, so behaviour is unchanged. The echoed lists are bounded, because the runtime drops an over-long record whole rather than truncating it. `ids` is documented for re-reading a known selection, so a few hundred GlobalIds is ordinary use and more than the 8 KiB budget — echoing them verbatim would delete the record precisely when the filter was largest, and the consumer would read that absence as "old bridge, do not draw". Lists are dropped largest-first until the record fits and each one is named in `selected.elided`; `candidates` always survives. Dropped rather than shortened, because a half-echoed list is a different filter. Refs #407
|
@codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Review result — Codex, clean. Codex reviewed CI green on the same commit: Merged per the CLAUDE.md §"Git workflow" carve-out (cross-model review on the final commit + CI green on that commit). Squashed as The three judgement calls flagged under Notes for reviewers — the Generated by Claude Code |
Summary
read-modelnow publishes theselectedfilter receipt on thetessellateprogress record — once, after the filter resolves to expressIDs and before the first segment, which is the last moment a consumer can still decide not to draw. Refs read-model: publish theselectedfilter receipt on the progress channel, so a filtered read can be drawn progressively #407.selectedkeeps its existing meaning ("this bridge cannot answer"), so an unfiltered read publishes none and no existing consumer sees a change.The problem
A consumer drawing
batch-sizesegments as they arrive cannot tell two runs apart while it draws: a bridge that honouredstoreys: [L2]and is streaming one floor, and a pre-1.2.0 bridge that ignored the filter, exited zero, and is streaming the whole building. Both are just geometry turning up.selectedsettles it, but it rides on the node output — available only once the walk is over, which is the wait progressive delivery exists to remove. So a progressive consumer had no honest option but to refuse to draw filtered reads at all, which is why floless.app ships progressive reference-model import for unfiltered reads only.Type of change
Decalog check
What changed
cli-connection-reader/index.mjsreadModelgains an optionalopts.onSelected(report), called exactly once with the receipt (ornullwhen no filter was applied), before any mesh streams. It mirrors the existingopts.onObjectsink, so the progress channel stays a concern of the streaming layer rather than leaking intoreadModel.readModelStreamedemits thetessellaterecord from that callback instead of before the read. The record's existing fields (phase,message) are unchanged; onlyselectedis new.selectExpressIdsreads the storey map and nothing else. Leaving them in front would have made the receipt arrive that much later, which is dead air a progressive consumer waits through. Behaviour is unchanged: both maps are consulted only fromonMesh, which cannot run untilStreamMeshesis called.cli-connection-reader/progress.mjs— exportsMAX_RECORD_BYTESandprogressRecordBytes(), so the producer can measure a record against the limit the runtime will actually apply.Docs —
read-model.mdgains a May I draw this? section;manifest.yaml→ 1.5.0 (additive, MINOR) with the version-history note the file keeps for every behaviour change.The part the issue does not cover: the record can delete itself
cli/src/runtime/progress.rsdrops a record overMAX_RECORD_BYTES(8 KiB) whole, not truncated, and silently — the channel is advisory. A receipt echoes caller-supplied values, andidsis documented for "re-reading a known selection", so a few hundred GlobalIds is ordinary use and already over budget.Emitting the full echo would therefore have deleted the record precisely when the caller filtered hardest — and the consumer would read that absence as "old bridge, do not draw". The feature would be silently off in its best case, which is worse than not shipping it.
So the echoed lists (
ids,unmatched,ifcTypes,storeys) are dropped largest-first until the record fits, and every one dropped is named inselected.elided:{ "phase": "tessellate", "selected": { "storeys": ["L2"], "candidates": 4980, "elided": ["ids", "unmatched"] } }Dropped rather than shortened, deliberately: a half-echoed list is a different filter, and a consumer comparing it against what it sent would find a mismatch it could only read as "this bridge honoured something else".
candidatesalways survives — it is the one number that says a filter ran at all. The response's ownselectedis never elided, so the complete artifact always carries the full receipt.Verification
Run on Linux with the pinned toolchain (1.95.0) and CI's apt deps (
clang libsecret-1-dev libdbus-1-dev pkg-config):cli-connection-reader:node --testcli/:cargo fmt --all -- --checkcli/:cargo clippy --all-targets -- -D warningscli/:cargo testNo Rust changed; the
cli/gates are the unchanged-baseline check.Mutation evidence
Every test added was proven red by breaking the code it covers, one mutation at a time, then restored.
an UNFILTERED read publishes no receipt, so absent keeps its meaningtessellateRecord: the LARGEST list goes first, so the fewest are losta huge ids filter still publishes a record the runtime will accept,…LARGEST list goes first…,…every echoed list can go…the filter receipt rides on tessellate, before the first segment,a huge ids filter still publishes…progress.rs)the producer measures against the runtime's OWN limit, not a copy of the numberelideda huge ids filter still publishes…,…LARGEST list goes first…,…every echoed list can go…candidatesalong with the listsa huge ids filter still publishes…,…LARGEST list goes first…,…every echoed list can go…Two mutations I tried were no-ops, and I am reporting them rather than counting them:
selection.applied ?gate changed nothing, becauseselectExpressIdsreturns noreportkey at all when no filter was given, sotessellateRecordalready receivesundefined. The gate is belt-and-braces that states the intent; M1b above is the mutation that models the real regression.candidatestoECHOED_LISTSchanged nothing: the elision filter requiresArray.isArray, so a number can never be picked, and it is the smallest value anyway so the loop stops before reaching it.candidatesis protected structurally rather than by that list. M9 is the single-edit mutation that does kill the assertion.Notes for reviewers
readModelis the one behavioural risk worth a second pair of eyes. My reasoning that it is safe:materialsandpropertySetsare captured byonMeshonly, andonMeshcannot fire beforeStreamMeshes/StreamAllMeshesis called further down.selectExpressIdstakes(api, modelID, opts, storeys)and touches neither map. All 73 running bridge tests agree, but they are not proof of absence.tessellateRecordis not wrapped in a try/catch, unlike the channel write inemitProgress. The "advisory, never fail a read" doctrine covers writing to a channel that may be missing or unwritable; building the record is our own pure code over data the bridge itself produced, and swallowing a throw there would hide a defect rather than tolerate an environment. Happy to be argued out of this.elidedmechanism is new surface. If a reviewer would rather see the record simply omitted when it cannot fit, say so — but that puts the feature silently off in exactly the large-filter case it is most wanted.Review
Codex is primary per CLAUDE.md §"PR review — non-negotiable"; requested on this PR. Result will be recorded here and in the routine log (#342).
Generated by Claude Code