tkmind/tkmind_go
1
0
Fork 0
Code Issues Pull Requests Actions 5 Packages Projects Releases Wiki Activity

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

Browse Source
This commit is contained in:
Jack Amadeo
2025-07-02 16:05:05 -04:00
committed by GitHub
parent 2e97621348
commit c77c1b364d
10 changed files with 179 additions and 213 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/goose/src/scheduler.rs
+3 -8
View File
@@ -1248,7 +1248,6 @@ async fn run_scheduled_job_internal(
&session_file_path,
&updated_metadata,
&all_session_messages,
true,
) {
tracing::error!(
"[Job {}] Failed to persist final messages: {}",
@@ -1279,7 +1278,6 @@ async fn run_scheduled_job_internal(
&session_file_path,
&fallback_metadata,
&all_session_messages,
true,
) {
tracing::error!("[Job {}] Failed to persist final messages with fallback metadata: {}", job.id, e_fb);
}
@@ -1306,12 +1304,9 @@ async fn run_scheduled_job_internal(
message_count: 0,
..Default::default()
};
if let Err(e) = crate::session::storage::save_messages_with_metadata(
&session_file_path,
&metadata,
&[],
true,
) {
if let Err(e) =
crate::session::storage::save_messages_with_metadata(&session_file_path, &metadata, &[])
{
tracing::error!(
"[Job {}] Failed to persist metadata for empty job: {}",
job.id,
crates/goose/src/session/storage.rs
+13 -72
View File
@@ -158,14 +158,6 @@ pub fn get_path(id: Identifier) -> Result<PathBuf> {
session_dir.join(format!("{}.jsonl", name))
}
Identifier::Path(path) => {
// Allow special paths for no-session mode
if let Some(path_str) = path.to_str() {
if path_str == "/dev/null" || path_str == "NUL" {
// These are special paths used for --no-session mode
return Ok(path);
}
}
// In test mode, allow temporary directory paths
#[cfg(test)]
{
@@ -199,13 +191,9 @@ pub fn get_path(id: Identifier) -> Result<PathBuf> {
};
// Additional security check for file extension (skip for special no-session paths)
if let Some(path_str) = path.to_str() {
if path_str != "/dev/null" && path_str != "NUL" {
if let Some(ext) = path.extension() {
if ext != "jsonl" {
return Err(anyhow::anyhow!("Invalid file extension"));
}
}
if let Some(ext) = path.extension() {
if ext != "jsonl" {
return Err(anyhow::anyhow!("Invalid file extension"));
}
}
@@ -1054,7 +1042,7 @@ pub async fn persist_messages(
messages: &[Message],
provider: Option<Arc<dyn Provider>>,
) -> Result<()> {
persist_messages_with_schedule_id(session_file, messages, provider, None, true).await
persist_messages_with_schedule_id(session_file, messages, provider, None).await
}
/// Write messages to a session file with metadata, including an optional scheduled job ID
@@ -1071,13 +1059,7 @@ pub async fn persist_messages_with_schedule_id(
messages: &[Message],
provider: Option<Arc<dyn Provider>>,
schedule_id: Option<String>,
save_session: bool,
) -> Result<()> {
if !save_session {
tracing::debug!("Skipping session persistence (save_session=false)");
return Ok(());
}
// Validate the session file path for security
let secure_path = get_path(Identifier::Path(session_file.to_path_buf()))?;
@@ -1097,14 +1079,8 @@ pub async fn persist_messages_with_schedule_id(
match provider {
Some(provider) if user_message_count < 4 => {
//generate_description is responsible for writing the messages
generate_description_with_schedule_id(
&secure_path,
messages,
provider,
schedule_id,
save_session,
)
.await
generate_description_with_schedule_id(&secure_path, messages, provider, schedule_id)
.await
}
_ => {
// Read existing metadata
@@ -1114,7 +1090,7 @@ pub async fn persist_messages_with_schedule_id(
metadata.schedule_id = schedule_id;
}
// Write the file with metadata and messages
save_messages_with_metadata(&secure_path, &metadata, messages, save_session)
save_messages_with_metadata(&secure_path, &metadata, messages)
}
}
}
@@ -1136,13 +1112,7 @@ pub fn save_messages_with_metadata(
session_file: &Path,
metadata: &SessionMetadata,
messages: &[Message],
save_session: bool,
) -> Result<()> {
if !save_session {
tracing::debug!("Skipping session file write (save_session=false)");
return Ok(());
}
use fs2::FileExt;
// Validate the path for security
@@ -1257,7 +1227,7 @@ pub async fn generate_description(
messages: &[Message],
provider: Arc<dyn Provider>,
) -> Result<()> {
generate_description_with_schedule_id(session_file, messages, provider, None, true).await
generate_description_with_schedule_id(session_file, messages, provider, None).await
}
/// Generate a description for the session using the provider, including an optional scheduled job ID
@@ -1274,13 +1244,7 @@ pub async fn generate_description_with_schedule_id(
messages: &[Message],
provider: Arc<dyn Provider>,
schedule_id: Option<String>,
save_session: bool,
) -> Result<()> {
if !save_session {
tracing::debug!("Skipping description generation (save_session=false)");
return Ok(());
}
// Validate the path for security
let secure_path = get_path(Identifier::Path(session_file.to_path_buf()))?;
@@ -1355,7 +1319,7 @@ pub async fn generate_description_with_schedule_id(
}
// Update the file with the new metadata and existing messages
save_messages_with_metadata(&secure_path, &metadata, messages, save_session)
save_messages_with_metadata(&secure_path, &metadata, messages)
}
/// Update only the metadata in a session file, preserving all messages
@@ -1371,7 +1335,7 @@ pub async fn update_metadata(session_file: &Path, metadata: &SessionMetadata) ->
let messages = read_messages(&secure_path)?;
// Rewrite the file with the new metadata and existing messages
save_messages_with_metadata(&secure_path, metadata, &messages, true)
save_messages_with_metadata(&secure_path, metadata, &messages)
}
#[cfg(test)]
@@ -1686,7 +1650,7 @@ mod tests {
let messages = vec![Message::user().with_text("test")];
// Write with special metadata
save_messages_with_metadata(&file_path, &metadata, &messages, true)?;
save_messages_with_metadata(&file_path, &metadata, &messages)?;
// Read back metadata
let read_metadata = read_metadata(&file_path)?;
@@ -1710,7 +1674,7 @@ mod tests {
// Test deserialization of invalid directory
let messages = vec![Message::user().with_text("test")];
save_messages_with_metadata(&file_path, &metadata, &messages, true)?;
save_messages_with_metadata(&file_path, &metadata, &messages)?;
// Modify the file to include invalid directory
let contents = fs::read_to_string(&file_path)?;
@@ -1792,15 +1756,8 @@ mod tests {
let metadata = SessionMetadata::default();
// Test with save_session = false - should not create file
save_messages_with_metadata(&file_path, &metadata, &messages, false)?;
assert!(
!file_path.exists(),
"File should not be created when save_session=false"
);
// Test with save_session = true - should create file
save_messages_with_metadata(&file_path, &metadata, &messages, true)?;
save_messages_with_metadata(&file_path, &metadata, &messages)?;
assert!(
file_path.exists(),
"File should be created when save_session=true"
@@ -1823,28 +1780,12 @@ mod tests {
Message::assistant().with_text("Test response"),
];
// Test persist_messages_with_schedule_id with save_session = false
persist_messages_with_schedule_id(
&file_path,
&messages,
None,
Some("test_schedule".to_string()),
false,
)
.await?;
assert!(
!file_path.exists(),
"File should not be created when save_session=false"
);
// Test persist_messages_with_schedule_id with save_session = true
persist_messages_with_schedule_id(
&file_path,
&messages,
None,
Some("test_schedule".to_string()),
true,
)
.await?;
crates/goose/tests/private_tests.rs
+1 -1
View File
@@ -788,7 +788,7 @@ async fn test_schedule_tool_session_content_action_with_real_session() {
];
// Save the session file
goose::session::storage::save_messages_with_metadata(&session_path, &metadata, &messages, true)
goose::session::storage::save_messages_with_metadata(&session_path, &metadata, &messages)
.unwrap();
// Test the session_content action