fix(lists): stop duplicating the trigger class in prose, which reds the CSV gate - #3107
Merged
Merged
Conversation
…he CSV gate
`main` has been red since 22:47 on exactly one violation, and it is prose:
packages/lists/src/engine.rfc4180.test.ts:396 [formula-trigger character class]
// is not a formula to an anchored `/^[=+\-@\t\r]/` but it is one to Excel.
The comment embedded a literal copy of the anchored trigger class. #3102 added
`check-csv-escaper-copies.mjs`, whose whole purpose is to stop that class being
copied, and it does not distinguish a copy in code from a copy in a comment.
Both PRs were green alone and red together: the comment landed via #3100 after
#3102 branched.
Now describes the guard instead of restating its pattern. A comment copy goes
stale exactly like a code copy, so the gate is right to object even though the
line is prose.
Verified rather than assumed:
before exit=1, 1 violation
after exit=0, 4255 files scanned, 1 known outstanding
(packages/lists/src/engine.ts, so the ratchet is intact and this
is a real pass rather than the gate going quiet)
Deliberately two lines and no more. Three earlier drafts of this comment were
rejected in review, and the failure mode was the same each time: a replacement
that asserted something extra and unverified. One claimed `csv-cell.ts` held the
"single surviving definition" of the trigger class, which the gate output quoted
in this very message contradicts. One pointed a future CSV writer at
`guardSpreadsheetFormula` when `csv-cell.ts` says CSV writers want
`escapeCsvCell`. One described the XLSX container as quoting when it
XML-escapes.
The last draft was accurate and still wrong to ship: it restated the #1772
numeric-exemption divergence that is already documented 55 lines below in this
file and, more completely, in the gate's own KNOWN_REMAINING block. Three homes
for one fact is how the next stale comment gets written.
So this keeps only what the round-trip invariant needs and adds no pointer, no
cross-reference and no second copy of anything.
check-source-text-assertions passes. No changeset: that gate validates that
existing changesets name workspace members and does not require one, and a
comment in a test file changes no behaviour or public API.
Teaching the gate to skip comments is the better long-term fix and is being
worked separately. Six attempts have each been found holed by review, so it
wants an unhurried review on a green main rather than another round under time
pressure.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_48c21298-1242-434d-825d-62e871e93a40) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. 📝 WalkthroughWalkthroughThe pull request updates two lines in the RFC 4180 engine test documentation. The text clarifies how invisible prefixes bypassed the former formula trigger check while Excel still recognized the resulting value as a formula. ChangesFormula injection documentation
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This PR only clarifies a test comment and does not change product behavior or public APIs; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Contributor
Viewer benchmark✅ No threshold regressions detected. 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
louistrue
added a commit
that referenced
this pull request
Aug 23, 2026
Rebase and reduce, per review. This PR predates #3102, which unified the CSV escapers and added a gate against the eleventh. Its CSV half is now redundant: packages/sdk/src/csv-escape.ts:50 /^[\p{Cf}\p{Z}]*[=+\-@\t\r]/u packages/export/src/csv-cell.ts:49 /^[\p{Cf}\p{Z}]*[=+\-@\t\r]/u Byte-identical. Zero security delta, and all five call sites this PR rewires already delegate to the canonical escaper on main -- which is why all five were the merge conflicts. Keeping it would add an eleventh implementation and fail check:csv-escapers. Dropped: csv-escape.ts, its test, the SDK index export, the api-surface entries, the five call-site edits, and the CSV changeset. Kept, because main has no equivalent: the 10 km coordinate ceiling as one exported constant (three unlinked viewer copies of the literal, all of which could be raised 25x with the whole 5751-test viewer suite staying green), and the seven collab role-gate holes where a viewer-role participant could delete a property set, reclass an entity or split a wall -- committing locally, dirtying the model, entering undo, and never reaching the room. Salvaged: the two end-to-end injection tests, which main has no equivalent of. They drive a real `\X2\FEFF\X0\=HYPERLINK` payload through both CLI column branches and through the adapter. Neither imported the dropped module -- they only named it in comments, now repointed at @ifc-lite/export. Three comments reworded to stop quoting the trigger class literally: describing the historical bare guard tripped the gate, the same false red #3107 fixed. csv gate exit 0, 4279 files, 1 known outstanding.
louistrue
added a commit
that referenced
this pull request
Aug 23, 2026
…ding to live config #3107 reworded `engine.rfc4180.test.ts:396` so it no longer quotes the trigger class. Its PROSE_MENTIONS entry went stale and the ratchet fired, which is the safe direction and exactly what it is for. Deleting the entry turned the gate green and took 7 tests red. They destructured the live registry by position: const [MENTION, ENGINE_MENTION] = PROSE_MENTIONS; so removing entry 0 shifted every fixture. The suite that proves the ratchet works was coupled to the order of the list the ratchet exists to change, and it broke for a reason unrelated to the behaviour it covers. `scanRepo` now takes the registry as a parameter, so the behavioural cases build their own fixtures. The real registry is still exercised: `validateMentions()` runs against it at import, and the regex-source case iterates it. Mutation-tested rather than reasoned about. Excusal made file-blind: 45/1. Stale ratchet disabled: 45/1. Control: 46/46.
louistrue
added a commit
that referenced
this pull request
Aug 23, 2026
`const [ENGINE_MENTION] = PROSE_MENTIONS` binds to index 0. The registry is a ratchet whose entire job is to change, so a positional bind re-points on any edit -- which is how the whole prose-mentions block broke when #3107's reword made an entry stale and it was deleted. Now `find(m => m.file === KNOWN_REMAINING[0])` with an assert, so the fixture says WHICH entry it wants rather than where it sits, and a miss is one named failure. Deliberately NOT claiming this prevents a silent failure. A prepended decoy reds the positional form too, 6 tests, and the probe that suggested otherwise was confounded: the decoy named a file absent from the fixture map, so both arms were measuring the stale-mention ratchet, not the binding. The honest claim is one named assertion instead of six cascading ones, plus a bind that survives reordering. 46/46.
louistrue
added a commit
that referenced
this pull request
Aug 23, 2026
…ding to live config #3107 reworded `engine.rfc4180.test.ts:396` so it no longer quotes the trigger class. Its PROSE_MENTIONS entry went stale and the ratchet fired, which is the safe direction and exactly what it is for. Deleting the entry turned the gate green and took 7 tests red. They destructured the live registry by position: const [MENTION, ENGINE_MENTION] = PROSE_MENTIONS; so removing entry 0 shifted every fixture. The suite that proves the ratchet works was coupled to the order of the list the ratchet exists to change, and it broke for a reason unrelated to the behaviour it covers. `scanRepo` now takes the registry as a parameter, so the behavioural cases build their own fixtures. The real registry is still exercised: `validateMentions()` runs against it at import, and the regex-source case iterates it. Mutation-tested rather than reasoned about. Excusal made file-blind: 45/1. Stale ratchet disabled: 45/1. Control: 46/46.
louistrue
added a commit
that referenced
this pull request
Aug 23, 2026
`const [ENGINE_MENTION] = PROSE_MENTIONS` binds to index 0. The registry is a ratchet whose entire job is to change, so a positional bind re-points on any edit -- which is how the whole prose-mentions block broke when #3107's reword made an entry stale and it was deleted. Now `find(m => m.file === KNOWN_REMAINING[0])` with an assert, so the fixture says WHICH entry it wants rather than where it sits, and a miss is one named failure. Deliberately NOT claiming this prevents a silent failure. A prepended decoy reds the positional form too, 6 tests, and the probe that suggested otherwise was confounded: the decoy named a file absent from the fixture map, so both arms were measuring the stale-mention ratchet, not the binding. The honest claim is one named assertion instead of six cascading ones, plus a bind that survives reordering. 46/46.
louistrue
added a commit
that referenced
this pull request
Aug 23, 2026
…ttern (#3106) * fix(scripts): scan code, not comments, in the CSV escaper gate `main` is red on `packages/lists/src/engine.rfc4180.test.ts:396`, a `//` comment describing the anchored regex the gate hunts for. A pair: that file came from #3100, which merged after #3102 branched. FIVE attempts. Four leading-character heuristics, each of which shipped a proven false negative, and then a tokeniser that lost string phase: skip //, * and /* missed Rust `*out = matches!(..)` and `/* c */ code` skip // only missed `/* \n // */ code` -- `*/` resumes the line track block state missed code after `const s = "/* x";` plus a star-space missed `* quote(cells) {}`, a live JS generator method tokenise an unpaired `'` opened a fake string to the next quote ANYWHERE in the file Every one was proven by planting the escaper and EXECUTING it, and every one was justified by a safety claim I had not checked. **All four heuristics passed the same 17 tests, unchanged.** That is the real defect: a suite scoring a fixed gate and a bypassed gate identically has no information in it, and CI, two sessions and several reviewers were all reading it. This version walks the text once, tracking line comments, block comments, the three string flavours and regex literals, with two properties the earlier ones lacked: **Per-line resync.** `'` and `"` cannot span a line in JS, TS or ordinary Rust, so they stop at the newline. Without that bound a Rust lifetime `&'a str` or a JSX contraction `What's New` -- both odd numbers of quotes -- opened a fake string that ran to the next quote anywhere in the file. Measured on this tree: without the bound 76 files, 1120 whole-line comments left unblanked with it 0 files, 0 Those 1120 were false reds waiting for someone to document the pattern in one of them. Phase loss also inverts: a real string's contents get read as code, so a `//` in a URL blanks live code and hides an escaper. Both directions reproduced. **Keyword-aware regex detection.** `return /["]/.test(s)` is a regex, not division; without the keyword arm the quote inside it shifted phase. The safety property, stated because the previous five rested on unstated ones: **only comment content is blanked.** Strings and regexes are skipped over, never written to. So every remaining way this can be wrong about where a string ends costs at most a missed comment -- a false red -- and cannot hide a line of code, because hiding requires writing spaces and that happens only inside a proven comment run. **22 tests added and the suite now rejects every prior attempt:** main as merged 14 of 39 fail attempt 2 7 fail attempt 3 8 fail attempt 4 5 fail attempt 5 6 fail this 0 fail The last row of that table is the one that took a second review to earn. The first 15 cases were all drawn from the four heuristics' failure shapes, so none of them could catch a tokeniser that loses string phase -- a suite rejecting the designs you already replaced is evidence about those, not about the one you shipped. Seven more cases target this design: Rust lifetimes, JSX contractions, raw strings with odd inner quotes, keyword-preceded regexes, and both bypass directions. Must-be-caught cases are PAIRED -- the escaper asserted bare as well as in context -- so a probe that cannot match reports as a broken probe rather than a pass. Both verification failures tonight, a probe outside `git ls-files` and one outside the pattern set, print exactly like a clean result without that. Full scan exits 0. Ratchet unaffected: `packages/lists/src/engine.ts` stays live on its two CODE matches at 790 and 796, so `staleKnown` stays empty. * fix(scripts): scan raw, excuse prose by exact registered line Six attempts at comment-awareness each hid a live escaper. Delete the tokeniser entirely: every line of every file is grepped raw and stateless, so no context -- string, template, lifetime, nested comment -- can hide a match. Prose quoting a pattern is excused one exact line at a time via PROSE_MENTIONS, ratcheted like KNOWN_REMAINING and validated so a registered line cannot execute. KNOWN_REMAINING liveness now counts only non-prose hits, so paying the engine.ts debt trips the ratchet even if its history comment stays. Also widen the trigger-class pattern to catch the string-spelled new RegExp form, which every prior version missed. * fix(scripts): drop the mention #3107 reworded, and stop the tests binding to live config #3107 reworded `engine.rfc4180.test.ts:396` so it no longer quotes the trigger class. Its PROSE_MENTIONS entry went stale and the ratchet fired, which is the safe direction and exactly what it is for. Deleting the entry turned the gate green and took 7 tests red. They destructured the live registry by position: const [MENTION, ENGINE_MENTION] = PROSE_MENTIONS; so removing entry 0 shifted every fixture. The suite that proves the ratchet works was coupled to the order of the list the ratchet exists to change, and it broke for a reason unrelated to the behaviour it covers. `scanRepo` now takes the registry as a parameter, so the behavioural cases build their own fixtures. The real registry is still exercised: `validateMentions()` runs against it at import, and the regex-source case iterates it. Mutation-tested rather than reasoned about. Excusal made file-blind: 45/1. Stale ratchet disabled: 45/1. Control: 46/46. * test(scripts): bind ENGINE_MENTION by file, not by registry position `const [ENGINE_MENTION] = PROSE_MENTIONS` binds to index 0. The registry is a ratchet whose entire job is to change, so a positional bind re-points on any edit -- which is how the whole prose-mentions block broke when #3107's reword made an entry stale and it was deleted. Now `find(m => m.file === KNOWN_REMAINING[0])` with an assert, so the fixture says WHICH entry it wants rather than where it sits, and a miss is one named failure. Deliberately NOT claiming this prevents a silent failure. A prepended decoy reds the positional form too, 6 tests, and the probe that suggested otherwise was confounded: the decoy named a file absent from the fixture map, so both arms were measuring the stale-mention ratchet, not the binding. The honest claim is one named assertion instead of six cascading ones, plus a bind that survives reordering. 46/46. * test(scripts): silence no-template-curly-in-string on a fixture that must keep its ${ The Lint lane went red on one oxlint error, mine: scripts/check-csv-escaper-copies.test.mjs:229 eslint(no-template-curly-in-string) `ENGINE_CODE` is a FIXTURE, not code. It is what `engine.ts` looks like TO THE SCANNER, so the `${` has to survive verbatim into the scanned text. Rewriting it to satisfy the rule would change what the test feeds the gate. The rule is right about the general case and wrong about a fixture whose whole job is to be the source text of something else, so it is suppressed on that one line with the reason stated rather than the fixture obfuscated. before Found 21 warnings and 1 error after Found 21 warnings and 0 errors (matches origin/main exactly) Three false starts getting here, worth recording because each cost a cycle: - `oxlint --deny-warnings` is NOT what CI runs. check-lint-ran.mjs invokes oxlint with `--config .oxlintrc.json --format default` and no deny flag, so warnings are not fatal. Adding the flag manufactured a failure that does not exist and pointed at pre-existing moonshot debt that was never the cause. - `check-unused-locals.mjs` needs the BUILT dist. Run without one it reports "these packages do not compile standalone" and exits 1, which reads exactly like a finding about main. Main is lint-green; that was my environment. - `oxlint-disable-next-line` disables the NEXT line. The offending string was the second line of a concatenation, so a comment above the `const` covered the wrong one and the error survived, unchanged, looking like the suppression had failed for some deeper reason. 46/46. Gate exit 0. * test(scripts): pair ENGINE_CODE with a probe, so its six cases cannot go vacuous Pre-flight found the gap and called it deferrable as pre-existing. Fixing it here instead, because it is exactly the defect class this PR exists to close and shipping it inside this change would be incoherent. Six cases feed ENGINE_CODE in as engine.ts's body and then assert something about the OTHER file, so an inert ENGINE_CODE leaves every one of them green. Measured, not assumed: neutering it to `return str;` passed 46/46. Its siblings at :252 and :263 already carry the paired probe idiom ("probe cannot match; result meaningless"). ENGINE_CODE had none. With the probe, the same mutation reds with a named message: before 46/46 pass, mutation invisible after the block fails: "ENGINE_CODE no longer reads as an escaper; every case using it is vacuous" control 46/46 The assert sits at describe-body level, so it aborts the whole block rather than failing one case. That is deliberate: every case in the block is meaningless once the fixture stops being an escaper, so failing them individually would report six mysteries instead of one cause. * test(scripts): move both fixture guards into before(), so a failure exits non-zero The probe I added one commit ago printed but did not fail. On node 22 -- which is what CI runs -- a throw from a describe BODY is recorded as a failed suite and `node --test` still exits 0. Measured on v22.14.0: describe-body assert -> exit 0 before() hook assert -> exit 1 So the previous commit's stated purpose was not achieved. It turned "the mutation is invisible" into "the mutation prints `not ok` into the log of a green job", which is the same defect wearing a disguise -- and it is exactly the absence-reads-as-success mode this gate exists to kill, reproduced against the fix for it. Now proven by exit code rather than by log text: ENGINE_CODE neutered -> exit 1 ("no longer reads as an escaper") control -> exit 0 The `ENGINE_MENTION` guard added two commits ago had the same defect and moves into the same hook, ordered first: without it the probe dereferences `.file` on undefined and reports a TypeError instead of the real cause. `before()` over a dedicated `it` deliberately: a hook failure cancels the six cases, so one cause is reported. As its own `it`, the other five would run and pass vacuously, which is the shape this whole change exists to remove. Also dropped two line numbers from a comment that its own insertion had already falsified -- it said :252 and :263 when the probes had shifted to :262 and :273.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mainhas been red since 22:47 on exactly one violation, and it is prose.What happened
The comment embedded a literal copy of the anchored trigger class. #3102 added
check-csv-escaper-copies.mjs, whose whole purpose is to stop that class being copied, and it does not distinguish a copy in code from a copy in a comment.Each PR was green alone and red only together. The comment landed via #3100 after #3102 branched, so #3102's own CI never saw it. That is the second such pair today, after #3058 + #3039, and neither is visible from either side.
The fix
Describe the guard instead of restating its pattern. A comment copy goes stale exactly like a code copy, so the gate is right to object even though the line is prose.
Verified by running, not by reading:
check-source-text-assertionspasses. No changeset: that gate validates that existing changesets name workspace members and does not require one to exist, and a comment in a test file changes no behaviour or public API.Why it is two lines and not twelve
Four earlier drafts of this comment were rejected in review. The failure mode was identical every time: a replacement that asserted something extra and unverified.
csv-cell.tsholds the "single surviving definition"engine.ts:790has one too, and the gate output quoted in my own commit message named itguardSpreadsheetFormulacsv-cell.ts:99says CSV writers wantescapeCsvCellKNOWN_REMAININGblockDraft 4 is the instructive one. It was accurate and still wrong to ship: three homes for one fact is how the next stale comment gets written, which is the same argument this PR makes about the pattern itself.
So this keeps only what the round-trip invariant needs, and adds no pointer, no cross-reference and no second copy of anything. Not one draft was rejected for saying too little.
Scope
Teaching the gate to skip comments is the better long-term fix and is being worked separately. Six attempts have each been found holed by review, so it wants an unhurried review on a green
mainrather than another round under time pressure. A reframing is also on the table: making the patterns themselves unmatchable by prose, by requiring.test(ormatches!(adjacency, which would remove the need for comment handling entirely.Nothing in this PR touches the gate.
🤖 Generated with Claude Code
Summary by CodeRabbit