希望支持ACP

[Pheobe-Southwood]

Problem Statement

在使用Paseo等工具时，需要通过ACP将open-code-review注册为一个单独的Provider，以便于多Agent协同工作

Proposed Solution

无

Alternatives Considered

No response

Affected Area

Other

Additional Context

No response

[stay-foolish-forever]

@Pheobe-Southwood Thanks for the suggestion!

We agree that ACP support would be a valuable addition, making OpenCodeReview easier to use with Paseo and other ACP-compatible clients. We have added it to our development roadmap and are currently evaluating the best implementation approach.

We don’t have a confirmed timeline yet, but we’ll share updates here as the work progresses. Thanks again for bringing this to our attention!

[chaojixinren]

@stay-foolish-forever Hi! I'd like to work on this issue. Could you please assign it to me?

[chaojixinren]

[Design Proposal] ACP server (stdio) + ocr subprocess harness

Motivation

OCR can only be invoked as a one-shot CLI process today. Every host that wants to integrate has to write its own plugin, there is no progress anyone can subscribe to at runtime (under --format json progress goes straight to io.Discard, so it exists on no stream at all), and cancelling mid-run leaves orphaned git processes behind.

Following what ACP v1 is for: put a protocol adapter in front of the review engine, so any compatible client (Paseo / Zed / JetBrains) uses one entry point to start a review, receive live progress, and cancel at any time, while the review logic under internal/ stays untouched. Non-goal: turning OCR into a general-purpose conversational agent.

Overall structure

flowchart TB
    C["ACP client<br/>Paseo / Zed / JetBrains"]

    subgraph acp["ocr-acp process (standalone binary)"]
        M["cmd/ocr-acp<br/>wire up and serve"]
        P["internal/acp/protocol<br/>SDK wrapper, generated types"]
        S["internal/acp/server<br/>session lifecycle · capabilities"]
        CMD["internal/acp/command<br/>slash commands + NL parsing"]
        H["internal/acp/ocrproc<br/>subprocess harness"]
    end

    C <==>|"stdio · newline-delimited JSON-RPC 2.0"| M
    M --> P --> S
    S --> CMD
    S --> H
    H ==>|"os/exec process boundary"| R["ocr review --format json --audience human"]
    R --> ENG["internal/ review engine<br/>no new import edge"]

Loading

Key trade-offs:

• A standalone ocr-acp binary rather than an ocr acp subcommand: the ACP entry point can be versioned on its own, at the cost of threading a second binary through four release paths (Makefile / install.sh / install.ps1 / npm), plus locating ocr at runtime and tolerating a version mismatch (--ocr-binary → OCR_BINARY → LookPath, with one --version probe at startup).
• stdio only. The whole Streamable HTTP section in transports.mdx reads "In discussion, draft proposal in progress." with no wire format defined, which makes it unimplementable rather than unstable. See the confirmation items below.
• Protocol library coder/acp-go-sdk (Apache-2.0, matching this repo; eino-contrib/acp ships no license, a hard blocker). Still wrapped behind internal/acp/protocol so a v2 migration stays inside one package.
• Sessions are stateless per turn, loadSession: false. A session holds {sessionId, cwd}, cwd becomes --repo, and nothing carries across turns.

Stream separation and cancellation

Once stdout is polluted the client fails to parse outright, and a cancellation that misses the process group leaves git orphans. These are the two places the spec is strictest about and the easiest to get wrong.

flowchart LR
    subgraph child["ocr subprocess"]
        CO["stdout<br/>one jsonOutput at exit"]
        CE["stderr<br/>live [ocr] text lines"]
    end

    CO -->|StdoutPipe| BUF["buffer, then unmarshal"]
    CE -->|StderrPipe| SCAN["bufio.Scanner, line by line"]
    BUF --> FRAME["JSON-RPC framer<br/>every byte wrapped here"]
    SCAN --> FRAME
    FRAME ==> OS["ocr-acp stdout<br/>valid ACP frames only"]
    LOG["own diagnostics"] --> OE["ocr-acp stderr<br/>spec permits"]

    NEVER["inherit child streams"] -.->|"never · would corrupt the channel"| OS

    style OS stroke-width:3px
    style NEVER stroke-dasharray: 5 5

