Skip to content

[#323] Let a Send wait for the composer to arm, not for the brief to paint - #330

Merged
realproject7 merged 1 commit into
mainfrom
task/323-send-zero-posts
Aug 15, 2026
Merged

[#323] Let a Send wait for the composer to arm, not for the brief to paint#330
realproject7 merged 1 commit into
mainfrom
task/323-send-zero-posts

Conversation

@realproject7

Copy link
Copy Markdown
Owner

Closes #323

Determination (required before any fix)

Wrong test expectation. Not a product defect. Nothing is shipped-broken, and src/browser/room.js's behaviour is unchanged.

enterRoom() paints the brief first and arms the composer last:

enterRoom()
  await Promise.all([loadBrief(), loadStatus()])   ← the brief renders HERE
  ... recordJoinedRoomLocal / seedEntryFromBackup
  await pollMessages()                             ← a full round trip
  bindEvents()  → state.composerReady = true       ← armed HERE (room.js:483-485)

The failing tests gated their first Send on text=Ship the browser room safely. and on .composer. Neither means entry finished — the brief renders at the start of entry and .composer exists from first paint. The click therefore lands inside #268's refusal window, the guard refuses it exactly as designed, no POST is issued, and the wait for the message runs its full 30s ceiling.

Proved deterministically by holding the entry /status (the same instrument #268's own test uses) and probing while entry is blocked:

[repro] brief visible while /status still held:    true   ← the "entry finished" signal, with entry BLOCKED
[repro] composer present while /status still held: 1
[repro] POSTs after the click:                     0
[repro] notice: "Still joining the room — wait for the roster to load, then send."
[repro] message rendered:                          0

This matches the CI evidence exactly — run 31668708072:

✖ Send before entry completes cannot navigate or eject the participant (#268) (30557ms)
  page.waitForSelector: Timeout 30000ms exceeded.
    - waiting for locator('text=click after entry') to be visible

That is the post-entry click, not the early one. Page healthy, all responses seen, zero POSTs: #303's recorder signature was correct and it was pointing at the test.

The change

bindEvents() publishes the readiness it already tracked internally — composer.dataset.ready = "true", set on the line beside state.composerReady = true so the two cannot drift — and the four tests whose first send was gated on a paint-time proxy now wait on .composer[data-ready="true"].

No wait raised, loosened, skipped or sleep-padded. No assertion weakened or removed. The gate is narrower than what it replaces, not wider: .composer[data-ready="true"] is strictly stronger than .composer.

Four sites, not three (a correction to my chat report at msg 1677, which said the #268 test plus two others):

line test previous gate
222 browser composer dedupes rapid submit … brief text only — this one I missed in the first count
429 browser reply affordance … (#113) brief text only
1137 last-message rail KV updates … (#121/#123) brief text only
2504 Send before entry completes … (#268) brief text + .composer

I re-checked all 16 send sites in the file. The remaining ones are safe because each first exercises a control bound inside bindEvents() (#brief-refresh, #brief-open, #broadcast-toggle, #notify-toggle, or the #message-text input handler that drives the mention autocomplete), which proves arming, or waits on a message rendered by the first pollMessages() — after which bindEvents() runs with no intervening await, so no other task can interleave.

The regression the ticket asked for

Per the ticket: "assert the actual refusal and preserve a regression that fails if the #268 defect returns."

The #268 test now counts POSTs to /messages. Zero is asserted where zero is correct (the refused click, the refused Enter) and the count is asserted again after the gate, where zero is the defect #323 was opened on. Nothing on screen distinguishes a deliberate refusal from a lost send; only the request count does.

Red-first controls (every one run on this tree)

Control Result
Old proxy wait (.composer) + a deterministically widened entry window (first poll held 2s) FAIL — reproduces CI byte-for-byte: page.waitForSelector: Timeout 30000ms exceeded — waiting for locator('text=click after entry'), 39.7s
New gate + the identical widened window PASS, 4.19s
Ready marker never published (composer.dataset.ready deleted) FAIL ×4 — all four gated tests time out on waiting for locator('.composer[data-ready="true"]'). The gate is not vacuous
Refusal shows the notice but posts anyway FAIL on the new the refused click posted a message anywayactual: [ 'http://127.0.0.1:59930/messages' ], expected: []
Real #268 defect restored (type="submit", both load-time guards deleted) FAIL in 3.1s on clicking Send before entry navigated the page instead of refusing it

Diagnostics signatures kept distinct

test/support/browser-diagnostics.ts and src/browser/restored-provenance.js are unchanged. The three signatures stay separate and the distinction is now named in the #268 test's own comments: #248 = a refused request, #319 = a request whose reply never came, #323 = no request at all.

Base-failure confirmation (as instructed)

Full suite on this branch: 626 · 623 pass · 3 fail.

Each failure was checked against a clean worktree at base d22169a (git worktree add, built from that tree):

test on branch on clean base d22169a verdict
a room held under both kinds … (#311) FAIL 32.9s — waiting for '.joined-row:has-text("Both Room") .joined-seat[data-seat="agent"]' FAIL 31.0s — same selector pre-existing
a dashboard open re-authenticates … (#248) FAIL 31.8s — waiting for 'text=posting as the selected identity' FAIL 32.0s — same selector pre-existing
a tampered backup renders only as restored-from-this-device … (#278) FAIL 31.7s — waiting for 'text=forged host claim' passes in isolation on base and on branch; see below pre-existing, and diagnosed

The third one is not mine, and I am not asking anyone to take that on a correlation. It has a demonstrated mechanism on the clean base, and it is the same stale premise #319 had:

Additional evidence that it is not intra-file to my change: dist/test/browser-room.test.js run whole on this branch — 59 pass, 0 fail (250.9s), the tampered-backup test included.

EPIC Alignment

Parent: none — standalone (#323 carries no ## EPIC Context parent; Batch 62 sequences #323#307#306). Merged siblings read: #268 (the early-send guard this preserves), #312 (the confirmation pass), #319/#321 (the sibling test premise), #303 (the diagnostics recorder whose zero-POST signature opened this ticket). Nothing here reopens #268 — its guard, its notice and its type="button" are untouched, and the restored-defect control above proves the guard is still what the test catches. #306/#307 are not touched.

Self-Verification

  • Adversarial diff re-read: 2 files, +50/−4. Every added line is a wait that is strictly narrower than the one it replaces, an assertion, or a comment. The single product line sets a data-* attribute; grep over src/browser/*.css confirms no rule keys on data-ready, so there is no visual or behavioural effect.
  • Kill-list scan: no widened wait, no waitForTimeout/sleep padding, no click retry, no skipped or weakened test, no --force, no re-run to green, no diagnostics change, no [P1][Room] Send before entry completes navigates the page and ejects the participant #268 reopening.
  • Build + test evidence: pnpm lint ✓ · pnpm typecheck ✓ · pnpm no-stub ✓ · git diff --check ✓ · node scripts/diagnostics-attachment-status.mjs156/156 browser blocks covered.
  • Acceptance criteria 1:1: (1) product-vs-expectation determined before any fix ✓ (2) determination is expectation, so the actual refusal is asserted ✓ (3) a regression that fails if the [P1][Room] Send before entry completes navigates the page and ejects the participant #268 defect returns — control 5 ✓ (4) three diagnostics signatures preserved as distinct ✓ (5) restored-provenance.js / browser-diagnostics.ts unchanged ✓.

Design Fidelity

No visual change. The one product edit sets a data-* attribute with no CSS rule bound to it (grep across src/browser/*.css above); nothing rendered, laid out, or announced to assistive technology changes.

Deviations

  1. Four test sites, not the three I reported in chat (msg 1677). The browser composer dedupes rapid submit … test has the identical latent race and I had missed it in the first count. It is the same demonstrated condition @Head authorized keeping in this PR; flagging the count correction rather than letting it pass silently.
  2. Base is d22169a as assigned; origin/main has since advanced to 13446c0 ([#327] Cut v0.2.5 release source, reconciled against the ten-merge range #328/[P1][Release] Cut v0.2.5 up to the npm publish gate #327: release-notes + version triplet). That range touches docs/release-notes-v0.2.5.md, package.json, src/cli/help.ts, test/cli-help.test.ts — none of this PR's surface. Say the word and I will rebase.
  3. The third suite failure is reported, not fixed. It needs its own ticket; the diagnosis and the base-only control are above so the next lane does not have to rediscover them.

…paint

#323's zero-POST Send is not a product defect: #268's early-send guard did
exactly what it exists to do. `enterRoom()` paints the brief FIRST and calls
`bindEvents()` LAST, with a full `pollMessages()` round trip in between, so the
two signals the tests gated on -- the brief text and `.composer` -- are both
true from first paint, before entry has armed anything. A click on that guess
lands inside the refusal window, is refused, issues no POST at all, and the
wait for its message then runs the full 30s ceiling.

`bindEvents()` now publishes the readiness it already tracks
(`composer.dataset.ready`), beside the flag it mirrors so the two cannot drift,
and the four tests whose first send was gated on a paint-time proxy wait on it.
No wait is raised, loosened, skipped or sleep-padded, and no assertion is
weakened -- the refusal itself gains one: a refused Send must issue no POST,
counted where zero is correct and counted again after the gate where zero is
the defect this ticket was opened on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@project7-interns project7-interns left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@re1 (local)

Verdict: APPROVE

Epic Alignment: PASS

#323 preserves #268’s early-send refusal while replacing false paint-time readiness proxies with the actual post-pollMessages() arming boundary, without conflating #248/#319/#323 signatures.

Checked (evidence)

  • Structural gate: filled ## EPIC Alignment, ## Self-Verification, and applicable Design Fidelity/Deviations sections in the live PR body.
  • Actual readiness: src/browser/room.js:483-494 publishes data-ready beside the existing state.composerReady transition; no send/refusal behavior changes.
  • Regression boundary: test/browser-room.test.ts:2421-2520 asserts zero /messages POSTs for refused Send/Enter, then exactly one/two after the ready gate.
  • Broader test corrections: test/browser-room.test.ts:215-220, :425-429, and :1133-1137 use the same real gate; Head authorized the demonstrated same-condition scope.
  • Riskiest part: the readiness marker could mask an early-send regression if it were set too early; its placement in bindEvents() and the retained refused-click assertions make that boundary observable and non-vacuous.
  • Kill-list: scanned full two-file diff — clean; no widened waits, retries, sleep padding, skipped/weakened assertions, diagnostics edits, or scope beyond the approved identical race class.
  • CI: gh pr checks 330 → Release gates pass; gh run view 31867892707 → success, attempt 1, SHA 6ab48f6766e656004ad5747db3cdde3948f866c1; /attempts/2 → HTTP 404.

Findings

  • None.

Decision

The stated expectation diagnosis is supported by the diff, and the new request-count assertions preserve both the deliberate refusal and post-entry send regression boundary. Current-SHA CI is green on its first recorded attempt.

@project7-interns project7-interns left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@re2 (local)

Verdict: APPROVE

Reviewed 6ab48f6766e656004ad5747db3cdde3948f866c1. Chat re-read by range and head re-read immediately before submitting. Lane check: the operator dispatched #323 to this instance by name (msg 1668).

Epic Alignment: PASS

The determination was made before the fix and is evidenced, the product's behaviour is unchanged, and the fix publishes the gate instead of widening a wait.

Checked (evidence)

  • The product change cannot drift, which is the property the comment claims. state.composerReady = true (room.js:485) and composer.dataset.ready = "true" (:493) are the only occurrences of either, both inside bindEvents(), and neither is ever cleared. bindEvents() has exactly one call site (:426, inside enterRoom()), which the #241 entry boundary already makes idempotent. The refusal guard at :151/:158 still reads state.composerReady and is untouched — so this is purely additive publication of existing state, not a behaviour change.
  • The selector genuinely matches and is strictly narrower. #composer carries class="composer k-composer" (room.html:187), so .composer[data-ready="true"] is the same element the old .composer matched, plus one condition. A narrower gate cannot be a widened wait.
  • Nothing was weakened; four assertions were addedmessagePosts empty after the refused click and the refused Enter, then length === 1 and === 2 after entry. The last two are the ones that matter: zero POSTs is #323's exact signature, asserted where it is correct and counted again where it would be the defect. Nothing on screen distinguishes those two states, which is why the request count is the right instrument.
  • #268 is not re-opened — it is more strongly pinned. The guard is unmodified and the refusal now has to prove it issued no request, not merely that it showed a notice.
  • Scope: 2 files. src/browser/restored-provenance.js and test/support/browser-diagnostics.ts are untouched, as the ticket required.
  • CI: run 31867892707, run_attempt: 1, conclusion: success, sole run on this SHA — and /attempts/2 returns 404, which is the positive evidence that nothing was retried. @dev applied the audit method from my msg 1662 unprompted; the head-sha query alone would not have shown it.
  • Riskiest part of this diff: that data-ready could be published somewhere the flag is not, making the gate lie. The single-site check above is what rules it out, and it is the reason to keep both writes adjacent.
  • Kill-list: scanned — clean.

Findings

None.

The third failure @dev diagnosed is mine to own

browser-room.test.ts:2648 waits for text=forged host claim and the same test asserts at :2678 that it is gone. The wait races its own assertion, because #312's confirmation pass erases a forged row at a real id. Verified on origin/main with this diff absent.

I reviewed that test in PR #315 and approved it. The settle wait [data-author-confirmed="true"] was added directly beneath the stale wait, and I read the added line without re-reading the line above it — the exact failure my own standing note describes as defects living at seams. It is the second test in this family I have missed after #319 was the first. @dev found it, attributed it to a clean base, and correctly kept it out of scope; @Head has it as #331.

Decision

One additive product line, four gates narrowed rather than widened, four assertions added, five red-first controls including one that reproduces the CI failure verbatim, and a first-attempt green with /attempts/2 proving no retry.

@realproject7
realproject7 merged commit 66886c1 into main Aug 15, 2026
1 check passed
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.

[P1][Room] A Send click produces zero POSTs — product regression or wrong test expectation, undetermined

2 participants