diff --git a/crates/stella-observatory/README.md b/crates/stella-observatory/README.md index 7cc49464d..c13deced5 100644 --- a/crates/stella-observatory/README.md +++ b/crates/stella-observatory/README.md @@ -233,6 +233,18 @@ accent): the glyph and the badge context, never the hue, say "warning". - **The page is embedded at compile time.** Editing [`src/assets/index.html`](src/assets/index.html) does nothing until you rebuild — `--example serve` included. +- **Text is clipped twice, and the two clippers must agree.** The server clips + in `db::truncate` and the page clips again in `clip` + ([`src/assets/index.html`](src/assets/index.html)); both cut on a **code + point** and append `…`, and only the *lengths* are per-surface. The page's + copy is the one no Rust gate can see — rustc, clippy and the golden payloads + all read straight past it — and it is the one that drifted: it counted + UTF-16 code units and cut with `slice`, so any prompt whose n-th unit landed + mid-emoji reached the DOM as a lone surrogate, a `�` (#2026). + [`tests/page_clipper.rs`](tests/page_clipper.rs) now runs that function under + `node` against the contract, so a fourth spelling of "clip" fails at + `cargo test`. If you add clipping to the page, call `clip` — do not inline + `slice`. - **The test schema is a hand-written copy, and it has already drifted.** `seeded_workspace` in `src/lib.rs` spells out its own DDL for the subset of tables the observatory reads, and nothing checks it against @@ -276,8 +288,17 @@ cargo test -p stella-observatory There is no crate-specific `make` target — the root `Makefile` has one only for core/model/tools/cli/protocol — so `make test` picks this up with the workspace. -Every test is an inline `#[cfg(test)] mod tests`: no `tests/` directory, no -fixture dir, feature flag or env var. +No fixture dir, feature flag or env var. Most tests are an inline +`#[cfg(test)] mod tests`; four suites live in `tests/` because each needs +something the inline module cannot have — a dev-dependency on the crate whose +schema it is checking, a real socket, or `node`: + +| Suite | Why it is not inline | +|---|---| +| [`tests/schema_conformance.rs`](tests/schema_conformance.rs) | Builds `store.db` through `stella-store`'s real migrations (#827) — a **dev**-dependency, so production still links nothing. | +| [`tests/journal_era.rs`](tests/journal_era.rs) | The other half of the digest alarm: which of two things a mismatch means depends on `executions.journal_era` (#1981). | +| [`tests/live_stream.rs`](tests/live_stream.rs) | Drives the live endpoint over a real socket. | +| [`tests/page_clipper.rs`](tests/page_clipper.rs) | Executes the page's own JavaScript under `node` — see below. | The dominant shape: `seeded_workspace()` builds a `TempDir` with a `store.db` seeded from hand-written DDL, `seed_fs_surfaces()` layers on the file-backed diff --git a/crates/stella-observatory/src/assets/index.html b/crates/stella-observatory/src/assets/index.html index 6c1153030..4c92bf198 100644 --- a/crates/stella-observatory/src/assets/index.html +++ b/crates/stella-observatory/src/assets/index.html @@ -1060,8 +1060,23 @@

${fmtInt(runs)} undated ${runs === 1 ? "run" : "runs"} } /* Clip to n characters without assuming the field is a string: several of - these arrive from JSON that a bad row can leave null. */ -const clip = (s, n) => { const v = String(s ?? ""); return v.length > n ? v.slice(0, n) + "…" : v; }; + these arrive from JSON that a bad row can leave null. + + `n` counts code points, matching the server's `db::truncate` exactly — see + tests/page_clipper.rs, which runs this very function against that one. It + is deliberately NOT `v.length`/`v.slice`, which count UTF-16 code units and + would cut a surrogate pair in half, putting a lone surrogate — a `�` — in + the DOM for any prompt whose n-th unit lands mid-emoji (#2026). + + The `v.length <= n` fast path is exact, not an approximation: 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. */ +const clip = (s, n) => { + const v = String(s ?? ""); + if (v.length <= n) return v; + const cp = Array.from(v); + return cp.length > n ? cp.slice(0, n).join("") + "…" : v; +}; /* ── models table ────────────────────────────────────────────────────── */ function renderModels(rows) { diff --git a/crates/stella-observatory/tests/page_clipper.rs b/crates/stella-observatory/tests/page_clipper.rs new file mode 100644 index 000000000..b9d9fcd7d --- /dev/null +++ b/crates/stella-observatory/tests/page_clipper.rs @@ -0,0 +1,280 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// Copyright (c) 2026 Oxagen, Inc. Commercial licensing: licensing@oxagen.sh + +//! **The page-clipper gate (#2026).** +//! +//! The dashboard clips long text in two places — once on the server, in +//! `db::truncate`, and again in the browser, in `clip` +//! ([`src/assets/index.html`](../src/assets/index.html)). Three Rust copies of +//! the server-side clipper were folded into one in #1999. The browser copy +//! could not be folded with them, and it is the one that had actually drifted: +//! it 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 `�`. Every prompt, title and +//! reflection on the page is user-authored text that can contain an emoji. +//! +//! Nothing could catch that. The page is JavaScript inside a Rust crate: the +//! compiler does not read it, clippy does not read it, and the crate's other +//! two page assertions +//! (`dashboard_html_has_no_external_references`, the golden route payloads) +//! never execute it. A duplicated decision that no gate can see is the one +//! that drifts — which is exactly what happened here. +//! +//! So this suite executes it. It pulls `clip` out of the page **as served by +//! [`respond`](stella_observatory::respond)**, not out of a copy or a +//! re-`include_str!`, runs it under `node` against a corpus built in Rust, and +//! asserts the contract below. An edit to that arrow function now fails at +//! `cargo test`. +//! +//! ## The contract +//! +//! For `clip(s, n)`, where `v` is `s` coerced to a string and a *character* is +//! a Unicode scalar value (Rust's `char`, JavaScript's code point): +//! +//! 1. **Well-formed output.** Never a lone surrogate. This is the defect. +//! 2. **Short strings are untouched.** `v` has `<= n` characters → `v` back, +//! byte for byte, with no ellipsis. +//! 3. **Long strings are cut on a character boundary.** Otherwise the first +//! `n` characters of `v`, then `'…'`. +//! 4. **Non-strings still work.** Several fields arrive from JSON a bad row +//! can leave `null`; `clip` coerces rather than throwing. +//! +//! Rules 2 and 3 are `db::truncate`'s contract, restated here rather than +//! imported: `db::truncate` is `pub(crate)`, so an integration test cannot +//! call it. The restatement is deliberate and is what an oracle should be — +//! an independent derivation, since an oracle that shared the implementation +//! could not observe a bug in it. The tradeoff is honest: this suite pins the +//! *contract*, so if `db::truncate`'s own behaviour is ever changed, this +//! fails and the author must change both — it does not silently follow. +//! +//! ## Why `node`, and what happens without it +//! +//! Executing JavaScript needs a JavaScript engine, and embedding one +//! (`boa`, `quickjs`) is a large dependency for one arrow function. Shelling +//! out to a binary and skipping when it is absent is this workspace's +//! established idiom for exactly this — `stella-tools`' git tests and +//! `stella-tools/src/registry/tests/fence.rs`'s `make` test both do it. CI's +//! runners ship node, so this gates there. + +use std::process::Command; + +use serde_json::{Value, json}; + +/// The page as the browser receives it — not `include_str!` of the asset, so +/// a change to how the route serves it cannot leave this suite testing a file +/// nobody is shipping. +fn served_page() -> String { + let dir = tempfile::tempdir().expect("tempdir"); + let res = stella_observatory::respond(dir.path(), "/"); + assert_eq!(res.status, "200 OK", "the dashboard route must serve"); + String::from_utf8(res.body).expect("the page is UTF-8") +} + +/// Cut `src` out of the page, from `const clip =` to the brace that closes it. +/// +/// Brace-counted rather than line-counted so reformatting the function does +/// not quietly extract half of it; the body contains no braces inside string +/// literals, which is what makes the naive count sound. +fn extract_clip(page: &str) -> String { + let start = page + .find("const clip =") + .expect("index.html must define `clip` (did it get renamed?)"); + let rest = &page[start..]; + let mut depth = 0usize; + let mut seen_open = false; + for (i, c) in rest.char_indices() { + match c { + '{' => { + depth += 1; + seen_open = true; + } + '}' => { + depth -= 1; + if depth == 0 && seen_open { + let end = i + c.len_utf8(); + let tail = rest[end..].strip_prefix(';').map_or(end, |_| end + 1); + return rest[..tail].to_string(); + } + } + _ => {} + } + } + panic!("`clip` in index.html has unbalanced braces"); +} + +/// Rule 2 + rule 3 of the contract — `db::truncate`'s behaviour, restated. +fn oracle(s: &str, n: usize) -> String { + if s.chars().count() <= n { + return s.to_owned(); + } + let mut out: String = s.chars().take(n).collect(); + out.push('…'); + out +} + +/// Strings whose clip boundary is worth probing, each swept across every `n` +/// from 0 to a few past its length so the cut lands *inside* every character. +fn corpus() -> Vec<(String, usize)> { + let subjects = [ + String::new(), + "plain ascii".to_string(), + // The defect: a 2-code-unit character. Placed at several offsets so + // some `n` is guaranteed to fall inside it. + "\u{1F44D}".to_string(), + format!("{}\u{1F44D}tail", "a".repeat(5)), + "\u{1F44D}\u{1F44D}\u{1F44D}".to_string(), + "mixed \u{1F600} emoji \u{1F680} run".to_string(), + // Astral-plane non-emoji: CJK extension B. + "\u{20000}\u{20001}\u{20002}".to_string(), + // BMP multi-byte in UTF-8 but single-unit in UTF-16 — the case where + // Rust bytes and JS units disagree in the *other* direction. + "日本語のテキスト".to_string(), + // A combining mark: two scalar values, one grapheme. Both clippers + // may split this, and agreeing that they do is the point. + "e\u{0301}e\u{0301}e\u{0301}".to_string(), + "\u{1F3F3}\u{FE0F}\u{200D}\u{1F308}".to_string(), + ]; + let mut cases = Vec::new(); + for s in subjects { + let span = s.chars().count() + 3; + for n in 0..=span { + cases.push((s.clone(), n)); + } + } + cases +} + +#[test] +fn page_clip_matches_db_truncate_and_never_splits_a_surrogate_pair() { + if Command::new("node").arg("--version").output().is_err() { + eprintln!("skipping page-clipper test: `node` not available"); + return; + } + + let clip_src = extract_clip(&served_page()); + + let mut cases: Vec = corpus() + .into_iter() + .map(|(s, n)| json!({ "s": s, "n": n, "want": oracle(&s, n) })) + .collect(); + // Rule 4: the fields these clip arrive from JSON, and a bad row can leave + // one null. Coercion is part of the contract, so it is part of the gate. + cases.push(json!({ "s": Value::Null, "n": 5, "want": "" })); + cases.push(json!({ "s": 1234567, "n": 3, "want": "123…" })); + cases.push(json!({ "s": 12, "n": 5, "want": "12" })); + + let dir = tempfile::tempdir().expect("tempdir"); + let cases_path = dir.path().join("cases.json"); + let script_path = dir.path().join("check.js"); + std::fs::write(&cases_path, Value::Array(cases).to_string()).expect("write cases"); + + // Two details in the driver are load-bearing. + // + // `isWellFormed` is Node 20+; where it is missing the regex is the same + // check — an unpaired high or low surrogate anywhere in the output. + // + // `show` escapes every reported string to bare ASCII, and without it this + // suite cannot report its own headline failure: a lone surrogate is not + // representable in a Rust `String`, so serde_json rejects the driver's + // output and the panic reads "unexpected end of hex escape" instead of + // naming the defect. Comparison still uses the raw values; only the + // diagnostic is escaped, so a failure prints `\ud83d` and says so. + let script = format!( + r#"const fs = require("fs"); +{clip_src} +const wellFormed = s => typeof s.isWellFormed === "function" + ? s.isWellFormed() + : !/[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(? {{ + let o = ""; + for (let i = 0; i < s.length; i++) {{ + const u = s.charCodeAt(i); + o += (u >= 0x20 && u < 0x7f) ? s[i] : "\\u" + u.toString(16).padStart(4, "0"); + }} + return o; +}}; +const bad = []; +for (const c of JSON.parse(fs.readFileSync(process.argv[2], "utf8"))) {{ + const shown = show(String(c.s ?? "")); + let got; + try {{ got = clip(c.s, c.n); }} + catch (e) {{ bad.push({{ n: c.n, why: "threw: " + e.message, s: shown }}); continue; }} + if (got !== c.want) {{ + bad.push({{ n: c.n, why: "mismatch", s: shown, want: show(c.want), got: show(got) }}); + }} else if (!wellFormed(got)) {{ + bad.push({{ n: c.n, why: "lone surrogate", s: shown, got: show(got) }}); + }} +}} +process.stdout.write(JSON.stringify(bad)); +"# + ); + std::fs::write(&script_path, script).expect("write script"); + + let out = Command::new("node") + .arg(&script_path) + .arg(&cases_path) + .output() + .expect("run node"); + assert!( + out.status.success(), + "node failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + + let failures: Vec = + serde_json::from_slice(&out.stdout).expect("the driver emits a JSON array"); + assert!( + failures.is_empty(), + "the page's `clip` disagrees with `db::truncate` in {} case(s).\n\ + `n` counts code points, not UTF-16 code units — see index.html:1064 \ + and the module docs.\nFirst few: {}", + failures.len(), + Value::Array(failures.into_iter().take(6).collect()) + ); +} + +#[test] +fn the_page_defines_exactly_one_clipper() { + // Two definitions would mean the extraction above is silently testing + // whichever came first — the failure mode that let three Rust copies + // accumulate before #1999. + let page = served_page(); + assert_eq!( + page.matches("const clip =").count(), + 1, + "index.html must define `clip` exactly once" + ); +} + +/// The extractor is load-bearing: if it silently grabbed the wrong text the +/// suite above would pass while testing nothing. +#[test] +fn extractor_takes_the_whole_function() { + let src = extract_clip(&served_page()); + assert!(src.starts_with("const clip ="), "starts at the definition"); + assert!(src.ends_with("};"), "runs to the closing brace: {src}"); + assert_eq!( + src.matches('{').count(), + src.matches('}').count(), + "balanced: {src}" + ); +} + +/// Pins the defect by shape, so the guard survives a machine with no `node`. +/// +/// The test above is the real one — it proves behaviour — but it skips when +/// `node` is absent, and a regression that only a skipped test catches is not +/// caught. This one always runs. +#[test] +fn code_unit_clipping_does_not_come_back() { + let src = extract_clip(&served_page()); + assert!( + !src.contains("v.length > n"), + "`clip` is counting UTF-16 code units again — that splits surrogate \ + pairs and puts a lone surrogate in the DOM (#2026): {src}" + ); + assert!( + src.contains("Array.from"), + "`clip` must iterate code points, not code units (#2026): {src}" + ); +}