Loading

Key points:

• Invariant: no path exists by which a child byte reaches our stdout unwrapped. Both pipes are captured, neither is inherited. The spec says it directly: "The agent MUST NOT write anything to its stdout that is not a valid ACP message". A dedicated test has the child print garbage to both streams and asserts our stdout is still byte-for-byte valid ACP.
• Cancellation must kill the process group. ocr spawns its own git subprocesses (--max-git-procs defaults to 16), which survive a bare Process.Kill() as orphans. This follows the pattern already in cmd/opencodereview/procattr_unix.go (Setpgid: true → syscall.Kill(-pid, SIGKILL)), with a separate file for Windows mirroring procattr_windows.go.
• Cancellation must never surface as an error. session/cancel is a notification with no response of its own; the in-flight session/prompt returns stopReason: "cancelled". Any abort error from the SDK or from exec is caught and converted at the boundary. The spec warns about this specifically, because clients display unknown errors straight to users.
• Progress lines are display-only. Each [ocr] line becomes one agent_message_chunk passed through as-is. Nothing structured is derived from the text, so a wording change upstream only affects presentation, and a missing stream means no progress updates rather than a failed turn.
• Findings arrive at end of turn (ocr emits one document at exit), mapped to tool_call with ToolCallLocation{path, line}. Whether that is clickable depends on the client's implementation, since the protocol offers the location without mandating the UI, so it is not promised as a guarantee.

Scope and breakdown

• Protocol layer + subprocess harness (internal/acp/protocol + internal/acp/ocrproc, no entry point): capability negotiation · argv assembly · binary discovery · stream separation · cancellation · jsonOutput → ACP update mapping. Acceptance: table-driven unit tests over argv per intent, the three discovery paths, and error classification; stream-isolation and cancellation tests driven by a mock ocr (a small Go helper binary that emits scripted stderr progress lines plus a canned jsonOutput), so CI needs no LLM and no API key.
• Server + entry point (internal/acp/server + cmd/ocr-acp + release plumbing): initialize (protocolVersion: 1 · authMethods: [] · loadSession: false) / session/new / session/prompt / session/cancel; /review (working tree · staged · --from/--to · single commit) and /scan (whole repo · given paths) advertised through available_commands_update, plus free-text intent parsing. Acceptance: a full initialize → session/new → session/prompt → session/cancel over in-memory pipes; one end-to-end run each on Paseo and Zed with reproducible config.
• Docs: pages/src/content/docs/{en,zh}/integrations/acp.md + README/README.zh-CN + ROADMAP.

Deferred: --progress-format ndjson (which is what would let ACP build real tool_call/plan updates, and findings stream one by one), session/load (OCR's --resume means resuming a review, not replaying history; conflating them ships a half-correct implementation), and Streamable HTTP (blocked on protocol finalization).

Three things to confirm

1. PR fix(cmd): stream review progress to stderr for json and sarif #929 is a hard prerequisite. On origin/main progress under --format json goes to io.Discard, so live progress is physically unavailable; fix(cmd): stream review progress to stderr for json and sarif #929 changes it to stdout.Swap(os.Stderr), which is what makes a progress stream exist. Suggest merging it before or alongside this work.
2. Streamable HTTP cannot be delivered against v1 (the spec defines no wire format). Suggest recording it as blocked on protocol finalization.
3. --audience semantics: --audience agent discards progress unconditionally, so an ACP server has to pass --audience human, meaning an agent declares itself a human audience. Worth a follow-up issue on a third value, or on decoupling progress from audience?