fix(plugin): add context.Context to HookFunc for cooperative cancellation#16
Conversation
zeroedin
left a comment
There was a problem hiding this comment.
Code Review: fix(plugin): add context.Context to HookFunc for cooperative cancellation
Verdict: Approve with merge sequencing note ✅
Correctly addresses the goroutine leak I flagged on PR #8. The HookFunc signature now carries a context.Context with the timeout deadline, enabling cooperative cancellation.
| Check | Result |
|---|---|
| Hooks tests | 24/24 pass |
| Other plugin tests | 35 fail (pre-existing on main: 0/59 pass without Phase 5) |
go vet |
Clean |
| Regressions | None |
Implementation review:
Run()correctly passescontext.Background()— no timeout for direct executionRunWithTimeout()createscontext.WithTimeout()per hook, passes the derived context, cancels in both branches (success and timeout)- Timeout behavior preserved: pre-hook payload kept on timeout, warning logged, build continues
- Hook functions can now check
ctx.Done()for cooperative cancellation
Test changes: All 24 test modifications are mechanical — adding _ context.Context as the first parameter to match the new signature. Test assertions are unchanged. The hooks are intentionally context-unaware in tests to verify the timeout safety net works even without cooperation.
Merge sequencing concern: PR #8 (Phase 5) also implements hooks.go from stubs with the old HookFunc signature. When both PRs merge to main, they will conflict on hooks.go and hooks_test.go. Recommendation: merge this PR first, then rebase PR #8 to adopt the context.Context signature.
PLAN.md/IMPLEMENTATION.md updated with the context-aware hook API documentation. Clean fix.
1095f8e to
318cc6d
Compare
…tion Change HookFunc signature from func(payload) to func(ctx, payload). Run() passes context.Background(). RunWithTimeout() uses context.WithTimeout() and passes the derived context to each hook, enabling cooperative cancellation. Updates spec (PLAN.md) and implementation plan to document the context-aware hook API. Closes #13 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
318cc6d to
7896690
Compare
Summary
HookFuncsignature fromfunc(payload interface{}) (interface{}, error)tofunc(ctx context.Context, payload interface{}) (interface{}, error)Run()passescontext.Background()to each hookRunWithTimeout()usescontext.WithTimeout()and passes the derived context, enabling cooperative cancellationCloses #13
Test plan
context.Contextsignatures🤖 Generated with Claude Code