fix(broker): address #1939 must-fix PTY delivery review findings

[khaliqgant]

Addresses the must-fix (M1–M6) review threads on #1939 (trunk → main, carrying #1891), plus thread #17 (OpenCode any_output, reassigned here as the OpenCode instance of M2). The other P2/P3 items are handled separately by train-relay-1939.

• M1 (pty_worker.rs ~2786): a terminally failed, already-written delivery is latched. Its id is fenced against replay for the PTY lifetime; the fence is unbounded, and a replay gets delivery_failed, never a second paste. Injection holds until the composer is provably idle, or until an operator-wide flush_injections releases it explicitly. A tail missing from the viewport is not treated as proof the draft is gone.
• M2 (delivery_verification.rs 188/185/419, detection.rs 48): generic any_output (including OpenCode) cannot confirm a draft whose tail ends at, straddles, or appears anywhere after the cursor, nor while this body's text sits to the cursor's right. Only known prompt and border glyphs are ignored; a placeholder is not body text. A wrapped-echo compact-tail match keeps the same-read suffix, so its turn marker is not lost.
• M3 (worker_events.rs 1069/1090): delivery_failed sets BlockedOnSend (and publishes stuck) only while other deliveries for the worker are pending. Otherwise the worker returns to Working and publishes working, replacing any earlier stuck.
• M4 (wrap.rs 139, pty_worker.rs 2521): recovery writes are cancellable in the PTY drainer, which re-checks before the primary key and after the follow-up delay. Human input withholds queued End/CR/LF in both worker and wrap mode. The recovery ack now assesses real acceptance after a takeover instead of always failing.
• M5 (worker_events.rs 953): a PTY delivery_verified must carry harness_acceptance or completed_replay. Legacy echo/timeout_fallback or unqualified PTY frames are ignored and do not clear custody. Headless confirmations are unchanged.
• M6 (pty_worker.rs 1422): only a non-empty, drainer-admitted write_pty takes composer ownership.

Human reviewer: please check explicitly

• Chunked Codex initial write, partial-body latch (pty_worker.rs, initial_injection_incomplete arm). This is the one fix without a dedicated regression test: the arm sits inside the worker select loop, and the repo has no harness that drives that loop. It calls the tested latch_failed_written_delivery helper, so please confirm by reading that the call happens before the arm returns and that the injection text passed is the body that was being written.
• Latch release policy. A failed draft now holds injection until the composer is provably idle (composer_is_idle), or until an operator-wide flush_injections. Harnesses the broker cannot model as idle will therefore wait for a human flush after a terminal failure. That is fail-closed by design, but it is a behaviour change worth a deliberate sign-off.

Verification (local, at 0202e9f81; CI never runs on PRs into trunk: rust-ci.yml gates changes to non-PR events or trunk→main)

