Start the process for a rename too -- the CLI keeps its own name
The last commit left renaming out on the grounds that the name is persisted and listed whether or not a process hears about it. That was wrong, and Iris said so: 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 -- every later start is a `--resume`, which passes no `--name`. So a rename that reached no process left the two lists disagreeing permanently, with this app's the only one that had moved. The cost of a resume buys the one thing renaming is for. It stays `rename_session` rather than becoming a command like the rest, because the name is persisted and listed as well as forwarded and that is one operation. The save happens first and the lock is dropped before the telling, so a failure to start reports that the telling failed rather than the rename, which by then has already happened. `LiveSession::run_command` went with it. It read `shared.status` and that read is exactly what a just-started session cannot be judged by, so every caller now goes through the manager -- which is also what the four tests that used it were standing in for. Verified against a stand-in CLI that echoes its stdin: a session reporting `exited` was renamed, the process started with `--resume`, and the CLI received `/rename after the restart` on stdin. The list shows the new name and the session reports idle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
1a132b4de3
commit
6119926a4d
4 files changed
+127
-46
No files matched your search
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
+98
-34
@@ -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.
|
||||
///
|
||||
|
||||
Reference in new issue
Block a user