Refactor/phase 2 architecture extraction - #178
Conversation
… agent_chunked.go
📝 WalkthroughWalkthroughThe change adds an ACP server, extracts shared context utilities, moves background-job contracts into ChangesACP server and session integration
Shared context utilities
Background jobs package migration
Centralized model discovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ACPClient
participant ACPServer
participant AgentSession
participant ACPView
ACPClient->>ACPServer: initialize and create session
ACPServer->>AgentSession: run prompt
AgentSession->>ACPView: emit agent events
ACPView->>ACPServer: send session updates
ACPServer->>ACPClient: return prompt result and updates
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
internal/agent/agent_truncation.go (1)
45-46: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCompute the line-cut offset once.
agentctx.FindLineCutBytePos(result, maxLines)runs twice with the same arguments.resultis an untruncated tool output, so the second scan repeats work over a large string. Hoist the value.♻️ Proposed refactor
- if lineCapped && (!byteCapped || agentctx.FindLineCutBytePos(result, maxLines) <= maxBytes) { - cutIdx = agentctx.FindLineCutBytePos(result, maxLines) + lineCutPos := agentctx.FindLineCutBytePos(result, maxLines) + if lineCapped && (!byteCapped || lineCutPos <= maxBytes) { + cutIdx = lineCutPos🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/agent_truncation.go` around lines 45 - 46, In the truncation logic around lineCapped, compute agentctx.FindLineCutBytePos(result, maxLines) once and store the offset in a local variable before the condition. Reuse that variable both for the byte-limit comparison and the cutIdx assignment, preserving the existing truncation behavior.internal/agent/context/tokens.go (1)
48-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose the summary template as a function, not a package variable.
SummaryTemplateis a mutable package-level variable. Any importer can overwrite it at run time. The coding guidelines forbid globals outside the allowed build-time, serve-mode, role-registry, and OpenTelemetry metric sets. The underlyingprompts.SummaryTemplate()is already a function, so a thin wrapper keeps the same cost.Note:
internal/agent/context_manager.goline 371 usesagentctx.SummaryTemplate; update that call site toagentctx.SummaryTemplate().As per coding guidelines: "Do not use globals except the explicitly allowed build-time, serve-mode, role-registry, and initialized OpenTelemetry metric variables."
♻️ Proposed refactor
-// SummaryTemplate is the structured Markdown prompt sent to the compact -// provider. It is loaded from the embedded prompts package. -var SummaryTemplate = prompts.SummaryTemplate() +// SummaryTemplate returns the structured Markdown prompt sent to the compact +// provider. It is loaded from the embedded prompts package. +func SummaryTemplate() string { return prompts.SummaryTemplate() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/context/tokens.go` around lines 48 - 50, Replace the mutable package-level SummaryTemplate variable with a SummaryTemplate() function that returns prompts.SummaryTemplate(), preserving the existing summary-template behavior. Update the context_manager.go call site to invoke agentctx.SummaryTemplate() instead of reading it as a value, and remove the global variable declaration.Source: Coding guidelines
internal/agent/agent_context.go (1)
20-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFunction re-exports use mutable package variables in three files. The extraction re-exports moved symbols with
var name = pkg.Func. Each entry is a mutable package-level variable that any code in the same package can reassign at run time, and the coding guidelines forbid globals outside the allowed build-time, serve-mode, role-registry, and OpenTelemetry metric sets. Wrapper functions give the same compatibility surface without the mutable state.
internal/agent/agent_context.go#L20-L28: replace the seven function-valued variables with wrapper functions that call theagentctxequivalents.internal/agent/agent_truncation.go#L18-L18: replacevar cleanTruncatedDir = agentctx.CleanTruncatedDirwithfunc cleanTruncatedDir(dir string) { agentctx.CleanTruncatedDir(dir) }, or delete it and update the test call sites, because line 67 already callsagentctx.CleanTruncatedDirdirectly.internal/tools/subagent_aliases.go#L18-L30: convert the ten function entries to wrapper functions and keepErrStuckChildas the only variable in the block.As per coding guidelines: "Do not use globals except the explicitly allowed build-time, serve-mode, role-registry, and initialized OpenTelemetry metric variables."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/agent_context.go` around lines 20 - 28, Replace the mutable function-valued re-exports with wrapper functions: in internal/agent/agent_context.go lines 20-28, wrap all seven agentctx functions; in internal/agent/agent_truncation.go line 18, wrap or remove cleanTruncatedDir, updating test call sites if removed; and in internal/tools/subagent_aliases.go lines 18-30, wrap all ten functions while retaining ErrStuckChild as the only variable. Preserve existing signatures and delegation behavior.Source: Coding guidelines
internal/agent/context/prune.go (1)
52-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCut
firstLineon a rune boundary.
firstLine[:120]slices bytes. If byte 120 falls inside a multi-byte UTF-8 sequence, the stub contains a corrupted rune. The same package already provides rune-safe truncation for this reason.♻️ Proposed refactor
- if len(firstLine) > 120 { - firstLine = firstLine[:120] + "..." - } + if r := []rune(firstLine); len(r) > 120 { + firstLine = string(r[:120]) + "..." + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/context/prune.go` around lines 52 - 54, Update the firstLine truncation logic in the surrounding pruning function to use the package’s existing rune-safe truncation helper instead of byte slicing with firstLine[:120]. Preserve the 120-character limit and existing ellipsis behavior while ensuring the result never splits a UTF-8 rune.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/acp/ctrl.go`:
- Around line 44-45: Update the forwarder handling of CtrlDone in the
control-loop function so it does not return while ctrlCh remains open; continue
draining subsequent control messages until ctx.Done(). Preserve normal
forwarding behavior for other control types and avoid blocking senders after
prompt completion.
In `@internal/acp/server.go`:
- Around line 277-305: Update the tools/call and session/prompt handlers to
check json.Unmarshal errors and return JSON-RPC -32602 for malformed parameters
instead of executing with empty values. In the tools/call path, run reg.Execute
asynchronously so the dispatch loop remains available for cancellation and other
requests, then write its response on completion. Create a per-call context with
the established/configured tool timeout and use it for Execute.
- Around line 213-233: Update the prompt lifecycle around RunPrompt and
currentPromptCancel by tracking the active prompt with a currentPromptDone
completion channel. When replacing a prompt, cancel it, capture its completion
channel, unlock promptMu, wait for completion, then relock before installing the
new view and control channel; close the new prompt’s completion channel when
RunPrompt returns, ensuring no old events or control-channel sends overlap the
swap.
- Around line 75-100: In the question-tool setup, replace the unchecked
assertion in the `qtp` initialization with a comma-ok type assertion and only
assign `Handler` when the registered tool is a `*tools.QuestionTool`. Update the
handler’s auto-answer logic to honor `Server.AutoAnswerQuestions`, preserving
the existing first-option answers only when enabled and returning the
appropriate non-auto-answer behavior when disabled; run gofmt afterward.
In `@internal/acp/types.go`:
- Around line 27-33: Update the ACP payload definitions and emitters, including
InitializeResult, ToolListEntry, and session update handling, to use ACP wire
names: protocolVersion, agentCapabilities, agentInfo, inputSchema, toolCallId,
and rawOutput. Replace nested tool_call properties with the tool_call
sessionUpdate value, and update the handshake protocol version to the ACP
version while preserving the existing payload behavior.
In `@internal/acp/view.go`:
- Around line 45-68: Update the ToolEndEvent handling to use e.ID when assigning
ToolResult.ID instead of v.curToolID.Load(). Preserve the existing result fields
and summary behavior while ensuring concurrent tool results correlate with their
own tool calls.
In `@internal/agent/context_manager.go`:
- Line 393: Update the guard in the compaction flow around oldMsgs to compare
compatible units: estimate the token count of oldMsgs and compare that value
with agentctx.MinChunkTokens, or introduce and use a dedicated message-count
threshold if the intended trigger is message-based. Ensure the
chunked-compaction fallback runs according to the intended threshold after
single-shot summarization fails.
In `@internal/agent/context/tokens.go`:
- Around line 19-23: Update the comment for DefaultRawCompactionThreshold to
state the correct 25% threshold, replacing the inaccurate 50% description while
preserving the rest of the documentation.
---
Nitpick comments:
In `@internal/agent/agent_context.go`:
- Around line 20-28: Replace the mutable function-valued re-exports with wrapper
functions: in internal/agent/agent_context.go lines 20-28, wrap all seven
agentctx functions; in internal/agent/agent_truncation.go line 18, wrap or
remove cleanTruncatedDir, updating test call sites if removed; and in
internal/tools/subagent_aliases.go lines 18-30, wrap all ten functions while
retaining ErrStuckChild as the only variable. Preserve existing signatures and
delegation behavior.
In `@internal/agent/agent_truncation.go`:
- Around line 45-46: In the truncation logic around lineCapped, compute
agentctx.FindLineCutBytePos(result, maxLines) once and store the offset in a
local variable before the condition. Reuse that variable both for the byte-limit
comparison and the cutIdx assignment, preserving the existing truncation
behavior.
In `@internal/agent/context/prune.go`:
- Around line 52-54: Update the firstLine truncation logic in the surrounding
pruning function to use the package’s existing rune-safe truncation helper
instead of byte slicing with firstLine[:120]. Preserve the 120-character limit
and existing ellipsis behavior while ensuring the result never splits a UTF-8
rune.
In `@internal/agent/context/tokens.go`:
- Around line 48-50: Replace the mutable package-level SummaryTemplate variable
with a SummaryTemplate() function that returns prompts.SummaryTemplate(),
preserving the existing summary-template behavior. Update the context_manager.go
call site to invoke agentctx.SummaryTemplate() instead of reading it as a value,
and remove the global variable declaration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 67dbf6de-f9d6-44bc-9173-12ef253f1f99
📒 Files selected for processing (37)
AGENTS.mdcmd/yaah/acp.gocmd/yaah/acp_cmd.gocmd/yaah/acp_view.gocmd/yaah/provider_resolve.gocmd/yaah/session.gocmd/yaah/tui.gocmd/yaah/tui2.gocmd/yaah/web.gocmd/yaah/wiring.gointernal/acp/ctrl.gointernal/acp/server.gointernal/acp/types.gointernal/acp/view.gointernal/agent/agent_context.gointernal/agent/agent_context_test.gointernal/agent/agent_truncation.gointernal/agent/builder.gointernal/agent/context/chunked.gointernal/agent/context/prune.gointernal/agent/context/split.gointernal/agent/context/tokens.gointernal/agent/context/truncation.gointernal/agent/context_manager.gointernal/agent/options.gointernal/agent/task_test.gointernal/agent/types.gointernal/jobs/context.gointernal/jobs/manager.gointernal/jobs/manager_test.gointernal/jobs/output.gointernal/jobs/types.gointernal/providers/models.gointernal/tools/subagent_aliases.gointernal/tools/subagent_jobs.gointernal/tools/task.gointernal/tools/task_test.go
💤 Files with no reviewable changes (2)
- cmd/yaah/acp.go
- cmd/yaah/acp_view.go
| case *types.CtrlDone: | ||
| return |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
CtrlDone stops the forwarder while the channel stays open.
The return at line 45 ends the goroutine. internal/acp/server.go line 224 creates ctrlCh with a 64-slot buffer and never closes it. Any later control message has no reader. Once the buffer fills, the sender blocks.
Drain the channel until ctx.Done(), or make the server close and replace the channel when the prompt completes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/acp/ctrl.go` around lines 44 - 45, Update the forwarder handling of
CtrlDone in the control-loop function so it does not return while ctrlCh remains
open; continue draining subsequent control messages until ctx.Done(). Preserve
normal forwarding behavior for other control types and avoid blocking senders
after prompt completion.
| promptCtx, promptCancel := context.WithCancel(ctx) | ||
|
|
||
| promptMu.Lock() | ||
| if currentPromptCancel != nil { | ||
| currentPromptCancel() | ||
| } | ||
| currentPromptCancel = promptCancel | ||
| promptMu.Unlock() | ||
|
|
||
| wrapped := NewViewWithWrite(sendUpdate, sessionID) | ||
|
|
||
| ctrlCh := make(chan types.CtrlMsg, 64) | ||
| s.sess.SetView(wrapped) | ||
| s.sess.SetCtrlCh(ctrlCh) | ||
|
|
||
| go s.forwardCtrl(promptCtx, ctrlCh, sessionID, sendUpdate) | ||
|
|
||
| go func(sID string) { | ||
| resp, _, runErr := s.sess.RunPrompt(promptCtx, promptText) | ||
| promptResults <- promptResult{sessionID: sID, response: resp, err: runErr} | ||
| }(sessionID) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
A new prompt replaces the session view and control channel before the previous prompt stops.
Line 217 cancels the previous prompt context but does not wait for RunPrompt to return. Lines 225-226 then install a new view and a new control channel. The previous run continues for some time and publishes its remaining events into the new session's view. The client receives updates from the old prompt that are tagged with the new session ID.
The old ctrlCh also has no reader after the old forwardCtrl returns. Any late sender to that channel blocks once the 64-slot buffer is full.
Track the running prompt with a completion signal and wait for it before you install the new view and control channel.
♻️ Sketch of the wait-before-swap change
promptMu.Lock()
if currentPromptCancel != nil {
currentPromptCancel()
}
+ if currentPromptDone != nil {
+ <-currentPromptDone
+ }
currentPromptCancel = promptCancel
+ pdone := make(chan struct{})
+ currentPromptDone = pdone
promptMu.Unlock()
wrapped := NewViewWithWrite(sendUpdate, sessionID)
ctrlCh := make(chan types.CtrlMsg, 64)
s.sess.SetView(wrapped)
s.sess.SetCtrlCh(ctrlCh)
go s.forwardCtrl(promptCtx, ctrlCh, sessionID, sendUpdate)
go func(sID string) {
+ defer close(pdone)
resp, _, runErr := s.sess.RunPrompt(promptCtx, promptText)
promptResults <- promptResult{sessionID: sID, response: resp, err: runErr}
}(sessionID)Declare currentPromptDone chan struct{} next to currentPromptCancel. Do not hold promptMu while you wait; capture the channel, unlock, wait, then relock.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/acp/server.go` around lines 213 - 233, Update the prompt lifecycle
around RunPrompt and currentPromptCancel by tracking the active prompt with a
currentPromptDone completion channel. When replacing a prompt, cancel it,
capture its completion channel, unlock promptMu, wait for completion, then
relock before installing the new view and control channel; close the new
prompt’s completion channel when RunPrompt returns, ensuring no old events or
control-channel sends overlap the swap.
| case "tools/call": | ||
| var params struct { | ||
| Name string `json:"name"` | ||
| Arguments map[string]any `json:"arguments"` | ||
| } | ||
| if len(msg.Params) > 0 { | ||
| json.Unmarshal(msg.Params, ¶ms) | ||
| } | ||
|
|
||
| var resultData json.RawMessage | ||
| if reg := s.sess.ToolReg(); reg != nil { | ||
| argsJSON, _ := json.Marshal(params.Arguments) | ||
| r, err := reg.Execute(ctx, params.Name, string(argsJSON)) | ||
| blocks := []map[string]any{ | ||
| {"type": "text", "text": r}, | ||
| } | ||
| isErr := err != nil | ||
| if isErr { | ||
| blocks[0]["text"] = err.Error() | ||
| } | ||
| result := map[string]any{ | ||
| "content": blocks, | ||
| "isError": isErr, | ||
| } | ||
| resultData, _ = json.Marshal(result) | ||
| } else { | ||
| resultData = json.RawMessage(`{"content":[{"type":"text","text":"session not ready"}],"isError":true}`) | ||
| } | ||
| writeMsg(Message{JSONRPC: "2.0", ID: msg.ID, Result: resultData}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Tool execution blocks the dispatch loop.
Line 289 calls reg.Execute on the reader goroutine. The loop cannot read the next line until the tool returns. A long-running tool therefore blocks session/cancel, session/prompt, and every other method. The call also uses the outer ctx with no per-call deadline.
Run the tool in a goroutine and write the response when it completes. Add a timeout for the tool context.
Line 283 also discards the json.Unmarshal error. Malformed params then produce an empty Name, and the server executes a lookup for the empty tool name instead of returning -32602. The same applies to line 191 for session/prompt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/acp/server.go` around lines 277 - 305, Update the tools/call and
session/prompt handlers to check json.Unmarshal errors and return JSON-RPC
-32602 for malformed parameters instead of executing with empty values. In the
tools/call path, run reg.Execute asynchronously so the dispatch loop remains
available for cancellation and other requests, then write its response on
completion. Create a per-call context with the established/configured tool
timeout and use it for Execute.
| // InitializeResult is the result of the initialize handshake. | ||
| type InitializeResult struct { | ||
| ProtocolVersion string `json:"protocol_version"` | ||
| Capabilities ServerCaps `json:"capabilities"` | ||
| ServerInfo ServerInfo `json:"server_info"` | ||
| Instructions string `json:"instructions,omitempty"` | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Agent Client Protocol sessionUpdate toolCall JSON field naming schema
💡 Result:
In the Agent Client Protocol (ACP), tool calls are managed through the session/update method [1][2]. The JSON field naming schema for tool call updates is defined by the ToolCallUpdate object [2][3]. The core structure for reporting or updating a tool call uses the sessionUpdate field set to "tool_call_update" [1][4]. All fields within this object are optional except for toolCallId, which is required to identify the specific tool call being created or patched [2][3][4]. The standard properties for a ToolCallUpdate include: toolCallId: The unique identifier for the tool call [2][3]. name: An optional string representing the programmatic name of the tool (e.g., "read_file") [5]. title: A human-readable title describing the tool's action [2][3]. kind: A category defining the tool's type (e.g., "read", "edit", "search", "execute") [2][6]. status: The current execution status (e.g., "pending", "in_progress", "completed") [2][3]. content: An array of ToolCallContent items [2][3]. locations: An array of ToolCallLocation items [2][3]. rawInput: The raw input parameters provided to the tool [2][3]. rawOutput: The raw output returned by the tool [2][3]. _meta: A reserved object for additional metadata [3][7]. When performing updates, the protocol follows upsert semantics: if the toolCallId is new, the client creates the entry; if the ID already exists, the client patches the fields included in the update [1][4]. Omitted fields generally leave the existing value unchanged, while setting a field to null explicitly clears or unsets the value [1][3][5]. For incremental output, the protocol also supports a sessionUpdate value of "tool_call_content_chunk", which uses a required toolCallId and a single content item [8][4].
Citations:
- 1: https://agentclientprotocol.com/protocol/v2/tool-calls
- 2: https://github.com/zed-industries/agent-client-protocol/blob/4f589532/docs/protocol/schema.mdx
- 3: https://agentclientprotocol.github.io/typescript-sdk/types/ToolCallUpdate.html
- 4: https://agentclientprotocol.com/rfds/v2/tool-call-updates
- 5: https://agentclientprotocol.com/rfds/tool-call-name
- 6: https://github.com/zed-industries/agent-client-protocol/blob/4f589532/docs/protocol/tool-calls.mdx
- 7: https://agentclientprotocol.com/protocol/v1/schema
- 8: https://agentclientprotocol.com/protocol/v2/prompt-lifecycle
🏁 Script executed:
sed -n '1,130p' internal/acp/types.go
printf '\n-- ACP JSON tags and protocol references --\n'
rg -n 'protocol_version|server_info|input_schema|tool_call|tool_result|sessionId|currentModeId|availableModes|listChanged|sessionUpdate|InitializeResult|ToolListEntry|type Update' internal .github README.md 2>/dev/nullRepository: buchenberg/yaah
Length of output: 18481
🏁 Script executed:
sed -n '140,190p' internal/acp/server.go
sed -n '1,90p' internal/acp/server.go
printf '\n-- ACP documentation and version declarations --\n'
rg -n -i 'agent communication protocol|agent client protocol|ACP|protocol.?version|initialize|session/update|tools/list' README.md docs internal/acp go.mod 2>/dev/nullRepository: buchenberg/yaah
Length of output: 7774
🌐 Web query:
Agent Client Protocol schema InitializeResult protocolVersion serverInfo inputSchema tool_call tool_result protocol_version server_info
💡 Result:
The terms you are inquiring about relate primarily to the Model Context Protocol (MCP), a standardized interface for connecting AI agents to data and tools [1][2]. Note that while there is an "Agent Client Protocol" [3][4], the fields you listed (InitializeResult, protocolVersion, serverInfo, instructions, tool_call, tool_result) are core components of the Model Context Protocol specification [1][5][6]. InitializeResult Schema The InitializeResult is a structured response sent by an MCP server to a client following an initialize request [1][7][2]. It establishes the communication parameters for the session [7]. Its primary fields include [8][5]: protocolVersion (string): The version of the Model Context Protocol that the server intends to use [1][8][5]. This may differ from the version requested by the client [1][9]. If the client cannot support this version, it must disconnect [1][10][5]. capabilities (ServerCapabilities): A definition of the features and capabilities supported by the server (e.g., resources, tools, prompts) [8][5]. serverInfo (Implementation): Metadata identifying the server, typically including its name and version [10][7]. instructions (string, optional): Contextual hints or instructions for the client, which can be used to improve an LLM's understanding of the server’s tools and features [1][8][5]. Protocol Terminology protocolVersion: This field is used during the handshake to negotiate a compatible version of the protocol between the client and server [1][2]. serverInfo: This field provides identifying information about the server implementation, used for logging, debugging, or compatibility checks [7]. tool_call and tool_result: These refer to the mechanism by which agents execute tools. A tool_call is a request from the client to the server to execute a specific tool with defined arguments, while a tool_result is the output returned by the server following that execution [1][6]. For official specifications and schema definitions, please refer to the Model Context Protocol documentation [5][2].
Citations:
- 1: https://raw.githubusercontent.com/modelcontextprotocol/specification/main/schema/2024-11-05/schema.ts
- 2: https://modelcontextprotocol.io/specification/2025-03-26/basic/lifecycle
- 3: https://agentclientprotocol.com/protocol/initialization
- 4: https://github.com/zed-industries/agent-client-protocol/blob/4f589532/docs/protocol/schema.mdx
- 5: https://modelcontextprotocol.io/specification/2025-06-18/schema
- 6: https://github.com/modelcontextprotocol/specification/blob/main/schema/2025-06-18/schema.ts
- 7: https://csharp.sdk.modelcontextprotocol.io/v2/api/ModelContextProtocol.Protocol.InitializeResult.html
- 8: https://docs.rs/rust-mcp-sdk/latest/rust_mcp_sdk/schema/struct.InitializeResult.html
- 9: https://jetbrains.github.io/mcp-kotlin-sdk/-m-c-p%20-kotlin%20-s-d-k/org.jetbrains.kotlinx.mcp/-initialize-result/index.html
- 10: https://kotlin.sdk.modelcontextprotocol.io/kotlin-sdk-core/io.modelcontextprotocol.kotlin.sdk.types/-initialize-result/index.html
🏁 Script executed:
printf '%s\n' '-- remaining ACP types --'
sed -n '120,290p' internal/acp/types.go
printf '%s\n' '-- ACP server tests and JSON expectations --'
fd -i '.*acp.*' internal cmd | sort
rg -n -C 3 'protocol_version|protocolVersion|server_info|serverInfo|input_schema|inputSchema|tool_call|toolCall|tool_result|toolResult|2024-11-05|sessionUpdate' --glob '*_test.go' --glob '*.go' internal/acp cmdRepository: buchenberg/yaah
Length of output: 4210
🏁 Script executed:
curl -L --fail --silent --show-error \
https://raw.githubusercontent.com/zed-industries/agent-client-protocol/main/schema/src/protocol.ts |
rg -n -C 4 'Initialize|protocolVersion|protocol_version|serverInfo|server_info|inputSchema|input_schema|toolCall|tool_call|toolResult|tool_result|sessionUpdate'Repository: buchenberg/yaah
Length of output: 202
🌐 Web query:
site:agentclientprotocol.com/protocol initialization ACP protocolVersion agentInfo agentCapabilities
💡 Result:
In the Agent Client Protocol (ACP), initialization is the foundational phase where the Client and Agent negotiate protocol versions, exchange implementation information, and establish supported capabilities [1][2]. This process ensures compatibility before any session-specific operations can occur [3]. Key components of the initialization exchange include: 1. Protocol Version (protocolVersion): Clients initiate the handshake by specifying the latest protocol version they support [1][2]. The Agent must respond with the chosen version, which is either the one specified by the Client (if supported) or the latest version the Agent supports [4]. If a Client does not support the version returned by the Agent, it should disconnect [4]. 2. Implementation Information (agentInfo / clientInfo): Both parties should provide metadata about their implementation to facilitate debugging, metrics, or UI display [1][5]. This typically includes: - name: A programmatic identifier. - title: A human-readable display name. - version: The implementation version. In v2 of the protocol, this is often structured within an info object [2]. 3. Capabilities (agentCapabilities / clientCapabilities): These advertise the features, transport methods (such as stdio or HTTP for MCP servers), and content types supported by the implementation [1][3]. During initialization, implementations may also advertise custom capabilities using a _meta field to maintain extensibility without breaking core compatibility [6]. All capabilities provided during initialization are considered optional, and implementations should be prepared to handle various combinations [2]. Additionally, agents may advertise supported authentication methods (authMethods) during this phase to indicate the availability of authentication services [7].
Citations:
- 1: https://agentclientprotocol.com/protocol/initialization
- 2: https://agentclientprotocol.com/protocol/v2/initialization
- 3: https://agentclientprotocol.com/protocol/v2/session-setup
- 4: https://agentclientprotocol.com/protocol/v2/schema
- 5: https://agentclientprotocol.com/protocol/v1/initialization
- 6: https://agentclientprotocol.com/protocol/v2/extensibility
- 7: https://agentclientprotocol.com/protocol/v2/authentication
🏁 Script executed:
curl -L --fail --silent --show-error \
https://raw.githubusercontent.com/zed-industries/agent-client-protocol/4f589532/docs/protocol/schema.mdx |
rg -n -C 5 'Initialize|protocolVersion|agentCapabilities|agentInfo|inputSchema|input_schema|toolCall|tool_call|toolResult|tool_result|sessionUpdate'Repository: buchenberg/yaah
Length of output: 6741
🏁 Script executed:
curl -L --fail --silent --show-error \
https://raw.githubusercontent.com/zed-industries/agent-client-protocol/4f589532/docs/protocol/v1/schema.mdx |
rg -n -C 5 'Initialize|protocolVersion|agentCapabilities|agentInfo|inputSchema|input_schema|toolCall|tool_call|toolResult|tool_result|sessionUpdate'Repository: buchenberg/yaah
Length of output: 202
Use the ACP wire schema for all ACP payloads.
InitializeResult emits MCP-style fields and version "2024-11-05" instead of ACP’s protocolVersion, agentCapabilities, and agentInfo. ToolListEntry must use inputSchema. Session updates must use ACP fields such as toolCallId and rawOutput; tool_call is a sessionUpdate value, not a nested tool_call property. These mismatches prevent ACP clients from decoding the handshake, tool definitions, and tool updates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/acp/types.go` around lines 27 - 33, Update the ACP payload
definitions and emitters, including InitializeResult, ToolListEntry, and session
update handling, to use ACP wire names: protocolVersion, agentCapabilities,
agentInfo, inputSchema, toolCallId, and rawOutput. Replace nested tool_call
properties with the tool_call sessionUpdate value, and update the handshake
protocol version to the ACP version while preserving the existing payload
behavior.
| resp, err := compactProvider.Send(ctx, req) | ||
| if err != nil || len(resp.Choices) == 0 || resp.Choices[0].Message.Content == "" { | ||
| if len(oldMsgs) > minChunkTokens { | ||
| if len(oldMsgs) > agentctx.MinChunkTokens { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
len(oldMsgs) is a message count, but MinChunkTokens is a token budget.
The guard compares the number of messages against a token floor of 1000. A conversation rarely holds more than 1000 messages, so the chunked-compaction fallback almost never runs after a failed single-shot summarization. The extraction did not create this behavior, but the new constant name makes the unit mismatch explicit. Confirm the intended trigger. A token estimate, or a dedicated message-count constant, matches the intent.
🐛 Proposed fix using a token estimate
- if len(oldMsgs) > agentctx.MinChunkTokens {
+ if agentctx.PreflightTokens(oldMsgs, nil, cm.EstimateFactor) > agentctx.MinChunkTokens {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(oldMsgs) > agentctx.MinChunkTokens { | |
| if agentctx.PreflightTokens(oldMsgs, nil, cm.EstimateFactor) > agentctx.MinChunkTokens { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agent/context_manager.go` at line 393, Update the guard in the
compaction flow around oldMsgs to compare compatible units: estimate the token
count of oldMsgs and compare that value with agentctx.MinChunkTokens, or
introduce and use a dedicated message-count threshold if the intended trigger is
message-based. Ensure the chunked-compaction fallback runs according to the
intended threshold after single-shot summarization fails.
…sults by event ID, wrappers over alias vars
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/subagent_aliases.go`:
- Around line 23-24: Replace all callers of ErrStuckChild in the internal tools
package with the canonical jobs.ErrStuckChild reference, then remove the
ErrStuckChild variable declaration from the aliases file. Preserve errors.Is
checks and other sentinel identity behavior without introducing another alias.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33a1529e-85a2-472a-9fa1-39390774d88c
📒 Files selected for processing (10)
README.mddocs/architecture.mdinternal/acp/server.gointernal/acp/view.gointernal/agent/agent_context.gointernal/agent/agent_truncation.gointernal/agent/context/prune.gointernal/agent/context/tokens.gointernal/agent/context_manager.gointernal/tools/subagent_aliases.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/agent/context/prune.go
- internal/agent/agent_context.go
- internal/agent/context_manager.go
- internal/agent/agent_truncation.go
- internal/acp/server.go
| // ErrStuckChild stays a variable alias so errors.Is identity is preserved. | ||
| var ErrStuckChild = jobs.ErrStuckChild |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 '\bErrStuckChild\b' --glob '*.go'
printf '\nAssignments:\n'
rg -nP '^\s*(?:var\s+)?(?:[[:alnum:]_]+\.)?ErrStuckChild\s*=' --glob '*.go' || trueRepository: buchenberg/yaah
Length of output: 267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate file ---'
cat -n internal/tools/subagent_aliases.go
printf '%s\n' '--- ErrStuckChild references ---'
rg -n -C 4 '\bErrStuckChild\b' --glob '*.go' . || true
printf '%s\n' '--- package-level declarations in candidate file ---'
sed -n '1,80p' internal/tools/subagent_aliases.go
printf '%s\n' '--- module and package context ---'
rg -n '^(module|go )' go.mod 2>/dev/null || true
fd -t f -e go . | rg '(^|/)(jobs|subagent|tools)' | head -80Repository: buchenberg/yaah
Length of output: 9028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- targeted diff ---'
git diff -- internal/tools/subagent_aliases.go internal/agent/agent_tools.go internal/tools/task.go
printf '%s\n' '--- relevant declarations and imports ---'
cat -n internal/jobs/context.go | sed -n '95,115p'
sed -n '1,55p' internal/agent/agent_tools.go
sed -n '215,238p' internal/agent/agent_tools.go
sed -n '1,35p' internal/tools/task.go
printf '%s\n' '--- all tools.ErrStuckChild references ---'
rg -n -C 2 'tools\.ErrStuckChild|jobs\.ErrStuckChild' --glob '*.go' . || trueRepository: buchenberg/yaah
Length of output: 6700
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
decls = []
assigns = []
for path in Path(".").rglob("*.go"):
if any(part in {".git", "vendor"} for part in path.parts):
continue
lines = path.read_text(errors="replace").splitlines()
depth = 0
in_block_comment = False
for lineno, raw in enumerate(lines, 1):
line = raw.strip()
if in_block_comment:
if "*/" in line:
in_block_comment = False
continue
if line.startswith("/*"):
if "*/" not in line[2:]:
in_block_comment = True
continue
if line.startswith("//") or not line:
continue
# A top-level declaration has brace depth zero. This is a focused
# read-only check for the ErrStuckChild identifier.
if depth == 0 and re.search(r"\bvar\s+ErrStuckChild\b", line):
decls.append((str(path), lineno, raw))
if re.search(r"(?:^|[;\s])(?:ErrStuckChild|tools\.ErrStuckChild|jobs\.ErrStuckChild)\s*=", line):
assigns.append((str(path), lineno, raw))
depth += line.count("{") - line.count("}")
print("package-level ErrStuckChild declarations:")
for item in decls:
print("%s:%d:%s" % item)
print("assignment-shaped ErrStuckChild lines:")
for item in assigns:
print("%s:%d:%s" % item)
PYRepository: buchenberg/yaah
Length of output: 624
Remove the unapproved mutable global.
internal/tools/subagent_aliases.go:24 duplicates jobs.ErrStuckChild as a mutable variable. Migrate the caller to jobs.ErrStuckChild, then remove this declaration to keep one canonical sentinel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tools/subagent_aliases.go` around lines 23 - 24, Replace all callers
of ErrStuckChild in the internal tools package with the canonical
jobs.ErrStuckChild reference, then remove the ErrStuckChild variable declaration
from the aliases file. Preserve errors.Is checks and other sentinel identity
behavior without introducing another alias.
Source: Coding guidelines
1 participant
yaah Architecture Phase 2: Package Extraction
What changed
Phase 2A —
internal/agent/context/(new leaf package)Pure context-management helpers extracted from
agent_context.go,agent_chunked.go, andagent_truncation.go:tokens.go— token/payload estimation:MessageTokens,PreflightTokens,EstimatePayloadBytes,LastUserPrompt,CountReasoningMessages, plus the compaction threshold/budget constantssplit.go— turn segmentation and compaction split logic:Turns,SplitTail,SplitTurn,PreserveBudget,ProtectReasoningTurns,EarliestReasoningIndex,TruncateRunesprune.go—PruneMessages,FormatToolStubchunked.go/truncation.go— chunked-compaction constants +ChunkSplit; tool-result truncation limits +FindLineCutBytePos,CleanTruncatedDiragent_chunked.godeleted;*Loopmethods (compactContext,ForceCompact,trimContext,truncateToolResult, etc.) stay inagent/per Go's method rulesProtectReasoningTurnskept as an exported wrapperContextManager.Reset()(zero callers)Phase 2B —
internal/jobs/(new package)The background sub-agent cluster moved out of
internal/tools:manager.go—BackgroundJobs(wastools/background.go, byte-identical, tests moved too)types.go,output.go,context.go—TaskRunner,SubAgentParams, escalation contract, and the sub-agent context-key helpers (now using a package-localcontextKey; all readers/writers funnel throughjobs.*so there is no split-brain key lookup)agentandcmd/yaahreferencejobs.BackgroundJobsdirectly;toolskeeps transparent aliases for the sub-agent I/O contract (TaskRunner,SubAgentParams,Escalation, ctx helpers,ErrStuckChild) soagent/runner, TUI, and event consumers are untouchedPhase 2C —
internal/providers/models.gofetchAllModelsmoved out ofcmd/yaah/tui.goasproviders.FetchAllModels, behind a narrowModelListerinterface with an injected provider-constructor callbacktui.go,tui2.go,web.go) now share it via amakeModelListeradapter inprovider_resolve.goPhase 2D —
internal/acp/(new package)The ACP (Agent Communication Protocol) server extracted from
cmd/yaah:types.go— all 18 JSON-RPC wire types (byte-identical JSON tags)view.go—View/ViewWithWriteevent translatorserver.go—Serverstruct + dispatch loop, depending on a narrowSessioninterfacectrl.go— control-channel translator; auto-answer and auto-continue are now overridableServerfields (defaulttrue, preserving old behavior)cmd/yaah/acp_cmd.gois now a ~30-line cobra shim;agentSessiongainedToolReg()Dependency improvements (verified)
internal/toolsno longer importsagent/subagentinternal/agent/contextis a true leaf — nointernal/agentimportsinternal/jobsis a near-leaf — notools/agentimportsinternal/acpis independently testable; policy decisions injectableVerification
go build ./...,go test ./...,go vet ./...,gofmt -l .,staticcheck— all cleanNotes
AGENTS.mdupdated for the new layoutSummary by CodeRabbit
New Features
Documentation