Conversation
Two related changes surfaced by a trace comparison between the classic JS
reg-cli and this Wasm build.
1. Remove the `setTimeout(resolve, 2000)` after `sdk.shutdown()` in
`js/tracing.ts`. `sdk.shutdown()` already awaits exporter flush; the extra
sleep was a paranoia workaround that dominated wall-clock when OTEL is on:
20 images × 1280×720, OTEL on:
before: Wasm 2.53s (JS 0.88s)
after: Wasm 0.49s (JS 0.81s) ← Wasm is now faster
2. Add JS-bridge spans so the invisible time between `main()` and Rust
`reg_cli_main` is visible. New spans (worker -> parent via postMessage,
parent converts to OTel):
main.init_tracing
main.new_entry_worker
main.new_thread_worker (× N)
main.process_trace_and_spans
main.run_total
entry.wasm_compile
entry.wasm_instantiate
entry.wasi_start
entry.init_tracing_rust
entry.wasm_main
entry.read_report_string
entry.collect_rust_traces
entry.worker_total
worker.wasm_compile
worker.wasm_instantiate
worker.wasi_start
worker.wasi_thread_start
worker.thread_total
Before this, the ~180ms between Node startup and Rust `reg_cli_main`
looked like a mystery gap in Jaeger. Now each stage is attributable.
3. Fix bare `postMessage(...)` in `js/worker.ts`. `postMessage` is not a
global inside Node's `worker_threads`; use `parentPort?.postMessage(...)`
to match `js/entry.ts`. This was silently not working or emitting an
async error that aborted thread-spawn under some conditions.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Add bench/ harness; bump image-diff-rs to 0.1.0 ## Changes - **Cargo.toml (workspace)**: add `[profile.release]` (lto / codegen-units=1 / opt-level=3 / panic=abort / strip) for smaller + faster wasm binary - **crates/reg_core/Cargo.toml**: pin `image-diff-rs` to tag `0.1.0` (https://github.com/bokuweb/image-diff-rs/releases/tag/0.1.0 — adds internal tracing spans for decode / compare / encode) - **bench/**: JS vs Wasm trace-comparison benchmark harness - `generate.sh` — ImageMagick-based fixture generator (20 pairs × 1280×720, 5 mutation types: shift / hue / badge / rect / stripe) - `run.sh` — run both `dist/cli.js` and `js/dist/cli.mjs` against the same fixture, capturing OTLP traces - `export-traces.sh` — pull latest JS and Wasm traces from Jaeger and write `out/traces.json` - `viz/index.html` — single-trace waterfall viewer (per-service) - `viz/compare.html` — **aligned-timescale stacked view** (JS on top, Wasm on bottom, shared x-axis) - `README.md` — mutation matrix and usage - `RESULTS.md` — measurement results with honest caveats (PNG vs WebP encode, reg.json file write, etc.) ## Measured (macOS Apple Silicon / Node v20 / warm median) | workload | JS | Wasm | ratio | |---|---:|---:|---:| | 20 × 1280×720 | 0.59 s | 0.42 s | 1.40× | | 1 × 1280×720 | 0.28 s | 0.24 s | 1.18× | | 1 × 3840×2160 | 1.29 s | 0.69 s | 1.87× | | identical × 20 | 0.30 s | 0.29 s | ~equal | Caveats included in `bench/RESULTS.md`: - JS writes diff images as PNG; Wasm writes as WebP (WebP encode is cheaper) - JS writes `reg.json` to file; Wasm returns it as a string - Concurrency default 4 for both; md5 / byte short-circuit for identical both Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix pathdiff panic; add --diffFormat flag; update RESULTS Three fixes that fell out of "why is the JS vs Wasm benchmark not apples-to-apples": ## 1. `pathdiff::diff_paths` no longer panics on mixed paths `resolve_dir(base, target)` previously called `.expect("should resolve relative path.")` which panicked whenever `base` and `target` were a mix of absolute and relative paths (e.g. absolute `--report` + relative fixture directory). Now: - Both-same-kind → pathdiff as before - Mixed → canonicalize both to absolute via `current_dir()` and retry - Still no match → return `target` unchanged instead of panicking Added 4 unit tests covering both-relative, both-absolute, and both mixed directions. ## 2. `--diffFormat {webp,png}` CLI flag Depends on image-diff-rs PR #34 (bokuweb/image-diff-rs#34) which adds `EncodeFormat`. Plumbed through: - `reg_core::Options` gets `diff_image_format: Option<DiffImageFormat>` - `DiffImageFormat::{Webp, Png}` as a user-facing mirror - File extension of written diff images + HTML report follows the choice - `reg_cli`'s `main.rs` exposes `--diffFormat {webp,png}` via clap Pointed `reg_core`'s `image-diff-rs` dep at the PR branch temporarily; will move back to a tag once image-diff-rs #34 is merged and released. ## 3. bench/RESULTS.md — corrected numbers Replaced the earlier WebP-vs-PNG confounded numbers with apples-to-apples PNG-vs-PNG measurement: | workload | JS (PNG) | Wasm `--diffFormat png` | ratio | |---|---:|---:|---:| | 20 × 1280×720 | 0.71 s | 0.46 s | **1.54× faster** | | 1 × 3840×2160 (4K) | 0.92 s | 0.42 s | **2.19× faster** | | identical × 20 (skip) | 0.30 s | 0.29 s | ~equal | Also documents all the setup caveats surfaced during the investigation (WASI preopen sandbox, pathdiff panic, postMessage form, 2s sleep, etc.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Extend benchmark to 100 images — gap widens to 1.98× faster Added 100 × 1280×720 row to the apples-to-apples and default tables. At 100 images the Wasm advantage grows from 1.54× → 1.98× compared to JS PNG (and 1.31× → 1.54× in default WebP mode). Consistent with the hypothesis that startup cost amortizes and per-image compute stays flat — the more work to do, the more the Rust core wins. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Bump image-diff-rs to 0.1.1 (EncodeFormat released) image-diff-rs#34 merged and tagged as 0.1.1, so we can drop the branch pin and use a proper tag. - Before: branch = "add-png-encode-option" - After: tag = "0.1.1" No code changes; still smoke-tested with `--diffFormat png`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The root package.json carried a `packageManager: "pnpm@10.26.2+..."` field (introduced via the wasm branch's tooling migration), but CI still installs the outer project with yarn and the repository still ships a `yarn.lock`. Corepack/yarn 1.x refuses to install when the declared packageManager doesn't match the invoked tool: error This project's package.json defines "packageManager": "yarn@pnpm@10.26.2". However the current global version of Yarn is 1.22.22. The simplest fix that restores CI without churn is to drop the field. The inner `js/` workspace (Wasm bridge) uses its own pnpm + lockfile and is unaffected by removing the root-level declaration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Matches the `packageManager: pnpm@10.26.2` declared in package.json. - `.github/workflows/ci.yml`: use `pnpm/action-setup@v4` + `pnpm install --frozen-lockfile`, bump `actions/checkout`/`setup-node` to v4, Node 20 - `package.json`: replace `resolutions` (yarn glob syntax) with `pnpm.overrides` (flat, pnpm native). Same 21 pinned versions, no behaviour change. - `pnpm-lock.yaml`: added (generated by `pnpm install`) - `yarn.lock`: removed (no longer authoritative) - `decls/deps.js`: add flow declarations for third-party modules that .flowconfig's `[ignore] node_modules` hides from flow's resolver. The nine "Cannot resolve module" errors that surface after a clean install are all third-party imports (`cli-spinner`, `meow`, `lodash`, `chalk`, etc.); this restores `pnpm run flow` to pass. The inner `js/` workspace (Wasm bridge) already uses its own pnpm install and is unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
An earlier commit removed the `packageManager` field as a workaround for yarn, but pnpm/action-setup@v4 reads exactly this field to pick the pnpm version. Restore it now that CI is on pnpm. Fixes: "Error: No pnpm version is specified." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Specify `version: 10.26.2` directly on pnpm/action-setup@v4 instead of relying on the outer `packageManager` field. The outer field was conflicting with the inner `yarn install` inside `report/ui/` because yarn 1.22 walks up the directory tree when checking the corepack packageManager constraint. - Remove `packageManager` from the root package.json so yarn inside the cloned `report/ui` (which has its own yarn.lock and needs Yarn 1.22) doesn't refuse to install. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pnpm 10 blocks postinstall scripts by default (via `onlyBuiltDependencies`). puppeteer 13.7 downloads Chromium in its postinstall; without it `test/screenshot.js` fails with: Error: Could not find expected browser (chrome) locally. Run `npm install` to download the correct Chromium revision (982053). Add `puppeteer` to `pnpm.onlyBuiltDependencies` so its install.js runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Click the label row of a span that has children to toggle the sub-tree. Rows with children show ▾ (expanded) or ▸ (collapsed); leaves show no arrow. New "collapse all" / "expand all" buttons in the toolbar operate on the union of parent-span IDs across both traces so the same state applies to every lane. Useful for the 100-image waterfall where `parallel_image_diff` or `diff_single_image` otherwise push all the interesting one-liners (boot / cleanup / threadpool) off-screen. Implementation notes: - Shared `collapsed: Set<spanId>` state at the module level - `flatten()` (compare.html) / `walk()` (index.html) skip descendants of collapsed ids, and tag each row with `hasChildren` - Invisible full-width hit rect per collapsible row so the entire label area is clickable, not just the arrow glyph - `pointer-events: none` on the text/badge inside so the underlying hit rect catches the click reliably Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Before this, entry.ts / worker.ts set `preopens: { './': './' }` and
`env: env as Record<string, string>`. That means any Wasm code that
takes control of the pipeline (e.g. via an image-decoder CVE — libpng,
libwebp, libjpeg have a long history of RCE-class ones) could read
anything under the invocation cwd (`.npmrc`, `.env`, `node_modules`,
...) and see every host env var (`NPM_TOKEN`, `AWS_*`, ...).
This change introduces `computeWasiSandbox(argv)` in `js/utils.ts` and
plugs it into both `entry.ts` and `worker.ts`:
- **preopen** — the narrowest single ancestor directory that contains
every path the run actually touches: `actualDir`, `expectedDir`,
`diffDir`, and the parents of `--report` / `--json`. In a typical
invocation inside a repo the sandbox shrinks from "all of CWD" to
"just the reg-cli fixture folder", i.e. the Wasm side no longer sees
sibling files like `.env` or `~/.config/...`
- **env** — allowlist only OTel-related variables
(`OTEL_ENABLED`, `OTEL_DEBUG`, `OTEL_EXPORTER_OTLP_ENDPOINT`,
`OTEL_SERVICE_NAME`, …). Everything else (AWS_*, NPM_TOKEN, CI
secrets) stops at the Wasm boundary
## Why only a single ancestor preopen?
Originally this PR tried to register one preopen per directory
(actual / expected / diff / report-parent / json-parent). Writes to
the first preopen worked; the other 4 silently became invisible to
Rust. Instrumenting `@tybys/wasm-util` showed that WASI-side all 3-5
preopens were correctly installed (fd 3, 4, 5, ...) but the Wasm side
only ever issued `fd_prestat_get(3)` and never queried fd 4+, which
smells like an enumeration bug in `wasm32-wasip1-threads` libstd.
Falling back to the common ancestor avoids the bug while still giving
a real defense-in-depth win. When the upstream issue is fixed we can
swap back to per-directory preopens.
## Not in scope (explicit follow-ups)
- Symlinks / FIFOs inside the preopened directory are still resolved
by the host. An attacker who can plant files under `./diff/` could
still exfiltrate via an evil symlink.
- `fs: fs as IFs` still hands `@tybys/wasm-util`'s WASI unrestricted
Node `fs`. We constrain *the paths Wasm is allowed to name*, not
what the host fs layer underneath would do on Wasm's behalf.
- stdout / stderr inheritance. If the host redirects them to a socket
(`reg-cli > >(nc ...)`) Wasm output leaves the box through the
host's pipe, not through a WASI syscall.
## Also bumps `rust-toolchain.toml` nightly-2024-08-24 → 2025-01-01
Build fix: dependencies now require `edition2024` which the old nightly
cannot stabilise. No behavioural change.
## Test plan
- [x] 20-image fixture: 16 diff images produced, report.html written
- [x] 100-image fixture: same (timings within noise of pre-change)
- [x] `SECRET_TOKEN=x NPM_TOKEN=y AWS_ACCESS_KEY_ID=z reg-cli ...` —
none of those are forwarded into Wasm (confirmed via logged
`sandbox.env` during bring-up)
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…583) Makes `@bokuweb/reg-cli-wasm`'s CLI a drop-in replacement for `reg-cli` for the most common CI use-cases. Closes the first tier of compat gaps between the JS classic CLI and the Wasm build; `--from`, `--junit`, `-D`, `-X` and the per-file library events remain for a follow-up PR. ## Before - `node js/dist/cli.mjs ... ` exited 0 regardless of diffs → CI didn't detect failures - Only long-form flags (`--json`, `--report`, ...) worked; `-J`, `-R`, `-M`, `-T`, `-S`, `-C`, `-A` all failed with "unexpected argument" - No progress / summary lines on stdout - `-U`, `-I`, `-E` were not recognised at all - Worker `error` events were re-thrown outside the EventEmitter, so library users couldn't `.on('error', ...)` them Swapping classic reg-cli → the Wasm build was therefore a script change, not a drop-in replacement. ## After ### Rust / clap (`crates/reg_cli/src/main.rs`) - Add POSIX short aliases to every existing option: `-R/--report`, `-J/--json`, `-M/--matchingThreshold`, `-T/--thresholdRate`, `-S/--thresholdPixel`, `-P/--urlPrefix`, `-C/--concurrency`, `-A/--enableAntialias` - `-A` now also accepts a bare form (`num_args = 0..=1` + `default_missing_value = "true"`) so `-A` alone means `-A=true`, matching meow's boolean semantics. - `--diffFormat` unchanged (long-only, matching no-short policy for wasm-only flag). ### CLI wrapper (`js/cli.ts`) Rewritten as a thin front-end around `run()` that handles everything the Wasm side doesn't (yet) know about: - Proper arg parsing with Node's built-in `util.parseArgs` — no extra dep; short aliases listed there too as a safety net. - `--help` / `-h` short-circuit prints the help text (clap help is reachable via the Wasm too but is cosmetically different). - `-U / --update` — after complete, copy `actualItems` over `expectedDir` with `mkdir(recursive)` + `copyFile`. - `-I / --ignoreChange` — suppresses non-zero exit. - `-E / --extendedErrors` — treats new/deleted counts as failures. - `-D / --customDiffMessage` — trailing message printed on diff, defaulting to classic's "Inspect your code changes, re-run with `-U` to update them.". - Per-file `✔ pass / ✘ change / ✚ append / − delete` log lines + summary. - `process.exitCode = 1` on failure (honouring `-I`). ### Library (`js/index.ts`) The two `worker.on('error', e => { throw new Error(e.message) })` hooks are now `emitter.emit('error', e)`, matching classic reg-cli's EventEmitter surface. Library users can now: const emitter = compare({ ... }); emitter.on('error', (err) => { /* handle */ }); emitter.on('complete', (data) => { /* ... */ }); instead of needing a global `uncaughtException` handler. ### Also: `rust-toolchain.toml` nightly-2024-08-24 → 2025-01-01 Transitive dep now requires `edition2024` which the old nightly cannot stabilise. No behavioural change. ## Test plan - [x] 20 images: all short aliases (`-R -J -M -C`) parsed, 16 PNG diffs produced, exit 1. - [x] `-I` flag: exit 0 even with diffs. - [x] `-E` with no diffs: exit 0 (no escalation triggered). - [x] `-E` with new images: exit 1. - [x] `-U`: `expected/` is updated to match `actual/`, exit 0. - [x] `--help`: prints usage and exits 0. - [x] Missing positional args: clear error + exit 1. ## Not in this PR (follow-up) - `-F / --from` (JSON → report without re-running diff) - `--junit` output - `-X / --additionalDetection` - Library `start` / `compare` (per-file) / `update` events - `reg.json` / `junit.xml` written to disk by default - Default `--diffFormat png` (to fully match classic's output filenames) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to #583 (phase A). Library surface now matches classic reg-cli. - run() / compare() emit 'start', 'compare' (per-file synthesised), 'update', 'complete', 'error' - compare(input) handles update / junitReport as side effects after the Wasm run, instead of forwarding them to clap (which rejected them) - Translates threshold -> thresholdRate alias - New js/junit.ts writes minimal JUnit XML matching classic's schema - CLI --junit <path> flag wires the writer on complete Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to #583 and #584. Closes the last first-class compat gap with classic reg-cli: the JSON/HTML report now references PNG diff images by default and `reg.json` is always written to disk. ## What changes for users ### Default `diffFormat` = `png` Before: Wasm defaulted to WebP, so `failedItems` / `diffItems` in the JSON contained `*.webp` entries and the HTML report loaded WebP images. That's a real behaviour break for downstream consumers of reg-cli's JSON (reg-suit, reg-notify-*, custom bots). After: both `cli.ts` and `compare()` default `--diffFormat` to `png` when the caller hasn't specified it. Users who want WebP can still opt in with `--diffFormat webp` or `{ diffFormat: 'webp' }`. ### `reg.json` is always persisted Before: the Wasm side returned the JSON report as a string through the Worker `complete` message, but nobody wrote it. CIs that read `./reg.json` after running reg-cli saw nothing. After: both `cli.ts` and `compare()` write the report to the `--json` path (defaulting to `./reg.json`) immediately before emitting the external `complete` event. If the write fails, the library emits `'error'` instead of `'complete'`. ## Implementation - `js/cli.ts`: reads `--json` / `-J` (default `./reg.json`), reads `--diffFormat` (default `png`). Pushes the effective values to the Wasm argv and writes `reg.json` itself after complete. - `js/index.ts`: same defaults applied inside `compare()` before building the Wasm argv; `writeRegJson()` extracted and exported so `cli.ts` reuses the exact same persistence code path. - The previous fast-path in `compare()` that returned the inner emitter unchanged when no side effects were needed is gone, since writing `reg.json` is now an unconditional side effect. Cheap — one `writeFile` of ~KB JSON. ## Test plan - [x] CLI: diff images in diff dir are `*.png`, `reg.json` exists, `reg.json` contains `*.png` entries in `diffItems`. - [x] CLI explicit `--diffFormat webp`: diff images are `*.webp`, JSON references `*.webp`. - [x] Library `compare({ ... })` with no `json`: `./reg.json` written, diffItems end in `.png`. - [x] Library `compare({ json: '/path/out.json' })`: written to /path/out.json, parent dir created if missing. - [x] Writing to an unwritable path fires `error`, not `complete`. ## Still not in scope - `-F / --from` (JSON → HTML without running diff) - `-X / --additionalDetection` Both have limited blast radius (few users rely on them) and can land in a follow-up without blocking migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CLI compat (phase B): library events + --junit
CLI compat (phase C): default PNG + always persist reg.json
…junit + reg.json writes
Closes the remaining CLI gaps with classic reg-cli:
- `-F, --from <reg.json>`: new `reg_core::run_from_json()` re-renders the HTML
report from an existing reg.json without running any image comparison, and
the CLI makes positional dirs optional in this mode.
- `-X, --additionalDetection <none|client>`: plumbs
`Options.enable_client_additional_detection` into `ReportInput`, flipping
the HTML report's `ximgdiffConfig.enabled`. The report template's
worker_url is finally wired ("./worker.js" instead of the leftover
"TODO:").
- `--junit <path>`: moved JUnit XML generation into `reg_core`
(`build_junit_xml`) and the `run()` / `run_from_json()` entry points now
write it themselves. `js/junit.ts` is deleted; the JS side no longer
writes reg.json or junit.xml (both artefacts land inside the WASI
preopened root instead of via host-side fs).
JS side:
- `computeWasiSandbox` now also covers `--junit` and `--from` parents when
deciding the common-ancestor preopen root.
- `cli.ts`: new `-F/--from`, `-X/--additionalDetection`, `--junit` flags
forwarded to Wasm; drops the duplicate reg.json / junit writes.
- `compare()` in `index.ts`: remaps `junitReport` → `--junit`, translates
the historical `enableClientAdditionalDetection: true` to
`additionalDetection: "client"`, drops the JS json/junit writes, and now
only keeps the `update` shim (file-copy still needs host fs).
Smoke-tested end to end against `js/sample/` with both diff + `-F`
regeneration; reg.json / report.html / junit.xml now all come from Rust.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CLI compat (phase D): -F/--from, -X/--additionalDetection, Rust-side junit + reg.json
…gration tests ## Fix: junit XML is now byte-identical to classic reg-cli Phase D (#586) shipped a simplified junit shape (`<testsuites>` bare, `classname="reg-cli"`, `message="changed|new|deleted"`) that diverged from what classic reg-cli produces via xmlbuilder2 — which is exactly what downstream CI parsers and `test/cli.test.mjs` snapshot-check. Fixed to the classic schema: - `<?xml version="1.0"?>` (no `encoding` attr) - `<testsuites name="reg-cli tests" tests=N failures=M>` (attrs on BOTH testsuites and testsuite) - `<testcase name="...">` (no `classname`) - `<failure message="failed"/>` for failedItems, `"newItem"`/`"deletedItem"` for new/deleted ONLY when `-E/--extendedErrors` is set - Without `-E`, new/deleted items are counted as passed testcases (classic behaviour) To make that last branch possible, `extended_errors` is now plumbed through `Options` → `reg_cli` clap (`-E/--extendedErrors`) → `compare()` library forwarding → `build_junit_xml`. The JS CLI wrapper still owns exit-code semantics for `-E`, but the flag is now dual-forwarded so Rust can also see it. ## New tests Rust (`cargo test -p reg_core --lib`, 6 new tests): - single failure - passed + failed mix - new/deleted with and without extended_errors (both branches) - XML attribute escaping (&, <, >, ") - empty report → self-closing `<testsuite/>` JS (`node --test`, 19 tests across two files, no new deps): - `js/test/cli.test.mjs` — spawns `node dist/cli.mjs`, asserts exit codes (`-I`, `-E`), reg.json schema, JUnit XML **byte-for-byte match** to classic, `-R`, `-X client`, `-F` (regenerate from reg.json, verifies source reg.json is immutable and no diff dir is recreated), stdout formatting, `-D` custom trailer. - `js/test/library.test.mjs` — `compare()` EventEmitter lifecycle (`start`→`compare(xN)`→`complete`), JUnit + reg.json via Rust, `update: true` file copy + `update` event, `additionalDetection: 'client'` and the legacy `enableClientAdditionalDetection: true` alias. Notable compatibility finding documented in the library test header: `wasm32-wasip1-threads` libstd's prestat enumeration only honours the first path segment of a preopen name. `computeWasiSandbox` collapses every touched dir to a single common ancestor, so when all positional dirs live under one scratch (update mode), that scratch must itself be one segment deep at the repo root. The library test scaffolding uses `.libtest-<pid>-<n>-<rand>` for exactly that reason. ## CI New `wasm-test` job runs `cargo test -p reg_core --lib`, then builds the js dist (using the committed `js/reg.wasm`, no wasi-sdk needed) and runs `pnpm --filter ./js test`. The classic `test` job is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`reg_core` embeds the report UI bundle via include_str!:
let js = include_str!("../../../report/ui/dist/report.js");
let css = include_str!("../../../report/ui/dist/style.css");
So `cargo test -p reg_core --lib` fails at macro expansion unless
`report/ui/dist/` has been populated. Run `scripts/build-ui.sh v0.3.0`
(same version the root `build:report` npm script uses) before the cargo
test step, and enable corepack so the yarn that script invokes is
available.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CLI compat (phase E): byte-for-byte junit + Rust unit tests + JS integration tests
…ion, small-set concurrency
Closes the remaining classic-reg-cli gaps from the phase-E audit. One
notable item (N9, live per-file `compare` events during the diff loop)
is deferred as a follow-up — it requires plumbing a stderr-parsing event
channel between Rust/Wasm and the worker and is intrusive enough to
deserve its own change.
## B1 — `-X client` now emits the browser-side assets
`ximgdiffConfig.enabled=true` alone is a no-op without a worker script
and wasm binary next to the HTML report. Matching classic reg-cli
(src/report.js:158-163):
- New `js/ximgdiff.ts` concatenates `worker_pre.js` (mustache-rendered
with the wasm URL) + the report-ui `worker.js` + the x-img-diff-js
loader and writes `<report-dir>/worker.js` + `detector.wasm` (the
x-img-diff-js wasm binary, renamed).
- `js/cli.ts` and the library's `compare()` both invoke it when
`-X client` is paired with `-R`.
- `x-img-diff-js` is now a runtime dep of `@bokuweb/reg-cli-wasm`.
- unbuild hook in `build.config.ts` stages `worker_pre.js` and the
report-ui worker into `dist/shared/` at build time; `writeXimgdiffAssets`
takes an explicit `distDir` from the entry module's `dir()` so chunk
splitting doesn't break asset lookup.
- `compare()` lazy-loads ximgdiff (dynamic import) to keep the cold path
for users who never pass `-X client`.
## B2 — `-U/--update` now matches classic's prune-then-selective-copy
Classic (`src/index.js:134-146`):
1. rm from expected: deletedItems ∪ failedItems (so stale baselines
don't linger and overwrites are clean).
2. copy actual→expected: newItems ∪ failedItems.
3. passedItems untouched (preserves mtime, keeps git status clean).
Previously we copied every `actualItems` entry, which (a) never pruned
deleted baselines (they accumulated in expected/ forever) and
(b) needlessly rewrote unchanged files, breaking reg-suit workflows that
rely on a clean tree. Implemented in both `js/cli.ts` and `js/index.ts`.
## N2 — Force concurrency=1 when < 20 images
Classic (`src/index.js:77`) short-circuits the per-image parallelism for
small sets because the ProcessAdaptor spin-up dominates. In our Rust
implementation the equivalent is the rayon thread-pool + cross-thread
span propagation. Apply the same < 20 heuristic in
`crates/reg_core/src/lib.rs`.
## N3 — Favicon data URL in HTML report
Classic embeds the PNG as a data URL so the generated HTML is
self-contained. `reg_core` now `include_bytes!`es
`report/assets/favicon_{success,failure}.png`, base64-encodes at render
time, and fills the existing `{{&faviconData}}` template slot. Switches
on the report status (Danger → failure favicon, Success → success).
## N4 — `--version` reads from package.json
Was `reg-cli-wasm\n` placeholder. Now uses `createRequire` to read the
adjacent `package.json` so reg-suit's `reg-cli --version` probe works.
## Tests
- `cargo test -p reg_core --lib` — 10 passing.
- `pnpm --filter ./js test` — 24 passing. New cases:
- `--version` prints semver-shaped string
- HTML report embeds a favicon data URL
- `-X client` writes worker.js (>10KB) + detector.wasm (>100KB)
- `-U` prunes deleted baselines, preserves passed-file mtime
- library test: `-U` prunes stale baseline via `compare()`
## Notable deferrals
- **N9** (live `compare` events): reg-suit / spinners currently see all
per-file events fire synchronously just before `complete`, not as each
diff completes. Fixing this needs a WASI-stderr event channel
(`REG_CLI_EVT:type:path` lines captured by `printErr` and forwarded via
`parentPort.postMessage`). Non-trivial and deserves its own PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CLI compat (phase F): -X client assets, -U semantics, favicon, --version, small-set concurrency
## N9 — Live per-file `compare` events during the diff loop
Classic reg-cli's `ProcessAdaptor` fires `emitter.emit('compare', ...)`
as each image completes, so spinners/progress bars animate. Phase E's
wasm port synthesised all events in one burst *after* `complete`, with
a comment admitting the limitation. Fixed by streaming them through a
tagged WASI stderr channel:
- `crates/reg_core/src/lib.rs` emits `emit_progress(kind, path)` which
writes `__REG_CLI_EVT__\t{"type":"…","path":"…"}\n` to stderr. Fired
from:
- `find_images` post-detection for `new` / `delete` items.
- inside `par_iter` immediately after the pixel-diff completes and
threshold classification runs (classification was moved into the
closure so events fire on whichever rayon thread did the work, not
serialised after collect).
- New `js/progress.ts` (`createPrintErrHook`) parses those lines out of
the WASI stderr stream, forwarding progress events to the caller and
passing everything else through to `console.error` so real diagnostics
still reach users. Piggybacks on `@tybys/wasm-util`'s
`StandardOutput.write` already being line-buffered.
- `js/entry.ts` + `js/worker.ts` each install `printErr` on their WASI
instance and forward events via `parentPort.postMessage({ cmd:
'compare-event', event })`.
- `js/index.ts` `run()` handles `compare-event` messages from both the
main entry worker AND every spawned rayon thread worker, emitting them
live on the outer EventEmitter.
- The old post-complete batched emission is removed — `emitter.on('compare',
…)` now sees events live, same as classic.
New library test `compare() emits compare events LIVE, before complete`
asserts the ordering so we don't silently regress.
## M1 / M2 — Drop the `.expect("TODO:")` and the stale input-validation TODO
`create_dir_for_json_report` previously `.expect`ed on `url::Url::join`
failures, which would crash with the useless panic message `"TODO:"` if
the `--urlPrefix` value ever produced an unjoinable URL (unlikely after
clap parses it as `url::Url`, but still). Swapped for a graceful fallback
to the relative path + a `tracing::warn!` log, matching classic
reg-cli's silent-fallthrough behaviour. The adjacent
`// TODO: please validate input on cli input.` comment is removed as the
edit subsumes it.
## Tests
- `cargo test -p reg_core --lib` — 10/10.
- `pnpm --filter ./js test` — 25/25 (one new: live-event ordering).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CLI compat (phase G): live compare events + .expect("TODO:") cleanup
…lent find_images==[] for multi-segment preopens
## The bug
When `computeWasiSandbox` registered a preopen whose mapped path had more
than one `/`-separated segment, `find_images` returned ZERO results —
silent data loss: reg.json has every list empty, exit code 0, HTML says
"success". CI goes green while no images were compared.
Reproduction:
preopen | find_images result
---------------------------|-------------------
./repro (1seg) | ✅ images found
./repro/inner (2seg) | ❌ empty set
./repro/a/b/c (3seg) | ❌ empty set
Who hit it:
- `reg-cli packages/app/screenshots/actual …` (monorepos) 💥
- `reg-cli /Users/alice/project/screens/actual …` (absolute paths) 💥
- `reg-cli screens/actual …` (flat) ✅
## Root cause (properly diagnosed this time)
Not wasi-sdk. Not @tybys/wasm-util. Not Rust toolchain. All three were
tested (wasm-util 0.9.0 → 0.10.1, Rust nightly-2025-01-01 →
nightly-2026-04-18, Node 20 → 22) with no change.
The actual culprit: **the `glob` crate**. For pattern
`deep/a/b/actual/**/*`, glob walks from cwd opening each intermediate
directory (`.` → `deep` → `deep/a` → `deep/a/b` → `deep/a/b/actual`).
Under our WASI sandbox the preopen IS `./deep/a/b` — so `.`, `deep`, and
`deep/a` are OUTSIDE the sandbox. `read_dir(.)` fails with EBADF, glob
silently swallows the error (`.flatten()` in our filter code also
contributed), and the iterator produces zero items.
Verified by instrumenting entry.ts's WASI imports: direct
`std::fs::read_dir("deep/a/b/actual")` SUCCEEDS via `path_open(fd=3,
"actual")`. Only glob's walk-from-cwd strategy fails.
## The fix
Replace `glob::glob` in `find_images` with a direct recursive
`std::fs::read_dir(root)` walker (`walk_images`). Starts AT the
sandboxed directory instead of traversing to it. No intermediate
`read_dir(".")` calls. Multi-segment preopens now Just Work with the
original narrowest-ancestor sandbox — no widening, no tradeoffs, no
warnings.
Also drops the unused `glob` and `path-clean` crates from
`crates/reg_core/Cargo.toml`.
## Tests
`js/test/cli.test.mjs`:
- `multi-segment positional dirs still discover images` — deep path
(`.phase-h-deep/<pid>/nested/level/...`), asserts `actualItems` is
populated despite the preopen being 4+ segments deep.
- `multi-segment positional dirs: nested subdirs still discover
images` — image at `actual/sub/a.png` under a multi-segment preopen,
verifies both recursion AND the actual_dir-relative path in reg.json.
Full JS: 27/27 passing. Rust: 10/10 unchanged.
## Why this PR is better than the earlier "truncate to first segment" attempt
The first revision of this PR shipped a JS-side workaround that
truncated multi-segment preopens to their first segment (widening the
sandbox — `./packages/app/screens` → `./packages`, or even
`/Users/alice/…` → `/Users`). That avoided the bug but exposed strictly
more of the host than needed.
The direct walker approach fixes the ACTUAL bug: `find_images` now works
correctly against the narrowest-ancestor preopen, and monorepo / absolute
path users get the same sandbox privileges as flat-layout users.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CLI compat (phase H): replace glob::glob with direct walker to fix silent find_images==[] for multi-segment preopens
…liAdditionalDetection before forwarding
## The bug
reg-viz/reg-suit's `packages/reg-suit-core/src/processor.ts:105-116`
invokes `compare({…})` with two keys the Wasm port's Rust clap layer
does not recognise:
compare({
actualDir, expectedDir, diffDir, json, report,
update: false,
ignoreChange: true, // ← not a Rust clap flag
urlPrefix: '',
…
enableCliAdditionalDetection: true, // ← not a Rust clap flag
enableClientAdditionalDetection: …,
…
})
`compare()` was forwarding unknown keys verbatim as `--foo value` pairs.
When reg-suit calls us, Rust clap aborts with `error: unexpected
argument '--ignoreChange' found` before the diff loop even runs.
## The fix
Add both to `CLI_ONLY_KEYS` in `js/index.ts`. Semantically both are
no-ops at the EventEmitter layer:
- `ignoreChange` only governs classic reg-cli's process exit code; the
library never needed it.
- `enableCliAdditionalDetection` was classic's flag for running an extra
CLI-side x-img-diff pass during the diff. Our Wasm pipeline already
produces the final pass/fail classification, so toggling it changes
nothing. (The *client*-side variant is handled separately via
`additionalDetection: 'client'` / the legacy
`enableClientAdditionalDetection: true` alias.)
Both keys also added to the `CompareInput` TS type so TypeScript users
can pass them without a `// @ts-ignore` dance.
## Tests
New `js/test/library.test.mjs` case `reg-suit compat: compare() accepts
reg-suit-shaped options without aborting` — constructs the exact option
bag reg-suit's `processor.ts` passes and verifies `complete` fires
cleanly with the expected reg.json. If someone later removes a key from
`CLI_ONLY_KEYS`, this catches it.
Full JS: 28/28 passing. Rust: 10/10 unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CLI compat (phase I): reg-suit drop-in — strip ignoreChange + enableCliAdditionalDetection
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 does this change?
A clear and concise description of what the changes is.
References
Screenshots
If applicable, add screenshots to help explain your changes.
What can I check for bug fixes?
Please briefly describe how you can confirm the resolution of the bug.