Skip to content

Add GitHub Copilot CLI ACP provider#8113

Closed
vincenzopalazzo wants to merge 2 commits into
aaif-goose:mainfrom
vincenzopalazzo:codex/copilot-acp-provider
Closed

Add GitHub Copilot CLI ACP provider#8113
vincenzopalazzo wants to merge 2 commits into
aaif-goose:mainfrom
vincenzopalazzo:codex/copilot-acp-provider

Conversation

@vincenzopalazzo
Copy link
Copy Markdown
Contributor

Summary

  • add a copilot-acp provider that wires GitHub Copilot CLI's native --acp mode into AcpProvider
  • register the provider, add the configurable COPILOT_CLI_COMMAND, and cover it in the agentic provider test matrix
  • document setup and usage for the new ACP provider in the provider docs

Closes #8104

Test plan

  • source bin/activate-hermit && cargo fmt
  • source bin/activate-hermit && cargo test -p goose --test providers test_copilot_acp_provider -- --nocapture
  • source bin/activate-hermit && cargo clippy --all-targets -- -D warnings
  • source bin/activate-hermit && cargo run -p goose-cli --bin goose -- run --no-session --provider copilot-acp --model current -t "Say hello in one short sentence."
  • same one-shot goose run probe with GOOSE_MODE=approve and GOOSE_MODE=chat to verify Copilot ACP accepts those session modes

Review notes

  • follow-up to closed PR Add GitHub Copilot CLI as a provider #8108: this intentionally uses Copilot CLI's native ACP server instead of a copilot -p prompt wrapper
  • local runtime verification is quota-limited after session creation, but provider creation now succeeds in auto, approve, and chat, and failures occur later as Copilot quota errors rather than ACP wiring errors

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

DOsinga commented Mar 26, 2026

Thanks for this — the implementation is clean and follows the existing gemini-acp pattern well.

One thing worth noting for the longer term: we already have a declarative provider system (see crates/goose/src/config/declarative_providers.rs) that lets us define providers as JSON config files and ship them built-in, without a Rust struct per provider. Right now that only covers LLM-API engines (OpenAI/Anthropic/Ollama-compatible). ACP providers are structurally different — they launch a subprocess rather than talk to an HTTP API — but the same idea applies.

The goal would be an ACP engine type in ProviderEngine and a declarative ACP config that captures the things that actually vary between ACP providers:

  • the binary name and how to resolve it (PATH / npm / brew)
  • the --acp / -c style args
  • the goose-mode → session-mode-id mapping
  • the permission option IDs (allow/reject vs approved/abort)
  • any env_remove entries

With that, adding a new ACP provider would be a JSON file rather than a new Rust source file and a new config_value! entry. We could also ship first-party ACP providers (Copilot, Gemini, Codex, Claude) as built-in declarative configs the same way we already do for Groq, Tanzu, etc.

Not asking you to do this now — it is a non-trivial refactor and this PR is a fine incremental step. Just flagging it as the direction we would want to head so future ACP providers do not keep adding to the per-provider boilerplate.

@vincenzopalazzo
Copy link
Copy Markdown
Contributor Author

@DOsinga Do you think that it is worth opening a PR to keep this in mind for the future?

@DOsinga
Copy link
Copy Markdown
Collaborator

DOsinga commented Mar 26, 2026

I don't know, I think we should close this and do the custom provider thing for acp /cc @codefromthecrypt ?

@codefromthecrypt
Copy link
Copy Markdown
Collaborator

agree declarative would be a good move forward and was the original intent (once we find all the various flags and mappings). It might be stable enough to do that now.

@vincenzopalazzo do you mind pasting into the desc actual output of the providers test e.g. source bin/activate-hermit && cargo test -p goose --test providers test_copilot_acp_provider -- --nocapture and the on-off command, as not everyone have creds and this providers.rs test isn't run on PR.

@codefromthecrypt
Copy link
Copy Markdown
Collaborator

codefromthecrypt commented Mar 26, 2026

ps I audited copilot manually and it seems to be fairly capable at a glance anyway. Not perfect but up there with cursor where ACP is a part of the standard binary and basic functionality works.

@codefromthecrypt
Copy link
Copy Markdown
Collaborator

codefromthecrypt commented Mar 27, 2026

fwiw definitely need to look at that providers.rs test and actually running it, while the below is vibed, the agent isn't thrilled ;) Just breeze through these if anything is legit


Here's what needs to change in the PR based on my runtime testing:

Critical bugs

  1. Permission optionIds are wrong.
    allow_option_id: Some("allow".to_string()),
    reject_option_id: Some("reject".to_string()),
    Should be:
    allow_option_id: Some("allow_once".to_string()),
    reject_option_id: Some("reject_once".to_string()),
    Copilot-cli uses allow_once, allow_always, reject_once. There is no allow or reject optionId. Permission flows will silently fail.

  2. mcpServers format mismatch.
    extension_configs_to_mcp_servers() likely returns the standard ACP object/map format. Copilot-cli expects mcpServers as an array where each entry has { name, command, args,
    env: [] }. If goose passes the standard format, session/new will fail with a Zod validation error: "expected array, received object" at path ["mcpServers"].

  3. protocolVersion format.
    Copilot-cli expects numeric 1, not the date string "2025-03-26". If goose's sacp crate sends a string, the initialize call will fail: "expected number, received string" at
    path ["protocolVersion"]. Need to verify what sacp sends.

Likely issues

  1. --model flag unverified.
    The PR passes --model as a CLI arg. I couldn't confirm this flag exists from the changelog. Model switching works via set_config_option with configId: "model" after
    session creation. If --model doesn't work, the provider will start with the wrong model silently.

  2. session/new requires cwd.
    Copilot-cli returns InvalidParams if cwd is missing from session/new. The PR passes work_dir: std::env::current_dir() which should cover this, but worth confirming the
    AcpProvider actually sends it.

Nits

  1. "Public preview" is outdated. Copilot CLI ACP has been available since v0.0.397, GA since v0.0.418 (2026-02-25). The docs say "currently in public preview" which
    undersells stability.

  2. Mode mapping is reasonable but GooseMode::Chat → Plan is a stretch. Copilot's Plan mode creates/executes multi-step plans; goose Chat is conversational. No perfect
    mapping exists though.

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo vincenzopalazzo force-pushed the codex/copilot-acp-provider branch from 65b9876 to 47a077f Compare March 27, 2026 08:54
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: 47a077f889

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// Don't run tests with ACP_CURRENT_MODEL, as gemini sets "auto-gemini-3" even when the user
// has no access to the Preview Release Channel, resulting in "Requested entity was not found."
// See https://github.com/google-gemini/gemini-cli/issues/22803
ProviderTestConfig::with_agentic_provider("gemini-acp", "auto-gemini-2.5", "gemini")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Use a registered provider ID in gemini ACP test

This test will fail on machines that actually have the gemini binary installed, because the skip gate passes and create_with_named_model("gemini-acp", ...) is executed, but the registry setup in crates/goose/src/providers/init.rs does not register a gemini-acp provider (only GeminiCliProvider, i.e. gemini-cli). That makes the new test deterministically error with Unknown provider: gemini-acp in environments where it runs instead of being skipped.

Useful? React with 👍 / 👎.

@vincenzopalazzo
Copy link
Copy Markdown
Contributor Author

@codefromthecrypt this is the output of the command, but IDK if this is what you are looking for

Hermit environment /Users/vincenzopalazzo/github/work/goose-copilot-acp activated
    Finished `test` profile [unoptimized + debuginfo] target(s) in 1m 18s
     Running tests/providers.rs (target/debug/deps/providers-b3b2040d4cbb71b2)

running 1 test
=== copilot-acp::model_listing ===
[crates/goose/tests/providers.rs:491:9] &models = [
    "claude-sonnet-4.6",
    "claude-sonnet-4.5",
    "claude-haiku-4.5",
    "claude-opus-4.6",
    "claude-opus-4.5",
    "claude-sonnet-4",
    "gpt-5.4",
    "gpt-5.3-codex",
    "gpt-5.2-codex",
    "gpt-5.2",
    "gpt-5.1-codex-max",
    "gpt-5.1-codex",
    "gpt-5.1",
    "gpt-5.1-codex-mini",
    "gpt-5-mini",
    "gpt-4.1",
]
===================
=== copilot-acp::basic_response === Error: 402 You have no quota (Request ID: E9D3:1D5C75:860DC3:959B0B:69C6471C)

thread 'test_copilot_acp_provider' (699470) panicked at crates/goose/tests/providers.rs:395:9:
Error: 402 You have no quota (Request ID: E9DF:3427ED:87CC46:975365:69C64721)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test test_copilot_acp_provider ... FAILED

failures:

failures:
    test_copilot_acp_provider

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 19 filtered out; finished in 16.48s


============== Providers ==============
❌ copilot-acp
=======================================

error: test failed, to rerun pass `-p goose --test providers`

@codefromthecrypt
Copy link
Copy Markdown
Collaborator

@vincenzopalazzo yeah basically we need to know if the provider succeeds for someone. I'll take a look at reproducing this.

@codefromthecrypt
Copy link
Copy Markdown
Collaborator

added this to replace it. thanks for the help, but we should really only raise PRs we have access to test #8154

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.

Support GitHub Copilot CLI as an ACP Provider

3 participants