Repository navigation
feat(copilot): opt-in Copilot ACP transport for streaming tool activity - #2385
germago119 wants to merge 5 commits into
Conversation
Capture timestamped NDJSON and handle read-only permissions before integrating the preview transport. Live session, prompt, tool updates, deltas and end_turn verified against the installed Copilot CLI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Own the stdio JSON-RPC lifecycle, classify authentication failures, and enforce one-shot task permissions. Cover malformed frames, cancellation, and the measured no-tools filter workaround with an offline server. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Translate tool activity and exact text deltas into existing runtime messages and persisted progress events. Add scoped cancellation, diagnostic-only ACP handles, bounded correlation, and safe pre-submission CLI fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the canonical Copilot backend and CLI default, while selecting ACP through trusted configuration. Leave the completion provider unchanged and reject invalid or untrusted transport overrides. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Document opt-in transport selection, measured preview quirks, permission and resume boundaries, and safe fallback. Add a real-runtime smoke script with timestamped event-store evidence and extend the raw probe for explicit shell grants. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
54780cd to
758af3b
Compare
Q00
left a comment
There was a problem hiding this comment.
Reviewed the ACP client/runtime boundary and focused tests. The opt-in transport design is coherent and preserves the legacy path. Before marking ready, please add an issue reference to satisfy the PR hygiene check and resolve the current CI failure; this PR is intentionally still draft, so I am leaving a comment review rather than approval.
lee3Q
left a comment
There was a problem hiding this comment.
APPROVE for the reviewed code at this head. I reviewed the opt-in configuration/factory path, ACP client framing and response/session validation, process lifecycle and cancellation, permission decisions, event normalization, and persistence integration. The legacy CLI transport remains the default; the change does not introduce a second runtime backend or replay a submitted prompt through fallback. I found no demonstrated contract violation in the changed scope.
I ran the ACP client/events/permissions/runtime/config/integration selection: 128 passed. These were local checks using existing cached packages rather than the exact lock-file environment. An authenticated live Copilot ACP run was not performed, and repository-required CI remains a separate merge gate. This review is not a merge or deployment authorization.
Reviewed head: 758af3bae87edb744e9e636ef17b7a4e5c24e3c3.
Summary
The installed GitHub Copilot CLI streams live tool activity (text chunks, tool start/progress/result, permission decisions, completion) over the ACP protocol (
copilot --acp --stdio, JSON-RPC protocol version 1). This adds an opt-inCopilotAcpRuntimealongside the existingcopilotbackend/runtime, preserving the originalcopilot -pruntime unchanged as the default.--runtimebackend enum: ACP is a transport option on the existingcopilotbackend, selected via config (seefeat(config): add opt-in Copilot ACP transport).src/ouroboros/copilot/acp_client.py-adjacent protocol boundary owns a fresh child process and one prompt per invocation, validates envelopes/response IDs/protocol version/session IDs/stop reasons, bounds frames and startup/idle waits, drains stderr concurrently, and sends cancel before closing/reaping owned processes.Testing
350 targeted unit/integration tests added (see diffstat), covering the ACP client/events/permissions, the new runtime, config wiring, and a standalone read-only streaming probe fixture (
tests/fixtures/fake_copilot_acp.py). Existing legacy Copilot runtime tests were re-verified to still pass unmodified.Full implementation notes, including the initial ACP diagnostic (protocol handshake, observed update types, subagent-ancestry caveat) and documentation cross-link updates (CONTRIBUTING's Orchestrator → Doc Mapping, runtime guide, capability matrix, CLI/config reference, getting-started, architecture, README):
.acp-artifacts/IMPLEMENTATION-REPORT.md(not intended to ship, happy to drop it from this PR if preferred — flagging here so reviewers have the trail).Why draft
Opening as draft to get early feedback on the transport-vs-backend design decision and event-projection approach before final polish. No credentials, config, or unrelated projects were touched — this branch only affects
src/ouroboros/copilot/,src/ouroboros/orchestrator/, and their tests/docs.🤖 Generated with GitHub Copilot CLI