fix(hooks): dispatch specialistStart/Stop lifecycle hooks - #796
fix(hooks): dispatch specialistStart/Stop lifecycle hooks#796Ayush7614 wants to merge 4 commits into
Conversation
These events were documented and accepted by hooks add, but never fired. Wire advisory dispatch from specialist accounting and allow specialist name matchers so EXTENDING.md matches runtime behavior.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughSpecialist lifecycle hooks now accept specialist matchers, dispatch ChangesSpecialist lifecycle hooks
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SpecialistExecutor
participant SpecialistAccounting
participant OutputTool
participant LifecycleHooks
participant HooksDispatcher
SpecialistExecutor->>SpecialistAccounting: recordSpecialistStart
SpecialistAccounting->>LifecycleHooks: Dispatch specialistStart
OutputTool->>SpecialistAccounting: Record background stop
SpecialistAccounting->>LifecycleHooks: Dispatch deduplicated specialistStop
LifecycleHooks->>HooksDispatcher: Forward event and payload
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai make an intensive review on this |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/specialist/accounting.go (1)
43-64: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winRelease
accountingMubefore running external hooks.
recordSpecialistStopkeeps the accounting mutex held whileDispatchruns arbitrary commands. A slow lifecycle hook can block all other specialist stop accounting, especially during concurrent swarm runs. Limit the lock to the deduplicated append, then dispatch after unlocking.🤖 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/specialist/accounting.go` around lines 43 - 64, Update recordSpecialistStop so accountingMu protects only the appendSpecialistEventOnce deduplication/write operation, releasing it before executor.dispatchLifecycleHook runs. Preserve the deferred unlock removal and ensure the lifecycle hook executes after the mutex is released.
🤖 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/hooks/hooks_test.go`:
- Around line 213-241: Extend TestLoadConfigAcceptsMatchersOnSpecialistHooks to
table-drive both EventSpecialistStart and EventSpecialistStop, using the
corresponding event value when selecting hooks. Preserve the existing matched
and unmatched ToolName assertions so matcher behavior is verified for both
specialist lifecycle events.
In `@internal/specialist/accounting.go`:
- Around line 60-64: Update the stop handling flow around
appendSpecialistEventOnce to retain and inspect its deduplication result.
Dispatch the specialistStop lifecycle hook only when the stop event is newly
recorded, while preserving the existing event append arguments and avoiding hook
execution for duplicate stop attempts.
---
Outside diff comments:
In `@internal/specialist/accounting.go`:
- Around line 43-64: Update recordSpecialistStop so accountingMu protects only
the appendSpecialistEventOnce deduplication/write operation, releasing it before
executor.dispatchLifecycleHook runs. Preserve the deferred unlock removal and
ensure the lifecycle hook executes after the mutex is released.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f78013fc-aca2-4520-9821-72ce5b71fcc6
📒 Files selected for processing (9)
docs/EXTENDING.mdinternal/cli/app.gointernal/cli/exec.gointernal/cli/hooks_manage.gointernal/hooks/hooks.gointernal/hooks/hooks_test.gointernal/specialist/accounting.gointernal/specialist/accounting_test.gointernal/specialist/exec.go
…pecialist-lifecycle-hooks
Release accountingMu before running lifecycle hooks, and dispatch specialistStop only when the stop event is newly recorded so concurrent finishers cannot re-run arbitrary hook commands.
|
Addressed CodeRabbit review in 19f98c2 (also synced with latest
|
|
cc: @kevincodex1 |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Both of the earlier findings are properly fixed here: the dedup result is honoured now, and accountingMu brackets only the append so a slow hook cannot stall other specialist accounting. The supporting checks all pass too, the hooks are genuinely advisory (blocksOn is beforeTool only and the outcome is discarded), the specialist-name matcher now matches what the docs table already advertised, and runWithTimeout bounds a hung hook at 30s.
The problem is that fixing (1) by gating on appended introduced the inverse defect, and it is deterministic rather than a race.
[blocker] Swarm members fire specialistStart and never specialistStop, 100% of the time.
internal/swarm/launcher_specialist.go builds TaskRunOptions with ParentModel, Cwd, PermissionMode and MemberAutonomy but never sets ParentSessionID. appendSpecialistEventOnce short-circuits on an empty parent session and returns false, so if appended suppresses the stop dispatch for every swarm member, while recordSpecialistStart dispatches unconditionally with no gate. Anyone pairing the two events (a notification, a timer, resource cleanup) leaks state on every member of every swarm run.
[major] Background specialists can drop the stop hook too.
OutputTool.readOutput reaps through a fresh Executor{SessionStore: ...} literal with no LifecycleHooks, and whichever executor wins the appendSpecialistEventOnce race owns the only dispatch. When the hook-less TaskOutput poll wins, the hook-carrying onExit path then sees appended==false and suppresses the hook. Deterministic variant: if the parent process exits before the background child, the onExit goroutine dies with it and a later TaskOutput is the only reaper, so the hook can never fire.
[minor] appended, _ := also discards the error, and AppendEventUnlessExists returns false on failure as well as on already-recorded, so a session-store error silently becomes a suppressed hook with no signal anywhere.
All three are the same root cause: dispatch is coupled to session-event bookkeeping that is skipped when there is no parent session, performed by a different executor, or failed. I would dispatch when the stop is first observed by THIS executor, decoupled from whether the audit event was appended, keeping the real duplicate suppression, and pass LifecycleHooks into OutputTool so the background reaper can dispatch.
Two smaller notes: for swarm members the matcher subject is the agent type (subagent/teammate) rather than a registered specialist name, so a matcher written against real specialist names will not match once stop hooks do fire, which deserves a docs line. And no test covers the CLI wire-up at all, so deleting either attachLifecycleHooks call would still pass the suite, which is exactly the class of bug #793 reports; the new concurrency test races two hook-carrying executors, so by construction it cannot catch either failure above.
The dedup fix itself is right, it just needs to not be the thing that decides whether the hook fires. Happy to re-review quickly.
Decouple stop-hook dispatch from parent-session event append so swarm members (empty parent) and TaskOutput still get exactly one hook via a shared LifecycleHooks claim, and wire that bridge into TaskOutput.
|
Addressed @Vasanthdev2004's review on
Ready for re-review. Thanks! |
3 participants
Summary
specialistStart/specialistStophooks from specialist accountingThese events were documented and accepted by
zero hooks add, but never fired at runtime.Closes #793
Test plan
go test ./internal/specialist/ ./internal/hooks/ -count=1go vet ./internal/specialist/ ./internal/hooks/ ./internal/cli/govulncheckon touched packages — no vulnerabilitiesSummary by CodeRabbit