fix(stella-observatory): clip the page's text on code points, not code units (#2026) - #2047
Merged
Merged
Conversation
…e units The dashboard's `clip` counted `String.prototype.length` — UTF-16 code units — and cut with `slice`, so a string whose n-th code unit fell inside a surrogate pair was cut into a lone surrogate and rendered as `�`. It has ten call sites, and the ones most exposed are user-authored text: prompts, session titles, recalled context, reflection lessons. It is the fourth copy of a clipper this crate consolidated in #1999, and the only one that had actually drifted. That is not a coincidence: the page is JavaScript inside a Rust crate, so rustc, clippy and the golden route payloads all read straight past it. A duplicated decision no gate can see is the one that drifts. `clip` now iterates code points via `Array.from`, matching `db::truncate` exactly. The `v.length <= n` fast path is kept and is exact rather than an approximation — a code point is one or two code units, so code units <= n proves code points <= n — which means only a string that might actually need clipping pays for the array, and the common short case stays O(1) on a page that re-renders every 5s. The witness is tests/page_clipper.rs: it pulls `clip` out of the page as served by `respond` (not a copy, not a re-`include_str!`), runs it under `node` against a corpus swept across every cut offset, and asserts agreement with `db::truncate`'s contract plus well-formed output. On the old code it fails with 35 mismatches, the first reading got `\ud83d…` against want `👍`; on the new code it passes. Shelling out to a binary and skipping when absent is this workspace's idiom for this (stella-tools' git tests, the registry fence test's `make`), and a shape assertion that needs no node covers the skip case so a regression is never caught only by a test that did not run. The corpus also showed the defect was wider than surrogate splitting: with any astral-plane character present the old clipper cut at the wrong position too. README: the Testing section claimed "no `tests/` directory", which three suites had already falsified before this one; it now names all four and why each is not inline. A new Gotchas entry states the invariant — text is clipped twice and the two clippers must agree on shape, only lengths are per-surface. Closes #2026
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideUpdates the dashboard’s JavaScript clipper to operate on Unicode code points instead of UTF-16 code units and adds a Node-driven integration test that extracts the actual served clipper from the HTML page to ensure it matches the Rust server-side truncation contract, alongside README documentation updates describing the dual clipping invariant and the external test suites. Sequence diagram for Node-driven page clipper testsequenceDiagram
participant RustTest as page_clipper_rs
participant Respond as respond
participant Node as node
participant JSClip as clip
participant DbTrunc as db_truncate
RustTest->>Respond: respond
Respond-->>RustTest: index_html
RustTest->>Node: execute_clip(index_html)
Node->>JSClip: clip(s, n)
JSClip-->>Node: clipped_js
RustTest->>DbTrunc: db_truncate(s, n)
DbTrunc-->>RustTest: clipped_db
RustTest-->>RustTest: assert_eq(clipped_js, clipped_db)
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
What & why
The dashboard's
clip(src/assets/index.html) countedString.prototype.length— UTF-16 code units — and cut withslice, so astring whose n-th code unit fell inside a surrogate pair was cut into a lone
surrogate and rendered as
�. It has ten call sites and the most exposedcarry user-authored text: prompts, session titles, recalled context, reflection
lessons.
It is the fourth copy of a clipper this crate consolidated in #1999, and the
only one that had actually drifted. That is not a coincidence — the page is
JavaScript inside a Rust crate, so rustc, clippy and the golden route payloads
all read straight past it. A duplicated decision that no gate can see is the
one that drifts.
clipnow iterates code points viaArray.from, matchingdb::truncateexactly. The
v.length <= nfast path is kept and is exact, not anapproximation: a code point is one or two code units, so code units ≤ n
proves code points ≤ n. Only a string that might actually need clipping pays
for the array, so the common short case stays O(1) on a page that re-renders
every 5 s.
Closes #2026
The witness
main, passes here)tests/page_clipper.rspullsclipout of the page as served byrespond— not a copy, not a re-include_str!— runs it undernodeagainst a corpus swept across every cut offset, and asserts agreement with
db::truncate's contract plus well-formed output.Verified both directions by restoring the old one-liner in the working tree:
clipThe first reported case is the defect in one line —
got\ud83d…(a lone high surrogate plus the ellipsis) against
want👍(the intact 👍).
Two notes on how it is built:
nodeand skipping when absent is this workspace'sestablished idiom for external binaries —
stella-tools' git tests andregistry/tests/fence.rs'smaketest both do it. Embedding a JS engine(
boa,quickjs) is a large dependency for one arrow function. CI runnersship node, so it gates there.
code_unit_clipping_does_not_come_backasserts the shape of the extracted function and needs no node. A regression
is never caught only by a test that did not run.
The driver escapes its diagnostics to bare ASCII, and that is load-bearing
rather than cosmetic: a lone surrogate is not representable in a Rust
String,so the first draft of this test failed on the old code with
unexpected end of hex escape— serde_json rejecting the driver's own output —instead of naming the defect. The bug broke the channel used to report it.
The corpus also showed the defect was wider than surrogate splitting: with
any astral-plane character present, the old clipper cut at the wrong position
too, which is why 35 cases fail rather than a handful.
The gate
cargo fmt --check(viamake guards-fast, exit 0)cargo clippy -p stella-observatory --all-targets -- -D warnings(exit 0)cargo test -p stella-observatory(exit 0 — 102 passed, 0 failed)make guards-fast(exit 0),make doc-links(exit 0)Closes #2026both here and as a commit trailerNothing left behind
Two things noticed and fixed here rather than deferred:
tests/directory" — threesuites had already falsified that before this one. It now names all four and
why each cannot be inline.
two clippers must agree on shape, and only the lengths are per-surface.
Ground-rule check
stella-core; no new deps (node is invoked, not linked)dashboard_html_has_no_external_referencespasses; nothing was imported
Anything reviewers should know?
Relationship to #2027. That PR (#1999) folds the three Rust clippers into
one
db::truncate; this one fixes the JavaScript clipper. They areindependent — this branch is cut from
main, touches no file #2027 touches,and the two merge in either order.
Why the oracle is restated rather than imported.
db::truncateispub(crate), so an integration test cannot call it, and the test states thecontract in six lines instead. That is what an oracle should be — an
independent derivation, since one sharing the implementation could not observe
a bug in it. The honest tradeoff: this pins the contract, so changing
db::truncate's behaviour fails here and forces the author to change bothdeliberately. It does not silently follow.
Grapheme clusters are explicitly out of scope. A flag emoji or an
emoji-with-modifier is several scalar values in both languages, and both
clippers may still cut inside one.
db::truncatedoes no graphemesegmentation either, so matching it at the scalar level is exactly the
consistency being asked for; the corpus includes a combining mark and a ZWJ
sequence to pin that the two agree on this, rather than to claim it is
segmented. No segmentation library is pulled in.
Summary by Sourcery
Align the dashboard’s client-side text clipping with the server-side truncation to operate on Unicode code points instead of UTF-16 code units, and guard it with integration tests that execute the shipped HTML under Node.
Bug Fixes:
Enhancements:
Tests: