-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(acp): per-session Agent for model isolation and load_session restore #7115
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.
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
Conversation
Each session/new and session/load_session now creates its own Agent with its own provider, fixing model changes leaking across sessions. load_session restores the persisted model from the database. Clarifies transport layer naming: Acp-Session-Id is a connection-scoped UUID, distinct from Goose session IDs. Test fixtures split into Connection and Session traits matching the protocol separation. Signed-off-by: Adrian Cole <adrian@tetrate.io>
| assert_eq!(models, expected); | ||
| } | ||
|
|
||
| pub async fn run_model_set<C: Connection>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the session independence test. if we later refactor goose to be more session independent internally, we can try to pass this test again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes ACP session isolation by ensuring each Goose session created/loaded via ACP gets its own Agent (and thus its own provider/model state), and updates tests/fixtures and transport naming to reflect the distinction between transport connections and Goose sessions.
Changes:
- Create a per-session
Agentinsession/newandsession/load_session, and restore the model from persisted session data on load. - Clarify transport semantics by treating
Acp-Session-Idas a connection-scoped UUID (separate from Goose session IDs). - Refactor ACP test fixtures around
ConnectionvsSession, and add tests for model isolation and model restore on load.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/goose-acp/src/server.rs | Moves to per-session Agent creation; updates new/load session flows and model switching. |
| crates/goose-acp/src/transport/http.rs | Uses connection-scoped UUID for Acp-Session-Id mapping to transport sessions. |
| crates/goose-acp/src/transport/websocket.rs | Same as HTTP: connection-scoped UUID and improved naming/log fields. |
| crates/goose-acp/tests/server_test.rs | Updates tests to use ClientToAgentConnection and adds model restore/isolation tests. |
| crates/goose-acp/tests/common_tests/mod.rs | Refactors shared test logic to use Connection/Session and adds run_model_set/run_load_model. |
| crates/goose-acp/tests/fixtures/mod.rs | Splits TestSessionConfig into TestConnectionConfig and introduces Connection trait. |
| crates/goose-acp/tests/fixtures/server.rs | Implements new Connection + Session fixtures and adds load_session/new_session helpers. |
|
Great feature i was just looking for this! Just checked out looking forward for the upstream merge!! |
Signed-off-by: Adrian Cole <adrian@tetrate.io>
|
|
||
| pub(crate) struct HttpState { | ||
| server: Arc<AcpServer>, | ||
| // Keyed by acp_session_id: a connection-scoped UUID serving many Goose sessions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confused so 1 acp session != goose session?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is a mechanic of the transport layer which is the same as MCP basically. when you create a connection to the server, you then have json-rpc on top. on-top are the session/new, fork, load all would be different app layer sessions on the same underlying connection. make sense?
|
|
||
| let models = session.models().unwrap(); | ||
| let expected = SessionModelState::new( | ||
| ModelId::new(TEST_MODEL), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't care about these now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I re-ordered the tests alphabetically so that the clusters of features are near eachother in the same file. this wasn't dropped.
| .unwrap(), | ||
| ); | ||
|
|
||
| // Initialization shouldn't fail even though we have a crashing provider factory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah sorry I should have made a review comment. this test is now pulled up as a common test, since the fixture is refactored to separate init from new session. https://github.com/block/goose/blob/main/crates/goose-acp/tests/common_tests/mod.rs#L62
michaelneale
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
felt like more than 70loc change! seems ok - a few questions were inline but mostly my unfamiliarity with session mapping from acp
* origin/main: (107 commits) feat: Allow overriding default bat themes using environment variables (#7140) Make the system prompt smaller (#6991) Pre release script (#7145) Spelling (#7137) feat(mcp): upgrade rmcp to 0.15.0 and advertise MCP Apps UI extension capability (#6927) fix: ensure assistant messages with tool_calls include content field (#7076) fix(canonical): handle gcp_vertex_ai model mapping correctly (#6836) Group dependencies in root Cargo.toml (#6948) refactor: updated elevenLabs API module and `remove button` UX (#6781) fix: we were missing content from langfuse traces (#7135) docs: update username in authors.yml (#7132) fix extension selector syncing issues (#7133) fix(acp): per-session Agent for model isolation and load_session restore (#7115) fix(claude-code): defensive coding improvements for model switching (#7131) feat(claude-code): dynamic model listing and mid-session model switching (#7120) Inline worklet source (#7128) [docs] One shot prompting is dead - Blog Post (#7113) fix: correct spelling of Debbie O'Brien's name in authors.yml (#7127) docs: GCP Vertex AI org policy filtering & update OnboardingProviderSetup component (#7125) feat: replace subagent and skills with unified summon extension (#6964) ... # Conflicts: # Cargo.lock # Cargo.toml
* upstream/main: (109 commits) [docs] Skills Marketplace UI Improvements (block#7158) More no-window flags (block#7122) feat: Allow overriding default bat themes using environment variables (block#7140) Make the system prompt smaller (block#6991) Pre release script (block#7145) Spelling (block#7137) feat(mcp): upgrade rmcp to 0.15.0 and advertise MCP Apps UI extension capability (block#6927) fix: ensure assistant messages with tool_calls include content field (block#7076) fix(canonical): handle gcp_vertex_ai model mapping correctly (block#6836) Group dependencies in root Cargo.toml (block#6948) refactor: updated elevenLabs API module and `remove button` UX (block#6781) fix: we were missing content from langfuse traces (block#7135) docs: update username in authors.yml (block#7132) fix extension selector syncing issues (block#7133) fix(acp): per-session Agent for model isolation and load_session restore (block#7115) fix(claude-code): defensive coding improvements for model switching (block#7131) feat(claude-code): dynamic model listing and mid-session model switching (block#7120) Inline worklet source (block#7128) [docs] One shot prompting is dead - Blog Post (block#7113) fix: correct spelling of Debbie O'Brien's name in authors.yml (block#7127) ...
…ore (block#7115) Signed-off-by: Adrian Cole <adrian@tetrate.io>
…ore (block#7115) Signed-off-by: Adrian Cole <adrian@tetrate.io>
…ore (block#7115) Signed-off-by: Adrian Cole <adrian@tetrate.io>
Summary
Fix session isolation in the ACP server (
goose acp). Previously, all sessions shared a singleAgent, sosession/set_modelon one session changed the model for every session, andsession/load_sessiondidn't restore the persisted model.Now each
session/newandsession/load_sessioncreates its ownAgentwith its own provider. Model changes are scoped to the session that requested them.load_sessionrestores the model from the database.The transport layer is clarified: the
Acp-Session-Idheader is a connection-scoped UUID (renamed fromsession_idtoacp_session_id), distinct from the Goose session IDs created bysession/new.Test fixtures are refactored to separate
Connection(transport + initialize) fromSession(prompt, set_model), matching the protocol's own separation.Type of Change
AI Assistance
Testing
test_model_set— creates two sessions on the same connection, sets a different model on each, prompts both, and verifies each uses its own model (proves session independence)test_load_model— sets model too4-mini, prompts, then loads the same session and verifies the model state is restored from the databaseRelated Issues
Follow-up to #7112 (model selection support). That PR added
session/set_modelbut all sessions shared one agent, so the model change leaked across sessions.