feat: implement command hook system (#32)#41
Merged
Conversation
Add extensibility hook system matching Claude Code's design. Hooks let users run shell commands at tool lifecycle points (PreToolUse, PostToolUse, PostToolUseFailure) to enforce policies, audit actions, or modify inputs. - 3 core abstractions: HookEvent (sealed), HookResult (sealed), HookExecutor - HookConfig/HookMatcher/HookRegistry for config-driven hook resolution - CommandHookExecutor: ProcessBuilder with JSON stdin/stdout, exit code semantics (0=proceed, 2=block, other=non-blocking error), timeout - PreToolUse hooks run before permission check; can block or modify input - PostToolUse/PostToolUseFailure hooks fire async on virtual threads - Config loaded from ~/.aceclaw/config.json and project config (appending) - 24 new tests across 3 test classes (unit + E2E integration)
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a hook system: core hook/event/result types and executor contract; daemon-side config, matcher, registry, and command-based executor; integrates hooks into the daemon handler to run blockable/modifiable PreToolUse and async PostToolUse/PostToolUseFailure hooks; includes unit and integration tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as StreamingAgentHandler
participant Registry as HookRegistry
participant Executor as CommandHookExecutor
participant Shell as Shell Process
participant Tool
Client->>Handler: Tool request
Handler->>Registry: resolve(PreToolUse event)
Registry-->>Handler: matching hooks
rect rgba(100,200,100,0.5)
Note over Handler,Shell: PreToolUse (blockable/modifiable)
Handler->>Executor: execute(PreToolUse)
Executor->>Shell: spawn process, write JSON stdin
Shell-->>Executor: exit code + stdout
alt exit code = 0 (Proceed)
Executor-->>Handler: Proceed (maybe updatedInput)
else exit code = 2 (Block)
Executor-->>Handler: Block(reason)
Handler-->>Client: Tool execution blocked
else other (Error)
Executor-->>Handler: Error
end
end
rect rgba(100,150,200,0.5)
Note over Handler,Tool: Tool execution
Handler->>Tool: execute(possibly modified input)
Tool-->>Handler: result or error
end
rect rgba(200,100,100,0.5)
Note over Handler,Shell: PostToolUse (async)
Handler->>Registry: resolve(PostToolUse/PostToolUseFailure)
Registry-->>Handler: matching hooks
Handler->>Executor: execute(PostToolUse) [async]
Executor->>Shell: spawn process, write JSON stdin
Shell-->>Executor: exit code + stdout
Executor-->>Handler: (async result logged)
end
Handler-->>Client: Tool result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryImplements a command hook system for tool lifecycle extensibility, matching Claude Code's design. Three hook events ( Key changes:
Potential issue:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent as StreamingAgentHandler
participant PAT as PermissionAwareTool
participant HE as HookExecutor
participant HR as HookRegistry
participant CHE as CommandHookExecutor
participant Shell as Shell Process
participant Tool as Actual Tool
Agent->>PAT: execute(inputJson)
Note over PAT,HE: PreToolUse Hook (Blocking)
PAT->>HE: execute(PreToolUse event)
HE->>HR: resolve(event)
HR-->>HE: List<HookConfig>
loop For each hook config
HE->>CHE: executeOne(hookConfig, event)
CHE->>Shell: spawn process with JSON stdin
Shell-->>CHE: exit code + stdout/stderr
CHE-->>HE: HookResult (Proceed/Block/Error)
alt Block result
HE-->>PAT: HookResult.Block
PAT-->>Agent: ToolResult (error)
end
end
alt Not blocked
Note over PAT: Permission Check
PAT->>PermissionManager: check(request)
Note over PAT,Tool: Tool Execution
PAT->>Tool: execute(effectiveInputJson)
Tool-->>PAT: ToolResult
Note over PAT,HE: PostToolUse Hook (Async)
PAT-->>HE: fire async on virtual thread
par Async hook execution
HE->>HR: resolve(PostToolUse/Failure event)
HR-->>HE: List<HookConfig>
loop All hooks run
HE->>CHE: executeOne(hookConfig, event)
CHE->>Shell: spawn process
Shell-->>CHE: exit code
end
end
PAT-->>Agent: ToolResult
end
Last reviewed commit: dd6c388 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+123
to
+124
| String stdout = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8).trim(); | ||
| String stderr = new String(process.getErrorStream().readAllBytes(), StandardCharsets.UTF_8).trim(); |
There was a problem hiding this comment.
reading streams after waitFor() can miss output if process terminates quickly - read streams concurrently before waiting
Suggested change
| String stdout = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8).trim(); | |
| String stderr = new String(process.getErrorStream().readAllBytes(), StandardCharsets.UTF_8).trim(); | |
| // Read streams concurrently while waiting | |
| var stdoutFuture = CompletableFuture.supplyAsync(() -> { | |
| try { return new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8).trim(); } | |
| catch (IOException e) { return ""; } | |
| }); | |
| var stderrFuture = CompletableFuture.supplyAsync(() -> { | |
| try { return new String(process.getErrorStream().readAllBytes(), StandardCharsets.UTF_8).trim(); } | |
| catch (IOException e) { return ""; } | |
| }); | |
| boolean finished = process.waitFor(hookConfig.timeout(), TimeUnit.SECONDS); | |
| if (!finished) { | |
| process.destroyForcibly(); | |
| log.warn("Hook command timed out after {}s: {}", hookConfig.timeout(), hookConfig.command()); | |
| return new HookResult.Error(-1, "Hook timed out after " + hookConfig.timeout() + "s"); | |
| } | |
| int exitCode = process.exitValue(); | |
| String stdout = stdoutFuture.getNow(""); | |
| String stderr = stderrFuture.getNow(""); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: aceclaw-daemon/src/main/java/dev/aceclaw/daemon/CommandHookExecutor.java
Line: 123-124
Comment:
reading streams after `waitFor()` can miss output if process terminates quickly - read streams concurrently before waiting
```suggestion
// Read streams concurrently while waiting
var stdoutFuture = CompletableFuture.supplyAsync(() -> {
try { return new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8).trim(); }
catch (IOException e) { return ""; }
});
var stderrFuture = CompletableFuture.supplyAsync(() -> {
try { return new String(process.getErrorStream().readAllBytes(), StandardCharsets.UTF_8).trim(); }
catch (IOException e) { return ""; }
});
boolean finished = process.waitFor(hookConfig.timeout(), TimeUnit.SECONDS);
if (!finished) {
process.destroyForcibly();
log.warn("Hook command timed out after {}s: {}", hookConfig.timeout(), hookConfig.command());
return new HookResult.Error(-1, "Hook timed out after " + hookConfig.timeout() + "s");
}
int exitCode = process.exitValue();
String stdout = stdoutFuture.getNow("");
String stderr = stderrFuture.getNow("");
```
How can I resolve this? If you propose a fix, please make it concise.Process pipe buffers are finite (~64KB on Linux). If the hook command writes more than the buffer capacity before we start reading, it blocks on write while we block on waitFor() — a classic deadlock. Fix by draining stdout and stderr on virtual threads concurrently with waitFor.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
aceclaw-daemon/src/test/java/dev/aceclaw/daemon/HookIntegrationTest.java (1)
287-290: Replace fixed sleeps with a bounded poll to reduce test flakiness.
Async hooks can be slower on busy CI; polling for the marker with a timeout is more deterministic.🛠️ Suggested refactor
- Thread.sleep(1000); + waitForFile(tempDir.resolve("audit-marker.json"), 2_000); @@ - Thread.sleep(500); + waitForFile(tempDir.resolve("audit-marker.json"), 2_000); @@ // -- Helper methods (mirrors DaemonIntegrationTest) -- + + private static void waitForFile(Path path, long timeoutMs) throws Exception { + long deadline = System.currentTimeMillis() + timeoutMs; + while (System.currentTimeMillis() < deadline) { + if (Files.exists(path)) { + return; + } + Thread.sleep(50); + } + throw new AssertionError("Timed out waiting for file: " + path); + }Also applies to: 346-347, 363-369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/test/java/dev/aceclaw/daemon/HookIntegrationTest.java` around lines 287 - 290, In HookIntegrationTest replace the fixed Thread.sleep(1000) after calling sendPromptAndCollectEvents with a bounded poll that repeatedly checks for the async PostToolUse hook completion marker (the same condition you assert later) until a configurable timeout (e.g., 5–10s) elapses; implement a small sleep between polls (e.g., 50–200ms), stop early when the marker is observed, and fail the test if the timeout is reached — apply the same change for the other occurrences around lines where sendPromptAndCollectEvents is used so tests no longer rely on fixed sleeps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawConfig.java`:
- Around line 454-473: Replace the mutable POJO classes HookMatcherFormat and
HookConfigFormat with Java records to make them immutable and use
constructor-based deserialization with Jackson; change the declarations of
HookMatcherFormat and HookConfigFormat to record types (keeping
`@JsonIgnoreProperties`) and update tests (HookIntegrationTest,
CommandHookExecutorTest, HookRegistryTest) that currently assign fields (e.g.,
mf.matcher = ...) to construct instances via the record constructors (e.g., new
HookMatcherFormat(matcher, hooks)) so deserialization and immutability are
preserved.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/HookRegistry.java`:
- Around line 18-33: Replace the immutable holder class HookRegistry with a
record declaration: declare "public record HookRegistry(Map<String,
List<HookMatcher>> matchers)". Move the static Logger (log) into the record as a
static field and keep all instance/static methods as-is. Implement a compact
canonical constructor for the record that performs the same
deep-copy/validation: create a new LinkedHashMap, copy each entry with
List.copyOf(entry.getValue()), then assign the record component to an
unmodifiable copy via Map.copyOf(copy) (so the component remains immutable).
Ensure any existing usages of the private constructor are updated to the
record's canonical constructor semantics and preserve visibility and behavior of
other methods.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java`:
- Around line 139-141: The hook that sets the current working directory should
prefer the session's project path instead of falling back to
System.getProperty("user.dir") or a generic workingDir; update the code around
the createPermissionAwareRegistry call in StreamingAgentHandler (and the similar
block at the other occurrence) to read the project path from the session (use
the sessionId/context API that holds the project path) and pass that into the
hook's cwd, falling back to the existing workingDir only if the session project
path is absent; ensure the permission-aware registry/hook constructors (the code
paths using cancelContext and sessionId) use this session-derived path for all
command execution contexts.
In
`@aceclaw-daemon/src/test/java/dev/aceclaw/daemon/CommandHookExecutorTest.java`:
- Around line 106-114: The test exit0WithDenyDecisionBlocks is asserting a block
but the deny.sh fixture exits 0 (proceed) causing CI failure; update the fixture
script used by buildExecutor("PreToolUse", "bash", scriptPath("deny.sh")) to
exit with the block code (e.g., exit 2) and emit the block reason, or
alternatively change the test assertion to expect a HookResult.Proceed; target
the deny.sh script (or the assertion in exit0WithDenyDecisionBlocks) and ensure
the HookResult.Block.reason() value matches the emitted message when using
HookResult.Block in the test.
---
Nitpick comments:
In `@aceclaw-daemon/src/test/java/dev/aceclaw/daemon/HookIntegrationTest.java`:
- Around line 287-290: In HookIntegrationTest replace the fixed
Thread.sleep(1000) after calling sendPromptAndCollectEvents with a bounded poll
that repeatedly checks for the async PostToolUse hook completion marker (the
same condition you assert later) until a configurable timeout (e.g., 5–10s)
elapses; implement a small sleep between polls (e.g., 50–200ms), stop early when
the marker is observed, and fail the test if the timeout is reached — apply the
same change for the other occurrences around lines where
sendPromptAndCollectEvents is used so tests no longer rely on fixed sleeps.
Comment on lines
+18
to
+33
| public final class HookRegistry { | ||
|
|
||
| private static final Logger log = LoggerFactory.getLogger(HookRegistry.class); | ||
|
|
||
| /** Event type name → ordered list of matchers. */ | ||
| private final Map<String, List<HookMatcher>> matchers; | ||
|
|
||
| private HookRegistry(Map<String, List<HookMatcher>> matchers) { | ||
| // Deep-copy to ensure immutability | ||
| var copy = new LinkedHashMap<String, List<HookMatcher>>(); | ||
| for (var entry : matchers.entrySet()) { | ||
| copy.put(entry.getKey(), List.copyOf(entry.getValue())); | ||
| } | ||
| this.matchers = Map.copyOf(copy); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n aceclaw-daemon/src/main/java/dev/aceclaw/daemon/HookRegistry.javaRepository: xinhuagu/AceClaw
Length of output: 6642
Convert HookRegistry to a record for immutable data.
This class is an immutable value holder with a compact constructor performing deep-copy validation. Records are the appropriate choice per the coding guideline and will simplify the implementation while preserving all instance and static methods.
♻️ Suggested refactor
-public final class HookRegistry {
+public record HookRegistry(Map<String, List<HookMatcher>> matchers) {
private static final Logger log = LoggerFactory.getLogger(HookRegistry.class);
- /** Event type name → ordered list of matchers. */
- private final Map<String, List<HookMatcher>> matchers;
-
- private HookRegistry(Map<String, List<HookMatcher>> matchers) {
+ public HookRegistry {
// Deep-copy to ensure immutability
var copy = new LinkedHashMap<String, List<HookMatcher>>();
for (var entry : matchers.entrySet()) {
copy.put(entry.getKey(), List.copyOf(entry.getValue()));
}
- this.matchers = Map.copyOf(copy);
+ matchers = Map.copyOf(copy);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/HookRegistry.java` around
lines 18 - 33, Replace the immutable holder class HookRegistry with a record
declaration: declare "public record HookRegistry(Map<String, List<HookMatcher>>
matchers)". Move the static Logger (log) into the record as a static field and
keep all instance/static methods as-is. Implement a compact canonical
constructor for the record that performs the same deep-copy/validation: create a
new LinkedHashMap, copy each entry with List.copyOf(entry.getValue()), then
assign the record component to an unmodifiable copy via Map.copyOf(copy) (so the
component remains immutable). Ensure any existing usages of the private
constructor are updated to the record's canonical constructor semantics and
preserve visibility and behavior of other methods.
Comment on lines
+106
to
+114
| @Test | ||
| void exit0WithDenyDecisionBlocks() { | ||
| var executor = buildExecutor("PreToolUse", "bash", scriptPath("deny.sh")); | ||
| var event = new HookEvent.PreToolUse("s1", "/tmp", "bash", emptyInput()); | ||
|
|
||
| var result = executor.execute(event); | ||
| assertThat(result).isInstanceOf(HookResult.Block.class); | ||
| assertThat(((HookResult.Block) result).reason()).isEqualTo("Blocked by policy"); | ||
| } |
There was a problem hiding this comment.
Test contradicts exit-code contract (CI failure).
Exit code 0 is documented as “Proceed,” but this test expects a block from deny.sh which currently exits 0. CI also reports a failure at Line 112. Align the fixture with the exit-code semantics (e.g., exit 2 for block) or change the expectation to Proceed.
🔧 Suggested fix: make deny.sh exit 2 and emit the block reason
- // Script that exits 0 with deny decision
+ // Script that exits 2 (block) with deny decision
writeScript("deny.sh", """
#!/bin/bash
- echo '{"decision":"deny","additionalContext":"Blocked by policy"}'
+ echo '{"decision":"deny","additionalContext":"Blocked by policy"}'
+ echo "Blocked by policy" >&2
+ exit 2
""");🧰 Tools
🪛 GitHub Actions: CI
[error] 112-112: Test failed: CommandHookExecutorTest.exit0WithDenyDecisionBlocks() threw java.lang.AssertionError at CommandHookExecutorTest.java:112. Gradle task ':aceclaw-daemon:test' failed during 'gradlew build test --no-daemon'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-daemon/src/test/java/dev/aceclaw/daemon/CommandHookExecutorTest.java`
around lines 106 - 114, The test exit0WithDenyDecisionBlocks is asserting a
block but the deny.sh fixture exits 0 (proceed) causing CI failure; update the
fixture script used by buildExecutor("PreToolUse", "bash",
scriptPath("deny.sh")) to exit with the block code (e.g., exit 2) and emit the
block reason, or alternatively change the test assertion to expect a
HookResult.Proceed; target the deny.sh script (or the assertion in
exit0WithDenyDecisionBlocks) and ensure the HookResult.Block.reason() value
matches the emitted message when using HookResult.Block in the test.
added 2 commits
February 20, 2026 20:32
On fast CI runners, the hook process may exit before stdin write completes. If the process doesn't read stdin, the write throws broken pipe IOException which previously aborted the entire executeOne() method — stdout/stderr reader threads were never started, so the hook's output was lost. Fix: start reader threads FIRST, then write stdin with a separate try-catch so broken pipe is non-fatal. This ensures we always capture the process output regardless of stdin write success.
- Convert HookMatcherFormat/HookConfigFormat from mutable classes to records - Use session's projectPath for hook cwd instead of daemon's workingDir - Replace Thread.sleep with bounded polling in HookIntegrationTest - Add clarifying comment on exit-0 deny test contract
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawConfig.java`:
- Around line 379-387: The merge loop for fileConfig.hooks can NPE when an event
key maps to null or contains null elements; update the loop in AceClawConfig to
skip entries where hookEntry.getValue() is null or empty and only append
non-null elements to this.hooks. Specifically, in the block using
computeIfAbsent and addAll, check hookEntry.getValue() for null/empty and either
continue or filter out null items (e.g., collect non-null elements or iterate
and add only non-null) before calling addAll on this.hooks; keep references to
fileConfig.hooks, hookEntry, computeIfAbsent and addAll so the change is local
and obvious.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java`:
- Around line 1046-1089: The code picks event type by checking error != null, so
when a tool throws with a null message or an error ToolResult has null output
the wrong PostToolUse event is fired; update executeWithPostHooks and the
exception path to always pass a non-null error string for failure cases (e.g.,
use e.getMessage() != null ? e.getMessage() : "" in the catch and use
result.output() != null ? result.output() : "" when result.isError()) so
firePostHookAsync(...) receives a non-null error for failures and null for
successes (no other logic changes required to fire HookEvent.PostToolUseFailure
vs HookEvent.PostToolUse).
---
Duplicate comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/HookRegistry.java`:
- Around line 18-32: Replace the immutable holder class HookRegistry with a Java
record: declare "public record HookRegistry(Map<String, List<HookMatcher>>
matchers) { ... }", keep the static logger field (private static final Logger
log = LoggerFactory.getLogger(HookRegistry.class)); implement a compact
canonical constructor that deep-copies the incoming map and lists (e.g., build a
new LinkedHashMap, put each entry.getKey() -> List.copyOf(entry.getValue()),
then assign this.matchers = Map.copyOf(copy)) so callers get the same
defensive-copy semantics; remove the old explicit class constructor and any
mutating methods, and keep the field name matchers unchanged to preserve API.
- AceClawConfig: skip null/empty hook lists during config merge to prevent NPE - StreamingAgentHandler: ensure non-null error string for PostToolUseFailure so firePostHookAsync dispatches the correct event type
This was referenced Feb 20, 2026
This was referenced Mar 1, 2026
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
1 participant
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
PreToolUse(blocking),PostToolUse(non-blocking),PostToolUseFailure(non-blocking)~/.aceclaw/config.jsonand project config (project appends to global per event type)Changes
New files (7 source + 3 test):
aceclaw-core:HookEvent(sealed interface),HookResult(sealed interface),HookExecutor(functional interface)aceclaw-daemon:HookConfig,HookMatcher,HookRegistry,CommandHookExecutorHookRegistryTest(9 tests),CommandHookExecutorTest(11 tests),HookIntegrationTest(4 E2E tests)Modified files (3):
AceClawConfig— hooks field, merge logic, inner format classes for Jackson deserializationStreamingAgentHandler— PreToolUse hooks before permission check (can block/modify input), PostToolUse hooks fire async on virtual threads after executionAceClawDaemon— wires HookRegistry + CommandHookExecutor into agent handlerTest plan
HookRegistryTest: empty registry, exact/regex/null matchers, multiple matchers order, invalid regex, unknown eventsCommandHookExecutorTest: exit codes 0/1/2, stdin piping, timeout, JSON parsing, malformed stdout, block-on-first for PreToolUseHookIntegrationTest: full E2E — PreToolUse blocks bash, unmatched tool passes, PostToolUse audit marker, invocation order./gradlew clean buildsucceeds (pre-existing SkillRegistryTest failures unrelated)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration
Tests