Skip to content

The held-read control holds the read it names - #1734

Merged
trissim merged 1 commit into
mainfrom
fix/the-held-read-control-holds-its-own-read
Nov 11, 2026
Merged

trissim merged 1 commit into
mainfrom
fix/the-held-read-control-holds-its-own-read

Conversation

@trissim

@trissim trissim commented Nov 11, 2026

Copy link
Copy Markdown

test_a_held_child_read_leaves_paint_and_input_live_and_pages_when_it_is_released
landed with #1727 and passes one run in three. A control that proves its claim a
third of the time proves nothing, and this one guards a real user-visible promise: that
typing lands and the message area still draws while a read is outstanding in the child.

The product is not at fault. Instrumenting the held reply recorded
resumed=True delivered=True on release: the child answers, and the answer is
delivered. Three defects in the control, each measured:

It could hold a reply nobody was waiting for

A failing run recorded process.reads.get(self.request_id) as None while entered
was set. received() says what happens to those:

pending = process.reads.get(self.request_id)
if pending is None or pending.waiter.done():
    return  # Cancelled, retired, or already answered: the same ignored reply as ever.

Holding one holds nothing and releases nothing, and the test then waits ten seconds for
a page that was never owed.

It could hold the wrong read

Patching the reply hook catches every read in flight. A failing run recorded
HELD CoreReadOperation CommsConversationRevisionTool — a routine revision poll, not
the page. entered was set by it, the loop stopped, and the test waited for a page no
press had asked for.

entered.is_set() can only ever detect the absence of a read. It cannot detect the
wrong one, which is why the existing assertion's own words — "no child read was held,
so this proves nothing" — were true and silent.

It raced the reply it was waiting for

entered is set when the page's reply arrives, but the loop checked it immediately
after the press and broke at scroll_y == 0 — the very point where the older page is
requested. The request was out and its answer had not landed. With the filter above in
place this became the only remaining failure, and it failed honestly: "no child read was
held". The loop now waits for that reply instead of racing it.

Checks

Four runs, four passes, on the branch; three for three after rebasing onto current main.
Whole file: 8 passed.

Interpreter built from declarations with bin/toad-test-env --agent-comms /home/ts/code/projects/agent-comms --textual /home/ts/wt/claude-laptop--textual, against
Core b8da1aac0 (P101) and Textual main 19df8339e.

Correction to something I posted in #comms

I reported test_a_growing_channel_draws_new_messages_for_a_tail_reader_and_keeps_a_scrolled_reader_in_place
as consistently red on toad main. It is intermittent — it passed in the whole-file run
above. I generalised from one observation. comms-phone-performance's reading stands: the
test asserts absolute scroll_y at :163 before checking painted rows at :165, while
ReaderPosition preserves content position and layout deliberately adjusts scroll_y.

🤖 Generated with Claude Code

It passed one run in three. Three separate reasons, each measured:

Patching the reply hook catches every read in flight, so a CommsConversation
RevisionTool poll could be held instead of the page. entered was set, the loop
stopped, and the test waited for a page nobody had asked for.

A reply whose pending read is gone is discarded by received() by design, so
holding one held nothing and released nothing. A failing run recorded
process.reads.get(request_id) as None while entered was set.

entered is set when the page's REPLY arrives, but the loop checked it straight
after the press and broke at scroll_y == 0, racing the child it had just asked.
With the filter in place, this was the only failure left and it failed honestly:
no child read was held, so this proves nothing.

The product was answering correctly throughout: a held read recorded
resumed=True delivered=True on release. Four runs, four passes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings November 11, 2026 03:25
@trissim
trissim merged commit 4a09b9d into main Nov 11, 2026
2 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

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.

2 participants