fix(acp): separate acp sessions from user sessions#7857
fix(acp): separate acp sessions from user sessions#7857DOsinga merged 2 commits intoaaif-goose:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed1f03299b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 640fc0d79a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
rabi
force-pushed
the
acp_seesions
branch
2 times, most recently
from
0b29024 to
c76ca36
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c76ca369e8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
rabi
force-pushed
the
acp_seesions
branch
2 times, most recently
from
8dd0f20 to
f854d37
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f854d37765
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1643fa0ab0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
https://github.com/block/goose/blob/fa594175493db7b5e2f42ba61bb013ba6e76639c/crates/goose/src/session/session_manager.rs#L1280-L1282
After narrowing list_sessions() to user/scheduled here, the search stack still queries every session with no session_type predicate (crates/goose/src/session/chat_history_search.rs:112-154). That leaves two user-visible regressions in ACP-heavy installs: /sessions/search asks for the top limit matches before filtering ACP IDs away (crates/goose-server/src/routes/session.rs:574-589), so the desktop search bar can return too few or zero user sessions even when matching history exists; and if the optional chatrecall extension is enabled, it calls the same search directly (crates/goose/src/agents/platform_extensions/chatrecall.rs:166-177) and will surface ACP transcripts inside normal user chats.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ACP sessions are created as `SessionType::User` and this results in goose CLI/UI sessions also listed in editors. Mixing these session types can cause issues as they run in different environments with different available tools. With `SessionType::Acp` variant. The ACP server now tags sessions appropriately and isolates its queries to only list `Acp` sessions. This also includes a refactor replacing the manual `Display` and `FromStr` with strum implementations removes boilerplate. Change-Id: I78dfa42c64e2d46b1547ec3ef2f4e3b677575479 Signed-off-by: Rabi Mishra <mishra.rabi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef32db41e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fn search_session_types(&self) -> Vec<SessionType> { | ||
| match self.context.session.as_ref().map(|s| s.session_type) { | ||
| Some(SessionType::Acp) => vec![SessionType::Acp], | ||
| _ => vec![SessionType::User, SessionType::Scheduled], |
There was a problem hiding this comment.
chatrecall now scopes searches by the current session type, but delegated tasks are created as SessionType::SubAgent in crates/goose/src/agents/platform_extensions/summon.rs while inheriting the parent session's enabled extensions. In an ACP session that uses delegation, the subagent can still call chatrecall, yet this branch treats it as a normal user session and searches only user/scheduled rows, so ACP history becomes invisible to the delegated task.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this is an edge case and subagent checking parents context to get parent session type would be invasive unless someone thinks this should be done.
Signed-off-by: Douwe Osinga <douwe@squareup.com> # Conflicts: # crates/goose-acp/src/server.rs # ui/desktop/src/api/types.gen.ts
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
Merged main in to resolve the merge conflict with #7834 (the delete-session event-bus cancellation). Build, clippy, and PR tests all pass. The one failing test (test_create_request_enabled_thinking_with_budget) is pre-existing on main, not introduced here.
Signed-off-by: Rabi Mishra <mishra.rabi@gmail.com> Co-authored-by: Douwe Osinga <douwe@squareup.com> Signed-off-by: Cameron Yick <cameron.yick@datadoghq.com>
2 participants
Summary
ACP sessions are created as
SessionType::Userand this results in goose CLI/UI sessions also listed in editors. Mixing these session types can cause issues when loading and resuming sessions as they run in different environments with different available tools (ex. acp read tool) and also tool calls stored in conversation history are different.With
SessionType::Acpvariant. The ACP server now tags sessions appropriately and isolates its queries to only listAcpsessions. It would also filter scheduled sessions.This also includes a refactor replacing the manual
DisplayandFromStrwith strum implementations removing boilerplate.Type of Change
AI Assistance
Testing
Unit tests and tested locally.