Conversation
sipeed-bot
bot
added
type: enhancement
| done := make(chan error, 1) | ||
| go func() { | ||
| _, writeErr := ph.stdin.Write(body) | ||
| done <- writeErr | ||
| }() | ||
|
|
||
| select { | ||
| case err := <-done: | ||
| if err != nil { | ||
| return fmt.Errorf("write process hook %q message: %w", ph.name, err) | ||
| } | ||
| return nil | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| } | ||
| } |
There was a problem hiding this comment.
If ctx.Done() fires, the function returns and defer ph.writeMu.Unlock() is executed. The background goroutine is still blocked trying to write to ph.stdin. When a subsequent hook request arrives, it acquires writeMu and starts a new write. The original goroutine might unblock simultaneously, resulting in interleaved, corrupted JSON being pushed to the external process's standard input.
If an IPC write times out, the stream state is permanently compromised. You cannot safely recover. You must tear down the process.
case <-ctx.Done():
// The pipe is hopelessly blocked. We must terminate the hook.
go ph.Close()
return ctx.Err()
| for scanner.Scan() { | ||
| var msg processHookRPCMessage | ||
| if err := json.Unmarshal(scanner.Bytes(), &msg); err != nil { | ||
| logger.WarnCF("hooks", "Failed to decode process hook message", map[string]any{ | ||
| "hook": ph.name, | ||
| "error": err.Error(), | ||
| }) | ||
| continue | ||
| } | ||
| if msg.ID == 0 { | ||
| continue | ||
| } | ||
| ph.pendingMu.Lock() | ||
| respCh, ok := ph.pending[msg.ID] | ||
| if ok { | ||
| delete(ph.pending, msg.ID) | ||
| } | ||
| ph.pendingMu.Unlock() | ||
| if ok { | ||
| respCh <- msg | ||
| close(respCh) | ||
| } | ||
| } |
There was a problem hiding this comment.
If the external process returns a JSON payload larger than 1MB (e.g., a response containing large base64 images or massive context arrays), scanner.Scan() will return false, and the loop will simply exit. The ProcessHook instance remains registered, but it is no longer reading responses. All pending and future requests to this hook will hang until they hit their context timeouts.
check scanner.Err() and proactively tear down the hook so the system knows it has failed.
for scanner.Scan() { ... }
if err := scanner.Err(); err != nil {
logger.ErrorCF("hooks", "Process hook read loop failed", map[string]any{"error": err})
ph.failPending(err)
go ph.Close()
}
afjcjsbx
left a comment
afjcjsbx
left a comment
There was a problem hiding this comment.
only two hints, but nice PR introducing the hooks!
yinwm
left a comment
yinwm
left a comment
There was a problem hiding this comment.
LGTM with one follow-up item.
Review Summary:
- ✅ Well-designed hook interfaces (EventObserver, LLMInterceptor, ToolInterceptor, ToolApprover)
- ✅ Comprehensive documentation in both English and Chinese
- ✅ Good test coverage (hooks_test.go, hook_mount_test.go, hook_process_test.go)
- ✅ Thread-safe implementation with proper mutex usage
- ✅ JSON-RPC 2.0 for process hooks - standard and extensible
- ✅ Graceful shutdown with closeOnce pattern
Follow-up item (non-blocking):
pkg/agent/hook_process.go: The stderr reader goroutine has no buffer limit. A misbehaving external hook process could exhaust memory by writing infinite stderr. Recommend adding a byte limit (e.g., 64KB) with truncation warning in a follow-up PR.
This is the key piece for Agent Refactor (#1216/#1316) — enables tool approval, LLM interception, and observability. Ready to merge.
3 participants
📝 Description
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist