feat(acp): expose Moltis as an ACP agent over stdio - #1169
Open
penso wants to merge 16 commits into
Open
Conversation
Moltis has only ever been an ACP client: `moltis-external-agents` spawns and drives `codex-acp`, `claude-agent-acp`, and Cursor's `agent acp`. The inverse did not exist, so no ACP harness (Zed, `buzz-acp`, a bespoke runner) could use Moltis as its agent. Adds `crates/acp` implementing the agent side of the protocol, plus a `moltis acp` subcommand behind a default-on `acp` feature. ACP is modelled as a surface beside the Web UI and GraphQL, not as a channel. `ChannelType` is a closed enum whose contract is about external correspondents — allowlists, sender identity, OTP, per-account settings. An ACP client is the local parent process that spawned us: there is no sender to gate and no account to configure. Three design points worth knowing before changing this code: - The protocol traits are `#[async_trait(?Send)]`, so the handler is pinned to a `LocalSet` while Moltis's services are `Send + Sync`. The `AcpBackend` trait is where they meet: implementations are `Send + Sync` and never learn a `LocalSet` exists, and streaming crosses back through an mpsc channel rather than a direct await on a `!Send` future. Running the `LocalSet` via `run_until` on the multi-threaded runtime keeps backend work on the full thread pool. - `prompt` drains updates and the turn future in one `select!` on the same task, then flushes what is queued before responding. A spawned forwarder would have been simpler but lets a backend that leaks a `TurnUpdates` clone wedge the response, since the channel only closes once every sender drops. - Cancellation is a notification that arrives while `prompt` is pending. It flags the session without waiting on the turn, and a cancel that races the turn's own completion is still reported as `Cancelled` — the client is waiting for that signal either way. stdout is the wire: a single log line written there corrupts the JSON-RPC stream and the client disconnects with a parse error. `init_telemetry` now switches the tracing writer to stderr for commands that reserve stdout. Covered both by a crate-level test that tees the wire with a subscriber at TRACE, and by an integration test that spawns the real binary at `--log-level trace` and parses every stdout byte — only the latter would catch a stray `println!` in the startup path. ACP sessions live in an `acp:` namespace so they cannot collide with Web UI or channel sessions, and the `SessionId` is the session key itself, which keeps `session/load` trivial. Real Moltis turns are not wired up yet, so `moltis acp` serves a built-in echo agent via `--echo` and otherwise exits with an error rather than pretending to work.
Contributor
Greptile SummaryACP session isolation is now enforced at the protocol boundary.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported namespace bypass is fixed before backend access, and successful loading registers only validated ACP session identifiers.
|
| Filename | Overview |
|---|---|
| crates/acp/src/agent.rs | Enforces the ACP namespace before backend session loading and validates newly minted backend session keys. |
| crates/acp/src/session.rs | Defines the namespace predicate and exact session-key registry behavior used by the protocol boundary. |
| crates/acp/tests/protocol.rs | Covers rejection of client-provided and backend-created session identifiers outside the ACP namespace. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Client[ACP client] -->|session/load ID| Validate{ID begins with acp: and has a suffix?}
Validate -->|No| Reject[Return invalid_params]
Validate -->|Yes| Backend[Load session from backend]
Backend --> Replay[Replay history]
Replay --> Register[Register session for prompt and cancel]
BackendNew[Backend creates session] --> ValidateNew{Created ID is ACP-namespaced?}
ValidateNew -->|No| InternalError[Return internal_error]
ValidateNew -->|Yes| RegisterNew[Register and return session ID]
Reviews (3): Last reviewed commit: "fix(web): prevent session title toolbar ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
session/load turned an arbitrary client-supplied SessionId into a session key, handed it to the backend, and registered it, without ever checking the namespace. Unlike prompt and cancel there is no registry entry to validate against — resuming a session the connection never opened is the point of the method — so the acp: prefix was the only thing separating an ACP client from Web UI and channel sessions, and nothing checked it. SessionKey::is_namespaced existed but had no production caller. A client could therefore name a foreign session key and, once a resumable Moltis backend is wired up, drive it with subsequent prompts. Checking in the crate rather than in each backend keeps the invariant in one place instead of making every future backend re-derive it to stay isolated. load_session now rejects out-of-namespace ids with invalid_params before consulting the backend, and new_session refuses to hand back a key a backend minted outside the namespace, which is an internal error rather than bad input.
…features The stdout-hygiene test in crates/acp/tests/protocol.rs calls tracing::Level::TRACE, but tracing was only an optional dependency behind the crate's tracing feature. The test target therefore failed to link it whenever the crate was built without that feature. Nothing local caught this because just test and just lint both build with --all-features on Linux, which enables the feature and makes the crate resolvable. The coverage job runs cargo test --tests --workspace with default features, where it does not resolve, so that job was the only one to fail. The test needs a subscriber regardless of which features the crate ships with, so tracing belongs in dev-dependencies rather than being reached for through the feature.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
@greptileai please re-review — the namespace-validation finding from the last pass is addressed. Two commits since the reviewed
Generated by Claude Code |
The ACP surface shipped with only an echo backend: the transport and handshake worked, but `moltis acp` refused to run without --echo, so no client could actually chat with Moltis. Wires the backend to ChatService::send_sync, the same seam the Web UI submits through, so a prompt runs the real agent loop with the configured providers, tools, memory, and session history. Streaming reuses the gateway's broadcast registry rather than adding a notification path to the chat crate. ConnectedClient is a bounded mpsc::Sender<String> plus subscription metadata, so registering one for the duration of a turn yields exactly the frames the Web UI receives, including any added later. send_sync resolving is what ends the turn -- the broadcast has no terminal frame, and waiting for the channel to close would hang because the gateway holds the sender until the client is unregistered. Queued frames are drained afterwards so the visible reply is not truncated, and a Drop guard unregisters the client on every exit path because broadcast() walks every registered client on every frame. Two wire-format details drive the frame mapper. thinking_text carries accumulated reasoning rather than a delta, since the Web UI replaces its thinking pane wholesale, so the mapper diffs against what it has seen and re-emits whole when a provider restarts its reasoning instead of extending it. Frames for every session share one broadcast, so each is matched against the turn's session key -- without that check one client's tokens would leak into another client's turn. prepare_gateway_core binds no socket, so the stack boots inside the spawned process without a server running or a port taken. It does open the data_dir databases, which is what makes an ACP session visible in the Web UI session list. --echo still short-circuits before any of this, so a client's handshake can be checked without providers or databases.
The initial ACP surface could only echo prompts and did not carry Moltis cancellation, workspace, or MCP semantics into real turns. Its synchronous chat path also duplicated execution and diverged from normal policy and queue behavior.\n\nRun ACP through the normal registered chat path, add a headless gateway profile, bind validated session workspaces, isolate client MCP processes per session, and clean up turns and tools on disconnect. Preserve request tool policy and ephemeral behavior while keeping stdout exclusively for ACP framing.
The real-backend protocol test inherited developer credentials and performed provider discovery, making its startup deadline unreliable during the full concurrent suite. Give the child an isolated home and config, disable code indexing, and allow enough startup time under nextest CPU contention.
The WASM HTTP timeout test could leave its fixture thread blocked forever in accept or read after the client timed out, causing the full workspace suite to hang. Use bounded nonblocking accept and read waits so the fixture always terminates under scheduler contention.
A shrinking session-name mount retained the title's intrinsic width, allowing the title to paint underneath the project selector and intercepting rename interactions. Let the title flex item shrink and truncate inside its assigned toolbar space.
Collaborator
Author
|
@greptileai please re-review the current head. The production ACP backend, cancellable chat integration, cwd binding, session-scoped MCP runtime, headless startup profile, cleanup paths, tests, and documentation are now complete. Full local validation passes, including Rust tests, macOS/iOS builds, E2E, and coverage. |
The inbound agent trusted its local parent but still accepted unbounded protocol, history, MCP, and streaming data, while lifecycle races and broadcast drops could truncate or misreport turns. Payload-bearing trace logs also exposed prompts, tool data, and provider responses. Apply bounded framing and session resources, pre-allocation history limits, session-scoped MCP isolation, relevant-frame delivery accounting, deterministic cleanup, headless credential isolation, and payload-safe logging. Add protocol, runtime, and transport regressions and document the resulting trust boundary.
QMD, real ACP startup, and synchronous WASM HTTP fixtures can time out when they compete with the full workspace suite. Reserve the nextest worker pool for those tests in local and CI profiles so scheduler pressure does not create false failures.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
withered-breeze-e956 | d4c7a7e | Commit Preview URL Branch Preview URL |
Jul 28 2026, 11:39 PM |
ACP prompt execution reloaded history without the replay limits, exec buffered complete child output before truncation, and dropped MCP requests retained pending state. This applies bounds on the active paths, makes child and MCP cancellation cleanup deterministic, and rejects output reconciliation failures instead of acknowledging incomplete text. Also stabilize session-local MCP names across reloads and remove the remaining command and provider event payloads from tracing.
Keep MCP pending-request guards armed until map removal completes so aborting timeout cleanup cannot leak entries. Reapply the exec byte limit after lossy UTF-8 decoding to prevent invalid output from expanding past the configured budget.
Exec timeouts and stdio MCP shutdown previously killed only the direct child, allowing inherited descendants to outlive their owner. Own spawned commands in Unix process groups or Windows Job Objects so explicit kill and cancellation-driven drop terminate and reap the full tree.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
moltis acpcommand, routing prompts through the normal cancellableLiveChatServicepath.Validation
Completed
LOCAL_VALIDATE_TEST_CMD="cargo +nightly-2026-06-20 nextest run -p moltis-tools exec && cargo +nightly-2026-06-20 nextest run -p moltis-mcp transport && cargo +nightly-2026-06-20 nextest run -p moltis-chat && cargo +nightly-2026-06-20 nextest run -p moltis-providers --features local-llm-metal && cargo +nightly-2026-06-20 nextest run -p moltis" ./scripts/local-validate.sh 1169./scripts/build-swift-bridge.sh && ./scripts/generate-swift-project.sh && ./scripts/lint-swift.sh && xcodebuild -project apps/macos/Moltis.xcodeproj -scheme Moltis -configuration Release -destination "platform=macOS" -derivedDataPath apps/macos/.derivedData-local-validate CODE_SIGNING_ALLOWED=NO buildcargo run -p moltis-schema-export -- apps/ios/GraphQL/Schema/schema.graphqls && ./scripts/generate-ios-graphql.sh && ./scripts/generate-ios-project.sh && xcodebuild -project apps/ios/Moltis.xcodeproj -scheme Moltis -configuration Debug -destination "generic/platform=iOS" CODE_SIGNING_ALLOWED=NO buildjust release-preflightcargo nextest run -p moltis-acp -p moltis-sessions -p moltis-mcp -p moltis-gatewaycargo nextest run -p moltis-agents -p moltis-chatcargo nextest run -p moltis-providers -p moltis-mcp -p moltis-gatewaycargo nextest run -p moltis --test acp_stdiocargo nextest run -p moltis-qmdRemaining
Manual QA
moltis acp, send ACPinitialize,session/new, andsession/promptrequests, and verify stdout contains only JSON-RPC while updates stream before the final stop reason.acp:session and verify bounded history replay; confirm an out-of-namespace session ID is rejected before backend access.RUST_LOG=traceand verify prompts, provider bodies, MCP payloads, and tool values are absent from stderr and the in-memory log buffer.