fix(runtime): recover ro event helpers#30
Merged
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: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds CompletionRequest tool callbacks and runtime lifecycle emitters, and introduces reactive event utilities (tick, after-context, context-done, file-watch, signal) plus diagnostics, tests, DI test helper, and go.mod plugin deps. ChangesEvent and Callback Infrastructure
Sequence DiagramsequenceDiagram
participant Runtime
participant EventBus
participant CompletionClient
participant ToolExecutor
Runtime->>EventBus: dispatch before_provider_request(payload)
Runtime->>CompletionClient: Complete(ctx, CompletionRequest{OnToolCall, OnToolResult, OnEvent})
CompletionClient->>ToolExecutor: executeToolCalls(ctx, cwd, toolCalls, OnEvent, OnToolCall, OnToolResult)
ToolExecutor->>Runtime: OnToolCall(ctx, ToolCallEvent)
ToolExecutor->>Runtime: OnToolResult(ctx, ToolEvent)
CompletionClient->>EventBus: completion result -> dispatch after_provider_response(payload)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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 #30 +/- ##
==========================================
+ Coverage 57.36% 58.13% +0.77%
==========================================
Files 160 165 +5
Lines 15782 15973 +191
==========================================
+ Hits 9053 9286 +233
+ Misses 5767 5719 -48
- Partials 962 968 +6
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:
|
![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 (2)
internal/event/diagnostics.go (1)
36-36: ⚡ Quick winAvoid an empty observer callback body to prevent quality-gate failures.
The no-op
func(context.Context, Envelope) {}is currently flagged by Sonar. Add a brief inline comment in the body (or route through a named no-op helper) so CI/static analysis doesn’t fail on this path.🤖 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/event/diagnostics.go` at line 36, The inline anonymous no-op observer func(context.Context, Envelope) {} in diagnostics should not have an empty body; either add a brief inline comment inside the function body (e.g. // intentionally no-op) or replace the literal with a named helper like noOpObserver defined as func(context.Context, Envelope) { /* no-op */ } and use that identifier where the observer callback is registered; this addresses the Sonar quality-gate by ensuring the no-op has an explicit comment or named helper while leaving the observable signature (context.Context, Envelope) unchanged.internal/event/signals.go (1)
29-29: ⚡ Quick winDocument the intentional no-op completion callback.
The empty completion handler can fail static analysis; add an inline comment explaining it is intentionally ignored because cancellation is handled in
next/error.Proposed tweak
- func(context.Context) {}, + func(context.Context) { + // Intentionally no-op: context cancellation is driven by next/error handlers. + },🤖 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/event/signals.go` at line 29, Add an inline comment to the no-op completion callback literal func(context.Context) {} in internal/event/signals.go explaining it's intentionally empty and ignored because cancellation/cleanup is handled via the next/error path; locate the anonymous completion handler (the literal func(context.Context) {}) and append a short comment like "intentionally no-op; cancellation handled in next/error" so static analysis and reviewers understand this is deliberate.
🤖 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/di/event_service_test.go`:
- Around line 17-20: Replace the unbounded context used in the cleanup with a
timeout-bound context: inside the t.Cleanup lambda that calls
container.ShutdownWithContext, create a context with context.WithTimeout (e.g.,
5s or appropriate CI-safe duration), defer cancel(), pass that ctx to
container.ShutdownWithContext, and keep the existing require.True assertion on
report.Succeed (report.Error()) so the test can't hang if shutdown blocks;
reference the t.Cleanup closure and the container.ShutdownWithContext invocation
when making the change.
In `@internal/event/diagnostics_test.go`:
- Line 46: The test's empty onError callback (func(context.Context, error) {})
silently ignores stream errors; replace it with a callback that fails the test
on error by asserting the error is nil (e.g., func(ctx context.Context, err
error) { require.NoError(t, err) } or func(ctx context.Context, err error) { if
err != nil { t.Fatalf("stream error: %v", err) } }), ensuring the callback used
in the test observer (the onError argument) explicitly handles and fails the
test on any non-nil error.
In `@internal/event/fsnotify_test.go`:
- Around line 29-31: The test currently calls require.ErrorIs(t, err,
context.Canceled) from the async observer callback (func(_ context.Context, err
error) { ... }) which can fail from a non-test goroutine; instead, capture the
callback error into a synchronized location (e.g., make an errCh chan error or
an atomic.Value before registering the observer) and in the observer body do a
non-blocking send or atomic.Store of err; then in the main test goroutine, after
the Eventually/timeout has passed, read the error from errCh/atomic and assert
require.ErrorIs(t, observedErr, context.Canceled). Update the anonymous observer
function, add the err channel/atomic variable and the final assertion in the
main test flow.
In `@internal/event/signals_unix_test.go`:
- Line 20: The tests in internal/event/signals_unix_test.go call t.Parallel()
which causes flakes because they mutate process-global signal state; remove the
t.Parallel() invocation(s) (the call in the test functions in this file, e.g.,
the test that currently contains t.Parallel()) so the signal-related tests run
serially instead of in parallel and no longer interfere with each other or other
tests.
---
Nitpick comments:
In `@internal/event/diagnostics.go`:
- Line 36: The inline anonymous no-op observer func(context.Context, Envelope)
{} in diagnostics should not have an empty body; either add a brief inline
comment inside the function body (e.g. // intentionally no-op) or replace the
literal with a named helper like noOpObserver defined as func(context.Context,
Envelope) { /* no-op */ } and use that identifier where the observer callback is
registered; this addresses the Sonar quality-gate by ensuring the no-op has an
explicit comment or named helper while leaving the observable signature
(context.Context, Envelope) unchanged.
In `@internal/event/signals.go`:
- Line 29: Add an inline comment to the no-op completion callback literal
func(context.Context) {} in internal/event/signals.go explaining it's
intentionally empty and ignored because cancellation/cleanup is handled via the
next/error path; locate the anonymous completion handler (the literal
func(context.Context) {}) and append a short comment like "intentionally no-op;
cancellation handled in next/error" so static analysis and reviewers understand
this is deliberate.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71fc4563-64df-4866-85ba-80034e0134d0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
go.modinternal/assistant/anthropic_internal_test.gointernal/assistant/client.gointernal/assistant/openai_responses.gointernal/assistant/runtime.gointernal/assistant/runtime_events.gointernal/assistant/runtime_events_test.gointernal/assistant/tool_loop.gointernal/di/event_service_export_test.gointernal/di/event_service_test.gointernal/event/context.gointernal/event/context_test.gointernal/event/diagnostics.gointernal/event/diagnostics_test.gointernal/event/fsnotify.gointernal/event/fsnotify_test.gointernal/event/signals.gointernal/event/signals_unix_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/event/context_test.go (1)
14-66: ⚡ Quick winConsolidate these core behavior cases into a table-driven test.
The scenarios here are closely related and repeat similar setup/assertion flow; converting to table-driven form will make future case additions cleaner and reduce drift across assertions.
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/event/context_test.go` around lines 14 - 66, Consolidate the five tests (TestAfterContextEmitsOnce, TestAfterContextEmitsImmediatelyForNonPositiveDelay, TestAfterContextStopsOnSourceCancel, TestAfterContextStopsOnSubscriberCancel, TestContextDoneEmitsOnCancel) into a single table-driven test that iterates cases with fields like name, source (a function returning event.Source), subscriberCtx (nil or a cancellable context), expectValues, expectErr; for AfterContext cases provide the delay parameter and for ContextDone use the appropriate source generator, then in each row call either ro.Collect or ro.CollectWithContext based on whether subscriberCtx is set and assert expectErr/expectValues using require, ensuring each case cancels any created contexts (source or subscriber) as needed and runs t.Parallel() per subtest.
🤖 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/event/context_test.go`:
- Around line 14-66: Consolidate the five tests (TestAfterContextEmitsOnce,
TestAfterContextEmitsImmediatelyForNonPositiveDelay,
TestAfterContextStopsOnSourceCancel, TestAfterContextStopsOnSubscriberCancel,
TestContextDoneEmitsOnCancel) into a single table-driven test that iterates
cases with fields like name, source (a function returning event.Source),
subscriberCtx (nil or a cancellable context), expectValues, expectErr; for
AfterContext cases provide the delay parameter and for ContextDone use the
appropriate source generator, then in each row call either ro.Collect or
ro.CollectWithContext based on whether subscriberCtx is set and assert
expectErr/expectValues using require, ensuring each case cancels any created
contexts (source or subscriber) as needed and runs t.Parallel() per subtest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78b5ae43-3d0b-4847-9385-781da90db9bc
📒 Files selected for processing (12)
internal/assistant/client_test.gointernal/assistant/errors.gointernal/assistant/openai_responses.gointernal/assistant/runtime.gointernal/assistant/tool_loop_test.gointernal/di/event_service_test.gointernal/event/context_test.gointernal/event/diagnostics.gointernal/event/diagnostics_test.gointernal/event/fsnotify_test.gointernal/event/signals.gointernal/event/signals_unix_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/assistant/errors.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/di/event_service_test.go
- internal/event/signals_unix_test.go
- internal/event/fsnotify_test.go
- internal/event/diagnostics_test.go
- internal/assistant/runtime.go
- internal/event/diagnostics.go
|
This was referenced May 21, 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
2 participants
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.
No description provided.