Skip to content

fix(terminal): retry a two-stage send without repeating paste or Enter - #29234

Merged
teamleaderleo merged 6 commits into
feat-cmux-nextfrom
sw10/terminal-send-replay-20261031
Nov 1, 2026
Merged

teamleaderleo merged 6 commits into
feat-cmux-nextfrom
sw10/terminal-send-replay-20261031

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Oct 31, 2026 •

Copy link
Copy Markdown
Collaborator

What changes

A single cmux terminal <id> send / MCP agents_terminal_send is two daemon mutations: bracketed-paste the text, then Enter. Until now each phase created a fresh random idempotency key. If the daemon applied a write but its acknowledgement was lost, retrying the whole send could paste the text twice or run the shell command twice.

This PR gives that composed operation one stable idempotency_key. The two child mutations receive distinct deterministic SHA-256-derived keys bound to that send ID, resolved terminal and phase. When a write's outcome is uncertain, CLI and MCP return the whole-send key plus details.phase; retrying with the key replays the daemon's previously accepted stage result. The complete send receipt also carries its ID. Callers may provide --idempotency-key in the CLI or idempotency_key in MCP; without one, the CLI generates a key and returns it on an uncertain write.

Rebuilt on feat-cmux-next (2026-11-01): #29207 was closed because base already routes the terminal verbs through status::run_args -> terminal_talk::run, so this PR's diff is only the send change, the CLI key forwarding, the MCP input and schema, and the tests.

Changelog

  • Fixed: Retrying an uncertain terminal-agent send can reuse the same paste and Enter request identities instead of duplicating terminal input.

Tests added

  • MCP descriptor/argument validation for an optional 1–128 byte stable key (test-first commit).
  • A fake daemon that applies the paste or Enter then loses the reply. Replaying with the same key must apply exactly one paste and one Enter.
  • Generated-key recovery after a lost paste acknowledgement.
  • The existing ordinary send tests and default behavior remain unchanged.

Scope and limitations

  • Uses the existing daemon's idempotency store; replay safety is bounded by that owner's key retention and consistency contract. This PR adds no new persistent send journal.
  • The same key is for the same terminal and prompt. A different prompt or a new run needs a new key; daemon payload-fingerprint checks remain authoritative.
  • This controls write duplication after a lost reply, not concurrent human typing or a terminal that has been closed/recreated. Those need an explicit revision/owner guard and a new integration test before claiming a complete atomic submit.
  • The existing special handling for permission dialogs stays unchanged. Do not use --force as implicit consent.

Observed / Not run

Observed: inspected feat-cmux-next@07088897284b6637a4ddb5763e06ca1601d4aae2 and the pushed diff against #29207's head b7a5371a819a0d7fa89191296ee2efa022dc41ff; original research and owner handoff in cmux #23026 and SW-10. The test-only MCP schema commit precedes the implementation.

Not run: Rust compiler, cargo test, real terminal/harness fault injection, hosted verification and independent review. This chat has GitHub write access but no Rust compiler or authorized Testbox. Draft until exact-head tests and review complete.

Suggested verification: ./scripts/verify-cmux-tui-hosted.sh --filter terminal_send_retry_replays_both_writes_after_either_lost_ack; --filter terminal_send_generated_key_is_returned_on_a_lost_reply; --filter terminal_send_exposes (or the MCP descriptor test name). Run a full terminal-talk suite and a live same-key retry against an isolated terminal before landing.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.


Summary by cubic

Fixes retrying a terminal send after a lost acknowledgement so it doesn't paste the text twice or run the shell command twice.

A cmux terminal <id> send (and the MCP agents_terminal_send tool) performs two daemon writes: bracketed paste, then Enter. Each write previously used a fresh random idempotency key, so retrying after an uncertain write could apply the same stage twice. Now both writes derive deterministic keys from one stable send identity, and retrying with that identity replays the daemon's previously accepted stage.

  • Callers can pass --idempotency-key/idempotency_key; without one, the CLI generates a key and returns it on an uncertain write.
  • Uncertain failures report the send key and the paste/enter phase, and the send receipt carries the key.
  • Adds tests using a fake daemon that drops acknowledgements, verifying a same-key retry applies exactly one paste and one Enter.

Written for commit 4a03be3. Summary will update on new commits.

View guided diff Turn on auto-fix

@teamleaderleo
teamleaderleo changed the base branch from sw10/terminal-talk-cli-route-20261031 to feat-cmux-next November 1, 2026 02:39
@github-actions github-actions Bot added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 1, 2026
@teamleaderleo
teamleaderleo force-pushed the sw10/terminal-send-replay-20261031 branch from 4a03be3 to 08525b0 Compare November 1, 2026 09:02
@teamleaderleo
teamleaderleo marked this pull request as ready for review November 1, 2026 09:02
@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review 08525b0: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr29234 -f ref=sw10/terminal-send-replay-20261031 -f platform=macos -f backend_mode=local

Shipped in:

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (claude opus, correctness first): SHIP at 08525b0

Rebuilt on feat-cmux-next: #29207's CLI wiring and cli_audit commits were dropped (base already routes terminal verbs through status::run_args -> terminal_talk::run); the diff is the PR's own 4 files.

Correctness read:

  • The paste and the Enter each get a distinct, deterministic daemon key (SHA-256 of send id + resolved terminal + stage). After a lost ack, a same-key retry replays the stage the daemon already accepted and applies only the missing one, so neither paste nor Enter repeats. The daemon keeps durable, fingerprinted receipts for terminal.input.write (resource_router/effects.rs).
  • No silent drop: every stage error is surfaced with details.phase and the whole-send key; reusing a key with a different payload hits the daemon's fingerprint conflict and fails loudly.
  • Tests: cmux-ci test -- cargo test -p cmux-tui --lib terminal_talk on the fleet Linux pool: 24 passed, 0 failed (incl. terminal_send_retry_replays_both_writes_after_either_lost_ack, terminal_send_generated_key_is_returned_on_a_lost_reply).

