fix(cli-mode): deliver large prompts via temp file + never lose the prompt on spawn failure#30
Merged
Merged
Conversation
…rompt on spawn failure
The initial CLI prompt was single-quoted into the tmux new-session command,
which tmux 3.4 rejects past ~16KB ("command too long"). The failure was
invisible: create_session returned Ok when the tmux client merely spawned, and
the route cleared pending_cli_prompt immediately after — so the prompt was
destroyed while the pane fell back to an empty TUI. The loop supervisor's
send_cli_keys hit the same ~16KB send-keys ceiling.
Transport: cli_bootstrap now reads the prompt from a private 0600 file
(asset_dir/cli-prompts/<workspace>.txt) via `vk_p="$(cat 'file')"; ... "$vk_p"`,
making the tmux command O(1) in prompt size. The prompt is never re-shell-quoted
(path single-quoted; content only ever expands double-quoted). Each CliPromptArg
arm keeps identical small-prompt semantics; the file self-deletes once consumed
and is also removed on kill/reaper. send_cli_keys stages texts >=4KB through a
namespaced tmux buffer (load-buffer - + paste-buffer -d -p) with no argv ceiling.
Recovery: the route now confirms the tmux session actually exists (poll
~5x200ms) before clearing the parked prompt; if it never appears the prompt
stays parked, the temp file is removed, and a red terminal error tells the user
it is saved for the next attach. Oversized (>100KB) Positional/Flag prompts are
delivered post-launch by paste instead of being baked past MAX_ARG_STRLEN. The
loop supervisor re-parks the prompt when a wake-up delivery fails.
…fails If write_cli_prompt_file() failed while baking a small/mid initial prompt (full or read-only FS, or a permissions problem creating cli-prompts/), the bootstrap silently fell through to continue_launch() — an empty TUI on a fresh worktree — and, because the tmux session came up healthy, terminal.rs cleared the parked prompt: the user's only copy was destroyed, gated on an I/O error rather than tmux's old 16KB limit. This violated the 'never destroy the prompt' invariant. create_session now fails the spawn with a dedicated PtyError::PromptStageFailed when there is prompt content to deliver but its file cannot be staged, instead of dropping it. terminal.rs's create_session Err path then returns before the prompt-clear, leaving the DB copy parked for the next attach and surfacing the same red recovery notice as the 'session never came up' branch (now a shared CLI_PROMPT_PARKED_NOTICE constant so the two paths can't drift). Any partial prompt file is removed so a torn write can't leave a stale prompt on disk.
…mpt routing Address three review nits in the long-prompt transport: - terminal.rs: the "session never came up" recovery branch returned without close_session, leaking the PtyService map entry (one per failed attach). - pty.rs paste_via_tmux_buffer: a failed stdin write returned without reaping the load-buffer child; kill+await it so it can't linger as a zombie. - Extract the baked-vs-deferred prompt routing into a pure route_initial_prompt helper and unit-test it (blank->None, small->Baked untrimmed, oversized-> Deferred trimmed, StdinPipe->Baked, Unsupported->Deferred).
…ing first-attaches
Council round 1 (P0/P1) on the long-prompt transport:
- Baked prompts: tmux session existence is not a delivery ack — a missing
agent binary leaves a healthy-looking session while the command -v guard
skips the launch arm, and the old flow then cleared the parked DB copy
(prompt lost in an orphaned file). The bootstrap's rm-at-hand-off is now the
acknowledgement: the route polls for the prompt file to disappear (<=4s)
before clearing, and StdinPipe launches rm inside the producer group
({ cat f; rm -f -- f; } | agent) so pipes ack at consumption too, not at
agent exit. Verified on a scratch tmux socket (file survives missing-binary
pane; self-deletes at hand-off while the agent still runs).
- Deferred (>100KB) prompts: a flat 750ms sleep could paste into the
missing-binary fallback shell (prompt executed as shell input) or a
still-booting TUI (canonical-mode truncation), then clear the DB copy.
Delivery now waits for a non-shell process in the pane (tmux #{pane_pid} +
ps/pgrep; #{pane_current_command} is useless here — it reports the outer
default-shell for the pane's whole life, verified empirically), stable
across two polls, bounded at ~10s; unready panes leave the prompt parked.
- Racing first-attaches: both could pass the no-session-yet gate, restage the
same prompt file (truncate can tear the winner's in-flight cat) or paste an
oversized prompt twice. CliPromptDelivery is an in-process exclusive claim,
taken before the peek and held through confirm-then-clear; the losing attach
simply attaches without the prompt.
- wait_for_cli_session widened to 10x250ms, and the session-never-came-up
branch now runs kill_cli_tmux_session so a session that squeaks in after the
window can't strand the parked prompt behind the session-exists gate.
- Delivery confirmation moved to a background task so the terminal starts
streaming immediately instead of blanking behind the polls.
- Hygiene: prompt file removed when create_session fails after staging;
cli-prompts/ dir created 0700; per-send sequence-suffixed tmux buffers plus
delete-buffer when a paste fails; the Flag spec value is shell-quoted like
every other emitted word.
…ery, simplify pass Codex xhigh review fixes: - clear_pending_cli_prompt is now a compare-and-swap on the exact delivered value: a newer prompt parked mid-confirmation (loop wake-up re-parked while an initial prompt was being confirmed) can no longer be destroyed by the older delivery's clear. Superseded clears log and leave it parked. - A parked prompt behind a LIVE tmux session or a resume launch is no longer stranded: attaches now deliver it by paste into the running agent (readiness gate + claim + confirmed clear), so an unconfirmed earlier delivery or a re-parked wake-up retries on every attach instead of waiting for the session to die — and resume precedence no longer permanently blocks re-parked wake-up prompts (they arrive as follow-ups after the resume launch). - The baked-prompt ack now requires the staged file to be gone AND a non-shell process owning the pane, closing the consumed-but-never-executed window (agent exec fails after command -v passes) that file-gone alone would have mistaken for delivery. Simplify pass (behavior-preserving): - PtyCommand::TmuxCli carries workspace_id instead of a session name that was reverse-parsed back into one (impossible-error branch deleted). - PromptDelivery bundles workspace/clear-session/peeked/deferred/claim so a claim without a prompt is unrepresentable; too_many_arguments allow dropped. - PtyError::PromptStageFailed is a unit variant whose display IS the shared recovery notice; active_resume_id() is the single owner of the resume-wins predicate for both the staging gate and the bootstrap; the read-and-delete bootstrap stage is emitted once for Positional/Flag; route_initial_prompt always carries trimmed text; loop_supervisor re-park has one owner (repark_prompt, now logging its failures); cli_tmux_session_exists reuses tmux_ok; the pane probe runs pgrep before ps and is_shell_command owns comm normalization; internal helpers dropped from the crate's public surface.
…e, guarded re-park, workspace peek
Recall-mode review (5 correctness finders + conventions) on the delivery
state machine:
- Deferred paste gates on THE agent we launched (tmux #{pane_pid} tree comm
== spec.program, 15-byte comm truncation handled) instead of any non-shell
process — a user running vim/npm inside the missing-binary fallback shell
can no longer receive (and effectively destroy) a prompt meant for the
agent. Re-verified right before the paste (the agent can die inside the
grace window and drop the pane to a shell that would EXECUTE the prompt)
and after it (an agent that exits immediately discarded the paste with its
tty; delivery is not confirmed).
- Fresh deferred launches start a bare TUI instead of --continue||fresh: the
doomed --continue first leg of a brand-new workspace lived just long enough
to pass the readiness gate, swallow the paste, exit, and let the CAS clear
destroy the prompt.
- Multi-line texts always ride the bracketed-paste buffer transport;
send-keys -l types newlines as Enter, submitting at the first line while
reporting success. Enter is retried once and a text-delivered/Enter-failed
send now counts as delivered (re-pasting would double the text).
- Loop wake-up re-parks park only into an EMPTY slot (guarded UPDATE):
continuation boilerplate can never overwrite a parked-but-undelivered user
prompt. Wake-up sends take the CliPromptDelivery claim so they can't
interleave paste+Enter pairs with an in-flight delivery; a busy claim
defers the wake-up to the next tick.
- The parked-prompt peek is workspace-scoped (creation parks on the CLI-first
session, wake-ups on the latest, the attach may resolve a third) so a
prompt parked on a sibling session row is never stranded.
- kill_cli_tmux_session kills before removing the prompt file (a first rm
could yank the file mid-bootstrap and let file-gone+agent-up confirm a
delivery that never happened); the session-never-came-up branch no longer
kills a late-appearing session (it may be running real work; a parked
prompt behind a live session is deliverable now); blank parked prompts are
CAS-cleared instead of re-probed forever; the baked confirm window widened
to 30x250ms and drops the never-consumed file on timeout; the claim
registry recovers from mutex poisoning instead of silently disabling all
deliveries; the embedded tmux conf pins default-shell /bin/sh so the POSIX
bootstrap survives fish/csh login shells (applied to running servers too).
Tests: in-memory SQLite coverage for the CAS clear (match/supersede/no-op),
if-empty park, and workspace-scoped peek; unit tests for the fresh-deferred
bootstrap, follow-up routing, paste-transport predicate, and expected-agent
comm matching.
…subtree walk The delivery gate cli_pane_agent_running only inspected the pane root and its direct children, and its doc claimed a shebang exec sets comm to the script basename. Both are wrong for node-wrapped agents: codex ships as a '#!/usr/bin/env node' launcher whose comm is 'node' and which spawns the native codex as a GRANDCHILD (sh -> node -> codex, verified on a scratch socket). The gate therefore never confirmed codex delivery, so every baked codex prompt was delivered via argv but left parked -> replayed on the next fresh launch, and >100KB codex prompts (deferred paste) never delivered at all. Walk the whole pane process subtree from a single 'ps' snapshot (pure, unit- tested helper) and match the exact program name at any depth; still refuse the intermediate 'node' and any fallback-shell program, so a user's vim/npm can't satisfy the gate. Also force 0600 on the prompt file after open (a pre-existing looser-perm file would otherwise keep its mode; the 0700 dir is the real control, this is defense in depth) and drop the now-unused process_comm helper.
… session creation Two racing first-attaches to a brand-new workspace both run 'tmux new-session -A'. The prompt-carrying attach (claim winner) can compute fresh_launch=true and bake the prompt into its bootstrap, yet the claim LOSER (promptless) can reach new-session first and create the session with a fresh/continue bootstrap; the winner's '-A' attach then just joins that session and its baked bootstrap is ignored. Confirmation then timed out, dropped the file, and left the prompt parked until a later attach -- the live agent started with no initial prompt. confirm_or_recover_baked_prompt now, on confirmation timeout, pastes the prompt into whatever agent owns the pane IF the staged file is still on disk. For argv agents (Positional/Flag) the bootstrap rm's the file before exec, so a surviving file proves our bootstrap never ran and the prompt was never argv-delivered -- pasting can't double-deliver. StdinPipe/Unsupported carry no fallback (a lingering file there can mean cat is mid-stream). File cleanup moves to the recovery function so the confirmed path never yanks a file out from under a consumer.
…nding on transient DB error Two gaps in the wake-up re-park recovery: 1. repark_prompt only checked the LATEST session row's empty slot (set_pending_cli_prompt_if_empty), but attach delivery peeks the workspace's newest pending prompt across ALL session rows. With an older session holding an undelivered user prompt and a newer empty session, a wake-up parked continuation boilerplate on the newer row and it delivered BEFORE the user prompt. Guard on a workspace-scoped peek so a prompt parked anywhere blocks re-park (the user prompt delivers first; the loop re-detects its banner). 2. Both failure branches called the best-effort repark_prompt (returning ()) and then marked the wake-up fired, so a transient DB error during re-park dropped the wake-up entirely -- neither delivered nor parked nor pending. repark_prompt now returns a ReparkOutcome; only Failed (a transient DB error) leaves the wake-up pending for the next tick, mirroring the existing capture-failure branch. Handled/NoSession stay final.
…r-lose The codex backstop found the paste-recovery added a prompt-LOSS window: when a losing racing first-attach won 'new-session -A' with a promptless bootstrap, the recovery pasted the prompt into whatever agent owned the pane — but the loser's '--continue || fresh' leg and the eventual fresh agent share a process name, so the paste could land on the dying '--continue' leg yet be 'confirmed' by the fresh one, CAS-clearing a prompt no agent ever received. Revert to leaving the prompt parked when baked confirmation fails: in that rare double-first-attach race the next attach delivers it as a follow-up paste into the live agent — slower, but never lost, which is the invariant that matters. confirm_baked_prompt_consumed documents the deliberate choice.
…e walk pane_subtree_has_program split each ps line on whitespace and took the third token as comm, truncating a comm that contains spaces. macOS 'ps -o comm=' reports a full executable path, which can contain spaces (e.g. '/Applications/My App/codex'), so an agent there would never match and its delivery would never confirm. Split off only pid and ppid and keep the whole remainder as comm (normalize_comm still reduces it to the basename).
…lready parked The workspace-scoped re-park guard skipped parking whenever any prompt was already parked and the callers then marked the wake-up fired. For a limit-kind continuation that self-heals (the loop re-detects its banner), but a MANUAL wake-up is the user's explicit one-time prompt and was silently dropped, with no second slot to hold it. repark_prompt now returns AlreadyParked distinctly and wakeup_stays_pending keeps manual wake-ups pending (retry when the slot frees) while still letting limit continuations fire.
… backstop - fresh_launch now uses the bootstrap's exact resume predicate (Uuid::parse_str == is_uuid). A non-UUID resume id is not resumed by the launch, so treating it as 'resuming' (the old resume_session_id.is_none() check) routed the prompt as a follow-up paste into continue_launch's doomed '--continue' leg, risking loss. - The loop supervisor's live-send path now peeks the workspace's parked prompt before send_cli_keys and defers the wake-up if one is queued, so a wake-up can never reach the agent ahead of an older undelivered prompt (the same 'parked prompt delivers first' invariant the re-park path enforces).
# Conflicts: # crates/local-deployment/src/pty.rs # crates/server/src/routes/terminal.rs
…g-dash text Two more delivery-loss paths found by the final backstop sweep: - send_cli_keys' small-text path ran 'tmux send-keys -l <text>' with no '--', so a prompt starting with '-' (e.g. '-rf ...') was parsed as an unknown send-keys flag and delivery silently failed (verified: 'unknown flag -f'). Add '--' before the text so it is always treated as a literal key. - deferred_prompt_pending was gated on fresh_launch (session absent at check time). A live session can exit between that check and 'new-session -A'; the freshly created pane would then run continue_launch's doomed '--continue' leg and the deferred paste could land on it and be lost. Request the bare-TUI bootstrap for any non-resume deferred prompt (ignored by '-A' when the session survives, so it's harmless in the common case).
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
1 participant
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.
Problem
Creating a CLI-mode workspace with a large initial prompt (e.g. tens of KB of pasted Chrome history) silently dropped the prompt and opened an empty TUI.
Root cause (two independent bugs):
cli_bootstrap()single-quoted the whole prompt into the tmuxnew-sessioncommand. tmux 3.4 rejects any client command past ~16KB ("command too long", exit 1) — verified empirically at ~16,324 bytes. EveryCliPromptArgshape (Positional/Flag/StdinPipe) was affected; quote-heavy prompts hit it sooner.create_sessionreturnsOkwhen the tmux client spawns; the client's exit-1 milliseconds later was never checked, and the terminal route clearedpending_cli_promptimmediately after thatOk— destroying the only copy. The WS then dropped, the frontend reconnected, and the bootstrap's--continuefallback launched a fresh empty TUI.send_cli_keyshit the same ~16KBsend-keys -lceiling and merely logged the failure while still marking the wake-up fired.What changed
Transport A (initial prompt) — temp file + pane-shell expansion.
cli_bootstrap()now takesprompt_file: Option<&Path>.create_sessionwrites the peeked prompt to a private 0600 file (asset_dir()/cli-prompts/<workspace>.txt) and the bootstrap reads it back:vk_p="$(cat 'file')"; rm -f -- 'file'; {base} "$vk_p"{base} {flag} "$vk_p"cat 'file' | {base}; rm -f -- 'file'The tmux command is now O(1) in prompt size. The prompt never re-enters shell quoting (path single-quoted; content only expands inside double quotes). Per-arm small-prompt semantics are byte-identical (leading-dash guard and StdinPipe trailing newline moved into the file contents). The file self-deletes once consumed and is also removed on
kill_cli_tmux_session/reaper and on spawn-failure recovery.Transport B (send path).
send_cli_keysnow stages texts ≥4KB through a namespaced tmux buffer —load-buffer -b vk_prompt_<wsid> -(text on stdin, no argv limit) +paste-buffer -d -p(bracketed paste, so embedded newlines don't submit early) + the existing separateEnter. Texts <4KB keep the live-verifiedsend-keys -lpath.Recovery — never destroy the prompt.
cli_tmux_session_exists(~5×200ms) before clearing the parked prompt. If the session never appears: leave it parked, delete the temp file, and send a redTerminalMessage::Error("your prompt is saved and will be delivered on the next attach").MAX_ARG_STRLEN, so they are delivered post-launch by paste (viasend_cli_keys) once the session is confirmed up, and cleared only on successful delivery.set_pending_cli_promptwhen a wake-up delivery fails (mirrors the session-gone branch).terminal.rsandsession.rs.How verified
cargo test -p local-deployment— 60 passed. New/updated unit tests cover: bootstrap length O(1) vs prompt size; path single-quoted / expansion double-quoted; per-arm file forms (Positional/Flag/StdinPipe);cli_prompt_file_contentquoting semantics (verbatim content, leading-dash guard, StdinPipe newline, blank→None);cli_prompt_fits_inlinecap; resume precedence ignores the prompt file.cargo check -p local-deployment -p serverandcargo clippy -p local-deployment -p server— clean (server required building the local-web dist for rust-embed).-L exec_scratch,test_*sessions): a 40KB Positional prompt through the generated file-transport bootstrap → the pane received the full 41,536 bytes byte-exact. The send path (load-buffer -+paste-buffer -d -pof 40KB into astty raw; catpane) delivered all 40,000 bytes; the only transformation is tmux's standard LF→CR paste translation (833 LF → 833 CR), which bracketed-paste TUIs handle.Risks
$(cat file)bootstrap form is re-verified for the claude/codex spec shapes in unit tests, but live TUI bracketed-paste-on-Entersemantics for the >100KB paste path could not be verified end-to-end in this environment (no authenticated agents). The path never loses the prompt: it stays parked until delivery is confirmed.paste-buffer -pbehavior depends on the TUI enabling bracketed paste; only ≥4KB loop-continuation and >100KB initial prompts take this path./optneeds a rebuild+restart to pick this up.ship-it battery
Council round 2 (
code-audit, engine mixed: Codex ×4 seats + Claude cross-check) returned BLOCK from all four Codex seats; extracted and resolved below. A Codex xhigh backstop then swept the diff to convergence (CodeRabbit CLI can't auth non-interactively → Codex is the substitute). Shell-injection was cleared by every seat.Council R2 BLOCK findings
cli_pane_agent_runningonly checked the pane root + direct children; codex runs as#!/usr/bin/env nodethat spawns the nativecodexas a grandchild (sh → node → codex, verified), so codex delivery never confirmed → baked prompts replayed, >100KB prompts never delivered. Now walks the whole pane process subtree from onepssnapshot; exact program-name match preserved (fallback shells / vim/npm still can't satisfy the gate).repark_promptnow guards on a workspace-scoped peek. The live-send path also peeks before sending so a wake-up can't reach the agent ahead of a parked prompt.repark_promptreturnsReparkOutcome; transient failures leave the wake-up pending (retry), and MANUAL wake-ups stay pending when a prompt is already parked instead of being silently dropped.--continueleg and false-confirm via the shared process name, CAS-clearing an undelivered prompt — a loss window that violates the paramount never-lose invariant. The prompt is instead left parked (CAS-guarded) and delivered on the next attach: slower in that rare double-first-attach race, but never lost.deferred_prompt_pendingandfresh_launchwere also made robust so a session dying beforenew-session -Acan't route a deferred paste into a--continueleg either.ampagent isStdinPipe; it requires a multi-MB prompt forcatto still be streaming at the ~7.5s confirm timeout, and the CAS clear bounds the consequence to a single re-delivery. Live agents (claude/codex) arePositional. Adding StdinPipe-specific confirmation is scope expansion beyond the locked decisions.Codex xhigh backstop (3 sweeps → NO ACTIONABLE FINDINGS)
ps commparsed as the full remainder after pid/ppid (macOS full-path comms with spaces).send-keys -l -- <text>guards prompts starting with-(verified:-lwithout--fails "unknown flag").fresh_launchuses the bootstrap's exactis_uuidresume predicate (Uuid::parse_str).deferred_prompt_pending = !resume_will_apply(robust to a live session exiting before attach).claudeis a native ELF here (found directly); a hypothetical pure-JS install has no process named after the agent anywhere (comm=node,argv=cli.js), so no process-inspection gate can confirm it — a pre-existing design limitation, not reliably fixable, not exercised by the verified native install.Merge with main
origin/main(#28 terminal-resize-newline, squasha22aa055) merged in; conflicts inpty.rs/terminal.rsreconciled keeping BOTH intents — #28'sAttachInputTripwire+hex_dump+ redaction +cols×rowslogging, and #30's temp-file transport + deferred-clear + re-park.tripwire_sessionnow derives fromworkspace_idviacli_tmux_session_name.Verification
cargo test -p local-deployment -p server -p db(db 8 / local-deployment 71 / server 43, all pass),cargo clippy,cargo fmt— clean. CI check-runs green by SHA. Final head:7a8033d3(mergebcdd32c5; R2 fix commits9197cd4d,572085f6,050ed84f,c10841ed,f04c888a,e6040b25,7a8033d3).