feat(extension): add lifecycle event contracts#21
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between bb1e41ffbf72cd16b1f66fcd04ee08bb7680f590 and 275ee9e. 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request introduces a complete agent lifecycle event system. It documents planned event groups and delivery phases across the roadmap, implements typed Go contracts and a handler dispatch loop, and validates behavior with comprehensive tests covering priority ordering, handler stopping, error aggregation, and validation. ChangesLifecycle Event System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
+ Coverage 56.79% 56.93% +0.14%
==========================================
Files 158 159 +1
Lines 15521 15572 +51
==========================================
+ Hits 8815 8866 +51
+ Misses 5754 5752 -2
- Partials 952 954 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/extension/lifecycle_test.go (1)
13-100: ⚡ Quick winPrefer table-driven subtests for these core lifecycle dispatch scenarios.
The four cases are strongly pattern-aligned; consolidating them into one table-driven test will reduce duplication and make additional edge cases cheaper to add.
As per coding guidelines
**/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs.🤖 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/extension/lifecycle_test.go` around lines 13 - 100, Consolidate the four similar tests (TestManager_DispatchLifecycleRunsHandlersInPriorityOrder, TestManager_DispatchLifecycleCanStopHandlers, TestManager_DispatchLifecycleCollectsHandlerErrors, TestManager_DispatchLifecycleRequiresName) into a single table-driven test that iterates cases describing input (either a Lua script passed to loadTestExtension or use of extension.NewManager), the LifecycleEvent to dispatch, and expected outcomes (expected Name/Payload values, HandlerCount, Errors length/contents, Consumed/Stopped flags, and whether an error is expected). Implement each case as a t.Run subtest and reuse manager.DispatchLifecycle, loadTestExtension, and extension.LifecycleEvent in the case runner; for the RequiresName case use extension.NewManager and t.Cleanup(manager.Shutdown). Ensure assertions inside each subtest check only the fields relevant to that case (e.g., check result.Errors when an error is expected) so the consolidated test covers all previous expectations without duplicating setup.
🤖 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.
Nitpick comments:
In `@internal/extension/lifecycle_test.go`:
- Around line 13-100: Consolidate the four similar tests
(TestManager_DispatchLifecycleRunsHandlersInPriorityOrder,
TestManager_DispatchLifecycleCanStopHandlers,
TestManager_DispatchLifecycleCollectsHandlerErrors,
TestManager_DispatchLifecycleRequiresName) into a single table-driven test that
iterates cases describing input (either a Lua script passed to loadTestExtension
or use of extension.NewManager), the LifecycleEvent to dispatch, and expected
outcomes (expected Name/Payload values, HandlerCount, Errors length/contents,
Consumed/Stopped flags, and whether an error is expected). Implement each case
as a t.Run subtest and reuse manager.DispatchLifecycle, loadTestExtension, and
extension.LifecycleEvent in the case runner; for the RequiresName case use
extension.NewManager and t.Cleanup(manager.Shutdown). Ensure assertions inside
each subtest check only the fields relevant to that case (e.g., check
result.Errors when an error is expected) so the consolidated test covers all
previous expectations without duplicating setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 707267a9-bdbf-4075-b8dd-3d4d36ce4946
📥 Commits
Reviewing files that changed from the base of the PR and between d37e548 and bb1e41ffbf72cd16b1f66fcd04ee08bb7680f590.
📒 Files selected for processing (6)
TODO.mddocs/extension-api.mddocs/extension-roadmap.mddocs/runtime-architecture.mdinternal/extension/lifecycle.gointernal/extension/lifecycle_test.go
bb1e41f to
275ee9e
Compare
|



Summary
Validation