Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e31eba86a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+501
to
+503
| if b.inFlightEventIDs[original_event.EventID] { | ||
| b.mu.Unlock() | ||
| return original_event, nil |
There was a problem hiding this comment.
When processEventImmediately sees the event already in inFlightEventIDs, it returns immediately instead of waiting for completion, so BaseEvent.Done/EventResult can return while handlers are still running. This is user-visible when a caller invokes Done after the runloop has already started processing the event: results can still be pending and the caller observes incomplete state instead of a completed event.
Useful? React with 👍 / 👎.
Comment on lines
+542
to
+544
| b.inFlightEventIDs[next_event.EventID] = true | ||
| b.mu.Unlock() | ||
| if err := b.processEvent(ctx, next_event, false, nil); err != nil && !errors.Is(err, context.Canceled) { |
There was a problem hiding this comment.
The runloop always processes each dequeued event synchronously, so events are serialized even when EventConcurrency is set to parallel. Because this path never spawns a concurrent worker for the next event, a second slow event on the same bus cannot start until the first finishes, which breaks the expected semantics of EventConcurrencyParallel and can severely reduce throughput.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| } | ||
| b.EventHistory.AddEvent(original_event) |
There was a problem hiding this comment.
Each emit appends to EventHistory but never applies MaxHistorySize/MaxHistoryDrop, so configured history limits are effectively ignored unless callers manually trim history. On long-running buses this causes unbounded history growth and defeats the memory/retention controls exposed in EventBusOptions.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
40 issues found across 38 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="bubus-go/tests/eventbus_find_test.go">
<violation number="1" location="bubus-go/tests/eventbus_find_test.go:11">
P0: Custom agent: **Test quality checker**
The Go `find` test file has only 2 basic happy-path tests (56 lines) versus Python's 40+ tests (1606 lines) and TS's 30+ tests (767 lines). This severely violates the rule that "tests must be bulletproof and hit every edge case." Missing critical test scenarios include: negative cases (nil on no-match, future timeout), time window filtering (`past` as float), bus-scoped history isolation, `where` filter with future waiting, `child_of` negative/grandchild/sibling-discrimination cases, concurrent waiters, wildcard patterns, `max_history_size=0`, default options behavior, and metadata field filters. Each of these is tested in both Python and TS and represents real edge cases users will hit in production with high-throughput event buses.</violation>
<violation number="2" location="bubus-go/tests/eventbus_find_test.go:41">
P1: Custom agent: **Make sure concurrency options work correctly and consistently**
Race condition: handler registered after `Emit()` in `TestFindChildOfFiltering`. The runloop goroutine launched by `Emit()` can process the `Parent` event before `bus.On(...)` registers the handler, meaning no `Child` event is ever emitted. This makes the test flaky and could mask real concurrency bugs. Register the handler **before** calling `Emit()`.</violation>
</file>
<file name="bubus-go/tests/event_handler_first_test.go">
<violation number="1" location="bubus-go/tests/event_handler_first_test.go:11">
P0: Custom agent: **Test quality checker**
This test file has drastically insufficient coverage compared to the TS equivalent (`bubus-ts/tests/event_handler_first.test.ts`, 20+ tests over 549 lines). The single test only covers the basic parallel happy-path race. Missing critical scenarios: handler cancellation verification, serial mode, error propagation, no-handler edge case, falsy value handling (0, false, empty string, null), retry integration, child event cancellation, and event metadata assertions.
The rule requires parity between implementations and bulletproof edge-case coverage for a system handling critical financial data. At minimum, port the TS test scenarios for: cancellation side-effects, serial mode, error handling, undefined/no-handler cases, and falsy value preservation.</violation>
</file>
<file name="bubus-go/tests/event_result_test.go">
<violation number="1" location="bubus-go/tests/event_result_test.go:11">
P0: Custom agent: **Test quality checker**
This test file is severely lacking coverage compared to the Python and TS counterparts. Only 2 superficial tests exist here, while Python has 10+ and TS has 12+ comprehensive tests for `EventResult`. Missing critical test scenarios include: result capture from handler execution, result type validation/casting, error handling for invalid results, result status transitions (pending → started → completed), handler concurrency (`event_handler_concurrency: serial`) lock-wait behavior, and result update ordering semantics. These are all tested in both Python and TS and are required by the rule to "hit every edge case" around "event_handler_concurrency / event_handler_completion config options / timeout enforcement."</violation>
<violation number="2" location="bubus-go/tests/event_result_test.go:24">
P1: Custom agent: **Test quality checker**
This roundtrip test only asserts 2 fields (`HandlerID` and `EventID`) after JSON deserialization, making it a shallow/"fake" test per the rule. A proper roundtrip test must verify all serialized fields survive the roundtrip correctly — including `status`, `result`, `error`, `handler_name`, `eventbus_name`, `eventbus_id`, `handler_registered_at`, and that internal-only fields are excluded from the wire format. See the TS `event_result.test.ts` for the expected level of assertion rigor.</violation>
</file>
<file name="bubus-go/tests/event_handler_test.go">
<violation number="1" location="bubus-go/tests/event_handler_test.go:10">
P0: Custom agent: **Test quality checker**
This is a shallow "fake" test that only verifies 2 fields (`ID` and `EventPattern`) survive a JSON roundtrip. The rule explicitly prohibits tests that "simply check if attrs/methods are present" — this test doesn't verify all serialized fields (`BusName`, `BusID`, `HandlerName`) survive roundtrip, doesn't validate the JSON wire format structure, and doesn't test any behavioral correctness of `EventHandler` (registration, execution, concurrency, completion, results). Compare with the Python tests (~470 lines covering completion modes, concurrency modes, cancellation, falsy results) and TS tests (~170 lines covering registration patterns, sync/async handlers, method handlers). This test needs to be substantially expanded to cover actual EventHandler behavior.</violation>
</file>
<file name="bubus-go/tests/base_event_test.go">
<violation number="1" location="bubus-go/tests/base_event_test.go:11">
P0: Custom agent: **Test quality checker**
This test file has critically inadequate coverage. It contains only 2 shallow tests for `BaseEvent`, while the TS equivalent has 15+ comprehensive behavioral tests. Missing coverage for: lifecycle state transitions, `done()` error re-raising, `eventResultUpdate` typed results, queue-jump child processing, `eventCompleted()` queue order, reserved field rejection, `toJSON`/`fromJSON` full roundtrips (including `event_results`, timestamps, nullable fields), event reset, and status hooks. The rule requires tests to be "high quality, rigorous" and "bulletproof and hit every edge case." These 2 tests come nowhere close to exercising the critical paths of BaseEvent behavior.</violation>
<violation number="2" location="bubus-go/tests/base_event_test.go:13">
P1: Custom agent: **Test quality checker**
`TestBaseEventDoneWithoutBus` is a "fake" test per the rule — it only asserts that `Done()` returns a non-nil error without verifying the error message content (e.g., checking it mentions missing bus), the event's status after the call, or any other behavioral invariant. The rule requires that "tests must assert that all writes, mutations, side effects, and output values are actually behaviorally correct," not just that a call returns *some* error.</violation>
</file>
<file name="bubus-go/event_history.go">
<violation number="1" location="bubus-go/event_history.go:131">
P0: Custom agent: **Make sure concurrency options work correctly and consistently**
`EventIsChildOfStatic` is missing cycle detection when traversing the parent chain, which can cause an infinite loop if parent references form a cycle. The Python implementation uses a `visited: set[str]` to guard against this. Add a visited set to break out of circular references.</violation>
<violation number="2" location="bubus-go/event_history.go:165">
P1: `TrimEventHistory` ignores `MaxHistoryDrop` — events will be trimmed even when `MaxHistoryDrop` is `false`. The Python implementation returns 0 immediately when `max_history_drop` is `False`, but the Go version skips this guard entirely. Add a check for `!h.MaxHistoryDrop` early in the method to match the Python behavior.</violation>
</file>
<file name="bubus-go/event_bus.go">
<violation number="1" location="bubus-go/event_bus.go:387">
P0: Custom agent: **Make sure concurrency options work correctly and consistently**
Data race and busy-wait polling in concurrent first-completion handler execution. When `EventHandlerCompletionFirst` is combined with parallel concurrency, handler goroutines write to `r.Status`/`r.Result` while the main goroutine reads them in a polling loop without any synchronization (mutex, atomic, or channel). This is a Go data race that will corrupt reads in production and trigger `go test -race` failures.
The proper fix is to use a channel or condition variable so handlers signal completion, eliminating both the data race and the `time.After(time.Millisecond)` busy-wait poll (which adds per-event overhead incompatible with the 0.2ms/event performance target at scale).</violation>
<violation number="2" location="bubus-go/event_bus.go:853">
P2: Bug: Duration is computed backwards (`start - end` instead of `end - start`), producing a negative value. Should be `completed_at.Sub(started_at)`.</violation>
</file>
<file name="bubus-go/lock_manager.go">
<violation number="1" location="bubus-go/lock_manager.go:27">
P0: Custom agent: **Make sure concurrency options work correctly and consistently**
`AsyncLock` is not re-entrant and will **deadlock** during queue-jumping (`event.Done(ctx)`) in `bus-serial` or `global-serial` mode. When a handler holds the event lock and emits a child event that needs the same lock, the channel is already full, causing the goroutine to block forever.
Both Python (`ReentrantLock` with `ContextVar` depth tracking) and TypeScript (`HandlerLock` with yield/reclaim + `bypass_event_locks`) correctly handle re-entrant lock acquisition during queue-jumps. The Go implementation needs an equivalent mechanism — either goroutine-aware re-entrancy tracking or a yield/reclaim pattern.</violation>
<violation number="2" location="bubus-go/lock_manager.go:180">
P1: Custom agent: **Make sure concurrency options work correctly and consistently**
`waitForIdle` uses busy-polling with `time.Sleep(time.Millisecond)` instead of a notification-based approach. This wastes CPU in high-throughput environments and adds minimum 1ms latency to idle detection. The TypeScript implementation uses proper event-driven notification (`_notifyIdleListeners` + waiter resolution), which has zero overhead when no waiters exist. Consider implementing a channel-based notification pattern where event/handler completion signals idle waiters directly.</violation>
</file>
<file name="bubus-go/tests/eventbus_locking_test.go">
<violation number="1" location="bubus-go/tests/eventbus_locking_test.go:31">
P1: Custom agent: **Test quality checker**
This test only asserts the negative case (second handler doesn't start within 20ms) but never verifies that both handlers actually complete, that FIFO order is preserved, or that `max_in_flight == 1` across multiple events. After `close(release)`, the test ends with no further assertions. The test would pass even if the second event was silently dropped.
The Python and TS equivalents emit multiple events per bus, track `max_in_flight`, assert it equals 1, verify FIFO ordering, wait for all buses to become idle, and clean up. This test should do the same to be rigorous and match the quality standard.</violation>
</file>
<file name="bubus-go/tests/eventbus_dispatch_defaults_test.go">
<violation number="1" location="bubus-go/tests/eventbus_dispatch_defaults_test.go:10">
P1: Custom agent: **Test quality checker**
This test file is named `eventbus_dispatch_defaults_test.go` but doesn't test any dispatch defaults behavior. The Python and TS counterparts each contain 4 tests verifying concurrency mode propagation/override semantics (`event_concurrency`, `event_handler_concurrency`, `event_handler_completion`). This single test only verifies that `Dispatch` returns a value — it doesn't assert any behavioral correctness around how defaults propagate or override, making it a "fake" test per the rule: *"no 'fake' tests that simply check if attrs/methods are present or that calls run without error, tests must assert that all writes, mutations, side effects, and output values are actually behaviorally correct"*.</violation>
</file>
<file name="bubus-go/tests/eventbus_serialization_test.go">
<violation number="1" location="bubus-go/tests/eventbus_serialization_test.go:23">
P1: Custom agent: **Test quality checker**
This is a "fake" test that only checks JSON key existence and a single field after roundtrip — the rule explicitly forbids tests that "simply check if attrs/methods are present" without asserting behavioral correctness. The Python and TS serialization tests verify **all** config fields (id, name, max_history_size, event_concurrency, event_handler_concurrency, event_handler_completion, timeouts, etc.), handler cross-references, event result integrity, missing handler recreation from event results, and pending event promotion into history. This Go test checks none of that.
At minimum, the roundtrip should verify all serialized config fields match, that `handlers`/`handlers_by_key` contain the correct entries, that `event_history` contains the emitted event with its results, and that `pending_event_queue` references resolve correctly. Additional test cases for missing-handler recreation and pending-event promotion are needed for parity with Python/TS.</violation>
</file>
<file name="bubus-go/tests/lock_manager_test.go">
<violation number="1" location="bubus-go/tests/lock_manager_test.go:10">
P1: Custom agent: **Test quality checker**
This test file is severely inadequate compared to the TS parity target. The single `TestWaitUntilIdle` test is essentially a "fake" test — it only verifies that `WaitUntilIdle` returns `true` without asserting any behavioral correctness (handler return value, event status, event results, side effects). The emit result is discarded (`_ = bus.Emit(...)`) and the handler uses `any` as its return type.
The TS `lock_manager.test.ts` has 8 rigorous tests (202 lines) covering lock fairness, semaphore concurrency limits, error propagation, queue-jump yield/reacquire, re-entrant pause semantics, and two-check idle stability. This Go file needs equivalent coverage of the lock manager's actual behavioral contract, with strict types and assertions on all outputs/side effects.</violation>
</file>
<file name="bubus-go/tests/eventbus_log_tree_test.go">
<violation number="1" location="bubus-go/tests/eventbus_log_tree_test.go:28">
P1: Custom agent: **Test quality checker**
This test is essentially a "fake" test per the rule: it claims to verify timed-out result errors in the log tree, but the assertions only check that the event/handler *names* appear in the output — never that any error or timeout indication is actually rendered. The rule explicitly forbids tests that "simply check if attrs/methods are present or that calls run without error" and requires that "tests must assert that all writes, mutations, side effects, and output values are actually behaviorally correct."
At minimum, this test should assert that the tree output contains an error/timeout indicator (e.g., `❌`, `timeout`, or the error message) to confirm the timeout was actually enforced and displayed.</violation>
</file>
<file name="bubus-go/tests/eventhistory_methods_test.go">
<violation number="1" location="bubus-go/tests/eventhistory_methods_test.go:20">
P1: Custom agent: **Test quality checker**
Test uses overly loose assertions that don't verify behavioral correctness. `removed == 0` should be `removed != 1` (exact count), `h.Size() > 2` should be `h.Size() != 2` (exact size), and after `RemoveEvent` the test should verify the final size and which specific events remain. As written, this test would pass even if `TrimEventHistory` removed all events or the wrong events — violating the rule that tests must "assert that all writes, mutations, side effects, and output values are actually behaviorally correct."</violation>
</file>
<file name="bubus-go/tests/event_results_list_test.go">
<violation number="1" location="bubus-go/tests/event_results_list_test.go:11">
P1: Custom agent: **Test quality checker**
This test only covers a single happy-path scenario for `EventResultsList` (both `RaiseIfAny` and `RaiseIfNone` set to `false`). It sets up 3 handlers but never verifies the error/nil handler behavior, never tests `RaiseIfAny: true` (which should propagate the error), never tests `RaiseIfNone: true`, and never inspects individual `EventResults` to confirm statuses. Per the rule: tests must assert all side effects are behaviorally correct and must hit every edge case across config options. Add test cases for:
- `RaiseIfAny: true` returning an error when a handler fails
- `RaiseIfNone: true` raising when no valid results exist
- Verification of individual handler result statuses (completed/error)
- Edge cases: all-error handlers, all-nil handlers, no handlers</violation>
</file>
<file name="bubus-go/tests/eventhistory_find_test.go">
<violation number="1" location="bubus-go/tests/eventhistory_find_test.go:9">
P1: Custom agent: **Test quality checker**
This test file is far too thin for a critical component like `EventHistory.Find`. With only one test covering two basic scenarios (past+child_of match and past=false miss), it misses the vast majority of edge cases that the TS equivalent tests thoroughly: future event waiting, time-window filtering (`past` as float), `where` predicate filtering, wildcard matching, empty history, multiple concurrent find operations, metadata/field filters, and event_status filtering. The rule requires tests to be "bulletproof and hit every edge case" and maintain parity with the comprehensive Python/TS test coverage (~30+ test cases for find behavior). This single test provides negligible confidence in the correctness of `EventHistory.Find`.</violation>
</file>
<file name="bubus-go/tests/eventbus_forwarding_parity_test.go">
<violation number="1" location="bubus-go/tests/eventbus_forwarding_parity_test.go:72">
P1: Custom agent: **Test quality checker**
The `idx()` helper returns `-1` for missing entries, making the ordering assertions silently pass even if handlers never execute. For example, if `inherited:b2_start` is never logged, `idx("inherited:b2_start")` returns `-1`, and `-1 < idx("inherited:b1_end")` is always true — the test passes despite the handler never running. This is effectively a "fake" test that the rule explicitly prohibits.
Before the ordering assertions, the test must verify that all expected log entries (`b1_start`, `b1_end`, `b2_start`, `b2_end` for both `inherited` and `override` modes) are actually present. Additionally, `idx()` should fatal on missing entries rather than returning a sentinel value.</violation>
</file>
<file name="bubus-go/tests/eventbus_error_handling_test.go">
<violation number="1" location="bubus-go/tests/eventbus_error_handling_test.go:23">
P1: Custom agent: **Test quality checker**
Error handling test coverage is critically thin — only 2 tests vs. 8 in the TS implementation and 4 in Python. This file is missing tests for timeout errors, error collection/aggregation (`event_errors`), queue isolation (error in one event not affecting subsequent queued events), cross-bus error forwarding, error result field validation (`started_at`, `completed_at`, `handler_name`, `eventbus_name` on error results), no-handler completion, and async error handling. The rule requires tests to be "bulletproof and hit every edge case" for a system handling critical financial data, and the test quality rule requires rigorous coverage organized as "a few longer tests that check everything about a given method or behavior."</violation>
</file>
<file name="bubus-go/event_handler.go">
<violation number="1" location="bubus-go/event_handler.go:54">
P1: Handler IDs will differ from Python/TS due to wrong UUID namespace. Both Python and TS derive a custom namespace `HANDLER_ID_NAMESPACE = uuid5(DNS, 'bubus-handler')` and then compute `uuid5(HANDLER_ID_NAMESPACE, seed)`. The Go `deterministicUUID` function uses `uuid.NameSpaceDNS` directly, producing incompatible IDs. Define a `handlerIDNamespace` constant (e.g., `uuid.NewSHA1(uuid.NameSpaceDNS, []byte("bubus-handler"))`) and use it when computing handler IDs.</violation>
</file>
<file name="bubus-go/go.mod">
<violation number="1" location="bubus-go/go.mod:1">
P1: Custom agent: **Make sure READMEs, docs/, and examples/ are up-to-date with implementation**
The main `README.md` and `docs/` are not updated to reflect the new Go implementation. The README still describes bubus as supporting only "Python and TS (node/browser)" and neither the README nor any `docs/**/*.mdx` file mentions Go. Per the rule, anytime code changes are made, the READMEs and docs must be updated to match — adding an entire new language implementation is a major documentation gap.
At minimum:
- `README.md` should mention Go support (title, description, quickstart/install instructions, supported runtimes table)
- `docs/operations/supported-runtimes.mdx` should list Go
- `docs/quickstart.mdx` should include Go installation/usage
- Relevant API docs under `docs/api/` should note Go equivalents where applicable</violation>
</file>
<file name="bubus-go/base_event.go">
<violation number="1" location="bubus-go/base_event.go:34">
P1: Custom agent: **Make sure concurrency options work correctly and consistently**
`EventResults` is a Go `map[string]*EventResult` with non-deterministic iteration order. Methods `EventResult()`, `First()`, and `EventResultsList()` all iterate this map to find the first completed/error result. Since Go randomizes map iteration order, these methods will return unpredictable results when multiple handlers are registered. This breaks consistency with Python (which uses insertion-ordered dicts) and makes event result aggregation unreliable. Consider using an ordered data structure (e.g., a slice of results preserving insertion order, or a `sync.Map` with a separate ordered key slice) to maintain deterministic handler result ordering.</violation>
<violation number="2" location="bubus-go/base_event.go:80">
P1: Payload keys can silently overwrite event metadata during JSON serialization. If Payload contains a key matching any metadata field name (e.g., `"event_id"`, `"event_status"`), it will overwrite the real metadata value in the output JSON. Either merge payload first and metadata second (so metadata always wins), or skip known keys when writing payload.</violation>
<violation number="3" location="bubus-go/base_event.go:241">
P1: Custom agent: **Make sure concurrency options work correctly and consistently**
`First()` does not set `EventHandlerCompletion` to `FIRST` mode before calling `Done()`, unlike the Python implementation which sets `self.event_handler_completion = EventHandlerCompletionMode.FIRST`. This means the Go version always waits for ALL handlers to complete, defeating the purpose of the `event_handler_completion` concurrency option. The `event_handler_completion='first'` mode is designed to short-circuit handler execution — the bus should stop running remaining handlers once the first one returns a non-None result.</violation>
</file>
<file name="bubus-go/helpers.go">
<violation number="1" location="bubus-go/helpers.go:22">
P1: Custom agent: **Make sure concurrency options work correctly and consistently**
`time.Now().UTC()` strips Go's monotonic clock reading, making this function rely on wall-clock time that can go backwards during NTP adjustments. The Python implementation uses `time.monotonic_ns()` anchored to wall time at startup to guarantee true monotonicity.
Use a monotonic clock source (e.g., anchor `time.Now()` at init and use its monotonic component via `time.Since()`, or use a `nanotime` approach) to match the Python implementation's behavior and ensure correct event ordering under clock skew.</violation>
<violation number="2" location="bubus-go/helpers.go:27">
P1: Custom agent: **Make sure concurrency options work correctly and consistently**
`time.RFC3339Nano` trims trailing zeros from fractional seconds (e.g., `.1Z` instead of `.100000000Z`), breaking wire format parity with Python which always outputs exactly 9 fractional digits. This will cause incorrect string-based timestamp ordering across languages and violates the claimed JSON-compatible wire format.
Use a custom format string like `"2006-01-02T15:04:05.000000000Z"` to always produce exactly 9 fractional digits, matching the Python canonical format.</violation>
</file>
<file name="bubus-go/tests/event_history_test.go">
<violation number="1" location="bubus-go/tests/event_history_test.go:9">
P1: Custom agent: **Test quality checker**
This test is too shallow for a critical data system. `TestEventHistoryBasic` only verifies the trivial happy path (add 2 events, check size/has/get/values). It's missing edge case coverage that the rule requires:
- **Duplicate event dedup**: `AddEvent` with the same event twice should not increase size (line 34-37 of `event_history.go` has this logic, untested)
- **Negative lookups**: `Has(unknownID)` → false, `GetEvent(unknownID)` → nil never asserted
- **Empty history**: No assertions on fresh `EventHistory` (size 0, empty values)
- **Default MaxHistorySize**: Passing `nil` defaults to 100, but this isn't verified
Also, per the rule's preference for "a few longer tests that check everything about a given method or behavior" over "10 separate tiny ones," this should be consolidated with `eventhistory_methods_test.go` into a single comprehensive test file.</violation>
</file>
<file name="bubus-go/tests/eventbus_concurrency_test.go">
<violation number="1" location="bubus-go/tests/eventbus_concurrency_test.go:27">
P1: Custom agent: **Test quality checker**
This test is flaky: it uses `time.Sleep(15ms)` to racily check that both parallel handlers have started. Under CI load, goroutine scheduling can easily exceed 15ms, causing false failures. Use a deterministic synchronization mechanism (e.g., a `sync.WaitGroup` or a buffered `started` channel that each handler sends on) to confirm both handlers are running before asserting. The rule requires "no flaky tests allowed."</violation>
</file>
<file name="bubus-go/tests/eventbus_on_off_test.go">
<violation number="1" location="bubus-go/tests/eventbus_on_off_test.go:11">
P1: Custom agent: **Test quality checker**
Test only asserts a side-effect call counter and never verifies event results, handler registry state, or off-by-multiple-methods. The rule requires tests to "assert that all writes, mutations, side effects, and output values are actually behaviorally correct" and forbids tests that only check "calls run without error." The TS equivalent (`eventbus_on_off.test.ts`) has 6 comprehensive tests covering handler registration state, multiple `off` removal methods (by callable, ID, entry, and remove-all), result values/status, and typed contracts. This Go test should match that rigor: check `EventResult` values from `Emit`, verify handler registry state after `On`/`Off`, and test all `Off` overload variants.</violation>
</file>
<file name="bubus-go/tests/eventbus_idle_test.go">
<violation number="1" location="bubus-go/tests/eventbus_idle_test.go:11">
P1: Custom agent: **Test quality checker**
This test is significantly less rigorous than its TS counterpart (`eventbus.test.ts:31-58`). It's missing:
- **Timing bound assertions**: The TS test verifies `elapsed_ms >= 30` and `elapsed_ms < 1000` to ensure the timeout is actually enforced within expected bounds. The Go test doesn't measure elapsed time at all.
- **`IsIdleAndQueueEmpty()` check after timeout**: The TS test asserts `bus.isIdleAndQueueEmpty() === false` while work is still in-flight. This test skips that.
- **No side-effect verification**: Neither Go idle test verifies that the handler actually executed to completion (e.g., via a counter or recorded value). The rule requires tests to "assert that all writes, mutations, side effects, and output values are actually behaviorally correct."
These gaps reduce confidence in timeout enforcement correctness — critical for high-throughput financial workloads.</violation>
</file>
<file name="bubus-go/tests/eventbus_teardown_test.go">
<violation number="1" location="bubus-go/tests/eventbus_teardown_test.go:10">
P1: Custom agent: **Test quality checker**
This test lacks pre-destroy state assertions, making the post-destroy checks meaningless — it cannot prove `Destroy()` actually cleared anything. The rule requires tests to "assert that all writes, mutations, side effects, and output values are actually behaviorally correct," not just check post-conditions in isolation.
At minimum, the test should:
1. Track that the handler actually executed (e.g., increment a counter)
2. Assert `EventHistory.Size() > 0` and event status **before** `Destroy()`
3. Then assert the post-destroy state
4. Use a concrete return type instead of `any`</violation>
</file>
<file name="bubus-go/tests/eventbus_queue_jump_test.go">
<violation number="1" location="bubus-go/tests/eventbus_queue_jump_test.go:30">
P1: Custom agent: **Test quality checker**
Test uses `context.Background()` without a timeout deadline, risking indefinite hangs in CI. The rule explicitly allows hardcoded timeout values to ensure tests complete in a reasonable time. Use `context.WithTimeout` to add a deadline (e.g., 5s) so the test fails fast instead of hanging.</violation>
<violation number="2" location="bubus-go/tests/eventbus_queue_jump_test.go:34">
P1: Custom agent: **Test quality checker**
Test only asserts one boolean flag but ignores handler return values (`"parent"`, `"child"`), the parent event's final completion status, and parent-child relationship tracking. The rule requires asserting all output values and side effects are behaviorally correct. At minimum, assert the return values from both handlers and the final event statuses.</violation>
</file>
<file name="bubus-go/tests/eventbus_completion_modes_test.go">
<violation number="1" location="bubus-go/tests/eventbus_completion_modes_test.go:11">
P1: Custom agent: **Test quality checker**
These completion-mode tests are too shallow — the `All` test never verifies the actual handler result values (only a side-effect boolean), and only 2 of 4 completion × concurrency combinations are covered (`all+parallel`, `first+serial`). Missing: `all+serial` and `first+parallel`.
Per the test quality rule: "tests must assert that all writes, mutations, side effects, **and output values** are actually behaviorally correct" and "hit every edge case… across all the various config options" (specifically calling out `event_handler_concurrency` / `event_handler_completion`).
At minimum, the `All` test should assert the collected results contain both `"fast"` and `"slow"`, and tests for the missing mode combinations should be added.</violation>
</file>
<file name="bubus-go/tests/eventbus_timeout_test.go">
<violation number="1" location="bubus-go/tests/eventbus_timeout_test.go:22">
P1: Custom agent: **Test quality checker**
This test only asserts that *some* error occurred (`err == nil`), but doesn't verify it's actually a timeout/deadline error. Any unrelated failure would make this test pass, violating the rule that tests must "assert that all writes, mutations, side effects, and output values are actually behaviorally correct" and not be "fake" tests that merely check a call errors without verifying the error is correct. Both Python and TS equivalents assert the specific error type (e.g., `EventHandlerAbortedError`). At minimum, assert `errors.Is(err, context.DeadlineExceeded)` to confirm the timeout path was triggered.</violation>
</file>
Architecture diagram
sequenceDiagram
participant C as Client / Handler
participant B as EventBus
participant LM as LockManager
participant EH as EventHandler
participant Hist as EventHistory
Note over C, Hist: Event Emission & Queueing
C->>B: Emit(BaseEvent)
B->>Hist: NEW: AddEvent(event)
B->>B: Push to pendingEventQueue
B->>B: startRunloop() (Async)
Note over B, EH: Runloop Execution
loop For each Pending Event
B->>LM: NEW: getLockForEvent(event)
LM-->>B: Lock (Global/Bus/None)
B->>B: markStarted()
par NEW: Handler Concurrency (Parallel/Serial)
B->>LM: runWithHandlerDispatchContext()
B->>EH: NEW: Handle(ctx, event)
alt NEW: Queue-Jump (Inside Handler)
EH->>B: event.Done(ctx)
B->>B: processEventImmediately()
B-->>EH: Return Child Result
end
EH-->>B: Result / Error
end
alt NEW: Completion Mode (First/All)
Note right of B: Wait for first non-nil OR all handlers
end
B->>B: markCompleted()
B->>LM: Release Lock
end
Note over C, B: Synchronous Interaction
C->>B: NEW: EventResult(ctx) / Done(ctx)
B->>B: processEventImmediately (if pending)
B->>B: Wait for done_ch
B-->>C: Return Handler Result
Note over C, Hist: History Management
C->>Hist: NEW: Find(pattern, filter, options)
Hist-->>C: Return matching event (Past/Future)
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,39 @@ | |||
| package bubus_test | |||
Contributor
There was a problem hiding this comment.
P0: Custom agent: Test quality checker
This test file is severely lacking coverage compared to the Python and TS counterparts. Only 2 superficial tests exist here, while Python has 10+ and TS has 12+ comprehensive tests for EventResult. Missing critical test scenarios include: result capture from handler execution, result type validation/casting, error handling for invalid results, result status transitions (pending → started → completed), handler concurrency (event_handler_concurrency: serial) lock-wait behavior, and result update ordering semantics. These are all tested in both Python and TS and are required by the rule to "hit every edge case" around "event_handler_concurrency / event_handler_completion config options / timeout enforcement."
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bubus-go/tests/event_result_test.go, line 11:
<comment>This test file is severely lacking coverage compared to the Python and TS counterparts. Only 2 superficial tests exist here, while Python has 10+ and TS has 12+ comprehensive tests for `EventResult`. Missing critical test scenarios include: result capture from handler execution, result type validation/casting, error handling for invalid results, result status transitions (pending → started → completed), handler concurrency (`event_handler_concurrency: serial`) lock-wait behavior, and result update ordering semantics. These are all tested in both Python and TS and are required by the rule to "hit every edge case" around "event_handler_concurrency / event_handler_completion config options / timeout enforcement."</comment>
<file context>
@@ -0,0 +1,39 @@
+ bubus "github.com/pirate/bbus/bubus-go"
+)
+
+func TestEventResultJSONRoundtrip(t *testing.T) {
+ bus := bubus.NewEventBus("ResultBus", nil)
+ h := bubus.NewEventHandler(bus.Name, bus.ID, "Evt", "h", nil)
</file context>
| @@ -0,0 +1,23 @@ | |||
| package bubus_test | |||
Contributor
There was a problem hiding this comment.
P0: Custom agent: Test quality checker
This is a shallow "fake" test that only verifies 2 fields (ID and EventPattern) survive a JSON roundtrip. The rule explicitly prohibits tests that "simply check if attrs/methods are present" — this test doesn't verify all serialized fields (BusName, BusID, HandlerName) survive roundtrip, doesn't validate the JSON wire format structure, and doesn't test any behavioral correctness of EventHandler (registration, execution, concurrency, completion, results). Compare with the Python tests (~470 lines covering completion modes, concurrency modes, cancellation, falsy results) and TS tests (~170 lines covering registration patterns, sync/async handlers, method handlers). This test needs to be substantially expanded to cover actual EventHandler behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bubus-go/tests/event_handler_test.go, line 10:
<comment>This is a shallow "fake" test that only verifies 2 fields (`ID` and `EventPattern`) survive a JSON roundtrip. The rule explicitly prohibits tests that "simply check if attrs/methods are present" — this test doesn't verify all serialized fields (`BusName`, `BusID`, `HandlerName`) survive roundtrip, doesn't validate the JSON wire format structure, and doesn't test any behavioral correctness of `EventHandler` (registration, execution, concurrency, completion, results). Compare with the Python tests (~470 lines covering completion modes, concurrency modes, cancellation, falsy results) and TS tests (~170 lines covering registration patterns, sync/async handlers, method handlers). This test needs to be substantially expanded to cover actual EventHandler behavior.</comment>
<file context>
@@ -0,0 +1,23 @@
+ bubus "github.com/pirate/bbus/bubus-go"
+)
+
+func TestEventHandlerJSONRoundtrip(t *testing.T) {
+ h := bubus.NewEventHandler("Bus", "bus-id", "Evt", "h", func(ctx context.Context, event *bubus.BaseEvent) (any, error) { return "ok", nil })
+ data, err := h.ToJSON()
</file context>
| @@ -0,0 +1,37 @@ | |||
| package bubus_test | |||
Contributor
There was a problem hiding this comment.
P0: Custom agent: Test quality checker
This test file has critically inadequate coverage. It contains only 2 shallow tests for BaseEvent, while the TS equivalent has 15+ comprehensive behavioral tests. Missing coverage for: lifecycle state transitions, done() error re-raising, eventResultUpdate typed results, queue-jump child processing, eventCompleted() queue order, reserved field rejection, toJSON/fromJSON full roundtrips (including event_results, timestamps, nullable fields), event reset, and status hooks. The rule requires tests to be "high quality, rigorous" and "bulletproof and hit every edge case." These 2 tests come nowhere close to exercising the critical paths of BaseEvent behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bubus-go/tests/base_event_test.go, line 11:
<comment>This test file has critically inadequate coverage. It contains only 2 shallow tests for `BaseEvent`, while the TS equivalent has 15+ comprehensive behavioral tests. Missing coverage for: lifecycle state transitions, `done()` error re-raising, `eventResultUpdate` typed results, queue-jump child processing, `eventCompleted()` queue order, reserved field rejection, `toJSON`/`fromJSON` full roundtrips (including `event_results`, timestamps, nullable fields), event reset, and status hooks. The rule requires tests to be "high quality, rigorous" and "bulletproof and hit every edge case." These 2 tests come nowhere close to exercising the critical paths of BaseEvent behavior.</comment>
<file context>
@@ -0,0 +1,37 @@
+ bubus "github.com/pirate/bbus/bubus-go"
+)
+
+func TestBaseEventDoneWithoutBus(t *testing.T) {
+ e := bubus.NewBaseEvent("NoBus", nil)
+ if _, err := e.Done(context.Background()); err == nil {
</file context>
| } | ||
| monotonic_mu.Lock() | ||
| defer monotonic_mu.Unlock() | ||
| now := time.Now().UTC() |
Contributor
There was a problem hiding this comment.
P1: Custom agent: Make sure concurrency options work correctly and consistently
time.Now().UTC() strips Go's monotonic clock reading, making this function rely on wall-clock time that can go backwards during NTP adjustments. The Python implementation uses time.monotonic_ns() anchored to wall time at startup to guarantee true monotonicity.
Use a monotonic clock source (e.g., anchor time.Now() at init and use its monotonic component via time.Since(), or use a nanotime approach) to match the Python implementation's behavior and ensure correct event ordering under clock skew.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bubus-go/helpers.go, line 22:
<comment>`time.Now().UTC()` strips Go's monotonic clock reading, making this function rely on wall-clock time that can go backwards during NTP adjustments. The Python implementation uses `time.monotonic_ns()` anchored to wall time at startup to guarantee true monotonicity.
Use a monotonic clock source (e.g., anchor `time.Now()` at init and use its monotonic component via `time.Since()`, or use a `nanotime` approach) to match the Python implementation's behavior and ensure correct event ordering under clock skew.</comment>
<file context>
@@ -0,0 +1,32 @@
+ }
+ monotonic_mu.Lock()
+ defer monotonic_mu.Unlock()
+ now := time.Now().UTC()
+ if !now.After(monotonic_last) {
+ now = monotonic_last.Add(time.Nanosecond)
</file context>
| if !deadline.IsZero() && time.Now().After(deadline) { | ||
| return false | ||
| } | ||
| time.Sleep(time.Millisecond) |
Contributor
There was a problem hiding this comment.
P1: Custom agent: Make sure concurrency options work correctly and consistently
waitForIdle uses busy-polling with time.Sleep(time.Millisecond) instead of a notification-based approach. This wastes CPU in high-throughput environments and adds minimum 1ms latency to idle detection. The TypeScript implementation uses proper event-driven notification (_notifyIdleListeners + waiter resolution), which has zero overhead when no waiters exist. Consider implementing a channel-based notification pattern where event/handler completion signals idle waiters directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bubus-go/lock_manager.go, line 180:
<comment>`waitForIdle` uses busy-polling with `time.Sleep(time.Millisecond)` instead of a notification-based approach. This wastes CPU in high-throughput environments and adds minimum 1ms latency to idle detection. The TypeScript implementation uses proper event-driven notification (`_notifyIdleListeners` + waiter resolution), which has zero overhead when no waiters exist. Consider implementing a channel-based notification pattern where event/handler completion signals idle waiters directly.</comment>
<file context>
@@ -0,0 +1,191 @@
+ if !deadline.IsZero() && time.Now().After(deadline) {
+ return false
+ }
+ time.Sleep(time.Millisecond)
+ }
+}
</file context>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+435
to
+436
| if resolved_handler_timeout == nil { | ||
| resolved_handler_timeout = bus.EventTimeout |
There was a problem hiding this comment.
🟡 Handler timeout fallback uses bus.EventTimeout instead of bus.EventHandlerTimeout
In runSingleHandler, the handler timeout resolution chain falls back to bus.EventTimeout (the overall event timeout) when no handler-specific or event-level handler timeout is set. This means individual handlers get the same timeout as the entire event, which is incorrect when multiple serial handlers share the event timeout budget.
Root Cause
At bubus-go/event_bus.go:435-436:
if resolved_handler_timeout == nil {
resolved_handler_timeout = bus.EventTimeout
}The fallback chain is: handler.HandlerTimeout → event.EventHandlerTimeout → bus.EventTimeout.
When there are multiple serial handlers and no explicit handler timeout, each handler gets the full event timeout. For example, with a 60s event timeout and 3 serial handlers, each handler gets 60s, meaning the total could be 180s — far exceeding the event timeout. While the event-level timeout in processEvent will eventually fire, the per-handler timeout is misleading and won't provide the expected per-handler time-bounding behavior.
The EventBus struct doesn't have an EventHandlerTimeout field, so there's no bus-level default for handler timeouts. This appears to be a missing field — the EventBusOptions should likely include an EventHandlerTimeout option.
Impact: Handler-level timeouts are effectively disabled when not explicitly set, since they default to the full event timeout rather than a fraction of it.
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
There was a problem hiding this comment.
4 issues found across 30 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="bubus-go/tests/event_result_test.go">
<violation number="1" location="bubus-go/tests/event_result_test.go:43">
P1: Custom agent: **Test quality checker**
Test sets `StartedAt` and `CompletedAt` before serialization but never verifies them after the roundtrip. Per the test quality rule, tests must assert that **all writes and output values are behaviorally correct** — not just a subset of the fields. A serialization bug in these timestamp fields would silently pass.
Add assertions like:
```go
if round.StartedAt == nil || *round.StartedAt != *r.StartedAt {
t.Fatalf("started_at mismatch: %v vs %v", round.StartedAt, r.StartedAt)
}
if round.CompletedAt == nil || *round.CompletedAt != *r.CompletedAt {
t.Fatalf("completed_at mismatch: %v vs %v", round.CompletedAt, r.CompletedAt)
}
```</violation>
</file>
<file name="bubus-go/tests/event_bus_test.go">
<violation number="1" location="bubus-go/tests/event_bus_test.go:50">
P1: Custom agent: **Test quality checker**
Test retrieves `EventResultsList` values but only asserts the count (`len(values) != 2`), not the actual content. Per the test quality rule, tests must verify that output values are behaviorally correct — checking only the length is insufficient. Assert that `values[0]` equals `"first"` and `values[1]` is the expected `map[string]any{"user_id": "abc"}` (or vice versa depending on ordering guarantees).</violation>
</file>
<file name="bubus-go/event_result.go">
<violation number="1" location="bubus-go/event_result.go:143">
P2: Mutex held unnecessarily during `json.Marshal(payload)`. The `payload` is already a fully independent copy (including the defensive `append([]*BaseEvent{}, ...)` for `EventChildren`), so the lock should be released before marshaling. Holding it during marshaling of nested `[]*BaseEvent` (which trigger their own `MarshalJSON` → serialize `EventResults` → call `EventResult.MarshalJSON`) increases lock hold time and risks deadlock if the object graph ever contains cycles back to this result.</violation>
</file>
<file name="bubus-go/helpers.go">
<violation number="1" location="bubus-go/helpers.go:11">
P2: `fixedRFC3339Nano` should be declared as `const` instead of `var`. It's an immutable format string, and `var` allows accidental mutation from anywhere within the package.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+43
to
+45
| if round.Result != "ok" || round.Error != "boom" { | ||
| t.Fatalf("result/error mismatch after roundtrip: %#v %#v", round.Result, round.Error) | ||
| } |
Contributor
There was a problem hiding this comment.
P1: Custom agent: Test quality checker
Test sets StartedAt and CompletedAt before serialization but never verifies them after the roundtrip. Per the test quality rule, tests must assert that all writes and output values are behaviorally correct — not just a subset of the fields. A serialization bug in these timestamp fields would silently pass.
Add assertions like:
if round.StartedAt == nil || *round.StartedAt != *r.StartedAt {
t.Fatalf("started_at mismatch: %v vs %v", round.StartedAt, r.StartedAt)
}
if round.CompletedAt == nil || *round.CompletedAt != *r.CompletedAt {
t.Fatalf("completed_at mismatch: %v vs %v", round.CompletedAt, r.CompletedAt)
}Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bubus-go/tests/event_result_test.go, line 43:
<comment>Test sets `StartedAt` and `CompletedAt` before serialization but never verifies them after the roundtrip. Per the test quality rule, tests must assert that **all writes and output values are behaviorally correct** — not just a subset of the fields. A serialization bug in these timestamp fields would silently pass.
Add assertions like:
```go
if round.StartedAt == nil || *round.StartedAt != *r.StartedAt {
t.Fatalf("started_at mismatch: %v vs %v", round.StartedAt, r.StartedAt)
}
if round.CompletedAt == nil || *round.CompletedAt != *r.CompletedAt {
t.Fatalf("completed_at mismatch: %v vs %v", round.CompletedAt, r.CompletedAt)
}
```</comment>
<file context>
@@ -21,8 +28,20 @@ func TestEventResultJSONRoundtrip(t *testing.T) {
+ if round.HandlerName != h.HandlerName || round.EventBusName != h.EventBusName || round.EventBusID != h.EventBusID {
+ t.Fatal("handler/event bus metadata mismatch")
+ }
+ if round.Result != "ok" || round.Error != "boom" {
+ t.Fatalf("result/error mismatch after roundtrip: %#v %#v", round.Result, round.Error)
}
</file context>
Suggested change
| if round.Result != "ok" || round.Error != "boom" { | |
| t.Fatalf("result/error mismatch after roundtrip: %#v %#v", round.Result, round.Error) | |
| } | |
| if round.Result != "ok" || round.Error != "boom" { | |
| t.Fatalf("result/error mismatch after roundtrip: %#v %#v", round.Result, round.Error) | |
| } | |
| if round.StartedAt == nil || *round.StartedAt != *r.StartedAt { | |
| t.Fatalf("started_at mismatch: %v vs %v", round.StartedAt, r.StartedAt) | |
| } | |
| if round.CompletedAt == nil || *round.CompletedAt != *r.CompletedAt { | |
| t.Fatalf("completed_at mismatch: %v vs %v", round.CompletedAt, r.CompletedAt) | |
| } |
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if len(values) != 2 { |
Contributor
There was a problem hiding this comment.
P1: Custom agent: Test quality checker
Test retrieves EventResultsList values but only asserts the count (len(values) != 2), not the actual content. Per the test quality rule, tests must verify that output values are behaviorally correct — checking only the length is insufficient. Assert that values[0] equals "first" and values[1] is the expected map[string]any{"user_id": "abc"} (or vice versa depending on ordering guarantees).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bubus-go/tests/event_bus_test.go, line 50:
<comment>Test retrieves `EventResultsList` values but only asserts the count (`len(values) != 2`), not the actual content. Per the test quality rule, tests must verify that output values are behaviorally correct — checking only the length is insufficient. Assert that `values[0]` equals `"first"` and `values[1]` is the expected `map[string]any{"user_id": "abc"}` (or vice versa depending on ordering guarantees).</comment>
<file context>
@@ -7,33 +7,65 @@ import (
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(values) != 2 {
+ t.Fatalf("expected two non-nil result values, got %#v", values)
}
</file context>
Comment on lines
+143
to
+158
| func (r *EventResult) MarshalJSON() ([]byte, error) { | ||
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
| payload := eventResultJSON{ | ||
| ID: r.ID, | ||
| Status: r.Status, | ||
| EventID: r.EventID, | ||
| HandlerID: r.HandlerID, | ||
| HandlerName: r.HandlerName, | ||
| EventBusName: r.EventBusName, | ||
| EventBusID: r.EventBusID, | ||
| StartedAt: r.StartedAt, | ||
| CompletedAt: r.CompletedAt, | ||
| Result: r.Result, | ||
| Error: r.Error, | ||
| EventChildren: append([]*BaseEvent{}, r.EventChildren...), |
Contributor
There was a problem hiding this comment.
P2: Mutex held unnecessarily during json.Marshal(payload). The payload is already a fully independent copy (including the defensive append([]*BaseEvent{}, ...) for EventChildren), so the lock should be released before marshaling. Holding it during marshaling of nested []*BaseEvent (which trigger their own MarshalJSON → serialize EventResults → call EventResult.MarshalJSON) increases lock hold time and risks deadlock if the object graph ever contains cycles back to this result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bubus-go/event_result.go, line 143:
<comment>Mutex held unnecessarily during `json.Marshal(payload)`. The `payload` is already a fully independent copy (including the defensive `append([]*BaseEvent{}, ...)` for `EventChildren`), so the lock should be released before marshaling. Holding it during marshaling of nested `[]*BaseEvent` (which trigger their own `MarshalJSON` → serialize `EventResults` → call `EventResult.MarshalJSON`) increases lock hold time and risks deadlock if the object graph ever contains cycles back to this result.</comment>
<file context>
@@ -107,6 +122,44 @@ func (r *EventResult) Wait(ctx context.Context) error {
+ return append([]*BaseEvent{}, r.EventChildren...)
+}
+
+func (r *EventResult) MarshalJSON() ([]byte, error) {
+ r.mu.Lock()
+ defer r.mu.Unlock()
</file context>
Suggested change
| func (r *EventResult) MarshalJSON() ([]byte, error) { | |
| r.mu.Lock() | |
| defer r.mu.Unlock() | |
| payload := eventResultJSON{ | |
| ID: r.ID, | |
| Status: r.Status, | |
| EventID: r.EventID, | |
| HandlerID: r.HandlerID, | |
| HandlerName: r.HandlerName, | |
| EventBusName: r.EventBusName, | |
| EventBusID: r.EventBusID, | |
| StartedAt: r.StartedAt, | |
| CompletedAt: r.CompletedAt, | |
| Result: r.Result, | |
| Error: r.Error, | |
| EventChildren: append([]*BaseEvent{}, r.EventChildren...), | |
| func (r *EventResult) MarshalJSON() ([]byte, error) { | |
| r.mu.Lock() | |
| payload := eventResultJSON{ | |
| ID: r.ID, | |
| Status: r.Status, | |
| EventID: r.EventID, | |
| HandlerID: r.HandlerID, | |
| HandlerName: r.HandlerName, | |
| EventBusName: r.EventBusName, | |
| EventBusID: r.EventBusID, | |
| StartedAt: r.StartedAt, | |
| CompletedAt: r.CompletedAt, | |
| Result: r.Result, | |
| Error: r.Error, | |
| EventChildren: append([]*BaseEvent{}, r.EventChildren...), | |
| } | |
| r.mu.Unlock() | |
| return json.Marshal(payload) | |
| } |
|
|
||
| var monotonic_mu sync.Mutex | ||
| var monotonic_last = time.Now().UTC() | ||
| var fixedRFC3339Nano = "2006-01-02T15:04:05.000000000Z07:00" |
Contributor
There was a problem hiding this comment.
P2: fixedRFC3339Nano should be declared as const instead of var. It's an immutable format string, and var allows accidental mutation from anywhere within the package.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bubus-go/helpers.go, line 11:
<comment>`fixedRFC3339Nano` should be declared as `const` instead of `var`. It's an immutable format string, and `var` allows accidental mutation from anywhere within the package.</comment>
<file context>
@@ -8,14 +8,15 @@ import (
var monotonic_mu sync.Mutex
var monotonic_last = time.Now().UTC()
+var fixedRFC3339Nano = "2006-01-02T15:04:05.000000000Z07:00"
func monotonicDatetime(isoString ...string) string {
</file context>
Suggested change
| var fixedRFC3339Nano = "2006-01-02T15:04:05.000000000Z07:00" | |
| const fixedRFC3339Nano = "2006-01-02T15:04:05.000000000Z07:00" |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a213a84f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+336
to
+340
| event.EventPendingBusCount-- | ||
| if event.EventPendingBusCount < 0 { | ||
| event.EventPendingBusCount = 0 | ||
| } | ||
| event.markCompleted() |
There was a problem hiding this comment.
This marks the event completed unconditionally after decrementing EventPendingBusCount, even when the count is still greater than zero. In a forwarding flow (busB.Emit(theSameEvent)), the first bus can close done_ch and let Done/EventResult return while another bus is still processing handlers, which exposes incomplete state and breaks cross-bus completion semantics.
Useful? React with 👍 / 👎.
Comment on lines
+132
to
+134
| for _, event := range h.Values() { | ||
| if matches(event) { | ||
| return event |
There was a problem hiding this comment.
History lookup iterates in insertion order and returns the first match, which means callers get the oldest matching event rather than the latest one. For repeated event types (for example debounce/recent-response lookups), this can return stale data even though a newer matching event exists in history.
Useful? React with 👍 / 👎.
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
1 participant
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.
Summary by cubic
Add bubus-go, a full Go implementation of the event bus core with concurrency controls, timeouts, queue-jump processing, and JSON serialization. Includes comprehensive tests; bridges and middlewares are intentionally left out.
New Features
Dependencies
Written for commit 0a213a8. Summary will update on new commits.