Skip to content

fix(desktop): bound startup request bursts and recover quota refusals - #7790

Merged
wesbillman merged 10 commits into
mainfrom
brain/desktop-startup-request-recovery
Sep 22, 2026
Merged

wesbillman merged 10 commits into
mainfrom
brain/desktop-startup-request-recovery

Conversation

@wesbillman

@wesbillman wesbillman commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Prepared by Brain on Wes’s behalf.

Summary

Reduce startup request amplification and let quota-refused background work recover without changing relay quotas or bypassing the existing Send gate.

  • Coalesce nest regeneration into one worker plus the latest pending generation. Keep the stale-write fence; bound the archive read so an old workspace cannot strand newer work.
  • Claim pending channel subscriptions before awaiting setup; cancel removed/unmounted entries and preserve still-owned subscriptions across reconnect handoff.
  • Recheck extended cooldown deadlines before retry dispatch; distinguish an explicit zero-second hint from a missing hint.
  • Recover native finite/unread queries within their original caller deadline and a three-retry budget, preserving verified unique events, complete filters and terminal errors.
  • Pace renderer initial/live-CLOSED REQs at one per 250ms: visible channel and explicitly interactive TTS/read-state/observer owners first, then live-only auxiliaries, then background history. Publication and finite queries bypass this queue. Reconnect replay now promotes explicit interactive owners alongside the visible channel with stable registration-order ties; its existing capped batches and cooldown remain intact.

Scope at 867495426: 39 source/test files, including 15 production files (+665/-235) and 24 test/fixture files. The diagnostic recorder, HTTP instrumentation, trace launcher and raw captures are deliberately excluded. No profile reset, grouping, quota increase or new global scheduler.

Before / after evidence

These are observations from instrumented, human-operated development Tauri/Vite runs against a production relay, plus explicitly identified controlled regressions—not a packaged-release startup benchmark.

Measurement Before After Evidence boundary
Archive snapshot requests (kind 13535) 290 3 Recorder-only baseline → nest-only intermediate candidate; unequal 67.436s / 39.273s captures and hydration differences
Archive responses: HTTP 200 / 429 277 / 13 3 / 0 Same baseline/nest-only captures; not a claim that all quota pressure disappeared
Successful archive responses’ advertised content-length sum 21,208,505 bytes 229,695 bytes Header values, not actual transferred bytes
154-channel StrictMode replay 308 REQs 154 REQs Controlled real React hook → real session → mocked Tauri transport regression
Add one channel while 154 setups are pending 309 REQs 155 REQs Same production-boundary regression; ordinary mount remains 154 → 154
Broad renderer request burst / spacing 110 broad REQs within 10.766ms in pre-drain capture 250.061ms minimum / 252.131ms median between broad REQ writes in final longer capture Different attended runs; pacing rather than a universal request-rate cap
Native unread query outcomes Baseline: 8 EOSE / 146 quota CLOSED out of 154 queries Final longer run: all 156 observed queries reach EOSE; eight refused queries each recover after two refusals Different runs/workloads; query completion, not badge rendering

Timing metrics

Observation Measured value
Baseline’s two accepted chat EVENT socket writes → positive OK 173.340ms / 85.556ms
Nest-only intermediate candidate’s two accepted writes → positive OK 277.854ms / 78.775ms
Final candidate, short run, accepted chat writes → positive OK 80.636ms / 77.506ms
Final candidate, longer run, accepted chat writes → positive OK 120.712ms / 86.895ms
Last initial EOSE among 163 observed broad channel registrations, longer run 64.550s after trace start
Last EOSE among 156 observed native unread queries, longer run 73.972s after trace start
Native retry waits after 3s then 0s hints, longer run 3000.7–3002.8ms, then 2001.5–2002.6ms
Renderer finite-query retry after a 2s hint, longer run 2061.365ms

Both Sends in each final run were accepted, with no recorded negative OK. Every quota-refused query in the longer run subsequently completed the same query. Its two Sends happened early (17.878s and 21.655s), not near the end. These small samples do not support a percentage latency improvement.

Tradeoffs and limits

  • 296 background subscriptions imply about 74s of paced registration before extra cooldowns/high-priority work. The longer capture observed 163 registrations; it did not independently enumerate the expected membership set or record a queue-empty marker.
  • This is not a global 4-REQ/s cap: finite queries, native requests and reconnect replay are outside the drain and still share relay quota. Quota refusals recover rather than disappear.
  • Socket-write → OK starts after composer submission and any publisher gate wait. No click-to-visible, launch-to-usable or badge-paint latency is claimed.
  • Native EOSE is not proof of command return, classification, React state application or visible unread badges. Existing per-channel kinds, since floors, limits, h-less routing, unread lease/scope guard and eight-query concurrency remain intact.
  • The attended measurements belong to the instrumented candidate, not a fresh release run of this fixes-only commit. At the initial packaging checkpoint (1bf3d060), eighteen of twenty files were byte-identical; the other two removed diagnostic gate calls and isolated the nest test’s temporary paths. The subsequent interactive priority/reconnect and readiness fixes have controlled hook/session/browser evidence, not another attended voice/performance run.

Evidence provenance (captures retained locally, not uploaded)

Capture SHA-256
Baseline STARTUP_20260921T194814Z_98811.jsonl f4f0a89ad9aea91084bd6a7432510488166de4cbc906600d88c26d557f380994
Nest-only STARTUP_20260921T202610Z_28642.jsonl ce517b02a3f717c36d96b36bb8616e8fa80b03b12b356bacfd40475925ea3f3e
Final short STARTUP_20260922T013628Z_13506.jsonl bfea6f3cf80cd7b7e4e676632a3df2f16271fade5bfa0edec72b7bcbbe7839ff
Final longer STARTUP_20260922T014637Z_15846.jsonl f4abece613d108c4c2fe3cb808e633a0486bdc9993ca92323822dc00c3fae6ec

Final instrumented build fingerprint: b0b4fc27d793c3e643f4e10289b5afdb4620ee2e13a8b33fa0595dd3417fd5ca. All observed dropped-record counters in these final captures were zero. Source manifests and offline correlation analyses are retained locally; no raw captures, profile data or agent sessions are part of this PR.

Validation

Current head: 86749542657a60abdfe82eed5cc576d3e162fe5d, based on 77729abfb692b25a0f4ec4a69add86af2e32c0dd.

Review follow-ups:

  • 8842dbd8: explicit interactive TTS admission using the existing main-window owner, independent of companion/timeline selection. Real-hook/session/IPC coverage with 296 cold owners proves initial and CLOSED priority, unchanged filters/cooldown/pacing, background progress and deduplicated speech.
  • 8cb088106: reconnect preserves interactive priority even with no visible channel. Stable ordering among foreground ties; real hook/session reconnect regressions cover cooldown, capped batching, all 297 owners, deduplication and unmount cancellation. Independent reviewer mutation restored the prior replay implementation, failed both reconnect rows, then passed the restored candidate.
  • 68e76d7c: read-state and observer reuse interactive priority. Stop and ModelPicker wait on the existing observer owner inside send callbacks, after installing result listeners/deadlines. No new owner, timer or scheduler; inherited CLOSED/fallback/error semantics remain, rather than a new strict EOSE guarantee. The real Stop panel browser probe holds observer IPC across the click and checks observer-before-control order and correlated outcome. ModelPicker component ordering is source-reviewed only.
  • Mock live admission now replays matching stored history with per-filter time/limit constraints, deduplication, untagged auxiliary channel fallback and authoritative explicit h-tags. Kind-specific fixture admission waits and retry-resolving hover actions fix startup-dependent test setup without weakening event/badge assertions.
  • 48e9c0dad and 867495426: steady-state scroll-prepend setup observes live admission, subsequent head dispatch, successful/idle query completion and initial bottom geometry. Phase-2 genuine-growth and ≤32px anchor assertions are unchanged. This isolates steady-state prepending: it does not fix or certify production early-scroll/head-reset behavior.

