Conversation
Signed-off-by: Logan Johnson <loganj@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source review via Wes’s account
No actionable findings; no changes requested. This is a bounded, test-only fixture optimization: one file, no production changes, no new/removed browser cases, and no timeout increase.
- Head:
1333a765c34b9e1285d2b98392a6259498809453 - Base:
ea7ddb81aebd6da9dd832378aa19df1bff8d7ac2
Coverage assessment
The fixture still selects from 501 distinct modeled hot records plus the older quiet mention using the actual outgoing filters, descending timestamps and each filter’s limit. Collapsing the request back into the former shared multi-channel filter would exclude quiet and fail both the new 501-delivery assertion and the retained quiet-message assertions. That is source reasoning, not an executed mutation test.
The repeated hot representative retains a real signature and crosses JSON/socket verification on every delivered frame. live.ts:729–797 counts accepted replay frames before the session’s event-ID deduplication; unread.ts:694–731 deduplicates retained evidence. Thus the test still reaches the replay cap, observes quiet unread/attention, suppresses replay notifications, and checks conservative limited status and disposal. It deliberately no longer exercises 500 distinct retained hot messages, but unique-message cardinality is not the assertion this test promises. The adjacent capped-replay test already uses repeated valid frames. No weakened assertion or replaced production lifecycle was found.
Hosted evidence, not local execution
The reported failing main job checked out the pinned base. The PR JavaScript job checked out synthetic merge d7d10ad41bbe4b32f2b46ed6febc6bb5baaf1661, explicitly combining this head and base.
| Evidence | Main/base | PR merge |
|---|---|---|
| Tests | 4,949 pass / 1 timeout | 4,950 pass |
| Files | 411 pass / 1 fail | 412 pass |
| Vitest elapsed wall time | 346.12 s | 341.22 s |
| Summed test execution | 607.05 s | 596.46 s |
live.test.ts |
10.490 s | 6.095 s |
| Changed quiet-channel case | 5.963 s, 5 s timeout | 1.796 s, pass |
The slowest file in both runs was unread-startup.test.ts (26.067 → 27.157 s). The slowest individual test was the 2,400-row send/acknowledgement case on main (14.306 s), versus ordinary reads across growth/history/restart on the PR (13.485 s). These are two hosted observations under parallel load, not a controlled benchmark or proof that all timing flakes are eliminated.
One current-head CI snapshot showed CI required, JavaScript, Rust/tool integration, all six browser shards, measurements, security checks and DCO passing; Windows native validation was skipped. No polling or reruns.
Limits and publication check
Source-only: 1,638 archived source entries verified against the pinned Git tree and rechecked unchanged, with no live-worktree/dirty inputs. No PR code, tests, builds, installs, app launches or live-relay writes executed by this reviewer. The public description, sole commit message and changed source were inspected; there are no attached images and no public-material finding. This change has no UI or error/retry focus transition; runtime/human acceptance is not established by this review.
COMMENT only—not approval or merge authorization.
|
This conflicts with the overlapping timeout fix already merged in #378 ( #378 moves the expensive signed-fixture construction into I recommend closing this PR as superseded rather than dropping that coverage to resolve the conflict. This is a source-level compatibility finding, not a claim that all current main tests pass. No source changes or push made; leaving closure to the maintainers. Carl, an automated reviewer, commenting via Wes’s GitHub account. |
Root cause
The main JavaScript job failed only
src/features/relay/live.test.ts > keeps quiet-channel unread evidence when another filter fills its replay allowance:Test timed out in 5000ms. Its 501 distinct signed hot-message fixture nearly consumed that timeout on the previous successful main run (4,332ms); the intervening main commit changed agent setup, not this path. This is a CPU-sensitive test fixture under CI parallelism, not an observed product regression.Fix
Model 501 distinct hot relay records for the per-filter allowance, then replay one valid signed hot representative for each selected wire frame. The existing live/session boundary still handles 500 verified hot frames and the real signed quiet mention; assert exactly 500 hot frames plus quiet, quiet unread attention, no spurious incoming notification, and conservative limited replay. Avoid 501 unnecessary signing operations, without increasing test timeouts or disabling assertions. Only
src/features/relay/live.test.tschanges.Validation
bin/pnpm exec biome check --error-on-warnings src/features/relay/live.test.ts— passed.bin/pnpm typecheck— passed.bin/pnpm exec vitest run src/features/relay/live.test.ts— 59/59 passed (2.41s file; before edit the named test alone took 1.41s locally, after 0.59s).BUZZ_TEST_WORKERS=2) — passed; pre-push selected 60/60 related tests plus type/design checks.Full Vitest/CI is left to PR checks; no live relay exercised because this is a test-only fixture change.
Signed-off-by: Logan Johnson loganj@squareup.com