refactor(drivers): extract shared acp_native base from gemini/kimi/opencode (#111) by Fullstop000 · Pull Request #117 · Fullstop000/Chorus

Fullstop000 · 2026-04-27T12:55:06Z

Summary

Extract ~6500 lines of structurally near-identical code from gemini.rs, kimi.rs, and opencode.rs into a single src/agent/drivers/acp_native/ module. Closes #111.

The three ACP-native drivers each carried their own copy of: reader loop, response routing, session lifecycle, ensure_started semantics, EOF drain, permission auto-approval, multi-session close teardown. Bug fixes had to be applied in three places and behaviors drifted apart at edges (unrouted error handling, init prompt timing, session-update routing).

Per-runtime variation now lives entirely in a &'static AcpDriverConfig struct of function pointers, bools, and enums. No trait, no generics — three runtimes, single instantiation, function-pointer dispatch.

Behavior is preserved bit-for-bit. Cancel stays local-only. stopReason continues to be ignored. session/close stays local. No capability checking before session/load. No session/resume. The 8 ACP spec gaps the plan catalogued are tracked as follow-ups — each becomes a 1-place fix now that the base is shared.

Diff

File	Before	After	Δ
`gemini.rs`	1718	423	−75%
`kimi.rs`	2737	328	−88%
`opencode.rs`	2834	339	−88%
Drivers total	7289	1090	−85%
New shared module	0	2560	of which 891 is the new test module
Net	7289	3650	−50%

Per-runtime config knobs the migration surfaced

Field	kimi	gemini	opencode
`init_prompt_strategy`	Immediate	Immediate	Immediate
`initialized_notification_payload`	None	`Some(initialized RPC)`	None
`session_load_includes_mcp`	true	false	false
`emit_starting_lifecycle`	true	false	true
`build_first_prompt_prefix`	`Some(standing_prompt)`	None	None

InitPromptStrategy::Deferred is defined for symmetry but currently unused — opencode's old "deferred bootstrap prompt" mechanism collapsed naturally into Immediate once the bootstrap/secondary handle split was removed (the race it protected against can't occur in the unified model).

Opencode shape conversion (the harder half)

Pre-refactor, opencode used a FactoryPath::Bootstrap | Secondary split with separate run_bootstrap / run_secondary paths and a deferred-prompt mechanism that pre-allocated a JSON-RPC id at handshake time to avoid colliding with a racing secondary new_session. The refactor collapses this into the unified AcpNativeHandle. The race cannot occur because ensure_started serializes through start_in_progress, and alloc_id only runs after that mutex is released.

Deletes: OpencodeAgentProcess, FactoryPath, run_bootstrap*, send_deferred_bootstrap_prompt, opencode's local classify_line/dispatch_line, and the bootstrap-only state fields (bootstrap_pending_prompt, bootstrap_session_id, bootstrap_requested_session_id).

Coverage gate

The 19 generic tests in acp_native/tests.rs use a TestConfig and exercise the shared paths without a real runtime binary. The audit table at the top of tests.rs maps each pre-migration per-driver test to its shared equivalent. Per-driver tests are now scoped to driver-specific concerns only (probe, list_models, MCP shape, command construction).

New shared test: ensure_started_concurrent — drives two concurrent ensure_started calls and asserts the slow path runs exactly twice (each caller retries after its predecessor's failure releases the lock). Closes a coverage gap that was previously only exercised indirectly via kimi's test-only counter.

Plan + locked decisions

Plan: docs/plans/2026-04-27-acp-native-driver-unification-plan.md (gitignored — local-only working doc).

Decisions locked at plan-eng-review:

D1 — Scope: extraction only. All 8 ACP spec gaps deferred to follow-up PRs. Behavior preserved bit-for-bit.
D2 — Abstraction: struct config with function pointers. No trait, no generics.
D3 — Migration: plan called for 2 PRs (kimi+gemini, then opencode); both landed in this single branch since the shape conversion is small and the diff stays reviewable as one piece. Happy to split if you'd rather.
D4 — Coverage gate: explicit audit table; per-driver tests deleted only after shared equivalents passed.

NOT in scope (each becomes a follow-up issue)

These are real ACP spec gaps. Each becomes a small focused PR after this base lands:

#	Gap	Spec
1	Cancel sends `session/cancel` notification; stopReason: cancelled mapping	schema#session-cancel
2	Parse `stopReason` from prompt response → `FinishReason` mapping	schema#session-prompt
3	`session/close` RPC behind capability gate	schema#session-close
4	Parse `AgentCapabilities` from initialize response	initialization#agent-capabilities
5	Prefer `session/resume` over `session/load` when advertised	session-setup#resuming-sessions
6	`session/load` history replay drain	session-setup#loading-a-session
7	Extend `AcpUpdateItem` with Plan / AvailableCommandsUpdate / ModeChange / SessionInfoUpdate	schema#session-update
8	MCP HTTP/stdio negotiation per `mcpCapabilities.http`	session-setup#mcp-servers

Test plan

🤖 Generated with Claude Code

…encode (#111) Three ACP-native drivers (gemini, kimi, opencode) each carried ~1500-2700 lines of structurally near-identical code: reader loop, response routing, session lifecycle, cancel/close, ensure_started semantics, EOF drain, permission auto-approval. Bug fixes had to be applied in three places and behaviors drifted apart at edges. Move all of it into `src/agent/drivers/acp_native/`: - `mod.rs` — `AcpDriverConfig` (struct of fn pointers + bools + enums) + `InitPromptStrategy` + shared `open_session` helper. - `state.rs` — `SharedReaderState`, `PendingRequest`, `SessionState`. - `core.rs` — `AcpNativeCore`, `ensure_started` (race-safe lazy spawn, non-sticky failure), `spawn_and_initialize`, `is_stale`, `Drop`. - `handle.rs` — `AcpNativeHandle` + full `Session` impl: run, prompt, cancel, close (with `closed_emitted` race guard). - `reader.rs` — `reader_loop`, `handle_response`, `handle_session_update`, `pick_session*`. Routes responses by JSON-RPC id through `pending_requests` map (avoids `acp_protocol::parse_line`'s id-bucketing that misclassifies `session/new` at id≥3 as PromptResponse). - `tests.rs` — 19 generic tests with a `TestConfig`. Audit table at top maps each pre-migration per-driver test to its shared equivalent. Per-runtime variation lives entirely in the static `&'static AcpDriverConfig` each driver owns. No trait, no generics — three runtimes, single instantiation, function-pointer dispatch. Behavior preserved bit-for-bit: - Cancel stays local-only. - `stopReason` continues to be ignored; all completions emit Natural. - `session/close` remains local-only (no RPC). - No capability checking before `session/load`; no `session/resume`. - HTTP MCP transport stays as-is. The 8 ACP spec gaps catalogued in the plan are tracked as follow-up issues — each becomes a 1-place fix now that the base is shared. New shared test `ensure_started_concurrent` closes a coverage gap: drives two concurrent `ensure_started` calls and asserts the slow path runs exactly twice (each caller retries after its predecessor fails, proving serialization + non-stickiness without needing a real runtime binary). Opencode shape conversion: collapse the `FactoryPath::Bootstrap | Secondary` split into the unified handle. The race the bootstrap protected against (deferred prompt id colliding with a racing secondary `new_session`) cannot occur in the unified model — `ensure_started` serializes through `start_in_progress`, and `alloc_id` runs only after that mutex is released. Deletes `OpencodeAgentProcess`, `FactoryPath`, `run_bootstrap*`, `send_deferred_bootstrap_prompt`, the local `classify_line`/`dispatch_line`, and the bootstrap-only state fields (`bootstrap_pending_prompt`, `bootstrap_session_id`, `bootstrap_requested_session_id`). Diff: - gemini.rs 1718 → 423 (−75%) - kimi.rs 2737 → 328 (−88%) - opencode.rs 2834 → 339 (−88%) - net: 7289 → 3650 lines (−50% across drivers + new shared module incl. tests) Verified: cargo test (527 passed), cargo test --test e2e_tests (10 passed), cargo clippy --lib --tests -- -D warnings (clean). Plan: docs/plans/2026-04-27-acp-native-driver-unification-plan.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Refactors the ACP-native runtime drivers (Gemini/Kimi/OpenCode) by extracting their shared process/session plumbing into a unified src/agent/drivers/acp_native/ module, leaving per-runtime differences behind a static config.

Changes:

Introduces acp_native shared core/handle/reader/state implementation and delegates driver open_session to a common helper.
Reworks kimi.rs, gemini.rs, and opencode.rs into thin wrappers that provide runtime-specific spawn/MCP/prompt/config behavior.
Adds a large shared test suite in acp_native/tests.rs to cover the extracted behavior.

Reviewed changes

Copilot reviewed 8 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/agent/drivers/mod.rs	Exposes the new `acp_native` module.
src/agent/drivers/kimi.rs	Converts Kimi driver to `AcpDriverConfig` + thin wrapper; keeps Kimi-specific spawn/MCP/prompt bits.
src/agent/drivers/gemini.rs	Converts Gemini driver to `AcpDriverConfig` + thin wrapper; keeps Gemini system prompt caching and initialized notification.
src/agent/drivers/opencode.rs	Converts OpenCode driver to `AcpDriverConfig` + thin wrapper; keeps opencode.json/system prompt file generation and model parsing.
src/agent/drivers/acp_native/mod.rs	Defines the shared ACP-native driver surface (`AcpDriverConfig`, `open_session`, shared exports).
src/agent/drivers/acp_native/core.rs	Implements shared per-agent core: spawn/init, stdio wiring, staleness, drop teardown.
src/agent/drivers/acp_native/handle.rs	Implements shared per-session handle: run/prompt/cancel/close semantics.
src/agent/drivers/acp_native/reader.rs	Implements shared stdout reader loop and response/session-update routing.
src/agent/drivers/acp_native/state.rs	Defines shared reader-loop state (`SharedReaderState`, `PendingRequest`, `SessionState`).
src/agent/drivers/acp_native/tests.rs	Adds generic plumbing tests and maps removed per-driver tests to shared equivalents.

Copilot · 2026-04-27T12:59:13Z

+
+    let j0 = tokio::spawn(async move { c0.ensure_started().await });
+    let j1 = tokio::spawn(async move { c1.ensure_started().await });
+    let (_r0, _r1) = tokio::join!(j0, j1);


In ensure_started_concurrent, the spawned task results are ignored (let (_r0, _r1) = tokio::join!(...)). If either task panics, the test can still pass, masking real regressions. Please unwrap/expect the JoinHandle results (and optionally assert the inner Result as appropriate) so panics fail the test.

Suggested change

let (_r0, _r1) = tokio::join!(j0, j1);

let (r0, r1) = tokio::join!(j0, j1);

let _ = r0.expect("first ensure_started task panicked");

let _ = r1.expect("second ensure_started task panicked");

Copilot · 2026-04-27T12:59:13Z

+//! | kimi: `multi_session_session_load_falls_back_to_expected_id`            | `session_load_falls_back_to_expected_id`              |
+//! | kimi: `multi_session_prompt_response_carries_correct_session_id`        | `prompt_response_carries_correct_session_id`          |
+//! | kimi: `handle_response_ignores_unknown_id`                              | `response_for_unknown_id_is_ignored`                  |
+//! | kimi: `alloc_id_starts_at_3_after_spawn_and_initialize`                 | (covered by `ensure_started_idempotent_after_success`) |


The coverage audit table claims alloc_id_starts_at_3_after_spawn_and_initialize is covered by ensure_started_idempotent_after_success, but that test never exercises alloc_id() or asserts the first allocated id. Either update the audit mapping or add a small explicit assertion/test that the post-start next_request_id behavior is correct.

Suggested change

//! | kimi: `alloc_id_starts_at_3_after_spawn_and_initialize` | (covered by `ensure_started_idempotent_after_success`) |

//! | kimi: `alloc_id_starts_at_3_after_spawn_and_initialize` | (no shared equivalent yet; add an explicit alloc_id assertion/test) |

Copilot · 2026-04-27T12:59:13Z

+//! negotiation) are tracked as follow-up work — see the plan at
+//! `docs/plans/2026-04-27-acp-native-driver-unification-plan.md`.


The module docs reference docs/plans/2026-04-27-acp-native-driver-unification-plan.md, but that file is not present under docs/plans/ in this repo. This will mislead readers and breaks rustdoc links; please either add the document to the repo, or change the reference to an existing doc/issue/PR.

Suggested change

//! negotiation) are tracked as follow-up work — see the plan at

//! `docs/plans/2026-04-27-acp-native-driver-unification-plan.md`.

//! negotiation) are tracked as follow-up work in the project's planning and

//! issue-tracking process.

Copilot · 2026-04-27T12:59:14Z

+    /// kimi.
+    Immediate,
+    /// Wait for the caller to invoke [`super::Session::prompt`] explicitly.
+    /// Used by opencode (deferred init prompt — landed in PR2).


InitPromptStrategy::Deferred is documented as “Used by opencode…”, but opencode is configured with InitPromptStrategy::Immediate and the PR description says Deferred is currently unused. Please update the enum’s doc comment to avoid misleading future changes.

Suggested change

/// Used by opencode (deferred init prompt — landed in PR2).

/// This strategy is currently unused.

CI fixes: - cargo fmt across acp_native module + per-driver wrappers (rustfmt rules prefer multi-line struct/match args). PR review (Copilot): - mod.rs: scrap reference to gitignored docs/plans/* file; point readers at issue #111 + the PR description for spec gap context. - mod.rs: rewrite InitPromptStrategy::Deferred docstring — opencode no longer needs it. Kept as a config knob for future runtimes that genuinely defer the first prompt. - tests.rs: ensure_started_concurrent now expects each tokio JoinHandle so a panic in either task fails the test instead of getting masked. - tests.rs + handle.rs: add the missing `alloc_id_starts_at_3_after_spawn_and_initialize` shared test the audit table claimed existed. Tests that the first allocated id after ensure_started seeds next_request_id=3 is exactly 3, and that no id-3 placeholder is pre-registered. Exposed via a #[cfg(test)] alloc_id_for_test shim on AcpNativeHandle. Verified locally: cargo fmt --check (clean), cargo test --lib (324 passed, +1 vs prior count for the new alloc_id test), cargo clippy --lib --tests -- -D warnings (clean). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… pointer (#117) Address self-review code smells flagged on PR #117: #1 — Three `*_for_test` shims leaked module internals just to bridge the sibling-module boundary between `acp_native::tests` and the files under test. Replaced with two visibility tightenings and one test relocation: - `AcpNativeHandle::alloc_id` is now `pub(super)`. Deleted `alloc_id_for_test`. Tests call `alloc_id()` directly. - `reader::handle_response` is now `pub(super)`. Deleted `handle_response_for_test`. Tests call `handle_response(...)` directly. - The three `close()` multi-session tests moved into `handle.rs::tests` as an inline `#[cfg(test)] mod tests` block. Inside the same module they construct `AcpNativeHandle` with private field access — no `set_session_for_test` setter shim required. Deleted that shim too. To support tests in multiple files, factored shared fixtures (`TEST_CFG`, `TEST_REGISTRY`, `test_spec`, `make_core`, `open_test_session`, `fresh_shared`) into a new `acp_native/test_fixtures.rs` gated on `#[cfg(test)]`. Both `acp_native::tests` and `acp_native::handle::tests` import from it. #2 — `InitPromptStrategy::Deferred` was annotated `#[allow(dead_code)]` "for future runtimes." YAGNI. Deleted the variant. The enum stays as a single-variant enum (rather than collapsing to "always immediate" behavior) so a future driver that genuinely needs to defer can extend without a wire-shape breaking change. Doc-comment on the enum explains why. #4 — `AcpDriverConfig::registry` was `fn() -> &'static AgentRegistry<...>` wrapping a function-local static. Hoisted each driver's static to module level (`KIMI_REGISTRY`, `GEMINI_REGISTRY`, `OPENCODE_REGISTRY`, `TEST_REGISTRY`) and changed the field type to `&'static AgentRegistry<AcpNativeCore>`. Removed the `(cfg.registry)()` call indirection at every use site. `AgentRegistry::new` is `const fn` so this just works. Verified: cargo fmt --check (clean), cargo test --lib (324 passed), cargo clippy --lib --tests -- -D warnings (clean). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

) Self-review followup: the two pick_session* helpers had identical hint resolution logic (lock, single-session fallback, warnings) and differed only in whether the return value bundled the session's run_id. Six call sites were split arbitrarily — Thinking/Text used pick_session_and_run, ToolCall/ToolCallUpdate/ToolResult/TurnEnd used pick_session. ~30 lines of duplicated logic for a single Option<Uuid> field lookup. Folded into one function: `pick_session_and_run` returning `(Option<String>, Option<RunId>)`. Callers that don't want run_id destructure with `_`. Behavior preserved bit-for-bit — same lock policy, same warn messages (renamed to driver-agnostic "session-update" since they no longer name a specific helper). Verified: cargo fmt --check (clean), cargo test --lib (324 passed), cargo clippy --lib --tests -- -D warnings (clean). Re acp_protocol.rs location: keep at drivers::acp_protocol. It's the ACP wire-format layer (JSON-RPC parsing + frame builders), used by acp_native AND by event_forwarder::strip_mcp_prefix outside acp_native. Moving into acp_native would imply ownership it doesn't have. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings April 27, 2026 12:55

Copilot started reviewing on behalf of Fullstop000 April 27, 2026 12:55 View session

Copilot AI reviewed Apr 27, 2026

View reviewed changes

Fullstop000 and others added 3 commits April 27, 2026 21:35

Fullstop000 merged commit 6b5a196 into main Apr 27, 2026
3 checks passed

Fullstop000 deleted the refactor/acp-native-base branch April 27, 2026 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(drivers): extract shared acp_native base from gemini/kimi/opencode (#111)#117

refactor(drivers): extract shared acp_native base from gemini/kimi/opencode (#111)#117
Fullstop000 merged 4 commits intomainfrom
refactor/acp-native-base

Fullstop000 commented Apr 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-    let (_r0, _r1) = tokio::join!(j0, j1);
+    let (r0, r1) = tokio::join!(j0, j1);
+    let _ = r0.expect("first ensure_started task panicked");
+    let _ = r1.expect("second ensure_started task panicked");

	//! \| kimi: `alloc_id_starts_at_3_after_spawn_and_initialize` \| (covered by `ensure_started_idempotent_after_success`) \|
	//! \| kimi: `alloc_id_starts_at_3_after_spawn_and_initialize` \| (no shared equivalent yet; add an explicit alloc_id assertion/test) \|

		//! negotiation) are tracked as follow-up work — see the plan at
		//! `docs/plans/2026-04-27-acp-native-driver-unification-plan.md`.

	/// Used by opencode (deferred init prompt — landed in PR2).
	/// This strategy is currently unused.

Conversation

Fullstop000 commented Apr 27, 2026

Summary

Diff

Per-runtime config knobs the migration surfaced

Opencode shape conversion (the harder half)

Coverage gate

Plan + locked decisions

NOT in scope (each becomes a follow-up issue)

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants