[Repo Assist] test(shared): add unit tests for AsyncEventHandlerGuard fault boundary#635
Conversation
Cover all exit paths of the async event-handler guard: - Null work argument throws ArgumentNullException immediately - Successful work completes normally - OperationCanceledException is swallowed (not forwarded to onError), and a debug message is logged - Unexpected exceptions invoke onError callback and log an error with the exception attached - operationName appears in logged messages - No callbacks scenario: unhandled exceptions are silently swallowed without tearing down the process Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Codex review: needs maintainer review before merge. Reviewed June 2, 2026, 10:07 AM ET / 14:07 UTC. Summary Reproducibility: not applicable. this is a test coverage PR rather than a user-reported bug. Source inspection shows current master has AsyncEventHandlerGuard but no dedicated AsyncEventHandlerGuardTests file. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Merge the focused test coverage after required build/test validation confirms the new shared tests compile and do not add regressions. Do we have a high-confidence way to reproduce the issue? Not applicable; this is a test coverage PR rather than a user-reported bug. Source inspection shows current master has AsyncEventHandlerGuard but no dedicated AsyncEventHandlerGuardTests file. Is this the best way to solve the issue? Yes; adding focused unit tests under OpenClaw.Shared.Tests is the narrowest maintainable way to cover this shared fault boundary. The open item before merge is validation, not a code repair. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against e0ce52d2f16c. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
🤖 This PR was created by Repo Assist, an automated AI assistant.
Summary
AsyncEventHandlerGuardis a critical fault boundary that prevents fire-and-forget async event handlers from crashing the process viaasync voidescapes. Despite being used throughout the codebase (chat, transport, tray services), it had no dedicated unit tests.This PR adds 8 unit tests covering all exit paths:
Run_NullWork_ThrowsArgumentNullExceptionRun_SuccessfulWork_CompletesWithoutErrorRun_CancelledException_DoesNotInvokeOnErrorOperationCanceledExceptionis swallowed, not forwarded toonErrorRun_CancelledException_LogsDebugMessageRun_UnexpectedException_InvokesOnErroronErrorcallbackRun_UnexpectedException_LogsErrorWithExceptionRun_OperationName_AppearsInLogMessagesoperationNamesurfaces in log outputRun_NoCallbacks_ExceptionIsSwallowedCleanlyonError— fault is silently swallowed without crashingTest Status
Full
OpenClaw.Shared.Testssuite (2045 pass, 8 pre-existing failures inMxcPolicyBuilderTests/ExecApprovalV2NormalizationTestsunrelated to this change).OpenClaw.Connection.Tests: 267/267 passed.