Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
Fresh off the press! I've got some check results for you. 🗞️I've aggregated the results of the automated checks for this PR below. 🏷️ Release PreviewComing soon to a stable branch near you! 📽️ Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔍 LintI've gathered the facts for your review. 📖 ❌ ruff: issues found — see job log 🔒 Security (pip-audit)Scanning the horizon for any zero-day threats. 🌅 ✅ No known vulnerabilities found (129 packages scanned). ⚖️ 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. 📋 Repo HealthA thorough inspection of the project's hygiene. 🧼 ✅ All required files present. Latest Version: ✅ 📊 CoverageThe coverage report is now available for inspection. 📋 Files below 80% coverage (15 files)
Full report: download the 🔨 Build TestsTesting the load-bearing capacity of your changes. 🏋️ ✅ All versions pass
Helping you build the future of voice, one check at a time. 🎙️ |
LEAN_DEFAULT_PIPELINE omitted ovos-fallback-pipeline-plugin-low, so any
fallback skill registered above priority 90 (e.g. fallback-unknown at
100) was unreachable under ovoscope's default pipeline — the harness
force-overrides both the default session pipeline and
Configuration()["intents"]["pipeline"] to this same lean list, so the
gap applied everywhere, not just to callers who opted into the lean
default explicitly. Added the -low stage after -medium, in tier order.
_DEFAULT_TRAINED_TIMEOUT defaulted to 5s outside CI. Any skill whose
training took longer raised RuntimeError out of get_minicroft, which
skips tearDownClass in E2E-style test classes built on it — leaking
class-level monkeypatches and MiniCroft state into later, unrelated
test files (observed corrupting six results in a downstream suite).
Raised the default to 180s unconditionally instead of gating it behind
the CI env var; the CI value was already sized for the worst observed
case and a local machine is not guaranteed to be faster than a
contended CI runner.
Updated test_lean_default_excludes_heavy_pipelines, which asserted the
absence of the -low stage as if it were a heavy/optional pipeline;
replaced with an assertion that it is present, since it's a required
tier not an opt-in extra.
Self-tested: reachability (fallback-unknown fires and speaks under the
default pipeline, confirmed to fail before the fix), non-regression
(an Adapt-registered intent still matches via adapt-high, unaffected
by the added tier), and the timeout default (a trained event arriving
at 6s raises RuntimeError under the old 5s default, completes cleanly
under the new 180s default).
TestTrainedTimeoutDefaults now imports _DEFAULT_TRAINED_TIMEOUT from the
module that defines it and asserts it equals 180.0. It previously
recomputed a literal inline from os.environ.get("CI") and compared it to
itself, so it stayed green for any value the module actually held.
Mutating the constant to 5.0 turns three of its four tests red.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1be5464 to
b187a8d
Compare
1 participant
LEAN_DEFAULT_PIPELINE ends at ovos-fallback-pipeline-plugin-medium and never includes ovos-fallback-pipeline-plugin-low. Fallback tiers run high(0,5)/medium(5,90)/low(90,101), so any fallback skill registered above priority 90 — ovos-skill-fallback-unknown registers at 100 — is unreachable under ovoscope's default pipeline. This isn't limited to callers who opt into the lean default: ovoscope's MiniCroft.run() force-overrides both SessionManager's default session pipeline and Configuration()["intents"]["pipeline"] to this same lean list, so the gap applies everywhere a test doesn't pass an explicit default_pipeline. Every fallback gate run against an unknown-utterance skill silently never fires. The fix adds the -low stage after -medium, in tier order.
The same file had a second too-short harness default: _DEFAULT_TRAINED_TIMEOUT was 5s outside CI. A skill whose training takes longer than that raises RuntimeError out of get_minicroft, which for E2E-style TestCases built on it means setUpClass raises and tearDownClass never runs — leaking class-level monkeypatches and MiniCroft state into whatever test file runs next in the same process (observed corrupting six results in an unrelated downstream test module, and turning a slow-but-successful boot into a misleading training RuntimeError in another gate). The CI-only 180s value was already sized for the worst field-observed case, and a local machine has no guarantee of being faster than a contended CI runner, so the default is now 180s unconditionally rather than gated behind the CI env var.
test_lean_default_excludes_heavy_pipelines previously asserted the absence of the -low stage as though it were an optional heavy pipeline (grouped with m2v/persona/common-query/OCP); it's a required tier, not an opt-in extra, so the assertion is replaced with one confirming its presence.
TestTrainedTimeoutDefaults now imports _DEFAULT_TRAINED_TIMEOUT from the module that defines it and asserts it equals 180.0. It used to recompute a literal inline from
os.environ.get("CI")and compare it to itself, so it stayed green whatever the module actually held — including the 5s value this PR removes. docs/minicroft.md states the single 180s default in both places it mentions one.AssertionError: 5.0 != 180.0)Full suite on the merged tree: 699 passed, 25 skipped.