Fix #1573: verify delivery reached the terminal before reporting it delivered - #1577
Merged
Conversation
…elivered `[ok] Message delivered` meant only "frames queued on a connected socket". Nothing between the CLI flag and the PTY bytes checked size, freshness, or arrival, so a receiving composer that ate the leading bytes (#1564) or was still repainting after a turn (#1521) produced a clean success at the sender. PIR #1365 closed the Tower-side trigger; these are the residuals. Three changes at the converged write edge: - Settle-before-write: require the session's screen to have been quiet for SETTLE_BEFORE_WRITE_MS (250) before writing, checked before the per-terminal lock and again inside it. Phrased so an unknown screen age holds rather than writes blind. - A loud 48KB body limit shared by the CLI and POST /api/send, plus bodyLength echoed on the response and printed by `afx send`. Never a silent truncation. - Echo verification: after a completed write and before markDelivered, look for the message's header line on the session's rendered mirror. Absent means "could not confirm", so the row is held for redelivery — the direction of error becomes a duplicate the agent can see, never a silent loss. The needle is normalized to its alphanumeric skeleton, measured against live claude and codex PTYs: claude echoes the `###`-fenced header verbatim into its composer, then markdown-strips the fences on submit, so a literal match would fail on every short claude delivery. The issue's optional sacrificial leading newline is not included: it would send a bare `\n` as its own write, and whether a harness reads that as a newline or a submit is the per-harness behaviour I could not measure for codex and agy.
CMAP round 1 (codex, REQUEST_CHANGES): verifying mere presence of the header left the false receipt in place, one redelivery later. A held attempt leaves its own echo in the same scrollback, so the next redelivery matched the copy the first attempt left behind and marked the row delivered even when the retry's bytes were swallowed. Two messages formatted in the same millisecond collide the same way, and a `--raw` first line repeating existing screen text verified vacuously. The port is now watch-then-verify: `watchEcho` is called immediately before the write and samples how many times the needle currently appears; `verify()` polls until that count is strictly greater. Returning an `EchoWatch` rather than exposing two ports makes the ordering impossible to get wrong — you cannot verify without having sampled first. Two binding-level regression tests against a real SessionScreen: a stale header plus a swallowed retry stays false, and a redelivery that does land is still true. A delivery-level assertion pins the order as watch → write → verify. Also from claude's review: dropped a stray eslint-disable left behind by a removed debug line, and pointed `commands/reset.ts`'s third independent `48 * 1024` literal at MAX_MESSAGE_BYTES — its --file content rides the message body, so a drifted literal would accept a file the send route then rejects.
CMAP round 2 (claude). Unifying the 48KB constant meant `afx refresh --file` could now be refused by the send route with a 400, because its addendum rides inside the generated prompt and only the file, not the total body, was checked locally. A shared `messageLimitError` helper is now called by both commands, so the refusal is immediate and identically worded wherever a body is assembled. Also corrects a doc link left pointing at the retired `verifyEcho` port, and records two verification residuals at the binding: a very long write can evict the pre-write copy from the 1000-line mirror so the count reads equal rather than greater, and an unconfirmed delivery re-normalizes the retained buffer once per poll. Both fail in the safe direction, and the second is off the happy path. The unbounded-redelivery residual claude asked to see bounded or filed is filed as #1578: bounding it needs either a new mailbox column or accepting a false delivery after N attempts, which is a decision rather than a bugfix.
#1575 (self-attesting frames, #1574) landed first and touches the same two files. Both conflicts were additive-vs-additive and keep both sides: the message-body size limit alongside the recipient/reply-hint helpers in message-format.ts, and one import block in tower-routes.ts. The frames now read `### [ARCHITECT INSTRUCTION → <toAgent> | <ts>] ###`, which composes with echo verification rather than breaking it. The needle is derived from the row's actual formatted_message, so it picked up the new shape for free; the arrow normalizes away as punctuation while the recipient NAME stays in the needle, making verification recipient-specific — a frame delivered to the wrong agent cannot satisfy the right agent's check. The frame is still 3 lines, so #1574's own constraint about PACED_WRITE_LINE_THRESHOLD holds and the write path this change guards is byte-identical. Fixtures now build the frame with formatArchitectToBuilderMessage instead of hand-writing it: a hand-copied header would have kept passing while the frame it modelled drifted away from what the delivery path writes.
Contributor
Author
|
Architect review (posted as comment — GitHub blocks a formal review from the PR author's own account): APPROVED and admin-merged on the owner's explicit word. Matches the #1573 prescription with three reviewed-and-accepted deviations (sacrificial newline dropped per the issue's escape hatch; unbounded-redelivery residual escalated as #1578; shared 48KB ceiling's --file behavior change accepted). The occurrence-count echo-verification (new evidence, not presence) is the right answer to codex's stale-echo finding, and measuring real harness rendering before shipping the matcher was exactly the discipline this write edge needed. CI green on 416eaf3 post-rebase. |
waleedkadous
added a commit
that referenced
this pull request
Sep 1, 2026
This was referenced Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[ok] Message deliveredmeant only "frames queued on a connected socket" — Tower had no evidence the receiving terminal absorbed anything. This adds the three checks that make the receipt honest: the screen must have settled before we write, the body must be within a stated size limit, and the message's header must actually appear on the terminal — as a result of this write — before the row is marked delivered.Fixes #1573
Root Cause
Confirmed from the post-#1492 code, not inferred:
submitMessagePacedresolveswrittenwhen everysession.write()returned true, andShellperClient.writereturns true iff the socket object is connected.deliverAgentMailcalledmarkDeliveredon that alone — no echo, no ACK. A composer that ate the leading bytes (afx send: long architect→builder messages truncated or delivered empty-body #1564) was indistinguishable from a clean delivery.session.lastDataAtwas never consulted. The quiescence drain trigger had an accidental 500 ms settle; the request path and thesubmitfast trigger had none — matching tower: mailbox delivery race eats leading bytes when the write lands mid-composer-settle #1521's "on-demand relays hit it more".--filecap.Fix
1. Settle-before-write.
DeliverySessiongainslastDataAt. Delivery requiresnow − lastDataAt ≥ SETTLE_BEFORE_WRITE_MS(250) both before the per-terminal lock and again inside the precheck, mirroring how the other three write-instant conditions are already double-checked. Written as a positive>=so a session with no usable timestamp (NaN) reads as not settled and holds — a fail-open there would look like a guarantee while being none. One existing test double did in fact lack the field.2. Loud 48KB limit.
MAX_MESSAGE_BYTES+ a shared error string inutils/message-format.ts, enforced at the CLI (local, immediate) and atPOST /api/send(public route).MAX_FILE_SIZEin bothcommands/send.tsandcommands/reset.tsis now defined as that constant, so the three former copies of48 * 1024cannot drift, and the CLI checks after the--fileappend because attachment content travels in the same body.bodyLengthrides every send response that carried a body, through the SDK toafx send's success line. Never a silent truncation.3. Echo verification. A new required
watchEchoport. It is opened immediately before the write and samples how many times the message's header currently appears on the session'sgateScreen— the same mirror the gate classifies; the returnedEchoWatch.verify()then polls (50 ms, up to 600 ms, full retained buffer including scrollback) until that count is strictly greater.New evidence, not mere presence, and that distinction is load-bearing: a held attempt leaves its own echo in the same scrollback, so a presence check would match the copy the first attempt left behind and certify a retry whose bytes were swallowed — the false receipt postponed by one redelivery rather than removed. Returning an
EchoWatchrather than exposing sample and verify as two ports makes the ordering impossible to get wrong.A negative means "could not confirm", so the row is held for the existing redelivery machinery: the direction of error becomes a duplicate the agent can see, never a silent loss the sender was told was a success.
Why the needle is normalized
I measured this against live PTYs rather than guessing, because the issue's scope guard asks for change 3 to be cut if it cannot be done reliably.
The load-bearing surprise: claude markdown-renders the header on submit. The composer echoes
### [ARCHITECT INSTRUCTION | <ts>] ###verbatim; the transcript then shows[ARCHITECT INSTRUCTION | <ts>], the fences consumed as an H3. An exact-line match would fail on every short claude delivery — the common case. Reducing both sides to their alphanumeric skeleton fixes that and incidentally absorbs quote prefixes, indentation, and wrapping. It stays header-only and harness-agnostic: no screen diffing, no repair, no per-harness branches. The tail is deliberately not used — #1564's message arrived as its final ~30 characters, so a footer needle would have certified the exact corruption this check exists to catch.Deliberate omissions and residuals
writeMessageToSessionwould send a bare\nas its own write, and whether a harness treats that as "insert newline" or "submit" is exactly the per-harness behaviour I could not measure for codex and agy. An empty submit ahead of every message is a worse failure than the head-eating it guards against, which change 1 already addresses. The issue marks it optional and says to drop it at the first sign of harness weirdness.afx sendtraffic that carries a header) and raw sends with a distinctive first line.markEscalatedsets a flag and fires an SSE event) but does not stop the drainer, so the rewrite loop itself is unbounded: one duplicate per clean gate for as long as the harness refuses to show the header. Deliberately not bounded here — every way to bound it means either a new mailbox column or accepting a false delivery after N attempts, and that trade is a decision rather than a bugfix.afx sendrequest waits. Bounded by the 600 ms timeout and off the happy path entirely — the measured case confirms on the first read.--fileattachment at or near 48KB plus any message text now fails where it previously went through — forafx sendand, because its--fileaddendum rides inside the generated prompt, forafx refreshtoo. That is the shared ceiling working as intended, but it is a real change. Both commands check the total body locally (sharedmessageLimitError), so the refusal is immediate and identically worded rather than a 400 from the route.Test Plan
bugfix-1573-delivery-verification.test.ts, 18 tests: the settle window (inside / at the boundary / re-checked in-lock / NaN), the control test (completed write + header never shown → held, not delivered, no broadcast), confirmed delivery with thewatch → write → verifyordering pinned, redelivery of a held row, short-needle skip, needle normalization against the three measured rendered forms, andwatchEchoOnScreenagainst a realSessionScreen(composer form, markdown-stripped form, scrolled-into-scrollback, stale header from an earlier attempt, a redelivery that does land, absent, early return). Plus route tests for over-limit / at-limit /bodyLength, and CLI tests for the local refusal, the--fileinteraction, and the byte-count echo.Review
CMAP round 1 — gemini APPROVE, claude APPROVE, codex REQUEST_CHANGES: the stale-evidence hole above, which was real and is fixed in
d611bb7. Claude's smaller findings (a strayeslint-disableleft by a removed debug line, andreset.ts's third48 * 1024literal) are fixed in the same commit.CMAP round 2 — gemini APPROVE, codex APPROVE (flipped, no remaining issues), claude COMMENT with five findings, all addressed: the unbounded-redelivery residual is filed as #1578;
afx refresh's route-400 regression is fixed with a local precheck; the stale{@link DeliveryPorts.verifyEcho}doc link and the test count in this body are corrected; the count false-negative and the per-poll re-normalization are documented as residuals here and at the binding.tower-routes.test.ts'sgateSessiondouble now echoes writes into its own mirror, because a real terminal does and the delivery path now depends on it. The #1492 suite stays green.Rebased onto #1575 (self-attesting frames, #1574)
#1575 landed first and touches
message-format.tsandtower-routes.ts. Both conflicts were additive-vs-additive and keep both sides.The new frame composes with echo verification rather than breaking it, and strengthens it. Headers now read
### [ARCHITECT INSTRUCTION → <toAgent> | <ts>] ###; the needle is derived from the row's actualformatted_message, so it picked the new shape up for free. The arrow normalizes away as punctuation while the recipient name stays in the needle — so verification is now recipient-specific: a frame delivered to the wrong agent cannot satisfy the right agent's check. The frame is still 3 lines, so #1574's own constraint aboutPACED_WRITE_LINE_THRESHOLDholds and the write path this PR guards is byte-identical.Fixtures now build the frame with
formatArchitectToBuilderMessagerather than hand-writing it, plus an explicit assertion that the recipient segment reaches the needle. That is the durable lesson from the rebase: a hand-copied header would have gone on passing while the frame it modelled drifted away from what the delivery path actually writes.Post-merge: 5360 passed, 48 skipped, 0 failures (including #1574's 16 new tests).