diff --git a/AGENTS.md b/AGENTS.md index bccbd04..c07e0d3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -296,10 +296,15 @@ day: `SessionManager::send_message` and `::run_command`, which start a process first when the session is known to have exited and then hand the thing to the driver that has one behind it. Only on `exited`: `unknown` has a - process that may well be reading its fifo. `/rename` is the exception — - the name is persisted and listed either way, so it is forwarded to a - process that happens to be there and never starts one. So the Start button - is for when you want a process and nothing to say to it yet. + process that may well be reading its fifo. `/rename` starts one too, and + for a sharper reason than the rest: the CLI keeps its own copy of the + name, that copy is what its session picker and other agents' session + lists show, and a session is only ever *given* a name at birth — every + later start is a `--resume` — so a rename that reached no process would + leave the two lists disagreeing for good. Its save happens before the + telling, so a failure there says the telling failed rather than the + rename. So the Start button is for when you want a process and nothing to + say to it yet. - **Each session directory now holds `process.json`, `stdin.fifo`, `stdout.log` and `stderr.log`.** `stdout.log` is the driver's input, read from the byte offset in `process.json`; removing either by hand while the diff --git a/PLAN.md b/PLAN.md index 3faffa7..123e936 100644 --- a/PLAN.md +++ b/PLAN.md @@ -380,10 +380,18 @@ Two rules come out of it, and neither is optional: other button, press it, type the thing again. Both plainly mean "do this now", and `--resume` puts the new process on the same conversation, so nothing about what was typed changes — only whether there was anything there -to read it. A rename is deliberately not one of them: it is persisted and -listed whether or not a process ever hears about it, so starting a CLI to -tell it a name would be spending a resume on nothing. The manager's -`send_message` and `run_command` and the Start button ask one function +to read it. A rename is included, and for a sharper reason than the rest: +Claude Code keeps its own copy of the name, that copy is what its session +picker shows and what other agents read when they list sessions, and a +session is only ever *given* a name at birth, since every later start is a +`--resume`. So a rename that reached no process would leave the two lists +disagreeing permanently, with this app's the only one that had moved — and +the cost of a resume buys the one thing renaming is for. It stays +`rename_session` rather than becoming a command like the others, because the +name is persisted and listed as well as forwarded and that is one operation; +the save happens first, so a failure to start reports that the telling +failed, not the rename. The manager's `send_message` and `run_command` and +the Start button ask one function (`start_if_exited`) and want opposite answers from it: "there is already a process" is a refusal worth showing to somebody who pressed Start, and nothing at all to a message. Deciding it in one place under one write lock is diff --git a/server/src/routes.rs b/server/src/routes.rs index 1c9151a..e949d90 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -841,10 +841,14 @@ async fn command( Some((name, rest)) => (name, rest.trim()), None => (text, ""), }; - // Everything but a rename goes through the manager, which starts the - // session's process first if it has exited. A rename is persisted and - // listed whether or not a process hears about it, so it neither needs - // one nor is worth starting one for. + // All of these start the session's process first if it has exited: a + // command is something somebody asked the session to do, and answering + // that its process is gone hands back the work of starting one. + // + // A rename still goes through `rename_session` rather than being a + // command like the rest, because the name is persisted and listed as + // well as forwarded, and that is one operation. It starts a process + // too, and for a sharper reason than the others -- see there. let command = match (name, rest) { ("/compact", _) => SessionCommand::Compact, ("/clear", _) => SessionCommand::Clear, diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index 92f68e0..5daaf08 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -381,13 +381,6 @@ impl LiveSession { self.driver().answer_question(question_id, answers); } - /// Asks the session to run a command on itself, now or at the next - /// boundary. See [`Commands`] for why it may not be now. - pub fn run_command(&self, command: SessionCommand) { - self.commands - .submit(command, *self.shared.status.lock().unwrap()); - } - pub fn interrupt(&self) { self.driver().interrupt(); } @@ -997,6 +990,15 @@ impl SessionManager { /// unlike the model and the permission mode, this is settled here and /// the driver is *told*, rather than asked and believed: see /// [`Driver::set_title`]. + /// + /// Telling it is not decoration, which is why this starts a stopped + /// session like any other command. Claude Code keeps its own copy of + /// the name, and that copy is what its session picker shows and what + /// other agents see when they list sessions -- and a session is only + /// ever *given* a name at birth, since every later start is a + /// `--resume`. So a rename that reached no process would leave the two + /// lists disagreeing permanently, with the app's the only one that had + /// moved. pub fn rename_session(&self, id: &str, title: &str) -> Result<()> { let title = title.trim(); // An empty name is not a name, and it is what a cleared field @@ -1005,25 +1007,37 @@ impl SessionManager { if title.is_empty() { bail!("a session needs a name"); } - let mut inner = self.inner.write().unwrap(); - if !inner.config.sessions.iter().any(|meta| meta.id == id) { - bail!("no session {id}"); + { + let mut inner = self.inner.write().unwrap(); + if !inner.config.sessions.iter().any(|meta| meta.id == id) { + bail!("no session {id}"); + } + let mut candidate = inner.config.clone(); + for meta in candidate.sessions.iter_mut().filter(|meta| meta.id == id) { + meta.title = title.to_string(); + } + candidate.save(&self.config_path)?; + inner.config = candidate; + // The name is this server's and changes now, whatever happens + // next: the list shows it immediately, and the process is told + // at the next boundary. + if let Some(session) = inner.live.get(id) { + *session.shared.title.lock().unwrap() = title.to_string(); + } } - let mut candidate = inner.config.clone(); - for meta in candidate.sessions.iter_mut().filter(|meta| meta.id == id) { - meta.title = title.to_string(); - } - candidate.save(&self.config_path)?; - inner.config = candidate; - if let Some(session) = inner.live.get(id) { - // The name is this server's and changes now. Telling whatever - // runs the session is a command, and commands wait for the - // turn to end -- so the list shows the new name immediately - // and the CLI is told at the next boundary. - *session.shared.title.lock().unwrap() = title.to_string(); - session.run_command(SessionCommand::SetTitle(title.to_string())); - } - Ok(()) + // Dropped the lock first -- `run_command` takes it again to decide + // whether anything needs starting, and this is not a reentrant one. + // + // The context matters more than it looks: the rename above is saved + // by the time this can fail, so a bare error would report a rename + // that did not happen. What failed is only the telling. + self.run_command(id, SessionCommand::SetTitle(title.to_string())) + .with_context(|| { + format!( + "renamed to \"{title}\" here, but the session's own copy of the name could \ + not be changed" + ) + }) } pub fn set_session_model(&self, id: &str, model: &str) -> Result<()> { @@ -1154,11 +1168,6 @@ impl SessionManager { /// for is exactly what a stopped session needs before it is useful /// again. /// - /// Renaming is deliberately not here. It is persisted and listed - /// whether or not a process ever hears about it, so starting a CLI to - /// tell it a name would be spending a resume on nothing -- see - /// [`SessionManager::rename_session`], which forwards it to a process - /// that happens to be there. pub fn run_command(&self, id: &str, command: SessionCommand) -> Result<()> { // Judged against the status *after* the start, not the one that // caused it. A driver that has just started a process announces @@ -2025,7 +2034,9 @@ mod tests { }) .await; - session.run_command(SessionCommand::Clear); + manager + .run_command(&info.id, SessionCommand::Clear) + .expect("clear"); let held = collect_until(&mut rx, |event| { matches!( event, @@ -2336,7 +2347,9 @@ mod tests { }) .await; - session.run_command(SessionCommand::Raw("/tool held".to_string())); + manager + .run_command(&info.id, SessionCommand::Raw("/tool held".to_string())) + .expect("command"); let seen = collect_until(&mut rx, |event| { matches!(event, Event::CommandQueued { .. }) }) @@ -2380,7 +2393,9 @@ mod tests { let session = manager.session(&info.id).expect("live session"); let mut rx = session.subscribe(); - session.run_command(SessionCommand::Raw("/tool now".to_string())); + manager + .run_command(&info.id, SessionCommand::Raw("/tool now".to_string())) + .expect("command"); let seen = collect_until(&mut rx, |event| matches!(event, Event::ToolStart { .. })).await; // Sent, and never queued: a session between turns has nothing to // wait for, and a phone should not draw a bubble that resolves in @@ -2537,7 +2552,9 @@ mod tests { // A clear leaves it unmeasured: the conversation is gone, and how // much is left is a thing nobody has counted. - session.run_command(SessionCommand::Clear); + manager + .run_command(&info.id, SessionCommand::Clear) + .expect("clear"); collect_until(&mut rx, |event| matches!(event, Event::Cleared)).await; assert_eq!(manager.sessions()[0].context_tokens, None); @@ -2713,6 +2730,53 @@ mod tests { assert_ne!(manager.sessions()[0].status, SessionStatus::Exited); } + /// A rename is not decoration, so it starts a stopped session too. + /// + /// Claude Code keeps its own copy of the name; that copy is what its + /// session picker shows and what other agents read when they list + /// sessions, and a session is only ever *given* a name at birth, since + /// every later start is a `--resume`. So a rename that reached no + /// process would leave the two lists disagreeing permanently, with this + /// app's the only one that had moved. + #[tokio::test] + async fn a_rename_reaches_the_process_even_when_one_has_to_be_started() { + let dir = tempfile::tempdir().expect("tempdir"); + let config_path = dir.path().join("config.ron"); + let data_dir = dir.path().join("sessions"); + seed_echo_only(&config_path); + let manager = SessionManager::new(config_path, data_dir.clone(), data_dir.join("models")) + .expect("manager"); + let info = manager.spawn_session(echo_spec()).expect("spawn"); + let session = manager.session(&info.id).expect("live session"); + let mut rx = session.subscribe(); + + let _ = session.sink.send(Event::Status { + state: SessionStatus::Exited, + }); + collect_until(&mut rx, |event| { + matches!( + event, + Event::Status { + state: SessionStatus::Exited + } + ) + }) + .await; + + manager + .rename_session(&info.id, "the new name") + .expect("rename a stopped session"); + collect_until( + &mut rx, + |event| matches!(event, Event::CommandSent { text, .. } if text == "/rename the new name"), + ) + .await; + // Both halves: the name this server lists changed, and it was told + // to a process rather than only written down. + assert_eq!(manager.sessions()[0].title, "the new name"); + assert_ne!(manager.sessions()[0].status, SessionStatus::Exited); + } + /// The status is a claim about a process, and the process record is /// what settles it. ///