feat: Add missing ACP custom methods and serve subcommand for three-binary consolidation#7720
feat: Add missing ACP custom methods and serve subcommand for three-binary consolidation#7720fresh3nough wants to merge 7 commits intoblock:mainfrom
Conversation
…inary consolidation Add ACP custom method handlers for prompts, provider_info, plan_prompt, and session/clear to close the gaps identified in issue block#7225. Add a goose serve subcommand to start the ACP server over HTTP/WebSocket, enabling binary consolidation (goose, goosed, goose-acp-server -> single binary). 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 New CLI subcommand: - goose serve [--host] [--port] [--with-builtin]: starts ACP server over HTTP/WS Includes 7 integration tests covering all new methods and error cases. Signed-off-by: fresh3nough <[email protected]> Signed-off-by: fre <[email protected]>
465cede to
7914558
Compare
Run generate-acp-schema + npm run generate to pick up the new custom methods added in this PR. Also add transitional context comment on the Serve subcommand linking to block#6642 and block#7697. Signed-off-by: fre <[email protected]>
fresh3nough
left a comment
There was a problem hiding this comment.
Good catch on the codegen -- pushed the regenerated schema and TS types.
On goose serve: intent is to keep it as a temporary bridge during the three-binary to one-binary transition. Once phase II of #6642 lands and goose-cli is collapsed, the serve logic can move directly into the main goose crate (or become the default behavior). Added a doc comment on the Serve variant linking to #6642 and #7697 for future readers. Happy to open a follow-up to remove the CLI subcommand once the old binaries are gone.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7906db7d82
ℹ️ 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".
Fixes a race condition where session/clear could return success while a streaming prompt task was still appending messages. Now cancels the active CancellationToken and yields before resetting, ensuring the prompt loop observes cancellation before the conversation is cleared. 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: c9feb37e6e
ℹ️ 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".
Add optional 'extension' field to GetPromptInfoRequest so clients can target a specific extension when multiple register the same prompt name. When omitted, iterate extensions in sorted order for deterministic results instead of relying on HashMap iteration order. Regenerated ACP schema and TypeScript types. Signed-off-by: fre <[email protected]>
fresh3nough
left a comment
There was a problem hiding this comment.
Fixed nondeterministic prompt lookup: added optional extension field to GetPromptInfoRequest so clients can disambiguate when multiple extensions register the same prompt name. When omitted, the fallback now iterates extensions in sorted order for deterministic results. Regenerated ACP schema and TS types.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b62ba0650f
ℹ️ 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".
… sessions Replace yield_now() with an Arc<Notify> that the prompt task signals on exit. session/clear now awaits this signal with an 800ms timeout before resetting state, guaranteeing the streaming loop has stopped. Also validate session existence via session_manager (SQLite) instead of requiring an in-memory GooseAcpSession, so session/clear works on persisted sessions consistent with session/get and session/delete. 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: 509e11ab75
ℹ️ 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".
Resolve conflict in crates/goose-acp/src/server.rs: - Combine Notify (from branch) and OnceCell (from upstream) imports - Keep branch's on_prompt setup which sets both cancel_token and prompt_done - Keep all new custom methods added by this PR Update crates/goose-acp/tests/new_custom_methods_test.rs to match upstream API changes: use ClientToAgentConnection::expected_session_id() and SessionResult destructuring instead of tuple unpacking. 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: ff2ba91d3b
ℹ️ 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".
Wrap agent.reply + stream loop in an async block so that cancel_token/prompt_done are always cleared and done.notify_waiters() is always called, even when an error causes an early return. Previously, any ? propagation out of the stream loop (reply failure, session-not-found, handle_message_content error, or explicit Err return) left a stale prompt_done in the session, causing session/clear to block for the full 800 ms timeout. Signed-off-by: fresh3nough <[email protected]> Signed-off-by: fre <[email protected]>
|
Closing this PR — thanks for the contribution effort, but there are a few issues that make it not viable in its current form:
Per the contributing guidelines, PRs need to be verified by the author before submission — please see: https://github.com/block/goose/blob/main/CONTRIBUTING.md#quick-responsible-ai-tips If you'd like to revisit this, I'd suggest:
|
|
Will do thanks @DOsinga |
|
Opened a new focused PR addressing the feedback here: #8165 Scoped to just the 5 new ACP custom methods (no |
Summary
Adds ACP custom method handlers and a unified serve subcommand to close the gaps identified in #7225 and discussed in #7697. This is the first step toward consolidating the three binaries (goose CLI, goosed server, goose-acp-server) into a single binary speaking ACP.
Changes
New ACP custom methods (server.rs, custom_requests.rs)
_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 name, model, context limit, and token usage_goose/agent/plan_prompt- get the plan prompt string for a session_goose/session/clear- clear conversation history and reset token countsNew CLI subcommand (cli.rs)
goose serve [--host 127.0.0.1] [--port 3284] [--with-builtin developer,...]- starts the ACP server over HTTP/WebSocket, replacing the need for separate goosed and goose-acp-server binariesTests
7 integration tests in
crates/goose-acp/tests/new_custom_methods_test.rscovering all new methods including error cases (invalid sessions, nonexistent prompts).Test results: 7 passed, 0 failed
Verification
cargo fmt-- cleancargo check -p goose-acp -p goose-cli-- cleancargo clippy -p goose-acp -p goose-cli --all-targets -- -D warnings-- cleancargo test -p goose-acp --test new_custom_methods_test-- 7/7 passedCloses #7225