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.