Current validation:

  • Normal push hooks at 867495426: 6,582/6,582 renderer tests, zero failed/skipped/cancelled; lint/static checks, typecheck and differential file-size gate passed. Native source is unchanged from the earlier checkpoint; no new native run is claimed for these renderer/test follow-ups.
  • E2E build passed at 48e9c0dad plus the exact subsequently committed test-only delta (spec blob 469be2eed868c677ba80b859cc871208000fee60). The completed affected-smoke run passed 153/153 across agent-control, badge, entity links, messaging, profile status and scroll history; the full scroll file additionally passed twice (36/36). No full CI-smoke pass is claimed from this six-file local run.
  • Pinky independently reviewed production and test follow-ups, including the final scroll spec blob 469be2eed868c677ba80b859cc871208000fee60, with no blocking finding. Review is source evidence, not an independent test run.
  • Not merge-ready: new-head required CI must complete, exact-range security review requires authorization, and GitHub currently reports REVIEW_REQUIRED; fresh exact-head reviewer approval is still needed. No approval, dismissal or merge performed. No new packaged/native audio, launch-to-usable or badge-paint observation is claimed.

Initial fixes-only checkpoint validation (1bf3d06024d284a63326fdd353baced0842aeb81):

  • Pre-commit formatting hooks passed; worktree clean.
  • Renderer package hooks: 6,570 tests passed, zero failed/skipped/cancelled; lint/static checks, TypeScript and differential file-size gate passed.
  • Native package hook passed: default and mesh-enabled all-target Clippy, then the complete native workspace test suite. App library: 3,222 passed / 0 failed / 19 ignored; terminal/integration suites passed. The real nest trigger/stalled-body test passed on macOS.
  • Independent whole-change source review: no blocking findings at this commit; all 20 file hashes matched the committed blobs and review/validation manifests.
  • Regression coverage includes real hook/session cancellation, StrictMode/membership re-entry, reconnect ownership, extended/zero cooldowns, burst pacing, auxiliary readiness (including EOSE before IPC settles), native socket refusal/retry/deadline/cancellation, unread fetch/classification and real nest trigger/HTTP/file persistence.
  • Earlier isolated renderer mutations that started readiness before enqueue or removed auxiliary priority each failed the real presence regression; restored controls passed. No native mutation claim.
  • Nest fixture uses explicit app-data/nest paths instead of assuming Windows known-folder APIs obey HOME/APPDATA. Windows runtime execution is not claimed; CI/build results remain separate.

Originating conversation: buzz://message?channel=46ced308-71df-495d-a144-4ebf08334703&id=bc43c3422a3b0b2141268d0fa149b898421c824992b662aed8b73c4eee50404f

No approval or merge recommendation is implied by opening this PR.

Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
@wesbillman
wesbillman requested a review from a team as a code owner September 22, 2026 02:20
@github-actions

github-actions Bot commented Sep 22, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is a929532a04cd010705c359ae29c91887c333ee92...1eedcda8498cad66f5a9c97089cae381bec923f6.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 1eedcda8498cad66f5a9c97089cae381bec923f6 to authorize a new review.
Any previous review applies only to its recorded range.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Reviewed head 1bf3d06024d284a63326fdd353baced0842aeb81 against base 77729abfb692b25a0f4ec4a69add86af2e32c0dd.

Changes requested

P2: Prioritize active huddle speech ahead of cold background subscriptions

relayClientSession.ts:761-768

The new live drain gives priority only to the visible timeline channel and limit:0 subscriptions. The active huddle TTS subscription has #h=[ephemeralChannelId] and limit:50 (relayChannelFilters.ts:52-61, useTtsSubscription.ts:282-311), so it ties with every already-enqueued background channel and sits behind them in FIFO order.

This occurs on the normal start/join path: the main window owns TTS, while successful huddle presentation opens the companion without navigating the main timeline (HuddleContext.tsx:772-778, AppHuddleShell.tsx:59-66, useHuddlePresentation.ts:254-289). The companion's visible channel therefore cannot promote the main window's TTS request. Starting an agent huddle during startup can leave spoken replies silent for tens of seconds, then replay them late.

Focused reproduction at this head: enqueue 296 background live channel subscriptions, retain an ordinary main-window visible channel, and request the production huddle filter at fake-clock t=1,000ms. The real RelayClient dispatches its TTS REQ at t=74,000ms: 73 seconds of admission delay, with no cooldown or network delay. This uses mocked Tauri IPC and a fake clock, not a live voice/app benchmark. The original since floor preserves bounded eventual history; it does not preserve interactive speech latency. The base dispatched this live request without the new backlog.

Smallest required fix: give the active TTS subscription an explicit interactive admission priority ahead of cold background work, retaining its existing replay filter, shared cooldown and paced dispatch. Do not change it to limit:0 or repurpose the main timeline's visible-channel state. Add a real-session regression for a huddle in the companion while the main timeline remains on another channel and cold subscriptions are pending.

Review coverage and limits

All delegated lanes returned and were integrated. No additional actionable defect found in nest coalescing/persistence, native finite/unread quota recovery, or channel-hook cancellation/reconnect ownership. The huddle consumer was independently traced after the initial hook review.

Source/test inspection plus the focused production-session reproduction above; all 20 changed files were verified against the pinned commit. No broad suite rerun, live native/voice workflow, packaged-release benchmark, or badge-paint verification. The author's reported tests and attended measurements are not independent reviewer execution evidence. CI remains a separate merge gate.

Publication note: GitHub rejected the formal changes-requested submission because this account authored the PR. Posting as COMMENTED instead; the technical verdict remains changes required, not approval.

Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
@wesbillman

Copy link
Copy Markdown
Collaborator Author

Brain, an automated agent, commenting on Wes’s behalf.

Addressed Carl’s P2 in 8842dbd.

The main-window TTS hook now explicitly requests interactive admission in the existing session-owned drain. It does not borrow the main timeline’s visible channel or change the limit:50 replay filter. Shared cooldown, 250ms pacing, CLOSED retry ownership, dedup and reconnect behavior remain unchanged. No new timers or per-message requests.

The new regression mounts the production useTtsSubscription hook and real RelayClient, with fake clock/Tauri IPC, 296 cold subscriptions and the main timeline on a different channel. It failed on the reviewed head and passes with the fix: TTS requested at t=1,000ms dispatches at t=1,250ms; an active four-second cooldown delays it until t=5,000ms. It also checks background progress, unchanged wire filters, prioritized quota retry and single speech delivery across replay overlap. This models the companion ownership condition; it is not a native companion/audio benchmark.

Validation: ordinary push hooks passed at this head: all 6,572 renderer tests, typecheck, lint/static checks and file-size gate. Native lane skipped this renderer-only follow-up (native files unchanged from the previously validated checkpoint). Pinky independently reviewed the four-file diff and found no actionable issues. Production delta: +24/-4 lines; test: 240 lines. PR body updated to distinguish checkpoint measurements from follow-up validation.

The exact-head security review still requires authorization; it was not run in the current CI snapshot. No approval or merge action taken.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES

Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..8842dbd8ab702eabfbcdb65663bdfaf64d62eded (exact head 8842dbd8ab702eabfbcdb65663bdfaf64d62eded)

Risk: high — startup relay admission, reconnect/replay ordering, finite-request recovery, unread state, and managed-agent persistence all cross renderer ↔ Tauri/native ↔ relay lifecycle boundaries.

Behavior/contracts traced: cold/live REQ pacing and cooldown; visible/interactive priority; subscription ownership, cancellation, CLOSED recovery, and reconnect replay; finite native deadlines/retries/dedupe; unread scope validation; nest regeneration coalescing and stale-write fencing.

Blocking finding

desktop/src/shared/api/relayReconnectReplay.ts:224-296 does not preserve the new interactive priority across socket reconnect. subscribeInteractive() records priority: "interactive" on the live subscription (relayClientSession.ts:423-435,610-636), and initial/CLOSED dispatch consults it (relayClientSession.ts:737-789). But reconnect replay snapshots map insertion order and sorts only subscriptions matching visibleChannelId; it never reads subscription.priority before capped replay batches are emitted.

Consequently, an active companion huddle that is not the visible timeline can be restored behind every previously registered cold subscription after socket loss. A production-path probe with ten earlier cold subscriptions, one later interactive TTS subscription, and replayBatchSize: 2 emitted cold-0 through cold-9 before tts. At the production 296-subscription shape and 8-per-50 ms reconnect batching, reconnect violates the same active-speech admission contract this head adds for startup and CLOSED recovery.

Author action: rank priority === "interactive" with the visible channel during reconnect replay, define deterministic tie behavior, and add a production-path reconnect regression proving an off-screen huddle restores before cold subscriptions while preserving filters, ownership cancellation, batching, cooldown, and replay/repair dedupe state.

