diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bc26df..4e349da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 and the reset hours went missing overnight. `u` now sweeps every badged session whose reset has passed, tells each to continue, and reports how many it nudged, how many are still waiting on their window, and any the agent - refused. For now it fires only when you press it — resuming capped sessions + refused. A capped session's supervisor is still holding it, so the sweep + releases each one before resuming it and abandons that nudge if the release + fails — nothing is sent that could only be refused. For now it fires only when + you press it — resuming capped sessions automatically once the window reopens is the intended next step, and this is the half that will sit underneath it. Sessions already back at work are untouched, and the badge drops as each nudge lands so a second press cannot @@ -32,8 +35,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 finalise a dead or stale session, and the agent is asked to retire its own entry for it. The conversation is kept: `claude stop` drops the entry from the default listing and `claude attach` still reopens it. Sessions Voro keeps open - on purpose (`needs-input`, `review`, `waiting`) are never stopped — you still - answer and reject into them. Agents declare the capability with a new optional + on purpose (`needs-input`, `review`, `waiting`) are retired at *handover* + rather than at close — once the agent's own listing agrees the turn is over, + not merely that the task is waiting on you — which is what leaves the session + free for a quick message to resume in place. The row stays open either way, so + you still answer and reject into it and `A` still opens it with its full + context. Agents declare the capability with a new optional `stop` verb (`{session}`), built in for `claude`; an agent without one, such as `codex`, behaves exactly as before, and a stop that fails leaves a line in `launches.log` rather than touching the transition. @@ -104,10 +111,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 untouched rather than recording feedback nobody received. The interactive jump-in moves from `a` to `A`. Agents declare the capability with a new optional `message` verb (`{session}` plus `{prompt_file}`, and the optional - `{new_session}` for an agent that can only be joined by forking — which is - how the built-in `claude` verb reaches a session its supervisor still holds), - built in for `claude`; an agent without one, such as `codex`, reports so on - the status line and keeps its jump-in. + `{new_session}` for an agent that can only be joined by forking), built in + for `claude`; an agent without one, such as `codex`, reports so on the status + line and keeps its jump-in. The message resumes the session in place, so the + whole life of a task is one session id, one `voro-` name and one + transcript — the handle you find it by in `claude agents` and the `/resume` + picker never moves out from under you. - `voro set --unlink :` drops a single dependency edge — `related:7`, `discovered-from:4`, `blocks:9` — named as `voro show` lists it. A pair of tasks carrying two edges keeps the one not named, so an edge diff --git a/crates/voro-core/src/agent.rs b/crates/voro-core/src/agent.rs index 01ff918..5e844ff 100644 --- a/crates/voro-core/src/agent.rs +++ b/crates/voro-core/src/agent.rs @@ -105,10 +105,14 @@ pub const VIEWER_BASE_PLACEHOLDER: &str = "{base}"; /// deliberate: a verb is an opaque per-agent contract, which is exactly what /// lets an agent define a subset of them and degrade per-verb. `codex` defines /// no `message` and the TUI's quick-message key says so on the status line. -/// It forks rather than resumes in place ([`NEW_SESSION_PLACEHOLDER`]): a +/// It resumes the session in place rather than forking it +/// ([`NEW_SESSION_PLACEHOLDER`], which the verb no longer carries): a /// `claude --bg` session keeps its supervisor process after finishing its turn, -/// and that supervisor refuses a headless `--resume` for as long as it lives, -/// so the plain resume was a send that could never land (DESIGN.md §8). +/// and that supervisor refuses a headless `--resume` for as long as it lives — +/// so Voro releases it at rest instead, through `stop`, and the send then lands +/// on the session's own reference (DESIGN.md §8). A fork would land too, but it +/// moves the conversation out from under the name Voro composed for it, and that +/// name is how the operator addresses the session everywhere outside Voro. /// /// It carries `--permission-mode` for the same reason `dispatch` does: the mode /// belongs to a launch rather than to a verb (DESIGN.md §8). The flag is per @@ -138,7 +142,9 @@ pub const VIEWER_BASE_PLACEHOLDER: &str = "{base}"; /// no cap signature in it means "not capped", however it came about. /// /// The claude `stop` verb retires a session from the agent's own listing once -/// Voro closes its row (DESIGN.md §8). A `claude --bg` session outlives its work +/// Voro closes its row — and, at rest, once it hands back (DESIGN.md §8): the +/// release the supervisor holds is what a headless `message` resumes through. +/// A `claude --bg` session outlives its work /// twice over — the entry stays in `claude agents` and the supervisor holding it /// runs until the machine reboots — so an operator who dispatches all week reads /// their session list through a wall of finished ones. The conversation survives @@ -153,7 +159,7 @@ dispatch = \"claude --bg --name \\\"{session_name}\\\" --permission-mode auto sessions = \"claude agents --json\" attach = \"claude attach {session}\" resume = \"claude --resume {session}\" -message = \"claude -p --resume {session} --fork-session --session-id {new_session} --permission-mode auto \\\"$(cat {prompt_file})\\\"\" +message = \"claude -p --resume {session} --permission-mode auto \\\"$(cat {prompt_file})\\\"\" logs = \"claude logs \\\"$(printf %.8s {session})\\\" 2>/dev/null | tail -c 20000\" stop = \"claude stop \\\"$(printf %.8s {session})\\\"\" plan = \"claude --name \\\"{session_name}\\\" --permission-mode auto --model {model} \\\"$(cat {prompt_file})\\\"\" @@ -1544,6 +1550,18 @@ impl AgentSessionEntry { _ => SessionLiveness::Dead, } } + + /// Whether this entry says its session's turn has *ended* — the narrow + /// reading the rest-stop rule acts on (DESIGN.md §8), which is not the same + /// question as [`liveness`](Self::liveness). Only `done` answers yes. + /// `blocked` is the case that makes the distinction load-bearing: it reads + /// dead without a live pid, but it is also what a permission prompt and a + /// supervisor mid-turn look like, and stopping either would cut a turn off + /// mid-sentence. Every other state, and an entry with no state at all, is + /// likewise not a hand-back. + pub fn at_rest(&self) -> bool { + self.state.as_deref() == Some("done") + } } /// Parse a `sessions` command's stdout. Entries without any id are skipped @@ -2082,29 +2100,24 @@ mod tests { #[test] fn verbs_lists_every_optional_verb_and_marks_a_forking_message() { let agents = builtin_agents(); + // The built-in claude resumes in place, so its message is named plainly. assert_eq!( agents["claude"].verbs(), vec![ - "sessions", - "attach", - "resume", - "message(fork)", - "logs", - "stop", - "plan" + "sessions", "attach", "resume", "message", "logs", "stop", "plan" ] ); assert_eq!(agents["codex"].verbs(), vec!["resume"]); - // A message that resumes in place keeps the reference it had, so it is - // named plainly; the roster still lists it. + // A message that forks names the session it forks into, and the roster + // says so — the marking outlives the built-in that used to carry it. let text = r#" [agents.a] dispatch = "run {prompt_file}" - message = "say --into {session} {prompt_file}" + message = "say --into {session} --as {new_session} {prompt_file}" "#; let config = AgentsConfig::parse(text, Path::new("/tmp/voro.toml")).unwrap(); - assert_eq!(config.agent("a").unwrap().verbs(), vec!["message"]); + assert_eq!(config.agent("a").unwrap().verbs(), vec!["message(fork)"]); } /// Every verb the warning can name is a verb the listing can name, which is @@ -2227,13 +2240,24 @@ mod tests { assert!(e.contains("dispatch carries {new_session}"), "{e}"); } - /// The built-in `claude` message verb forks, because a `--bg` session's - /// supervisor refuses a headless resume while it lives (DESIGN.md §8). + /// The built-in `claude` message verb resumes in place (DESIGN.md §8): the + /// supervisor that refuses a headless resume has been released by the + /// rest-stop before any send is made, so the send addresses the session's own + /// reference and the conversation stays under the name Voro composed for it. #[test] - fn the_builtin_claude_message_verb_forks() { + fn the_builtin_claude_message_verb_resumes_in_place() { let message = builtin_agents()["claude"].message().unwrap(); - assert!(message.contains("--fork-session"), "{message}"); - assert!(message.contains(NEW_SESSION_PLACEHOLDER), "{message}"); + assert!(!message.contains("--fork-session"), "{message}"); + assert!(!message.contains(NEW_SESSION_PLACEHOLDER), "{message}"); + assert!(message.contains("-p --resume {session}"), "{message}"); + // The session it resumes is the one it was given, so nothing downstream + // has a new reference to record. + let rendered = render_message(message, "uuid-1", Path::new("/tmp/p.txt")); + assert_eq!(rendered.new_session_ref, None); + assert!( + rendered.command.contains("--resume 'uuid-1'"), + "{rendered:?}" + ); } #[test] @@ -2450,6 +2474,33 @@ mod tests { assert_eq!(entry(r#""state": "idle""#), SessionLiveness::Dead); } + /// Rest is a narrower reading than death (DESIGN.md §8): the rest-stop acts + /// on a turn that has *ended*, and only `done` says so. `blocked` is the + /// separation that matters — dead to the liveness question, yet a turn still + /// under way (a permission prompt, a supervisor mid-turn) that a stop would + /// cut off. + #[test] + fn at_rest_is_done_alone() { + let entry = |json: &str| { + let listing = format!("[{{\"sessionId\": \"u\", {json}}}]"); + parse_sessions_json(&listing).unwrap().remove(0) + }; + assert!(entry(r#""state": "done""#).at_rest()); + // a supervisor that outlives the turn does not make it unfinished + assert!(entry(r#""state": "done", "pid": 4321"#).at_rest()); + for json in [ + r#""state": "blocked""#, + r#""state": "blocked", "pid": 4321"#, + r#""state": "working""#, + r#""state": "idle""#, + r#""state": "something-new""#, + r#""pid": 4321"#, + r#""cwd": "/tmp""#, + ] { + assert!(!entry(json).at_rest(), "{json}"); + } + } + #[test] fn parse_sessions_json_rejects_non_arrays() { assert!(parse_sessions_json("{}").is_err()); diff --git a/crates/voro/src/app.rs b/crates/voro/src/app.rs index af578cb..ebb4092 100644 --- a/crates/voro/src/app.rs +++ b/crates/voro/src/app.rs @@ -351,8 +351,12 @@ pub struct AttachRequest { /// liveness probe reads to be sure the session is between turns. struct MessageTarget { /// The session row the send updates once it is confirmed — its process, and - /// its reference where the agent's verb forks (DESIGN.md §8). - session_id: i64, + /// its reference where the agent's verb forks (DESIGN.md §8). Carried whole + /// rather than as an id, because the inline rest-stop the send may have to + /// make first is addressed at the session itself. + session: voro_core::Session, + /// The reference the send is addressed to: the row's, already established to + /// be present. session_ref: String, template: String, sessions_cmd: Option, @@ -2269,15 +2273,35 @@ impl App { self.report(refreshed); } - /// Say [`NUDGE`] into one capped session and record the send, exactly as the - /// quick-message key does — the same verb, the same forked reference, the - /// same tracked pid — so a nudged session stays as visible to the reconciler - /// as a messaged one. + /// Say [`NUDGE`] into one capped session and record the send, much as the + /// quick-message key does — the same verb, the same tracked pid — so a + /// nudged session stays as visible to the reconciler as a messaged one. + /// + /// It releases the session first, and *unconditionally*, which is the one + /// place a send departs from the rest rule (DESIGN.md §8). That rule's + /// `done` test is only sufficient because every session it does not cover is + /// refused by the liveness gate before a send is ever attempted — and a + /// capped session is exactly such a session, `blocked` with its supervisor + /// alive, which the sweep walks past on the strength of the cap reading. + /// Having stood down the guard that made the test sufficient, it cannot then + /// lean on the test: the hold is there, nothing will ever report it as + /// `done`, and an in-place resume would simply be refused. The bypass has to + /// be complete or the nudge does not land. + /// + /// What that costs is worth naming. The stop is exactly as safe as the cap + /// reading is right — the same bet the sweep already makes — but the + /// consequence of a wrong one is worse than it was: a send into a session + /// that turned out to be mid-turn used to be a redundant turn, and is now a + /// killed one. The reading can also be up to a probe interval stale, so a + /// session an operator restarted by hand in the last minute is still badged + /// and can still be stopped from under them. Both are why the sweep stays on + /// a keypress rather than on the clock (DESIGN.md §8). fn nudge_one(&mut self, task_id: i64) -> Result<(), String> { let target = self .message_target(task_id) .ok_or_else(|| self.status.clone().unwrap_or_else(|| "no session".into()))?; let cwd = self.task_checkout(task_id).map_err(|e| e.to_string())?; + self.release_session(&target.session)?; let sent = crate::dispatch::send_message( &self.dispatch_ctx, crate::dispatch::SessionMessage { @@ -2291,7 +2315,7 @@ impl App { let pid = sent.pid(); if let Err(e) = self.store - .record_session_send(target.session_id, sent.new_session_ref(), pid) + .record_session_send(target.session.id, sent.new_session_ref(), pid) { sent.abandon(); return Err(format!( @@ -2302,6 +2326,24 @@ impl App { Ok(()) } + /// Release the agent's hold on a session, waiting for the answer (DESIGN.md + /// §8), so a headless resume into it can land. The reconciler's rest-stop + /// makes this call off the send path on every pass; the two senders make it + /// inline where that pass cannot have covered them — the quick-message key + /// for the window between an agent handing back and the next pass noticing, + /// the capped-session sweep because no pass will ever release its target. + /// + /// Nothing about the row changes either way — the session stays the task's + /// conversation — so a config that will not load costs the release and + /// nothing else, and the send that follows is refused by the agent rather + /// than by Voro. + fn release_session(&self, session: &voro_core::Session) -> Result<(), String> { + let Ok(config) = AgentsConfig::load(&self.dispatch_ctx.agents_path) else { + return Ok(()); + }; + crate::dispatch::stop_session_now(&self.dispatch_ctx, &config, session) + } + /// Resolve what a quick message needs, reporting whichever piece is missing /// on the status line exactly as `jump_into_session` does. Config is loaded /// fresh, so an agent that gained a `message` verb since the TUI started @@ -2342,10 +2384,10 @@ impl App { return None; }; Some(MessageTarget { - session_id: session.id, - session_ref, - template: template.to_string(), sessions_cmd: agent.and_then(|a| a.sessions()).map(str::to_string), + template: template.to_string(), + session: session.clone(), + session_ref, }) } @@ -2378,18 +2420,34 @@ impl App { let Some(target) = self.message_target(task_id) else { return; }; - // A live session is mid-turn, so a headless resume would either be - // refused or land out of order; the operator wants the real terminal. - if crate::session_probe::session_is_live( + // One listing read answers both of the questions the send has about the + // session: whether it is mid-turn, and whether the agent is still + // holding it registered at rest. + let verdict = crate::session_probe::probe_session( target.sessions_cmd.as_deref(), Some(&target.session_ref), - ) == Some(true) - { + ); + // A live session is mid-turn, so a headless resume would either be + // refused or land out of order; the operator wants the real terminal. + if verdict.live == Some(true) { self.status = Some(format!( "task {task_id}'s session is still running — A attaches to it" )); return; } + // Normally reconcile has already released a handed-back session + // (DESIGN.md §8), and this finds nothing to do. It fires when the + // operator has outrun a pass — messaging within the same tick the agent + // reported in — and the hold that would refuse an in-place resume is + // still there. Failing to release it refuses the send outright rather + // than spawning one that cannot land, so the task is left exactly where + // it was. + if verdict.at_rest + && let Err(e) = self.release_session(&target.session) + { + self.status = Some(format!("{e} — task {task_id} is unchanged")); + return; + } let cwd = match self.task_checkout(task_id) { Ok(path) => path, Err(e) => { @@ -2428,7 +2486,7 @@ impl App { let pid = sent.pid(); if let Err(e) = self.store - .record_session_send(target.session_id, sent.new_session_ref(), pid) + .record_session_send(target.session.id, sent.new_session_ref(), pid) { sent.abandon(); self.status = Some(format!( @@ -5680,6 +5738,9 @@ mod tests { task_id: i64, project_path: std::path::PathBuf, listing: std::path::PathBuf, + /// Where the stub's `stop` verb records the reference it was fired at, + /// so a test can tell a release that happened from one that did not. + stopped: std::path::PathBuf, } /// Rewrite the canned listing, moving the session between live and @@ -5703,6 +5764,7 @@ mod tests { let (mut store, ctx, project_path) = scratch_env("jumpin", None); let listing = project_path.parent().unwrap().join("listing.json"); write_listing(&listing, listing_json); + let stopped = project_path.parent().unwrap().join("stopped"); let templates = [ ("sessions", format!("cat '{}'", listing.display())), ("attach", "agent attach {session}".into()), @@ -5711,6 +5773,10 @@ mod tests { "message", "sleep 30 # agent message {session} {prompt_file}".into(), ), + ( + "stop", + format!("printf '%s' {{session}} >> '{}'", stopped.display()), + ), ] .into_iter() .filter(|(verb, _)| verbs.contains(verb)) @@ -5749,12 +5815,13 @@ mod tests { task_id: task.id, project_path, listing, + stopped, } } /// Every session verb, the ordinary configuration. fn all_verbs() -> &'static [&'static str] { - &["sessions", "attach", "resume", "message"] + &["sessions", "attach", "resume", "message", "stop"] } // --- capped-but-alive sessions (task #415) --- @@ -5783,15 +5850,22 @@ mod tests { // the quick-message key uses, so the stub defines one that records what // it was told and exits — a delivered send, as far as the caller can see. let delivered = project_path.parent().unwrap().join("delivered.txt"); + // A capped session is `blocked` with its supervisor alive, so the hold + // that would refuse an in-place resume is still there and the sweep has + // to release it itself (DESIGN.md §8). The stub records what it was + // fired at, so a test can see that it ran and at which session. + let stopped = project_path.parent().unwrap().join("stopped.txt"); std::fs::write( &ctx.agents_path, format!( "default_agent = \"stub\"\n\n[agents.stub]\n\ dispatch = \"cat {{prompt_file}} && sleep 30\"\n\ sessions = \"cat '{}'\"\n\ - message = \"cat {{prompt_file}} >> '{}' # {{session}}\"\n{logs}", + message = \"cat {{prompt_file}} >> '{}' # {{session}}\"\n\ + stop = \"printf '%s' {{session}} >> '{}'\"\n{logs}", listing.display(), - delivered.display() + delivered.display(), + stopped.display() ), ) .unwrap(); @@ -5906,6 +5980,11 @@ mod tests { std::fs::read_to_string(project_path.parent().unwrap().join("delivered.txt")).ok() } + /// Which session the sweep released before sending, if it released one. + fn nudge_stopped(project_path: &std::path::Path) -> Option { + std::fs::read_to_string(project_path.parent().unwrap().join("stopped.txt")).ok() + } + /// The headline case (DESIGN.md §8): one key puts every capped session whose /// window has reopened back to work, without the operator visiting any of /// them. Both the guards the quick-message key answers to are stood down — @@ -5947,6 +6026,70 @@ mod tests { let _ = std::fs::remove_dir_all(project_path.parent().unwrap()); } + /// The sweep releases its target before resuming it, and does so + /// unconditionally (DESIGN.md §8). A capped session is `blocked` with its + /// supervisor alive — it never reads `done`, so no reconcile pass will ever + /// release it, and the rest rule's own test would answer "nothing to do" + /// while the hold that refuses an in-place resume sits right there. The + /// sweep has already walked past the liveness gate that makes that test + /// sufficient, so it cannot lean on the test. + #[test] + fn u_releases_the_capped_session_before_resuming_it() { + let (mut app, task_id, project_path) = + cap_env(true, "Session limit reached - Retrying in 5m (9:50pm)"); + settle_cap(&mut app, task_id, true); + app.now_minutes = Some(22 * 60 + 50); + + key(&mut app, KeyCode::Char('u')); + + assert_eq!( + nudge_stopped(&project_path).as_deref(), + Some("ref-1"), + "the sweep released the session it was about to resume: {:?}", + app.status + ); + // And the send still went, at the same reference: a release, not a move. + assert_eq!( + delivered(&project_path).as_deref().map(str::trim), + Some(NUDGE) + ); + let session = app.store.sessions_for(task_id).unwrap().remove(0); + assert_eq!(session.session_ref.as_deref(), Some("ref-1")); + + let _ = std::fs::remove_dir_all(project_path.parent().unwrap()); + } + + /// A release that fails means the hold is still there and the resume behind + /// it could only be refused, so the nudge is abandoned before it spawns + /// anything — the session keeps its badge and the sweep reports the refusal + /// rather than counting a send that never happened. + #[test] + fn a_nudge_whose_release_fails_sends_nothing() { + let (mut app, task_id, project_path) = + cap_env(true, "Session limit reached - Retrying in 5m (9:50pm)"); + settle_cap(&mut app, task_id, true); + app.now_minutes = Some(22 * 60 + 50); + set_verb( + &app.dispatch_ctx.agents_path, + "stop", + "printf 'no such session' >&2; exit 1 # {session}", + ); + + key(&mut app, KeyCode::Char('u')); + + assert_eq!(delivered(&project_path), None, "nothing was sent"); + assert!( + app.caps.contains_key(&task_id), + "the badge stays, since the session was never nudged" + ); + let status = app.status.as_deref().unwrap_or("").to_string(); + assert!(status.contains("nudged 0"), "{status}"); + assert!(status.contains("refused"), "{status}"); + assert!(status.contains("could not be released"), "{status}"); + + let _ = std::fs::remove_dir_all(project_path.parent().unwrap()); + } + /// A cap whose window has not reopened is left alone: nudging it would spend /// a send on a session the agent will only refuse again, and the badge is /// the operator's cue that there is nothing to do yet. @@ -6401,18 +6544,26 @@ mod tests { let _ = std::fs::remove_dir_all(&root); } - /// Rewrite the stub agent's `message` verb — loaded fresh on every send, so - /// a test can change what a send *does* after the environment is built. - fn set_message_verb(env: &JumpIn, template: &str) { - let config = std::fs::read_to_string(&env.ctx.agents_path).unwrap(); - let rewritten = config + /// Rewrite one of the stub agent's verbs — the config is loaded fresh on + /// every send, so a test can change what a send *does* after the environment + /// is built. + /// Define or redefine one verb of the stub agent, by path — so a test can + /// also do it *after* the App is built, which is what isolates a verb the + /// reconcile-on-read pass would otherwise have fired first. + fn set_verb(agents_path: &std::path::Path, verb: &str, template: &str) { + let prefix = format!("{verb} = "); + let mut config: String = std::fs::read_to_string(agents_path) + .unwrap() .lines() - .map(|line| match line.starts_with("message = ") { - true => format!("message = \"{template}\"\n"), - false => format!("{line}\n"), - }) - .collect::(); - std::fs::write(&env.ctx.agents_path, rewritten).unwrap(); + .filter(|line| !line.starts_with(&prefix)) + .map(|line| format!("{line}\n")) + .collect(); + config.push_str(&format!("{prefix}\"{template}\"\n")); + std::fs::write(agents_path, config).unwrap(); + } + + fn set_message_verb(env: &JumpIn, template: &str) { + set_verb(&env.ctx.agents_path, "message", template); } /// The defect this ordering exists for (task #390): the send is what the @@ -6500,6 +6651,182 @@ mod tests { let _ = std::fs::remove_dir_all(&root); } + // --- releasing a session the agent still holds (task #428) --- + + /// A review task whose session verbs are all defined *except* `stop`, plus + /// the config path a test adds one at afterwards. Withholding it until the + /// App exists is what separates the send path's own inline release from the + /// reconcile-on-read pass that would otherwise have made it first. + fn send_env(listing: &str) -> (App, i64, std::path::PathBuf, SendPaths) { + let mut env = jump_in_env(&["sessions", "attach", "resume", "message"], listing); + env.store + .apply(env.task_id, Action::Complete(None)) + .unwrap(); + let root = env.project_path.parent().unwrap().to_path_buf(); + let paths = SendPaths { + agents_path: env.ctx.agents_path.clone(), + stopped: env.stopped.clone(), + }; + let task_id = env.task_id; + (App::new(env.store, env.ctx).unwrap(), task_id, root, paths) + } + + /// The two files a send test writes to and reads back after the App has + /// taken ownership of everything else. + struct SendPaths { + agents_path: std::path::PathBuf, + stopped: std::path::PathBuf, + } + + impl SendPaths { + /// A `stop` verb that records the reference it was fired at. + fn recording_stop(&self) { + set_verb( + &self.agents_path, + "stop", + &format!("printf '%s' {{session}} >> '{}'", self.stopped.display()), + ); + } + + /// What the stop verb recorded; empty when no stop ran. + fn stopped_refs(&self) -> String { + std::fs::read_to_string(&self.stopped).unwrap_or_default() + } + } + + /// The inline half of the rest rule (DESIGN.md §8). Normally reconcile has + /// already released a handed-back session and this finds nothing to do; when + /// the operator outruns a pass, the send releases the session itself, at its + /// own reference, and then resumes it in place. Without that the agent's hold + /// would refuse the resume and the feedback would go nowhere. + #[test] + fn a_send_releases_a_session_the_agent_still_holds_at_rest() { + let (mut app, task_id, root, paths) = send_env(FINISHED_LISTING); + paths.recording_stop(); + app.toggle_screen(); + send_message(&mut app, "the tests are missing"); + + assert_eq!(paths.stopped_refs(), "ref-1"); + let task = app.store.task(task_id).unwrap(); + assert_eq!(task.state, TaskState::Running); + assert!(task.body.contains("the tests are missing"), "{}", task.body); + // Released, not moved: the send is addressed at the same reference the + // stop named, and the row still holds it afterwards. + assert!( + launches(&root).contains("agent message 'ref-1'"), + "{}", + launches(&root) + ); + let session = app.store.sessions_for(task_id).unwrap().remove(0); + assert_eq!(session.session_ref.as_deref(), Some("ref-1")); + + let _ = std::fs::remove_dir_all(&root); + } + + /// The other half of the same guard: nothing about a send stops a session + /// unconditionally. A `blocked` entry is a turn still under way and an + /// absent one has nothing registered to release, so both go straight to the + /// send — the stop verb is defined and simply never fires. + #[test] + fn a_send_makes_no_stop_when_the_session_is_not_listed_at_rest() { + for (name, listing) in [("blocked", ZOMBIE_LISTING), ("absent", "[]")] { + let (mut app, task_id, root, paths) = send_env(listing); + paths.recording_stop(); + app.toggle_screen(); + send_message(&mut app, "the tests are missing"); + + assert_eq!( + paths.stopped_refs(), + "", + "{name}: stopped a session mid-turn" + ); + assert_eq!( + app.store.task(task_id).unwrap().state, + TaskState::Running, + "{name}" + ); + assert!(launches(&root).contains("agent message 'ref-1'"), "{name}"); + + let _ = std::fs::remove_dir_all(&root); + } + } + + /// A release that fails is a session still held, so the resume behind it + /// could only be refused. The send is abandoned before it is spawned and the + /// task is left exactly where it was — the same commit-nothing rule a + /// refused send already answers to. + #[test] + fn a_failed_release_refuses_the_send_and_commits_nothing() { + let (mut app, task_id, root, paths) = send_env(FINISHED_LISTING); + set_verb( + &paths.agents_path, + "stop", + "printf 'no such session' >&2; exit 1 # {session}", + ); + app.toggle_screen(); + send_message(&mut app, "the tests are missing"); + + let task = app.store.task(task_id).unwrap(); + assert_eq!(task.state, TaskState::Review); + assert!( + !task.body.contains("the tests are missing"), + "{}", + task.body + ); + assert!( + !app.store + .events_for(task_id) + .unwrap() + .iter() + .any(|e| e.kind == "feedback"), + "no rejection is logged for a message that was never sent" + ); + assert!( + !launches(&root).contains("agent message"), + "{}", + launches(&root) + ); + let status = app.status.as_deref().unwrap_or("").to_string(); + assert!(status.contains("could not be released"), "{status}"); + assert!(status.contains("unchanged"), "{status}"); + + let _ = std::fs::remove_dir_all(&root); + } + + /// An agent defining no `stop` verb is one Voro was never going to release, + /// so the send goes as it always did and whatever the agent makes of it is + /// the agent's answer — a refusal there, not a refusal here. + #[test] + fn a_send_without_a_stop_verb_is_unaffected() { + let (mut app, task_id, root, _paths) = send_env(FINISHED_LISTING); + app.toggle_screen(); + send_message(&mut app, "the tests are missing"); + + assert_eq!(app.store.task(task_id).unwrap().state, TaskState::Running); + assert!(launches(&root).contains("agent message 'ref-1'")); + + let _ = std::fs::remove_dir_all(&root); + } + + /// A confirmed in-place send moves the pid and nothing else: one session id + /// for the task's whole life, which is what the operator's `voro-` name + /// is attached to. + #[test] + fn a_confirmed_in_place_send_moves_the_pid_and_keeps_the_reference() { + let (mut app, task_id, root, _paths) = send_env(FINISHED_LISTING); + let before = app.store.sessions_for(task_id).unwrap().remove(0); + app.toggle_screen(); + send_message(&mut app, "the tests are missing"); + + let after = app.store.sessions_for(task_id).unwrap().remove(0); + assert_eq!(after.id, before.id, "the same session row"); + assert_eq!(after.session_ref.as_deref(), Some("ref-1")); + assert_ne!(after.pid, before.pid, "the process carrying the turn"); + assert!(crate::session_probe::pid_is_alive(after.pid.unwrap())); + + let _ = std::fs::remove_dir_all(&root); + } + /// A session still running is mid-turn, so the headless send is refused /// and the operator is pointed at the terminal instead. Nothing is sent /// and — the part that matters — nothing is transitioned. diff --git a/crates/voro/src/cli.rs b/crates/voro/src/cli.rs index 6f52e25..a41467e 100644 --- a/crates/voro/src/cli.rs +++ b/crates/voro/src/cli.rs @@ -2589,10 +2589,10 @@ mod tests { assert!(listed.contains("claude"), "{listed}"); assert!(listed.contains("codex"), "{listed}"); assert!(listed.contains("built-in"), "{listed}"); - // every optional verb the agent defines, the quick message included and - // marked as the forking send it is + // every optional verb the agent defines, the quick message included — + // named plainly, since the built-in resumes its session in place assert!( - listed.contains("[sessions attach resume message(fork) logs stop plan]"), + listed.contains("[sessions attach resume message logs stop plan]"), "{listed}" ); diff --git a/crates/voro/src/dispatch.rs b/crates/voro/src/dispatch.rs index 01fc628..1b3db09 100644 --- a/crates/voro/src/dispatch.rs +++ b/crates/voro/src/dispatch.rs @@ -1023,6 +1023,13 @@ const MESSAGE_POLL_INTERVAL: Duration = Duration::from_millis(25); /// How much of a failed send's log to look at for the line to quote back. const LOG_TAIL_BYTES: u64 = 4096; +/// How long the send path's inline rest-stop is waited on before the send is +/// refused ([`stop_session_now`]). Generous next to the sub-second call it +/// covers, and it is only ever paid when the operator has outrun a reconcile +/// tick — but bounded, because a stop that hangs must not take the cockpit with +/// it. +const STOP_WAIT: Duration = Duration::from_secs(5); + /// One line said into a session that already exists (DESIGN.md §8), assembled by /// the TUI's quick-message key. Unlike an [`Expansion`] this opens no session /// row: it joins a conversation Voro already knows about rather than starting @@ -1272,11 +1279,77 @@ pub(crate) fn append_launch_log(path: &Path, line: &str) { /// log rather than an error the caller must decide about. Nothing waits on it — /// the entry is gone or it is not, and Voro reads neither answer. pub fn stop_session(ctx: &DispatchCtx, config: &AgentsConfig, session: &Session) { + let Ok(Some((mut child, label, launch_log))) = spawn_stop(ctx, config, session) else { + return; + }; + // Nothing waits on the stop, so reap it off the loop — an exited child must + // not linger as a zombie in a long-lived TUI. + std::thread::spawn(move || { + if let Ok(status) = child.wait() + && !status.success() + { + append_launch_log(&launch_log, &format!("{label}: exited with {status}")); + } + }); +} + +/// The same stop, waited on: the send path's inline fallback (DESIGN.md §8), +/// where the operator has outrun a reconcile tick and the session is still +/// registered at rest. Unlike the detached form the answer matters — a session +/// whose hold was not released cannot be resumed in place — so this reports +/// rather than merely logging, and the caller refuses the send on an `Err` +/// having committed nothing. +/// +/// "Nothing to stop" is `Ok(())`, not a failure: an agent that defines no `stop` +/// verb, or a session with no captured reference, is one Voro was never going to +/// release, and the send that follows either lands or is refused by the agent +/// itself — which is the pre-#428 behaviour, not a regression this should +/// pre-empt. A stop still running when [`STOP_WAIT`] is up is a failure, since +/// the lock demonstrably has not been released yet; the straggler is reaped off +/// the loop as the detached form's is. +pub fn stop_session_now( + ctx: &DispatchCtx, + config: &AgentsConfig, + session: &Session, +) -> Result<(), String> { + let Some((mut child, label, launch_log)) = spawn_stop(ctx, config, session)? else { + return Ok(()); + }; + match wait_for_early_exit(&mut child, STOP_WAIT) { + Some(status) if status.success() => Ok(()), + Some(status) => { + append_launch_log(&launch_log, &format!("{label}: exited with {status}")); + Err(format!( + "the session could not be released for a headless resume ({status}){}", + log_tail_note(&launch_log) + )) + } + None => { + std::thread::spawn(move || { + let _ = child.wait(); + }); + Err(format!( + "the session is still being released after {}s", + STOP_WAIT.as_secs() + )) + } + } +} + +/// Spawn an agent's `stop` verb at one session, shared by the detached and +/// waited-on forms. `Ok(None)` is nothing to stop — no verb, no reference, or a +/// launch log that would not open — and `Err` is a spawn that failed, both +/// already recorded in the launch log where there was anything to record. +fn spawn_stop( + ctx: &DispatchCtx, + config: &AgentsConfig, + session: &Session, +) -> Result, String> { let (Some(template), Some(session_ref)) = ( config.agent(&session.agent).and_then(|a| a.stop()), session.session_ref.as_deref(), ) else { - return; + return Ok(None); }; let command = voro_core::render_session(template, session_ref); let launch_log = ctx.launch_log_path(); @@ -1288,33 +1361,26 @@ pub fn stop_session(ctx: &DispatchCtx, config: &AgentsConfig, session: &Session) .append(true) .open(&launch_log) else { - return; + return Ok(None); }; - let Ok(log_err) = log.try_clone() else { return }; - let child = Command::new("sh") + let Ok(log_err) = log.try_clone() else { + return Ok(None); + }; + match Command::new("sh") .arg("-c") .arg(&command) .stdin(Stdio::null()) .stdout(Stdio::from(log)) .stderr(Stdio::from(log_err)) .process_group(0) - .spawn(); - let mut child = match child { - Ok(child) => child, + .spawn() + { + Ok(child) => Ok(Some((child, label, launch_log))), Err(e) => { append_launch_log(&launch_log, &format!("{label}: cannot spawn: {e}")); - return; + Err(format!("the session's agent could not be stopped: {e}")) } - }; - // Nothing waits on the stop, so reap it off the loop — an exited child must - // not linger as a zombie in a long-lived TUI. - std::thread::spawn(move || { - if let Ok(status) = child.wait() - && !status.success() - { - append_launch_log(&launch_log, &format!("{label}: exited with {status}")); - } - }); + } } /// Load the agents config for a one-off [`stop_session`], best-effort: a missing diff --git a/crates/voro/src/reconcile.rs b/crates/voro/src/reconcile.rs index 395df7c..0afdd8b 100644 --- a/crates/voro/src/reconcile.rs +++ b/crates/voro/src/reconcile.rs @@ -59,9 +59,15 @@ //! //! Whatever a pass finalises, it also stops (task #433): the agent's `stop` verb //! is fired at the closed session's reference so its own listing loses the entry -//! along with the row, best-effort and unwaited-on. Sessions left open — -//! `needs-input`, `review`, `waiting` — are never stopped; the operator still -//! answers and rejects into them. +//! along with the row, best-effort and unwaited-on. +//! +//! A session left *open* is stopped too, on a narrower test — the rest-stop +//! ([`rest_stop`], task #428). A task in `needs-input`, `review` or `waiting` +//! has handed back, and once its listing entry agrees that the turn is over, the +//! agent's hold on the session is released. Its row stays open and stays the +//! task's conversation; only the registration goes, and with it the lock that +//! made a headless quick message impossible to deliver in place. The operator +//! still answers and rejects into that session, and `A` still opens it. //! //! There is no daemon watching for process exit. Reconciliation runs on read: //! `App::refresh` and every CLI verb call [`reconcile_live_sessions`] before @@ -77,9 +83,13 @@ use voro_core::{ use crate::dispatch::{DispatchCtx, stop_session}; use crate::session_probe::{ - listing_says_live, pid_is_alive, read_session_cap, run_sessions_command, + listing_says_at_rest, listing_says_live, pid_is_alive, read_session_cap, run_sessions_command, }; +/// An agent's session listing for this pass, keyed by agent name. `None` against +/// a name means the listing was asked for and could not be read. +type Listings = HashMap>>; + /// How much of a session's log tail to scan for a usage-cap signature. const LOG_TAIL_BYTES: u64 = 4096; @@ -98,17 +108,23 @@ pub fn reconcile_live_sessions(store: &mut Store, ctx: &DispatchCtx) -> Result>> = HashMap::new(); + let mut listings: Listings = HashMap::new(); let mut finalised = 0; for session in live { let task_state = store.task(session.task_id)?.state; if !matches!(task_state, TaskState::Running | TaskState::Refining) { - // needs-input / review keep their session open (reconcile_session - // returns None); a session on a closed task is stale and finalised. + // needs-input / review / waiting keep their session open + // (reconcile_session returns None); a session on a closed task is + // stale and finalised. if let Some((closed, _)) = store.reconcile_session(session.id, false, false)? { stop_finalised(ctx, config.as_ref(), &closed); finalised += 1; + } else if matches!( + task_state, + TaskState::NeedsInput | TaskState::Review | TaskState::Waiting + ) { + rest_stop(ctx, config.as_ref(), &mut listings, &session); } continue; } @@ -123,22 +139,10 @@ pub fn reconcile_live_sessions(store: &mut Store, ctx: &DispatchCtx) -> Result { - let sessions_cmd = config - .as_ref() - .and_then(|c| c.agent(&session.agent)) - .and_then(|a| a.sessions()); - sessions_cmd - .zip(session.session_ref.as_deref()) - .and_then(|(cmd, session_ref)| { - let listing = listings - .entry(session.agent.clone()) - .or_insert_with(|| run_sessions_command(cmd, None)); - listing - .as_ref() - .map(|entries| listing_says_live(entries, session_ref)) - }) - } + LivenessSource::Listing => session.session_ref.as_deref().and_then(|session_ref| { + listing(config.as_ref(), &mut listings, &session.agent) + .map(|entries| listing_says_live(entries, session_ref)) + }), }; // A recorded process that is still there proves the session is live // whatever the listing says: a quick message forks a `-p` turn that @@ -180,6 +184,61 @@ pub fn reconcile_live_sessions(store: &mut Store, ctx: &DispatchCtx) -> Result( + config: Option<&AgentsConfig>, + listings: &'a mut Listings, + agent: &str, +) -> Option<&'a [AgentSessionEntry]> { + let cmd = config?.agent(agent)?.sessions()?; + listings + .entry(agent.to_string()) + .or_insert_with(|| run_sessions_command(cmd, None)) + .as_deref() +} + +/// Release a session that has handed back (DESIGN.md §8): a task at rest — +/// `needs-input`, `review`, `waiting` — whose session the agent still holds +/// registered with its turn ended is stopped, so the lock a headless message +/// resumes through is already gone by the time the operator sends one. +/// +/// The rest state is only half the test, because a task reaches it the moment +/// the agent calls `voro done`/`ask` — from inside a turn still running, whose +/// tail a stop would cut off. The listing supplies the other half: the entry +/// must itself read `at_rest`, which it does only once that turn is over. +/// Everything else is left alone — a `blocked` entry is mid-turn or waiting on a +/// permission prompt, and an absent one was never registered or has been stopped +/// already. +/// +/// Nothing about the row changes: the session is still the task's conversation, +/// `A` still reopens it with its full context, and only the agent-side +/// registration goes. Which is also why firing this on every pass converges +/// rather than repeating — a stopped session leaves the listing, so the next +/// pass finds nothing at rest to stop. +fn rest_stop( + ctx: &DispatchCtx, + config: Option<&AgentsConfig>, + listings: &mut Listings, + session: &voro_core::Session, +) { + let Some(session_ref) = session.session_ref.as_deref() else { + return; + }; + let handed_back = listing(config, listings, &session.agent) + .is_some_and(|entries| listing_says_at_rest(entries, session_ref)); + if !handed_back { + return; + } + // `handed_back` can only be true with a config loaded, since the listing + // came out of one; the agent defining no `stop` verb is skipped in there. + if let Some(config) = config { + stop_session(ctx, config, session); + } +} + /// Retire the agent's registry entry for a session reconciliation has just /// finalised (DESIGN.md §8) — both flavours, the dead dispatch it stalls and the /// stale row it heals, since either way the session is over and Voro has said so. @@ -713,23 +772,26 @@ mod tests { // --- finalising a session stops it (task #433) --- - /// A `voro.toml` whose `claude` agent lists nothing and whose `stop` verb + /// A `voro.toml` whose `claude` agent lists `listing` and whose `stop` verb /// records the reference it was fired at, plus that marker's path — so a /// test can tell a stop that happened from one that did not, and read which /// session it named. - fn stop_fixture(name: &str) -> (DispatchCtx, PathBuf, PathBuf) { + fn stop_fixture_listing(name: &str, listing: &str) -> (DispatchCtx, PathBuf, PathBuf) { let dir = std::env::temp_dir().join(format!("voro-reconcile-stop-{name}-{}", std::process::id())); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).unwrap(); let marker = dir.join("stopped"); + let listing_path = dir.join("sessions.json"); + std::fs::write(&listing_path, listing).unwrap(); let agents_path = dir.join("voro.toml"); std::fs::write( &agents_path, format!( "default_agent = \"claude\"\n\n[agents.claude]\n\ - dispatch = \"cat {{prompt_file}}\"\nsessions = \"printf '[]'\"\n\ + dispatch = \"cat {{prompt_file}}\"\nsessions = \"cat '{}'\"\n\ stop = \"printf '%s' {{session}} >> '{}'\"\n", + listing_path.display(), marker.display() ), ) @@ -737,6 +799,13 @@ mod tests { (ctx_at(agents_path), marker, dir) } + /// The same fixture with an empty listing, which is what every close-time + /// stop test wants: the session is finalised on its own terms and the + /// listing has nothing to say about it either way. + fn stop_fixture(name: &str) -> (DispatchCtx, PathBuf, PathBuf) { + stop_fixture_listing(name, "[]") + } + /// Wait for a detached stop to have written its marker, since nothing in the /// reconcile path waits on it. Returns what it wrote, or `None` if it never /// ran. @@ -802,37 +871,185 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } - /// The sessions that stay open are never stopped, whatever their process is - /// doing: the operator answers into a `needs-input` session and rejects into - /// a `review` one, so retiring either would take away the conversation they - /// are about to continue. + // --- the rest-stop releases a session that has handed back (task #428) --- + + /// The transitions that put a task at rest with its session still open, and + /// the name each test labels its fixture with. + fn rest_states() -> Vec<(&'static str, Vec)> { + vec![ + ("needs-input", vec![Action::Ask("A or B?".into())]), + ("review", vec![Action::Complete(None)]), + ("waiting", vec![Action::Complete(None), Action::HandOff]), + ] + } + + /// A task at rest with an open session answering to `full-uuid-1`. + fn task_at_rest(actions: &[Action]) -> (Store, i64, i64) { + let (mut s, task_id) = running_task(); + let session = s + .create_session( + task_id, + "claude", + Some(dead_pid()), + LivenessSource::Listing, + None, + ) + .unwrap(); + s.set_session_ref(session.id, "full-uuid-1").unwrap(); + for action in actions { + s.apply(task_id, action.clone()).unwrap(); + } + (s, task_id, session.id) + } + + /// The rule itself (DESIGN.md §8): a task that has handed back, whose + /// session's own listing entry agrees the turn is over, has that session + /// released — so the lock a headless quick message resumes through is gone + /// before the operator sends one. Every state the message key serves. + #[test] + fn a_handed_back_session_at_rest_is_stopped() { + for (name, actions) in rest_states() { + let (ctx, marker, dir) = + stop_fixture_listing(name, r#"[{"sessionId": "full-uuid-1", "state": "done"}]"#); + let (mut s, task_id, session_id) = task_at_rest(&actions); + let before = s.task(task_id).unwrap().state; + + assert_eq!(reconcile_live_sessions(&mut s, &ctx).unwrap(), 0, "{name}"); + assert_eq!( + stopped_ref(&marker).as_deref(), + Some("full-uuid-1"), + "{name}" + ); + // Nothing about the task or the row moves: the session is still the + // task's conversation, and only the agent-side registration went. + let session = s.session(session_id).unwrap(); + assert!(session.ended_at.is_none(), "{name}"); + assert_eq!(session.outcome, None, "{name}"); + assert_eq!( + session.session_ref.as_deref(), + Some("full-uuid-1"), + "{name}" + ); + assert_eq!(s.task(task_id).unwrap().state, before, "{name}"); + + let _ = std::fs::remove_dir_all(&dir); + } + } + + /// The guard that makes the rule safe in the other direction. A `blocked` + /// entry is a turn still under way — a permission prompt, a supervisor + /// mid-turn — and the handover verbs fire from *inside* the turn that + /// reports, so a task reaches `review` while its agent is still finishing + /// the sentence. Reading rest off the task state alone would cut that off. + /// An absent entry has nothing to release. #[test] - fn a_session_kept_open_is_never_stopped() { - for (name, action) in [ - ("needs-input", Action::Ask("A or B?".into())), - ("review", Action::Complete(None)), + fn a_session_not_at_rest_is_left_registered() { + for (name, listing, actions) in [ + ( + "blocked", + r#"[{"sessionId": "full-uuid-1", "state": "blocked"}]"#, + vec![Action::Complete(None)], + ), + ( + "working", + r#"[{"sessionId": "full-uuid-1", "state": "working"}]"#, + vec![Action::Ask("A or B?".into())], + ), + ("absent", "[]", vec![Action::Complete(None)]), ] { - let (ctx, marker, dir) = stop_fixture(name); - let (mut s, task_id) = running_task(); - let session = s - .create_session( - task_id, - "claude", - Some(dead_pid()), - LivenessSource::Listing, - None, - ) - .unwrap(); - s.set_session_ref(session.id, "full-uuid-1").unwrap(); - s.apply(task_id, action).unwrap(); + let (ctx, marker, dir) = stop_fixture_listing(name, listing); + let (mut s, ..) = task_at_rest(&actions); assert_eq!(reconcile_live_sessions(&mut s, &ctx).unwrap(), 0, "{name}"); - assert!(!marker.exists(), "{name} stopped a session it kept open"); + std::thread::sleep(std::time::Duration::from_millis(150)); + assert!(!marker.exists(), "{name}: stopped a session mid-turn"); let _ = std::fs::remove_dir_all(&dir); } } + /// A `running` task is mid-work by definition and is never released, + /// whatever its listing entry happens to say — the entry going `done` there + /// is a session that died without reporting, which the liveness arm + /// finalises and stops on its own terms rather than leaving the row open. + #[test] + fn a_running_task_is_not_rest_stopped() { + let (ctx, marker, dir) = stop_fixture_listing( + "running", + r#"[{"sessionId": "full-uuid-1", "state": "working"}]"#, + ); + let (mut s, task_id) = running_task(); + let session = s + .create_session(task_id, "claude", None, LivenessSource::Listing, None) + .unwrap(); + s.set_session_ref(session.id, "full-uuid-1").unwrap(); + + assert_eq!(reconcile_live_sessions(&mut s, &ctx).unwrap(), 0); + std::thread::sleep(std::time::Duration::from_millis(150)); + assert!(!marker.exists()); + assert_eq!(s.task(task_id).unwrap().state, TaskState::Running); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// An agent with no `stop` verb skips in silence, as it does at close time: + /// its sessions stay registered and nothing errors. `codex` needs none. + #[test] + fn a_stopless_agent_is_not_rest_stopped() { + // The same `done` listing the rule fires on — only the `stop` verb is + // missing, so this is the verb's absence deciding and nothing else. + let (ctx, dir) = sessions_fixture( + "rest-stopless", + r#"[{"sessionId": "full-uuid-1", "state": "done"}]"#, + ); + let (mut s, task_id, session_id) = task_at_rest(&[Action::Complete(None)]); + + assert_eq!(reconcile_live_sessions(&mut s, &ctx).unwrap(), 0); + assert!(s.session(session_id).unwrap().ended_at.is_none()); + assert_eq!(s.task(task_id).unwrap().state, TaskState::Review); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// A session with no captured reference has nothing to name in a stop, and + /// the rest-stop is skipped rather than guessed at. + #[test] + fn a_refless_session_at_rest_is_not_stopped() { + let (ctx, marker, dir) = stop_fixture_listing( + "rest-refless", + r#"[{"sessionId": "full-uuid-1", "state": "done"}]"#, + ); + let (mut s, task_id) = running_task(); + s.create_session(task_id, "claude", None, LivenessSource::Listing, None) + .unwrap(); + s.apply(task_id, Action::Complete(None)).unwrap(); + + assert_eq!(reconcile_live_sessions(&mut s, &ctx).unwrap(), 0); + std::thread::sleep(std::time::Duration::from_millis(150)); + assert!(!marker.exists()); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// The rest-stop writes nothing, so a pass that makes one leaves the event + /// log exactly as it found it — the session's registration is agent-side + /// state, not Voro's. + #[test] + fn a_rest_stop_records_no_event() { + let (ctx, marker, dir) = stop_fixture_listing( + "rest-events", + r#"[{"sessionId": "full-uuid-1", "state": "done"}]"#, + ); + let (mut s, task_id, _) = task_at_rest(&[Action::Complete(None)]); + let before = s.events_for(task_id).unwrap().len(); + + assert_eq!(reconcile_live_sessions(&mut s, &ctx).unwrap(), 0); + assert_eq!(stopped_ref(&marker).as_deref(), Some("full-uuid-1")); + assert_eq!(s.events_for(task_id).unwrap().len(), before); + + let _ = std::fs::remove_dir_all(&dir); + } + /// An agent naming no `stop` verb degrades to what Voro did before: the row /// closes, the entry lingers, and nothing errors. #[test] diff --git a/crates/voro/src/session_probe.rs b/crates/voro/src/session_probe.rs index e20e9bf..dcedf63 100644 --- a/crates/voro/src/session_probe.rs +++ b/crates/voro/src/session_probe.rs @@ -61,6 +61,18 @@ pub fn listing_says_live(entries: &[AgentSessionEntry], session_ref: &str) -> bo .is_some_and(entry_is_live) } +/// Whether a listing shows this ref as a session whose turn has ended and which +/// the agent is therefore still holding registered — the rest-stop's trigger +/// (DESIGN.md §8). A ref that has dropped out of the listing answers no: it was +/// never registered, or it has already been stopped, and either way there is +/// nothing to release. +pub fn listing_says_at_rest(entries: &[AgentSessionEntry], session_ref: &str) -> bool { + entries + .iter() + .find(|e| e.matches_ref(session_ref)) + .is_some_and(AgentSessionEntry::at_rest) +} + /// Whether a process with this pid still exists, via `kill -0` (existence /// check, no signal sent). A non-positive pid is refused: 0 and negative pids /// address process groups, not the single process meant here. Shared by both @@ -117,12 +129,40 @@ pub fn local_minutes() -> Option { (hour < 24 && minute < 60).then_some(hour * 60 + minute) } -/// Whether a session is still running, for a caller holding no listing of its -/// own — one probe, one answer. `None` when liveness is unknowable. +/// What one listing says about one session, for a caller that needs more than +/// the liveness bool out of a single probe — the send path, which asks both +/// whether the session is mid-turn (refuse) and whether it is registered at rest +/// (release it first). Running the listing twice for the two questions would +/// cost a second subprocess on a keypress, and could answer them from two +/// different readings of a session that moved in between. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub struct SessionVerdict { + /// Whether the session is still going, `None` when that is unknowable. + pub live: Option, + /// Whether the agent still holds it registered with its turn ended + /// ([`listing_says_at_rest`]). Unknowable reads as no, since the rest-stop + /// acts only on positive evidence. + pub at_rest: bool, +} + +/// Read one session's listing entry, for a caller holding no listing of its own +/// — one probe, both answers. +pub fn probe_session(sessions_cmd: Option<&str>, session_ref: Option<&str>) -> SessionVerdict { + let read = || { + let session_ref = session_ref?; + let entries = run_sessions_command(sessions_cmd?, None)?; + Some(SessionVerdict { + live: Some(listing_says_live(&entries, session_ref)), + at_rest: listing_says_at_rest(&entries, session_ref), + }) + }; + read().unwrap_or_default() +} + +/// Whether a session is still running, for a caller that needs only that. +/// `None` when liveness is unknowable. pub fn session_is_live(sessions_cmd: Option<&str>, session_ref: Option<&str>) -> Option { - let session_ref = session_ref?; - let entries = run_sessions_command(sessions_cmd?, None)?; - Some(listing_says_live(&entries, session_ref)) + probe_session(sessions_cmd, session_ref).live } #[cfg(test)] @@ -241,6 +281,38 @@ mod tests { assert!(minutes < 24 * 60, "{minutes}"); } + /// The rest reading the send path and the reconciler act on: a session the + /// agent still holds registered with its turn ended. `blocked` — a + /// permission prompt, a supervisor mid-turn — is the one that must not + /// answer yes, and an entry that has left the listing has nothing to + /// release. + #[test] + fn only_a_done_entry_reads_as_at_rest() { + let at_rest = |json: &str| probe_session(Some(&listing_cmd(json)), Some("uuid-1")).at_rest; + assert!(at_rest(r#"[{"sessionId": "uuid-1", "state": "done"}]"#)); + assert!(!at_rest(r#"[{"sessionId": "uuid-1", "state": "blocked"}]"#)); + assert!(!at_rest(r#"[{"sessionId": "uuid-1", "state": "working"}]"#)); + assert!(!at_rest("[]")); + // and the unknowable cases read as no rather than as a stop to make + assert!(!probe_session(None, Some("uuid-1")).at_rest); + assert!(!probe_session(Some("false"), Some("uuid-1")).at_rest); + assert!(!probe_session(Some(&listing_cmd("[]")), None).at_rest); + } + + /// Both readings come off one listing run, so the send path cannot see a + /// session as mid-turn and at rest from two different moments. + #[test] + fn one_probe_answers_both_questions() { + let cmd = listing_cmd(r#"[{"sessionId": "uuid-1", "state": "done"}]"#); + assert_eq!( + probe_session(Some(&cmd), Some("uuid-1")), + SessionVerdict { + live: Some(false), + at_rest: true + } + ); + } + /// The three ways liveness is unknowable, each answering `None` rather /// than picking a side. #[test] diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 17657eb..d867149 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -296,17 +296,29 @@ Cheap actions need one further guard, or the pricing swaps one swamping for anot **Answering a question happens in the session, not through Voro.** When an agent hits a blocker it calls `voro ask`, landing the task in `needs-input` with its `question` set. Voro's job from there is to be an accurate *signpost* — which task is blocked, on what question, surfaced on the inbox row and in the detail pane — not the door into the conversation. The operator opens the agent's own session directly (the `voro--` session it runs under, in a `claude agents` pane or the equivalent) and answers there, where the agent still has its full context; Voro records no answer text, since the exchange lives in the session transcript, addressable through the session ref already captured on the session row. Once answered, `voro resume ` moves the task `needs-input → running` and nothing more: the dispatch preamble tells the agent to run it in-session after its question is answered (the primary path), and Enter on a `needs-input` inbox row is the operator's backstop for the same transition. This replaces an earlier headless-continuation design — a fresh session re-sent the whole task body carrying the appended answer — whose only reliable path for the built-in `claude`, which has no headless *continue* verb, was to *restart* the task rather than resume the conversation; a human already watching the session is a strictly better place to answer than a text box in Voro. `voro reject` is the symmetric move on the `review → running` (and `waiting → running`) edge: it appends the feedback to the task body under a `## Feedback` heading and returns the task to `running`. Because `review`/`waiting` keep the session open (see *Session lifecycle* below), the feedback lands back on the *same* agent session when its process is still alive — the operator having stayed attached — and otherwise the task stalls on the next reconcile and is redispatched with the feedback now in its body (the redispatch prompt carries it). Both `resume` and `reject` route through the transition API and change only state, so neither can smuggle a task into `running` outside the machine; a task never dispatched still answers or rejects as a plain transition, since nothing about them depends on a prior session. -**Steering a session without entering it** is the cheap half of that door. Answering in the session is right, but suspending the whole cockpit for a full attach round-trip to say one sentence is not, so the agent verb set gains an optional `message`: a *headless* send carrying both `{session}` and `{prompt_file}`, which appends one turn to an existing session's transcript and returns without owning the terminal. It is the only session verb Voro backgrounds, and — once its delivery is confirmed, below — the only one that is fire-and-forget: Voro reads no reply, so what the agent says afterwards lands in the launch log rather than in the UI, and the exchange is simply there on any later attach. The built-in `claude` spells it as its `resume` plus `-p`, forked; the near-duplication between verb bodies is accepted rather than factored, because a verb is an opaque per-agent contract, and that opacity is exactly what lets an agent define a subset of the verbs and degrade one at a time. It applies to the three states whose session is open and between turns — `needs-input`, `review`, `waiting` — and is refused on the rest: `running` and `refining` are mid-turn with no injection channel, and `stalled` has a dead session, where a headless resume would restart the work with no tracked pid and no session row, invisible to the reconciler. Redispatch is the honest path there. A liveness probe refuses a session still running for the same reason the state gate does, and the send opens no session row: it joins a conversation Voro already knows about rather than starting one. In the cockpit this is `a`, with the interactive jump-in moving to `A` — the lowercase-quick, uppercase-interactive pairing the `r`/`R` refine keys already use (§9). +**Steering a session without entering it** is the cheap half of that door. Answering in the session is right, but suspending the whole cockpit for a full attach round-trip to say one sentence is not, so the agent verb set gains an optional `message`: a *headless* send carrying both `{session}` and `{prompt_file}`, which appends one turn to an existing session's transcript and returns without owning the terminal. It is the only session verb Voro backgrounds, and — once its delivery is confirmed, below — the only one that is fire-and-forget: Voro reads no reply, so what the agent says afterwards lands in the launch log rather than in the UI, and the exchange is simply there on any later attach. The built-in `claude` spells it as its `resume` plus `-p`, resuming the session in place; the near-duplication between verb bodies is accepted rather than factored, because a verb is an opaque per-agent contract, and that opacity is exactly what lets an agent define a subset of the verbs and degrade one at a time. It applies to the three states whose session is open and between turns — `needs-input`, `review`, `waiting` — and is refused on the rest: `running` and `refining` are mid-turn with no injection channel, and `stalled` has a dead session, where a headless resume would restart the work with no tracked pid and no session row, invisible to the reconciler. Redispatch is the honest path there. A liveness probe refuses a session still running for the same reason the state gate does, and the send opens no session row: it joins a conversation Voro already knows about rather than starting one. In the cockpit this is `a`, with the interactive jump-in moving to `A` — the lowercase-quick, uppercase-interactive pairing the `r`/`R` refine keys already use (§9). **A send that is refused must change nothing, so delivery is confirmed before the rejection commits.** A headless send can be refused outright, and the case that matters is not exotic: a `claude --bg` session that has finished its turn is still owned by a live supervisor process, and that supervisor refuses `--resume` for as long as it lives. Fire-and-forget hid it — the refusal exited in under a second into the launch log while the transition had already appended the feedback and returned the task to `running`, leaving a task nobody was working on, a body claiming otherwise, and a reconcile pass that stalled it for redispatch a moment later. So the ordering is inverted: the send is spawned first and watched for a short grace window, an early non-zero exit is reported as a message that did not happen — with the agent's own last log line quoted on the status line — and the task stays exactly where it was, feedback unwritten. Only a send still running past the window is followed by the session-row update and the `RejectWork` transition, together, so no other window's reconcile reads one without the other. A clean exit inside the window is a delivery, not a failure: a verb that says its piece and returns has done its job. This trades a lost transition for a lost send in the rare case where the store write fails after the spawn — and that case takes the agent down with it (the process group is killed) rather than leaving it working on feedback nothing records. -**Where a session cannot be resumed, it is forked, and the session reference follows the fork.** The `message` template may carry a third, optional placeholder, `{new_session}`, which Voro binds to a freshly generated v4 UUID: an agent declares by using it that its sessions are joined by forking rather than resumed in place, and the built-in `claude` message verb does exactly that (`--fork-session --session-id {new_session}`), because forking is the one scriptable channel into a supervisor-held session. The fork continues the same conversation under a reference the caller names up front, which is what makes it usable here — Voro records that reference on the session row once the send is confirmed, so the next message, the next jump-in, and the reconciler all address the conversation where it actually continued. A verb without the placeholder resumes in place and keeps the reference it had, so the headless-resume agents are unaffected. One consequence reaches reconciliation: a forked `-p` turn does not appear in the agent's own session listing at all, so the listing would report the session gone while the message it was just sent is still being answered. The row's recorded pid settles it, in one direction only — a *live* pid proves the session is live whatever the listing says, since the quick message replaces that pid with the process carrying its turn, while a dead pid still proves nothing (a dispatch's pid is a launcher that exits at birth) and falls back to the listing verdict. +**A message resumes the session in place, and what makes that possible is releasing the session at rest.** Every agent session, foreground or background, is a server process registered with a daemon, and `-p --resume` bypasses that daemon to own the transcript file — which is why the registry refuses it while a supervisor lives. Voro's answer is not to route around that hold but to remove it: the built-in `claude` message verb is a plain `claude -p --resume {session}`, and the session is stopped, through the same optional `stop` verb the close path uses, as soon as it comes to rest — so the hold is already gone by the time any message is sent. One session id, one Voro-composed name, one linear transcript for the task's whole life, and no kill step anywhere near a send. + +Fork delivery was the previous answer to the same problem, and mechanically it worked. The `message` template may still carry a third, optional placeholder, `{new_session}`, which Voro binds to a freshly generated v4 UUID: an agent declares by using it that its sessions are joined by forking rather than resumed in place, the fork continues the same conversation under a reference the caller names up front, and the session row follows it — Voro records that reference once the send is confirmed, so the next message, the next jump-in and the reconciler all address the conversation where it actually continued. It shipped, lived briefly, and was reverted for the built-in, because the *name* is the operator's addressing scheme: the cockpit is a tmux split with Voro on one side and the agent's own session list on the other, `voro-` is the join key between them and the only handle away from the desktop, and a fork moves the conversation out from under that name on every send — the live continuation never appears in the agent's session view, the row that does appear is the stale parent, and the picker accumulates one same-named transcript per message. Naming the fork patches the symptom; the disease is the fork. The placeholder stays for agents whose sessions genuinely can only be joined that way, and for them the row still follows; the built-in simply no longer uses it. + +Both stop and fork are back doors around the same ownership model, and stop is the back door that preserves session identity, which is why it is the one Voro takes. The sanctioned front door exists — cross-session messaging wakes an idle background session in place — but has no supported external entry today: no CLI send verb, an in-session-only SDK send, an undocumented inbox-socket frame, and *channels*, the designed push mechanism whose contract and permission relay fit Voro exactly, gated behind a research preview whose dev-flag bypass the background launch path strips. When an external injection path lands, the `message` verb swaps to it and the stop machinery below stops being necessary — a config-sized change under the agent contract, because a verb is an opaque per-agent contract and nothing above it knows how a message is carried. + +One consequence of a headless send reaches reconciliation either way: a `-p` turn does not appear in the agent's own session listing while it runs, so the listing would report the session gone while the message it was just sent is still being answered. The row's recorded pid settles it, in one direction only — a *live* pid proves the session is live whatever the listing says, since the quick message replaces that pid with the process carrying its turn, while a dead pid still proves nothing (a dispatch's pid is a launcher that exits at birth) and falls back to the listing verdict. + +**The release happens at rest, not at the transition that hands back.** The handover verbs fire mid-turn: `voro done` and `voro ask` run inside the agent's still-executing turn, so stopping at the transition itself would kill the tail of the very turn that is reporting. The trigger is therefore *rest*, judged from the two sources reconciliation already reads — a task in a between-turns state (`needs-input`, `review`, `waiting`) whose open session's listing entry reports its turn ended (state `done`) is stopped, best-effort, exactly as a closing row is. The guard is load-bearing in both directions. A session at `blocked` — a permission prompt, a supervisor mid-turn — never reads `done`, so it is never stopped; an entry that is absent was never registered or has already been stopped, so there is nothing to do, and the stop is idempotent and safe on a dead session. Nothing about the row changes: it stays open, it stays the task's conversation, and no event and no transition is recorded. Only the agent-side registration goes, which is also why firing the rule on every pass converges rather than repeating — a stopped session leaves the listing, so the next pass finds nothing at rest to stop. + +What emerges is an invariant worth stating on its own: **a message can only ever be delivered to a session that has explicitly handed back to Voro.** Mid-turn, the liveness gate refuses the send honestly; between turns, the rest-stop has already released the hold. Consecutive messages fall under the same rule, and it does not matter whether a finished `-p` turn puts the session back in the agent's registry: where it does, the entry reads `done` again and the next pass releases it again before anything can be sent; where it does not, there was never a hold to release. Either way, after one message the task is back at rest — the agent called `voro done` — or `running` with a dead pid, which reconciliation already stalls. Stacked mid-turn sends are structurally impossible rather than merely discouraged. The send path carries no unconditional stop of its own: it gates on liveness as before, and only where the target's listing entry still reads `done` — the operator outrunning a reconcile tick, messaging within the same moment the agent reported in — does it make the same release inline and waited-on, refusing the send outright if that release fails. A message that could not be made deliverable commits nothing and leaves the task exactly where it was, which is the rule a refused send already answers to. + +The trade, recorded rather than discovered later: releasing at rest retires the session's entry from the agent's own view at *handover* rather than at close, so a review task's named row lives in Voro's queue and not in the agent's session list. Attach still opens the stopped session with its full context — a stop keeps the conversation — so jumping in, answering in-session and reading the output are all unaffected. **A permission mode is a property of a launch, not of a verb.** `--permission-mode` is per invocation rather than something the session remembers, so every built-in template that hands an agent a prompt and expects it to *act* carries it — `dispatch`, `plan` and `message` alike — and a turn launched without it runs in the default ask mode, stopping on approvals against a stdin at `/dev/null`. The built-in `claude` `resume` is the deliberate exception rather than an oversight: it carries no prompt and starts no work of its own, handing the operator a terminal in which the ask-mode default is answerable by the person sitting in front of it. Omitting the flag on a prompted launch fails in the way that is hardest to read back. The session thinks, is refused its edits and its commands, and so cannot run `voro done` — it exits having done real work Voro never hears about, the reconciler finds the process gone, and the task lands in `stalled`. A missing flag therefore surfaces as a *dead agent*, sending the operator to liveness (which is working) rather than to the launch that could not act, which is why the property is asserted over the built-in templates by a test instead of being left to each verb's spelling. **Session lifecycle.** A session's life follows the *task*, not the agent's process listing. An open session therefore no longer implies the task is *executing*: a refine round (§6) opens one too, in the same transaction as `proposed → refining`, and closes it on the transition back — `completed` when the rewritten body landed, `failed` when the agent died, `aborted` when the round was quit or cancelled. What a session means is "an agent Voro launched is working on this task", and which kind of work it is comes from the task's state, which is why every session-consuming query reads that state rather than the session's existence: the running strip lists `running` and `refining` (§9), reconciliation probes those two and leaves the rest alone, and dispatch's preconditions never look at sessions at all. The dispatch half of that life is unchanged: a session is opened at dispatch (in the same transaction as `ready → running`), stays open across `running → needs-input → review` — `needs-input` keeps it open so the operator answers in that same session, and `review` keeps it open so a reject-with-feedback returns the work to it — and is closed by the terminal transition that tears the running work down, stamped with the matching outcome in the same transaction: `Accept` closes it `completed`, `Abort` and `Abandon` close it `aborted`. `Resume` and `RejectWork` deliberately leave it open (the task returns to `running` on the session it already had). `waiting` (§6) behaves exactly as `review` here: the hand-off keeps the session open, so a change-requested `RejectWork` from `waiting` returns to the same agent session, and `Accept`/`Abandon` from `waiting` close it (`completed`/`aborted`) precisely as they do from `review`. Reconciliation therefore leaves a `waiting` task's open session untouched regardless of process liveness, the same treatment it gives `needs-input` and `review`. A task holds **at most one open session** as an invariant: opening a redispatch first closes any predecessor still open in the same transaction, enforced by a partial unique index on `sessions(task_id) WHERE ended_at IS NULL`. Rows stay one-per-attempt — each keeps its own pid, log, and outcome, and the redispatch flag still derives from the latest one — but two can never be open at once, so a task can never render twice in the running strip. -A session's entry in the *agent's own* registry follows its row in the same way: closing the row stops the session, so the agent's listing converges on work actually in flight. The listing is how the operator finds a session to attach to and how Voro reads liveness, and both get worse the longer it grows — a `claude --bg` session outlives its work twice over, keeping its entry in `claude agents` and a supervisor process that runs until the machine reboots, so an operator who dispatches all week reads their session list through a wall of finished ones. The stop rides an *optional* `stop` verb (below), fired at the closing session's reference, detached, its output going to the launch log: an agent that defines none — or one whose `stop` fails — degrades to exactly what Voro did before, a lingering listing entry and nothing broken. The transition never waits on it and never rolls back for it. Which closes stop is deliberately narrower than which closes happen: the operator's closing verdicts (`Accept`, `Abort`, `Abandon`) stop, and so do the reconciler's finalisations, both the dead-dispatch stall and the stale-row heal, since by then the session is over and Voro has said so. Sessions that stay open — `needs-input`, `review`, `waiting` — are never stopped; the operator still answers and rejects into them. A refine round's conclusion is the one close that does not stop, because its commonest trigger is the rewriting agent's own `voro set --body-file`, a call made from inside the session and mid-turn: stopping there would kill the agent that just reported. `voro-core` decides *whether* a close stops (`Store::apply_closing` hands back the session a verdict retired, `Store::reconcile_session` the one a pass finalised); the `voro` crate supplies the spawn, beside the other process seams. +A session's entry in the *agent's own* registry follows its row in the same way: closing the row stops the session, so the agent's listing converges on work actually in flight. The listing is how the operator finds a session to attach to and how Voro reads liveness, and both get worse the longer it grows — a `claude --bg` session outlives its work twice over, keeping its entry in `claude agents` and a supervisor process that runs until the machine reboots, so an operator who dispatches all week reads their session list through a wall of finished ones. The stop rides an *optional* `stop` verb (below), fired at the closing session's reference, detached, its output going to the launch log: an agent that defines none — or one whose `stop` fails — degrades to exactly what Voro did before, a lingering listing entry and nothing broken. The transition never waits on it and never rolls back for it. Which closes stop is deliberately narrower than which closes happen: the operator's closing verdicts (`Accept`, `Abort`, `Abandon`) stop, and so do the reconciler's finalisations, both the dead-dispatch stall and the stale-row heal, since by then the session is over and Voro has said so. Sessions that stay open — `needs-input`, `review`, `waiting` — are not stopped by a *close*, because there is no close; they are released by the rest-stop above instead, on its own narrower test, and that release takes the registration without taking the row, so the operator still answers and rejects into them. A refine round's conclusion is the one close that does not stop, because its commonest trigger is the rewriting agent's own `voro set --body-file`, a call made from inside the session and mid-turn: stopping there would kill the agent that just reported. `voro-core` decides *whether* a close stops (`Store::apply_closing` hands back the session a verdict retired, `Store::reconcile_session` the one a pass finalised); the `voro` crate supplies the spawn, beside the other process seams. **Worktree lifecycle.** A dispatched agent does its work in a throwaway git worktree of the project checkout it creates itself — the dispatch preamble instructs this, and Voro runs no git during dispatch (below), so the branch and its worktree are the agent's to make. *How* it makes one is the agent's own business, and the preamble prefers the harness's mechanism to a hand-rolled `git worktree add` where one exists: Claude Code refuses file edits until its `EnterWorktree` tool has run, and aiming that tool at an already-made worktree raises an approval prompt a headless session cannot answer, so an agent following the manual instruction literally hangs at the first edit. Such a tool names the branch itself, so the preamble also spells out the `git switch -c ` that puts the work on the branch Voro tracks. Where that worktree lands is immaterial to everything downstream — `open` and the cleanup below find it through `git worktree list` on the checkout, which sees a worktree nested under the checkout (Claude Code puts them in `.claude/worktrees/`) exactly as it sees a sibling one. Nothing else prunes those, so a worktree's lifetime is tied to the *session's*: it lives as long as the session does, and is torn down when the session closes — that is, at the terminal transition that closes the task. The teardown is owned by Voro rather than the agent because by then the agent has exited, and because it is an operator action: "Voro runs no git" governs branch *management during dispatch*, not operator-invoked git at task close. Only the closing transitions that discard or accept the work clean up — `Accept` and `Abandon`; `Abort` deliberately does not, since it returns the task to the queue and its in-progress worktree may be wanted on redispatch. Given a task with a branch, Voro finds the worktree of the project checkout on that branch (never the primary checkout) and removes it with a plain, non-forced `git worktree remove`: a dirty worktree makes git refuse, which is reported and left in place rather than force-removed, and the transition stands regardless. With the worktree gone the branch is checked out nowhere and can be deleted too — but only when its work is verifiably upstream, since squash-merging (this repo's convention) leaves the branch tip a non-ancestor of `main` that `git branch -d` will not recognise: a merged PR (checked via the task's `pr_url` with `gh pr view`) authorises a `git branch -D`, and without one a plain `git branch -d` is attempted and the branch left alone if git refuses. An unverified branch is never force-deleted. This on-close cleanup is a CLI-only affair: only `voro accept`/`abandon` perform it, announcing every destructive step before it runs — the operator is shown the worktree path, the branch, and why it is judged safe, and confirms at a `y/N` prompt (with `--yes` to skip it for scripting); declining skips the cleanup but still completes the transition. Closing a task in the TUI does no cleanup at all — the transition applies and the worktree and branch are left in place, to be removed later on the CLI or by hand. The git/`gh` I/O lives in the `voro` crate beside dispatch, so `voro-core` stays free of process and filesystem I/O. @@ -329,9 +341,11 @@ Three properties keep that badge honest. It carries **no state change**: `stalle **Recovering a capped session** is then one key, `u`, which nudges *every* badged session whose reset has gone by. A cap does not retry: it ends the session's turn and leaves it sitting there, so work waits for a human however long ago the window reopened — and walking the strip by hand costs an attach, a typed word and a detach per session, which is how the overnight reset hours get lost. The sweep is that walk as a single keystroke, and it reports what it did: how many it nudged, how many are still before their reset, and any the agent refused. The message it sends is one word, *continue*, because the session already holds the whole task — its transcript, its worktree, its half-written work — and anything longer would be Voro restating a brief the agent can already read. -It goes out through the existing `message` verb rather than through any new channel. The verb forks (`--fork-session` with a pre-assigned `{new_session}`), which is what makes it land at all: a supervisor-owned session refuses a plain headless `--resume` for as long as its supervisor lives, and a capped session's supervisor is alive by definition. A forked send has been confirmed accepted against a live, supervisor-held session mid-turn — a strictly harder case than a capped one, which sits idle with its turn already ended — so no `tmux send-keys` channel or supervisor IPC is needed, and none is built. The send is recorded exactly as a quick message is, with the forked reference and the pid now carrying the turn, so a nudged session stays as visible to the reconciler as a messaged one; the badge is dropped the moment the send lands, so a second press cannot put a second agent on the same worktree, and it returns on the next reading if the session is still held. +It goes out through the existing `message` verb rather than through any new channel, and it is the one send that releases its target *unconditionally* first. A supervisor-owned session refuses a plain headless `--resume` for as long as its supervisor lives, and a capped session's supervisor is alive by definition, so something has to remove that hold. The rest-stop above never will: it fires on a listing entry that reads `done`, and a capped session reads `blocked` — the same word a permission prompt earns — for as long as it sits there. Nor can the sweep wait for the rule, because the rule's `done` test is only *sufficient* by virtue of the liveness gate refusing everything it does not cover, and the sweep has already walked past that gate. Having stood down the guard that makes the test enough, it cannot then lean on the test; the bypass has to be complete or the nudge does not land. So the sweep stops the session itself, waits for the answer, and resumes it in place, abandoning the nudge if the release fails rather than spawning a send that could only be refused. No `tmux send-keys` channel or supervisor IPC is needed, and none is built. The send is otherwise recorded exactly as a quick message is, with the pid now carrying the turn, so a nudged session stays as visible to the reconciler as a messaged one; the badge is dropped the moment the send lands, so a second press cannot put a second agent on the same worktree, and it returns on the next reading if the session is still held. + +Two costs come with that unconditional stop, priced rather than discovered. The stop is exactly as safe as the cap reading is right — the same bet the sweep already makes when it skips the guards — but the *consequence* of a wrong reading is worse than it was under a forked send: a nudge into a session that turned out to be mid-turn was once a redundant turn and is now a killed one. And the reading is debounced to one probe per session per minute, so it can be that stale: a session an operator restarted by hand a moment ago is still badged, and can be stopped from under them. Neither is a reason to route around the release — a fork would land, but at the price the whole delivery model was changed to avoid — and both are reasons the sweep stays on a keypress rather than on the clock. -Two guards are deliberately stood down for it, and the cap reading is what earns that. A quick message is refused on a `running` task because its session is mid-turn, and refused again when the session is listed live — but a capped session is `running`, listed live, and *not* mid-turn, which is the one combination nothing else in the cockpit can recognise. Nothing else may skip those guards. The sweep fires only when pressed, and that is a staging decision rather than a principle: automatic resumption once the window reopens is wanted, and this is deliberately the half it can be built on top of. Manual first buys the evidence automation needs — that a nudge reliably lands, and that the badge it would key on does not false-positive — while a wrong reading still costs one keypress instead of an unwatched agent. What automation adds is a trigger, not a channel: the reset-passed test the badge already computes, evaluated on the tick rather than on the key, plus a bound so a session that will not restart is not nudged around the clock. A cap whose reset time never parsed is swept too — the operator pressing the key is the judgement the clock could not supply, and a send that turns out to be early is refused by the agent rather than doing harm — and that is precisely a case automation must decide differently, since no keypress would stand behind it. Because the nudged turn does real work, it depends on the `message` verb's permission mode (above) exactly as a dispatch does: without it the refusals land in the launch log and the send appears delivered while quietly doing nothing, which is the one failure a fire-and-forget channel cannot report. +Two guards are deliberately stood down for it, and the cap reading is what earns that. A quick message is refused on a `running` task because its session is mid-turn, and refused again when the session is listed live — but a capped session is `running`, listed live, and *not* mid-turn, which is the one combination nothing else in the cockpit can recognise. Nothing else may skip those guards, and standing them down is what obliges the sweep to release its own target rather than trusting the rest rule to have done it (above). The sweep fires only when pressed, and that is a staging decision rather than a principle: automatic resumption once the window reopens is wanted, and this is deliberately the half it can be built on top of. Manual first buys the evidence automation needs — that a nudge reliably lands, and that the badge it would key on does not false-positive — while a wrong reading still costs one keypress instead of an unwatched agent. What automation adds is a trigger, not a channel: the reset-passed test the badge already computes, evaluated on the tick rather than on the key, plus a bound so a session that will not restart is not nudged around the clock. A cap whose reset time never parsed is swept too — the operator pressing the key is the judgement the clock could not supply, and a send that turns out to be early costs a released session and a turn that re-caps at once rather than doing lasting harm, the conversation surviving the release either way — and that is precisely a case automation must decide differently, since no keypress would stand behind it. Because the nudged turn does real work, it depends on the `message` verb's permission mode (above) exactly as a dispatch does: without it the refusals land in the launch log and the send appears delivered while quietly doing nothing, which is the one failure a fire-and-forget channel cannot report. A dispatched process must also be reaped once it exits, or it sits as a zombie for the life of the spawning `voro` process — and `kill -0` on a zombie still reports it alive, which would silently defeat this whole mechanism in a long-lived TUI session. Dispatch therefore hands the child to a detached reaper thread the moment the session is recorded, rather than leaving it to `Drop`. diff --git a/docs/agent-integration.md b/docs/agent-integration.md index c707ecc..d394ecd 100644 --- a/docs/agent-integration.md +++ b/docs/agent-integration.md @@ -77,7 +77,7 @@ dispatch = "claude --bg --name \"{session_name}\" --permission-mode auto --mod sessions = "claude agents --json" attach = "claude attach {session}" resume = "claude --resume {session}" -message = "claude -p --resume {session} --fork-session --session-id {new_session} --permission-mode auto \"$(cat {prompt_file})\"" +message = "claude -p --resume {session} --permission-mode auto \"$(cat {prompt_file})\"" logs = "claude logs \"$(printf %.8s {session})\" 2>/dev/null | tail -c 20000" stop = "claude stop \"$(printf %.8s {session})\"" plan = "claude --name \"{session_name}\" --permission-mode auto --model {model} \"$(cat {prompt_file})\"" @@ -138,18 +138,20 @@ resume = "codex resume {session}" the status line and the jump-in still works. - `message` may also carry `{new_session}`, replaced with a fresh v4 UUID Voro generates for the send. Use it when your agent's sessions cannot be resumed - headlessly but can be *forked*: the built-in `claude` message verb is - `--resume` plus `--fork-session --session-id {new_session}`, because a - `claude --bg` session keeps a supervisor process after finishing its turn and - that supervisor refuses a plain `--resume` while it lives. The fork continues - the same conversation under the reference Voro named, and Voro records that - reference on the session row once the send is confirmed — so later messages, - the jump-in keys, and reconciliation all follow the conversation to where it - continued. A `message` template without the placeholder resumes in place and - keeps the reference it had. `{new_session}` is refused on every other verb: - it names the session a send opens, and nothing else opens one. Which of the - two a configured agent got is visible without reading the file back: `voro - agent list` names a forking send `message(fork)` and a resuming one `message`. + headlessly but can be *forked*: the fork continues the same conversation under + the reference Voro named, and Voro records that reference on the session row + once the send is confirmed — so later messages, the jump-in keys, and + reconciliation all follow the conversation to where it continued. A `message` + template without the placeholder resumes in place and keeps the reference it + had, which is what the built-in `claude` one does: a `claude --bg` session + keeps a supervisor process that refuses a plain `--resume` while it lives, and + Voro removes that hold with the `stop` verb when the session comes to rest + rather than forking around it (DESIGN.md §8), so the conversation stays under + one id and one name for the task's whole life. `{new_session}` is refused on + every other verb: it names the session a send opens, and nothing else opens + one. Which of the two a configured agent got is visible without reading the + file back: `voro agent list` names a forking send `message(fork)` and a + resuming one `message`. - A `message` template should carry whatever permission flag its agent's `dispatch` carries — the built-in `claude` one carries `--permission-mode auto`. A resumed turn does real work, and on agents where the flag is per @@ -174,22 +176,50 @@ resume = "codex resume {session}" truncation: `claude logs` keys on the *job* id, the first eight characters of the session id, so `{session}` is trimmed rather than passed whole. - `stop` retires a session from the agent's own registry, taking `{session}` - alone. Voro fires it when it closes the session's row (DESIGN.md §8), so the - agent's listing shows work actually in flight rather than every dispatch ever - made — a `claude --bg` session otherwise keeps both its `claude agents` entry - and a supervisor process until the machine reboots, and that listing is how - you find a session to attach to and how Voro reads liveness. It is fire and - forget: Voro spawns it detached with its output going to the launch log, reads - neither the output nor the exit status, and never waits on it or rolls a - transition back for it. Nothing is checked first, so the verb must tolerate - being fired at a session that is already gone (`claude stop` prints its line - and exits zero). Define it only if stopping is *safe* — Voro fires it on the - operator's closing verdicts (accept, abort, abandon) and on a reconciled dead - or stale session, never on a session it is keeping open for an answer or a - rejection. Note the built-in's truncation, the same as `logs`: `claude stop` - keys on the eight-character job id, so `{session}` is trimmed rather than - passed whole. `codex` names none, and a session under it lingers in whatever - listing it keeps exactly as before. + alone. Voro fires it on three triggers (DESIGN.md §8), and all three must be + safe before you define it. + + The first is **closing the session's row**, so the agent's listing shows work + actually in flight rather than every dispatch ever made — a `claude --bg` + session otherwise keeps both its `claude agents` entry and a supervisor + process until the machine reboots, and that listing is how you find a session + to attach to and how Voro reads liveness. Voro fires it on the operator's + closing verdicts (accept, abort, abandon) and on a reconciled dead or stale + session. + + The second is **rest**: a task in `needs-input`, `review` or `waiting` — a + session Voro is *keeping open* — whose listing entry reports its turn ended + (`state: "done"`) is stopped too. This is what makes an in-place headless + `message` possible at all, since the registration is the hold that would + refuse it. The row is untouched by it: the session stays open, stays the + task's conversation, and stays attachable, so `stop` must keep the + conversation rather than discard it (`claude stop`: "Its conversation is + kept"). The `done` test is the whole guard — a session at `blocked`, which is + what a permission prompt and a supervisor mid-turn both look like, is never + released, because the handover verbs run from inside a turn that may still be + finishing. + + The third is the **capped-session sweep** (`u`), and it is the one that fires + with no listing test at all. A session sitting on a usage cap is `blocked` + with its supervisor alive, so it will never read `done` and the rest trigger + will never reach it — yet the hold is exactly what would refuse the resume the + sweep is about to make. What identifies it instead is the `logs` reading, and + on the strength of that the sweep releases unconditionally. If your agent's + sessions cannot be safely stopped while merely idle-looking, define no `stop` + and the sweep degrades to a refused send rather than doing damage. + + The first two are fire and forget: Voro spawns them detached with output going + to the launch log, reads neither the output nor the exit status, and never + waits or rolls a transition back. A stop made by a *sender* is waited on + instead — the quick message that outran a reconcile pass, and every nudge — + and a non-zero exit refuses that send rather than spawning a message that + could not land. Nothing is checked first in any case, so the verb + must tolerate being fired at a session that is already gone (`claude stop` + prints its line and exits zero). Note the built-in's truncation, the same as + `logs`: `claude stop` keys on the eight-character job id, so `{session}` is + trimmed rather than passed whole. `codex` names none, and a session under it + lingers in whatever listing it keeps exactly as before — and its `message`, + were it to define one, would resume without any release being needed. - `plan` runs an interactive *foreground* session for the TUI's agent-assisted task creation (DESIGN.md §8): `{prompt_file}` holds the planning brief, and the command owns the terminal until the conversation ends, so it must not @@ -260,8 +290,8 @@ agents (DESIGN.md §8). When liveness is unknowable (no ref, listing failed) the session is left alone. The row's own pid is still read in one direction, for every agent: a pid that is *alive* proves the session is, whatever the listing says. That is what a quick message leaves behind — the process carrying its turn -— and a forked send never appears in the listing at all, so without this rule -the next reconcile would stall a task whose agent is mid-answer. +— and a headless send does not appear in the listing while it runs, so without +this rule the next reconcile would stall a task whose agent is mid-answer. **Jump-in.** In the TUI, `A` on a running task runs the agent's `attach` command with the TUI suspended — the real session, full control, including answering @@ -279,7 +309,11 @@ standing. It applies to the three states whose session is open and between turns `refining` are mid-turn with no injection channel, and `stalled` has a dead session that redispatch, not a headless resume, is the honest answer for. Voro probes liveness first and refuses a session still running, since that one wants -the terminal. On a `review` or `waiting` task the message *is* a +the terminal; if the same probe finds the session merely registered at rest, it +releases it through `stop` before sending, which reconciliation has usually done +already. Between them the two checks make one guarantee: a message only ever +reaches a session that has explicitly handed back. On a `review` or `waiting` +task the message *is* a reject-with-feedback: the send goes first and the transition follows it, so feedback is appended to the body and logged only once the message is known to have started, and a send the agent refuses leaves the task untouched. On a @@ -288,7 +322,8 @@ the agent's own `voro resume` moves the task back (DESIGN.md §6). Either way th session row follows the send: it records the process now carrying the turn, so reconciliation leaves the task `running` while the agent answers, and — for a verb that forks (`{new_session}`) — the reference the conversation continued -under. +under. An in-place verb changes no reference, which is the point: one session +id and one name for the task's whole life. The same jump-in resolves a **stale review branch**. A task can sit in `review` while other work merges, leaving its branch in conflict with the moved base @@ -487,6 +522,24 @@ which is what lets Voro fire it without checking first. `claude stop` has its ow it rides an optional verb: a claude that drops it degrades to a lingering listing entry, nothing broken. +The rest-release the built-in `message` depends on is verified on the same +footing, end to end against a real `--bg` dispatch on a scratch store. The +session hands over, reconcile releases it (its entry leaves the default listing; +a second stop the same second is accepted and exits zero, which is the +idempotence the rule relies on), and two quick messages then land back to back +with a `voro done` between them — both rendered as `claude -p --resume ` +against the *same* session id, with no stop between them and no manual step. The +work of all three turns is there afterwards, and the conversation is one +transcript file: no fork siblings. The display name rides the transcript's own +`customTitle` record and survives the stop and both in-place resumes, which is +the whole reason delivery resumes rather than forks. One observation worth +recording because it cuts the other way from the assumption: on this version a +finished `-p` turn did *not* put the session back into the default listing, so +no further release was needed and none was made. The rule is written to be +indifferent to that — a session that does re-register reads `done` and is +released on the next pass — but nothing should be built on the re-registration +happening. + The hooks *firing* is verified against a live Claude Code session (v2.1.206): the sample configuration above, driving a real session under a dispatched task's environment. `SessionEnd` fires on a normal exit and upgrades a still-`running`