fix: repair what a full verification pass found by ribdsp · Pull Request #6 · ribdsp/Traces

ribdsp · 2026-08-28T03:07:53Z

A verification pass over the whole repository, one item at a time, with every claim checked
against a command rather than against memory. Twelve commits, grouped below by what kind of
wrong they fix. Two of them are demo blockers.

Demo blockers

052ea0e — the app could not open its own recordings. loadRecording expected a bare
array of rrweb events; the recorder writes { meta, events }. Every one of the three
committed recordings was therefore unloadable, and 257 passing tests missed it because they
all constructed event arrays inline and never read a file. Fixed by adding a
loadRecordingFile seam that unwraps the envelope and delegates to a loadRecording that
stays strict about what it accepts, so the validation the tests cover is the validation that
runs.

748da95 — a seek before the first full snapshot left a stale DOM. rrweb cannot
reconstruct a moment it has no snapshot for; asked to, it leaves the previous frame in the
iframe. The playhead said 400ms, the DOM was whatever 30s looked like, and nothing reported
an error. Measured in a browser rather than reasoned about — the DOM goes stale, not blank,
which is why this survived so long. Now clamped to the snapshot.

Wrong answers from correct-looking code

9829532, ae02dc3 — two call sites inherited a 40-item budget meant for agent
responses. DIGEST_LIMIT = 40 exists to protect an agent's context window. The bug report
builder and the timeline event band both called through it, so a report described the first
40 events of a recording and the event band painted the first 40 events across the full
width — a timeline that looked complete and was a fifth of the recording. Both now pass
limit: Number.MAX_SAFE_INTEGER, which is the honest way to say "this consumer is not an
agent".

Documentation that had drifted from the code

e3d346d — "six probes" was wrong in seven places, two of them checkable. Measured: a
default-precision search over a 47-second window costs exactly ten probes — eight
halvings plus the two boundary probes — and ten wherever the transition sits, because a
binary search pays the same price everywhere. The README's example output claimed six with a
trace that had no boundary probes in it, and its transcript quoted a firstTrue that the
halving lattice for that window cannot produce. Both now carry measured values. The test's
assertion was deliberately left alone: the ceiling is the specification, and tightening it to
exactly ten would encode one window's arithmetic.

8110369, 4401f4f — three "not implemented yet" claims that were implemented. The
network-request custom event, the sixteen tools, and the TODO markers. All present, all
verified: 285 tests green across 22 files, zero TODO( markers left in either app. Each note
was corrected while keeping the rule that outlived it — including the prohibition on making a
specification suite pass by weakening it, which now reads as a standing rule rather than a
description of red tests. 4401f4f also documents why output: 'export' must never be added:
it would silently stop the Origin-Trial header, which is the one change that breaks WebMCP
without breaking the build.

The last placeholder in the design

1e6f7b8, 06e1c06 — the type pair. IBM Plex Sans and Plex Mono, chosen for a mechanical
reason rather than a taste one: this UI mixes mono into sans on the same line constantly, and
two families with different x-heights put a visible step in every such row. Plex Sans and
Plex Mono share metrics. Verified end to end — the woff2 files are served from this origin,
and no file in the build references fonts.gstatic or fonts.googleapis.

Hygiene

7cbe396 — every advisory cleared, and npm ci works for the first time. Both fixes are
narrower than the ones npm proposes: the postcss advisory is against a copy nested under
next, so an overrides entry pins the tree instead of taking a Next major; vitest moves to
4.x because that is where the critical advisory is fixed, and all 285 tests pass on it with no
test changed. npm ci had been failing in both apps on a lockfile that had drifted from its
manifest. Also pins outputFileTracingRoot, since Next was inferring it from a stray lockfile
outside the checkout.

b01bd77, 46090e3 — personal names out of code comments, agent working directories
ignored.

Test plan

The repository is going open source, and per-file ownership headers in the comments do not survive that transition: `Owner: X.`, `Implemented — x, Day n:` and `TODO(x), Day n:` name people who are no longer the only readers. Comments only. Every changed line is inside a comment block, verified with: git diff -U0 | grep -E '^[+-]' | grep -vE '^(\+\+\+|---)' \ | grep -vE '^[+-]\s*(\*|//|/\*\*)' which returns nothing. Gates unchanged from before the pass: tsc --noEmit exit 0, 19 files / 257 tests passing. What was kept deliberately: - credit lines in LICENSE, README.md and CONTRIBUTING.md — those are meant to carry names - git log and git blame — history is not rewritten, and commit authorship is legitimate contribution record for an open-source project - both live TODO markers, still greppable as `TODO:` with their description intact — deleting a marker while implementing around it would destroy the only record of outstanding work Technical content in the headers was preserved rather than dropped: a header reading `Owner: X, over Y's lib/z.` becomes `Wraps lib/z.`, so the dependency it documented survives the loss of the two names.

`.claude/`, `.cursor/` and `.codex/` hold per-machine agent configuration, and `.playwright-mcp/` collects console logs and page snapshots from browser automation runs. All four are local working state that would otherwise show up as untracked noise in every `git status`.

Clicking any sample in the picker failed with "Recording is invalid: expected a JSON array of rrweb events." Every committed recording is unloadable, which makes the whole app unusable, and the test suite was 257 green. The pipeline persists `loadRecording`'s output and feeds it back to `loadRecording`, which only accepts `loadRecording`'s *input*. `bugbait/src/lib/record.ts` downloads the serialised `Recording` — `{ id, label, events, startedAt, durationMs, meta }` — and the three files in `public/recordings/` are exactly that. Nothing caught it because every other test builds its events in memory, so no test had ever read one of those files. Adds `loadRecordingFile`, a narrow adapter that decides *where the events are* and delegates every question of whether they are valid. Unwrapping does not go into `loadRecording`: it validates untrusted input (docs/threat-model.md T6), and its strictness is why a truncated file fails at load with a readable message instead of on the fourth bisect probe. The new module is also the first documented home the file format has had. Only `events` is read from the wrapper. `startedAt`, `durationMs` and `meta` are recomputed from the events even though the file carries them, because the recorder writes a deliberately partial `meta` — `{ userAgent, viewport }` — so its `eventCount`, `navigations` and `counts` are `undefined` on disk. Forwarding it would satisfy `RecordingMeta` and hand every downstream tool a contract with holes in it. Tests, red before the fix and green after (12 failures, all "expected a JSON array of rrweb events"): - load-recording-file.recordings.test.ts runs all three real files through the exact function the picker calls, with ids from SAMPLE_RECORDINGS so a manifest entry without a committed file turns red instead of shipping a button that throws - load-recording-file.test.ts pins the shapes those three do not exercise: a bare array, a wrapper whose precomputed fields disagree with its events, and the errors for input that is neither The picker is in this commit rather than its own because the seam and its only call site are one change; it loses the temporary `eventsOf` helper that duplicated the unwrapping inline, where no test could reach it. Suite: 21 files, 278 tests, all passing. `tsc --noEmit` clean.

`gotoTime(atMs)` passed `atMs` straight to rrweb. rrweb replays a seek by applying every event *strictly before* the target and clearing the queued remainder on `pause()`, so a target at or below the first full snapshot's own timestamp applies nothing — including the snapshot that rebuilds the page. The iframe does not go blank, which is what made this hard to see. rrweb rebuilds the first snapshot from a timer in its own constructor, and a backward seek's `mirror.reset()` clears the id-to-node map rather than the document, so the iframe always holds a page. A seek that applies nothing just leaves the moment rendered last. `read_dom_at(0)` therefore answered with whatever the previous tool call had looked at: right by luck on the first call of a fresh engine, wrong on every call after any other seek. Measured in a browser against all three committed recordings, on the rrweb build in node_modules. Each probe was preceded by a seek to 2500 ms — past the first mutation, ~2040 ms in all three — so a target that rebuilds nothing leaves a trace. For `empty-province` (cart quantities 1/2/1 at the start, 1/3/1 after the click at ~2.5 s): asked for body chars quantities rewound? 0 … 21 3349 1/3/1 no — still showing 2500 ms 22 3314 1/2/1 yes 30, 100 3314 1/2/1 yes The boundary is exactly the snapshot's own offset in each recording: stale through 21 for `empty-province` (snapshot at 21 ms), through 17 for `race-condition` (17 ms), through 20 for `overlay-blocks-button` (20 ms), and correct one millisecond later. With the clamp in place no offset was stale in any of the three. That window is 17-21 ms wide rather than zero because rrweb stamps the Meta event at offset 0 and walks the document afterwards, and `read_dom_at(0)`, `diff_dom(from: 0)` and `bisect(from: 0)` all land inside it. So the clamp is `firstFullSnapshotMs + 1` — the exclusive bound is why `+ 1` is needed, since `21 < 21` is false. It only moves a seek that would otherwise land before the recording's first frame; a seek that is already answerable is left alone, so the deliberate non-workaround for the checkout gap documented in `gotoTime` still stands. `earliestSeekableMs` is exported and tested on its own. `createReplayEngine` stays untested for the reason already recorded on it — jsdom has no iframe document lifecycle — but the arithmetic is pure, it is what a change to the recorder's snapshot timing would silently invalidate, and the test pins the per-recording values the browser runs produced. `SEEK_TOLERANCE_MS` in `use-playhead.ts` is 40 ms and already absorbs a clamp of at most 22 ms, so the player's own seek accounting needs no change.

…0 events `buildReport` called `buildEventDigest(recording)` with no limit, so it got the default `DIGEST_LIMIT` — the earliest 40 digest events. That default is right where it comes from: `list_events` is a browsable list and 40 is one page of it. A report is not a page. It is a claim about the whole session, and truncating the evidence it is checked against has two consequences, both silent: - `reconcileStep` marks a proposed step that cites a real event past the cap as `verified: false`. The step is a true statement about the recording, and the report calls it unsupported. - `synthesizeStepsFromDigest` stops at whatever happened in the first stretch of the recording, so a report with no model-proposed steps ends before the bug. Both are worse than a missing feature, because the report still looks complete. `verified: false` is this module's way of saying "look here" — spending it on events that were merely truncated is how that signal stops meaning anything. Fixed by passing `limit: Number.MAX_SAFE_INTEGER`, the same override `list-events.ts` and `read-console.ts` already use when they need the true set before applying their own cap. There is no cap to apply afterwards here: `STEP_EVENT_KINDS` does the narrowing, and in these recordings the events worth reporting are the late ones. Both tests were run red first, against a 51-click fixture whose last click sits past the cap and further than `MATCH_WINDOW_MS` from every event that survives truncation, so nothing can verify it by matching a nearer neighbour: × verifies a proposed step against a late event, not only the ones inside the digest default - "atMs": 9000, - "verified": true, + "verified": false, × synthesizes steps from the whole recording, not just its first forty events expected [ …(40) ] to have a length of 51 but got 40 The other call site with the same shape is `event-track.tsx`; it belongs to the timeline area and is a separate commit.

…irst 40 events `EventTrack` called `buildEventDigest(recording)` with no limit, so it painted the earliest `DIGEST_LIMIT` (40) digest events and nothing after them. Measured against the three committed recordings: empty-province 40 of 73 ticks — band ends at 23184ms of 45052ms dropped: 26 input, 5 click, 1 consoleError, 1 rageClick race-condition 40 of 75 ticks — band ends at 23170ms of 44085ms dropped: 27 input, 7 click, 1 consoleError overlay-blocks-button 40 of 67 ticks — band ends at 23170ms of 44040ms dropped: 22 input, 4 click, 1 rageClick The second half of every sample session had no marks at all, and two of the three dropped their console error — the one tick this band exists to make findable, painted last precisely so it survives a crowd of clicks. A truncated list looks truncated; a truncated timeline does not. `list_events` says `truncated: true` and offers a narrower window, so 40 there is a page size. Here there is no such affordance: an empty right half reads as a session where nothing happened after the halfway point, which is a wrong answer rather than a partial one. Fixed with the same `limit: Number.MAX_SAFE_INTEGER` override `list-events.ts` and `read-console.ts` use when they need the true set. Nothing about the band needs a cap — a tick is 1px and the axis is the whole recording. Verified via a throwaway vitest run over the real recordings for the counts above (not committed; every test in this repo lives under src/lib/ and there is no component test harness). `npx tsc --noEmit` clean, 285 tests green.

…become one Five places described Traces as a static export. It is not one, and the mistake is a trap rather than a wording slip: `next.config.mjs` delivers the WebMCP origin-trial token through `headers()`, and `output: 'export'` has no server to send headers from. Adding it would leave the build green, the page working, and WebMCP quietly falling back to the polyfill on a host that could have run the real API — the exact failure the config comment already warns about for `<meta>` tags, arriving by a different door. The README made the contradiction explicit, listing "static export" and "response headers for the origin trial token" as reasons for the same dependency, one line apart. What is actually true, from `next build`: Route (app) Size First Load JS ┌ ○ / 12.9 kB 207 kB └ ○ /_not-found 995 B 104 kB ○ (Static) prerendered as static content Prerendered at build time, served by a Node host. So the hydration reasoning those comments were built on still holds — a first-render `Date.now()` or `matchMedia` read is still something the prerendered HTML could not contain — and only the mechanism named was wrong. Each comment keeps its argument and loses the false premise. `sample-recordings.ts` needed a different correction: its "there is no server at runtime" was load-bearing for why the manifest is hardcoded, and there *is* a server. The honest reason is that nothing lists `public/recordings/` — the picker is a client component fetching static files — and adding a route handler to enumerate three committed files is not worth it. `next.config.mjs` gets the guard, because it is the file that would break. One commit rather than five: this is a single false statement with five copies, and splitting it by area would leave the repo contradicting itself in between.

A default-precision search over a 47-second window is eight halvings plus the two boundary probes, and ten wherever the transition sits. Every claim of six was wrong by the same amount, including the README's example output and the transcript's reachable timestamp.

The recorder that emits network-request events exists, all 16 tools are implemented, every TODO marker is discharged and the whole suite is green. Each of those was still documented as pending somewhere, which makes a reader distrust the rest of the file. Rules that outlive the state they described - never weaken a specification suite, never delete someone's marker - are kept.

The config shipped a placeholder that resolved to system-ui, which means the interface looked like a different product on every machine. Plex is one superfamily: this UI mixes mono values into sans lines constantly, and matched metrics are what keep those rows flat. Loaded through next/font, so the files are served from this origin and no request reaches Google at runtime.

next/font puts the woff2 files in the deployed bundle, which is redistribution under OFL-1.1 rather than an npm dependency. Say where the licence text lives and that no face is modified.

Three things that were quietly broken at once. `npm audit` reported 7 vulnerabilities in traces, one critical. Both fixes are narrower than the ones npm offers: - postcss (high, <=8.5.22) is vulnerable in a copy nested under `next`, not in the one either app depends on directly, so raising the direct range fixes nothing. `npm audit fix --force` proposes next@16 — a major for a one-line problem. An `overrides` entry pins every copy in the tree to ^8.5.26 instead, in both apps. - vitest (critical, <=3.2.5) and vite (high, <=6.4.2, dev-only) are only fixed in vitest 4.x. All 285 tests pass on 4.1.11 with no changes to any test, which is the result worth reporting: the suite did not need to be talked into agreeing. Both apps now report 0 vulnerabilities. `npm ci` failed in both apps on "Missing: @emnapi/runtime@1.11.3 from lock file" — the lockfiles had drifted from their manifests, so the one command CI would run was the one command nobody could. Regenerating them fixes it; `npm ci` is green in both for the first time. Also two smaller things the above surfaced: - vitest.config.ts becomes .mts. Vite warned that ESM in a .ts config inside a package without "type": "module" is being loaded as CommonJS and that its native loader will stop allowing it. Renaming also cut the suite from 18s to 6s. - outputFileTracingRoot is now pinned to each app's own folder. Next infers it by walking up until lockfiles run out, so a stray package-lock.json in a home directory silently made the build trace a tree well outside the checkout. Both builds are warning-free and `/` is still ○ (Static).

ribdsp added 12 commits August 28, 2026 08:50

docs: state the font licence precisely

06e1c06

next/font puts the woff2 files in the deployed bundle, which is redistribution under OFL-1.1 rather than an npm dependency. Say where the licence text lives and that no face is modified.

This was referenced Aug 28, 2026

chore(deps): bump esbuild and vitest in /traces #1

Closed

chore(deps): bump vite and vitest in /traces #5

Closed

chore(deps): bump postcss and next in /traces #3

Closed

chore(deps): bump postcss and next in /bugbait #4

Closed

ribdsp merged commit 7b11bf8 into main Aug 28, 2026
2 of 3 checks passed

ribdsp deleted the fix/pre-submission-verification branch August 28, 2026 03:09

Navigation Menu

fix: repair what a full verification pass found - #6

fix: repair what a full verification pass found#6
ribdsp merged 12 commits into
mainfrom
fix/pre-submission-verification

ribdsp commented Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ribdsp commented Aug 28, 2026

Demo blockers

Wrong answers from correct-looking code

Documentation that had drifted from the code

The last placeholder in the design

Hygiene

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant