test: isolate direct-message delivery gate from read-state publications - #233
Conversation
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
efc538b to
29af0ab
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No blocking defects found. Reviewed 29af0abffcc628e7e91625bf60d6e64c7f42e658 against f6caa83f724253ca11fb83481e7b60f091b4cf7a, including independent review of the shared fixture contract. One optional scope-tightening suggestion is inline; no approval submitted.
The held-message gate now survives the deliberately injected competing write. Separately, the Diagnostics action exercises a real encrypted read-state write through the production broker without enabling complete snapshot discovery. Those are distinct contracts, and the PR accurately distinguishes their evidence.
Exact-head CI is green. Logs confirm both complete touched browser files passed in Chromium and WebKit (12 cases). The WebKit artifact pins clean merge tree ccf9808, readState: false, the expected channel read marker, and its matching successful OK receipt. No browser cases were removed and no retries/timeouts/assertions were relaxed. No broad local suites rerun for this review.
The interrupted local broad run is not a pass; current hosted CI supplies the configured coverage. This does not establish that unrelated flakes are fixed. No required changes from this review.
| pending, | ||
| // The production broker advertises read-state writes for every session, | ||
| // not only tests opting into complete snapshot reads. | ||
| acceptPublication: acceptReadPublication, |
There was a problem hiding this comment.
Non-blocking: consider retaining the default fixture’s narrower write boundary. acceptReadPublication also accepts kinds 9, 7 and 5 (lines 753–789), so moving the whole handler here makes ordinary production-broker journeys accept messages/reactions/deletions as well as read-state. Previously, without readState or actionProfile, policyRelay rejected every non-presence publication. For example, the default live-status journeys have no independent zero-publications assertion, so a valid accidental write would now be recorded rather than fail that guard.
The read-state correction is sound. A smaller behavioral change would have the default callback require kind 30078 before delegating, while retaining the full acceptor for the existing readState and actionProfile lanes. This is a test guardrail suggestion, not a demonstrated production authorization defect or merge blocker.
Causes and fixes
new-message.spec.mjsfixture held every upstream publication behind one overwrittenreleasecallback. A concurrent kind 30078 read-state write could replace the held kind 9 resolver, stranding the send in “Sending message…”. Hold only kind 9 and inject a concurrent publication to guard resolver ownership. The injection exercises the fixture gate, not real read-state authorization.readStateoption, while thepolicyRelaymodel only accepted those writes when the option was enabled. Accept and validate read-state publications in all production-broker journeys; keep complete snapshot discovery conditional. A UI-driven diagnostics action in the agent-activity journey now forces a real read-state write through the broker and asserts its receipt.Fail → pass
8842b3a, WebKit 120 repeats/6 workers: 11 failed at “Another message” while the row remained “Sending message…”. With the regression and old gate, WebKit timed out 1/1; with the fix, WebKit passed 120/120. Independent review reproduced the old-gate failure and passed the full new-message file 20/20 across both engines at the earlier PR head.policyRelayconfiguration on basea05a6f6, WebKit failed 1/1 withUnexpected publication kind 30078. With the fixture fix, the completeagent-activity.spec.mjsfile passed 10/10 across Chromium and WebKit. The prior CI failure on the first PR head showed this same exception in WebKit shard 1/2: https://github.com/block/buzz-app/actions/runs/36046562032/job/107791450884 .29af0abffcc628e7e91625bf60d6e64c7f42e658rebased onf6caa83f724253ca11fb83481e7b60f091b4cf7a: complete touched filesnew-message.spec.mjsandagent-activity.spec.mjspass 12/12 across Chromium and WebKit (2 workers,--no-deps). The broad local--no-deps --workers=1browser run selected 586 cases but hit the 20-minute shell cap around the halfway mark; it is incomplete, not a pass.No browser cases removed; no retries, timeout changes, or relaxed assertions. No production files changed. Browser-only justification: the failures require the built app, its broker/socket write path, and the fixture's upstream relay model. Hosted CI at the combined head remains pending. A separate WebKit double-mention flake and other intermittent failures remain outside this PR.