Follow-ups (non-blocking):

  • If the terminal's bracketed-paste mode flips between the first try and the retry, the retry's paste bytes differ, so the daemon refuses it as a key conflict rather than completing the send. That fails safe (no double input), but the message could tell the caller to use a new key.

Reds at this head are not this PR's: clippy fails on collapsible-if lints in acpmux/cmux-tui-core files this PR does not touch; cmux-next checks fail on xcstrings, the Swift daemon-capabilities export, script/job-structure/tier-routing tests over untouched files; tooling-tests is red on base too.

@teamleaderleo
teamleaderleo merged commit d8e1b7c into feat-cmux-next Nov 1, 2026
209 of 238 checks passed
@teamleaderleo
teamleaderleo deleted the sw10/terminal-send-replay-20261031 branch November 1, 2026 09:42
lawrencecchen added a commit that referenced this pull request Nov 1, 2026
…ads use get

The ratchet at the merge with feat-cmux-next e27f116 (18cef5a,
job 9d666b02dcb4c69dbfc147f0) is still red: cmux-tui indexing_slicing
849 > 831. The 18 new hits after #30414 come from:
- d8e1b7c (#29234) cli/terminal_talk/send.rs stage_failure: 2
- 9f5e2e1 (#30634) link/seed_cli.rs: 13, link/mod.rs `&args[1..]`: 1
- 7b30784 (#30664) link/machine_pin.rs: 1, link/mod.rs `&args[2..]`: 1

Each is now a get / pointer-free get read, `JsonSet::set` for the one
write, and `args.get(n..)` for the slices (the branch runs only when
`args.get(n - 1)` matched, so the slice is always there). A missing key
read through get gives None where indexing gave Null: `as_str()` and
`as_bool()` see the same, and json! writes None as null.
No behavior change; the baseline is not raised.
@lawrencecchen

Copy link
Copy Markdown
Contributor

This PR added new clippy::indexing_slicing hits to cmux-tui production code: Value v["key"] reads or writes, or &args[n..]. Together with #30414, #29234 and #30634 / #30664 make the panic ratchet red at feat tip e27f116: cmux-tui indexing_slicing 849 > 831 (job 9d666b02dcb4c69dbfc147f0). #30661 fixes them forward with get/JsonSet::set/args.get(n..) and does not change behavior. Before you land, run cmux-ci run --class light --script scripts/ci/cmux-tui-rust-check.sh --arg panic --os linux --ref SHA.

lawrencecchen added a commit that referenced this pull request Nov 1, 2026
…0414, #29234, #30634, #30664 and #30574 use checked forms (#30661)

* base red: panic ratchet, #30414's new index and string slices in start_chat_cli use checked forms

The Rust panic ratchet is red on feat-cmux-next at 7e0a0f2 (tip
watcher job 21c6ba0ebda787821cc7bc4b): cmux-tui indexing_slicing
833 > 831 and string_slice 84 > 82. All four hits are from 0507fc4
(#30414) in cmux-tui/src/link/start_chat_cli.rs:
- valid_environment: `id[4..]` -> `id.get(4..)`; the id already starts
  with the ASCII "env_", so the slice is always Some and the same.
- github_repo: `&url[scheme.len()..]` -> `split_at_checked`, None in
  exactly the cases where the `get(..scheme.len())` before it was None.
- Ensure::body: `body["ref"] = ..`, `body["environment"] = ..` ->
  `JsonSet::set` on the same object literal (same insert).
No behavior change; the baseline is not raised.

* base red: panic ratchet, #29234's, #30634's and #30664's new index reads use get

The ratchet at the merge with feat-cmux-next e27f116 (18cef5a,
job 9d666b02dcb4c69dbfc147f0) is still red: cmux-tui indexing_slicing
849 > 831. The 18 new hits after #30414 come from:
- d8e1b7c (#29234) cli/terminal_talk/send.rs stage_failure: 2
- 9f5e2e1 (#30634) link/seed_cli.rs: 13, link/mod.rs `&args[1..]`: 1
- 7b30784 (#30664) link/machine_pin.rs: 1, link/mod.rs `&args[2..]`: 1

Each is now a get / pointer-free get read, `JsonSet::set` for the one
write, and `args.get(n..)` for the slices (the branch runs only when
`args.get(n - 1)` matched, so the slice is always there). A missing key
read through get gives None where indexing gave Null: `as_str()` and
`as_bool()` see the same, and json! writes None as null.
No behavior change; the baseline is not raised.

* base red: panic ratchet, #30574's new index reads in start_chat_cli use get

#30574 (93772c4) landed after the e27f116 merge and added four
Value index reads in start-chat plan (line["machine"], ["created"],
["snapshot"], ["environment"]). They are now get reads: a missing key
is None where indexing gave Null, as_str() sees the same and json!
writes None as null. No behavior change.

This branch was previously deployed

3 inactive deployments
artifacts — 08525b06 Deployed Nov 1, 2026 by teamleaderleo via publish tree-addressed cmux-tui #94822
sentry-dsyms — 08525b06 Deployed Nov 1, 2026 by teamleaderleo via upload cmux-tui dSYMs to Sentry #94822
ci-routing-agent — 08525b06 Deployed Nov 1, 2026 by github-actions[bot] via cmux-next macOS placement #58341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants