fix: strengthen tests and remove duplicate coverage - #370
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d658ec5b10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { createServer } from "./vite-server.mjs"; | ||
| import react from "@vitejs/plugin-react"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import { test, expect } from "./source-fixture.mjs"; |
There was a problem hiding this comment.
Commit ae2446a29ce62082e435fef7c223417cb48e11a6 has no Signed-off-by trailer, so any PR containing it violates the repository's DCO requirement and cannot pass the required DCO verification. Add a valid trailer using the actual author's verified Git identity before pushing this commit.
AGENTS.md reference: AGENTS.md:L151-L160
Useful? React with 👍 / 👎.
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
d658ec5 to
21f09cf
Compare
Signed-off-by: Salman Mohammed <smohammed@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 in this revision.
Reviewed head 2252a0886319292223caa1ac2f16eec74aa338c4 against base ea7ddb81aebd6da9dd832378aa19df1bff8d7ac2 using Git-blob-verified, unchanged source snapshots.
- Traced the removed/relocated assertions to their remaining owners. The three deleted emoji helpers have no references in the reviewed head; provider precedence, save notices, optional-plugin persistence and owner-only storage coverage remain.
- Reviewed the real React lifecycle migration, including StrictMode cleanup, subscriptions, reading leases, navigation cancellation, history failure/retry and own-send positioning. Controlled jsdom geometry establishes policy, not browser layout acceptance.
- The strengthened reload test follows the newly installed source; the gated Goose child observes premature stdin closure before returning a response. Independent source review of the Rust lane also found no actionable defect.
- Browser fixture sharing preserves fresh per-test contexts and existing cases, including reaction error/retry focus transitions, success and cancellation. Reviewed the public description, all three commit messages and changed source/configuration/fixtures; no actionable disclosure finding. The description contains no image attachments.
Validation evidence and limits: I executed no PR code, tests, builds or app. The read-only hosted CI snapshot is associated with this head and reports JavaScript, Rust/tool integration, all six Chromium/WebKit journey shards, measurements and CI required successful; Windows validation was skipped. The downloaded Vitest evidence reports 4,951 passed, zero failed/skipped; wall time 414.77 s and summed case time 457.15 s. Slowest file was unread-startup (23.21 s); slowest case was the existing marker-deadline case (10.02 s). The JavaScript job took 464 s of its 600 s limit. This supports the reported completed run, not a controlled performance comparison or proof against future contention.
Author-reported local mutation/timing evidence was not independently rerun. Human testing, native/cross-platform acceptance and actual UI behavior remain unverified by this source-only review. This is a non-blocking COMMENT, not approval or merge authorization.
Signed-off-by: Salman Mohammed <smohammed@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 in the merge-resolution update.
Reviewed head 5674d7d7baa98066780cd0641633db59b8005317 against base 0c3a601bfee641d58bf3d8806730ffa398398464, using Git-blob-verified source snapshots with unchanged hashes and no dirty checkout inputs. This is a follow-up to the review at 2252a088: I separated imported main from the new conflict resolutions rather than reopening the previously reviewed implementation.
MessageRow.test.tsx:925–947preserves the safe-URL media-call assertion in its rendered owner; the image-strip, mixed-media ordering and unavailable-image cases remain alongside the migrated timecode/thread-target/focus assertions.ThreadPanel.test.tsxis byte-identical to the previously reviewed version. The conflict resolution retains its real React lifecycle, history failure/retry, successful positioning and navigation-cancellation coverage, with the row-only contracts inMessageRow.test.tsxrather than the shallow fixture.live.test.tsis byte-identical to the incoming base, preserving the stronger 501-delivery / 500-hot-message assertions and signature preparation outside timed replay. The two other overlapping agent-test files retain the same feature edits as before the merge.- Inspected the current public description and all four feature-commit messages/trailers, plus the changed publication surface. No actionable disclosure finding; the description has no image attachments. All four feature commits have DCO sign-offs.
Evidence and limits: No PR code, tests, builds or app were executed for this review. One read-only hosted CI snapshot for this head showed JavaScript and browser measurements successful; Rust/tool integration and all six browser journey shards were still running. Windows was skipped; DCO and the displayed security checks passed. I did not wait for or poll completion.
The downloaded JavaScript timing artifact reports 5,063 passed, zero failed/skipped, 344.91 s runner wall time and 387.84 s summed case time. Slowest file: unread-startup.test.ts (20.14 s); slowest case: the existing marker-read deadline case (10.02 s). The JavaScript job took 391 s. These are current-run observations, not a controlled speed comparison against the older, smaller test inventory.
Author-reported local checks were not independently rerun. Final CI completion, human testing, native/cross-platform acceptance and actual browser focus/layout remain unverified by this source-only follow-up. This COMMENT is not approval or merge authorization.
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review (via Wes’s account)
No new actionable findings in the four-shard CI follow-up.
Reviewed head 3ea2b0543b9099a395aac4a78eaecb342bdf7a99 against base 0c3a601bfee641d58bf3d8806730ffa398398464. This is a bounded follow-up to the review at 5674d7d7, not a repeat review of the unchanged test migrations. All 1,682 archived source blobs match the pinned Git tree; source hashes were rechecked unchanged, with no dirty checkout inputs.
- The matrix, displayed denominator and actual
--shard=N/4invocation agree. Existing engine selection, two workers, zero retries, timeouts, measurement isolation and strict required-check aggregation are unchanged. Artifact names remain engine/shard-specific. - The routing test expands the actual workflow command and checks its selections against unsharded discovery, rejecting missing, duplicated or measurement cases. The current hosted native job reports this guard passing with 112/102/110/103 cases per engine: 854 executions selected exactly once. This is discovery evidence, not 854 completed browser executions.
- The new commit changes only CI configuration, its routing guard and two documents. No browser case or UI lifecycle was changed. Separately checked retained reaction success/Escape-focus and error/rollback/retry-focus coverage (
tests/browser/reactions.spec.mjs:111–125,170–199,240–273); this follow-up does not establish live focus behavior. - Inspected the public PR description, all five commit messages/trailers and changed publication surface. No actionable disclosure finding in those surfaces; the description has no attached images/videos. All five commits carry DCO sign-offs.
Hosted evidence and limits
One read-only current-run snapshot showed JavaScript, Rust/tool integration, measurements and five of eight browser shards successful; three browser shards were still running. Windows was skipped. The native log’s merge commit 215fc0385ea82b540043b71741cbdd21d723757a has the exact reviewed head/base as parents.
Downloaded timing evidence for those five completed browser shards reports 529 passed, zero failed/skipped. Per-shard runner wall time was 195.45–440.60 s, and summed case time 346.71–793.45 s. Among those samples, the slowest file was Chromium message-navigation.spec.mjs (148.38 s summed); the slowest case was WebKit image-navigation blocked-input handling (38.29 s).
The prior three-shard run confirms four cancelled browser jobs and Rust-cache steps of 180–231 s. Its WebKit shard 2 completed all 141 cases in 590.25 s wall / 1,113.71 s summed case time, yet the overall job was cancelled. Shard membership and setup costs differ, so these partial samples do not establish whole-suite speedup, total runner-cost savings or final CI completion. The extra two runner setups are explicitly disclosed.
No PR code, tests, installs, builds or app workflows were executed locally. Author-reported local checks were not rerun. Final CI completion, human testing and native/cross-platform acceptance remain unverified. This non-blocking COMMENT is not approval or merge authorization.
Why
Several Buzz tests repeat stronger coverage, keep unused helpers alive, or simulate React instead of testing its lifecycle. Two regression tests also pass when their named behavior is broken: plugin reload can retain an old source, and Goose can close stdin too early.
What
Consolidate redundant coverage, mount the reading and thread tests with real React, and strengthen the reload and stdin tests. Remove three unused emoji helpers. Production: −56 lines; tests and support: +1,166/−1,448 lines, net −282. CI configuration: +8/−5 lines; CI documentation: +6/−6 lines.
How
Risk
Product behavior is unchanged; the removed emoji helpers have no product callers. The main review risk is lost assertions during test migration. Independent UI, Rust and browser/tooling reviews found no blockers.
Testing
Browser CI repair at
3ea2b054: use four file-level shards per engine instead of three. On5674d7d7, Rust-cache initialization took 180–231 s; four browser jobs hit the unchanged 15-minute limit. WebKit shard 2 passed all 141 cases before cancellation during cleanup (590.25 s test-run wall time; 1,113.71 s summed case time; slowest file navigation-sidebar 145.69 s, slowest case mute/read 37.50 s). The two completed Chromium shards took 583.97/379.97 s wall and 1,134.42/702.96 s summed case time. Cancelled shards have incomplete evidence, so these are not whole-suite totals. The new layout selects 112/102/110/103 cases per engine, all 854 executions exactly once. Both engines, measurements, two workers, assertions, retries and timeouts are unchanged. This adds two runner setups per run. All 11 CI-routing/reporting integration tests and the required 678-test push gate passed locally; independent review found no blockers. Hosted validation on3ea2b054: all automatic checks passed, including DCO and the strict aggregate. All 854/854 browser executions passed, zero failed/skipped. The slowest complete browser job took 676 s (11m16s), leaving 224 s under the unchanged limit. Windows remains manual-only.Hosted after timings: Ubuntu 24.04, Chromium and WebKit, two workers per shard,
pnpm test:browser:ci --project <engine> --no-deps --shard=N/4 --reporter=list,json, run by the existing timing wrapper. Setup took 62–88 s and complete jobs 270–676 s. Per-shard test-run wall time was 195.45–586.04 s; summed case execution across all eight shards was 6,197.26 s. Slowest file: WebKit nested-replies, 264.91 s summed; slowest case: crowded capped branches at 1492, 62.33 s. Setup was warmer than the failed run, so this is hosted completion/headroom evidence, not a controlled speedup benchmark. The earlier cancelled run cannot supply a complete before total.Conflict update: merged main
0c3a601bat5674d7d7. Retained both the image-strip coverage and the rendered row tests; carried the safe-URL media assertion into its new owner. Adopted main’s stronger replay test unchanged. All 174 focused tests across the three conflict files and React siblings passed; independent review found no blockers. The required push gate also passed 678 related tests plus TypeScript and design checks. Hosted evidence below belongs to the earlier snapshots. The merge passed JavaScript, Rust and measurements, but browser jobs exceeded their 15-minute limit.CI follow-up: the full JavaScript run timed out while preparing and replaying 501 signed hot-channel messages. The test now prepares signatures in a scoped
beforeAll, following the existing scale-fixture pattern. It retains the full dataset, real signature verification, assertions and default timeouts. The old combined-channel-filter mutation still fails the quiet-message assertion. All 74 live/event tests passed locally; the rebased branch’s push gate passed 712 related tests plus types/design checks.Local full-file wall time, including setup, stayed about the same (2.70 s before; 2.65 s after). Timed replay changed from 1.146 s to 0.544 s because preparation moved into the fixture; this is not a total-work reduction. The original CI test took 5.135 s and timed out. The next hosted run passed that replay test in 2.515 s, but a separate store-discovery setup hook timed out while signing 2,050 fixture events. JavaScript CI now uses the existing
BUZZ_TEST_WORKERS=2setting to bound contention. All test counts, signature checks, dataset sizes, timeouts and retries remain unchanged. The discovery fixture itself is unchanged. The two affected files pass 85 tests locally with two workers, and all six CI-routing tests pass. Hosted JavaScript validation passed on2252a088: 4,951/4,951 tests, zero skipped, including all 59 live and 26 store-discovery cases. Quiet-channel replay took 1.575 s. The full job took 464 s of its unchanged 600 s limit. Before/after the worker limit on the same code/test inventory: Vitest wall 360.35 → 414.77 s; summed case time 590.47 → 457.15 s. Before had 26 cases skipped by the setup failure; after ran every case. Slowest file: unread-startup 27.24 → 23.21 s; slowest case changed from durable read-state (14.58 s) to the existing marker deadline case (10.02 s). These are separate hosted samples, not a controlled benchmark. All hosted lanes and CI required passed on2252a088, including Rust/tool integration, all six browser shards, browser measurements, security scans and DCO. Windows validation was skipped by the existing workflow condition.Deliberate mutations demonstrated failure for missing reading dependencies, a stale reload source, and premature stdin closure. Restoring each implementation made its complete affected suite pass.
Focused checks passed: 339 Vitest tests across 11 files; 57 Rust tests; 13 Node integration tests; 48 browser executions across Chromium and WebKit. One existing native test remains ignored because it requires staged runtime resources. Typecheck and the required commit/push hooks passed.
Browser cases added/removed: 0/0. The six cases in reactions, audio attachment and avatar loading retain geometry, focus, CSS, lazy-loading, referrer and recovery assertions. The 18 retained message/thread/unread journeys passed in both engines; no browser coverage moved into jsdom. Controlled DOM dimensions in the unit tests assert positioning policy only.
Coverage mapping and timing evidence
savedMessage; mounted Save retains the combined restart/failure notice and payload checks.agent_defaults_persist_owner_only_and_never_project_valuesretains file mode, reopen, secret projection and malformed-storage checks.Mutation commands:
bin/pnpm exec vitest run src/features/messages/use-reading.test.ts -t 'releases the old channel';bin/cargo test --locked -p buzzodz-plugins --test management rollback_swaps_reload_sources_and_same_byte_reload_refreshes_source;bin/cargo test --locked -p buzz-foundation --lib goose_models::tests::one_shot_acp_request_reads_catalog_before_closing_stdin. Each failed for the intended assertion, then passed after restoration.Local browser comparison: macOS, pinned Node 24.18.0, Playwright 1.63.0, Chromium and WebKit, two workers, zero retries. Command:
bin/pnpm test:browser tests/browser/reactions.spec.mjs tests/browser/audio-attachment.spec.mjs tests/browser/avatar-loading.spec.mjs --project=chromium --project=webkit --no-deps --reporter=list,json. Baseline258c6d6b; the after sample used the initial audit tree atd658ec5b, before later upstream changes. The initial rebase onto3a19fa43changed no file contents. The CI follow-up is rebased ontoea7ddb81, including its agent setup changes.Setup timing used identical temporary instrumentation of the existing Vite wrapper in both snapshots; it is not in the PR. Case durations include their fixture costs and overlap across workers. These single samples vary and do not establish hosted CI savings. Shard-routing checks still account for all 826 functional browser executions. The baseline hosted run passed all six browser lanes; its separate JavaScript failure preceded the gallery fix now on main.
Human testing remains outstanding. Human check: open a thread, load older replies, switch threads, and navigate Back; verify messages, draft ownership and unread state stay correct. The avatar test's existing fixed wait is unchanged; replacing it needs a valid browser lifecycle barrier. No full local scan was run.
Generated with Codex