Sessions required #5548

DOsinga · 2025-11-03T17:26:04Z

Summary

Makes session ids required, hides sessions of non user type and sends only the user message to the end point

also gets rid of tests that are pointless or don't actually succeed

Copilot

Pull Request Overview

This PR refactors the session management system to introduce a SessionType enum and simplifies the agent's reply interface. The key changes include:

Added SessionType enum with variants: User, Scheduled, SubAgent, and Hidden to categorize different session types
Refactored Agent::reply() interface to accept a single Message instead of a Conversation, with the conversation now loaded from session storage internally
Removed execution_mode field from ScheduledJob and SessionConfig structures, simplifying the scheduled job configuration
Removed working_dir field from SessionConfig as the working directory is now retrieved from the Session object
Updated database schema to version 5, adding a session_type column with an index for filtering sessions by type

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
crates/goose/src/session/session_manager.rs	Adds `SessionType` enum, updates schema to v5, adds `session_type` column to sessions table with filtering in `list_sessions`
crates/goose/src/agents/agent.rs	Refactors `reply()` to accept `Message` instead of `Conversation`, removes `execution_mode` logic, simplifies session handling
crates/goose/src/agents/types.rs	Removes `execution_mode` and `working_dir` from `SessionConfig`
crates/goose/src/scheduler.rs	Removes `execution_mode` field from `ScheduledJob`, updates job creation to use `SessionType::Scheduled`
crates/goose/tests/test_support.rs	Removes `execution_mode` from test job creation, adds `session_type` to test session metadata
crates/goose/tests/private_tests.rs	Entire test file deleted (833 lines removed)
crates/goose/tests/agent.rs	Consolidates tests, removes provider tests, simplifies schedule tool tests using new session API
crates/goose-cli/src/session/mod.rs	Changes `session_id` from `Option<String>` to `String`, removes all optional session handling
crates/goose-server/src/routes/reply.rs	Updates reply call to use single message instead of conversation
Multiple CLI/server files	Updates `SessionManager::create_session()` calls to include `SessionType` parameter

Comments suppressed due to low confidence (1)

crates/goose/src/agents/agent.rs:1

Commented-out code should be removed rather than left in the codebase. If this is intentionally kept for reference during migration, add a TODO comment explaining why.

use std::collections::HashMap;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-03T17:28:40Z

crates/goose/src/agents/subagent_task_config.rs

        provider: Arc<dyn Provider>,
-        parent_session_id: Option<String>,
-        parent_working_dir: PathBuf,
+        parent_session_id: &str,


The parent_session_id parameter changed from Option<String> to &str, which makes it required. However, the change from owned Option<String> to borrowed &str means callers can no longer pass None. Consider whether there are legitimate cases where a subagent might not have a parent session, and if so, this should remain Option<&str> or document why it's always required.

Copilot · 2025-11-03T17:28:40Z

crates/goose/src/agents/agent.rs

+        SessionManager::add_message(&session_config.id, &user_message).await?;
+        let session = SessionManager::get_session(&session_config.id, true).await?;

-            let needs_auto_compact = crate::context_mgmt::check_if_compaction_needed(
-                self,
-                &unfixed_conversation,
-                None,
-                session_metadata.as_ref(),
-            )
-            .await?;
+        let conversation = session
+            .conversation
+            .clone()
+            .ok_or_else(|| anyhow::anyhow!("Session {} has no conversation", session_config.id))?;


The reply() method now fetches the entire session (including conversation) after adding a message, which involves two database operations. This could be optimized by having SessionManager::add_message() return the updated session, or by maintaining the conversation in memory and only persisting messages incrementally.

Copilot · 2025-11-03T17:28:40Z

crates/goose-cli/src/commands/acp.rs

+        let session = SessionManager::create_session(
+            std::env::current_dir().unwrap_or_default(),
+            "ACP Session".to_string(),
+            SessionType::Hidden,
+        )
+        .await?;


A new session is created for every prompt in the ACP handler, but the session is never cleaned up or reused. This will create a new database entry for each prompt, potentially leading to database bloat. Consider reusing the session for the lifetime of the ACP session or cleaning up Hidden sessions periodically.

Copilot · 2025-11-03T17:28:40Z

crates/goose-cli/src/session/builder.rs

        }
    } else {
-        session_config.session_id
+        session_config.session_id.unwrap()


Calling unwrap() on session_id will panic if it's None. The logic should either guarantee that session_id is always Some at this point, or handle the None case explicitly. Consider using expect() with a descriptive message or restructuring the code to ensure the value is always present.

Suggested change

session_config.session_id.unwrap()

match session_config.session_id {

Some(session_id) => session_id,

None => {

output::render_error("No session ID provided and not resuming or creating a new session.");

process::exit(1);

}

}

katzdave · 2025-11-03T17:49:47Z

crates/goose/src/agents/agent.rs

        &self,
-        unfixed_conversation: Conversation,
-        session: Option<SessionConfig>,
+        user_message: Message,


katzdave · 2025-11-03T17:53:04Z

crates/goose/src/agents/types.rs

    pub id: String,
    /// Working directory for the session
-    pub working_dir: PathBuf,
+    //pub working_dir: PathBuf,


is this supposed to be deleted?

yes. let me delete it for real. the session config has a working directory and a session id which maps to a session which also has a working dir. they were the same in practice, but we should only have one source of truth

katzdave · 2025-11-03T17:59:29Z

crates/goose-server/src/routes/reply.rs

            retry_config: None,
        };

