hive-subagent-mcp: say so when a resume falls back to the bare name
resume_target logs the fallback and returns a notice naming the project dir searched; continue appends it to its reply and to a failed resume's error, so a caller hitting claude's 'matches 2 sessions' knows to pass dir. The test's final assertion now pins the whole attach list so the continue site is covered on its own, and the fallback branch has a test.
This commit is contained in:
parent
8f2069486d
commit
d03f10822c
2 changed files with 159 additions and 25 deletions
|
|
@ -71,8 +71,10 @@ project directory, read off disk at every resume. claude looks a bare name
|
||||||
up across every git worktree of the repo and refuses when two sessions
|
up across every git worktree of the repo and refuses when two sessions
|
||||||
match, and `start` archives a prior session only in its own directory, so
|
match, and `start` archives a prior session only in its own directory, so
|
||||||
a name reused under a sibling worktree would otherwise stop resuming. When
|
a name reused under a sibling worktree would otherwise stop resuming. When
|
||||||
`dir` holds no session under the name, the name goes to claude as-is and
|
`dir` holds no session under the name, the name goes to claude as-is,
|
||||||
claude's answer stands — see below.
|
the daemon logs it, and `continue`'s reply — or its error, when claude refuses
|
||||||
|
— carries a note naming the directory searched; claude's answer stands, see
|
||||||
|
below.
|
||||||
|
|
||||||
Because the remembered directory goes with the rest of it, a `continue`
|
Because the remembered directory goes with the rest of it, a `continue`
|
||||||
after a restart has to re-supply `dir` when the session lives anywhere
|
after a restart has to re-supply `dir` when the session lives anywhere
|
||||||
|
|
|
||||||
|
|
@ -1120,20 +1120,53 @@ fn build_store(config: &Config) -> std::io::Result<SessionStore> {
|
||||||
))
|
))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// What a resume hands `--resume`, and whether that was a fallback the
|
||||||
|
/// caller should be told about.
|
||||||
|
struct ResumeTarget {
|
||||||
|
/// The `--resume` value.
|
||||||
|
value: String,
|
||||||
|
/// Set when `value` is the bare name because the project dir held no
|
||||||
|
/// session titled it: what the caller needs to know to recover.
|
||||||
|
fallback_notice: Option<String>,
|
||||||
|
}
|
||||||
|
|
||||||
/// What a resume of `name` hands `--resume`: the id of the live session
|
/// What a resume of `name` hands `--resume`: the id of the live session
|
||||||
/// titled `name` in `config`'s own project dir, or `name` itself when that
|
/// titled `name` in `config`'s own project dir, or `name` itself when that
|
||||||
/// dir has none, so a miss is still claude's to report.
|
/// dir has none, so a miss is still claude's to report. The by-name fallback
|
||||||
|
/// is logged and carries a notice naming the dir searched.
|
||||||
///
|
///
|
||||||
/// claude resolves a *title* across the project dirs of every git worktree
|
/// claude resolves a *title* across the project dirs of every git worktree
|
||||||
/// of the repo, and refuses when more than one matches; `start` archives only
|
/// of the repo, and refuses when more than one matches; `start` archives only
|
||||||
/// in its own cwd, so a name reused under a sibling worktree's `dir` stays
|
/// in its own cwd, so a name reused under a sibling worktree's `dir` stays
|
||||||
/// live there. An id names one file.
|
/// live there. An id names one file.
|
||||||
fn resume_target(config: &Config, name: &str) -> String {
|
fn resume_target(config: &Config, name: &str) -> ResumeTarget {
|
||||||
build_store(config)
|
let store = build_store(config).ok();
|
||||||
.ok()
|
let found = store
|
||||||
|
.as_ref()
|
||||||
.and_then(|store| store.find_by_title(name))
|
.and_then(|store| store.find_by_title(name))
|
||||||
.and_then(|path| path.file_stem().map(|id| id.to_string_lossy().into_owned()))
|
.and_then(|path| path.file_stem().map(|id| id.to_string_lossy().into_owned()));
|
||||||
.unwrap_or_else(|| name.to_owned())
|
if let Some(id) = found {
|
||||||
|
return ResumeTarget {
|
||||||
|
value: id,
|
||||||
|
fallback_notice: None,
|
||||||
|
};
|
||||||
|
}
|
||||||
|
let searched = store.map_or_else(
|
||||||
|
|| "a location that could not be resolved".to_owned(),
|
||||||
|
|store| store.project_dir().display().to_string(),
|
||||||
|
);
|
||||||
|
tracing::warn!(
|
||||||
|
name = %name,
|
||||||
|
searched = %searched,
|
||||||
|
"subagent: no session found to resume by id; resuming by name"
|
||||||
|
);
|
||||||
|
ResumeTarget {
|
||||||
|
value: name.to_owned(),
|
||||||
|
fallback_notice: Some(format!(
|
||||||
|
"no session titled `{name}` was found under {searched}, so it was resumed by name — \
|
||||||
|
if it was started in another directory, pass the `dir` it was started in"
|
||||||
|
)),
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Everything a `start` needs, as one struct rather than a parameter list:
|
/// Everything a `start` needs, as one struct rather than a parameter list:
|
||||||
|
|
@ -1388,17 +1421,27 @@ pub async fn continue_(
|
||||||
state.restart_turns(name);
|
state.restart_turns(name);
|
||||||
let (tx, rx) = oneshot::channel();
|
let (tx, rx) = oneshot::channel();
|
||||||
let verdict: VerdictTx = Arc::new(Mutex::new(Some(tx)));
|
let verdict: VerdictTx = Arc::new(Mutex::new(Some(tx)));
|
||||||
let started = continue_reserved(state, name, prompt, model, effort, dir.as_deref(), &verdict);
|
let (started, notice) =
|
||||||
if started.is_err() {
|
match continue_reserved(state, name, prompt, model, effort, dir.as_deref(), &verdict) {
|
||||||
state.release_reservation(name);
|
Ok(spawned) => spawned,
|
||||||
return started;
|
Err(e) => {
|
||||||
}
|
state.release_reservation(name);
|
||||||
|
return Err(e);
|
||||||
|
}
|
||||||
|
};
|
||||||
// Nothing to release on this path either way: a turn that ended inside
|
// Nothing to release on this path either way: a turn that ended inside
|
||||||
// the grace has already been through `finish_turn`, which clears the
|
// the grace has already been through `finish_turn`, which clears the
|
||||||
// tracking the spawn put there, and one that a goal carried into another
|
// tracking the spawn put there, and one that a goal carried into another
|
||||||
// turn still owns the name via `between_turns`.
|
// turn still owns the name via `between_turns`.
|
||||||
await_resume(rx).await?;
|
// The notice rides on the error too: a by-name resume is where claude
|
||||||
started.map(|msg| note_resumed_after_kill(&msg, killed))
|
// answers "matches 2 sessions", and the caller needs to know to pass `dir`.
|
||||||
|
if let Err(e) = await_resume(rx).await {
|
||||||
|
anyhow::bail!("{}", with_resume_notice(&e.to_string(), notice.as_deref()));
|
||||||
|
}
|
||||||
|
Ok(with_resume_notice(
|
||||||
|
¬e_resumed_after_kill(&started, killed),
|
||||||
|
notice.as_deref(),
|
||||||
|
))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Hold a `continue` open until its resume is known to have landed — or
|
/// Hold a `continue` open until its resume is known to have landed — or
|
||||||
|
|
@ -1452,6 +1495,15 @@ fn note_resumed_after_kill(msg: &str, killed: Option<i32>) -> String {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Append the by-name-resume notice (see `ResumeTarget`) to a `continue`'s
|
||||||
|
/// reply or error; pass `msg` through unchanged when there is none.
|
||||||
|
fn with_resume_notice(msg: &str, notice: Option<&str>) -> String {
|
||||||
|
match notice {
|
||||||
|
None => msg.to_owned(),
|
||||||
|
Some(notice) => format!("{msg} — note: {notice}"),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// The slow, fallible part of `continue_`, run only after `reserve` has
|
/// The slow, fallible part of `continue_`, run only after `reserve` has
|
||||||
/// already closed the TOCTOU window — same split rationale as
|
/// already closed the TOCTOU window — same split rationale as
|
||||||
/// `start_reserved`.
|
/// `start_reserved`.
|
||||||
|
|
@ -1463,7 +1515,7 @@ fn continue_reserved(
|
||||||
effort: Option<String>,
|
effort: Option<String>,
|
||||||
dir: Option<&str>,
|
dir: Option<&str>,
|
||||||
verdict: &VerdictTx,
|
verdict: &VerdictTx,
|
||||||
) -> anyhow::Result<String> {
|
) -> anyhow::Result<(String, Option<String>)> {
|
||||||
// A fresh URL for the resumed run, not the one the last run was handed:
|
// A fresh URL for the resumed run, not the one the last run was handed:
|
||||||
// a token is per run, and this is a new one.
|
// a token is per run, and this is a new one.
|
||||||
let signal_url = state.mint_signal_url(name);
|
let signal_url = state.mint_signal_url(name);
|
||||||
|
|
@ -1473,6 +1525,9 @@ fn continue_reserved(
|
||||||
|
|
||||||
/// Resume `name`'s session on the daemon's runtime for one more run.
|
/// Resume `name`'s session on the daemon's runtime for one more run.
|
||||||
///
|
///
|
||||||
|
/// Returns the reply and, when the session could not be found by id, the
|
||||||
|
/// notice that goes with it.
|
||||||
|
///
|
||||||
/// # Errors
|
/// # Errors
|
||||||
///
|
///
|
||||||
/// On ACP, a name with no recorded session: the ACP runtime would start a
|
/// On ACP, a name with no recorded session: the ACP runtime would start a
|
||||||
|
|
@ -1483,7 +1538,7 @@ fn continue_on_runtime(
|
||||||
config: Config,
|
config: Config,
|
||||||
prompt: String,
|
prompt: String,
|
||||||
verdict: &VerdictTx,
|
verdict: &VerdictTx,
|
||||||
) -> anyhow::Result<String> {
|
) -> anyhow::Result<(String, Option<String>)> {
|
||||||
if let SubagentRuntime::Acp { command, sessions } = &state.runtime {
|
if let SubagentRuntime::Acp { command, sessions } = &state.runtime {
|
||||||
let session = acp_session_file(sessions, name);
|
let session = acp_session_file(sessions, name);
|
||||||
if !session.exists() {
|
if !session.exists() {
|
||||||
|
|
@ -1501,20 +1556,23 @@ fn continue_on_runtime(
|
||||||
config,
|
config,
|
||||||
prompt,
|
prompt,
|
||||||
Some(Arc::clone(verdict)),
|
Some(Arc::clone(verdict)),
|
||||||
);
|
)
|
||||||
|
.map(|reply| (reply, None));
|
||||||
}
|
}
|
||||||
// No existence pre-check: claude's own `--resume` is the authority on
|
// No existence pre-check: claude's own `--resume` is the authority on
|
||||||
// whether the session is there, and it errors rather than quietly
|
// whether the session is there, and it errors rather than quietly
|
||||||
// starting a fresh one. `verdict` is how that answer gets back to the
|
// starting a fresh one. `verdict` is how that answer gets back to the
|
||||||
// caller in time to be its error. See this module's doc.
|
// caller in time to be its error. See this module's doc.
|
||||||
spawn_and_track(
|
let target = resume_target(&config, name);
|
||||||
|
let reply = spawn_and_track(
|
||||||
state,
|
state,
|
||||||
name,
|
name,
|
||||||
&config,
|
&config,
|
||||||
&Attach::Resume(resume_target(&config, name)),
|
&Attach::Resume(target.value),
|
||||||
prompt,
|
prompt,
|
||||||
Some(Arc::clone(verdict)),
|
Some(Arc::clone(verdict)),
|
||||||
)
|
)?;
|
||||||
|
Ok((reply, target.fallback_notice))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Bumps `name`'s liveness clock on every line of the turn's output, and
|
/// Bumps `name`'s liveness clock on every line of the turn's output, and
|
||||||
|
|
@ -1656,7 +1714,10 @@ fn spawn_and_track(
|
||||||
let Some(next) = after_turn(&state, &task_name, end, verdict.as_ref()).await else {
|
let Some(next) = after_turn(&state, &task_name, end, verdict.as_ref()).await else {
|
||||||
return;
|
return;
|
||||||
};
|
};
|
||||||
match Claude::spawn(&config, &Attach::Resume(resume_target(&config, &task_name))) {
|
match Claude::spawn(
|
||||||
|
&config,
|
||||||
|
&Attach::Resume(resume_target(&config, &task_name).value),
|
||||||
|
) {
|
||||||
Ok(next_running) => {
|
Ok(next_running) => {
|
||||||
state.track(&task_name, Stopper::Claude(next_running.cancel_handle()));
|
state.track(&task_name, Stopper::Claude(next_running.cancel_handle()));
|
||||||
state.note_event(&task_name);
|
state.note_event(&task_name);
|
||||||
|
|
@ -4378,7 +4439,7 @@ echo '{"type":"result","subtype":"success","is_error":false}'
|
||||||
assert!(state.reserve("n"));
|
assert!(state.reserve("n"));
|
||||||
let (tx, rx) = oneshot::channel();
|
let (tx, rx) = oneshot::channel();
|
||||||
let verdict: VerdictTx = Arc::new(Mutex::new(Some(tx)));
|
let verdict: VerdictTx = Arc::new(Mutex::new(Some(tx)));
|
||||||
continue_on_runtime(&state, "n", config(&here), "again".into(), &verdict)
|
let (_, notice) = continue_on_runtime(&state, "n", config(&here), "again".into(), &verdict)
|
||||||
.expect("the fake claude spawns");
|
.expect("the fake claude spawns");
|
||||||
await_resume(rx)
|
await_resume(rx)
|
||||||
.await
|
.await
|
||||||
|
|
@ -4388,9 +4449,80 @@ echo '{"type":"result","subtype":"success","is_error":false}'
|
||||||
})
|
})
|
||||||
.await;
|
.await;
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
attaches().last().map(String::as_str),
|
attaches(),
|
||||||
Some("resume session-2")
|
[
|
||||||
|
"create session-0",
|
||||||
|
"create session-1",
|
||||||
|
"create session-2",
|
||||||
|
"resume session-2",
|
||||||
|
"resume session-2"
|
||||||
|
],
|
||||||
|
"the continue must add its own resume of the session, not of the title"
|
||||||
);
|
);
|
||||||
|
assert_eq!(notice, None, "an id was found, so nothing was a fallback");
|
||||||
|
std::fs::remove_dir_all(&root).ok();
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn resuming_where_no_session_is_on_disk_falls_back_to_the_name_and_says_so() {
|
||||||
|
// `n` lives only in the sibling worktree; resuming from `here` finds
|
||||||
|
// no id and hands claude the bare name, which claude resolves across
|
||||||
|
// worktrees.
|
||||||
|
let root = scratch_dir("resume-fallback");
|
||||||
|
let (here, sibling) = (root.join("here"), root.join("sibling"));
|
||||||
|
let home = root.join("claude-home");
|
||||||
|
for dir in [&here, &sibling, &home] {
|
||||||
|
std::fs::create_dir_all(dir).expect("scratch subdir");
|
||||||
|
}
|
||||||
|
let program = root.join("session-claude");
|
||||||
|
write_executable(&program, SESSION_CLAUDE);
|
||||||
|
let config = |cwd: &Path| Config {
|
||||||
|
program: Some(program.clone()),
|
||||||
|
cwd: Some(cwd.to_path_buf()),
|
||||||
|
claude_config_dir: Some(home.clone()),
|
||||||
|
..Config::default()
|
||||||
|
};
|
||||||
|
let state = Arc::new(State::new(PathBuf::from("/dev/null"), signal_url()));
|
||||||
|
assert!(state.reserve("n"));
|
||||||
|
state.clear_stop("n");
|
||||||
|
start_on_runtime(&state, "n", config(&sibling), None, "go".into())
|
||||||
|
.expect("the fake claude spawns");
|
||||||
|
until("the run ends", || state.last_event_age("n").is_none()).await;
|
||||||
|
|
||||||
|
let target = resume_target(&config(&here), "n");
|
||||||
|
assert_eq!(target.value, "n", "no session here, so the bare name");
|
||||||
|
let searched = SessionStore::new(&home, &here).project_dir();
|
||||||
|
let notice = target.fallback_notice.expect("a fallback says so");
|
||||||
|
assert!(
|
||||||
|
notice.contains("no session titled `n`")
|
||||||
|
&& notice.contains(&searched.display().to_string())
|
||||||
|
&& notice.contains("resumed by name")
|
||||||
|
&& notice.contains("pass the `dir`"),
|
||||||
|
"{notice}"
|
||||||
|
);
|
||||||
|
|
||||||
|
assert!(state.reserve("n"));
|
||||||
|
let (tx, rx) = oneshot::channel();
|
||||||
|
let verdict: VerdictTx = Arc::new(Mutex::new(Some(tx)));
|
||||||
|
let (_, carried) =
|
||||||
|
continue_on_runtime(&state, "n", config(&here), "again".into(), &verdict)
|
||||||
|
.expect("the fake claude spawns");
|
||||||
|
await_resume(rx).await.expect("one title match resumes");
|
||||||
|
until("the continue's turn ends", || {
|
||||||
|
state.last_event_age("n").is_none()
|
||||||
|
})
|
||||||
|
.await;
|
||||||
|
assert_eq!(
|
||||||
|
lines(&home.join("attaches")),
|
||||||
|
["create session-0", "resume session-0"],
|
||||||
|
"the name was handed to claude, which found the sibling's session"
|
||||||
|
);
|
||||||
|
assert_eq!(carried.as_deref(), Some(notice.as_str()));
|
||||||
|
assert_eq!(
|
||||||
|
with_resume_notice("subagent `n` started", carried.as_deref()),
|
||||||
|
format!("subagent `n` started — note: {notice}")
|
||||||
|
);
|
||||||
|
assert_eq!(with_resume_notice("m", None), "m");
|
||||||
std::fs::remove_dir_all(&root).ok();
|
std::fs::remove_dir_all(&root).ok();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue