feat(acp): add complete hooks support for ACP integration#3248
Merged
tanzhenxin merged 5 commits intomainfrom Apr 16, 2026
Merged
feat(acp): add complete hooks support for ACP integration#3248tanzhenxin merged 5 commits intomainfrom
tanzhenxin merged 5 commits intomainfrom
Conversation
Contributor
📋 Review SummaryThis PR implements comprehensive hooks support for ACP (Agent Client Protocol) integration, bringing parity with the core CLI path. The implementation covers session lifecycle hooks (SessionStart/SessionEnd), prompt interception (UserPromptSubmit), tool execution hooks (PreToolUse/PostToolUse/PostToolUseFailure), and Stop/StopFailure hooks. The changes are well-structured and align with the existing core implementation patterns. 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
4 tasks
tanzhenxin
reviewed
Apr 15, 2026
Collaborator
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
Well-structured implementation — the stop hook loop extraction is clean, test helpers are a nice refactor, and using core firePreToolUseHook/firePostToolUseHook helpers rather than reimplementing is the right call.
Issues
-
SessionEnd fires twice on SIGTERM. The shutdown handler fires
SessionEnd(Other), then destroys stdio. Destroying streams closes the ACP transport, causingconnection.closedto resolve, which firesSessionEnd(PromptInputExit)a second time. TheshuttingDownguard only prevents handler re-entry — it doesn't prevent the post-connection-close path. Suggestion: add asessionEndFiredflag that short-circuits the second fire. -
Missing
permissionModeon SessionStart.fireSessionStartEvent(source, model)omits the third argument. The core CLI passesString(config.getApprovalMode()). Hook scripts filtering on permission mode will silently fall back todefaultfor all ACP sessions. -
PostToolUseFailure not fired for returned errors. The failure hook only fires from the
catchpath (thrown exceptions). When a tool returnstoolResult.error— the standard error path — the hook is silently skipped. The core scheduler handles both paths (coreToolScheduler.ts). -
PostToolUse response shape diverges from core. ACP sends
{ success, resultDisplay }while core sends{ llmContent, returnDisplay }. Hooks inspecting tool output will see different data depending on CLI vs ACP.
Verdict
COMMENT — The implementation is solid but these contract divergences undermine the parity goal. Items 1-3 are behavioral bugs; item 4 is a consistency gap. Worth addressing before merge.
Collaborator
Author
Resolve 1,2,3,4 |
tanzhenxin
approved these changes
Apr 16, 2026
Collaborator
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
Well-structured implementation — all four critical issues from the previous review are properly addressed. SessionEnd double-fire is prevented with sessionEndFired guard, SessionStart now passes permissionMode, PostToolUseFailure fires for both returned errors and caught exceptions, and PostToolUse response shape matches the core path.
Suggestions (non-blocking)
- PostToolUseFailure on interrupt: The catch path passes the already-aborted
abortSignaltofirePostToolUseFailureHook— the core path omits the signal for interrupts. May cause the hook to silently short-circuit in ACP mode. - PostToolUseFailure additionalContext: Both error paths log
additionalContextbut don't append it to the error message like the core path does. - Async shutdown handler:
fireSessionEndOnce()is awaited in the SIGTERM handler — if the SessionEnd hook hangs, process exit is delayed.
Verdict
APPROVE — Hook lifecycle is now well-aligned with the core CLI path. The remaining items are minor and can be handled in follow-ups.
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.
TLDR
Add complete hooks support for ACP integration, aligning with the core path implementation. This includes lifecycle hooks (SessionStart/SessionEnd), prompt interception (UserPromptSubmit), tool execution hooks (PreToolUse/PostToolUse/PostToolUseFailure), and Stop/StopFailure hooks.
Screenshots / Video Demo
Stop Hook:
v-stop.mp4
SessionStart and SessionEnd:
4.14.mp4
UserPromptSubmit:
PreToolUse:
PostToolUse:
Dive Deeper
This PR implements the full hooks lifecycle for ACP sessions, bringing parity with the core CLI path:
Session Lifecycle:
SessionStarthook fires when a new ACP session beginsSessionEndhook fires on session termination (normal or error)Prompt Interception:
UserPromptSubmithook allows blocking/modifying user prompts before sending to the modelTool Execution:
PreToolUsehook fires before tool execution, can block with a reasonPostToolUsehook fires after successful tool executionPostToolUseFailurehook fires when tool execution failsStop Hook Loop:
Stophook fires after model response completes (no pending tool calls)StopFailurehook replaces Stop event for API errorsKey Implementation Details:
Reviewer Test Plan
Run the Session test suite:
Testing Matrix
Linked issues / bugs
#3205
#3108