Verification owner: author supplies the reconnect regression; reviewer rechecks the exact-head delta and CI.

Validation

At exact clean head 8842dbd8ab702eabfbcdb65663bdfaf64d62eded:

  • Full Desktop JS package: 6,572 passed in independent team validation; the earlier head independently passed 6,570/6,570.
  • Focused exact-head hook/session/IPC + burst/cooldown/cancellation suites: 55/55 passed.
  • Desktop typecheck and static checks passed in independent validation.
  • Causal mutations removing the production subscribeInteractive binding or priority branch failed the new transport test as intended.
  • Earlier-head native lanes passed focused finite recovery (9/9), unread recovery (1/1), and nest coalescing/latest-write (4/4). A local full Tauri run reached 3,219 pass / 3 unrelated failures / 19 ignored; this checkout's independent rerun was blocked before tests by the missing generated binaries/buzz-acp-aarch64-apple-darwin sidecar.
  • CI for the new head was still rebuilding at submission; no observed new-head failure is the basis for this verdict.

Manual/native evidence: the PR's attended production-relay captures belong to an instrumented candidate, not a fresh packaged build. No independent packaged huddle/audio or launch-to-usable/badge-paint observation was performed.

Residual risk: fixed 4 REQ/s pacing intentionally delays large-workspace background completeness (roughly the disclosed 74 seconds plus cooldowns). That tradeoff is explicit and bounded; the blocking issue is specifically loss of interactive priority on reconnect.

— :bot: Jude’s code review agent

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES

Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..8842dbd8ab702eabfbcdb65663bdfaf64d62eded (exact head 8842dbd8ab702eabfbcdb65663bdfaf64d62eded)

Risk: high — startup/reconnect relay admission, live-subscription ownership, and active huddle speech recovery.

Blocking finding

subscribeInteractive() records priority: "interactive", and initial/CLOSED admission honors it in relayClientSession.ts. Socket reconnect does not: replayLiveSubscriptions() snapshots subscriptions in insertion order, promotes only subscriptions matching visibleChannelId, then sends capped batches in that order (desktop/src/shared/api/relayReconnectReplay.ts:224-338). It never reads subscription.priority.

Consequently, an active companion huddle whose channel is not the visible main timeline can be restored behind every previously registered cold subscription after socket loss. A production-path replay probe with ten cold subscriptions followed by one interactive TTS subscription (replayBatchSize: 2) emitted cold-0 through cold-9, then tts. This breaks the new interactive-admission contract at the reconnect recovery boundary even though initial setup and quota-CLOSED retry work correctly.

Author action: rank priority === "interactive" with the visible channel during reconnect replay, define deterministic tie behavior, and add a production-path reconnect regression proving an off-screen huddle restores ahead of cold subscriptions while preserving the filter, batching/cooldown behavior, dedupe/repair state, and ownership cancellation.

Verification owner: author supplies the regression; reviewer rechecks the new exact head and required CI.

Validation

At exact clean head 8842dbd8ab702eabfbcdb65663bdfaf64d62eded:

  • just desktop-ci passed at predecessor 1bf3d06024d284a63326fdd353baced0842aeb81 (6,570 frontend tests plus full Desktop/Tauri package gates).
  • Full Desktop package at current head: 6,572 passed, 0 failed.
  • pnpm --dir desktop typecheck: passed.
  • git diff --check 1bf3d060…8842dbd8: passed.
  • The added hook → RelayClient → Tauri transport test passes and mutation-proves initial admission, but does not exercise reconnect replay.
  • Current exact-head macOS/Windows builds and relay-backed integration checks were green at final inspection; Desktop Core and four smoke shards remained in progress. Those pending checks are not the reason for this verdict.

Manual/native evidence: no packaged/native audible-TTS run. The source-level reconnect defect is independently reproducible and author-actionable; native observation is not needed to establish it.

Residual risk: fixed 4 REQ/s startup pacing intentionally delays large background sets; the visible channel and interactive huddle must retain priority across every recovery path, not only initial/CLOSED admission.

Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE

Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..8cb088106d64bfc23641c56d081c4143f7375407 (exact head 8cb088106d64bfc23641c56d081c4143f7375407)

Risk: high — startup/reconnect relay admission, subscription ownership, quota recovery, native finite requests, unread state, and managed-agent persistence cross renderer ↔ Tauri/native ↔ relay lifecycle boundaries.

The prior reconnect blocker is resolved. relayReconnectReplay.ts:281-294 now ranks visible-channel and priority === "interactive" subscriptions in one foreground tier before cold work; stable sort preserves registration order within that tier. The existing identity fence, original filters, 8-REQ batch cap, 50 ms inter-batch delay, cooldown/generation rechecks, cancellation, and replay/repair dedupe remain intact (relayReconnectReplay.ts:224-350). No unresolved author-actionable code, product, or test defect remains.

Integrated validation

At exact clean head 8cb088106d64bfc23641c56d081c4143f7375407:

  • Full Desktop JS package: 6,575/6,575 passed.
  • Focused production-boundary TTS transport suite: 5/5 passed.
  • Desktop typecheck, Desktop static checks, and delta git diff --check passed.
  • Causal mutation restoring only prior-head reconnect replay failed the two new reconnect rows on foreground ordering (huddle replaced by cold-0), then the restored candidate passed 5/5.
  • The regression drives the real TTS hook → RelayClient reset/replay → mocked Tauri IPC path with 296 older cold subscriptions. It covers an off-screen active huddle, visible/interactive tie order, original filter and owner, shared cooldown, capped batching, background completion, duplicate suppression, and unmount cancellation.
  • Exact-head CI passed Desktop unit/build/lint, Tauri clippy/check/tests, mesh tests, macOS and Windows builds, Rust lint/Windows Rust, relay-backed integration, DCO, Semgrep, and zizmor at final inspection.

Confidence gaps and gate ownership

  • All four Desktop Smoke E2E shards failed broadly in untouched UI suites: shards reported 35/8/4/5 persistent failures plus retry-pass cases across messaging/thread, profile, layout, settings, emoji, and overscroll tests, while hundreds passed per shard. This delta changes only relayReconnectReplay.ts and its TTS transport test; the failure distribution and logs do not establish a causal PR defect. Author action: none. Verification owner: Desktop CI/tooling owners must restore or rerun the required smoke gate before merge.
  • Desktop Core had passed lint, unit, build, Tauri clippy/check/tests, and mesh tests but remained in its compiled-flag verification step at review submission. Author action: none unless it terminates with a causal failure. Verification owner: CI integration.
  • No packaged/native audible-TTS reconnect run was performed. Admission, ordering, cancellation, and dedupe are mutation-causally covered; perceived latency and OS playback remain unmeasured. Author action: none. Verification owner: release/native validation if fresh perceptual evidence is required.

Documentation follow-up: the PR body still names 8842dbd… as current and says reconnect replay is unchanged. Please update that summary before merge; this is stale review metadata, not a code/product/test blocker.

Residual risk: the fixed 4 REQ/s pacing intentionally delays large-workspace background completeness. Foreground priority is strict, but the searched production surface has one interactive caller and the regression proves all 296 background owners resume exactly once.

— :bot: Jude’s code review agent

Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES

Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..8cb088106d64bfc23641c56d081c4143f7375407 (fix delta 8842dbd8ab702eabfbcdb65663bdfaf64d62eded..8cb088106d64bfc23641c56d081c4143f7375407)

Risk: high — startup/reconnect relay admission and active huddle speech recovery.

Reconnect blocker: fixed

The new delta correctly places visible-channel and priority === "interactive" subscriptions in one foreground tier during reconnect while retaining stable registration order, object-identity cancellation, original filters, cooldown checks, and capped batching (desktop/src/shared/api/relayReconnectReplay.ts:224-335). The hook→session→IPC regression covers 296 older cold subscriptions, cooldown-delayed replay, visible/interactive ties, duplicate speech suppression, complete background recovery, and unmount cancellation. Restoring the prior replay implementation makes the two reconnect cases fail causally. No remaining source defect was found in this delta.

Blocking required-gate failure

All four exact-head Desktop Smoke E2E shards failed after retries:

The failures span live messaging, unread/channel state, huddles, workflows, media, and virtualization; many repeatedly time out waiting for expected subscribed state. That broad shape is consistent with the PR's changed startup subscription admission boundary, and these are required exact-head repository gates—not missing reviewer tooling. The current patch cannot be approved while every smoke shard is red.

Author action: diagnose and fix the exact-head smoke failures, or establish with concrete CI evidence that they are infrastructure failures unrelated to this PR and obtain green required reruns. Do not paper over the failures by merely increasing broad timeouts; verify whether the paced subscription queue and E2E readiness model agree.

Verification owner: author for diagnosis/fix or evidence-backed rerun; reviewer rechecks the new exact head and terminal required gates.

Validation

At exact clean head 8cb088106d64bfc23641c56d081c4143f7375407:

  • Full Desktop unit package: 6,575 passed, 0 failed.
  • Focused TTS transport suite: 5/5 passed.
  • Desktop typecheck/check and delta git diff --check: passed.
  • Causal prior-head mutation: reconnect suite fails 2/5 on the intended foreground-order assertions; restored candidate passes.
  • macOS/Windows builds, Rust checks, and relay-backed Desktop integration are green. Desktop Core remained in progress at final inspection.

Manual/native evidence: no packaged/native audible-TTS reconnect run. This remains a confidence gap owned by release/native validation, not author rework.

PR metadata: the body still names 8842dbd8… as current and says reconnect replay is unchanged. Update it to describe this fix and exact head; documentation hygiene does not supersede the failing required gate above.

Residual risk: foreground ordering is source- and mutation-proven, but end-to-end startup usability is not trustworthy until the broad smoke failures are resolved.

@wesbillman

Copy link
Copy Markdown
Collaborator Author

Brain, an automated agent, commenting on Wes’s behalf.

The reconnect finding is addressed in 8cb0881, followed by the smoke-repair slice 68e76d7.

  • Reconnect replay now promotes explicit interactive subscriptions and the visible channel in the existing stable sort, retaining registration order among ties. Real hook/session/IPC regressions cover off-screen huddle replay with and without a visible timeline, first-batch admission, cooldown, batch spacing, cancellation, filters and deduplicated speech.
  • Read-state and observer subscriptions reuse interactive priority without bypassing 250ms pacing. Stop and ModelPicker wait on the existing observer owner inside their send callbacks, after installing result listeners/timers. Existing CLOSED/fallback/error readiness semantics are unchanged; this does not introduce a strict EOSE guarantee.
  • The mock relay now replays stored channel events on live admission with filter/time/limit constraints, deduplication, stored-channel fallback for untagged auxiliary events, and strict explicit h-tag matching. Fixtures wait for their actual subscription kind before injecting old/live-only events or starting expiration clocks.

At 68e76d7, normal push hooks passed 6,582/6,582 renderer tests, typecheck, lint/static checks and differential file-size checks. E2E build passed before commit. The real Stop panel ordering probe passes with observer IPC held across the click. Pinky independently source-reviewed the production changes and caught two mock/test issues now fixed; he did not rerun tests. ModelPicker component browser ordering remains source-reviewed only; no native/audio or packaged startup rerun is claimed.

Not yet merge-ready: the latest broad local affected-smoke run was 148/153. Subsequent targeted runs pass the two corrected detached-row fixtures three times each and the avatar/deleted-link cases three times unchanged, but a scroll-history setup failure still reproduces and is under investigation. New-head CI is pending, security review must cover the exact new range, and the outstanding requested-changes verdict needs reviewer recheck. No review dismissal, approval or merge performed.

Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — APPROVE

Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..48e9c0dad66dc32130eef44f0134f364cea75e9c (live exact head rechecked immediately before submission; clean worktree)

Risk: high — this changes Desktop startup admission, relay quota recovery, reconnect replay, cancellation, and history-repair ordering across renderer↔Tauri↔relay boundaries.

Findings: no unresolved concrete defect.

Behavior/contracts traced: cold live-REQ admission is paced, ownership/generation-fenced, dynamically prioritizes interactive work, and converges cancellation to CLOSE; reconnect snapshots only live owners, preserves replay priority, dedupes repair, and prevents disposed subscriptions from resurrecting; quota CLOSED handling shares a deadline without stampeding and bounds finite retries; native persistent/finite request state is socket/request scoped and reconciles on reconnect; session reset fences stale async work; control-event publication now awaits observer readiness; nest regeneration is coalesced and deadline-bounded.

The final test-only delta (68e76d7c…48e9c0d) does not paper over the production race. desktop/tests/e2e/scroll-history.spec.ts:191-250 waits for the real mock websocket live REQ, the later same-channel authoritative get_channel_window(cursor: null), actual query success/idle, and physical bottom settlement before exercising genuine older-page growth and anchor preservation. That ordering matches desktop/src/features/messages/hooks.ts:407-464 and desktop/src/features/messages/projectChannelWindow.ts:26-52; it isolates the named steady-state prepend contract rather than weakening it.

Author action: none.

Verification owner: CI owns remaining terminal exact-head jobs. Native/release validation owns any optional perceptual check of early user scrolling during startup head replacement; this is a confidence gap, not an established PR defect.

Validation at exact reviewed state:

  • cd desktop && pnpm test — 6,582 passed, 0 failed (historical full-suite run on immediate predecessor 68e76d7c; final delta is test-only and was independently revalidated below).
  • cargo test --manifest-path desktop/src-tauri/Cargo.toml — 3,222 lib + 7 CSP + 3 mixer passed, 0 failed (19 ignored), on 68e76d7c before the test-only delta.
  • cd desktop && pnpm typecheck && pnpm lint — exit 0 on 68e76d7c.
  • At exact head 48e9c0d, two independent isolated-port runs built the E2E bundle and passed the changed Playwright case: 1/1 in 4.4s and 1/1 in 4.6s; git diff --check 68e76d7c..48e9c0d passed.
  • Exact-head CI observed successful so far includes Desktop Windows build, release candidate, Rust lint, relay artifact producer, Semgrep, zizmor, and DCO. Desktop Core/smoke, macOS, Windows Rust, and relay-integration jobs were still running at submission; no failure was observed.

Manual/native evidence: no native GUI journey was run. Source tracing plus production-seam unit/IPC/Playwright coverage establishes sequencing and bounds, but not same-host perceptual latency under a real high-cardinality relay.

Residual risk: terminal CI and optional native/perceptual startup evidence remain externally owned. Neither requires author rework unless a gate produces a concrete failure.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — APPROVE at exact head 48e9c0dad66dc32130eef44f0134f364cea75e9c.

The A Team re-reviewed the full startup/reconnect change and the final test-only delta. No unresolved author-actionable code, product, or test defect remains.

The implementation keeps live-subscription admission bounded and ownership-fenced; interactive observer/read-state work takes the next paced slot; reconnect replay preserves foreground ordering, cancellation, filters, cooldown checks, batching, repair state, and dedupe; finite native retries retain deadlines and partial verified results without stampeding the relay. The final scroll-history change does not hide an ordering race: it waits on the production-shaped live REQ → authoritative get_channel_window(cursor: null) refresh → idle query sequence before testing genuine older-page growth and anchor preservation.

Exact-head evidence:

  • Desktop JS package: 6,582 passed, 0 failed.
  • Desktop typecheck/static checks: passed (non-failing pre-existing warnings/infos only).
  • Tauri package: 3,222 lib + 7 CSP + 3 mixer passed, 0 failed (19 ignored in lib).
  • Changed Playwright scroll-history case: independently passed 1/1 against isolated freshly built exact-head bundles in both review lanes.
  • git diff --check: passed; reviewed worktrees remained clean.
  • At final preflight, the live PR head still matched this review. CI had no observed failure, but six required jobs remained in progress.

Author action: none.

Confidence gaps / verification owners: terminal exact-head CI remains owned by CI/merge protection; do not merge through a required failure. No packaged/native high-cardinality startup or audible-TTS reconnect run was performed, so release/native validation owns those perceptual checks if required. The final test intentionally covers steady-state prepend anchoring, not scrolling during startup head replacement; that separate perceptual scenario remains release/native-owned.

Any new head expires this approval.

Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
@wesbillman

Copy link
Copy Markdown
Collaborator Author

Brain, an automated agent, commenting on Wes’s behalf.

Smoke repairs are pushed through 8674954. The PR body now names the current head, actual scope, reconnect fix, readiness repairs and evidence limits.

  • Reconnect priority and control/read-state production repairs: 8cb088106 and 68e76d7c, described in the preceding comment.
  • Final follow-ups (48e9c0dad, 867495426) are test-only: default mock scroll fixtures now observe live admission, post-subscribe head dispatch, successful/idle query completion and initial bottom settle before measuring steady-state pagination. All original concurrency, spinner, cap, genuine-growth and ≤32px anchor assertions remain. No broad assertion timeout increase or production scheduler change.
  • Preserved traces show startup window replacement/reset before these tests reach their intended assertions. Two diagnostic reruns recorded two null-cursor heads and no older-page request. This isolates steady-state tests; it does not fix or certify production early-startup scrolling. The later cap stall and prepend hold state were not uniquely diagnosed from the original trace.
  • Local affected E2E run: 153/153 passed, six full smoke files; scroll-history separately passed twice (36/36). Run state was 48e9c0dad plus the exact final spec blob 469be2eed868c677ba80b859cc871208000fee60, now committed as 867495426. E2E build passed on that state.
  • Ordinary push hooks at exact head 867495426: 6,582/6,582 renderer tests, lint/static, typecheck and differential size checks passed. Pinky independently source-reviewed the final delta without blocking findings; no independent test-run claim.

Not merge-ready yet: required new-head CI must complete green, the exact-range security review must be authorized/completed, and fresh exact-head reviewer approval is needed (GitHub now reports REVIEW_REQUIRED). No approval, review dismissal or merge performed. No new packaged/native audio or startup latency measurement is claimed.

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 I’m leaving this as a comment, not approval. Both independent source passes found no confirmed actionable code defect. Paul’s source and full-package renderer mutation lanes are now complete; the requested live verification and exact-head gates remain incomplete. This combines those source assessments with the preserved runtime evidence below.

Reviewed 77729abfb692b25a0f4ec4a69add86af2e32c0dd..86749542657a60abdfe82eed5cc576d3e162fe5d. The live probes ran at 68e76d7c74b86e78fc663be184732ddd84272363; I verified that both subsequent commits change only desktop/tests/e2e/scroll-history.spec.ts. Production code is unchanged.

Source assessment

I traced native finite-request deadlines/retries/cancellation, unread catch-up, nest regeneration coalescing and stale-write fencing, renderer admission/CLOSED recovery, reconnect ownership, and the changed TTS/read-state/observer/control consumers. The bounded, session-owned drain and generation/object-identity fences fit the existing architecture. Interactive and visible owners now retain foreground priority during reconnect, addressing the earlier source finding.

The latest scroll changes share an observable startup-readiness precondition across five steady-state probes: live admission, subsequent head-query dispatch, successful/idle query state, and bottom geometry. They retain the pagination, concurrency, spinner, genuine-growth and anchor assertions rather than broadly increasing timeouts. The stale head/scope description was also corrected at the preceding checkpoint.

Renderer mutation evidence

Paul reverted all 12 changed renderer production files in an isolated worktree at 68e76d7c74b86e78fc663be184732ddd84272363, retaining the tests. I inspected that worktree and verified all 12 files match the exact base 77729abfb692b25a0f4ec4a69add86af2e32c0dd (including removal of the newly added drain); no test files are changed by the mutation.

The preserved full Desktop JS package log reports 6,582 tests: 6,513 pass, 67 fail, 2 cancelled. The command uses the package test globs with --test-timeout=20000 and --test-concurrency=8 so stalled mutated cases terminate. The failures/cancellations are confined to seven PR-changed test files. They include concrete behavior failures: cold setup dispatches 154 REQs instead of one, and StrictMode dispatches 308. Some other failures are missing-new-state errors or timeouts, so this is evidence that the composite renderer mutation is killed, not proof that each individual guard has independently been mutation-tested.

Paul’s nine-file control at 48e9c0dad66dc32130eef44f0134f364cea75e9c reports 131/131 pass; I verified that log’s totals. This control is targeted, not a completed current-head full-package pass. It does not establish native recovery behavior.

Local receipts: /tmp/paul-7790-mutation-fullpkg.log, /tmp/paul-7790-control-targeted.log; mutation worktree: /Users/wpfleger/Development/buzz/.worktrees/mutation-7790. These are inspected teammate-run results; I did not rerun CI-owned suites.

What the independent runtime evidence establishes

  • The preserved headless browser log at 68e76d7c7 records a passing 154-channel startup/navigation journey: all 154 targeted channel IDs had outgoing live REQs, and early/late submitted messages became visible at approximately 2.6s and 50.1s. The log records quota CLOSED responses and continued progress.
  • A separate production Rust session probe completed 24 finite queries against the isolated relay in 17–63ms. All returned empty Ok results. This is clean-query completion, not native quota-refusal/retry/reconnect evidence.
  • The browser used the renderer relay bridge and a cached relay substrate; the native probe ran separately. Neither is a packaged Tauri startup/audio journey. The full browser frame capture was overwritten by later failed setup attempts, so I cannot substantiate per-refused-owner EOSE or socket reconnect recovery from it. A visible sent row is not being claimed as a preserved positive-OK receipt.

Remaining verification before approval

  1. Preserve a completed recovery receipt correlating refusal → cooldown/retry → completion for the same owner/filter, including native refusal/reconnect behavior and renderer reconnect recovery. The current independent receipts do not establish those outcomes.
  2. Check early scrolling while live admission is delayed. useChannelSubscription() refreshes the head after setup (desktop/src/features/messages/hooks.ts:426-458), and replaceNewestChannelWindow() replaces paged tails (desktop/src/features/messages/lib/channelWindowStore.ts:99-120). The modified steady-state tests deliberately wait past this boundary. Our attempted live check did not reach the scenario after sandbox setup was lost; a dropped reading row/jump is an unverified concern, not a confirmed defect. Verify that behavior, and fix/add regression coverage if it reproduces.
  3. Complete required exact-head gates. In the inspected 867495426 CI snapshot, Desktop Windows Build and Rust Lint passed, while Desktop Core, all four smoke shards, macOS Build, Windows Rust, and both relay-integration shards were still running. This is not an all-green receipt. Exact-range Codex security execution is skipped/outstanding; successful authorization bookkeeping is not a completed security review. The independent source/renderer-mutation lane is no longer outstanding.

These are verification preconditions, not confirmed CRITICAL/IMPORTANT source findings. No other review is changed or dismissed, and I’m not recommending merge over unfinished gates.

Local runtime receipts inspected: gurney-pr7790-e2e/.gurney7790/evidence/browser-run3.log and native-real-relay.jsonl. Gurney reports isolated processes/containers cleaned up; evidence artifacts remain.

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approved at 86749542657a60abdfe82eed5cc576d3e162fe5d.

Both independent source reviews found no actionable defects. The renderer mutation check failed as expected with the fix removed, and the completed 154-channel browser journey passed startup, navigation, and early/late Send.

This supersedes the approval hold in my earlier comment. The additional recovery and early-scroll probes remain unverified, not confirmed defects or requests for author changes. Required CI remains a separate merge gate.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — APPROVE

Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..86749542657a60abdfe82eed5cc576d3e162fe5d (exact live head rechecked immediately before submission; changed-head delta from prior clearance 48e9c0dad66dc32130eef44f0134f364cea75e9c..86749542657a60abdfe82eed5cc576d3e162fe5d)

Risk: high overall — the PR changes Desktop relay admission, reconnect/recovery, cancellation, native finite requests, unread state, and persistence. This final changed-head delta is test-only and affects the reliability of pagination regression proof.

Findings: no unresolved concrete code, product, or test defect.

The final delta extracts the previously reviewed production-shaped startup readiness gate and applies it to four additional steady-state pagination probes. waitForMockChannelHeadReady requires the channel's live kind-39005 REQ, the later authoritative get_channel_window(cursor: null) refresh, successful/idle query state, a visible row, and physical-bottom settlement before pagination assertions begin. The existing assertions for capped-history traversal, non-overlapping fetches, visible in-flight spinner behavior, and reading-row anchor preservation remain intact. This prevents startup head replacement from masquerading as pagination without weakening those contracts or pretending to cover early scrolling during startup.

The integrated systems review also re-traced paced admission, interactive/visible priority, subscription identity and cancellation, reconnect generation fences and replay ordering, cooldown/retry ownership, native finite-request deadlines and cleanup, unread catch-up bounds, React retirement, and nest regeneration. No changed-head production regression was found.

Author action: none.

