Conversation
…th JSON-RPC support
…lient from the model
…AcpServer base structure this is just a first pass, significant refactoring is pending
…to simplify server handleTurnUpdate logic
…specified during tool construction
oca-agent
left a comment
oca-agent
left a comment
There was a problem hiding this comment.
Code Review Summary
Overall, this is a solid implementation. Refactoring Agent -> Session clarifies the codebase architecture (distinguishing the overall agent application/service from an individual context window session), and the new acp module is clean, well-structured, and thoroughly tested. The compile-time enum string mapping (session_new <-> session/new) is particularly slick with zero runtime allocation overhead.
Below are inline comments highlighting a panic hazard, error handling edge cases, and protocol robustness suggestions to help guide the implementation before taking this out of draft.
| .json_rpc_writer = &json_rpc_writer, | ||
| }; | ||
|
|
||
| _ = try session.session.executeTurnStreaming(.{ .prompt = client_request.params.session_prompt.prompt[0].text }, handleTurnUpdate, &ctx); |
There was a problem hiding this comment.
🔴 Panic Hazard & Prompt Handling:
- If a client sends an empty
promptarray (prompt: []), indexingprompt[0]directly will panic at runtime. Consider validatingprompt.len > 0first or returning a JSON-RPC error. - ACP prompt turns can contain multiple content blocks (e.g. text blocks alongside embedded context or resource links). Currently, only
prompt[0]is evaluated. Consider joining or handling all blocks inclient_request.params.session_prompt.prompt.
| } else if (T == Io) { | ||
| args[func_idx] = io; | ||
| } else if (T == ?*anyopaque) { | ||
| args[func_idx] = ctx.?; |
There was a problem hiding this comment.
T is already ?*anyopaque, so unwrapping ctx.? forces ctx to be non-null and will panic if null is ever passed.
Assigning args[func_idx] = ctx; directly avoids the forced unwrap.
| const arena_allocator = arena.allocator(); | ||
|
|
||
| while (true) { | ||
| try self.io.checkCancel(); |
There was a problem hiding this comment.
err terminates the main server loop.
Catching request-level errors inside the loop and sending standard JSON-RPC error responses back to the client will prevent standard client errors from crashing the server process.
| // TODO: We assume that the first header (in LSP-style JSON-RPC) is "Content-Length". | ||
| // This may not always be true. "Content-Type" is another common header and the | ||
| // spec does not specify ordering. Additionally, the headers are not case-sensitive. | ||
| if (std.mem.startsWith(u8, line, "Content-Length:")) { |
There was a problem hiding this comment.
💡 Header case sensitivity: std.mem.startsWith(u8, line, "Content-Length:") is case-sensitive. Using std.ascii.startsWithIgnoreCase(line, "content-length:") will make header parsing more robust against different client implementations.
| } || std.json.Error; | ||
|
|
||
| const ServerSessionContext = struct { | ||
| server: *Server, |
There was a problem hiding this comment.
💡 Unused field: server: *Server in ServerSessionContext isn't read in handleTurnUpdate. If it's not needed for future notification handlers, it can be removed.
2 participants
Summary
This PR implements the initial Agent Communication Protocol (ACP) server specification over JSON-RPC 2.0. It enables client applications to communicate with
comaas an agent service over stdio/JSON-RPC, managing agent sessions, executing turns, and streaming thought and message content back to clients in real-time.Additionally, this PR refactors core agent types to align with ACP session semantics (
Agent→Session) and introduces support for context-aware tool execution.