feat: Add missing ACP custom methods for prompts, provider_info, plan_prompt, and session/clear#8165
feat: Add missing ACP custom methods for prompts, provider_info, plan_prompt, and session/clear#8165fresh3nough wants to merge 6 commits intoblock:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37f0c4b125
ℹ️ 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".
37f0c4b to
b53253c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b53253cdbe
ℹ️ 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".
…_prompt, and session/clear Add ACP custom method handlers to close the gaps identified in issue block#7225. Routes CLI prompt listing, provider info, plan prompt, and session clear through goosed via ACP custom methods. New ACP methods: - _goose/config/prompts: list extension prompts for a session - _goose/config/prompt_info: get details for a single prompt by name - _goose/agent/provider_info: get provider, model, context limit, tokens - _goose/agent/plan_prompt: get the plan prompt string for a session - _goose/session/clear: clear conversation and reset token counts All new request types include #[serde(rename_all = "camelCase")] for JSON field naming consistency with existing types. Includes 7 integration tests covering all new methods and error cases, using the existing AcpServerConnection, SessionData, and send_custom test infrastructure. Signed-off-by: fre <[email protected]>
b53253c to
901678b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 901678b8bf
ℹ️ 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".
Replace the single cancel_token + prompt_finished pair in GooseAcpSession with a Vec<ActivePrompt>, where each in-flight prompt owns its own CancellationToken and Notify. This fixes two concurrency issues: 1. prompt_finished Notify was replaced each time a new prompt started in get_session_agent. If prompt B started before prompt A exited, A would notify B's Notify instead of its own, leaving session/clear blocked indefinitely on the original Notify. 2. cancel_token was overwritten by each new prompt, so session/clear only cancelled/waited for the latest prompt while older prompt tasks could continue streaming and append messages after clear returned. Now on_cancel, on_close_session, and on_clear_session iterate all active_prompts, cancelling every token and (for clear) awaiting every finished signal. Signed-off-by: fre <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8fe4736b0
ℹ️ 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".
|
Minor question on the API surface: |
|
nope you are totally correct that is very feasible switch, i really chose the route I did for flexibility reasons with dynamic schemas, i will implement the solution you mention its clearly the type safe fix |
…ptInfoResponse Replace serde_json::Value with typed fields: - ListPromptsResponse.prompts: HashMap<String, Vec<rmcp::model::Prompt>> - GetPromptInfoResponse.prompt: rmcp::model::Prompt Remove intermediate serde_json::to_value calls in server.rs handlers. Signed-off-by: fre <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f415e35e61
ℹ️ 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".
…ookup P1: Add a 10s timeout to notify.notified().await in _goose/session/clear so a stalled provider stream cannot block the clear indefinitely. P2: Collect all matching prompts across extensions in prompt_info handler; return an explicit ambiguity error when duplicate names exist instead of non-deterministic HashMap iteration. Signed-off-by: fre <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bedfbe35a0
ℹ️ 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".
Replace sequential per-notify timeout loop with futures::future::join_all wrapped in one tokio::time::timeout, bounding total clear latency to 10s regardless of active prompt count. Signed-off-by: fre <[email protected]>
Co-Authored-By: Oz <[email protected]> Signed-off-by: fre <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35342a871c
ℹ️ 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".
Summary
Adds ACP custom method handlers to close the gaps identified in #7225. This is a focused PR for just the new custom methods (without
goose serve), addressing the feedback from the closed #7720.Changes
New ACP custom methods (server.rs, custom_requests.rs)
_goose/session/clear- clear conversation history and reset token counts_goose/agent/plan_prompt- get the plan prompt string for a session_goose/agent/provider_info- get provider name, model, context limit, and token usage_goose/config/prompt_info- get details for a single prompt by name_goose/config/prompts- list extension prompts for a sessionFeedback from #7720 addressed
goose servesubcommand - scoped to custom methods onlyAcpServerConnection,SessionData, sharedsend_custom#[serde(rename_all = "camelCase")]Tests
7 integration tests in
crates/goose-acp/tests/custom_requests_test.rscovering all new methods including error cases (invalid sessions, nonexistent prompts).Test results: 11 passed (4 existing + 7 new), 0 failed
Verification
cargo test -p goose-acp --test custom_requests_test-- 11/11 passedcargo clippy -p goose-acp --all-targets-- cleancargo fmt-- cleanCloses #7225
Supersedes #7720