Skip to content

feat: Add missing ACP custom methods for prompts, provider_info, plan_prompt, and session/clear#8165

Open
fresh3nough wants to merge 6 commits intoblock:mainfrom
fresh3nough:feat/acp-custom-methods-7225
Open

feat: Add missing ACP custom methods for prompts, provider_info, plan_prompt, and session/clear#8165
fresh3nough wants to merge 6 commits intoblock:mainfrom
fresh3nough:feat/acp-custom-methods-7225

Conversation

@fresh3nough
Copy link
Copy Markdown
Contributor

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 session

Feedback from #7720 addressed

  • No goose serve subcommand - scoped to custom methods only
  • Correct test types - uses existing AcpServerConnection, SessionData, shared send_custom
  • serde consistency - all new request types include #[serde(rename_all = "camelCase")]
  • No race condition - session/clear cancels via CancellationToken and clears immediately, no arbitrary timeouts

Tests

7 integration tests in crates/goose-acp/tests/custom_requests_test.rs covering 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 passed
  • cargo clippy -p goose-acp --all-targets -- clean
  • cargo fmt -- clean

Closes #7225
Supersedes #7720

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

@fresh3nough fresh3nough force-pushed the feat/acp-custom-methods-7225 branch from 37f0c4b to b53253c Compare March 28, 2026 02:20
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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]>
@fresh3nough fresh3nough force-pushed the feat/acp-custom-methods-7225 branch from b53253c to 901678b Compare March 28, 2026 02:41
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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]>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

@DOsinga
Copy link
Copy Markdown
Collaborator

DOsinga commented Mar 30, 2026

Minor question on the API surface: ListPromptsResponse and GetPromptInfoResponse both use serde_json::Value for their payload fields. Since the underlying Prompt type from the MCP SDK already implements Serialize, is there a reason we can't keep the type information around and use a typed field (e.g. prompts: HashMap<String, Vec<Prompt>> / prompt: Prompt) instead of erasing to Value? That would give callers compile-time guarantees about the shape of the response and avoid an extra serialise-then-deserialise round-trip if anyone ever wants to consume these structs from Rust.

@DOsinga DOsinga added the needs_human label to set when a robot looks at a PR and can't handle it label Mar 30, 2026
@fresh3nough
Copy link
Copy Markdown
Contributor Author

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]>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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]>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

fresh3nough and others added 2 commits March 30, 2026 17:19
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]>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs_human label to set when a robot looks at a PR and can't handle it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI-via-goosed: add missing goosed API endpoints and remove remaining self.agent calls

2 participants