From 420431d5e28eb2f6193f33fa4961708dec24a4b3 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:00:14 +0800 Subject: [PATCH] fix(cli): close the gaps review found in wait, --key and pane close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five things the first pass got wrong, in the order they bite. `--until free --changed` waited on a command it had already missed: the "something ran" edge is only set by a poll that catches the pane busy, and a command that starts and finishes inside one 500ms interval never is. That is indistinguishable from a command that never ran, so the timeout now names both doors instead of letting a finished build read as a hang. `free` also outranked the agent ladder, which is backwards. A pane whose depth-0 process *is* the agent — the tree cannot tell that apart from a shell at its prompt — reads free for its whole turn, so a `waiting` the caller explicitly asked for could be overwritten by a process-tree fact and then withheld by the `--changed` rule that comes with it. `free` is now consulted only when none of the requested agent states answered, which is both cheaper and what the docs already claimed. An empty process tree is "we could not see in" rather than "free" for the same reason `no-agent` exists. `--key M-X` sent `ESC x`: the whole spelling was folded to lowercase, which is free for Ctrl (the C0 rule clears the case anyway) and wrong for Alt, where the character rides through as itself. `send --help` listed the key vocabulary by hand next to the table it is a list of; it had already drifted by one alias. It is generated now. And a `pane close` batch that could not close everything raised an error, which left `--json` holding prose exactly when a cleanup script needs to know which panes are still its problem. It exits 1 with `{"closed":[…],"failed":[…]}`, with the complaint still on stderr so `-q` reports it. --- CHANGELOG.md | 8 +- crates/tty7-cli/src/cli.rs | 12 +-- crates/tty7-cli/src/commands.rs | 164 +++++++++++++++++++++++++++-- crates/tty7-cli/src/keys.rs | 75 ++++++++++++- docs/agents/orchestration.mdx | 18 +++- docs/cli/reference.mdx | 10 +- skills/tty7/SKILL.md | 9 ++ skills/tty7/references/commands.md | 20 ++-- 8 files changed, 283 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7eec9bbe..16d196ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,7 +68,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 pane is back to its bare shell, which is what a `cargo test` running in a pane has instead of an agent status. With `--changed` it means "something ran and then finished", the shape you want on the line after a `send`. It costs a - second request per poll and so is only checked when you name it. + second request per poll and so is only checked when you name it — and only + when none of the agent states you named answered first, so `waiting,done,free` + on a pane of unknown kind cannot lose you a `waiting`. - **`tty7 send --key` presses keys instead of typing characters** — `C-c` to stop a runaway build, `escape` to close a TUI, `up`/`down`/`enter` to answer @@ -92,7 +94,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - **`tty7 pane close --json` now reports `{"closed": [ids]}`** rather than a - single `{"closed": id}`, because the verb takes more than one pane. + single `{"closed": id}`, because the verb takes more than one pane. A batch + that could not close everything exits 1 with `{"closed": […], "failed": […]}` + and the complaint on stderr, so a retry knows what is left. ### Fixed diff --git a/crates/tty7-cli/src/cli.rs b/crates/tty7-cli/src/cli.rs index d17bdf82..ce246cbf 100644 --- a/crates/tty7-cli/src/cli.rs +++ b/crates/tty7-cli/src/cli.rs @@ -68,16 +68,12 @@ pub enum Command { #[command(about = "Split a pane (= tty7 pane split)")] Split(SplitArgs), + // The key list is built from the table it is a list *of*, rather than + // written out here: a hand-copied vocabulary drifts the first time a key + // is added, and this is the text a caller reaches for to learn the names. #[command( about = "Type text into a pane, or send it keystrokes with --key", - long_about = "Types TEXT into the pane exactly as a keyboard would.\n\n\ - --key sends a keystroke rather than characters, which is what a pane \ - wants once something is already running in it: answering a prompt that \ - only takes arrow keys, closing a TUI with escape, stopping a build with \ - C-c. Repeat it for a sequence.\n\n\ - Keys: enter, escape, tab, backtab, space, backspace, delete, up, down, \ - right, left, home, end, pageup, pagedown, C- (Ctrl), M- \ - (Alt). Aliases: return, cr, esc, del, bs, shift-tab, pgup, pgdn." + long_about = crate::keys::send_long_help() )] Send(SendArgs), diff --git a/crates/tty7-cli/src/commands.rs b/crates/tty7-cli/src/commands.rs index e4f07836..1ea301a0 100644 --- a/crates/tty7-cli/src/commands.rs +++ b/crates/tty7-cli/src/commands.rs @@ -826,12 +826,24 @@ fn pane_close( } if !failures.is_empty() { - bail!( - "closed {} pane(s); {} could not be closed — {}", + // Structured even here, for the reason `wait` is: the caller was + // cleaning up, and what they need next is which panes are still theirs + // to deal with — an anyhow error would leave `--json` holding prose. + // The complaint goes to stderr all the same, so `-q` still reports it + // and the exit code is not the only thing that says so. + eprintln!( + "tty7: closed {} pane(s); {} could not be closed — {}", closed.len(), failures.len(), failures.join("; ") ); + return Ok(Outcome::Exit( + 1, + Report { + human: String::new(), + json: json!({ "closed": closed, "failed": failures }), + }, + )); } let human = match closed.as_slice() { // The single-pane case is the overwhelming one and has always been @@ -973,10 +985,12 @@ fn wait(args: WaitArgs, ctx: &Context, backend: &mut dyn Backend) -> Result Result Result Result { - Ok(backend.procs(pane)?.procs.iter().all(|p| p.depth == 0)) + let procs = backend.procs(pane)?.procs; + Ok(!procs.is_empty() && procs.iter().all(|p| p.depth == 0)) } /// Whether the daemon still has a live pane behind this id. Absent from the @@ -2020,6 +2055,10 @@ mod tests { /// A batch keeps going after a failure. Stopping at the first one would /// leave the rest of the leak exactly where it was — while still reporting /// the failure, because a half-done cleanup that claims success is worse. + /// + /// Reported as an exit code carrying a report, not as an error: the caller + /// was cleaning up, and the useful answer is which panes are still theirs + /// to deal with. An anyhow error would leave `--json` with prose. #[test] fn pane_close_reports_failures_without_abandoning_the_batch() { let mut backend = mock(); @@ -2030,13 +2069,30 @@ mod tests { ]; backend.kill_failures = vec![78]; - let err = execute( + let out = execute( cli(&["tty7", "pane", "close", "--orphans"]), &Context::default(), &mut backend, ) - .expect_err("a pane that could not be closed has to be reported"); - assert!(err.to_string().contains("%78"), "{err}"); + .expect("a partial cleanup is an exit code, not an error"); + let Outcome::Exit(1, r) = out else { + panic!("a pane that could not be closed has to be reported"); + }; + assert_eq!( + r.json["closed"], + serde_json::json!([77, 79]), + "the survivors of the batch are what a retry needs: {}", + r.json + ); + assert!( + r.json["failed"] + .as_array() + .expect("the failures are a list") + .iter() + .any(|f| f.as_str().is_some_and(|f| f.contains("%78"))), + "{}", + r.json + ); assert_eq!( backend.killed, vec![77, 78, 79], @@ -2866,6 +2922,94 @@ mod tests { assert_eq!(json["stale"], false); } + /// A command that starts and finishes between two polls is never *seen* + /// busy, which is indistinguishable from one that never ran — so the + /// timeout has to name both doors instead of letting a finished command + /// read as "still going". + #[test] + fn wait_changed_free_says_why_it_saw_nothing_run() { + let mut backend = mock(); + for _ in 0..2 { + backend.replies.push_back(ReplyOk::AgentStates(Vec::new())); + } + backend.procs_reply = idle_procs(); + + let out = execute( + cli(&[ + "tty7", + "wait", + "%3", + "--until", + "free", + "--changed", + "--timeout", + "0", + ]), + &Context::default(), + &mut backend, + ) + .expect("a timeout is an exit code, not an error"); + let Outcome::Exit(124, r) = out else { + panic!("a pane that was free all along has not run anything"); + }; + assert!( + r.human.contains("--interval") && r.human.contains("--changed"), + "the timeout should name the two ways out: {}", + r.human + ); + } + + /// `free` answers for a pane the agent ladder cannot, so it must not answer + /// *over* it. A pane whose depth-0 process is the agent itself reads free + /// for its whole turn; letting that outrank a `waiting` the caller asked + /// for would strand exactly the delegation loop the verb exists for. + #[test] + fn wait_free_does_not_overrule_a_state_the_caller_asked_for() { + use tty7_core::core::cli_agent::AgentStatus; + let mut backend = mock(); + backend + .replies + .push_back(ReplyOk::AgentStates(vec![agent_state( + 3, + AgentStatus::Waiting, + )])); + // The agent is the pane's only process, so the tree reads "free". + backend.procs_reply = idle_procs(); + + let json = json_of(run_cli( + &["tty7", "wait", "%3", "--until", "waiting,free"], + &Context::default(), + &mut backend, + )); + assert_eq!(json["status"], "waiting", "the ladder answered first"); + assert_eq!(json["matched"], true); + assert!( + backend.procs_calls.is_empty(), + "and the process tree was never asked" + ); + } + + /// An unreadable process tree is not an idle one. Answering `free` on an + /// empty reply would be the same false success `no-agent` was added to + /// remove, one layer down. + #[test] + fn wait_free_does_not_read_an_empty_process_tree_as_finished() { + let mut backend = mock(); + backend.replies.push_back(ReplyOk::AgentStates(Vec::new())); + backend.procs_reply = tty7_core::daemon::protocol::PaneProcs::default(); + + let out = execute( + cli(&["tty7", "wait", "%3", "--until", "free", "--timeout", "0"]), + &Context::default(), + &mut backend, + ) + .expect("a timeout is an exit code, not an error"); + assert!( + matches!(out, Outcome::Exit(124, _)), + "nothing was seen, so nothing can be claimed" + ); + } + /// Watching `free` must not cost anything for callers who did not ask: /// the process tree is a second round trip per poll on top of the agent /// snapshot, and the default wait is for agents. diff --git a/crates/tty7-cli/src/keys.rs b/crates/tty7-cli/src/keys.rs index 4eb0da7e..055af2b7 100644 --- a/crates/tty7-cli/src/keys.rs +++ b/crates/tty7-cli/src/keys.rs @@ -68,17 +68,36 @@ const ALIASES: &[(&str, &str)] = &[ ("pgdown", "pagedown"), ]; -/// What `--help` prints, and what an unknown name is answered with. +/// Every spelling `--key` accepts, in one line: what an unknown name is +/// answered with, and half of what `tty7 send --help` prints. pub fn vocabulary() -> String { let named: Vec<&str> = NAMED.iter().map(|(name, _)| *name).collect(); format!("{}, C- (Ctrl), M- (Alt)", named.join(", ")) } +/// `tty7 send --help`. Assembled from the tables above so that adding a key or +/// an alias cannot leave the help text describing the vocabulary of an older +/// build — the drift nobody notices until a caller is told a key exists and it +/// does not, or the reverse. +pub fn send_long_help() -> String { + let aliases: Vec<&str> = ALIASES.iter().map(|(from, _)| *from).collect(); + format!( + "Types TEXT into the pane exactly as a keyboard would.\n\n\ + --key sends a keystroke rather than characters, which is what a pane wants once \ + something is already running in it: answering a prompt that only takes arrow keys, \ + closing a TUI with escape, stopping a build with C-c. Repeat it for a sequence.\n\n\ + Keys: {}. Aliases: {}.", + vocabulary(), + aliases.join(", ") + ) +} + /// clap's `value_parser` for `--key`, so an unknown name is a usage error /// caught before a single byte reaches the pane — sending half a key sequence /// and then failing would leave the pane in a state nobody asked for. pub fn parse(spelling: &str) -> Result { - let folded = spelling.trim().to_ascii_lowercase(); + let trimmed = spelling.trim(); + let folded = trimmed.to_ascii_lowercase(); let canonical = ALIASES .iter() .find_map(|(from, to)| (*from == folded).then_some(*to)) @@ -90,6 +109,8 @@ pub fn parse(spelling: &str) -> Result { bytes: bytes.to_vec(), }); } + // Ctrl reads the folded spelling: the C0 rule clears the top three bits, so + // C-c and C-C are the same byte and always were. if let Some(rest) = strip_modifier(canonical, &["c-", "ctrl-", "control-"]) { return control(rest).map(|byte| Key { name: format!("c-{rest}"), @@ -98,7 +119,14 @@ pub fn parse(spelling: &str) -> Result { } // Alt is a prefixed ESC — the encoding every Unix terminal has used for it // since long before there was a modifier-reporting protocol to do better. - if let Some(rest) = strip_modifier(canonical, &["m-", "alt-", "meta-"]) { + // Which means the character rides through as itself, so unlike Ctrl this + // one has to read the spelling as written: M-X is not M-x. + // Stripped from `folded` rather than `canonical`: only that one is the + // caller's own spelling with the case knocked out of it, which is what + // makes the tail recoverable from `trimmed` by length. + if let Some(rest) = + strip_modifier(&folded, &["m-", "alt-", "meta-"]).map(|rest| as_written(trimmed, rest)) + { let mut chars = rest.chars(); return match (chars.next(), chars.next()) { (Some(ch), None) => { @@ -128,6 +156,15 @@ fn strip_modifier<'a>(name: &'a str, prefixes: &[&str]) -> Option<&'a str> { .filter(|rest| !rest.is_empty()) } +/// The same tail of the spelling the caller wrote, before it was folded. +/// +/// Safe to index by length: `to_ascii_lowercase` is byte-for-byte, and every +/// modifier prefix is ASCII, so a suffix of the folded form is a suffix of the +/// original at the same offset and on the same char boundary. +fn as_written<'a>(original: &'a str, folded_rest: &str) -> &'a str { + &original[original.len() - folded_rest.len()..] +} + /// The C0 control byte a Ctrl-chord produces. This is the ASCII table's own /// rule — clear the top three bits — which is why the range runs past the /// letters and into `[ \ ] ^ _`, and why Ctrl-? is the odd one out at 0x7f. @@ -205,6 +242,38 @@ mod tests { assert_eq!(bytes("alt-b"), vec![0x1b, b'b']); } + /// Ctrl can be folded and Alt cannot: the ASCII rule throws the case away + /// either way for a control byte, while Alt carries the character through + /// as itself, so `M-X` and `M-x` are two different keys and must stay so. + #[test] + fn alt_keeps_the_case_the_caller_wrote() { + assert_eq!(bytes("M-X"), vec![0x1b, b'X']); + assert_eq!(bytes("Meta-X"), vec![0x1b, b'X']); + assert_eq!(bytes("M-x"), vec![0x1b, b'x']); + assert_eq!(parse("M-X").unwrap().name, "m-X"); + // Non-ASCII rides through as its own UTF-8, and the prefix arithmetic + // must not land mid-character doing it. + assert_eq!(bytes("M-ä"), vec![0x1b, 0xc3, 0xa4]); + } + + /// The help text is generated from the tables, so it cannot describe a + /// vocabulary the parser does not have. This is the assertion that the + /// generating is real rather than a second copy that happens to agree. + #[test] + fn the_help_text_lists_every_key_the_parser_takes() { + let help = send_long_help(); + for (name, _) in NAMED { + assert!(help.contains(name), "`{name}` is missing from --help"); + } + for (alias, _) in ALIASES { + assert!(help.contains(alias), "`{alias}` is missing from --help"); + } + assert!( + help.contains("C-") && help.contains("M-"), + "{help}" + ); + } + /// An unknown name has to fail before anything is written: a key sequence /// half-delivered into a live pane is worse than one not delivered at all. /// So the error names the vocabulary rather than just refusing. diff --git a/docs/agents/orchestration.mdx b/docs/agents/orchestration.mdx index c2ccd185..7c830378 100644 --- a/docs/agents/orchestration.mdx +++ b/docs/agents/orchestration.mdx @@ -78,7 +78,17 @@ tty7 wait "$PANE" --until free --changed --timeout 900 cat /tmp/t.rc /tmp/t.log ``` -`free` costs one extra request per poll, so it is only checked when you name it. +`free` costs one extra request per poll, so it is only checked when you name it — +and only once the agent ladder has not already answered, so pairing it with +`waiting,done` never costs you a state you asked for. + + + `free` is read off the process tree, which has two blind spots. A pane whose + own root process *is* the command — what `tty7 run` spawns — looks free the + whole time it runs; wait on `tty7 run` itself instead, it already blocks. And + a backgrounded job (`… &`) keeps the pane busy after the foreground command + has finished. + A pane with no agent reports `no-agent`, **not** `idle`. That distinction is @@ -101,6 +111,12 @@ free ends up where it started, so there is no new state to compare against. There `--changed` means "something ran while I was watching", which is exactly what you want in the line after a `send`. +That does mean a command which starts *and* finishes between two polls is never +seen running, and the wait sits there until it times out. If the thing you are +waiting on can be that quick, poll faster (`--interval 100`) or drop `--changed` +and let a sentinel file carry the answer. The timeout says as much when it +happens. + ## Answering a prompt A worker that stops at `waiting` is usually showing something that keystrokes, diff --git a/docs/cli/reference.mdx b/docs/cli/reference.mdx index 18082645..0c0d87fd 100644 --- a/docs/cli/reference.mdx +++ b/docs/cli/reference.mdx @@ -161,8 +161,10 @@ by its [hooks](/agents/status); the last three are facts about the pane: `free` is how you wait for a **command** rather than an agent, and it is the one state that costs a second request per poll — so it is only checked when you -name it. With `--changed` it means "something ran and then finished", which is -what you want directly after a `send`. +name it, and only when none of the agent states you asked for already matched. +With `--changed` it means "something ran and then finished", which is what you +want directly after a `send`; a command quick enough to finish inside one +`--interval` is never seen running, and the timeout says so. The reply carries the agent's message and native session id. The JSON's `stale` flag says whether the answer might belong to the previous turn. @@ -268,7 +270,9 @@ holds it. An interrupted `run` and a removed workspace both leave orphans here. lists as orphaned and nothing else — panes a workspace holds are untouched — and reports an empty list rather than an error when there is nothing to clean up, so a script does not have to guard it. A pane that cannot be closed does -not abandon the rest of the batch; the failure is reported at the end. +not abandon the rest of the batch: the rest are still attempted, the complaint +goes to stderr, and the verb exits 1 with `{"closed":[…],"failed":[…]}` — the +list a retry needs. `--orphans` closes every orphan on the machine, and an orphan can still be diff --git a/skills/tty7/SKILL.md b/skills/tty7/SKILL.md index e08dd321..1835fb33 100644 --- a/skills/tty7/SKILL.md +++ b/skills/tty7/SKILL.md @@ -186,6 +186,11 @@ Exit codes are built for this: `0` means a state you asked for was reached, `124` means the timeout ran out (the `timeout(1)` convention, so "not yet" is distinguishable from "broken"), `1` means the pane died first. +One trap in `--changed`: a command that finishes inside a single poll (500ms by +default) is never *seen* running, so the wait keeps going until it times out. +For something that quick, `--interval 100`, or drop `--changed` and read the +`.rc` file. The timeout message says so when it happens. + If you want the process tree itself — "what is running in there", "which port is this pane serving" — that is `tty7 procs %83`: indented by depth, `*` on the foreground process, then the ports those processes are listening on. @@ -226,6 +231,10 @@ understanding. a pane running a build is `no-agent`, never `idle`, so `--until idle` is never the way to ask "is the command finished". That is `free`. +Mixing the two is safe: `--until waiting,done,free` covers a pane whose kind you +don't know, because `free` is only consulted when none of the agent states you +named matched first. + ### `--changed` is not optional in a loop The status is a **level, not an event**: `done` stands until the next turn diff --git a/skills/tty7/references/commands.md b/skills/tty7/references/commands.md index b7ef61b1..4f3c3a55 100644 --- a/skills/tty7/references/commands.md +++ b/skills/tty7/references/commands.md @@ -202,11 +202,18 @@ Notes that decide whether a loop works: - **`idle` is not "the command finished".** It is something an *agent* says about itself. A pane running a build has no agent and reports `no-agent`. Use `free` for commands. -- **`free` costs a second request per poll**, so it is only checked when named. +- **`free` costs a second request per poll**, so it is only checked when named, + and only if none of the agent states you asked for matched first — pairing + `waiting,done,free` never loses you a `waiting`. - **`--changed` means something different for `free`**: a shell goes free → busy → free and ends where it started, so there is no new state to compare against. There it means "something ran while I watched" — exactly what you - want on the line after a `send`. + want on the line after a `send`. A command fast enough to finish inside one + `--interval` is never seen running, so it times out instead; use + `--interval 100` for those, or drop `--changed` and read a sentinel file. +- **`free` reads the process tree**, so a pane whose root process is the command + itself (a `tty7 run` pane) looks free while it runs, and a backgrounded job + keeps a pane busy after the foreground command is gone. ### `tty7 events` Streams server events until interrupted, one per line — pane exits, agent @@ -303,10 +310,11 @@ still tell a real name from a stand-in. `orphan: true` means no workspace holds it. An interrupted `tty7 run` is what leaves them. -`close` takes several ids at once and keeps going after a failure, reporting -what could not be closed at the end rather than abandoning the rest. `--orphans` -closes exactly what `pane ls --all` marks orphaned, and reports an empty list -instead of an error when there is nothing to do. +`close` takes several ids at once and keeps going after a failure: the rest are +still attempted, and it exits 1 with `{"closed":[...],"failed":[...]}` so you +know what is left. `--orphans` closes exactly what `pane ls --all` marks +orphaned, and reports an empty list instead of an error when there is nothing +to do. **`--orphans` is the user's broom, not yours.** It closes every abandoned pane on the machine, and an abandoned pane may still be running someone's command.