+        let user_message = match messages.last() {


Seems like we're still passing in all messages from the client + building/modifying all_messages below. Is that intentional?

we're not really

we're still sending the entire conversation over, but only pick up the last message

we keep track of all_messages here separately but only for telemetry. this is somewhat misguided, I think it is measuring the wrong thing and it should happen in agent.reply, not here, but this PR is big enough as is

Copilot

Pull Request Overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (2)

crates/goose/tests/private_tests.rs:1

The entire private_tests.rs file (833 lines) has been deleted. These tests covered schedule management functionality including list, create, run_now, pause, unpause, delete, kill, inspect, and sessions actions. Verify that equivalent test coverage exists elsewhere, or these features lack testing. If moving to agent.rs was intentional, ensure all test scenarios are preserved.
crates/goose/src/agents/subagent_handler.rs:1
The TaskConfig.parent_session_id changed from Option to String, requiring a parent session. This is a breaking change that prevents standalone subagent tasks. Verify this is intentional and all callers can provide a valid parent session ID.

use crate::session::session_manager::SessionType;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-03T21:55:44Z

crates/goose/src/session/session_manager.rs

               COUNT(m.id) as message_count
        FROM sessions s
        INNER JOIN messages m ON s.id = m.session_id
+        WHERE s.session_type = 'user' OR s.session_type = 'scheduled'


[nitpick] The hardcoded filter excludes SubAgent and Hidden session types from list_sessions(). Consider making this configurable via a parameter to allow listing all sessions when needed for debugging or administrative purposes.

Copilot · 2025-11-03T21:55:45Z

crates/goose-server/src/routes/reply.rs

+        let user_message = match messages.last() {
+            Some(msg) => msg,
+            _ => {
+                let _ = stream_event(
+                    MessageEvent::Error {
+                        error: "Reply started with empty messages".to_string(),
+                    },
+                    &task_tx,
+                    &task_cancel,
+                )
+                .await;
+                return;
+            }
+        };


Extracting the last message assumes it's the user's new message, but the session already contains the full conversation history. If the client passes the entire conversation including the new message, this works. However, if messages is empty or contains only system messages, this will fail silently. The logic should validate that the extracted message is actually a new user message and not already in the session.

Copilot · 2025-11-03T21:55:45Z

crates/goose/src/agents/agent.rs

+        SessionManager::add_message(&session_config.id, &user_message).await?;
+        let session = SessionManager::get_session(&session_config.id, true).await?;


[nitpick] The user_message is added to the session and then immediately the full session (with conversation) is loaded. This results in two database operations where the conversation could potentially be assembled more efficiently. Consider if the message can be appended to an in-memory conversation or if this pattern creates unnecessary round-trips.

Suggested change

SessionManager::add_message(&session_config.id, &user_message).await?;

let session = SessionManager::get_session(&session_config.id, true).await?;

// Fetch the session once

let mut session = SessionManager::get_session(&session_config.id, true).await?;

// Add the user message to the session's conversation in-memory

if let Some(conversation) = &mut session.conversation {

conversation.push(user_message.clone());

} else {

return Err(anyhow::anyhow!("Session {} has no conversation", session_config.id));

}

// Persist the message if needed

SessionManager::add_message(&session_config.id, &user_message).await?;

Copilot · 2025-11-03T21:55:46Z

crates/goose/src/agents/types.rs

 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct SessionConfig {
-    /// Unique identifier for the session
+    /// Identifier of the underlying Session


[nitpick] The comment update from 'Unique identifier for the session' to 'Identifier of the underlying Session' loses clarity. The original comment was clearer about the field's purpose. Consider reverting to the original or improving further.

Suggested change

/// Identifier of the underlying Session

/// Unique identifier for the session

* 'main' of github.com:block/goose: Sessions required (#5548) feat: add grouped extension loading notification (#5529) we should run this on main and also test open models at least via ope… (#5556) info: print location of sessions.db via goose info (#5557) chore: remove yarn usage from documentation (#5555) cli: adjust default theme to address #1905 (#5552)

* main: (85 commits) improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) fix: improve server error messages to include HTTP status code (#5532) improvement: add useful error message when attempting to use unauthenticated cursor-agent (#5300) fix: unblock acp via databricks (#5562) feat: add --output-format json flag to goose run command (#5525) Sessions required (#5548) feat: add grouped extension loading notification (#5529) we should run this on main and also test open models at least via ope… (#5556) info: print location of sessions.db via goose info (#5557) chore: remove yarn usage from documentation (#5555) cli: adjust default theme to address #1905 (#5552) Manual compaction counting fix + cli cleanup (#5480) chore(deps): bump prismjs and react-syntax-highlighter in /ui/desktop (#5549) fix: remove qwen3-coder from provider/mcp smoke tests (#5551) fix: do not build unsigned desktop app bundles on every PR in ci. add manual option. (#5550) fix: update Husky prepare script to v9 format (#5522) ...

* main: (54 commits) add clippy warning for string_slice (#5422) improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) fix: improve server error messages to include HTTP status code (#5532) improvement: add useful error message when attempting to use unauthenticated cursor-agent (#5300) fix: unblock acp via databricks (#5562) feat: add --output-format json flag to goose run command (#5525) Sessions required (#5548) feat: add grouped extension loading notification (#5529) we should run this on main and also test open models at least via ope… (#5556) info: print location of sessions.db via goose info (#5557) chore: remove yarn usage from documentation (#5555) cli: adjust default theme to address #1905 (#5552) Manual compaction counting fix + cli cleanup (#5480) chore(deps): bump prismjs and react-syntax-highlighter in /ui/desktop (#5549) fix: remove qwen3-coder from provider/mcp smoke tests (#5551) fix: do not build unsigned desktop app bundles on every PR in ci. add manual option. (#5550) ...

Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: fbalicchia <[email protected]>

Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: Blair Allan <[email protected]>

Douwe Osinga added 6 commits October 30, 2025 16:58

Start making it required

06781cc

Check

e23dcf1

WIP 2

d2a318a

Merge branch 'main' into sessions-required

f6b0619

Less tests

abec712

Delete some more

820e500

Copilot AI review requested due to automatic review settings November 3, 2025 17:26

Copilot AI reviewed Nov 3, 2025

View reviewed changes

katzdave reviewed Nov 3, 2025

View reviewed changes

Delete it

1db1695

katzdave approved these changes Nov 3, 2025

View reviewed changes

Merge branch 'main' into sessions-required

239231d

Copilot AI review requested due to automatic review settings November 3, 2025 21:53

Copilot AI reviewed Nov 3, 2025

View reviewed changes

Dumb

898f0be

DOsinga merged commit 5b93ee5 into main Nov 4, 2025
13 of 14 checks passed

github-actions bot mentioned this pull request Nov 5, 2025

chore(release): release version 1.13.0 (minor) #5582

Merged

fbalicchia pushed a commit to fbalicchia/goose that referenced this pull request Nov 7, 2025

Sessions required (block#5548)

b316afa

Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: fbalicchia <[email protected]>

BlairAllan pushed a commit to BlairAllan/goose that referenced this pull request Nov 29, 2025

Sessions required (block#5548)

380bb9e

Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: Blair Allan <[email protected]>

codefromthecrypt mentioned this pull request Dec 21, 2025

test: fix test_max_turns_limit slow execution and wrong message type #6221

Merged

3 tasks

-        session_config.session_id.unwrap()
+        match session_config.session_id {
+            Some(session_id) => session_id,
+            None => {
+                output::render_error("No session ID provided and not resuming or creating a new session.");
+                process::exit(1);
+            }
+        }

		SessionManager::add_message(&session_config.id, &user_message).await?;
		let session = SessionManager::get_session(&session_config.id, true).await?;

-        SessionManager::add_message(&session_config.id, &user_message).await?;
-        let session = SessionManager::get_session(&session_config.id, true).await?;
+        // Fetch the session once
+        let mut session = SessionManager::get_session(&session_config.id, true).await?;
+        // Add the user message to the session's conversation in-memory
+        if let Some(conversation) = &mut session.conversation {
+            conversation.push(user_message.clone());
+        } else {
+            return Err(anyhow::anyhow!("Session {} has no conversation", session_config.id));
+        }
+        // Persist the message if needed
+        SessionManager::add_message(&session_config.id, &user_message).await?;

	/// Identifier of the underlying Session
	/// Unique identifier for the session

Sessions required #5548

Sessions required #5548

Uh oh!

Conversation

DOsinga commented Nov 3, 2025

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

katzdave Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

katzdave Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

DOsinga Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

katzdave Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

DOsinga Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants