Skip to content

feat(export): native merged/federated IFC export at parity with the JS exporter (#2951) - #2970

Open
BIMvoice wants to merge 10 commits into
mainfrom
consolidated/2951-native-merged-export
Open

feat(export): native merged/federated IFC export at parity with the JS exporter (#2951)#2970
BIMvoice wants to merge 10 commits into
mainfrom
consolidated/2951-native-merged-export

Conversation

@BIMvoice

Copy link
Copy Markdown
Collaborator

Closes #2951 — native (Rust) merged/federated IFC export at parity with the JS merged exporter.

Verified

  • cargo test -p ifc-lite-export --lib256 passed / 0 failed
  • cargo test -p ifc-lite-processing --test module_size_ratchet5/5, including allowlist_digest_is_pinned
  • check-changesets.mjs — pass
  • No TS public surface touched, so check-api-surface does not apply

The decomposition stays under the 400-line rule without an allowlist entry: merged_guid.rs 198 lines, merged_visibility.rs 298.

One defect fixed on the way, worth naming

leading_guid scanned for the first quoted token anywhere in the line, so any entity whose first string attribute was not a GlobalId could be misread. It now reads the GlobalId positionally and gates on IfcType::is_subtype_of(IfcRoot) (rust/core/src/generated/schema.rs:6354), so a non-rooted entity is never treated as carrying one.

That mattered more than it looks: a test elsewhere had been using IFCGRIDAXIS's AxisTag — a non-rooted attribute — as a stand-in GlobalId, and passed because of the bug. Code and fixture shared one wrong assumption about what a GlobalId is, which is the class of defect mutation testing cannot catch, since the mutation is judged by the same blind test.

🤖 Generated with Claude Code

export_merged_with_stats (rust/export/src/merged.rs) only ID-offsets
STEP entity instance names (#123) across federated models; GlobalId is
a separate 22-char attribute the offset never touches. Two federated
models sharing an element (same file merged twice, a shared grid, a
linked type) emitted that element's GlobalId twice into one file -- a
spec violation independent of the exporter's other parity gaps tracked
in #2951.

A model after the first now has each IfcRoot entity's GlobalId checked
against everything already emitted; a collision is re-stamped with a
fresh deterministic id (mirroring merged-exporter.ts's mintUniqueGuid)
rather than written through unchanged. Ported the same 22-char-alphabet
GlobalId check, the non-rooted-type denylist, and the deterministic-id
hash from packages/parser/src/deterministic-global-id.ts, cross-checked
byte-for-byte against the JS implementation for several seeds.
…porter (#2951)

Adds MergedOptions.included -- a per-model (index-aligned) VisibilityFilter
{roots, excluded}, mirroring the role StepOptions.included already plays for
single-model export and computeIncludedEntityIds's role in
merged-exporter.ts. An absent field/entry keeps a model in full; an
explicit empty filter keeps nothing from it, pinned by dedicated tests so
the two can't collapse into each other.

The closure never walks into an excluded id (so a hidden product can't
re-enter through a relationship naming it), and a kept IFCREL* entity that
still names an excluded id is dropped to a fixpoint rather than emitted
with a dangling #ref -- there is no per-SET/LIST-attribute narrowing on the
Rust side yet, so dropping the whole relationship is the conservative,
correctness-first choice for this increment.

Also fixes a latent bug this surfaced: excluding model 0's own IfcProject
used to leave every later model's own project dropped anyway (redirected to
a canonical project that was never written), producing a file with no
IfcProject at all.

New module rust/export/src/merged_visibility.rs keeps merged.rs under its
400-line module-size ratchet budget (396/400).
…Root, not a scan+denylist

leading_guid() in rust/export/src/merged.rs scanned for the first quoted
token anywhere on a STEP entity line, then excluded a fixed denylist of
non-IfcRoot types whose first attribute is itself a string. Both halves
were unsound: a non-rooted entity whose first attribute is not a string
(e.g. IFCMATERIALLAYER, whose Name is its 4th attribute) could still
expose a later quoted string to the scan no matter how complete the
denylist was, and the denylist itself was missing several non-rooted
types (IFCMATERIALLAYER, IFCMATERIALLAYERSET, and others). When that
coincidentally 22-character, GlobalId-charset string collided with a
real GlobalId already emitted, the GlobalId-collision reconciliation
this branch added silently rewrote it -- corrupting ordinary model data.

GlobalId identification is now positional (the quoted token must be the
entity's true first attribute, only whitespace allowed before it) and
type-checked against rust/core's generated IfcRoot subtype table via
IfcType::is_subtype_of, rather than a hand-maintained denylist that can
drift out of sync with the schema.

The existing shared-GlobalId reconciliation test used IFCGRIDAXIS's
AxisTag as a stand-in "GlobalId" -- IfcGridAxis is not an IfcRoot
subtype, so that test only exercised reconciliation because of this
same bug. Rebuilt it with genuinely rooted entities (IFCDOOR/IFCWALL/
IFCSPACE), and added regression coverage for the corruption itself:
a non-rooted entity's coincidentally GlobalId-shaped string must survive
the merge byte-for-byte, whether it's the entity's first attribute or a
later one, and even for a malformed line where a genuinely rooted type's
first attribute isn't a string at all.
…r's GUID check

leading_guid's IfcType::is_subtype_of(IfcRoot) check is resolved against
rust-core's generated schema, which is derived from IFC4X3 only. A rooted
type that exists in IFC2X3 and/or IFC4 but was dropped or renamed in
IFC4X3 (IFCPROXY, IFCDOORSTYLE, the IFC4 *STANDARDCASE variants, and 51
others) resolves to IfcType::Unknown and was silently treated as
non-rooted, so its GlobalId was never reconciled across a merge -- the
same defect the schema-derived check was meant to fix, just on older
files. Reproduced by execution (RED) with a shared IFCDOORSTYLE GlobalId
across two IFC2X3 models duplicating in the merged STEP text.

Adds a small supplemental table of the 54 confirmed IFC2X3/IFC4-only
rooted types (derived by diffing @ifc-lite/data's per-schema entity
tables against the IFC4X3-only generated schema), consulted only when
IfcType::from_str resolves to Unknown -- anything genuinely unrecognised
stays non-rooted, so the corruption the prior fix closed stays closed.
…e merged exporter's visibility filter

merged_visibility.rs's fixpoint withheld a kept relationship's whole
line the instant it named ANY excluded id, regardless of where that id
sat. On real IFC this is more than conservative: an exporter that
lists every element of a storey in one
IFCRELCONTAINEDINSPATIALSTRUCTURE loses that storey's containment for
every other, still-visible element just because one sibling was
hidden.

narrow_relationship_line mirrors filterHiddenRefsFromRelationshipLine
(reference-collector.ts): a SET/LIST attribute is narrowed to its
surviving members, and the whole line is withheld only when an
excluded id sits in a single-valued slot or was a SET/LIST's only
member -- including the one schema-optional exception
(IfcRelConnectsStructuralMember.ConditionCoordinateSystem) JS carries.
Reuses step_text::split_top_level_args rather than adding a second
attribute-group parser.

Also corrects merged_visibility.rs's docstring, which claimed the
old whole-drop approach "can only under-connect, never dangle" -- true
only for the IFCREL* shape it inspects, not for a non-IFCREL* entity
(e.g. IFCSTYLEDITEM.Item) still referencing an excluded id. That gap
is inherited from the JS reference, which documents it openly, not
introduced here.
# Conflicts:
#	rust/export/src/merged_tests.rs
…ng changesets

Both describe rust/export/src/merged.rs's visibility-filter feature; the
narrowing changeset directly amended a claim made in the visibility-filter
changeset (whole-relationship-drop -> per-attribute narrowing), so keeping
both as separate entries would contradict itself within the same release.
The two forked #2951 chains each grew merged.rs independently
(guid-misidentification/schema-coverage added the IfcRoot type check and
the 54-entry legacy rooted-type table; visibility-filter/relationship-
narrowing added MergedOptions.included and narrowing). Combined, the file
crossed the 400-line module-size ratchet (432 lines) with no allowlist row.

Split along the seam merged.rs already documented in its own comments:
GlobalId identification and re-stamping (leading_guid, is_legacy_rooted_type,
replace_leading_guid, deterministic_global_id, mint_unique_guid) is a
self-contained concern independent of the emission loop that calls it
(offsetting, visibility filtering, relationship narrowing) -- the same kind
of seam merged_visibility.rs already split out for the filtering side.

merged.rs: 245 lines. merged_guid.rs: 198 lines. Ratchet passes with no
allowlist changes and no digest change.
…e line

Adds narrowing_and_guid_restamping_both_apply_to_the_same_relationship_line,
exercising the one interaction neither forked #2951 chain could test on its
own: an IFCREL* line (itself an IfcRoot subtype, so it carries its own
GlobalId) that is BOTH narrowed (relationship-narrowing chain, a SET member
excluded by visibility filtering) AND re-stamped (base/misidentification/
schema-coverage chain, its GlobalId collides with an earlier model's).

Asserts the exact emitted line: narrowed SET, re-stamped GlobalId (shape-
checked, not just non-empty), and everything else byte-identical to what
narrowing+offsetting alone would produce -- confirming re-stamping only ever
touches the leading quoted attribute.

Mutation-tested locally (not committed): disabling narrowing alone left the
excluded SET member in the output while re-stamping still worked; disabling
re-stamping alone left the collided GlobalId unchanged while narrowing still
worked. Both mutations made this test fail as expected; file restored via
`cp` + `diff` to byte-identical afterward.
@BIMvoice
BIMvoice requested a review from louistrue as a code owner August 21, 2026 08:01
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@BIMvoice, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 46 minutes

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: adb73c75-cc52-4bb4-adbd-a992b534ae47

📥 Commits

Reviewing files that changed from the base of the PR and between e6a651d and 60e4282.

📒 Files selected for processing (11)
  • .changeset/merged-export-globalid-collisions.md
  • .changeset/merged-export-guid-misidentification.md
  • .changeset/merged-export-guid-schema-coverage.md
  • .changeset/merged-export-visibility-filter.md
  • rust/export/src/lib.rs
  • rust/export/src/merged.rs
  • rust/export/src/merged_guid.rs
  • rust/export/src/merged_tests.rs
  • rust/export/src/merged_visibility.rs
  • rust/export/src/merged_visibility_tests.rs
  • rust/export/src/step_text.rs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Viewer benchmark

✅ No threshold regressions detected.

01_Snowdon_Towers_Sample_Structural(1).ifc

Baseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric Current Baseline Delta Threshold Status
firstBatchWaitMs 1334ms 2905ms -54.1% +50%
firstVisibleGeometryMs 1723ms 3652ms -52.8% +50%
streamCompleteMs 1981ms 3598ms -44.9% +50%
spatialReadyMs 985ms 1032ms -4.6% +50%
metadataCompleteMs 1365ms 3063ms -55.4% +50%
totalWallClockMs 2100ms 3700ms -43.2% +50%

AC20-FZK-Haus.ifc

Baseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric Current Baseline Delta Threshold Status
firstBatchWaitMs 270ms 1075ms -74.9% +50%
firstVisibleGeometryMs 952ms 1572ms -39.4% +50%
streamCompleteMs 970ms 1980ms -51.0% +50%
spatialReadyMs 755ms 915ms -17.5% +50%
metadataCompleteMs 854ms 1392ms -38.6% +50%
totalWallClockMs 1200ms 3300ms -63.6% +50%

Refresh the baseline from a CI run: dispatch the Benchmark workflow with record_baseline, download the benchmark-baseline artifact, and commit baseline.json (see tests/benchmark/README.md).

@louistrue

Copy link
Copy Markdown
Collaborator

#2951 is assigned to @Blogbotana, who has had #2952 open on it since 2026-08-20 17:20. This PR opened today at 08:01 and says Closes #2951.

Two implementations of the same feature, one of them from the person the issue was assigned to. Flagging rather than judging the code — I have not reviewed either.

For whoever untangles it, the state as of now:

#2952 @Blogbotana, opened 08-20 17:20, currently red
#2970 @BIMvoice, opened 08-21 08:01, green
#2951 assigned to @Blogbotana

#2952's single failure is merged::tests::merge_two_models_unifies_project_and_offsets_ids at rust/export/src/merged/tests.rs:41assertion left == right failed, left: 75965, right: 77795. 260 passed, 1 failed. A count mismatch, not a structural problem, and it is the only thing standing between that PR and green.

The house convention is to check for an existing contributor PR before starting an assigned issue, precisely so this does not happen. Since it has, the call on which lands is a maintainer's, not mine and not either author's — but whichever way it goes, the work in the other PR deserves an explicit disposition rather than being closed silently, and @Blogbotana deserves to hear it from a person.

If #2970 is the one that lands, the fair outcome is credit to @Blogbotana on the issue and a note on #2952 saying why. If #2952 is, this one supersedes cleanly since it is newer.

@louistrue

Copy link
Copy Markdown
Collaborator

Holding this one. #2951 was filed by @Blogbotana, assigned to @Blogbotana, and implemented in #2952 fifteen hours before this PR opened. The maintainer decision is that the external contributor's PR takes precedence, so #2952 is the one that lands for #2951.

That is a coordination failure on our side rather than anything wrong with the work here, and this PR is not wasted: four things in it exist nowhere in #2952 and are being carried over. Detail is in #2952 (comment), in short:

  1. merged_visibility.rs in full. The excluded set plus narrow_relationship_line. feat(export): native merged/federated IFC export at parity with the JS MergedExporter (#2951) #2952's included is roots plus a forward closure, which cannot express a hidden product: a kept IFCRELCONTAINEDINSPATIALSTRUCTURE pulls the hidden wall back in through the closure. This is the faithful port of the JS hiddenProductIds and filterHiddenRefsFromRelationshipLine path, and without it a JS-parity visibleOnly export is not expressible. This is the biggest single gap and the reason this PR still matters.
  2. The schema-derived rooted check (is_subtype_of(IfcRoot) plus the 54-entry legacy table), against feat(export): native merged/federated IFC export at parity with the JS MergedExporter (#2951) #2952's 52-entry denylist. Both halves or neither: an allowlist without the legacy table stops deduplicating IFC2X3 GUIDs.
  3. The raw-text GUID-count oracle and the end-to-end non-rooted-string tests. feat(export): native merged/federated IFC export at parity with the JS MergedExporter (#2951) #2952's dedup check reads GUIDs with leading_rooted_global_id, the production classifier, so its differential shares a contract-reading with the code it is testing. Counting occurrences in the output text does not.
  4. The changesets.

Worth being explicit about what does not carry over, so nobody ports it by reflex: #2952 has the unify branch of GUID reconciliation, unit compatibility and the federation fallback, spatial unification, infra dedup, IfcRelAggregates pruning, cross-schema conversion, JS golden-value anchors for deterministic_global_id, and salts the mint by model id rather than index, which keeps GUIDs stable when an unrelated model is added or reordered. On within-model duplicate GUIDs the two differ deliberately: this PR re-stamps them, #2952 preserves them, and preserving matches the JS exporter.

Next step depends on @Blogbotana: either they fold these in, or #2952 lands and the port goes on top as a follow-up. Leaving this open until that is settled rather than closing it, since the visibility work is the thing we would otherwise have to write from scratch.

@louistrue

Copy link
Copy Markdown
Collaborator

The CodeRabbit tick on this PR is a false pass. The only coderabbitai comment here is "Review limit reached" (posted 08:02Z); the API shows zero reviews and zero inline review comments on this PR. The check renders green anyway. What follows is a local coderabbit CLI run (v0.7.5) against origin/main, which goes through a different path and did produce a review.

CLI run: completed, exit 0, 11 files reviewed, 2 findings (1 major, 1 minor).

Real: a parse miss returns None, and None means "withhold", not "unchanged"

rust/export/src/merged_visibility.rs:162-171

The doc block directly above the function (lines 157-161) says a line unparseable as a single #N=TYPE(...); record is "returned unchanged ... matching the JS original's own 'return line unchanged' contract for a regex miss". The code does the opposite: strip_suffix(';')?, find('=')?, find('(')? and rfind(')')? each turn a parse miss into None, and None is the withhold-this-line signal defined at line 146. The JS original really does return the line: reference-collector.ts:635, if (!match) return line;.

Where it bites, and where it does not:

  • Emission path is fine. narrow_for_emission (line 253) maps the None arm to Cow::Borrowed(line), so a parse miss there emits the original bytes, which is what the doc promises.
  • Keep-set path is not. dangling_relationship_ids (line 122) treats None as "this relationship is dangling". compute_keep_set then adds that id to excluded and recomputes the closure, so the record is dropped and anything reachable only through it is pruned too.

Concrete case that reaches it: a malformed record with no parentheses, for example #42=IFCRELAGGREGATES; in a source model. EntityScanner::next_entity does emit that record, and its type_end walk stops at line_end rather than at a (, so type_name comes out as IFCRELAGGREGATES;, which still passes the starts_with("IFCREL") gate at line 118. narrow_relationship_line then hits after.find('(')? and returns None, so #42 is declared dangling and silently deleted along with anything only it referenced. The JS path keeps that line untouched. So this is a real divergence from the parity target, on malformed input, and it fails by deleting rather than by passing through.

One correction to the CLI's proposed fix: the test it suggests (a record without a trailing ;, asserting Some(line)) is asserting on an input this pipeline cannot produce. EntityScanner::next_entity sets line_end to the terminating semicolon plus one (rust/core/src/parser/scanner.rs:157) and only emits a record after validating #<digits>[ws]*= (scanner.rs:144), so strip_suffix(';') and find('=') cannot miss for any slice merged.rs:161 hands over. The two ?s that are actually reachable are find('(') and rfind(')'). If you take the fix, the test that proves it should be the parenthesis-less record.

I read this out of the source and did not run it, so treat the #42=IFCRELAGGREGATES; trace as source-level, not executed.

Judged not worth acting on: MD018 in the changeset

.changeset/merged-export-globalid-collisions.md:14

The CLI wants line 14 reflowed so #2951). does not start a line, citing markdownlint MD018. This repo has no markdownlint: grep -rl markdownlint over the tree hits only prose in docs/guide/server.md, there is no config file, no npm script and no workflow step. Nothing enforces it. It also does not misrender, since GitHub's parser needs a space after # to make a heading, so #2951). stays literal text in the generated CHANGELOG. Harmless to reflow if you are touching the file anyway, but it is not a defect.

Separately, and independent of the CLI: the maintainer decision above still stands. This is about the code, not about whether the PR lands.

louistrue added a commit that referenced this pull request Aug 21, 2026
…3040)

* docs(agents): respect assignments, and claim work before starting it

Three PRs today duplicated work that was already claimed. The one that
matters: issue #2951 was filed by an external contributor, assigned to
them, and implemented in #2952 — and #2970 arrived fifteen hours later
implementing the same thing. They objected, correctly.

The cost is not the wasted effort. It is that someone who did everything
right watched the project duplicate their work.

The rule has three parts, and the third is the one that was missing:
check assignees, check for an open PR referencing the issue, and assign
yourself BEFORE writing code rather than when opening the PR. An
assignment made at PR time claims nothing, because the window it needed to
cover has already closed. Check again just before opening, since a claim
can appear while you work.

Also states who keeps the work when a duplicate happens: the person who
was assigned, not whoever is further along. And that a duplicate is
enumerated before it is closed, so what it uniquely holds is not lost.

* docs(agents): helping is welcome, taking over is not

The first version said "if someone else is assigned, it is theirs, do not
start", which forbids the cases that are actually fine and gives no way to
tell them apart from the case that is not.

Two things make it help rather than a takeover:

They accepted an offer. Comment saying what you would do and wait for a
yes. Silence is not a yes. An assignee who is mid-development and reads
"we have already built this in parallel" is being told, not asked, which is
exactly what happened on #2670.

It has genuinely gone quiet: no commits and no word for about a week, and
even then comment first, wait a couple of days, and reassign explicitly
rather than working in the shadows.

Also lists what needs no permission at all, since the first version could
be read as discouraging it: reviewing their PR, diagnosing a failing check
and posting the cause, answering a question, reporting a defect in shipped
code. And what is not help however good the code: a parallel
implementation announced afterwards, an unraised branch duplicating their
work, pushing to their branch, a competing PR.

If you already built something before noticing, say so, hand it over, and
let them decide. That is recoverable. Landing it is not.

Applies to us as much as to any bot.
@louistrue

Copy link
Copy Markdown
Collaborator

Status note so this does not sit here unexplained.

This and #2952 are competing implementations of #2951, both rewriting rust/export's merged exporter. #2952 is from an outside contributor, and the call was that an outside contributor's PR takes precedence over an internal one on the same issue.

I am not closing this yet, deliberately. #2952 has seven unresolved Major findings on its head, so closing this now would drop working code with nothing merged in its place.

The piece here that #2952 does not have in the same shape is the visibility filter (merged_visibility.rs, merged_visibility_tests.rs, 460 lines). #2952 does handle visibility in mod.rs and plan.rs, and its mod.rs:195 finding is about applying the first model's visibility before building the shared merge targets, so the two are working the same ground from different ends.

Plan: once #2952 lands, this gets closed and the visibility work rebased on top of it as its own PR, rather than the two continuing to diverge. If you want to start that rebase before #2952 merges, that is fine, just expect churn.

merged_guid.rs here overlaps merged/guid.rs there, and #2952 has four Major findings open against exactly that rootedness logic. If you have already solved rootedness properly in this PR, saying so on #2952 would be worth more than the rebase.

BIMvoice added a commit that referenced this pull request Aug 22, 2026
…not green

`check-coderabbit-review.mjs` answers one question per PR: did CodeRabbit
read the diff at the head commit. A sweep across all 36 open PRs found
the failure modes it was built for, plus four it does not cover — each of
which renders as green, or as "nothing failing", in any report that
counts only failing and pending checks:

  - `headRepositoryOwner.login` is not ours. Every other column is then a
    report about a branch we cannot push to.
  - `mergeStateStatus == DIRTY` WITH runs present: green checks over a
    merge commit that can no longer be formed. #2970 and #2971 are in
    exactly that state, and both show `fail=0, pending=0`.
  - ZERO workflow runs at the head commit. A PR that was already DIRTY at
    push time never gets a run, so `statusCheckRollup` comes back EMPTY —
    and an empty rollup counts up to the same `fail=0, pending=0` as a
    fully passing one. This is the vacuous-pass shape, so the run count
    is a first-class signal here rather than something derived.
  - The newest run on the BRANCH is against a superseded commit, while
    the rollup still shows that older commit's green.

The sweep that found these was a throwaway script, which makes it a habit
someone has to remember. It lands here as a second entry point,
`scripts/check-pr-green.mjs`, over a new pure module
`scripts/lib/pr-green-sweep.mjs`. `coderabbit-review-state.mjs` is reused
unchanged — the review verdict is one column of the table, not a
reimplementation.

The disqualifier order is worst-first and each earlier reason invalidates
the later ones AS EVIDENCE: a fork's counts are not ours to read, a DIRTY
base means the green ran on a merge that cannot be formed, zero runs
means the counts are empty rather than passing, a stale run means they
describe a commit that is no longer the head. `not ours` takes precedence
as a disqualifier but ranks LOWEST for sorting, because it is the one
verdict that is not a task; both halves of that deliberate disagreement
are pinned by tests.

It must not pass vacuously, so three cases are hard failures with their
own `kind` and their own message rather than a shorter, cleaner report:
zero PRs returned (what a wrong `--author`, a wrong repo and a truncated
response all look like), an unreachable API — including mid-sweep, where
a swallowed per-PR failure would drop a row from a report that still
looks complete — and a malformed response, which covers a non-JSON body,
an empty body, a runs payload with no numeric `total_count`, and a
GraphQL payload with no `pullRequest` node. A missing `total_count` is
reported as malformed, never defaulted to 0: zero runs is a real verdict
about the PR, and defaulting would manufacture it out of a broken sweep.

24 tests in the style of the existing classifier's. Verified by mutation:
dropping the zero-runs disqualifier fails 2, returning `[]` for an empty
PR list fails 1, swallowing a transport failure fails 2, tolerating a
runs payload with no `total_count` fails 1, disabling stale-run detection
fails 3, and removing the not-ours precedence fails 3.

Wiring: the tests run in CI as a named step in `node-tests` (and were
already covered by that job's `scripts/lib/*.test.mjs` catch-all). The
entry point itself is marked `@unwired-by-design` per the convention
#3071 establishes — every verdict it produces is transient GitHub state
(queued runs, review latency, a base that is DIRTY only until the next
rebase, rate limiting), so a required check built on it would fail for
reasons unrelated to the diff under test.
@louistrue

Copy link
Copy Markdown
Collaborator

Nudging this one, because it is the only PR on the board with no movement and it is now rotting rather than waiting.

State: DIRTY, last commit 2026-08-20, and its CI is stale on two independent axes — runs=5 but shards=0, so those runs predate both the sharded viewer lane and the conflict. Green, real, and computed against a base that no longer exists.

I said this morning I would not close it until #2952 lands, so the visibility work would not be lost. That reasoning was right and the plan attached to it is not working: #2952 still has 6 unresolved Major findings on its head, including the wrapping arithmetic on EXPRESS ids. It is not landing this week, and every day this waits the conflict widens.

The specific thing at risk

merged_visibility.rs (298 lines) and merged_visibility_tests.rs (162). #2952 handles visibility inside mod.rs and plan.rs, and CodeRabbit's mod.rs:195 finding there is about the ordering of exactly that — applying the first model's visibility before building the shared merge targets. So the two are working the same ground from different ends, and the fuller filter here is the piece with no counterpart.

What I would suggest, and it is your call

Extract the visibility filter as its own PR against current main, decoupled from the #2951 allowlist-vs-denylist race entirely. It does not depend on which merged-export implementation wins: whichever one lands, a visibility filter sits on top of it. That turns a rotting 11-file PR into a small one that can be reviewed and merged on its own merits, and it stops the work being hostage to an external contributor's review cycle.

Then close this, and the merged_guid.rs overlap goes with it — #2952's four rootedness Majors are open against exactly that logic, so duplicating it here helps nobody.

If you have already solved rootedness properly in this PR, saying so on #2952 is worth more than either the rebase or the extraction. That is the finding blocking the external PR, and you may have the answer sitting in merged_guid.rs.

Happy to do the extraction myself if you would rather not — say the word. Not touching it otherwise, it is yours.

@louistrue

Copy link
Copy Markdown
Collaborator

Nudge with the specific state, since this one's checks column is misleading in a way that is easy to miss.

This PR is stale on two independent axes at once.

mergeable: CONFLICTING
GitHub Actions runs on head: present
viewer shard jobs: 0

Two separate things, and each alone would be misread:

  1. The runs predate the sharded lane. Node tests was split into four Viewer tests (shard N) jobs when perf(test): run the viewer suite 4-wide, so the Node lane stops timing out #3035 landed. Zero shard jobs means these runs came from a workflow definition that no longer exists, so a green here describes a lane that is not the one your code will actually face.
  2. The runs also predate the conflict. They were computed against a base that has since diverged.

A check for either one alone gets this wrong in a different direction — "it has runs, so it is fine" misses the first, and "the lane changed, so rebase" misses that the base is gone too.

Only you can clear it, because it is conflicting:

git fetch origin main
git merge origin/main     # or rebase
# resolve, then push

update-branch and gh run rerun will both no-op while the branch is CONFLICTING — neither has a merge commit to work with. Once it is resolved, CI runs on the current definition and you get a signal worth reading.

Nothing here is about the change itself.

louistrue added a commit that referenced this pull request Aug 22, 2026
* feat(scripts): tell a real CodeRabbit review from a green tick

A CodeRabbit pass does not mean the diff was reviewed. Three states render
as the same check: reviewed and found nothing, reviewed only the newest
commit, or never ran because the org is rate limited. Under Fair Usage the
third is green on every open PR at once, so 'all our PRs show CodeRabbit
passing' is a symptom rather than reassurance.

Two signals are needed and neither works alone. The rate-limit sentinel
alone gives false positives: a PR can carry it verbatim and still have
genuine inline findings posted minutes later, because the summary comment
is not rewritten when a later pass succeeds. So the sentinel is conclusive
only when the inline thread count is also zero.

Note gh pr view --json comments does not return inline review threads at
all, so the count comes from reviewGraphQL threads; without that every PR
reads as having no findings.

Classification is a pure function with unit tests over synthetic bodies.
Not wired into CI, and should not be: the answer depends on transient
GitHub state, so a required check on it would fail for reasons unrelated
to the diff. Empty target list exits non-zero rather than reporting a
vacuous all-clear.

* fix(scripts): stop the review detector failing in both directions at once

The classifier returned NO-REVIEW whenever the CodeRabbit comment list was
empty, before the inline thread count was consulted. CodeRabbit posts inline
findings while its summary comment is absent, deleted, or rewritten in place,
so three live threads read as "posted no comment at all". Only the absence of
both signals is absence of review.

And no timestamp reached the classifier at all: threads were fetched for their
author, comments for their body, and the input was {bodies, inlineThreadCount}.
A review from two pushes ago therefore certified commits the bot never saw. The
GraphQL query now also returns each thread's first-comment createdAt, each
CodeRabbit review's submittedAt, and the head commit's pushedDate (falling back
to committedDate), and evidence older than its head commit reports
PRE-PUSH-REVIEW rather than REVIEWED.

The two mistakes are not symmetric. A false "not reviewed" is loud and
self-correcting; a false "reviewed" is silent and terminal, because nobody
looks again and the tool has certified the thing it exists to catch. So the
ambiguous cases now resolve to reviewed: false -- a comment with none of the
markers (INCONCLUSIVE), and a timestamp that is missing or unparseable
(UNDATED). The parse returns null rather than NaN on purpose: every comparison
against NaN is false in both directions, so a bare `<` would have reached the
reassuring branch by accident.

Tests go 10 -> 19, covering both directions, both composed, a genuinely
reviewed-at-head PR still reading REVIEWED, and each ambiguous case. The file
was never run by CI; it now has a step.

* docs(scripts): declare the CodeRabbit review check unwired-by-design, machine-readably

The header already explained in prose why this must not gate CI -- its
verdict depends on transient GitHub state -- but prose is not something
#3071's wiring checker can read. That checker accepts exactly one
declaration, an `@unwired-by-design <reason>` line, and reports every other
unreferenced scripts/check-*.mjs as an accidental omission. Say it in the
form the gate reads, so the deliberate exception stays visible and the
genuine omissions stand out.

Comment only. Its classifier tests still run:
  node --test scripts/lib/coderabbit-review-state.test.mjs -> 19 pass, 0 fail

* feat(scripts): sweep every open PR for the four green ticks that are not green

`check-coderabbit-review.mjs` answers one question per PR: did CodeRabbit
read the diff at the head commit. A sweep across all 36 open PRs found
the failure modes it was built for, plus four it does not cover — each of
which renders as green, or as "nothing failing", in any report that
counts only failing and pending checks:

  - `headRepositoryOwner.login` is not ours. Every other column is then a
    report about a branch we cannot push to.
  - `mergeStateStatus == DIRTY` WITH runs present: green checks over a
    merge commit that can no longer be formed. #2970 and #2971 are in
    exactly that state, and both show `fail=0, pending=0`.
  - ZERO workflow runs at the head commit. A PR that was already DIRTY at
    push time never gets a run, so `statusCheckRollup` comes back EMPTY —
    and an empty rollup counts up to the same `fail=0, pending=0` as a
    fully passing one. This is the vacuous-pass shape, so the run count
    is a first-class signal here rather than something derived.
  - The newest run on the BRANCH is against a superseded commit, while
    the rollup still shows that older commit's green.

The sweep that found these was a throwaway script, which makes it a habit
someone has to remember. It lands here as a second entry point,
`scripts/check-pr-green.mjs`, over a new pure module
`scripts/lib/pr-green-sweep.mjs`. `coderabbit-review-state.mjs` is reused
unchanged — the review verdict is one column of the table, not a
reimplementation.

The disqualifier order is worst-first and each earlier reason invalidates
the later ones AS EVIDENCE: a fork's counts are not ours to read, a DIRTY
base means the green ran on a merge that cannot be formed, zero runs
means the counts are empty rather than passing, a stale run means they
describe a commit that is no longer the head. `not ours` takes precedence
as a disqualifier but ranks LOWEST for sorting, because it is the one
verdict that is not a task; both halves of that deliberate disagreement
are pinned by tests.

It must not pass vacuously, so three cases are hard failures with their
own `kind` and their own message rather than a shorter, cleaner report:
zero PRs returned (what a wrong `--author`, a wrong repo and a truncated
response all look like), an unreachable API — including mid-sweep, where
a swallowed per-PR failure would drop a row from a report that still
looks complete — and a malformed response, which covers a non-JSON body,
an empty body, a runs payload with no numeric `total_count`, and a
GraphQL payload with no `pullRequest` node. A missing `total_count` is
reported as malformed, never defaulted to 0: zero runs is a real verdict
about the PR, and defaulting would manufacture it out of a broken sweep.

24 tests in the style of the existing classifier's. Verified by mutation:
dropping the zero-runs disqualifier fails 2, returning `[]` for an empty
PR list fails 1, swallowing a transport failure fails 2, tolerating a
runs payload with no `total_count` fails 1, disabling stale-run detection
fails 3, and removing the not-ours precedence fails 3.

Wiring: the tests run in CI as a named step in `node-tests` (and were
already covered by that job's `scripts/lib/*.test.mjs` catch-all). The
entry point itself is marked `@unwired-by-design` per the convention
#3071 establishes — every verdict it produces is transient GitHub state
(queued runs, review latency, a base that is DIRTY only until the next
rebase, rate limiting), so a required check built on it would fail for
reasons unrelated to the diff under test.

---------

Co-authored-by: Louis Trümpler <78563314+louistrue@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native (Rust) merged/federated IFC export at parity with the JS MergedExporter

2 participants