Stop claiming a terminal, and stop calling a recoverable delete final
Two bugs Bryan hit, with one shape between them: a claim stronger than the thing that was measured. **The delete warning branched on `imported`.** It told him deleting `ai-app` could be undone and deleting `manager` could not, when both are claude-cli sessions whose conversations survive equally. `imported` records how a session got into the app; what decides recoverability is whether the *driver* keeps its own record -- the Claude Code CLI does, under ~/.claude/projects, however the session started; echo and llama.cpp do not, and for those the app's transcript is the only copy. So the fact now sits on DriverKind and rides on SessionInfo, decided by the server from the provider's kind rather than by the phone from its name, which a person can change. The comment above the branch asserted "a session started here has no copy anywhere". That sentence was the bug written down and reasoned from, and it is gone. Neither branch promises a restore, which it should not: nothing here checks the file is still on disk, and re-importing was never a restore anyway -- this app's transcript holds images, peer messages and command events the CLI's record never had. So the recoverable text says what is known and names what goes either way. "Can't be undone" is now said only where it is true, which is the point of saying it at all. **"open in a terminal -- close it there first" named a place that need not exist.** The detection is right and worth keeping: something live holds that session, and importing it would reproduce the double-resume incident. But which something was never measured. The live descriptors here include two of this backend's own adopted sessions and a peer agent's; none is a terminal, so the instruction sent the reader looking for a window that was not there. **And this app did not recognise its own spawned sessions.** The import list filters out what the app is already driving, but it matched only the import cursor -- which exists solely for imported sessions. Every session the app spawned therefore stayed in the list, marked in use, telling the reader to go and close it somewhere: here. Matching the resume token too, which both kinds have, is the fix; `session_importing` is now `session_driving`, because that is what it was always being asked. Verified on the emulator against a scratch backend: a spawned claude-cli session reports keepsOwnTranscript true with imported false -- Bryan's `manager` case exactly -- and draws the recoverable warning; the echo session draws "can't be undone"; and once the CLI named itself, the spawned session's id was absent from the import list, where the old match would have listed it. 75 tests, clippy and rustfmt clean; ktfmt, compileDebugKotlin and lintDebug clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VETa8afmpWaYezLCqJhDB8
This commit is contained in:
1 parent
5f6fd34aba
commit
71067275e2
7 files changed
+181
-29
No files matched your search
+111
-16
@@ -77,6 +77,16 @@ pub struct SessionInfo {
|
||||
pub title: String,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub model: Option<String>,
|
||||
/// Whether the conversation would survive deleting this session --
|
||||
/// see [`DriverKind::keeps_own_transcript`].
|
||||
///
|
||||
/// Reported rather than worked out on the phone, because the phone
|
||||
/// has the provider's *name* and this is a property of its *kind*: a
|
||||
/// provider can be called anything, so a client deciding by name
|
||||
/// would get the answer wrong for anyone who renamed one. It decides
|
||||
/// what the delete confirmation says will happen, so it is not a
|
||||
/// field to guess at.
|
||||
pub keeps_own_transcript: bool,
|
||||
/// How much this session asks before acting. Reported so the phone can
|
||||
/// *show* the current mode rather than assume one -- a picker that
|
||||
/// guesses its own value is how you end up changing something you
|
||||
@@ -300,7 +310,7 @@ impl LiveSession {
|
||||
|
||||
/// `setup_name` is passed in rather than stored: only the manager
|
||||
/// holds the config, and the label can change under a running session.
|
||||
fn info(&self, setup_name: &str, imported: bool) -> SessionInfo {
|
||||
fn info(&self, setup_name: &str, imported: bool, keeps_own_transcript: bool) -> SessionInfo {
|
||||
SessionInfo {
|
||||
id: self.meta.id.clone(),
|
||||
provider: self.meta.provider.clone(),
|
||||
@@ -310,6 +320,7 @@ impl LiveSession {
|
||||
model: self.shared.model.lock().unwrap().clone(),
|
||||
permission_mode: self.shared.permission_mode.lock().unwrap().clone(),
|
||||
imported,
|
||||
keeps_own_transcript,
|
||||
cwd: self.meta.cwd.clone(),
|
||||
status: *self.shared.status.lock().unwrap(),
|
||||
last_activity: *self.shared.last_activity.lock().unwrap(),
|
||||
@@ -581,24 +592,36 @@ impl SessionManager {
|
||||
);
|
||||
}
|
||||
|
||||
/// The session already continuing `source`, if there is one.
|
||||
/// The session already driving `source`, if there is one.
|
||||
///
|
||||
/// Importing the same Claude Code session twice would leave two
|
||||
/// `--resume` processes appending to one transcript, each seeing the
|
||||
/// other's writes as work done elsewhere and replaying them. Nothing
|
||||
/// is corrupted, but both sessions show a conversation neither of them
|
||||
/// is having, which is worse than a refusal.
|
||||
pub fn session_importing(&self, source: &str) -> Option<String> {
|
||||
/// Two `--resume` processes on one transcript each see the other's
|
||||
/// writes as work done elsewhere and replay them, so both sessions
|
||||
/// show a conversation neither is having -- worse than a refusal.
|
||||
///
|
||||
/// Two ways to already be driving one, and only the first used to
|
||||
/// count. An **imported** session records a cursor naming the file it
|
||||
/// follows. A session this app **spawned** has no cursor at all, but
|
||||
/// it has a resume token, which is the CLI's own id for the
|
||||
/// conversation and is exactly the thing being asked about. Matching
|
||||
/// only the cursor left every spawned session looking like somebody
|
||||
/// else's: it appeared in the import list, marked as in use, telling
|
||||
/// the reader to go and close it somewhere -- and the somewhere was
|
||||
/// this app.
|
||||
pub fn session_driving(&self, source: &str) -> Option<String> {
|
||||
let inner = self.inner.read().unwrap();
|
||||
inner.config.sessions.iter().find_map(|meta| {
|
||||
let cursor = import::read_cursor(&self.data_dir.join(&meta.id))?;
|
||||
cursor
|
||||
.path
|
||||
.rsplit('/')
|
||||
.next()?
|
||||
.strip_suffix(".jsonl")
|
||||
.filter(|found| *found == source)
|
||||
.map(|_| meta.id.clone())
|
||||
let dir = self.data_dir.join(&meta.id);
|
||||
let followed = import::read_cursor(&dir).and_then(|cursor| {
|
||||
cursor
|
||||
.path
|
||||
.rsplit('/')
|
||||
.next()
|
||||
.and_then(|name| name.strip_suffix(".jsonl"))
|
||||
.map(str::to_string)
|
||||
});
|
||||
let resuming = claude::read_resume_token(&dir);
|
||||
(followed.as_deref() == Some(source) || resuming.as_deref() == Some(source))
|
||||
.then(|| meta.id.clone())
|
||||
})
|
||||
}
|
||||
|
||||
@@ -614,6 +637,7 @@ impl SessionManager {
|
||||
Some(session) => session.info(
|
||||
label_of(&inner.config, &meta.setup),
|
||||
import::read_cursor(&self.data_dir.join(&meta.id)).is_some(),
|
||||
keeps_own_transcript(&inner.config, &meta.setup, &meta.provider),
|
||||
),
|
||||
None => SessionInfo {
|
||||
id: meta.id.clone(),
|
||||
@@ -624,6 +648,11 @@ impl SessionManager {
|
||||
model: meta.model.clone(),
|
||||
permission_mode: meta.permission_mode.clone(),
|
||||
imported: import::read_cursor(&self.data_dir.join(&meta.id)).is_some(),
|
||||
keeps_own_transcript: keeps_own_transcript(
|
||||
&inner.config,
|
||||
&meta.setup,
|
||||
&meta.provider,
|
||||
),
|
||||
cwd: meta.cwd.clone(),
|
||||
status: status_of_unlaunched(&self.data_dir.join(&meta.id)),
|
||||
last_activity: meta.created,
|
||||
@@ -737,6 +766,7 @@ impl SessionManager {
|
||||
let info = session.info(
|
||||
&setup.name,
|
||||
import::read_cursor(&self.data_dir.join(&id)).is_some(),
|
||||
provider.kind.keeps_own_transcript(),
|
||||
);
|
||||
inner.live.insert(id, session);
|
||||
Ok(info)
|
||||
@@ -904,6 +934,21 @@ fn label_of<'a>(config: &'a Config, id: &'a str) -> &'a str {
|
||||
config.setup(id).map_or(id, |setup| setup.name.as_str())
|
||||
}
|
||||
|
||||
/// Whether this session's provider keeps the conversation somewhere this
|
||||
/// app's delete cannot reach.
|
||||
///
|
||||
/// False when the provider can't be found, which is the safe way round: a
|
||||
/// setup or provider removed from the config leaves sessions naming one
|
||||
/// that is gone, and the warning that then shows is the strong one. Saying
|
||||
/// "this can be brought back" on no evidence is the answer that loses
|
||||
/// somebody's conversation.
|
||||
fn keeps_own_transcript(config: &Config, setup: &str, provider: &str) -> bool {
|
||||
config
|
||||
.setup(setup)
|
||||
.and_then(|setup| setup.providers.iter().find(|it| it.name == provider))
|
||||
.is_some_and(|provider| provider.kind.keeps_own_transcript())
|
||||
}
|
||||
|
||||
/// Names for a failure message: what there is, so the reader can see what
|
||||
/// they meant instead of only that they were wrong.
|
||||
fn names<'a>(all: impl Iterator<Item = &'a str>) -> String {
|
||||
@@ -1307,6 +1352,56 @@ mod tests {
|
||||
.expect("seed config");
|
||||
}
|
||||
|
||||
/// Deleting an echo session ends the conversation; deleting a
|
||||
/// claude-cli one does not, because the CLI keeps its own transcript
|
||||
/// whether this app spawned the session or imported it.
|
||||
///
|
||||
/// The delete confirmation is worded off this, so getting it backwards
|
||||
/// either loses a conversation somebody was told they could recover,
|
||||
/// or cries wolf about one they can.
|
||||
#[test]
|
||||
fn only_a_driver_that_keeps_its_own_record_survives_deletion() {
|
||||
assert!(DriverKind::ClaudeCli.keeps_own_transcript());
|
||||
assert!(!DriverKind::Echo.keeps_own_transcript());
|
||||
assert!(!DriverKind::LlamaCpp.keeps_own_transcript());
|
||||
}
|
||||
|
||||
/// A session this app *spawned* is one it is driving, and used to look
|
||||
/// like somebody else's.
|
||||
///
|
||||
/// Only imported sessions leave an import cursor, so matching on that
|
||||
/// alone missed every spawned session: each one stayed in the import
|
||||
/// list, marked in use, telling the reader to close it wherever it was
|
||||
/// open -- and it was open here. The resume token is the CLI's own id
|
||||
/// for the conversation, which both kinds have.
|
||||
#[tokio::test]
|
||||
async fn a_spawned_session_counts_as_one_we_are_already_driving() {
|
||||
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.clone(),
|
||||
data_dir.clone(),
|
||||
data_dir.join("models"),
|
||||
)
|
||||
.expect("manager");
|
||||
let info = manager.spawn_session(echo_spec()).expect("spawn");
|
||||
|
||||
// No cursor -- nothing was imported -- so this is the case the old
|
||||
// check could not see.
|
||||
assert!(import::read_cursor(&data_dir.join(&info.id)).is_none());
|
||||
assert_eq!(manager.session_driving("5ecf21da-d53f"), None);
|
||||
|
||||
// What a claude session records once the CLI names itself.
|
||||
claude::write_resume_token(&data_dir.join(&info.id), "5ecf21da-d53f");
|
||||
assert_eq!(
|
||||
manager.session_driving("5ecf21da-d53f").as_deref(),
|
||||
Some(info.id.as_str())
|
||||
);
|
||||
assert_eq!(manager.session_driving("some-other-session"), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_session_we_are_not_driving_says_exited_only_when_it_is_gone() {
|
||||
let dir = tempfile::tempdir().expect("tempdir");
|
||||
|
||||
Reference in new issue
Block a user