fix(data,export,parser): one space per control character, and pin the quoted-keyword header shapes (#3284)

[BIMvoice]

Refs #3284.

Stacked on #3279 (fix/schema-detect-file-schema-3278), not on main — the two touch the same file. Same arrangement as #3261 on #3255. Please merge #3279 first; the diff against main will then be exactly what is below.

Item 1 — the quote-unaware header searches

Reproduced on upstream/main @ 8dd8a9db1 by running main's parseSourceHeader and #3279's side by side over the same bytes (the two shapes from the issue, unchanged):

MAIN 1a: undefined
MAIN 1b: {"description":["per the FILE_NAME convention"],"implementationLevel":"2;1",
          "author":[],"organization":[],"schemaIdentifiers":["IFC4"]}
HEAD 1a: {"description":["note: the ENDSEC; marker is described here"],...,"name":"a.ifc",
          "author":["Jane"],...,"schemaIdentifiers":["IFC4"]}
HEAD 1b: {... full header ...}

So the searches themselves are already fixed on this branch's base: #3279 landed indexOfRecord (used for both ENDSEC and each FILE_* keyword) while this was being written. That closes the connection the issue's follow-up comment describes — detectSchemaVersion no longer falls back to the raw byte scan for these two header shapes, so #3279 is complete rather than holed.

What this PR adds is the coverage #3279 does not have. Its tests assert the detected schema, and in shape 1b the schema was never wrong: FILE_SCHEMA parsed fine while every FILE_NAME field was silently empty. A header can carry the right schemaIdentifiers and have lost its author, timestamp and originating system — the provenance erasure #3282 is about, reached from the other door. packages/parser/test/source-header-quoted-keywords.test.ts asserts all ten fields, by name.

Mutation check (the assertion can fail): reverting the two call sites to main's indexOf reds exactly the two hostile cases and nothing else —

× a quoted ENDSEC does not truncate the header away (#3284 item 1a)
    AssertionError: expected undefined to be defined
× a quoted FILE_NAME does not shadow the real record (#3284 item 1b)
    AssertionError: expected undefined to be 'a.ifc'
✓ parses every declared field from a plain header
✓ still stops at the real ENDSEC and never reads the DATA section
✓ returns undefined for input with no header records at all

restored by the inverse edit, diff byte-identical.

The negative controls matter here: the plain-description case pins that the fix did not break ordinary headers, the DATA-section case pins the other direction (quote-awareness must not switch the terminator off — a FILE_NAME/FILE_SCHEMA planted after ENDSEC must not win), and the non-STEP case pins that undefined still means "no header", not "the scan tripped".

Item 2 — the escapers disagree on a run of control characters

packages/data/src/step-serializers.ts and packages/export/src/step-serialization.ts both used /[\x00-\x1F\x7F]+/g — note the + — collapsing a whole run to one space. ifc_lite_export::step_text::escape maps each control character to its own space. Each escaper's doc comment claims it matches the other; one of them had to move.

The expectations are derived from the Rust half, not written by hand: a throwaway #[test] printed escape()'s output over six inputs and was then removed (rust/export/src/step_text_tests.rs is byte-identical, diff-proven, and belongs to #3283 anyway):

RUST tab run   -> "a   b"
RUST crlf      -> "a  b"
RUST mixed     -> "a    b"      // NUL, VT, US, DEL
RUST single    -> "a b"
RUST quote+run -> "O''Brien  x"
RUST plain     -> "plain text 123"

Those six vectors are now the TS tests in both packages. ISO 10303-21 6.3.3.4 mandates neither behaviour (it only bars the control byte from a literal), so the tie is broken by the parity claim and by information loss: collapsing discards the run's length. The TypeScript halves move.

Mutation check: putting the + back reds 9 tests in @ifc-lite/data and 5 in @ifc-lite/export, with the run length named in the message —

AssertionError: expected '\'a b\'' to be '\'a   b\''
AssertionError: expected 'O\'\'Brien x' to be 'O\'\'Brien  x'
AssertionError: expected 'FILE_NAME(\'output.ifc\',\'TS\',(\'a …' to contain '(\'a   b\')'

restored by the inverse edit, diff byte-identical in both files. 'plain text 123' is the negative control (byte-identical passthrough), and the length loop asserts both directions at once: same length as the input and no surviving control byte — a run left intact would also keep its length, so neither half alone would have failed the old collapse.

Honest severity

Unchanged from the issue: both shapes are constructed. No real authoring tool is known to emit either, and nothing here is a reproduced field failure. Item 1's searches are robustness that turned out to be load-bearing for #3279; item 2 is a real, observed disagreement between two halves that each document themselves as matching the other.

Sweep

• /[\x00-\x1F\x7F]+/g — the only two occurrences in the repo, both fixed here.
• Non-quote-aware header locators in TS — one other site, packages/cli/src/commands/extract-entities.ts's text.indexOf('DATA;'). Same shape, different (and much narrower) blast radius: the scan that follows it is quote-aware and header lines carry no #. Left alone deliberately rather than widening this PR; noted here so it is on the record.

Verification

@ifc-lite/data:   205 tests passed (16 files)
@ifc-lite/export: 886 passed | 30 skipped (67 files)
@ifc-lite/parser: 773 passed | 2 skipped (77 files)
pnpm typecheck (data, export, parser): 11 tasks successful
pnpm lint: 3,662 files across 4 targets, 98 rules, no errors

🤖 Generated with Claude Code

https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ad71890-5157-481e-821e-bc9358a54c1d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[louistrue]

Heads up that #3297 and this PR overlapped, and I have removed the overlap from mine rather than have two changes doing the same thing.

Both of us fixed the control-character run in packages/data/src/step-serializers.ts and packages/export/src/step-serialization.ts, same direction, same reasoning. You were first by about twenty minutes and you are the issue's author, so this is the one that should land. I have reverted both files on my branch to main, so there is nothing left to conflict.

Your version is also the better test. Mine pinned the literal "a b"; yours asserts the length relation at every run length, which is a property rather than an example and cannot pass by coincidence.

Two things I kept, because they do not overlap:

A Rust test pinning the run length on the third half. rust/export/src/step_text_tests.rs. The parity claim had a hole neither of our PRs closed: escape_maps_every_ascii_control_char_to_a_space feeds one control character at a time, and per-character and collapse-a-run give the same answer for a run of one, so nothing on the Rust side ever asserted the rule the TypeScript side is being made to match. Mutating step_text::escape to skip a push when the output already ends in a space leaves the existing test green and reddens only the new one.

The header scan, which is a different defect. #3297 is now just source-header.ts and its Rust mirror. It turns out indexOfRecord landing via #3279 fixed the quoted-keyword half but left comments: an apostrophe in /* John's export */ inverts quote state for the rest of the file, a comment between a keyword and its ( drops that record, and a comma inside a comment shifts every later field along.

One note relevant to your PR's framing. You wrote that #3279 is "complete rather than holed" for these two header shapes, and that is right for the shapes in the issue. It is not right in general: the same parse_source_header returning None on a commented header makes export_step fall back to its own defaults, so the source file's author, organization and authorization get replaced with ifc-lite. Same provenance erasure as #3282, reached through a third door. That is fixed in #3297 now, both halves, so it is not a gap in yours.

[BIMvoice]

Adversarial review of 0126e82c1. I verified RED on both halves myself and mutation-tested the parser pin.

What I ran

Premise first — the whole PR rests on Rust being one-space-per-char. rust/export/src/step_text.rs:44-66:

for c in s.chars() {
    match c {
        ...
        '\0'..='\u{1F}' | '\u{7F}' => out.push(' '),

Per character, no run collapsing. The premise is correct and the fix direction (TS follows Rust) is right.

GREEN — pnpm exec turbo run test --filter=@ifc-lite/data --filter=@ifc-lite/export:

@ifc-lite/data:test     Test Files  16 passed (16)     Tests  205 passed (205)
@ifc-lite/export:test   Test Files  65 passed | 2 skipped (67)   Tests 886 passed | 30 skipped (916)

No displaced expectation anywhere — nothing in either package depended on the collapsed form.

RED, independently — restored + in both escapers (packages/data/src/step-serializers.ts:205, packages/export/src/step-serialization.ts:151), keeping the tests:

@ifc-lite/data     Tests  9 failed | 196 passed (205)
   AssertionError: expected '\'a b\'' to be '\'a   b\''
   AssertionError: expected '\'O\'\'Brien x\'' to be '\'O\'\'Brien  x\''
@ifc-lite/export   Tests  5 failed | 881 passed (916)
   AssertionError: expected 'a b' to be 'a   b'

Both negative controls (single control char, no control chars) stayed green in both suites, which is what makes the 9/5 meaningful. Restored by copying back the pre-mutation files; git status clean.

The changeset claim — "Both TS escapers":

$ grep -rn 'x00-\\x1F' packages apps | grep -v node_modules | grep -v /dist/
packages/export/src/step-serialization.ts:151
packages/data/src/step-serializers.ts:205

Exactly two. Accurate. Changeset starts with ---, no licence header, both affected published packages listed at patch. Correct.

---

1. This collides with #3297 — the same one-character source change, in the same two files

#3297 (Refs #3284, open, based on main) contains:

-    .replace(/[\x00-\x1F\x7F]+/g, ' '); // Collapse control chars
+    .replace(/[\x00-\x1F\x7F]/g, ' ');  // One space PER control char
-    .replace(/[\x00-\x1F\x7F]+/g, ' ');
+    .replace(/[\x00-\x1F\x7F]/g, ' ');

Byte-identical to this PR's source change, in packages/data/src/step-serializers.ts and packages/export/src/step-serialization.ts. It also carries its own escaper-parity tests (packages/export/src/step-escaper-parity.test.ts) and its own packages/parser/test/source-header.test.ts (+101) over #3284 item 1 — the same ground as this PR's source-header-quoted-keywords.test.ts (+173). Two open PRs implementing one issue in the same files; whichever lands second will conflict. Flagging for a merge-order decision rather than as a defect in either.

2. The base branch is merged, so this needs retargeting — and item 1 is already fixed on main

base=fix/schema-detect-file-schema-3278 — #3279 is MERGED. And the quote-aware scan this PR's parser test pins is already on main:

$ git show upstream/main:packages/parser/src/source-header.ts | grep -n indexOfRecord
141:function indexOfRecord(upper: string, keyword: string, fromIndex = 0): number {

So source-header-quoted-keywords.test.ts is a characterisation pin over shipped behaviour, not a RED-then-GREEN fix. The title says "pin the quoted-keyword header shapes", which is honest — no overclaim — but it means the file has to be judged on whether it is capable of failing. I checked, by replacing the body of indexOfRecord with return upper.indexOf(keyword, fromIndex):

✓ parses every declared field from a plain header
× a quoted ENDSEC does not truncate the header away (#3284 item 1a)      expected undefined to be defined
× a quoted FILE_NAME does not shadow the real record (#3284 item 1b)     expected undefined to be 'a.ifc'
✓ a quoted FILE_DESCRIPTION does not shadow the real record either
✓ still stops at the real ENDSEC and never reads the DATA section
✓ returns undefined for input with no header records at all
Tests  2 failed | 4 passed (6)

Restored, byte-identical, tree clean. Two of the six cases are load-bearing. That is a real pin.

3. Minor: the FILE_DESCRIPTION case cannot detect the shadowing it says it detects

From the run above, 'a quoted FILE_DESCRIPTION does not shadow the real record either' stays green under a plain indexOf. Its own comment claims it covers "the third keyword the same scan looks up, and the one whose shadowing would be self-concealing" — but in headerBytes() the quoted FILE_DESCRIPTION( text sits inside the real FILE_DESCRIPTION record, i.e. always after it, so a naive left-to-right indexOf finds the genuine record first and the case passes either way. It is a fine regression pin for the escaping of the description text; it is not evidence about record shadowing. Either soften the comment, or move the decoy so it can actually win — e.g. put the quoted FILE_DESCRIPTION(('x'),'2;1') text inside the FILE_NAME record, which precedes nothing that would rescue it.

Not blocking; the two cases that matter do fail.

---

Cleared: the Rust reference vectors (all six checked against step_text::escape's per-char branch), the ordering constraint (control-char replacement before the \X2\ directive encoding, matching Rust's single match), the keeps the run length … at every length two-direction assertion (length and absence of control bytes — neither alone would catch the collapse), the REQUIRED_FIELDS named-field anti-vacuity in the header test, and the never reads the DATA section inverse case with the planted FILE_NAME('planted.ifc', …, ('Mallory'), …), which is exactly the right shape for "quote-awareness must not turn the terminator off".

[louistrue]

Not merging this: it reads green because almost nothing ran, not because it passed.

I came here to merge it — it is MERGEABLE, has no failing checks, and I had just confirmed it carries the one half of #3284 still outstanding on main. Then I counted the check runs:

#3294   4 check runs
#3298  26 check runs
#3306  34 check runs

No Typecheck, no Lint, no Node tests, none of the four viewer shards, and no Build + WASM + Rust + Node — the aggregate that branch protection actually gates on. The two real entries are parity and a Vercel preview comment.

The mechanism, from the timeline: this PR was retargeted (base_ref_changed) at 07:29:03, and its last commit is 04:44:23. It was stacked on fix/schema-detect-file-schema-3278 and moved to main after that base landed. GitHub does not re-run the checks on a retarget, so what is showing is whatever ran against the old base — and against a base that no longer exists, most of it never ran at all.

So "no failing checks" here is the absent-CI-reads-as-green shape: the rollup cannot distinguish "passed" from "never ran", and mergeable is about textual conflicts, not about tests.

To clear it, the head needs to move so the workflows fire against main — an empty commit (git commit --allow-empty) or a rebase onto current main will do it. I have not pushed to your branch.

The content is right, for what it is worth, and I checked before writing this off: main still carries /[\x00-\x1F\x7F]+/g in both TS escapers (zero hits for the per-character form), so "a\t\t\tb" still serialises as 'a b' here and 'a b' in Rust. This PR fixes exactly that, in both escapers. I have added Closes #3284 so it closes the issue when it lands — it was carrying no closing keyword.

Also worth knowing: the other half of #3284 is already done. indexOfRecord landed via #3279, and I ran the issue's own quoted-ENDSEC fixture against main — the header parses and the description survives, where it previously returned undefined. So this PR is the remainder, not a duplicate of that.

On the overlap with #3297: that one is CONFLICTING with Node tests and the aggregate red, head unchanged since 05:08, and its central addition indexOfOutsideQuotes does the job main's indexOfRecord now does. I have said on #3284 that this PR looks like the survivor, but that is a recommendation for its author to weigh, not a decision I am making for them.

[louistrue]

Sharper cause than I gave before, and it generalises past this PR.

test.yml never ran on this PR at all — not once, at any head. Only one workflow has ever been created for 0126e82c:

IfcOpenShell parity   completed/success

test.yml declares on: pull_request: branches: [main]. This PR was opened at 04:45 against fix/schema-detect-file-schema-3278, so the trigger did not match and the workflow was never created. Retargeting to main at 07:29 does not fire it retroactively — GitHub evaluates the trigger when the event happens, not when the base changes.

So it is not "CI did not re-run". It is that the required lane has never existed for this branch, and the rollup has nothing to report a failure from.

The general shape, which is worth someone knowing: any PR stacked on a feature branch gets no test.yml, and retargeting it to main leaves it that way. It then presents as MERGEABLE with no failing checks — indistinguishable from green at a glance. #3246 was in the same state earlier today and only got real CI once its base landed and it was retargeted before a push.

The remedy is unchanged: the head has to move. git commit --allow-empty -m "retrigger CI on main" and push, or a rebase onto current main. I have not pushed to your branch.

The content still looks right — main carries /[\x00-\x1F\x7F]+/g in both TS escapers (zero hits for the per-character form), so "a\t\t\tb" is 'a b' here and 'a b' in Rust, and this fixes exactly that. Closes #3284 is on the body.

[louistrue]

Do not merge this yet. It reads MERGEABLE with zero failures, and it has never been compiled, typechecked, linted or tested.

This PR carries 8 checks. A healthy PR on this repo carries 38. The missing lanes:

Typecheck                     Lint
Node tests                    Rust tests
Build packages + WASM         Build + WASM + Rust + Node   (the required aggregate)
Viewer tests (shards 0-3)     Viewer E2E smoke
Rust crate semver             Detect changes
... 30 lanes in total

What it does have is parity, three Vercel deployments, Vercel Agent Review, CodeRabbit and Vercel Preview Comments. None of those builds the code.

Cause: test.yml never fired. The PR was opened against a feature branch, and retargeting it to main does not fire workflows retroactively. So the checks that exist are the ones that ran against the original base, and the whole test matrix simply never started.

Why no failure count catches this. The required aggregate is not red, it is absent. fail=0 is literally true, and so is mergeable. Anything asking "is anything failing?" gets the answer "no" and is correct — it is just answering a different question from "has this been tested?".

The fix is one push of any kind — an empty commit, a rebase onto current main, anything creating a new head event now that the base is main. That fires the full matrix. I have not pushed it myself; it is your branch.

Worth doing before that push, since the base changed: this has never been merged against current main, which has moved a long way today (#3298's semver gate, #3306's sharded module-size digest, and eleven other merges). A rebase rather than an empty commit will tell you whether it still applies, not just whether it compiles in isolation.

#3185 has the identical shape — 6 checks, same missing set — but it is CONFLICTING so it cannot merge by accident. Same treatment when it is resolved.

For anyone sweeping the board: count the checks before reading their status. gh pr view <n> --json statusCheckRollup --jq '.statusCheckRollup|length'. Under ~15 on a non-fork PR means a workflow never ran, and no amount of reading the green ones reveals it.

[BIMvoice]

Checked the three findings the bots raised on #3297 (the conflicting duplicate of this PR) against this branch. None of the three applies here. A fourth problem does, and it is now fixed: 87370e6c0e3fe2145c3e746feff85df90e215723.

The three #3297 findings, against this branch

1. Missing @ifc-lite/export in the changeset (Codex, CodeRabbit) — does not apply. #3297 puts the escaper change in a single changeset listing only parser and data. This PR splits it in two, and the one covering packages/export/src/step-serialization.ts already declares export:

--- .changeset/hungry-pugs-sneeze.md
'@ifc-lite/export': patch
'@ifc-lite/data': patch

.changeset/tidy-schemas-declare.md covers @ifc-lite/parser for source-header.ts / columnar-parser.ts. Every changed packages/* path is attributed.

2. Source-text assertion at source-header.test.ts:516 (CodeRabbit) — does not apply. git diff upstream/main...HEAD --name-only does not list packages/parser/test/source-header.test.ts; the file is byte-identical to main here and is 332 lines long, so there is no line 516. The new test files (source-header-quoted-keywords.test.ts, schema-detection.test.ts) assert on parsed results (header?.name, parseSourceHeader(...)), never on fixture text. node scripts/check-source-text-assertions.mjs → OK (8 allowlisted, 0 marked, 0 new).

3. Wrong license header in rust/export/src/schema_detect.rs (CodeRabbit) — does not apply. This PR touches no Rust at all; git diff upstream/main...HEAD --name-only | grep rust/ is empty. There is no schema_detect.rs. The four new/changed TypeScript files all carry the block MPL notice, which is the TS convention here (matching packages/parser/src/source-header.ts and packages/export/src/step-serialization.ts); the SPDX-only form the finding asks for is the rust/export/src convention, which nothing here is under.

A fourth finding that does apply — check-module-size fails

Given that test.yml has never run on this PR at any head, I ran the cheap gates and the affected suites locally. One gate fails on this branch and passes on main:

Allowlisted file(s) grew PAST their recorded budget. Shrink or split instead of
raising the budget:

  packages/data/src/step-serializers.ts: 465 lines, budget 459

node scripts/check-module-size.mjs exits 1 here, exits 0 on upstream/main. It runs in test.yml line 636, so it would have fired the first time that workflow ran.

The escaper change itself was one line; the +6 was all doc comment. Fixed by tightening that comment back to its original five lines, keeping the fact it records — the count is per control character, and the + this carried until #3284 wrote "a\t\t\tb" as 'a b' where the export escaper and ifc_lite_export::step_text::escape write 'a b'. The budget is not raised.

Verification after the fix

check	result
check-module-size	OK (1953 files, 310 allowlisted, 0 new over 400)
check-test-wiring	OK (47 packages, 41 gate scripts)
check-test-glob-coverage	OK (47 packages, 0 unrun test files)
check-source-text-assertions	OK (8 allowlisted, 0 marked, 0 new)
@ifc-lite/data	205 passed (16 files)
@ifc-lite/parser — source-header-quoted-keywords, schema-detection, source-header	50 passed (3 files)
@ifc-lite/export — step-serialization	33 passed

These are local runs, not CI. They are not a substitute for test.yml actually running on this head.