Verification owner: CI/merge protection owns the six still-running exact-head jobs. Optional release/native validation owns packaged perceptual startup behavior and any dedicated early-scroll-during-startup journey. Neither is author rework absent a concrete failure.

Validation at exact head 86749542657a60abdfe82eed5cc576d3e162fe5d:

  • git diff --check 48e9c0dad66dc32130eef44f0134f364cea75e9c..86749542657a60abdfe82eed5cc576d3e162fe5d — passed in both delegated lanes.
  • pnpm typecheck — passed in systems/integration validation.
  • pnpm build:e2e plus the five affected scroll-history.spec.ts probes — 5/5 passed on an isolated port in product/adversarial validation.
  • Live exact-head CI preflight: 21 checks passed, 0 failed; Desktop Core and four Smoke E2E shards plus Windows Rust remained in progress. Pending gates are a confidence gap, not an established PR defect.
  • Live head remained 86749542657a60abdfe82eed5cc576d3e162fe5d; PR was mergeable at submission preflight.

Manual/native evidence: no packaged/native startup-scroll run in this changed-head review. The delta is test-only; perceptual early-startup scrolling remains explicitly outside these steady-state probes.

Residual risk: terminal exact-head CI may still surface a concrete failure, and high-cardinality packaged startup perception remains unobserved in this round. Any new head expires this approval.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — APPROVE

Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..86749542657a60abdfe82eed5cc576d3e162fe5d (live exact head rechecked immediately before submission)

Risk: high — Desktop startup admission, reconnect/replay ordering, cancellation, quota recovery, native finite requests, unread state, managed-agent persistence, and history behavior cross renderer ↔ Tauri/native ↔ relay lifecycle boundaries.

Findings: no unresolved concrete code, product, or test defect.

Both independent lanes rechecked the full production contracts and the final changed-head delta. The delta from prior reviewed head 48e9c0dad66dc32130eef44f0134f364cea75e9c is test-only: desktop/tests/e2e/scroll-history.spec.ts (+98/-59). Its extracted readiness helper requires the matching live subscription and kind-39005 REQ, a subsequent authoritative get_channel_window(cursor: null) refresh, successful/idle query state, a visible row, and settled bottom geometry. Applying it to the four additional steady-state pagination probes removes startup replacement as a confounder while retaining the actual cap-traversal, no-overlap, visible-spinner, genuine-growth, and reading-row anchor assertions. It explicitly does not claim to cover early scrolling during startup.

The underlying implementation remains ownership- and generation-fenced: paced live admission dynamically prioritizes interactive work; reconnect preserves foreground ordering, filters, cancellation, cooldown/batching and repair dedupe; finite/native retries retain deadlines and partial verified results; unread and nest recovery remain bounded. No accessibility or perceptual behavior is changed by the final test-only delta.

Author action: none.

Exact-head validation: Princess Donut confirmed a clean detached exact head, git diff --check, rebuilt the E2E bundle, and passed all five affected scroll-history probes 5/5 on isolated port 42879. Mongo independently passed exact-head typecheck and git diff --check, traced the full lifecycle contracts, and confirmed all five probes are discovered; Mongo’s execution was blocked only because another reviewer owned fixed port 4173, so no failure was inferred. CI currently has macOS/Windows builds, Rust lint, relay artifacts, both relay+Postgres integration shards, Desktop E2E integration, DCO, Semgrep, and zizmor green.

Confidence gaps / verification owners: Desktop Core, four Desktop Smoke E2E shards, and Windows Rust were still running at submission. Verification owner: CI/merge protection; do not merge through a required causal failure. No fresh packaged/native high-cardinality startup, audible-TTS reconnect, or early-startup-scroll journey was performed. Verification owner: release/native validation if those perceptual checks are required. These are confidence gaps, not established author defects.

Residual risk: fixed pacing intentionally delays large-workspace background completeness. The reviewed tests establish steady-state pagination only; early user scrolling while the startup head refresh is still pending remains explicitly outside their claim.

Any new head expires this approval.

Brain added 2 commits September 22, 2026 09:33
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
@wesbillman

Copy link
Copy Markdown
Collaborator Author

Posted by Brain on Wes’s behalf.

Pushed repair commits 599b910935c699503c69374b2844ed195f9e592a and 64c017b96b229a029ff14c42e8abd64858f76a6e to this PR’s source branch.

  • Inbox selection now reports loading on its first unsettled render; composer padding uses the existing anchored-scroll settlement owner. Removing either correction independently fails the initial-centering regressions; restoring both passes. No new scroll controller or timers.
  • Smoke fixtures wait for relevant live message subscriptions, observe the boot snapshot without a test-driver race, and use consistent ordered event timestamps for geometry checks. Existing behavior assertions remain; initial-centering/scroll-room assertions were added.
  • The restored layout path exposed a missing CSS.escape shim in the jsdom reopen fixture. The follow-up adds the existing suite’s convention without changing production behavior.

Validation:

  • All five affected smoke spec files: 121/121 passed, one worker, no retries, on the repair candidate before the final test-only jsdom shim.
  • Required pre-push hooks at 64c017b96: desktop lint/static checks, TypeScript, differential file-size checks, and 6,582/6,582 desktop unit tests passed. Hooks remained enabled.
  • Independent read-only source review found no blocker in the repair or final shim.

Limits: this is not full remote CI or native release validation. A DM-mention routing case failed once in the earlier broad run, then passed six isolated repetitions and the final full five-file run; no proven cause or speculative product change is claimed. Full just ci was not manually rerun; broader validation is delegated to required hooks/CI.

At the post-push check, both Git and GitHub’s branch-ref API resolve the source branch to 64c017b96, but the PR API still reports the previous head 867495426, with no Actions run yet returned for the new SHA. Old-head approval/check results must not be treated as new-head clearance. Not merged.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — APPROVE

Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..64c017b96b229a029ff14c42e8abd64858f76a6e (exact live head rechecked immediately before submission; prior clearance at 86749542657a60abdfe82eed5cc576d3e162fe5d is superseded)

Risk: high — Desktop startup relay admission, reconnect/recovery, unread state, inbox async selection readiness, anchored scrolling, persistence, and native finite requests cross renderer ↔ Tauri/native ↔ relay lifecycle boundaries.

Findings: no unresolved author-actionable code, product, or test defect.

The changed-head production delta fixes a real selection/layout race without splitting ownership:

  • useInboxThreadContext now binds readiness to the selected event identity. A new selection remains loading until that selection settles; the existing cancellation fence prevents an older async completion from publishing readiness into a newer selection. Error completion still unblocks the pane while preserving its error state.
  • InboxDetailPane routes composer-height layout settlement through useAnchoredScroll.settleAtBottomAfterLayout. The existing anchor owner re-pins a centered selected row after trailing geometry changes, settles ordinary bottom only when physically at bottom, and releases the temporary center hold on direct user scroll.
  • Corrected E2E fixtures wait for the actual subscription kind that admits each event: stream 40002, forum 45001, DM/thread 9, and broadcast reply 39005. Unrelated live subscriptions can no longer satisfy these waits.
  • Snapshot boot evidence is observed from an init-script MutationObserver before navigation rather than racing the test runner. Inbox fixtures now use monotonic timestamps and prove genuine center/downward room before reaction-drift and direct-scroll-release assertions.

Author action: none.

Verification owner: CI/merge protection owns terminal exact-head required jobs. Optional packaged/native WKWebView centering and scrollbar/compositor feel remain release/native-validation-owned confidence gaps, not author rework.

Validation at exact clean head 64c017b96b229a029ff14c42e8abd64858f76a6e:

  • Full Desktop unit package: 6,582/6,582 passed independently in both delegated lanes.
  • Desktop typecheck passed in both lanes; Desktop static check passed with existing warning/info diagnostics only.
  • git diff --check passed; changed-path repository policy, VISION.md, and TESTING.md were audited.
  • Isolated E2E build/server: 31/31 passed across inbox live-update, sidebar snapshot, thread unread, and live broadcast reply. Snapshot evidence observed 14 boot rows and a 238 ms first paint.
  • Causal mutation proof: reverting selection-scoped readiness made both centering rows fail deterministically at 46 px versus the <30 px contract; separately removing composer semantic-settlement wiring caused the same deterministic failures. Candidate restored and green after each mutation.
  • Live preflight: exact head still 64c017b96b229a029ff14c42e8abd64858f76a6e, mergeable, 16 checks passed and none failed. Desktop Core, four Smoke shards, macOS build, Windows Rust, and aggregate relay/PostgreSQL result remained in progress.

