feat: skill_id lifecycle filter + eof_count for End2EndTest#110
Conversation
Two composable features for asserting scenarios that produce CONCURRENT dispatch lifecycles whose messages interleave non-deterministically (e.g. stopping a skill that is mid-dispatch — the stop dispatch and the interrupted skill's own §8 trio + §9.5 terminal race). - skill_id: filter captured messages to a single context skill_id before asserting, isolating one lifecycle. Routing checks are skipped while filtering (the filtered stream is not a source/destination flip-chain). - eof_count: end capture only after an eof topic has been seen N times — so capture spans all N lifecycles that each terminate on the same topic (e.g. two ovos.utterance.handled) before the filter runs. Run the same scenario once per skill_id to assert each lifecycle deterministically. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
Multi-EOF capture and skill_id filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Reporting for duty! The automated checks have completed. 🎖️I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthHow's the repo's pulse? Let's take a look. 💓 ✅ All required files present. Latest Version: ✅ ⚖️ License CheckReading the fine print so you don't have to! 🔎 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔒 Security (pip-audit)I've checked for any hardcoded credentials. 🔑 ✅ No known vulnerabilities found (78 packages scanned). 🔍 LintThe automated sentinel is back with news. 💂♂️ ❌ ruff: issues found — see job log 🏷️ Release PreviewChecking if we've included all the important changes. 📋 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔨 Build TestsEnsuring the gears are properly lubricated. 💧 ✅ All versions pass
📊 CoverageEnsuring the logic is battle-tested. ⚔️ ❌ 53.3% total coverage Files below 80% coverage (17 files)
Full report: download the Just doing my bit for the OpenVoiceOS ecosystem. 🌍 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ovoscope/__init__.py (1)
859-866: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOptional: guard against an empty post-filter result.
If
skill_idmatches nothing (e.g. a typo or a lifecycle that never tagged its context),messagesbecomes empty. Downstream consumers that index the list — notablymessages[-1]in thetest_final_sessionblock (Line 980) — would then raise anIndexErrorrather than producing a clear assertion failure. A short explicit check after filtering would surface the misconfiguration directly.🛡️ Optional guard
if self.skill_id is not None: messages = [m for m in messages if (m.context or {}).get("skill_id") == self.skill_id] + assert messages, f"❌ no messages matched skill_id='{self.skill_id}'" if self.verbose: print(f"💡 filtered to skill_id='{self.skill_id}': {len(messages)} messages")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ovoscope/__init__.py` around lines 859 - 866, Add an explicit empty-result check immediately after the skill_id filtering in the message collection logic so a typo or missing lifecycle tag fails with a clear assertion instead of later causing an IndexError. Update the filtering path in the relevant message-processing block that uses self.skill_id, and make the failure message explain that no messages matched the requested skill_id before any downstream code like the test_final_session logic accesses messages[-1].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unittests/test_capture_session.py`:
- Around line 188-207: The test currently creates a new CaptureSession for the
second capture, so it does not verify that CaptureSession.capture() resets EOF
state on reuse. Update the test to use the same CaptureSession instance for both
capture() calls, then confirm the second run still waits for the full eof_count;
reference CaptureSession.capture() and the _eof_seen state it should reset
between captures.
In `@test/unittests/test_end2end_extended.py`:
- Around line 1014-1025: The filtered end-to-end cases are disabling routing too
early in _common_kwargs, so they never cover the new execute() guard when
skill_id is set. Update _common_kwargs in test_end2end_extended.py to keep
routing enabled by default and only let execute() suppress routing assertions
when skill_id is present, so the filtered tests still exercise that branch.
---
Nitpick comments:
In `@ovoscope/__init__.py`:
- Around line 859-866: Add an explicit empty-result check immediately after the
skill_id filtering in the message collection logic so a typo or missing
lifecycle tag fails with a clear assertion instead of later causing an
IndexError. Update the filtering path in the relevant message-processing block
that uses self.skill_id, and make the failure message explain that no messages
matched the requested skill_id before any downstream code like the
test_final_session logic accesses messages[-1].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bfa79cb-feb5-4084-b297-14fae777203d
📒 Files selected for processing (3)
ovoscope/__init__.pytest/unittests/test_capture_session.pytest/unittests/test_end2end_extended.py
| def test_eof_count_resets_between_captures(self): | ||
| """The eof counter resets per capture() call so eof_count applies fresh.""" | ||
| cs = CaptureSession(self.mc, | ||
| eof_msgs=["test.eof"], | ||
| eof_count=2, | ||
| ignore_messages=[]) | ||
| self._emit_after(0.05, Message("test.eof")) | ||
| self._emit_after(0.10, Message("test.eof")) | ||
| cs.capture(Message("test.trigger1"), timeout=3) | ||
| cs.finish() | ||
| # a second capture must again require 2 eofs, not be already-done | ||
| cs2 = CaptureSession(self.mc, eof_msgs=["test.eof"], eof_count=2, | ||
| ignore_messages=[]) | ||
| self._emit_after(0.05, Message("test.eof")) | ||
| self._emit_after(0.10, Message("test.mid")) | ||
| self._emit_after(0.15, Message("test.eof")) | ||
| cs2.capture(Message("test.trigger2"), timeout=3) | ||
| types = [m.msg_type for m in cs2.finish()] | ||
| self.assertIn("test.mid", types) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reuse the same CaptureSession for the second capture.
This only proves that a new instance starts clean. If CaptureSession.capture() stopped resetting _eof_seen, this test would still pass because cs2 gets a fresh counter and fresh handlers.
Suggested change
def test_eof_count_resets_between_captures(self):
"""The eof counter resets per capture() call so eof_count applies fresh."""
cs = CaptureSession(self.mc,
eof_msgs=["test.eof"],
eof_count=2,
ignore_messages=[])
self._emit_after(0.05, Message("test.eof"))
self._emit_after(0.10, Message("test.eof"))
cs.capture(Message("test.trigger1"), timeout=3)
- cs.finish()
- # a second capture must again require 2 eofs, not be already-done
- cs2 = CaptureSession(self.mc, eof_msgs=["test.eof"], eof_count=2,
- ignore_messages=[])
+ first_len = len(cs.responses)
+ # a second capture on the same session must again require 2 eofs
self._emit_after(0.05, Message("test.eof"))
self._emit_after(0.10, Message("test.mid"))
self._emit_after(0.15, Message("test.eof"))
- cs2.capture(Message("test.trigger2"), timeout=3)
- types = [m.msg_type for m in cs2.finish()]
+ cs.capture(Message("test.trigger2"), timeout=3)
+ types = [m.msg_type for m in cs.finish()[first_len:]]
self.assertIn("test.mid", types)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_eof_count_resets_between_captures(self): | |
| """The eof counter resets per capture() call so eof_count applies fresh.""" | |
| cs = CaptureSession(self.mc, | |
| eof_msgs=["test.eof"], | |
| eof_count=2, | |
| ignore_messages=[]) | |
| self._emit_after(0.05, Message("test.eof")) | |
| self._emit_after(0.10, Message("test.eof")) | |
| cs.capture(Message("test.trigger1"), timeout=3) | |
| cs.finish() | |
| # a second capture must again require 2 eofs, not be already-done | |
| cs2 = CaptureSession(self.mc, eof_msgs=["test.eof"], eof_count=2, | |
| ignore_messages=[]) | |
| self._emit_after(0.05, Message("test.eof")) | |
| self._emit_after(0.10, Message("test.mid")) | |
| self._emit_after(0.15, Message("test.eof")) | |
| cs2.capture(Message("test.trigger2"), timeout=3) | |
| types = [m.msg_type for m in cs2.finish()] | |
| self.assertIn("test.mid", types) | |
| def test_eof_count_resets_between_captures(self): | |
| """The eof counter resets per capture() call so eof_count applies fresh.""" | |
| cs = CaptureSession(self.mc, | |
| eof_msgs=["test.eof"], | |
| eof_count=2, | |
| ignore_messages=[]) | |
| self._emit_after(0.05, Message("test.eof")) | |
| self._emit_after(0.10, Message("test.eof")) | |
| cs.capture(Message("test.trigger1"), timeout=3) | |
| first_len = len(cs.responses) | |
| # a second capture on the same session must again require 2 eofs | |
| self._emit_after(0.05, Message("test.eof")) | |
| self._emit_after(0.10, Message("test.mid")) | |
| self._emit_after(0.15, Message("test.eof")) | |
| cs.capture(Message("test.trigger2"), timeout=3) | |
| types = [m.msg_type for m in cs.finish()[first_len:]] | |
| self.assertIn("test.mid", types) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unittests/test_capture_session.py` around lines 188 - 207, The test
currently creates a new CaptureSession for the second capture, so it does not
verify that CaptureSession.capture() resets EOF state on reuse. Update the test
to use the same CaptureSession instance for both capture() calls, then confirm
the second run still waits for the full eof_count; reference
CaptureSession.capture() and the _eof_seen state it should reset between
captures.
| def _common_kwargs(self): | ||
| return dict( | ||
| minicroft=self.mc, | ||
| skill_ids=[SKILL_ID], | ||
| eof_msgs=["ovos.utterance.handled"], | ||
| eof_count=2, # both lifecycles terminate on ovos.utterance.handled | ||
| test_routing=False, | ||
| test_active_skills=False, | ||
| test_final_session=False, | ||
| ignore_messages=DEFAULT_IGNORED + HANDLER_LIFECYCLE, | ||
| verbose=False, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep routing enabled in the filtered cases.
The new contract is that execute() suppresses routing assertions when skill_id is set, but _common_kwargs() disables routing for every case up front. That means the filtered tests never exercise the new guard, so a regression there would still pass.
Suggested change
def _common_kwargs(self):
return dict(
minicroft=self.mc,
skill_ids=[SKILL_ID],
eof_msgs=["ovos.utterance.handled"],
eof_count=2, # both lifecycles terminate on ovos.utterance.handled
- test_routing=False,
test_active_skills=False,
test_final_session=False,
ignore_messages=DEFAULT_IGNORED + HANDLER_LIFECYCLE,
verbose=False,
)
@@
test = End2EndTest(
source_message=src,
expected_messages=[src],
test_message_number=False,
test_msg_type=False,
test_msg_data=False,
test_msg_context=False,
+ test_routing=False,
**self._common_kwargs(),
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unittests/test_end2end_extended.py` around lines 1014 - 1025, The
filtered end-to-end cases are disabling routing too early in _common_kwargs, so
they never cover the new execute() guard when skill_id is set. Update
_common_kwargs in test_end2end_extended.py to keep routing enabled by default
and only let execute() suppress routing assertions when skill_id is present, so
the filtered tests still exercise that branch.
1 participant
What
Two small, composable
End2EndTestfeatures for asserting scenarios that produce concurrent dispatch lifecycles whose messages interleave non-deterministically.The motivating case (OVOS-PIPELINE-1 §8): stopping a running skill emits two lifecycles — the stop dispatch and the interrupted skill's own §8 trio + §9.5 terminal, which completes asynchronously when the skill's daemon unwinds. Their interleaving order (and even which
ovos.utterance.handledarrives first) varies under load, so a strict single-sequence assertion is inherently flaky.skill_id(filter)Filter captured messages to a single
context["skill_id"]before asserting, isolating one lifecycle. Run the same scenario once perskill_idto assert each lifecycle deterministically. Routing checks are skipped while filtering (the filtered stream is not a source/destination flip-chain).eof_count(capture span)End capture only after an eof topic has been seen
Ntimes, so capture spans allNlifecycles that terminate on the same topic (e.g. twoovos.utterance.handled) before the filter runs. The counter resets percapture()call.Tests
test_capture_session.py:eof_countwaits for N occurrences; resets between captures.test_end2end_extended.py::TestSkillIdFilter: a skill emitting twoskill_id-tagged lifecycles is asserted once perskill_id; the unfiltered run (witheof_count=2) sees both.Both default to today's behaviour (
skill_id=None,eof_count=1) — fully backward compatible.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes