feat: OVOS-PIPELINE-1 conformance (matched/unmatched/handled, updated_session, active_handlers)#778
Draft
JarbasAl wants to merge 7 commits into
Draft
feat: OVOS-PIPELINE-1 conformance (matched/unmatched/handled, updated_session, active_handlers)#778JarbasAl wants to merge 7 commits into
JarbasAl wants to merge 7 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds OVOS-PIPELINE-1 orchestration to ChangesOVOS-PIPELINE-1 intent orchestration and spec topic migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
I've returned from the depths of the test suite with news. 🤿I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthEnsuring the repo is getting enough sleep (aka stable releases). 💤 ✅ All required files present. Latest Version: ✅ 🔒 Security (pip-audit)A detailed security audit of your contribution. 📝 ✅ No known vulnerabilities found (110 packages scanned). 🏷️ Release PreviewChecking if we've met all our release criteria. ✅ Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🌍 Locale BuildHere's the lowdown on the latest automated check. 📉 ✅ Locale properly configured (64 files, 17 languages) Locale directories found:
Localization coverage:
pyproject.toml: ✅
Build manifest: ✅ 31 locale files included in package 📊 CoverageEnsuring we've got all the bases covered! ⚾ Files below 80% coverage (9 files)
Full report: download the 🔨 Build TestsI've fired up the furnaces and forged your changes. ⚒️ ✅ All versions pass
⚖️ License CheckDouble-checking the fine print for any surprises. 🔍 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔌 Skill Tests (ovoscope)Testing the skill's 'personality' for warmth and helpfulness. 😊 ✅ 2/36 passed ❌ **TestAdaptIntent** — 0/4
❌ **TestCancelIntentMidSentence** — 0/1
❌ **TestConverse** — 0/1
❌ **TestCountSkills** — 0/4
❌ **TestDeactivate** — 2/3
❌ **TestFallback** — 0/1
❌ **TestGlobalStopVocWithActiveSkill** — 0/1
❌ **TestGlobalStopVocabulary** — 0/2
❌ **TestIntentPipelineRouting** — 0/4
❌ **TestLangDisambiguation** — 0/4
❌ **TestNoSkills** — 0/2
❌ **TestPadatiousIntent** — 0/4
❌ **TestStopNoSkills** — 0/3
❌ **TestStopServiceNotASkill** — 0/1
❌ **TestStopSkillCanHandleFalse** — 0/1
🚌 Bus CoverageIs every event handler pullin' its weight? Let's check! 🏋️♂️ 🔴 Coverage Summary
📊 Per-Skill Breakdown
🔍 Detailed Message Type Breakdown
|
| Plugin Type | Wheel | Editable |
|---|---|---|
| pipeline | ✅ | ✅ |
Entry Point Validation:
| Entry Point | Type | Import | Interface |
|---|---|---|---|
ovos-converse-pipeline-plugin |
pipeline | ✅ 17ms | ⊘ |
ovos-fallback-pipeline-plugin |
pipeline | ✅ 2ms | ⊘ |
ovos-stop-pipeline-plugin |
pipeline | ✅ 56ms | ⊘ |
⊘ No settingsmeta.json
✅ requires-python >=3.10 — running Python 3.11
Issues:
⚠️ No settingsmeta.json found⚠️ No settingsmeta.json found
Standard Automated Signature v2.0 🏷️
JarbasAl
force-pushed
the
feat/pipeline-1-conformance-wt
branch
from
97f5751 to
215121b
Compare
JarbasAl
force-pushed
the
feat/pipeline-1-conformance-wt
branch
from
4f519cc to
279b183
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
test/unittests/test_intent_service_extended.py (1)
466-546: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
pipeline_idpropagation explicitly.These tests already pass
pipeline_id, but they never check the new attribution fields on the emitted dispatch orovos.intent.matchednotification. A regression that dropspipeline_idwould still pass this PR's new coverage.🤖 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_intent_service_extended.py` around lines 466 - 546, The active_handler tests cover session stamping but do not verify that pipeline attribution is preserved on emitted events. Update the _emit_match_message test cases to explicitly assert that the passed pipeline_id is propagated into the emitted dispatch and the ovos.intent.matched notification payload/context, so a regression that drops pipeline_id will fail. Use the existing _emit_match_message, svc.bus.emit, and pipeline_id values in these tests to locate and verify the new assertions.
🤖 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 @.github/workflows/ovoscope.yml:
- Around line 22-28: The `post_install_pip` setting in the `ovoscope` workflow
is using floating Git refs like `@dev` and `@feat/intent-4-producer`, which
makes the install set non-reproducible. Update the pinned package list to use
immutable commit SHAs or tags for each repository referenced there, keeping the
same package order and coverage while replacing the mutable refs so the workflow
resolves consistently.
In `@ovos_core/intent_services/service.py`:
- Around line 343-347: The required-slot check in the match filtering logic is
treating falsy values as missing, so valid slots like 0 or False get rejected.
Update the required-slot validation in the function that reads
match_data/__required_slots__ to test whether each required slot key is absent
from match_data rather than whether its value is truthy, so only truly missing
slots are filtered out.
- Around line 445-451: The updated session is being attached to reply.context
after self.bus.emit(reply.forward(f"{match.skill_id}.activate")), so the first
forwarded .activate event can still carry a stale session snapshot. In the
intent service flow around the match handling in service.py, update
reply.context["session"] from sess.serialize() before emitting the activation
event, and keep the session mutation tied to the same reply object so downstream
handlers always receive the latest session state.
In `@pyproject.toml`:
- Around line 19-24: The dependency constraint for ovos-workshop is still too
low and can resolve to an incompatible release. Update the version range in
pyproject.toml where ovos-workshop is listed so the minimum required version
matches the newer INTENT-4 producer branch, and keep the upper bound unchanged
unless the codebase requires otherwise.
In `@test/end2end/test_adapt.py`:
- Around line 76-81: The `test_adapt.py` intent-matched broadcast assertion is
too loose and can miss regressions in the `IntentService` payload. Update the
`Message(INTENT_MATCHED, ...)` expectation in this test to assert the full
payload, including `slots={}` and
`pipeline_id="ovos-adapt-pipeline-plugin-high"` alongside the existing
`skill_id`, `intent_name`, `utterance`, and `lang` fields.
In `@test/end2end/test_padatious.py`:
- Around line 76-81: The INTENT_MATCHED expectation in the padatious end-to-end
test is missing required payload fields, so update the Message(INTENT_MATCHED)
assertion to include the new slots and pipeline_id data alongside skill_id,
intent_name, utterance, and lang. Use the existing test case in
test_padatious.py to assert slots={} and
pipeline_id="ovos-padatious-pipeline-plugin-high" so the payload check matches
the spec and catches regressions.
In `@test/end2end/test_stop.py`:
- Around line 49-51: The INTENT_MATCHED filter is currently applied too broadly
in the stop test helpers, which can hide accidental matched broadcasts on the
no-match path. Update the stop scenario message filtering so INTENT_MATCHED is
only excluded for the dispatching exact/medium cases, and remove it from
_run_not_exact_high while keeping the IGNORE_MESSAGES behavior there. Use the
helper names _run_not_exact_high and the dispatching stop scenario setup to
locate the filter list.
---
Nitpick comments:
In `@test/unittests/test_intent_service_extended.py`:
- Around line 466-546: The active_handler tests cover session stamping but do
not verify that pipeline attribution is preserved on emitted events. Update the
_emit_match_message test cases to explicitly assert that the passed pipeline_id
is propagated into the emitted dispatch and the ovos.intent.matched notification
payload/context, so a regression that drops pipeline_id will fail. Use the
existing _emit_match_message, svc.bus.emit, and pipeline_id values in these
tests to locate and verify the new assertions.
🪄 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: 7a528f8a-451c-4749-94a9-9daa5cd65a2c
📒 Files selected for processing (14)
.github/workflows/ovoscope.ymlovos_core/intent_services/service.pypyproject.tomltest/end2end/test_activate.pytest/end2end/test_adapt.pytest/end2end/test_converse.pytest/end2end/test_fallback.pytest/end2end/test_intent_pipeline.pytest/end2end/test_lang_detect.pytest/end2end/test_no_skills.pytest/end2end/test_padatious.pytest/end2end/test_stop.pytest/end2end/test_stop_refactor.pytest/unittests/test_intent_service_extended.py
Comment on lines
+22
to
+28
| # Pin the integrated spec stack AFTER `.[test]` so the resolver cannot | ||
| # downgrade ovos-workshop below the 9.x INTENT-4 producer (the published | ||
| # fixture skills + plugins cap it lower). Mirrors the proven pin-set from | ||
| # OpenVoiceOS/ovos-test-harness@dev requirements.txt. Runs with | ||
| # --force-reinstall --no-deps; this PR's own core source still wins via the | ||
| # workflow's final local reinstall. Flip refs to @dev as each branch lands. | ||
| post_install_pip: "git+https://github.com/OpenVoiceOS/ovos-workshop@feat/intent-4-producer git+https://github.com/OpenVoiceOS/ovos-adapt-pipeline-plugin@dev git+https://github.com/OpenVoiceOS/ovos-padatious-pipeline-plugin@dev git+https://github.com/OpenVoiceOS/padacioso@dev git+https://github.com/OpenVoiceOS/ovoscope@dev git+https://github.com/OpenVoiceOS/ovos-skill-parrot@dev git+https://github.com/OpenVoiceOS/ovos-skill-fallback-unknown@dev git+https://github.com/OpenVoiceOS/ovos-skill-count@dev git+https://github.com/OpenVoiceOS/ovos-skill-hello-world@dev" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import pathlib, re
text = pathlib.Path(".github/workflows/ovoscope.yml").read_text()
m = re.search(r'post_install_pip:\s*"([^"]+)"', text)
for dep in m.group(1).split():
ref = dep.rsplit("@", 1)[-1]
immutable = bool(re.fullmatch(r"[0-9a-fA-F]{7,40}", ref))
print(f"{dep}\t{'immutable' if immutable else 'moving-ref'}")
PYRepository: OpenVoiceOS/ovos-core
Length of output: 805
Use immutable refs for post_install_pip
@dev and @feat/intent-4-producer still drift, so this job isn’t actually pinned and can change behavior between runs. Use commit SHAs or tags here instead.
🤖 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 @.github/workflows/ovoscope.yml around lines 22 - 28, The `post_install_pip`
setting in the `ovoscope` workflow is using floating Git refs like `@dev` and
`@feat/intent-4-producer`, which makes the install set non-reproducible. Update
the pinned package list to use immutable commit SHAs or tags for each repository
referenced there, keeping the same package order and coverage while replacing
the mutable refs so the workflow resolves consistently.
Comment on lines
+343
to
+347
| match_data = match.match_data or {} | ||
| required_slots = match_data.get("__required_slots__") | ||
| if not required_slots: | ||
| return [] | ||
| return [slot for slot in required_slots if not match_data.get(slot)] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't treat every falsy slot value as missing.
Line 347 currently declines matches when a required slot is present but holds a falsy value like 0 or False. That widens the backstop from “slot absent” to “slot falsy” and can skip valid matches.
Suggested fix
- return [slot for slot in required_slots if not match_data.get(slot)]
+ return [
+ slot for slot in required_slots
+ if slot not in match_data or match_data[slot] in (None, "")
+ ]📝 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.
Suggested change
| match_data = match.match_data or {} | |
| required_slots = match_data.get("__required_slots__") | |
| if not required_slots: | |
| return [] | |
| return [slot for slot in required_slots if not match_data.get(slot)] | |
| match_data = match.match_data or {} | |
| required_slots = match_data.get("__required_slots__") | |
| if not required_slots: | |
| return [] | |
| return [ | |
| slot for slot in required_slots | |
| if slot not in match_data or match_data[slot] in (None, "") | |
| ] |
🤖 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 `@ovos_core/intent_services/service.py` around lines 343 - 347, The
required-slot check in the match filtering logic is treating falsy values as
missing, so valid slots like 0 or False get rejected. Update the required-slot
validation in the function that reads match_data/__required_slots__ to test
whether each required slot key is absent from match_data rather than whether its
value is truthy, so only truly missing slots are filtered out.
Comment on lines
445
to
451
| if not _produces_reserved_name(pipeline_id): | ||
| sess.add_active_handler(match.skill_id) | ||
| # emit event for skills callback -> self.handle_activate | ||
| self.bus.emit(reply.forward(f"{match.skill_id}.activate")) | ||
|
|
||
| # update Session if modified by pipeline | ||
| reply.context["session"] = sess.serialize() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate the updated session before emitting .activate.
Line 448 forwards <skill_id>.activate before Line 451 overwrites reply.context["session"]. When Match.updated_session is a different snapshot, that first downstream event still carries the pre-match session, so the new propagation contract is already broken on the first bus hop.
Suggested fix
if not was_deactivated:
if not _produces_reserved_name(pipeline_id):
sess.add_active_handler(match.skill_id)
+ reply.context["session"] = sess.serialize()
# emit event for skills callback -> self.handle_activate
self.bus.emit(reply.forward(f"{match.skill_id}.activate"))
# update Session if modified by pipeline
reply.context["session"] = sess.serialize()📝 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.
Suggested change
| if not _produces_reserved_name(pipeline_id): | |
| sess.add_active_handler(match.skill_id) | |
| # emit event for skills callback -> self.handle_activate | |
| self.bus.emit(reply.forward(f"{match.skill_id}.activate")) | |
| # update Session if modified by pipeline | |
| reply.context["session"] = sess.serialize() | |
| if not _produces_reserved_name(pipeline_id): | |
| sess.add_active_handler(match.skill_id) | |
| reply.context["session"] = sess.serialize() | |
| # emit event for skills callback -> self.handle_activate | |
| self.bus.emit(reply.forward(f"{match.skill_id}.activate")) | |
| # update Session if modified by pipeline | |
| reply.context["session"] = sess.serialize() |
🤖 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 `@ovos_core/intent_services/service.py` around lines 445 - 451, The updated
session is being attached to reply.context after
self.bus.emit(reply.forward(f"{match.skill_id}.activate")), so the first
forwarded .activate event can still carry a stale session snapshot. In the
intent service flow around the match handling in service.py, update
reply.context["session"] from sess.serialize() before emitting the activation
event, and keep the session mutation tied to the same reply object so downstream
handlers always receive the latest session state.
Comment on lines
+19
to
+24
| "ovos_bus_client>=2.6.1a1,<3.0.0", | ||
| "ovos-plugin-manager>=2.5.0a1,<3.0.0", | ||
| "ovos-config>=0.0.13,<3.0.0", | ||
| "ovos-workshop>=8.3.0a1,<10.0.0", | ||
| "rapidfuzz>=3.6,<4.0", | ||
| "ovos-spec-tools[langcodes]>=0.9.0a1", | ||
| "ovos-spec-tools[langcodes]>=0.11.0a1,<1.0.0", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "pyproject.toml:"
sed -n '19,24p' pyproject.toml
echo
echo ".github/workflows/ovoscope.yml:"
sed -n '22,28p' .github/workflows/ovoscope.ymlRepository: OpenVoiceOS/ovos-core
Length of output: 1483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All ovos-workshop constraints in the repo:"
rg -n --hidden --no-ignore-vcs 'ovos-workshop' .
echo
echo "pyproject.toml dependency section:"
sed -n '1,120p' pyproject.tomlRepository: OpenVoiceOS/ovos-core
Length of output: 8974
Raise the ovos-workshop floor in pyproject.toml.
This dependency range still allows ovos-workshop>=8.3.0a1, while the workflow now requires the newer INTENT-4 producer branch. That leaves published/local installs able to resolve an incompatible workshop version.
🤖 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 `@pyproject.toml` around lines 19 - 24, The dependency constraint for
ovos-workshop is still too low and can resolve to an incompatible release.
Update the version range in pyproject.toml where ovos-workshop is listed so the
minimum required version matches the newer INTENT-4 producer branch, and keep
the upper bound unchanged unless the codebase requires otherwise.
Comment on lines
+76
to
+81
| # OVOS-PIPELINE-1 §9.2: matched broadcast precedes the dispatch | ||
| Message(INTENT_MATCHED, | ||
| data={"skill_id": self.skill_id, | ||
| "intent_name": f"{self.skill_id}:HelloWorldIntent", | ||
| "utterance": "hello world", "lang": session.lang}, | ||
| context={"skill_id": self.skill_id}), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the full ovos.intent.matched payload.
IntentService now includes both slots and pipeline_id in this broadcast, so this test would still pass if either contract regressed. Please assert slots={} and pipeline_id="ovos-adapt-pipeline-plugin-high" here as well.
🤖 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/end2end/test_adapt.py` around lines 76 - 81, The `test_adapt.py`
intent-matched broadcast assertion is too loose and can miss regressions in the
`IntentService` payload. Update the `Message(INTENT_MATCHED, ...)` expectation
in this test to assert the full payload, including `slots={}` and
`pipeline_id="ovos-adapt-pipeline-plugin-high"` alongside the existing
`skill_id`, `intent_name`, `utterance`, and `lang` fields.
Comment on lines
+76
to
+81
| # OVOS-PIPELINE-1 §9.2: matched broadcast precedes the dispatch | ||
| Message(INTENT_MATCHED, | ||
| data={"skill_id": self.skill_id, | ||
| "intent_name": f"{self.skill_id}:Greetings.intent", | ||
| "utterance": "good morning", "lang": session.lang}, | ||
| context={"skill_id": self.skill_id}), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the full ovos.intent.matched payload.
This matched-event expectation omits the new slots and pipeline_id fields, so it won't catch regressions in the spec payload the PR is adding. Please assert slots={} and pipeline_id="ovos-padatious-pipeline-plugin-high" here too.
🤖 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/end2end/test_padatious.py` around lines 76 - 81, The INTENT_MATCHED
expectation in the padatious end-to-end test is missing required payload fields,
so update the Message(INTENT_MATCHED) assertion to include the new slots and
pipeline_id data alongside skill_id, intent_name, utterance, and lang. Use the
existing test case in test_padatious.py to assert slots={} and
pipeline_id="ovos-padatious-pipeline-plugin-high" so the payload check matches
the spec and catches regressions.
Comment on lines
+49
to
+51
| # ovos.intent.matched (§9.2) precedes every dispatch; these scenarios assert | ||
| # stop routing/activation, not the matched broadcast, so it is filtered here. | ||
| INTENT_MATCHED, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope this INTENT_MATCHED filter to the dispatching stop scenarios.
_run_not_exact_high also reuses IGNORE_MESSAGES, so an accidental ovos.intent.matched emission on that no-match path would be hidden and the test would still pass. Keep this filter for the exact/medium dispatch cases, but not for the unmatched one.
🤖 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/end2end/test_stop.py` around lines 49 - 51, The INTENT_MATCHED filter is
currently applied too broadly in the stop test helpers, which can hide
accidental matched broadcasts on the no-match path. Update the stop scenario
message filtering so INTENT_MATCHED is only excluded for the dispatching
exact/medium cases, and remove it from _run_not_exact_high while keeping the
IGNORE_MESSAGES behavior there. Use the helper names _run_not_exact_high and the
dispatching stop scenario setup to locate the filter list.
be8864c to
43825c8
Compare
…, active_handlers push, updated_session adoption Rebases the conformance work cleanly onto dev (which already landed §8 trio + §9 utterance-terminal events via #788, the spec-tools 1.x cap via #790, and the workshop 9.x floor via #779). The previous branch bumped bus-client to 2.6.1a1 and re-added a stale spec-tools pin, producing an unsatisfiable resolver (ovos-core==2.2.4a3 cannot be used) — dropped; dev's deps already satisfy the stack. The PR's revert of #788's IntentDispatcher is also dropped — dev's dispatcher-based §7/§8 path is kept and the new clauses are layered onto it. OVOS-PIPELINE-1 conformance added: - §4.2 ``updated_session`` adoption: ``_dispatch_match`` now returns the working session (``Match.updated_session or inbound``) and ``handle_utterance`` adopts it for the remainder of the lifecycle, so a plugin's match-phase session mutation reaches downstream stages and the default-session sync. - §6.2 required_slots orchestrator backstop: after the §5.3/§5.4 denylist checks, the orchestrator verifies the match's slot map contains every slot in ``match_data['__required_slots__']``; if any is *absent* it treats the match as if the plugin declined and continues iteration (no bus event, observable only as a non-match). A present slot carrying a falsy value (0/False/'') is a legitimate, present slot — only a missing key is missing (CodeRabbit fix; the prior truthiness check wrongly rejected valid slots). Sourced from the Match itself today (no INTENT-4 §10 manifest is plumbed yet); TODO notes the upgrade path. - §7.1 ``session.active_handlers`` push + §7.3 reserved-name suppression: a claiming non-reserved dispatch pushes ``{skill_id, activated_at}`` head-first via the spec ``Session.add_active_handler`` helper (dedup-promoting prior entries), applied after ``Match.updated_session`` is committed so a plugin's wipe (e.g. STOP-1 global stop) sees the stamp land on top. The push is SUPPRESSED for reserved intent_names (converse/response/stop/fallback/common_query) per §7.1 — keyed *strictly off the reserved-name registry (the Match's intent_name)*, not off the producing pipeline_id (the prior pipeline_id-based keying was non-conformant; a reserved name produced by any pipeline suppresses). - Ordering fix (CodeRabbit): the working session is serialized into ``reply.context['session']`` *before* the ``{skill_id}.activate`` forward is emitted, so subscribers receive the latest session snapshot instead of a stale one. Tests: - test_intent_service_extended: new ``TestActiveHandlersPush`` (push, dedup, reserved-name suppression keyed off intent_name, deactivated, pipeline_id propagation into §9.2 matched + dispatch context, session-serialize-before- activate ordering, §4.2 return) and ``TestRequiredSlotsBackstop`` (unit + match-loop behaviour; falsy-present-value coverage). - test_adapt / test_padatious e2e: assert the §3.1/§9.2 ``pipeline_id`` is propagated into the ``ovos.intent.matched`` payload and dispatch context (a regression that drops it now fails).
… __required_slots__ The orchestrator backstop for required_slots (PIPELINE-1 §6.2) was reading from a made-up match_data.__required_slots__ convention that no plugin implements. Per spec, required_slots is an optional field in ovos.intent.register.template broadcasts (INTENT-4 §6.1), stored in the orchestrator's manifest (INTENT-4 §10) as part of the full registration payload. - Remove __required_slots__ convention entirely - _missing_required_slots now reads from IntentManifest.definition via _manifest_required_slots() - Converted from staticmethod to instance method to access self.intent_manifest - Tests rewritten to register template intents on the manifest rather than injecting __required_slots__ into match_data
JarbasAl
force-pushed
the
feat/pipeline-1-conformance-wt
branch
from
43825c8 to
01d7ea1
Compare
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.
Brings the intent-service orchestrator (
ovos_core/intent_services/service.py) into line with OVOS-PIPELINE-1.What's done
Match.updated_sessionreaches downstream state. Previously the snapshot was adopted only into the dispatch reply; for the default session it was discarded fromSessionManager(the end-of-handle_utterancesync reserialized the pre-match session)._emit_match_messagenow returns the working session andhandle_utterancewrites it back intomessage.context["session"], so a plugin's match-phase mutation reaches the default-session sync and every downstream stage.ovos.intent.matched. Broadcast (SpecMessage.INTENT_MATCHED) before every dispatch, carrying{skill_id, intent_name, lang, utterance, slots, pipeline_id}. It's a notification, not a dispatch.ovos.intent.unmatched.send_complete_intent_failureemits the spec topicSpecMessage.INTENT_UNMATCHED. This topic is a 1:1 rename in ovos-spec-toolsMIGRATION_MAP(complete_intent_failure → ovos.intent.unmatched), so the bus bridge re-delivers legacycomplete_intent_failuretransparently — no hand-rolled dual-emit.pipeline_idstamping. The matching stage's id is stamped on the dispatchcontext["pipeline_id"]so every downstream Message is attributable to the plugin, and it surfaces in the matched payload.SpecMessage.UTTERANCE_CANCELLED/UTTERANCE_HANDLEDconstants.ovos-spec-tools>=0.11.0a1(the floor whereINTENT_UNMATCHEDis inMIGRATION_MAP).Tests updated to assert the spec topics (the bus bridge keeps legacy working): no-match sequences assert
ovos.intent.unmatched; matched-path sequences assert the newovos.intent.matched(or filter it viaignore_messageswhere the test asserts routing, not the notification).What's deferred (out of scope here)
session.active_handlerspush. The bus-clientSessionmodel storesactive_skillsas[skill_id, timestamp]pairs, not the spec'sactive_handlersarray of{skill_id, activated_at, ...}objects (OVOS-SESSION-1 §3 field table). Adding theactive_handlersfield + its{skill_id, activated_at}push (and the reserved-name suppression of §7.3) is an ovos-bus-client session-model change and belongs in a bus-client PR; this PR does not force a risky model change. The existingactivate_skill(...)recency tracking is preserved.ovos.utterance.handled. These are still emitted by ovos-workshop's skill base (_on_event_start/_on_event_end), not the orchestrator. The end-marker invariant (exactly oneovos.utterance.handledper utterance) holds today across cancel / no-match / matched paths. Relocating the trio + success-path end-marker into the orchestrator (which dispatches asynchronously over the bus and has no synchronous handler-return point) is a separate cross-repo refactor.Tests
python -m pytest test/unittests/→ 244 passed (incl. the updatedsend_complete_intent_failureassertion).LD_LIBRARY_PATHpointing at a local libfann build (the padatious plugin needslibdoublefann.so.2). Updated no-match / matched sequences pass (test_no_skills,test_lang_detect,test_activate, and the blacklist/no-match subtests oftest_adapt/test_padatious/test_converse). The success-match scenariostest_adapt_match,test_padatious_match,test_parrot_modefail identically on cleandev— a pre-existing skill-matching/fixture issue in the local MiniCroft, not introduced here. The fulltest/end2endrun does not finish within the local time budget (persona/converseget_responsetimeouts) on eitherdevor this branch; noFappears before the cap.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes