fix: normalize pipeline ids when matching session blacklists - #854
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 17 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
Hello! The automated checks have been performed. 👋I've aggregated the results of the automated checks for this PR below. 📚 DocsEnsuring the codebase remains stable and healthy. 🛡️ ✅ All required documentation files present. ✅ 📋 Repo HealthEvaluating the repository's overall condition. 📋 ✅ All required files present. Latest Version: ✅ 🔎 Type CheckI've got some results for you! 📝 ❌ mypy: 300 error(s) found
Errors (showing first 10/300)🏷️ Release PreviewChecking if we're ready for the big release. 🏁 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🌍 Locale BuildJust a little bit of automation magic for you. ✨ ✅ Locale properly configured (64 files, 17 languages) Locale directories found:
Localization coverage:
pyproject.toml: ✅
Build manifest: ✅ 31 locale files included in package 📊 CoverageScanning the codebase for untested secrets... 🕵️ Files below 80% coverage (8 files)
Full report: download the 🔌 Plugin DetectionI've verified the plugin's 'versioning' logic. 🏷️ ❌ Plugin Status: ERRORS (1) Plugin Info:
OPM Detection:
Entry Point Validation:
⊘ No Issues:
🔒 Security (pip-audit)I've checked the firewalls of your PR. 🔥 ✅ No known vulnerabilities found (112 packages scanned). 🔨 Build TestsRunning the final assembly check. 🔧 ✅ All versions pass
🔌 Skill Tests (ovoscope)I ran the end-to-end skill tests to see how your skill behaves in the real world! 🎤 ✅ 13/37 passed ❌ **TestAdaptIntent** — 0/4
❌ **TestCancelIntentMidSentence** — 0/1
❌ **TestConverse** — 0/1
❌ **TestDeactivate** — 2/3
❌ **TestFallback** — 0/1
❌ **TestIntentPipelineRouting** — 0/4
❌ **TestLangDisambiguation** — 0/4
❌ **TestLegacyIntentIdBackCompat** — 0/2
❌ **TestNoSkills** — 0/2
❌ **TestPadatiousIntent** — 0/4
🚌 Bus CoverageA deep dive into the skill's communication patterns. 🌊 🔴 Coverage Summary
📊 Per-Skill Breakdown
🔍 Detailed Message Type Breakdown
|
JarbasAl
added a commit
that referenced
this pull request
Aug 13, 2026
…egration/full-compat rebuild v3
JarbasAl
force-pushed
the
fix/session-pipeline-blacklist-normalization
branch
from
August 31, 2026 14:09
154e9e5 to
73af2eb
Compare
JarbasAl
force-pushed
the
fix/session-pipeline-blacklist-normalization
branch
from
August 31, 2026 15:51
73af2eb to
5485141
Compare
session.blacklisted_pipelines is compared against session.pipeline as literal strings, but the deployment blacklist is naturally expressed in installed plugin IDs (e.g. "ovos-adapt-pipeline-plugin") while a session pipeline carries confidence-suffixed matcher IDs (e.g. "ovos-adapt-pipeline-plugin-high"). A base plugin policy ID silently failed to block its suffixed matcher variants. Per OVOS-PIPELINE-1 §3/§5.2 a blacklist entry names the plugin (the single actor), never a matcher variant of it: a plugin cannot be denied in one tier and invoked in another. A confidence-suffixed blacklist entry, legacy or canonical spelling, previously only blocked that one tier of the plugin, leaving its other tiers invokable. Normalize both the blacklist entries and the matcher id being checked through _PIPELINE_MIGRATION_MAP and strip the confidence suffix (_PIPELINE_RE) down to a bare plugin id before comparing, so a blacklisted plugin id -- suffixed or bare, legacy or canonical -- blocks every one of its matcher variants, filtered before matcher lookup (no invocation, no bus event for the skip). Split out of ovos-core#808 per review -- unrelated to that PR's fallback ping/pong migration-window fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JarbasAl
force-pushed
the
fix/session-pipeline-blacklist-normalization
branch
from
August 31, 2026 16:27
5485141 to
a6ae7fb
Compare
JarbasAl
marked this pull request as ready for review
August 31, 2026 16:34
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.
get_pipeline()comparedsession.blacklisted_pipelinesentries againstsession.pipelinematcher ids as literal strings. A deployment blacklist is naturally expressed in installed plugin ids (ovos-adapt-pipeline-plugin), while a session pipeline carries confidence-suffixed matcher ids (ovos-adapt-pipeline-plugin-high), so a bare plugin id in the blacklist silently failed to block any of its suffixed matcher variants. In addition, a confidence-suffixed blacklist entry itself — legacy spelling (adapt_high) or canonical (ovos-adapt-pipeline-plugin-high) — only denied that one tier, leaving the plugin's other tiers invokable in the same session.OVOS-PIPELINE-1 §3 and §5.2 are explicit that a
blacklisted_pipelinesentry names the plugin, a single actor, never a specific matcher configuration of it: a plugin cannot be denied in one confidence tier and invoked in another.The fix normalizes both sides of the comparison to a bare plugin id before checking membership: each blacklist entry is passed through
_PIPELINE_MIGRATION_MAP(legacy short id -> canonical id) and then has its confidence suffix stripped by_PIPELINE_RE, and the matcher id under test is normalized and stripped the same way.is_blacklisted()then only tests plugin-id membership, so a bare or suffixed, legacy or canonical, blacklist entry now denies every confidence tier of the plugin it names.Two regression tests were added to
test/unittests/test_intent_service_extended.py: one blacklists the legacy suffixed idadapt_highand one the canonical suffixed idovos-adapt-pipeline-plugin-medium, in both cases against a session pipeline listing all three adapt tiers plusfallback_high; both assert onlyfallback_highsurvives and thatget_pipeline_matcheris called exactly once, withfallback_high. Reverting only the source change (git apply -Ragainst the source diff, keeping the tests) made both new tests fail with anAssertionErrorshowing the adapt matchers still present in the result (e.g.['ovos-adapt-pipeline-plugin-medium', 'ovos-adapt-pipeline-plugin-low', 'fallback_high'] != ['fallback_high']); restoring the fix made them pass. The fulltest/unittests/test_intent_service_extended.pyandtest/unittests/test_intent_service.pysuites were run after the fix: 88 passed (9 subtests), no regressions.docs/prerelease-quirks.mdgained an entry stating that a session blacklist entry now denies the whole plugin regardless of tier spelling, legacy or canonical.