Skip to content

Tell the user the resume route that actually works (#6225) #6233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Hmbown wants to merge 1 commit into
base: main
Choose a base branch
from
+28 −2
Open

Tell the user the resume route that actually works (#6225) #6233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion crates/tui/src/tui/ui/apply.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3626,7 +3626,22 @@ pub(crate) fn apply_loaded_session_with_goal(
recovered_binding = tasks.session_store_binding();
}
if recovered_binding.is_none() {
return Err("This session belongs to another Runtime host. Resume it in a new Codewhale process to reopen its saved store.".into());
// Name the real condition and the path that actually works. The
// old wording ("resume it in a new Codewhale process") sent users
// in circles: starting a new process and then picking the session
// from `/resume` lands here again, because that is this same
// switch path. Opening the session *at launch* is a different
// route — `TaskManager::start` passes the saved binding through to

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] Comment duplicates PR narrative and references internal symbols that can drift

The 9-line comment (apply.rs:3629-3637) restates the PR description and hardcodes internal names (TaskManager::start, open_for_session, runtime_threads.rs, validate_existing_store, open_inner) plus issue numbers. These are not checked by the compiler, so they will silently go stale as the call path is refactored. A shorter comment describing why the picker differs from launch would carry the intent without the drift risk.

// `open_for_session`, which validates the existing store and
// adopts it (runtime_threads.rs, `validate_existing_store` then
// `open_inner`). So the advice has to say which one (#6207, #6225).
return Err(format!(
"This session's saved Runtime store belongs to a different host. \

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] New wording may still misdiagnose the #6225 case

The message asserts the store "belongs to a different host." The PR description states the underlying refusal (#6207) is still wrong for the case #6225 actually reports — a store that exists, is owned by nobody, and holds nothing. For that user the new message attributes a cause (foreign host) that may not be true, even though the actionable advice is now correct. Consider softening the causal clause (e.g. describing the observable condition rather than the inferred owner) so the message is accurate for both cases.

Switching to it from inside a running session cannot carry that \
store's queued work across, but opening it directly can: run \
`codewhale resume {}` from your shell.",

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] Message hardcodes an unverified CLI invocation; no test proves it works

The new message tells users to run codewhale resume {id}, but nothing here validates that this exact command/binary/flags form is real or that passing the session id (rather than a store id/path) actually adopts the store. session.metadata.id is the session id, and the message is about reopening a saved Runtime store; if the CLI resume subcommand expects a different identifier or invocation, users are sent to a second dead end — the exact failure mode this PR is fixing. Because the test only asserts the substring "codewhale resume", a divergence between the message and the real CLI (wrong flag, wrong binary name, id rejected) would pass CI silently.

session.metadata.id
));
}
}
if app.session_transition_blocked() {
Expand Down
13 changes: 12 additions & 1 deletion crates/tui/src/tui/ui/tests/runtime_store_binding.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,18 @@ async fn runtime_store_binding_survives_launch_snapshot_and_resume() -> anyhow::
let old_id = other_app.current_session_id.clone();
let error = apply_loaded_session_with_goal(&mut other_app, &mut resumed_config, &loaded, None)
.unwrap_err();
assert!(error.contains("Resume it in a new Codewhale process"));
// The refusal must name the route that actually works. "Resume it in a new
// Codewhale process" was true but unactionable: starting a new process and
// then picking the session from `/resume` returns here, because that is
// this same switch path (#6207, #6225).
assert!(
error.contains("codewhale resume"),

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] Core user-facing claim is untested; only the string is tested

The value of this change is the claim that opening the session at launch adopts the store. The updated test only asserts that the error string contains "codewhale resume" and the session id (runtime_store_binding.rs:288-295); it does not exercise TaskManager::start -> open_for_session/validate_existing_store to prove adoption succeeds, nor does it pin the full command shape (codewhale resume <id>). A regression that changes the CLI syntax, or that breaks the direct-open path the message now advertises, would leave the message confidently wrong with all tests green.

"the refusal must point at the direct-open path: {error}"
);
assert!(
error.contains(&loaded.metadata.id),
"the refusal must name the session to open: {error}"
);

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep these substring assertions, but treat them as a message-shape check rather than proof of the advertised route. Add a separate test that actually drives the launch-based adoption path (start the runtime with the saved binding already present, as TaskManager::start does, and assert the store is opened/adopted and no switch error is produced). That directly validates the promise the new message makes, which the current string-only assertions cannot. If a launch-route test is expensive here, at minimum add a CLI/integration test that codewhale resume <id> (with the exact command text in this message) succeeds against the foreign store. This is a test-infrastructure change, so no literal replacement is provided.

assert_eq!(other_app.current_session_id, old_id);
assert_eq!(other_app.input, "preserve pending input");
foreign.shutdown_and_wait().await?;
Expand Down
Loading