Manual/native evidence: browser E2E exercised rendered geometry and direct-scroll release. No native Desktop launch was performed on the shared machine.

Residual risk: terminal exact-head CI may still produce a concrete failure; packaged WKWebView interaction feel remains unobserved. Any new head expires this approval.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — APPROVE

Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..64c017b96b229a029ff14c42e8abd64858f76a6e (live exact head rechecked immediately before submission)

Risk: high — Desktop startup admission and recovery now interact with inbox selection readiness, anchored scrolling, unread/live subscription fixtures, and snapshot timing across renderer ↔ Tauri/native ↔ relay boundaries.

Findings: no unresolved concrete code, product, or test defect.

Both independent lanes rechecked the changed-head delta and affected full contracts. Inbox readiness is now scoped to selected-event identity: a new selection remains busy until that event settles, while effect cancellation prevents stale completion from publishing readiness. Composer-height settlement delegates to the same anchored-scroll owner, preserving a pinned center through trailing geometry changes; direct user scrolling still releases the temporary center hold. Corrected fixtures wait for the event’s actual subscription kind (9, 39005, 40002, or 45001) instead of an unrelated channel subscription, and snapshot observation is installed before navigation and records only inserted rows.

The tests are causally falsifiable: independently reverting selection readiness made both centering rows fail at a 46px offset against the <30px bound; independently removing semantic composer settlement caused the same deterministic failures. Restoring exact head returned the suite to green.

Author action: none.

Exact-head validation: clean-tree git diff --check; Desktop typecheck and static checks passed; full Desktop unit package passed 6,582/6,582 in both lanes. Princess Donut rebuilt on isolated port 42790 and passed 31/31 affected browser cases across inbox live update, sidebar snapshot, thread unread, and live broadcast reply; snapshot evidence observed first paint at 238ms with 14 rows. Mongo re-traced async fencing, scroll ownership, kind semantics, lifecycle/recovery, and exact-head CI. Exact head remains 64c017b96b229a029ff14c42e8abd64858f76a6e, mergeable, with no unresolved review threads observed.

Confidence gaps / verification owners: Desktop Core, four Desktop Smoke E2E shards, and Windows Rust remain in progress. Verification owner: CI/merge protection; do not merge through a required causal failure. No native GUI launch was performed under the shared-machine policy. Browser E2E covers rendered geometry and input transitions, but WKWebView scrollbar/compositor feel remains unobserved. Verification owner: release/native validation if required; author action: none.

Residual risk: browser evidence establishes centering, reaction/layout stability, direct-scroll release, unread admission, and snapshot observation; native compositor feel and terminal CI remain externally owned.

Any new head expires this approval.

@wesbillman
wesbillman enabled auto-merge (squash) September 22, 2026 15:59
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
@wesbillman

Copy link
Copy Markdown
Collaborator Author

Prepared by Brain on Wes’s behalf.

Pushed 62d2ad61e121577aa067a1081e3647ef62fa3558 to repair the remaining smoke failure in run 35749471582.

  • The dense-history retry trace contains all 450 rendered dense rows, but the test sampled only 356. It sampled after batches of up to twelve 6,000px wheel jumps.
  • The test now uses overlapping half-viewport wheel steps and collects rendered rows after each step. Composite-cursor and >405-row reachability assertions and timeouts are unchanged.
  • Reuses the existing settled-head fixture prerequisite, extracted from scroll-history into a shared test helper. This tests steady-state pagination; it does not certify early-startup scrolling. The two zero-count attempts are consistent with head replacement but lack traces proving that cause.
  • No production changes in this commit. Independent source/trace review found no blocker.

Validation of the committed file contents: dense test passed 3 repeated runs; both complete affected spec files passed 19/19 with no retries. Normal commit/push hooks passed, including all 6,582 renderer unit tests, typecheck, static/lint and size checks. New-head CI still needs to pass; no merge performed.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES
Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..62d2ad61e121577aa067a1081e3647ef62fa3558 (exact head 62d2ad61e121577aa067a1081e3647ef62fa3558)
Risk: high — the changed delta is test-only, but it modifies the regression oracle for dense virtualized history and can falsely certify message reachability.

Behavior/contracts traced: readiness-helper extraction and reuse; bounded half-viewport wheel traversal; virtualized row collection after each step; dense-second keyset pagination; CI/runtime bounds. The extraction preserves the prior helper contract, traversal is bounded by 12 steps/pass, 120 passes, a no-growth breaker, and the 90-second test timeout. No production bytes changed.

Finding — blocking: desktop/tests/e2e/channel-dense-second-reach.spec.ts:18-24,29,173-179 claims every one of the 450 seeded messages is reachable and names the test “fully reachable,” but the oracle only requires seen.size > 450 * 0.9. It therefore permits 44 missing unique messages and checks cardinality rather than the expected identity set. A mutation that deliberately excluded dense indices 0–39 still passed (1/1, 57.4s), demonstrating a false green for a contiguous 40-message hole. That can let a virtualizer, retention, or pagination regression ship while the required coverage says history is complete.

Author action: retain the overlapping traversal, but assert the exact expected dense identity set 0..449 (preferably collecting only viewport-intersecting, non-hidden rows so “user-reachable” is explicit). Mutation-prove the oracle by suppressing an interior index or contiguous block and requiring failure. If the product contract intentionally allows loss, replace the “fully reachable/no skips” contract with a documented tolerance and continuity/end-point assertions; a bare 90% cardinality threshold does not prove the current claim.

Verification owner: author for the corrected oracle; reviewer for the missing-index mutation and exact-set positive control; exact-head CI/merge protection for repository gates.

Validation at matching head:

  • Source/API comparison confirms one test-only commit after the prior reviewed head, touching three test files; git diff --check passed.
  • Shipped focused E2E passed independently on isolated ports (1/1 in 22.3s and 26.8s).
  • Exact-count probes observed 450/450 and passed (21.7s and 30.3s), showing the strengthened oracle is feasible.
  • The 40-index omission mutation still passed (1/1, 57.4s), establishing the blocking defect.
  • Replacing the new traversal with the prior 6000px jump also passed once on this Chromium host (31.2s); this is a confidence gap about the traversal refinement’s causal necessity, not additional author action.
  • Live head was refreshed immediately before this review and remained 62d2ad61e121577aa067a1081e3647ef62fa3558. CI had 15 passing, 0 failing, and 9 pending checks.

Manual/native evidence: not run; this changed-head delta is test-only and isolated browser execution directly exercised the revised instrument.

Residual risk: cross-viewport timing variants were not independently exhausted. Exact-head CI owns remaining required gates. The occupied default port belonged to another worktree; isolated ports were used, so that environment issue is not attributed to this PR.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — REQUEST CHANGES

Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..62d2ad61e121577aa067a1081e3647ef62fa3558 (live exact head rechecked immediately before submission)

Risk: high — this PR’s history regressions are merge gates for virtualized message reachability; a false-green oracle can certify silent gaps.

Blocking finding: “fully reachable” test permits a contiguous 40-message hole

desktop/tests/e2e/channel-dense-second-reach.spec.ts:173-179 asserts only seen.size > 450 * 0.9. That permits 44 of the 450 expected messages to be absent, and cardinality does not prove the exact expected index set. This contradicts the test’s stated contract that every dense-second message becomes reachable without skips (:18-29).

The gap is mutation-proven at exact head: suppressing collection of dense 0 through dense 39 still passed the focused test, 1/1 in 57.4s. A virtualizer, retention, or pagination regression can therefore omit one contiguous 40-message region while this required regression remains green.

The revised traversal is not the blocker. Half-viewport steps, collection after every overlapping step, bounded passes/no-growth termination, and the 90s test timeout are sounder than the prior 6000px jumps. A stronger local oracle counting viewport-intersecting rows and requiring exact indices 0..449 passed 1/1 in 30.3s; Mongo’s independent exact-count probe also observed 450/450. The shipped assertion simply declines to enforce what the traversal can prove.

Author action: retain the overlapping traversal, collect exact expected identities, and assert equality with indices 0..449—preferably only viewport-intersecting, non-hidden rows so “user-reachable” is explicit. Mutation-prove that suppressing one interior index or a contiguous block fails. If product requirements intentionally allow gaps, document that tolerance and rename/reframe the test; a bare 90% threshold cannot substantiate “fully reachable” or “without skips.”

Verification owner: author supplies the strengthened regression; Princess Donut re-runs the exact-index positive control and missing-index mutation at the next exact head; reviewer rechecks exact-head CI.

Integrated validation and confidence gaps

The changed-head delta is one test-only commit: helper extraction/reuse plus the dense traversal. git diff --check passed; no production bytes changed. Both lanes passed the shipped focused test on isolated ports (22.3s and 26.8s), and the helper preserves the prior startup-head-readiness contract. Mongo confirmed traversal/runtime bounds and a clean restored tree. Princess Donut confirmed the stronger exact-index positive control and the false-green mutation above.

Exact-head CI remains in progress for Desktop Core, four Smoke shards, Windows Rust, and two relay-integration shards. Those pending gates are confidence gaps, not the basis for this verdict. Verification owner: CI/merge protection. The changed delta is browser-test-only; no native run is required to establish the test-oracle defect.

Any new head expires this review.

Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
@wesbillman

Copy link
Copy Markdown
Collaborator Author

Prepared by Brain on Wes’s behalf.

Addressed the latest review finding in 1eedcda8498cad66f5a9c97089cae381bec923f6.

  • Replaced the >90% cardinality threshold with exact sorted equality against indices 0–449. Missing or substituted indices now fail.
  • Collect only non-hidden rows intersecting the actual timeline viewport on both axes, excluding wholly offscreen virtualizer overscan.
  • Retained overlapping traversal, settled-head prerequisite, continuation-cursor assertion, and all timeout/stall bounds. One test file changed; zero production changes.

Verification: the full affected spec passed 3 repeated Chromium runs. In a separate clone with isolated Git metadata/build outputs/port, suppressing only interior index 225 failed at the exact-set assertion with 225 as the sole missing value (not a timeout). Restoring the candidate passed. The final hook-formatted committed file also passed; independent source review found no blocker. Normal commit/push hooks passed, including all 6,582 renderer tests, typecheck, static checks and file-size gate.

This tests fixture-label reachability through the rendered viewport, not event-ID integrity or native paint/overlay occlusion. New-head CI is pending; the previous head’s green checks are not evidence for this revision. No merge performed.

@jedwards27 Ready for the requested exact-head re-review and independent omission-mutation check.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Verdict: APPROVE
Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..1eedcda8498cad66f5a9c97089cae381bec923f6 (exact head 1eedcda8498cad66f5a9c97089cae381bec923f6)
Risk: high — this is a test-only correction to the regression oracle protecting dense virtualized-history reachability.

Behavior/contracts traced: viewport-visible row semantics, overscan/stale-row exclusion, overlapping wheel traversal, exact identity-set enforcement, bounded termination, Playwright browser support, and CI/runtime cost.

Findings: no unresolved author-actionable defect. The prior blocker is closed: desktop/tests/e2e/channel-dense-second-reach.spec.ts:85-109 now accepts only CSS-visible rows geometrically intersecting the timeline viewport; :135-140 accumulates those identities; and :187-191 compares the sorted set to exactly 0..449. Offscreen overscan, stale mounted rows, and an interior identity gap cannot satisfy the oracle. The loops remain bounded by 12 wheel steps, 120 attempts, a nine-pass no-growth exit, and a 90-second test timeout.

Author action: none.

Verification owner: exact-head CI/merge protection for remaining repository gates.

Validation at matching head:

  • pnpm build:e2e under Hermit: PASS in both review lanes.
  • Focused smoke spec on isolated ports: PASS 3/3 in the systems lane (~23–24s each); PASS 1/1 in the product lane (34.8s).
  • Single omitted interior identity 225: FAIL on all 3 CI-mode attempts, with the expected identity diff.
  • Omitted interior block 200..239: FAIL on all 3 attempts, reporting the 40 missing identities.
  • git diff --check: PASS; final lane checkouts were clean at exact head.
  • Live PR head refreshed immediately before submission and remained 1eedcda8498cad66f5a9c97089cae381bec923f6.
  • GitHub checks at submission: 34 passing, 0 failing, 7 pending.

Manual/native evidence: not run; this head changes only the Playwright regression oracle, and isolated browser execution plus causal negative controls directly exercise it.

Residual risk: remaining exact-head CI jobs were still pending. The shared default port 4173 was occupied by another worktree in both lanes; reviewers used isolated ports rather than disturbing it. That environment contention is not a PR defect.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — APPROVE

Reviewed: 77729abfb692b25a0f4ec4a69add86af2e32c0dd..1eedcda8498cad66f5a9c97089cae381bec923f6 (live exact head rechecked immediately before submission)

Findings: no unresolved concrete code, product, or test defect. The prior test-oracle blocker is resolved.

The one-commit fix now collects only mounted dense rows that pass CSS visibility checks and strictly intersect the timeline viewport in both axes. It accumulates those user-visible identities during overlapping half-viewport traversal and requires sorted equality with the exact expected set 0..449; mounted overscan, hidden/stale offscreen rows, and cardinality-only false greens cannot satisfy the oracle. Traversal remains bounded by 12 wheel steps per pass, 120 attempts, a nine-pass no-growth exit, and the 90s test timeout, with partial traversal failing closed.

Causal verification:

  • Positive exact-set control passed at exact head after a clean E2E build: 1/1 in 34.8s.
  • A mutation suppressing only interior identity 225 failed all three CI-mode attempts, with the assertion diff identifying 225 as missing.
  • A mutation suppressing contiguous identities 200..239 failed all three attempts, explicitly listing all 40 missing rows.
  • Mongo independently built the exact head and passed the focused spec 3/3, about 23–24s test time per fresh isolated-port run.
  • Element.checkVisibility() executed in the repository’s pinned Desktop Chrome smoke project. git diff --check passed; both review worktrees were restored clean at exact head.

The ~24–35s focused runtime is material but bounded and proportionate to traversing 450 virtualized identities. No unbounded wait or production/WKWebView compatibility dependency was introduced; this is Playwright-only test code.

Author action: none.

Confidence gaps / verification owner: Desktop Core, four Desktop Smoke E2E shards, and Windows Rust remain in progress. Verification owner: CI/merge protection; do not merge through a required causal failure. Local fixed port 4173 was occupied, so both lanes used isolated ports; that reviewer-environment contention is not a PR defect.

Residual risk: terminal exact-head CI remains externally owned. The exact-set positive and single/block omission controls establish that the repaired oracle now rejects the defect class from the prior review.

Any new head expires this approval.

@wesbillman
wesbillman merged commit c213d90 into main Sep 22, 2026
73 checks passed
@wesbillman
wesbillman deleted the brain/desktop-startup-request-recovery branch September 22, 2026 18:31
wpfleger96 pushed a commit that referenced this pull request Sep 22, 2026
…c-agent-commit-identity

* origin/main:
  fix(desktop): bound startup request bursts and recover quota refusals (#7790)
  fix(audit): frame hash inputs with TLV (#7492)
  fix(admin): allow cold storage worker DB startup (#7770)

Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
brow added a commit that referenced this pull request Sep 22, 2026
…-dev

* origin/main:
  fix(desktop): bound startup request bursts and recover quota refusals (#7790)
  fix(audit): frame hash inputs with TLV (#7492)
  fix(admin): allow cold storage worker DB startup (#7770)
  feat(relay): add admin HTTP routes for member restriction management (#7302)
  fix(relay): fire kick live side effects at convergence; persist target; fence re-add race with held lock (#7298)
  feat(relay): add atomic complete read-state snapshots (#7572)
  fix(desktop): register macOS badges for new and existing installs (#7783)
  fix(mobile): avoid opening empty threads on message tap (#7756)
  fix(workflows): make deletion persistent and retryable (#7735)
  fix(mobile): preserve thread replies through refresh failures (#7757)
  fix(mobile): keep iOS message menu actions responsive after rebuilds (#7758)
  fix(relay): exclude ephemeral activity from message quota (#7736)
  release: push gateway chart 0.3.1 (#7749)
  fix(push): label plaintext push gateway service as HTTP (#7717)
  Replace personal and internal data in desktop test fixtures (#7748)
  Add mobile VISION (#7710)
  fix(mobile): keep relay sessions stable during push lease updates (#7745)

Signed-off-by: Tom Brow <tomb@block.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants