feat(observability): implement Event System Domain Event Taxonomy (full EventType enum + DomainEvent model) #712
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Blocks
#587 feat(observability): implement Event System Domain Event Taxonomy (full EventType enum + DomainEvent model)
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!712
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m6plus-event-system-domain-taxonomy"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Verifies and completes the Event System Domain Event Taxonomy implementation. The
EventTypeenum (36 types across 11 categories),DomainEventPydantic model,EventBusprotocol,ReactiveEventBus(RxPY-based), andLoggingEventBusalready existed ininfrastructure/events/. This PR adds the persistentaudit_logproperty toReactiveEventBusand comprehensive test coverage.EventTypeenum completeness: 36 event types across 11 categories (plan, decision, invariant, actor, tool, resource, sandbox, context, validation, session, cost)DomainEventmodel with all correlation fields (plan_id,root_plan_id,session_id,actor_name,project_name,details)audit_logproperty toReactiveEventBusfor persistent event loggingReactiveEventBusemit/subscribe/stream behavior with RxPY SubjectFiles Changed
src/cleveragents/infrastructure/events/reactive.pyaudit_logpropertyfeatures/observability/event_system_taxonomy.featurefeatures/steps/event_system_taxonomy_steps.pyrobot/event_system_taxonomy_integration.robotrobot/helper_event_system_taxonomy.pybenchmarks/bench_event_bus.pyQuality Gates
linttypecheckunit_testsintegration_testscoverage_reportbenchmarkCloses #587
ISSUES CLOSED: #587
PM Review — Day 32
Status: COMMENT — Needs peer code review before approval
Summary
Event System Domain Event Taxonomy PR by @CoreRasurae (authored) / @freemo (assigned). Completes and tests the EventType enum (36 types, 11 categories) and adds
audit_logproperty toReactiveEventBus. Mergeable, no conflicts.Process Compliance
Closes #587— correct. Also hasISSUES CLOSED: #587(redundant).Technical Assessment (from PR body)
audit_logproperty toReactiveEventBus— the only production code change.Action Required
ISSUES CLOSED:line afterClosesis non-standard — consider removing.Labels Added
PM Review — Day 34
Status: Mergeable, 0 reviews, M7 (v3.6.0), Priority/Medium, Points/8
Author: @CoreRasurae
Event System Domain Event Taxonomy for observability. M7 due March 28 — medium urgency.
Action Items
PM Status — Day 36 (2026-03-16)
Day 34 review assignment check. 0 reviewer activity after 2 days.
This is M7 (v3.6.0) due March 28 — 12 days out. Medium urgency.
Assigned reviewer: Please acknowledge and provide an ETA for review.
PM Day 36: Event System Domain Event Taxonomy. Closes #587. M7 scope. Reviewer: @brent.edwards. @CoreRasurae author.
@CoreRasurae Not sure why it got assigned to me, reassigned back to you.
Code Review Report — PR #712 (Event System Domain Event Taxonomy)
Branch:
feature/m6plus-event-system-domain-taxonomyCommit:
2dbde33by Luis MendesIssue: #587
Reviewer: Automated multi-cycle review (4 global passes across all categories)
Summary
This PR adds the
audit_logproperty toReactiveEventBus(in-memory persistent event trail), plus comprehensive BDD (Behave), Robot Framework integration tests, and ASV benchmarks for the Event System Domain Event Taxonomy. The EventType enum (38 members across 12 domains), DomainEvent model, EventBus Protocol, ReactiveEventBus, and LoggingEventBus were pre-existing and verified as complete.Overall the implementation is solid — the production code change is small and well-scoped, the test coverage is thorough (36 BDD scenarios, 13 Robot integration cases, 5 benchmark suites), and the code quality is high. The findings below are improvement opportunities ranked by severity.
Findings by Severity
MEDIUM Severity
M1 — Bug / Memory: Unbounded
audit_loggrowthFile:
src/cleveragents/infrastructure/events/reactive.py:47,82The
_audit_log: list[DomainEvent]list grows without bound. In a long-running process (the platform's primary use case — "long-running, complex, large-scale tasks"), this will consume unlimited memory. There is no size cap, eviction policy, or rotation.Recommendation: Add a configurable
max_audit_log_sizewith an eviction strategy (e.g., FIFO ring buffer orcollections.deque(maxlen=N)). Alternatively, document this as intentionally unbounded for the current milestone and file a follow-up issue.M2 — Spec Deviation: In-memory list vs. spec's
_persist_audit()SQLite patternFile:
src/cleveragents/infrastructure/events/reactive.py:82The specification (§Event System, line ~45744) shows
self._persist_audit(event)as a method call inemit(), implying persistence to theaudit_logSQLite table (defined in the spec at line ~45502). The implementation uses an in-memory list. While issue #587 marks "Persistent event log: all events written to audit_log on emit" as complete, the in-memory list is volatile — data is lost on process restart. The commit message calls this "Persistent audit_log" which is semantically misleading.Recommendation: Either (a) rename to
event_logor add a docstring clarifying this is an in-memory trace, not durable persistence; or (b) implement actual SQLite persistence via a_persist_audit()method as the spec suggests; or (c) file a follow-up issue for durable audit persistence, and clarify the current scope in the PR description.M3 — Performance:
audit_logproperty O(n) defensive copy on every accessFile:
src/cleveragents/infrastructure/events/reactive.py:64return list(self._audit_log)creates a full list copy on every property access. With thousands of events accumulated over a session, this becomes expensive. The benchmark suite acknowledges this cost (time_audit_log_snapshot_100_events,time_audit_log_snapshot_after_1000_events).Recommendation: Consider returning a
tuple(immutable, same copy cost but signals intent), or using acollections.abc.Sequencewrapper that lazily prevents mutation without copying. If callers need a mutable copy, they can dolist(bus.audit_log)explicitly.M4 — Benchmark Flaw:
time_audit_log_snapshot_after_1000_eventsmeasures creation + emission, not just snapshotFile:
benchmarks/bench_event_bus.py:120-124This creates a new bus and emits 1000 events inside the timed method. The benchmark claims to measure "audit_log property access" (suite docstring: "Benchmark audit_log property access"), but actually measures
ReactiveEventBus()construction + 1000emit()calls + oneaudit_logsnapshot. The 1000-event setup should be insetup().Recommendation: Move the bus creation and 1000-event emission to a separate setup fixture, or use a parameterized setup pattern like
TaxonomyFanOutSuite.M5 — Test Coverage Gap: No test for handler exception during
emit()Files:
features/observability/event_system_taxonomy.feature,features/steps/event_system_taxonomy_steps.pyNo scenario tests what happens when a subscribed handler raises an exception during
emit(). Currently, an exception in one handler:audit_log(event already appended)on_next)This creates an inconsistency:
audit_logrecords the event as emitted, but not all handlers processed it. For a production event bus, this is a real concern.Recommendation: Add a scenario like "ReactiveEventBus continues dispatching when a handler raises" (or document the current fail-fast behavior as intentional), and consider wrapping handler calls in try/except.
LOW Severity
L1 — Spec Deviation: Emit ordering differs from specification
File:
src/cleveragents/infrastructure/events/reactive.py:82-85The spec shows: (1) stream
on_next, (2)_persist_audit, (3) handler dispatch. The implementation does: (1)audit_log.append, (2)on_next, (3) handler dispatch. Persist-first is arguably better (guarantees audit trail even if stream/handlers fail), but it's a documented deviation.L2 — Architectural:
audit_lognot inEventBusProtocolFile:
src/cleveragents/infrastructure/events/protocol.pyaudit_logis only onReactiveEventBus, not on theEventBusProtocol. Consumers receiving the bus through DI via the protocol type hint cannot access audit logs.LoggingEventBushas noaudit_logeither. If audit logging is a cross-cutting concern (which the spec implies), it should arguably be in the protocol or in a separateAuditableEventBusprotocol.L3 — Test Flaw: Frozen model mutation test catches generic
ExceptionFile:
features/steps/event_system_taxonomy_steps.py:111-118Should catch
pydantic.ValidationError(the specific error Pydantic raises for frozen model mutation). Catching bareExceptioncould mask unrelated errors (e.g.,AttributeError,TypeError). Same issue inrobot/helper_event_system_taxonomy.py:142-147.L4 — Test Flaw: Duplicate step definitions for singular/plural event count
File:
features/steps/event_system_taxonomy_steps.py:221-230Two nearly identical step definitions:
the taxonomy handler should have received {n:d} event(singular)the taxonomy handler should have received {n:d} events(plural)Both do the same thing. A single step with an optional "s" regex (or Behave's
{n:d} event(s)) would reduce duplication.L5 — Test Weakness: "at least 30 members" threshold is too loose
File:
features/observability/event_system_taxonomy.feature:10The actual EventType count is 38. The threshold of 30 means a regression removing up to 8 event types (~21%) would go undetected by this scenario. Individual domain scenarios provide better coverage, but the top-level count check could be tighter.
Recommendation: Raise to
>=38or==38for exact regression detection.L6 — Benchmark Flaw:
TaxonomyLoggingEmitSuite.setupdisables all logging globally without cleanupFile:
benchmarks/bench_event_bus.py:158logging.disable(logging.CRITICAL)suppresses all logging for the entire process. If ASV runs multiple benchmark suites in the same process, this could affect other suites. Noteardown()restores logging.Recommendation: Add a
teardown()that callslogging.disable(logging.NOTSET).L7 — Benchmark Flaw:
_receivedlist accumulates across ASV timing iterationsFile:
benchmarks/bench_event_bus.py:88The
_receivedlist created insetup()is never cleared between ASV timing calls. Across many iterations, the list grows, andlist.append()amortized cost increases slightly due to periodic resizing. This introduces noise into the benchmark.Recommendation: Clear
self._receivedin each timed method, or accept the minor noise.L8 — Code Style:
importlib.reload(cleveragents)at module level in benchmarkFile:
benchmarks/bench_event_bus.py:23-24Module-level
importlib.reload()can cause subtle issues: re-execution of module init code, duplicate singleton registrations, etc. This appears to be a workaround for thesys.pathinsertion.Recommendation: Verify this pattern is consistent with other ASV benchmarks in the project. If not, consider removing the reload.
Items Verified as Correct
emitandsubscribeemit()rejects non-DomainEvent,subscribe()rejects non-EventType and non-callableaudit_logdefensive copy prevents external mutationvulture_whitelist.pyentry foraudit_logproperty2dbde33d5f05be0c611d05be0c611d398c4bf684Code Review — PR #712 (Ticket #587)
Branch:
feature/m6plus-event-system-domain-taxonomyAuthor: @CoreRasurae
Reviewer: @brent.edwards
Method: Multi-focus parallel review (7 threads: spec compliance, bug detection, test quality, performance, security, code quality, architecture) + fresh-eyes pass
Verdict: Request Changes
There are 4 P1:must-fix findings that must be resolved before merge, primarily around exception handling gaps in
emit(), missing test coverage for a critical behavioral contract, and a protocol implementation inconsistency. The production code change is well-scoped and the overall implementation quality is high — these are targeted fixes, not fundamental rework.Per the review playbook escalation rules: 2+ P1 findings in the same subsystem (infrastructure/events) → escalate to subsystem owner (@CoreRasurae / @hamza.khyari).
P1:must-fix — Must Fix Before Merge
1. Unguarded
on_next()inemit()— stream subscriber exception skips audit_log and handlersFile:
src/cleveragents/infrastructure/events/reactive.py:96self._subject.on_next(event)is called beforeself._audit_log.append(event)and handler dispatch. If any subscriber registered viabus.stream.subscribe(on_next=...)raises an exception, that exception propagates unhandled throughon_next(), and the event is never recorded inaudit_logand never dispatched to per-type handlers.This is inconsistent with how per-type handlers are treated (wrapped in try/except at lines 99–109). A single misbehaving stream subscriber silently breaks the entire event bus, including the audit trail.
While the
on_next()call is pre-existing, this PR adds_audit_log.append(event)at line 97, which increases the severity — the audit trail is now also a casualty of stream subscriber failures.Recommendation: Wrap
self._subject.on_next(event)in try/except to match the handler dispatch pattern:2. Missing test coverage for handler exception resilience
Files:
features/observability/event_system_taxonomy.feature,robot/event_system_taxonomy_integration.robotReactiveEventBus.emit()has a try/except block at lines 99–109 that catches handler exceptions, logs a warning, and continues dispatching to remaining handlers. This is the most important behavioral contract of the event bus for fault isolation. Yet there is zero test coverage for this behavior anywhere in the BDD feature file, Robot integration tests, or Robot helper.A regression removing the try/except would cause one failing handler to break all subsequent handlers and would go completely undetected by the test suite.
Recommendation: Add a BDD scenario:
Also add a corresponding Robot integration test.
3.
LoggingEventBushandler dispatch has no exception isolation (LSP violation)File:
src/cleveragents/infrastructure/events/logging_bus.py:73–74LoggingEventBus.emit()calls handlers without try/except:The class docstring (line 41) explicitly says it "can be used as a drop-in replacement for
ReactiveEventBus." Since both implementEventBusProtocol and are swappable via DI, this behavioral divergence is a Liskov Substitution Principle violation — code that works underReactiveEventBuswill crash underLoggingEventBusif any handler raises.While this file is pre-existing and not modified in this PR, the PR adds test assertions for protocol conformance (both satisfy
EventBus), making the behavioral inconsistency directly relevant.Recommendation: Add the same try/except handler-wrapping pattern from
ReactiveEventBus.emit()toLoggingEventBus.emit().4. Ticket acceptance criterion "Persistent event log" is misleadingly marked complete
File: PR description, Ticket #587
Ticket #587 acceptance criterion states: "Persistent event log: all events written to audit_log on emit" — marked as complete. The spec (§Persistence Schema) says: "Persistent Event Log: All events are written to the
audit_logtable for post-hoc analysis." The SQLite table DDL is defined in the spec.The implementation is an in-memory
list[DomainEvent], which is volatile — all data is lost on process restart. The docstring (lines 63–69) correctly acknowledges this, but the ticket checkbox and CHANGELOG ("Added persistent in-memoryaudit_log") are semantically contradictory.The pre-existing
AuditEventSubscriberbridges EventBus → SQLite, but only for 9 security-relevant event types — not all 43 as the spec requires.Recommendation: Either (a) update the ticket/PR description to clarify the in-memory
audit_logis the runtime trace and that full SQLite persistence is a separate concern, and file a follow-up ticket for expandingAuditEventSubscriberto all event types, OR (b) implement the durable persistence in this PR.P2:should-fix — Fix in Follow-Up PR (Within 3 Days)
5. Unbounded
_audit_loggrowth in Singleton-scoped busFile:
src/cleveragents/infrastructure/events/reactive.py:50, 97_audit_log: list[DomainEvent]grows without bound.ReactiveEventBusis a Singleton (per DI container), so it lives for the entire process lifetime. In a long-running session (the platform's primary use case), this list will grow monotonically with no cap, eviction, or rotation. Combined with theaudit_logproperty creating an O(n) defensive copy on every access (line 75), this is both a memory leak and a performance concern.Recommendation: Use
collections.deque(maxlen=N)with a configurable cap (e.g., 10,000), or provide aclear_audit_log()method. If unbounded growth is intentional for the current milestone, document this explicitly and file a follow-up issue.6.
_audit_logaccumulates across ASV benchmark iterationsFile:
benchmarks/bench_event_bus.py:81–100, 136–143TaxonomyReactiveEmitSuiteandTaxonomyFanOutSuitecreate a single bus insetup(), and each ASV timing iteration appends toself.bus._audit_log. The_received.clear()calls (lines 90, 94, 98) show awareness of list accumulation — but the bus's own_audit_logwas missed. Fortime_emit_100_events, after 1000 ASV iterations the audit_log holds 100,000 entries, introducing systematic upward drift in timing measurements.Recommendation: Add
self.bus._audit_log.clear()at the start of each timed method, or recreate the bus per timed call.7. Robot test event count threshold inconsistent with BDD
File:
robot/helper_event_system_taxonomy.py:35,robot/event_system_taxonomy_integration.robot:14The Robot helper checks
count < 30, while the BDD feature file checks>= 38. The actualEventTypemember count is 43. The Robot test would miss removal of up to 13 event types (~30%). The BDD threshold is also loose — it would miss 5 removed types.Recommendation: Update both tests to check the exact count:
>= 43(BDD) andcount < 43(Robot helper). Update the Robot test case title accordingly.8. Four event type domains have no explicit named-value test coverage
File:
features/observability/event_system_taxonomy.featureThe BDD feature has explicit presence checks for 12 of 16 domains. Four domains with 5 event types are never verified by name:
correction.applied,config.changed,entity.deleted,auth.success,auth.failureThese are only covered indirectly by the count threshold. They could be replaced with different values without any named-value test catching it.
Recommendation: Add BDD scenarios for the correction, config, entity, and auth domains.
9.
LoggingEventBusonly has structural test coverage, not functionalFile:
features/observability/event_system_taxonomy.featureLoggingEventBusis only tested viaisinstance(bus, EventBus). There are zero functional tests for emit dispatching to handlers, subscribe registering correctly, or type validation. Combined with Finding #3 (no exception isolation), this means a significant portion of theLoggingEventBusbehavioral contract is unverified.Recommendation: Add at minimum a BDD scenario verifying
LoggingEventBusdispatches events to subscribed handlers.10. Handler exception log discards error message
File:
src/cleveragents/infrastructure/events/reactive.py:101–108The
except Exception as excblock logserror_type=type(exc).__name__but not the actual error message (str(exc)). In production, knowing the exception type without the message makes debugging handler failures very difficult.Additionally, catching bare
Exceptionsilently swallowsMemoryErrorand similar serious errors. Per CONTRIBUTING.md: "Do not use bare catch-all exception handlers without re-raising unless you have specific recovery logic."Recommendation: Include
error=str(exc)in the structured log, and considerexc_info=Truefor debug-level traceback.11.
streamproperty leaks mutableSubjectasObservableFile:
src/cleveragents/infrastructure/events/reactive.py:137–145The
streamproperty returnsself._subjectdirectly. While typed asObservable, the runtime object is aSubjectwhich also extendsObserver— exposing.on_next(),.on_error(),.on_completed(). Any consumer could callbus.stream.on_next(fake_event)to bypassemit(), skipping audit_log, type validation, and handler dispatch.Recommendation: Return a read-only observable wrapper:
return self._subject.pipe()(RxPY'spipe()with no operators returns a new Observable that doesn't expose Subject methods).12.
detailsdict is mutable in-place despitefrozen=TruemodelFile:
src/cleveragents/infrastructure/events/models.py:40, 65Pydantic's
frozen=Trueprevents field reassignment but does not make nested mutable objects immutable.event.details["injected"] = "surprise"succeeds silently. Sinceaudit_logstores the same object reference (not a deep copy), any code that mutatesevent.detailscorrupts the audit trail for all consumers.Recommendation: Either deep-copy
event.detailswhen appending to_audit_log, or convertdetailstotypes.MappingProxyTypevia a Pydantic validator.13. CHANGELOG "persistent in-memory" is self-contradictory
File:
CHANGELOG.md:3The entry says "Added persistent in-memory
audit_log" — "persistent" and "in-memory" are contradictory. The docstring correctly says "volatile, in-memory."Recommendation: Change to "Added volatile in-memory
audit_log" or "Added in-memory event trail."P3:nit — Author Discretion
14. PR description states wrong event count
PR body says "36 event types across 11 categories." Actual count is 43 types across 15 categories (11 spec + 4 extensions). Update for accuracy.
15. Unnecessary
hasattrguard onEventType.valuereactive.py:104–106:hasattr(event.event_type, "value")is unreachable —event_typeis always anEventType(StrEnum) which always has.value. Simplify toevent_type=event.event_type.value.16. PR label
State/In Progressshould beState/In ReviewTicket #587 correctly has
State/In Review, but the PR itself carriesState/In Progress. Update for consistency.17. Benchmark suite duplication with
event_bus_bench.pybench_event_bus.pyduplicates several benchmarks from the pre-existingevent_bus_bench.py(DomainEvent construction, reactive emit, fan-out, logging emit). Consider extending the existing file with the unique new benchmarks (audit_log snapshot, JSON roundtrip) instead.18.
audit_lognaming collision with spec's SQLite tableThe in-memory property and the spec's durable SQLite table both use the name
audit_log. Considerevent_trailorevent_logfor the in-memory version to avoid maintainer confusion.19.
step_protocol_has_methoduses weakhasattrcheckfeatures/steps/event_system_taxonomy_steps.py:184–185: Useshasattr(EventBus, method)which doesn't verify the attribute is callable. Strengthen toassert callable(getattr(EventBus, method, None)).Items Verified as Correct ✅
emit()+subscribe()with@runtime_checkableemit()andsubscribe()reject invalid args withTypeErrorlist(self._audit_log)prevents external mutationCloses #587taxonomy-namespace prefixaudit_logcorrectly addedSummary
This is a well-crafted PR with a small, focused production change and comprehensive test coverage. The EventType enum, DomainEvent model, and EventBus Protocol are all correctly implemented per spec. Multiple issues from the earlier self-review have been addressed. The code quality is high — commit standards, typing, and documentation are exemplary.
The 4 P1 findings center on exception handling completeness (the stream subscriber gap and the LoggingEventBus inconsistency) and test coverage for the fault isolation contract. These are targeted, well-scoped fixes that should not require significant rework. The P2 findings are improvements that can be addressed in a follow-up PR within 3 days.
Supplementary Code Review — PR #712 (Second Pass)
Reviewer: @brent.edwards
Method: Deep-dive analysis (3 focused threads:
emit()interaction analysis, test correctness verification, CONTRIBUTING.md compliance audit)This is a follow-up to my initial review. The second pass focused on subtle interaction bugs, whether tests actually verify what they claim, and CONTRIBUTING.md rule compliance. All findings below are new — not duplicates of the initial review.
Verdict: Still Request Changes
The original 4 P1 findings remain. This second pass adds 2 new P2:should-fix correctness issues and 4 new P2:should-fix test quality issues, plus minor items. The P1 findings from the first review are sufficient for "Request Changes"; this supplement strengthens the case.
New P2:should-fix Findings
20. Re-entrant
emit()has no recursion guardFile:
src/cleveragents/infrastructure/events/reactive.py:96–109emit()has no re-entrancy protection. If a handler registered forEventType.Xcallsbus.emit(DomainEvent(event_type=EventType.X)), unbounded recursion occurs →RecursionError. No current handler does this, but the architecture encourages it —CostBudgetService._emit_warning()and_emit_exceeded()callbus.emit()from within service methods. If any future handler forBUDGET_WARNINGtriggered another budget check that re-emitted, the infinite loop would fire. There's no_emittingdepth guard, no documentation warning against re-entrant emit, and no failsafe.Recommendation: Add a depth counter:
21.
subscribe()during handler dispatch appends to live iteration listFile:
src/cleveragents/infrastructure/events/reactive.py:98, 135At line 98,
emit()iteratesself._subscriptions.get(event.event_type, []), which returns a direct reference to the list stored in the dict. At line 135,subscribe()does.append(handler)on the same list object. If a handler callsbus.subscribe(same_event_type, new_handler)duringemit(), the new handler is appended to the list currently being iterated. Python'slist.__iter__will see the new element, causing the new handler to fire immediately in the sameemit()call — unexpected and undocumented.Recommendation: Iterate over a snapshot:
22. "DomainEvent details dict is typed per event" scenario is tautological
File:
features/observability/event_system_taxonomy.feature:116–118Step:
features/steps/event_system_taxonomy_steps.py:163–175The scenario title claims "typed per event," implying per-event-type schema validation. The actual test creates a DomainEvent with
details={"old_phase": ..., "new_phase": ...}and asserts those keys exist — testing only that Python dicts preserve their keys, which is tautologically true. No per-event-type schema enforcement, no invalid-key rejection, no value-type constraint is tested.Recommendation: Either rename to "DomainEvent details dict preserves custom keys" (accurate) or implement actual per-event-type validation testing.
23. audit_log tests use count-only assertions — don't verify event content
Files:
features/observability/event_system_taxonomy.feature:181–185— "captures all emitted events" checkslen(log) == 2onlyrobot/helper_event_system_taxonomy.py:133–146— "reactive_emit_subscribe_audit" checkslen(received) != 1andlen(bus.audit_log) != 1onlyNeither test verifies that the recorded events are the correct events. An implementation that appended placeholder events would pass. The audit_log order test (lines 187–192) partially compensates, but these scenarios are independently insufficient for their claims.
Recommendation: Add event type assertions:
assert log[0].event_type == EventType.PLAN_CREATED.24. Missing test for default correlation_id uniqueness
File:
features/observability/event_system_taxonomy.featureThe correlation test (lines 203–206) only verifies the positive case — two events CAN share a correlation_id. There is no test that independently created events get DIFFERENT auto-generated correlation_ids. If
default_factory=lambda: str(ULID())were accidentally changed to a constant, all events would share the same ID and correlation tracing would be completely broken — but the test suite would not catch this.Recommendation: Add a scenario:
25. Imports inside functions violate CONTRIBUTING.md import guidelines
Rule: CONTRIBUTING.md lines 1378–1383 — "Ensure all imports are at the top of the Python file. Do not scatter imports throughout the file or bury them inside functions or methods."
Files (8 instances):
features/steps/event_system_taxonomy_steps.py:106,248robot/helper_event_system_taxonomy.py:100,121,151,194benchmarks/bench_event_bus.py:157,164Imports of
ValidationError,Observable,ULID, andloggingare inside functions rather than at module level. While this is a pervasive project-wide pattern (not unique to this PR), these are straightforward to fix — move them to the top of each file.Recommendation: Move all imports to file-level. For step files,
from pydantic import ValidationErrorandfrom rx.core.observable.observable import Observablecan be top-level unconditionally.New P3:nit Findings
26. Correlation field assertions only check
is not None, not actual valuesfeatures/steps/event_system_taxonomy_steps.py:148–159— The "correlation fields accept ULID strings" steps assertplan_id is not None,root_plan_id is not None, etc. — but don't save the generated ULIDs to context for comparison. A bug that silently replaced values with different ULIDs would pass.27. audit_log order step's string splitting is fragile
features/steps/event_system_taxonomy_steps.py:313—expected = order.split(",")splits without stripping whitespace. If anyone adds a space after commas in the feature file, the comparison fails with confusing error messages.Recommendation:
expected = [v.strip() for v in order.split(",")]28.
str(e.event_type)pattern is fragile across Python versionsfeatures/steps/event_system_taxonomy_steps.py:315androbot/helper_event_system_taxonomy.py:172— Both usestr(e.event_type)instead ofe.event_type.value. Python's__str__behavior on StrEnum changed between 3.10 and 3.11. Using.valueexplicitly is more robust.Additional Positive Observations
The deep-dive confirmed several architectural strengths:
AuditEventSubscriber._handle_eventcorrectly contains its own exceptions — no double-catch withemit()'s try/except. The redaction pipeline (redact_dict()) creates a new dict and doesn't mutate the original event'sdetails.dict(event.details)in AuditEventSubscriber line 100 creates a copy before redaction, so the original event is never modified by the audit pipeline.Given a fresh ReactiveEventBuscreates a new bus per scenario. No test pollution viactxattributes between scenarios.sys.exit(1)on failure. No code paths can succeed without the sentinel.Consolidated Finding Count (Both Reviews)
The 4 original P1 findings remain the merge-blockers. The new P2 findings strengthen the recommendation to improve test quality (several scenarios don't actually test what they claim) and add re-entrancy/iteration safety to
emit().398c4bf684ab81cf44e0Code Review — PR #712 (Ticket #587) — Third Peer Review at Commit
ab81cf4Branch:
feature/m6plus-event-system-domain-taxonomyAuthor: @CoreRasurae
Reviewer: @brent.edwards
Previous reviews: 2 rounds by @brent.edwards (REQUEST_CHANGES + supplementary, 28 total findings), 1 self-review by @CoreRasurae (5M + 8L), PM status by @freemo
Method: 4 parallel agents (P1 verification, new bug hunt, review extraction, code trace) + manual fresh-eyes verification
Verdict: Approve
All 4 P1 items from my initial review are fixed. No new P0 or P1 bugs were found. The production code quality is now excellent — exception isolation, defensive copying, and spec-aligned ordering are all properly implemented.
Previous P1 Items — All Fixed ✅
on_next()inemit()reactive.py:97–105:on_next()wrapped in try/except, logs warning with error type + messageLoggingEventBusandReactiveEventBus— "continues dispatching when a handler raises"LoggingEventBusno exception isolationlogging_bus.py:73–83: handler calls wrapped in try/except matchingReactiveEventBuspatternaudit_log" — no longer contradictoryAdditional Improvements Noted ✅
Beyond the P1 fixes, I noticed several P2 items from my initial review were also addressed:
audit_log.appendnow usesmodel_copy(deep=True)(reactive.py:107) — this addresses the P2-12 finding about mutabledetailsdict corruption. Event details are now deep-copied before audit storage.list(...)snapshot (reactive.py:109) — this addresses the P2 supplementary finding about subscribe-during-iteration mutation.error=str(exc)(reactive.py:104, 117) — this addresses P2-10 about discarded error messages.New P0/P1 Bugs: None Found ✅
The production code (
reactive.py,logging_bus.py,models.py,types.py) was reviewed line-by-line. The emit flow is now properly guarded: on_next → audit_log → handlers, with each step isolated by try/except.CoreRasurae Self-Review Findings
The 5 MEDIUM items from @CoreRasurae's self-review (M1-unbounded growth, M2-spec deviation, M3-O(n) copy, M4-benchmark flaw, M5-handler exception test gap) — M5 is fully addressed (test added). M1 (unbounded growth) and M3 (O(n) copy) remain as P2-level items for follow-up. M2 (spec deviation) is addressed by the CHANGELOG/docstring clarification. M4 (benchmark flaw) is addressed by moving setup code.
P2:should-fix — For Follow-Up
1. Unbounded
_audit_loggrowth (unchanged from initial review)The in-memory list still grows without bound.
deque(maxlen=N)or aclear_audit_log()method would address this. Low urgency since sessions are typically finite.2.
streamproperty still exposesSubjectasObservablereactive.py:137–145: Consumers can callbus.stream.on_next(fake_event)to bypassemit(). Returnself._subject.pipe()for a read-only wrapper.Prerequisite for Merge
The branch is 24 commits behind master and Forgejo reports
mergeable: false. A rebase ontoorigin/masteris required before merge.ab81cf44e0c579fe1ee2New commits pushed, approval review dismissed automatically according to repository settings
c579fe1ee200881a3e5f