Skip to content

Clean up session file optionality for --no-session #3230

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

Merged
ahau-square merged 4 commits into from
Jul 2, 2025
Merged

Clean up session file optionality for --no-session #3230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85d790f
Clean up session files
jamadeo Jul 2, 2025
953abe1
Check for --resume
jamadeo Jul 2, 2025
8238417
Lint fixes
jamadeo Jul 2, 2025
893a2c6
cargo fmt
jamadeo Jul 2, 2025
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
4 changes: 2 additions & 2 deletions crates/goose-bench/src/bench_session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub struct BenchAgentError {
#[async_trait]
pub trait BenchBaseSession: Send + Sync {
async fn headless(&mut self, message: String) -> anyhow::Result<()>;
fn session_file(&self) -> PathBuf;
fn session_file(&self) -> Option<PathBuf>;
fn message_history(&self) -> Vec<Message>;
fn get_total_token_usage(&self) -> anyhow::Result<Option<i32>>;
}
Expand Down Expand Up @@ -52,7 +52,7 @@ impl BenchAgent {
pub(crate) async fn get_token_usage(&self) -> Option<i32> {
self.session.get_total_token_usage().ok().flatten()
}
pub(crate) fn session_file(&self) -> PathBuf {
pub(crate) fn session_file(&self) -> Option<PathBuf> {
self.session.session_file()
}
}
11 changes: 9 additions & 2 deletions crates/goose-bench/src/runners/eval_runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,15 @@ impl EvalRunner {
.canonicalize()
.context("Failed to canonicalize current directory path")?;

BenchmarkWorkDir::deep_copy(agent.session_file().as_path(), here.as_path(), false)
.context("Failed to copy session file to evaluation directory")?;
BenchmarkWorkDir::deep_copy(
agent
.session_file()
.expect("Failed to get session file")
.as_path(),
here.as_path(),
false,
)
.context("Failed to copy session file to evaluation directory")?;

tracing::info!("Evaluation completed successfully");
} else {
Expand Down
18 changes: 15 additions & 3 deletions crates/goose-cli/src/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,11 @@ pub async fn cli() -> Result<()> {
})
.await;
setup_logging(
session.session_file().file_stem().and_then(|s| s.to_str()),
Copy link
Collaborator Author

@jamadeo jamadeo Jul 2, 2025

Choose a reason for hiding this comment

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

This is new behavior -- setup_logging takes an Option<&str>, but here we'd pass it /dev/null which becomes a log file like 20250521_154141-null.log.

After this change, we'll pass None, which just becomes 20250521_154141.log

session
.session_file()
.as_ref()
.and_then(|p| p.file_stem())
.and_then(|s| s.to_str()),
None,
)?;

Expand Down Expand Up @@ -831,7 +835,11 @@ pub async fn cli() -> Result<()> {
.await;

setup_logging(
session.session_file().file_stem().and_then(|s| s.to_str()),
session
.session_file()
.as_ref()
.and_then(|p| p.file_stem())
.and_then(|s| s.to_str()),
None,
)?;

Expand Down Expand Up @@ -950,7 +958,11 @@ pub async fn cli() -> Result<()> {
})
.await;
setup_logging(
session.session_file().file_stem().and_then(|s| s.to_str()),
session
.session_file()
.as_ref()
.and_then(|p| p.file_stem())
.and_then(|s| s.to_str()),
None,
)?;
if let Err(e) = session.interactive(None).await {
Expand Down
2 changes: 1 addition & 1 deletion crates/goose-cli/src/commands/bench.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ impl BenchBaseSession for Session {
async fn headless(&mut self, message: String) -> anyhow::Result<()> {
self.headless(message).await
}
fn session_file(&self) -> PathBuf {
fn session_file(&self) -> Option<PathBuf> {
self.session_file()
}
fn message_history(&self) -> Vec<Message> {
Expand Down
69 changes: 31 additions & 38 deletions crates/goose-cli/src/session/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ async fn offer_extension_debugging_help(
std::env::temp_dir().join(format!("goose_debug_extension_{}.jsonl", extension_name));

// Create the debugging session
let mut debug_session = Session::new(debug_agent, temp_session_file.clone(), false, None, true);
let mut debug_session = Session::new(debug_agent, Some(temp_session_file.clone()), false, None);

// Process the debugging request
println!("{}", style("Analyzing the extension failure...").yellow());
Expand Down Expand Up @@ -229,24 +229,16 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> Session {
}

// Handle session file resolution and resuming
let session_file: std::path::PathBuf = if session_config.no_session {
// Use a temporary path that won't be written to
#[cfg(unix)]
{
std::path::PathBuf::from("/dev/null")
}
#[cfg(windows)]
{
std::path::PathBuf::from("NUL")
}
let session_file: Option<std::path::PathBuf> = if session_config.no_session {
None
} else if session_config.resume {
if let Some(identifier) = session_config.identifier {
let session_file = match session::get_path(identifier) {
Ok(path) => path,
Err(e) => {
output::render_error(&format!("Invalid session identifier: {}", e));
process::exit(1);
}
Ok(path) => path,
};
if !session_file.exists() {
output::render_error(&format!(
Expand All @@ -256,11 +248,11 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> Session {
process::exit(1);
}

session_file
Some(session_file)
} else {
// Try to resume most recent session
match session::get_most_recent_session() {
Ok(file) => file,
Ok(file) => Some(file),
Err(_) => {
output::render_error("Cannot resume - no previous sessions found");
process::exit(1);
Expand All @@ -276,40 +268,42 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> Session {

// Just get the path - file will be created when needed
match session::get_path(id) {
Ok(path) => path,
Ok(path) => Some(path),
Err(e) => {
output::render_error(&format!("Failed to create session path: {}", e));
process::exit(1);
}
}
};

if session_config.resume && !session_config.no_session {
// Read the session metadata
let metadata = session::read_metadata(&session_file).unwrap_or_else(|e| {
output::render_error(&format!("Failed to read session metadata: {}", e));
process::exit(1);
});
if session_config.resume {
if let Some(session_file) = session_file.as_ref() {
// Read the session metadata
let metadata = session::read_metadata(session_file).unwrap_or_else(|e| {
output::render_error(&format!("Failed to read session metadata: {}", e));
process::exit(1);
});

let current_workdir =
std::env::current_dir().expect("Failed to get current working directory");
if current_workdir != metadata.working_dir {
// Ask user if they want to change the working directory
let change_workdir = cliclack::confirm(format!("{} The original working directory of this session was set to {}. Your current directory is {}. Do you want to switch back to the original working directory?", style("WARNING:").yellow(), style(metadata.working_dir.display()).cyan(), style(current_workdir.display()).cyan()))
let current_workdir =
std::env::current_dir().expect("Failed to get current working directory");
if current_workdir != metadata.working_dir {
// Ask user if they want to change the working directory
let change_workdir = cliclack::confirm(format!("{} The original working directory of this session was set to {}. Your current directory is {}. Do you want to switch back to the original working directory?", style("WARNING:").yellow(), style(metadata.working_dir.display()).cyan(), style(current_workdir.display()).cyan()))
.initial_value(true)
.interact().expect("Failed to get user input");

if change_workdir {
if !metadata.working_dir.exists() {
output::render_error(&format!(
"Cannot switch to original working directory - {} no longer exists",
style(metadata.working_dir.display()).cyan()
));
} else if let Err(e) = std::env::set_current_dir(&metadata.working_dir) {
output::render_error(&format!(
"Failed to switch to original working directory: {}",
e
));
if change_workdir {
if !metadata.working_dir.exists() {
output::render_error(&format!(
"Cannot switch to original working directory - {} no longer exists",
style(metadata.working_dir.display()).cyan()
));
} else if let Err(e) = std::env::set_current_dir(&metadata.working_dir) {
output::render_error(&format!(
"Failed to switch to original working directory: {}",
e
));
}
}
}
}
Expand Down Expand Up @@ -373,7 +367,6 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> Session {
session_file.clone(),
session_config.debug,
session_config.scheduled_job_id.clone(),
!session_config.no_session, // save_session is the inverse of no_session
);

// Add extensions if provided
Expand Down
Loading