fix: dispatch ChatEngine.continue_chat with keyword arguments - #429
fix: dispatch ChatEngine.continue_chat with keyword arguments#429JarbasAl wants to merge 1 commit into
Conversation
Base ChatEngine wrappers (stream_tokens, stream_sentences, MultimodalChatEngine.stream_chat) called continue_chat positionally, so a subclass whose override dropped the `tools` parameter never crashed - nothing ever passed `tools` positionally. It only breaks when a caller passes tools= by keyword, as ovos-persona-server's server-side tool loop does. Switching those wrappers to keyword dispatch stops the divergent signature from hiding. Also add a ChatEngine.__init_subclass__ check that inspects the subclass's continue_chat signature via inspect.signature and emits a single LOG.warning per class (never a raise - a degraded plugin must not crash an entire install) when it names neither `tools` nor **kwargs. Fail-before: reverting only the source change (keeping the new tests) makes test_non_conforming_subclass_warns_once fail with "Expected 'warning' to have been called once. Called 0 times." Restoring the fix makes it and the rest of test_agents.py (72 tests) pass. Full suite: 970 passed, 49 skipped; 10 pre-existing failures in test_hardware.py from an unrelated ovos_color_parser/ovos_hardware_helpers version mismatch in the environment, unaffected by this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
I've gathered some intelligence on your latest changes. 🕵️♀️I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthChecking if we're following maintenance best practices. 📏 ✅ All required files present. Latest Version: ✅ ⚖️ License CheckEvaluating the impact of these changes on our licensing. 📈 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔒 Security (pip-audit)Security report: No threats detected in the area. ✅ ✅ No known vulnerabilities found (65 packages scanned). 🔨 Build TestsThe build is complete. No hard hats required. 👷♂️
❌ 3.10: Install OK, tests failed 📊 CoverageQuantifying the invisible strength of your code. 💪 ✅ 82.7% total coverage Files below 80% coverage (23 files)
Full report: download the 🔍 LintThe automated checks have finished their work. 🏁 ❌ ruff: issues found — see job log 🏷️ Release PreviewSetting the stage for the upcoming deployment. 🎭 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
Making code review just a little bit easier. 💆♂️ |
1 participant
ChatEngine's base wrappers (stream_tokens, stream_sentences, and MultimodalChatEngine.stream_chat) called self.continue_chat(messages, session_id, lang, units) positionally. That positional call is why plugins whose continue_chat override dropped the tools parameter never crashed: nothing in the base class ever passed tools positionally. The break only shows up when a caller passes tools= by keyword, which is exactly what ovos-persona-server's server-side tool loop does, and by then the failure surfaces far from the plugin that is actually non-conforming.
This PR switches those three wrappers to keyword dispatch (messages=, session_id=, lang=, units=), so any subclass whose continue_chat signature diverges from the base is exercised the same way real callers exercise it, instead of being shielded by argument order.
It also adds a ChatEngine.init_subclass check that inspects the subclass's continue_chat signature with inspect.signature at class-definition time. If the override names neither a tools parameter nor **kwargs, it logs a warning once per class, naming the class and the fix. It deliberately only warns and never raises: turning a degraded plugin into an import-time crash would take down an entire install over a single non-conforming plugin, which is worse than the silent-hiding problem this PR is fixing.
Tests cover a conforming subclass (names tools) producing no warning, a non-conforming subclass producing exactly one warning naming the class, a **kwargs subclass producing no warning, and the three base wrappers dispatching with the expected keyword arguments. Reverting only the source change and re-running the new tests makes test_non_conforming_subclass_warns_once fail with "Expected 'warning' to have been called once. Called 0 times."; restoring the fix makes it and the rest of test_agents.py (72 tests) pass. The full suite runs 970 passed, 49 skipped, with 10 pre-existing failures in test_hardware.py caused by an unrelated ovos_color_parser/ovos_hardware_helpers version mismatch in the test environment, unaffected by this change.