fix: Han audit round 1 — teardown safety, session isolation, harness lifecycle - #118
Merged
Conversation
End2EndTest.execute() and from_message() only stopped the MiniCroft on the success path, so a failing assertion left SessionManager.bus, default_session and Configuration patched for every later test. Both now run stop() from a finally block. MiniCroft snapshots the whole default Session at boot and restores it in stop(), so inject_active activations and wire-folded session values no longer outlive the test that made them. Mock-TTS unduck timers are tracked, made daemon and cancelled in stop(). An orphaned timer could otherwise emit onto a closed bus and fold a stale session onto the global SessionManager during a later test. CaptureSession resets its eof state atomically, records a timed_out flag, and returns a copy from finish(). A capture timeout now fails with a clear message instead of surfacing as a message-count mismatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BusCoverageTracker snapshotted the session-cumulative global collector and added it into per-test counts, so every later test inherited the invocations of every earlier one. The snapshot is now a baseline and the report uses the delta over the tracker's own lifetime, frozen at start_tracking() so the tracking window is not counted twice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cmd_run booted a MiniCroft but never assigned it to the test, so execute() booted a second managed one and both patched the same globals. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
match() treated mycroft.skill.handler.start as a failure signal, but it fires on a SUCCESSFUL match — so a successful match returned None. It also checked the failure flag before the captured message and spun a watcher thread that polled at 20Hz forever after a timeout. match_result() now returns a discriminated matched/no-match/timeout outcome and waits on the events directly. assert_no_match() fails on a timeout instead of passing vacuously; match() keeps its old signature. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The docstring told callers to emit after calling the helper, which is impossible single-threaded because the helper blocks. It now takes an optional emit= message and sends it once the handlers are in place. A match that raced an intent failure could also be dropped; appends are guarded by a lock and re-read once before giving up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The side effect inspected `mock.url` on a MagicMock, so no configured URL
ever matched and json() always returned {}. It now lives on the patched
GET, which receives the URL.
OCPTest also waits for ovos.common_play.query.response instead of sleeping
half the timeout, and stops the MiniCroft from a finally block.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AudioServiceHarness.__exit__ skipped bus.close() when shutdown() raised. ListenerHarness and MiniListener left their wildcard "message" capture handler on the bus, so a shared bus kept feeding a dead harness. PlaybackServiceHarness now restores the TTS.queue object it replaced and refuses a second concurrent harness, because TTS.queue is process-wide class state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A load failure was warned about and skipped, then resurfaced much later as an unrelated assert_emitted timeout. Loading now raises by default; pass tolerate_load_errors=True to keep going, in which case the errors are kept in load_errors and quoted in assert_emitted failures. MiniPHAL.__exit__ also detaches its capture handler and closes the bus. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cached MiniCrofts were never stopped and two could be live at once, each clobbering the same globals. At most one stays live now, and an atexit hook stops the rest. _wait_for_m2v_sync removes its three listeners in a finally block and only pays the 3.5s pad when no m2v activity was observed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feed_file ignored the join() result, so a listener thread that outlived its stop() kept appending to _messages during the next run. A still-alive thread is now logged and replaced with a fresh listener object. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`except (ImportError, Exception): pass` turned a malformed pyproject.toml into an understated coverage number. TOMLDecodeError and OSError are now caught explicitly and recorded in EcosystemCoverageReport.parse_errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The client was left in place on a ConnectionError, so its reconnect thread lived for the rest of the process. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
One test per defect, each written to fail against the pre-fix code: teardown on the failure path, default-session isolation, TTS timer lifecycle, bus-coverage deltas, CaptureSession races, pipeline match verdicts, wait_for_match subscription order, the OCP HTTP mock, harness teardown, PHAL load errors, coverage parse errors and the RemoteRecorder connect leak. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
raise-by-default made the old warn-and-skip expectation wrong; cover both the default raise and the tolerate_load_errors opt-out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThe PR hardens MiniCroft and harness lifecycle cleanup, improves event matching and timeout reporting, isolates coverage tracking, exposes parser failures, clarifies PHAL load-error handling, and adds regression tests for these behaviors. ChangesHarness reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PipelineHarness
participant MessageBus
participant wait_for_match
participant OCPTest
PipelineHarness->>MessageBus: Emit utterance
PipelineHarness->>wait_for_match: Wait for expected messages
wait_for_match->>MessageBus: Register handlers
MessageBus-->>wait_for_match: Return match or failure
OCPTest->>MessageBus: Register query listeners
MessageBus-->>OCPTest: Return query response
Possibly related PRs
✨ 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 |
Tada! The results of the latest automation run are here. 🎉I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthEnsuring the repo's skin is clear (aka linting errors). ✨ ✅ All required files present. Latest Version: ✅ 🔍 LintA quick update on the status of your PR. 🔔 ❌ ruff: issues found — see job log 🔒 Security (pip-audit)The security sentinel has finished its patrol. 💂♂️ ✅ No known vulnerabilities found (79 packages scanned). 🏷️ Release PreviewI've checked the 'Security Updates' section. 🛡️ Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔨 Build TestsConstruction of your features is officially finished. 🏠 ✅ All versions pass
📊 CoverageThe coverage report is now available for inspection. 📋 ❌ 56.6% total coverage Files below 80% coverage (18 files)
Full report: download the ⚖️ License CheckI've verified the license compliance for your changes. ✅ ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. An automated high-five for your latest changes! 🖐️ |
CI showed two gaps: the session restore bailed out when boot replaced the default-session singleton, leaking exactly the state it exists to scrub — restore now targets whatever object holds the role at stop() time. And on Python 3.10 there is no stdlib tomllib, so a malformed pyproject.toml was silently ignored — depend on the tomli backport there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ovos-bus-client 1.x has serialize/deserialize, 2.x to_dict/from_dict; the snapshot silently became None on 1.x and the restore no-opped. Support both and warn instead of failing silently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Fixes 16 defect groups found by an automated expert audit (behavioral, concurrency, and resilience analysts) of the ovoscope package:
Teardown & global state
End2EndTest.execute()/from_message()/MockOCPTestnow tear down MiniCroft infinally— a failing assertion no longer leaks patched process-globals (SessionManager.bus, Configuration, pipeline/blacklists) into later testsSessionis snapshotted at boot and fully restored instop()(incl.active_skillsmutated viainject_activeor session-less messages); fixes the pre-existingtest_default_pipeline_overrides_default_sessionfailurestop()— no more orphaned timers emitting on a closed bus and corrupting the global SessionManager mid-later-test_shared_minicroft(intent_cases) keeps at most one live cached instance and stops them at exit; m2v sync listeners are removed and the 3.5s pad only applies when no activity was seenCorrectness of verdicts and reports
PipelineHarness.match():mycroft.skill.handler.startno longer treated as failure (it fires on success); real matches win over concurrent failure signals; the 20Hz-forever watcher thread is gone;assert_no_matchnow fails on timeout instead of passing vacuouslyCaptureSession: atomic eof-counter reset, capture timeout surfaced as an explicit "capture timed out" assertion instead of a baffling count mismatch,finish()returns a copycmd_runreuses the already-booted MiniCroft instead of booting a second one over the same globalsError propagation & resource lifecycle
tolerate_load_errors=False), with load errors quoted in assert failures when toleratedwait_for_matchgained anemit=parameter (docstring previously described an impossible single-threaded call order); match/failure race hardened__exit__s close buses and remove capture handlers even when shutdown raises (audio, PHAL, voice_loop, listener)MiniSimpleListener.feed_filereplaces a wedged listener thread instead of reusing it (silent cross-run message pollution)RemoteRecorder.connect()closes the bus client on timeout instead of leaking a reconnecting threadTests: 40 adversarial regression tests (one per defect, written to fail against pre-fix code). Note: the local shared venv gained unrelated editable plugin installs mid-campaign that make some MiniCroft boots slow/flaky locally (fails on pristine dev too); clean CI is the arbiter here. Known upstream follow-up tracked for round 2: MiniCroft instances retain ~650MB after stop() (reference leak).
Bugs found by automated expert audit; fixes implemented by Claude (opus), orchestrated by Claude Fable.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes