test(macos): Filters subscription events by entity

[NickSeagull]

Addresses test interference in macOS by filtering subscription events based on the entity name. This ensures that tests only process events relevant to their specific context, preventing cross-test contamination.

Summary by CodeRabbit

• Bug Fixes

  • Improved test isolation by adding per-entity filtering to event subscribers so tests only record and fail on relevant events, reducing cross-test interference and preserving existing test behaviors (collection, ordering, unsubscription).

• Chores

  • Reformatted CI workflow health-check options for the database service (no functional change).

[coderabbitai]

Caution

Review failed

The pull request is closed.

Walkthrough

I command: subscriber handlers now filter incoming events by entityName, invoking handlers only for matching entities to avoid cross-entity noise in tests.

Changes

Cohort / File(s)	Summary
Event subscriber tests core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs	Introduces entityFilteredSubscriber wrapper; replaces direct subscriber implementations with filtered variants (collectEvent, failingHandler, etc.); applies per-event entityName checks and adjusts failing-subscriber/error paths and concurrent subscriber setups.
CI workflow formatting .github/workflows/test.yml	Reformats PostgreSQL service health-check options onto fewer lines; no behavioral changes intended.

Sequence Diagram(s)

sequenceDiagram
    participant Test as Test Context
    participant Sub as Filtered Subscriber
    participant Store as Event Store

    Test->>Sub: Subscribe(with entityName)
    Store->>Sub: Emit event(event.entityName)

    rect `#f0f8ff`
      Note over Sub: entityFilteredSubscriber checks entityName equality
    end

    alt entity matches
        Sub->>Sub: Invoke handler (record / fail if intended)
        Note over Sub: ✓ handler runs
    else entity differs
        Sub->>Sub: Yield / ignore event
        Note over Sub: ✗ ignored
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Inspect core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs for correct entityName comparison, correct wrapping of existing handlers, and that failing paths only trigger for matching entities.
• Verify concurrent subscriber changes preserve ordering/collection semantics and unsubscription behavior.
• Confirm .github/workflows/test.yml health-check formatting did not alter semantics.

Poem

I, sovereign watcher, sift each tale,
Only kin of name shall pass my veil,
Unwanted echoes fall away,
Tests align and keep their sway,
Order kept beneath my pale. 🪄

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the primary change: adding per-event filtering in subscription implementations to match entity names, which directly addresses the macOS test interference issue described in the PR objectives.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 80598fd and 487cd20.

📒 Files selected for processing (2)

• .github/workflows/test.yml (1 hunks)
• core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs (6 hunks)

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude finished @NickSeagull's task —— View job

---

PR Review: Filtering Subscription Events by Entity

Review Tasks:

• Read and analyze the changed file
• Examine the specific changes made
• Review for code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage impact
• Provide constructive feedback

---

✅ Overall Assessment: Good Fix with Room for Consistency

This PR successfully addresses the macOS test interference issue with a targeted, minimal change that's well-documented. The fix is sound but could be applied more broadly for consistency.

---

📝 Code Quality & Best Practices

✅ Positive:

• Clear documentation: The comment explaining the purpose is excellent
• Minimal change: Only affects the problematic test case
• Follows NeoHaskell style: Uses explicit if-then-else, pipe operator, proper imports
• Proper error handling: Maintains existing Task.yield unit pattern

⚠️ Improvement Suggestions:

1. Inconsistent filtering across test cases: The filtering is only applied to the first test (core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs:32-37) but not other tests that also subscribe to "all events". Tests at lines 248-311, 313-361, 362-411, and others could face the same cross-contamination issue.

2. Consider extraction: The filtering logic could be extracted to a helper function:

   entityFilteredSubscriber :: Event.EntityName -> (Event MyEvent -> Task Text Unit) -> Event MyEvent -> Task Text Unit
   entityFilteredSubscriber targetEntity handler event = do
     if event.entityName == targetEntity
       then handler event
       else Task.yield unit

---

🐛 Potential Issues

Minor Concerns:

• Race conditions: While the filtering helps, there might still be timing issues in concurrent test execution. The AsyncTask.sleep 100 suggests potential timing sensitivity.
• Test isolation: Consider if individual tests should use unique entity names per test run to ensure complete isolation.

No Critical Issues Found: The change doesn't introduce any bugs or breaking changes.

---

⚡ Performance Considerations

✅ Performance Impact: Minimal

• The entityName comparison is O(1) string equality
• Early return via Task.yield unit is efficient
• Filtering happens in-memory before expensive operations

Potential optimization: Since all tests in this suite seem to use context.entityName, this filtering pattern could be centralized.

---

🔒 Security Concerns

✅ No Security Issues: The change involves test-only code with no security implications. Entity name filtering is a safe operation.

---

🧪 Test Coverage Impact

✅ Positive Impact:

• Reduces flaky tests: Eliminates cross-test interference on macOS
• Maintains test intent: The first test still validates "subscribing to all events" functionality
• Preserves assertions: All existing test assertions remain unchanged

Suggestion: Consider adding a dedicated test that verifies entity filtering works correctly across multiple entities.

---

🎯 Recommendations

1. Apply consistently: Consider applying the same filtering pattern to other tests that use subscribeToAllEvents (lines 248, 313, 362, etc.)

2. Add helper function: Extract the filtering logic to improve maintainability

3. Document pattern: Add a comment in the Context module about entity isolation best practices

4. Consider test naming: The entity names are UUIDs - consider prefixing with test names for better debugging

---

🏆 Summary

This is a well-executed fix that directly addresses the macOS test interference problem. The implementation is clean, documented, and follows project conventions. While there's room for applying this pattern more broadly across the test suite, this targeted fix effectively resolves the immediate issue without over-engineering.

Recommendation: ✅ Approve - The change is safe, effective, and improves test reliability.

--- | Branch: fix/macos-tests

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs (3)

248-263: The divine protection remains incomplete! Additional tests remain vulnerable to cross-test interference.

The filtering applied to the first test is absent here. This test subscribes to all events without filtering by context.entityName, rendering it susceptible to the same cross-test interference this PR seeks to vanquish.

Apply entity filtering to achieve protection:

         -- Define different subscriber functions
         let subscriber1 event = do
+              if event.entityName == context.entityName
+                then subscriber1Events |> ConcurrentVar.modify (Array.push event)
+                else Task.yield unit
-              subscriber1Events |> ConcurrentVar.modify (Array.push event)
               Task.yield unit :: Task Text Unit
         let subscriber2 event = do
+              if event.entityName == context.entityName
+                then subscriber2Events |> ConcurrentVar.modify (Array.push event)
+                else Task.yield unit
-              subscriber2Events |> ConcurrentVar.modify (Array.push event)
               Task.yield unit :: Task Text Unit
         let subscriber3 event = do
+              if event.entityName == context.entityName
+                then subscriber3Events |> ConcurrentVar.modify (Array.push event)
+                else Task.yield unit
-              subscriber3Events |> ConcurrentVar.modify (Array.push event)
               Task.yield unit :: Task Text Unit

---

313-320: Another unshielded test discovered! Cross-test contamination may corrupt thy assertions.

This test also subscribes to all events without entity filtering, leaving it vulnerable to events from concurrent test executions.

Apply the sacred filter:

         -- Define subscriber function
         let subscriber event = do
+              if event.entityName == context.entityName
+                then receivedEvents |> ConcurrentVar.modify (Array.push event)
+                else Task.yield unit
-              receivedEvents |> ConcurrentVar.modify (Array.push event)
               Task.yield unit :: Task Text Unit

---

362-369: The high-frequency test stands exposed! Grant it the same protection.

This test processes rapid event insertion without entity filtering, making it vulnerable to interference from events generated by parallel test executions.

Bestow the filtering logic upon this subscriber as well:

         -- Define subscriber function that tracks events
         let subscriber event = do
+              if event.entityName == context.entityName
+                then receivedEvents |> ConcurrentVar.modify (Array.push event)
+                else Task.yield unit
-              receivedEvents |> ConcurrentVar.modify (Array.push event)
               Task.yield unit :: Task Text Unit

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between b718f63 and 942e851.

📒 Files selected for processing (1)

• core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: tests-macos
• GitHub Check: tests-linux

[claude]

Claude finished @NickSeagull's task —— View job

---

PR Review: Filtering Subscription Events by Entity

Review Tasks:

• Read and analyze the changed files
• Examine the specific changes made
• Review for code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage impact
• Provide constructive feedback

---

✅ Overall Assessment: Good Fix with Room for Consistency

This PR successfully addresses the macOS test interference issue with a targeted, minimal change that's well-documented. The fix is sound but could be applied more broadly for consistency.

---

📝 Code Quality & Best Practices

✅ Positive:

• Clear documentation: The comment explaining the purpose is excellent
• Minimal change: Only affects the problematic test case
• Follows NeoHaskell style: Uses explicit if-then-else, pipe operator, proper imports
• Proper error handling: Maintains existing Task.yield unit pattern

⚠️ Improvement Suggestions:

1. Inconsistent filtering across test cases: The filtering is only applied to the first test (core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs:32-37) but not other tests that also subscribe to "all events". Tests at lines 248-311, 313-361, 362-411, and others could face the same cross-contamination issue.

2. Consider extraction: The filtering logic could be extracted to a helper function:

   entityFilteredSubscriber :: Event.EntityName -> (Event MyEvent -> Task Text Unit) -> Event MyEvent -> Task Text Unit
   entityFilteredSubscriber targetEntity handler event = do
     if event.entityName == targetEntity
       then handler event
       else Task.yield unit

---

🐛 Potential Issues

Minor Concerns:

• Race conditions: While the filtering helps, there might still be timing issues in concurrent test execution. The AsyncTask.sleep 100 suggests potential timing sensitivity.
• Test isolation: Consider if individual tests should use unique entity names per test run to ensure complete isolation.

No Critical Issues Found: The change doesn't introduce any bugs or breaking changes.

---

⚡ Performance Considerations

✅ Performance Impact: Minimal

• The entityName comparison is O(1) string equality
• Early return via Task.yield unit is efficient
• Filtering happens in-memory before expensive operations

Potential optimization: Since all tests in this suite seem to use context.entityName, this filtering pattern could be centralized.

---

🔒 Security Concerns

✅ No Security Issues: The change involves test-only code with no security implications. Entity name filtering is a safe operation.

---

🧪 Test Coverage Impact

✅ Positive Impact:

• Reduces flaky tests: Eliminates cross-test interference on macOS
• Maintains test intent: The first test still validates "subscribing to all events" functionality
• Preserves assertions: All existing test assertions remain unchanged

Suggestion: Consider adding a dedicated test that verifies entity filtering works correctly across multiple entities.

---

🎯 Recommendations

1. Apply consistently: Consider applying the same filtering pattern to other tests that use subscribeToAllEvents (lines 248, 313, 362, etc.)

2. Add helper function: Extract the filtering logic to improve maintainability

3. Document pattern: Add a comment in the Context module about entity isolation best practices

4. Consider test naming: The entity names are UUIDs - consider prefixing with test names for better debugging

---

🏆 Summary

This is a well-executed fix that directly addresses the macOS test interference problem. The implementation is clean, documented, and follows project conventions. While there's room for applying this pattern more broadly across the test suite, this targeted fix effectively resolves the immediate issue without over-engineering.

Recommendation: ✅ Approve - The change is safe, effective, and improves test reliability.

--- | Branch: fix/macos-tests

[claude]

Claude finished @NickSeagull's task —— View job

---

PR Review: Filtering Subscription Events by Entity

Review Tasks:

• Read and analyze the changed files
• Examine the specific changes made
• Review for code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage impact
• Provide constructive feedback

---

✅ Overall Assessment: Excellent Fix with Strong Implementation

This PR successfully addresses the macOS test interference issue with a well-architected solution. The introduction of the entityFilteredSubscriber helper function demonstrates good software engineering practices and significantly improves test reliability.

---

📝 Code Quality & Best Practices

✅ Positive:

• Excellent helper function design: The entityFilteredSubscriber function (core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs:25-34) is well-typed, reusable, and follows functional programming principles
• Clear documentation: Function has descriptive name and purpose is obvious from context
• Follows NeoHaskell style: Uses explicit if-then-else, proper imports, and correct pipe operator usage
• Comprehensive application: Applied consistently across ALL affected test cases (lines 47, 218, 269-271, 327, 375)
• Proper error handling: Maintains existing Task.yield unit pattern for clean task completion

✅ Code Style Compliance:

• Follows CLAUDE.md conventions: qualified imports, explicit function application, proper do block usage
• Type signatures are descriptive with Event.EntityName and proper Task Text Unit return type
• No forbidden patterns (no point-free style, no ecosystem imports)

---

🐛 Potential Issues

✅ No Critical Issues Found:

The implementation is robust and well-thought-out. The helper function approach eliminates the previous inconsistency identified by CodeRabbit and provides a clean, maintainable solution.

Minor Observations:

• Race condition mitigation: The AsyncTask.sleep calls suggest some timing sensitivity, but the filtering should eliminate cross-test contamination regardless of timing
• Entity isolation: The UUID-based entity names provide good isolation between test runs

---

⚡ Performance Considerations

✅ Performance Impact: Optimal

• The entityName equality check is O(1) for string comparison
• Early return via Task.yield unit when entities don't match is efficient
• The helper function adds no measurable overhead
• Filtering happens before expensive event processing operations

Optimization achieved: The refactoring from inline filtering to a helper function actually improves code organization without performance cost.

---

🔒 Security Concerns

✅ No Security Issues:

• Test-only code with no production security implications
• Entity name filtering is a safe operation
• UUID generation provides good randomness for test isolation

---

🧪 Test Coverage Impact

✅ Significant Positive Impact:

• Eliminates flaky tests: Comprehensive filtering prevents cross-test interference across all subscription types
• Maintains test intent: All tests still validate their intended functionality while being properly isolated
• Improved reliability: The helper function ensures consistent filtering behavior across the test suite
• Better maintainability: Future tests can easily use the same isolation pattern

Test Coverage Analysis:

• Applied to all relevant test cases: basic subscription (line 47), error handling (line 218), concurrent subscribers (lines 269-271), unsubscription (line 327), and high-frequency events (line 375)
• Preserves all existing assertions and test logic
• Adds proper isolation without changing test semantics

---

🎯 Recommendations

✅ Implementation Completed Excellently

The PR addresses all the concerns from the previous CodeRabbit review and implements a superior solution:

1. ✅ Consistent application: All affected tests now use the helper function
2. ✅ Helper function extracted: Clean, reusable entityFilteredSubscriber function
3. ✅ Documentation clarity: Function name and usage make the purpose clear
4. ✅ Maintainability: Future tests can easily adopt this pattern

Minor Enhancement Opportunities (for future PRs):

• Consider adding a docstring to the helper function explaining the cross-test interference prevention
• The pattern could be documented in test guidelines for new contributors

---

🏆 Summary

This is an exemplary fix that transforms a targeted bug fix into a comprehensive, maintainable solution. The introduction of the entityFilteredSubscriber helper function shows excellent software engineering judgment. The change:

• Solves the root problem: Eliminates macOS test interference completely
• Improves code quality: Reduces duplication and improves maintainability
• Follows best practices: Proper functional design with reusable components
• Maintains reliability: Preserves all existing test behaviors while adding proper isolation

Recommendation: ✅ Excellent Implementation - This PR demonstrates how to properly evolve from a tactical fix to a strategic solution.

---

📋 Change Summary

Core Changes (core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs):

• Added: entityFilteredSubscriber helper function (lines 25-34) for consistent entity-based event filtering
• Updated: All subscription tests to use the helper function instead of inline filtering
• Improved: Code maintainability and test isolation across the entire test suite

CI Changes (.github/workflows/test.yml):

• Reformatted: PostgreSQL health check options for better readability (lines 31-33)
• No functional changes: CI behavior remains identical

--- | Branch: claude/pr-225-20251116-1057