Toolchain: cargo +1.97.0. The repo has no rust-toolchain pin, and CI installs dtolnay/rust-toolchain@stable (as #1947 does for StarSling). Trunk does not compile on 1.94 because of atomic_try_update (E0658).

Clean env used for broker tests: env -u RELAY_ATTEST_BROKER_HOOK_PATH -u RELAY_ATTEST_GIT_CONFIG_COUNT -u RELAY_ATTEST_GIT_CONFIG_INDEX -u RELAY_ATTEST_SESSION_ID -u GIT_CONFIG_COUNT. Inside a broker-spawned agent shell, those variables leak into the spawner:: git-hook tests; this is independent of this change.

$ cargo +1.97.0 fmt -p agent-relay-broker -p relay-pty -- --check
(no output, exit 0)

# CI's exact command (rust-ci.yml:122), run verbatim on both trees:
$ cargo +1.97.0 clippy -- -D warnings      # PR 0202e9f81: exit 0
$ cargo +1.97.0 clippy -- -D warnings      # trunk da7d54de3: exit 0

$ cargo +1.97.0 clippy -p agent-relay-broker -p relay-pty --all-targets -- -D warnings \
    -A clippy::large_enum_variant -A clippy::result_large_err
exit 0
# Without the two -A flags, the only errors are pre-existing, in untouched code:
#   large_enum_variant  crates/broker/src/node_control.rs:7930
#   result_large_err    crates/broker/src/node_control.rs:8550

$ cargo +1.97.0 test -p relay-pty --lib
test result: ok. 263 passed; 0 failed

$ <clean env> cargo +1.97.0 test -p agent-relay-broker --no-fail-fast
lib:                     test result: ok. 1445 passed; 0 failed; 7 ignored
tests/continuity.rs:     ok. 12 passed
tests/fleet_wire_fixtures.rs: ok. 2 passed
tests/journal_lock_cli.rs:    ok. 3 passed
tests/muse_startup_cli.rs:    ok. 2 passed
# Timing flakes, each in code this PR does not touch; each passed 3/3 when re-run alone:
#   snippets::tests::mcp_preflight_tolerates_broken_pipe_on_write
#   cli_mcp_args::tests::register_timeout_remains_bounded
#   pty_worker::tests::initial_native_pty_human_input_cancels_without_submit_or_replay
#   devin::tests::paste_burst_parks_but_delayed_enter_submits

Red-first (mutation) checks. Each fix was reverted on its own, then cargo +1.97.0 test -p <pkg> --lib <filter> was run and the file restored. All 15 went red. The chunked-Codex latch (Bugbot/cubic) has no dedicated test: that arm sits inside the select loop, which has no harness. It reuses the tested latch_failed_written_delivery and composer_owned_by_delivery helpers.

Item	Mutation (reverts the fix)	Killing test(s)	Result
M1 gate	composer_owned_by_delivery: drop || failed_composer_latch.is_some()	failed_written_delivery_latches_injection_and_fences_replay	FAILED 1/1
M1 release	failed_draft_released: return true unconditionally	failed_draft_latch_holds_until_the_composer_releases_it	FAILED 1/1
M2 any_output / #17	assess_harness_acceptance: drop && !tail_touches_cursor(snapshot, &tail)	generic_repaint_cannot_confirm_a_draft_straddling_the_cursor, opencode_output_cannot_confirm_a_draft_at_the_cursor	FAILED 2/2
M2 wrapped echo	end_of_compact_match: compact.rfind(..).filter(|_| false)? (no suffix preserved)	wrapped_echo_preserves_same_chunk_acceptance_activity	FAILED 1/1
M3	let still_blocked = true;	terminal_failure_without_remaining_work_does_not_block_the_worker	FAILED 1/1
M4	drainer: remove the post-delay if is_cancelled() { return Err(write_cancelled()); }	cancelled_followup_is_withheld_and_drainer_keeps_running (relay-pty)	FAILED 1/1
M5	accepted_delivery_verification: drop the echo/timeout_fallback and unqualified-PTY arms	pty_delivery_verified_requires_harness_acceptance	FAILED 1/1
M6	human_write_takes_ownership: return true	only_an_admitted_nonempty_human_write_takes_ownership	FAILED 1/1
#1954 review: long draft	drop !tail_near_cursor and !has_visible_text_at_or_after_cursor from the generic branch	generic_output_cannot_confirm_a_long_draft_with_cursor_at_its_start	FAILED 1/1
#1954 review: off-viewport	failed_draft_released: also release when the tail is absent from the viewport	a_tail_missing_from_the_viewport_does_not_release_the_latch	FAILED 1/1
#1954 review: cursor-row chrome	restore the blanket !has_visible_text_at_or_after_cursor	cursor_row_placeholder_does_not_block_generic_acceptance	FAILED 1/1
#1954 review: off-screen tail	drop !body_text_right_of_cursor	generic_output_cannot_confirm_a_draft_whose_tail_scrolled_away	FAILED 1/1
#1954 review: bordered draft head	probe the cursor row without stripping chrome	bordered_draft_head_right_of_cursor_stays_inconclusive	FAILED 1/1
#1954 review: symbolic draft head	strip every non-alphanumeric character instead of CURSOR_ROW_CHROME	symbolic_draft_head_right_of_cursor_stays_inconclusive	FAILED 1/1
#1954 review: published stuck	zero-pending case publishes stuck/blocked_on_send	last_failed_delivery_publishes_a_non_stuck_state	FAILED 1/1

Known follow-up: #1939 P2 (train-relay-1939) adds composer_holds_tail. Whichever of the two PRs merges second should switch failed_draft_released from current_composer(..).contains(&tail) to it, so a > b row inside a body cannot release the latch early.

Regression tests: failed_written_delivery_latches_injection_and_fences_replay, failed_draft_latch_holds_until_the_composer_releases_it, generic_repaint_cannot_confirm_a_draft_straddling_the_cursor, opencode_output_cannot_confirm_a_draft_at_the_cursor, wrapped_echo_preserves_same_chunk_acceptance_activity, compact_match_end_maps_back_to_raw_offsets, terminal_failure_without_remaining_work_does_not_block_the_worker, terminal_failure_with_remaining_work_stays_blocked_on_send, cancelled_followup_is_withheld_and_drainer_keeps_running, pty_delivery_verified_requires_harness_acceptance, only_an_admitted_nonempty_human_write_takes_ownership.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes core PTY injection, acceptance heuristics, and worker blocking behavior; incorrect latch release could stall queues, but the design is intentionally fail-closed with operator flush override.

Overview
Hardens PTY message delivery so relay injections are not mistaken for acceptance and failed bodies are not pasted over or replayed.

Harness acceptance (delivery_verification) now treats generic post-echo output (e.g. Muse/OpenCode any_output) as inconclusive while a draft’s tail is near the cursor, body text sits right of the cursor, or wrapped echoes need compact-tail matching. failed_draft_released only clears when the composer is provably idle—not when the tail scrolled off-screen. Snapshot::to_plain_from_cursor supports those cursor checks.

PTY worker adds a failed-composer latch: terminally failed written deliveries fence their id from replay, pause further injection until idle (or operator-wide flush_injections), and latch on partial chunked writes and human takeover. write_pty ownership applies only to non-empty, drainer-admitted writes. Submit-key recovery is cancellable via relay-pty so human input cannot fire a delayed Enter/CR on their draft.

Broker runtime ignores PTY delivery_verified without harness_acceptance / completed_replay, and after delivery_failed leaves the worker Working when no other deliveries are pending (instead of staying stuck).

^{Reviewed by Cursor Bugbot for commit 0202e9f. Bugbot is set up for automated code reviews on this repo. Configure here.}

---

Agent Relay sessions

• claude session a4d98f2e · opened via gh pr create · last active 2026-10-10

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 362d98bf-a0bb-4c38-babd-0b4a8f777ba5

---

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-09T20:23:42.615556Z	ca627bf	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[devin-ai-integration]

Devin Review found 2 potential issues.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ca627bf7d0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Record the user-visible PTY delivery fixes in Unreleased

This commit changes user-visible broker delivery behavior—failed drafts are fenced, legacy PTY confirmations are rejected, and recovery keys can be cancelled—but leaves the root changelog's empty [Unreleased] section unchanged. Add an impact-first Fixed entry and raise the heading to [Unreleased - Patch] as required for the first pending user-visible fix.

AGENTS.md reference: AGENTS.md:L31-L49

Useful? React with 👍 / 👎.

[khaliqgant]

Not adding a separate entry here. These are fixes to the PTY harness-acceptance delivery change from #1891, which has not shipped yet. The changelog entry for that change is being moved into [Unreleased] and rewritten impact-first under #1939's CHANGELOG thread, owned by train-relay-1939. Per AGENTS.md, a fix to an unreleased change belongs in that change's single entry, not a second bullet. I've asked that lane to make sure the entry covers fail-closed delivery for a failed draft and cancellation of recovery keys on human input.

[cursor]

Stale Bugbot comment from a previous run.

[cubic-dev-ai]

1 issue found across 7 files

_{You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic</sub>

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/broker/src/pty_worker.rs">

<violation number="1" location="crates/broker/src/pty_worker.rs:109">
P3: Add the required impact-first `Fixed` entry under `[Unreleased - Patch]` for these user-visible broker delivery changes.</violation>
</file>

[cubic-dev-ai]

P3: Add the required impact-first Fixed entry under [Unreleased - Patch] for these user-visible broker delivery changes.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At crates/broker/src/pty_worker.rs, line 109:

<comment>Add the required impact-first `Fixed` entry under `[Unreleased - Patch]` for these user-visible broker delivery changes.</comment>

<file context>
@@ -100,6 +101,54 @@ impl CompletedWorkerDeliveries {
+/// injected until the composer is observed without it; otherwise the next body
+/// would append to the failed draft or submit both as one turn.
+#[derive(Debug, Clone)]
+struct FailedComposerLatch {
+    delivery_id: DeliveryId,
+    event_id: EventId,
</file context>

[khaliqgant]

Declining here, same rationale as the Codex thread on line 107. These are fixes to the PTY harness-acceptance delivery change from #1891, which has not shipped. That change's single changelog entry is being moved into [Unreleased] and rewritten impact-first under #1939's CHANGELOG thread, owned by train-relay-1939. AGENTS.md wants one impact-first bullet per user-visible change, so a fix to an unreleased change belongs in that entry, not a second bullet. I've asked that lane to cover fail-closed delivery for a failed draft and cancellation of recovery keys on human input.

[cursor]

Stale Bugbot comment from a previous run.

[cursor]

Stale Bugbot comment from a previous run.

[cubic-dev-ai]

1 issue found across 3 files (changes from recent commits).

_{You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic</sub>

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/broker/src/broker/delivery_verification.rs">

<violation number="1" location="crates/broker/src/broker/delivery_verification.rs:287">
P2: A cursor-row placeholder is mistaken for draft content when the delivered message contains the same phrase, so generic output cannot confirm an accepted delivery. Exclude recognized harness chrome independently of `expected_echo` before applying this match.</violation>
</file>

[cubic-dev-ai]

P2: A cursor-row placeholder is mistaken for draft content when the delivered message contains the same phrase, so generic output cannot confirm an accepted delivery. Exclude recognized harness chrome independently of expected_echo before applying this match.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At crates/broker/src/broker/delivery_verification.rs, line 287:

<comment>A cursor-row placeholder is mistaken for draft content when the delivered message contains the same phrase, so generic output cannot confirm an accepted delivery. Exclude recognized harness chrome independently of `expected_echo` before applying this match.</comment>

<file context>
@@ -270,6 +270,23 @@ fn tail_near_cursor(snapshot: &Snapshot, tail: &str) -> bool {
+        .char_indices()
+        .nth(32)
+        .map_or(row.len(), |(index, _)| index);
+    compact_render(expected_echo).contains(&row[..probe_end])
+}
+
</file context>

[khaliqgant]

Declining, with the trade-off now documented on body_text_right_of_cursor. A misread here only fails toward Inconclusive: the delivery is reported unconfirmed and dead-lettered, never falsely confirmed. That's the direction this gate is designed to fail. Recognizing placeholders independently of the body would need a per-harness chrome registry, and the harnesses on this generic path are by definition the ones the broker has no model for. The opposite trade-off is the one Bugbot raised on this same line: dropping body-text matching lets a typed draft be confirmed. The collision also requires the body to contain the same leading 32 compact characters as the placeholder.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit c749f74. Configure here.}

[kjgbot]

@codex review

[chatgpt-codex-connector]

To use Codex here, create a Codex account and connect to github.

[miyaontherelay]

Superseded by #1959: a single PR into main that carries the #1939 promotion (#1891, #1946, #1947, the prettier fix) plus the #1954 and #1955 review fixes. Relay no longer has a merge train, so per-PR CI runs there. Its head contains this PR's head unchanged.