fix: cross-skill context writes go to the session; the legacy topics stay as compat - #629
openvoiceos-bot wants to merge 2 commits into
Conversation
…stay as compat set_cross_skill_context and remove_cross_skill_context now write or remove a shared bare key in session.intent_context (OVOS-CONTEXT-1 §3, §5.0, §5.3). The write rides every Message the handler derives with forward. The legacy mycroft.skill.set_cross_context and remove_cross_context broadcasts are still sent for skills that listen to them, with a deprecation that names the removal in 10.0.0. set_context and set_cross_skill_context accept the CONTEXT-1 §2 decay fields turns_remaining and expires_at. Supersedes #605. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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 |
Beep boop! Here's the latest status of your PR checks. 🤖I've aggregated the results of the automated checks for this PR below. 📡 Channel Compat — stableJust keeping you informed on the state of things. ℹ️ 🚧 Channel unresolvable with this checkout — the repo's dependency floors exceed what the channel pins (fleet finding; see the install log). Constraints: https://raw.githubusercontent.com/OpenVoiceOS/OpenVoiceOS/main/constraints-stable.txt 📡 Channel Compat — testingAnalysis complete! Check out the details below. 📊 🚧 Channel unresolvable with this checkout — the repo's dependency floors exceed what the channel pins (fleet finding; see the install log). Constraints: https://raw.githubusercontent.com/OpenVoiceOS/OpenVoiceOS/main/constraints-testing.txt 📋 Repo HealthEnsuring the project's documentation is healthy. 📚 ✅ All required files present. Latest Version: ✅ ⚖️ License CheckEnsuring our CLA requirements are met. 🖋️ ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔨 Build TestsRunning the final assembly check. 🔧 ✅ All versions pass
🔍 LintEnsuring the quality bar remains high. 📈 ❌ ruff: issues found — see job log 🔒 Security (pip-audit)A detailed security audit of your contribution. 📝 ✅ No known vulnerabilities found (72 packages scanned). From the digital workshop of OpenVoiceOS. 🛠️ |
…ontext
Fix 5 of architecture's review of this pull request, required before the
merge. The emit is untouched, so no deployed surface is withdrawn.
OVOS-CONTEXT-1 §5.0, at architecture fd48a09, the ref this branch adopts:
There is no context-mutation topic: no participant emits a Message
whose purpose is to announce a context change to the orchestrator or
to another component.
At the reviewed head `handle_set_cross_context` called `self.set_context`,
which passes `original_key`, which is the branch that writes
`session.set_intent_context`. So a Message whose only purpose is to
announce a context change caused a session write in every receiving
skill: one logical mutation became N+1 session pushes, each returned at
handler completion (SESSION-2 §2.6), which is the race §5.0 names in its
own rationale.
Both listeners now reach the `original_key is None` path, which touches
only the pre-CONTEXT-1 adapt `session.context` field through the legacy
`add_context` / `remove_context` message. A skill's own `set_context` and
`remove_context` calls keep the private session write they have on dev;
only the broadcast-driven path gives it up. The shared entry written by
`set_cross_skill_context` is then the only `session.intent_context`
mutation the call causes.
The docstrings that called the legacy path "a different mechanism" now
say what each path touches instead, and `set_cross_skill_context` records
that its decay window comes from the configuration of the process that
calls it, which on a satellite is not the orchestrator's (review fix 3).
The `{"value": null, "turns_remaining": 1}` literal is cited to §3.2,
where it is, and no longer to §1.2 (review fix 4).
The two listener tests assert the new contract: the emitted compat
message carries no resolved `key`, and `session.intent_context` is
untouched in the receiver.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
This PR supersedes #605. It carries the #605 change on current dev. #605 was 19 commits behind dev, and its build red came from that old base.
Governing clauses
OpenVoiceOS/architecture
intent-context.md(OVOS-CONTEXT-1) at fd48a09.§5.0: "There is no context-mutation topic: no participant emits a Message whose purpose is to announce a context change to the orchestrator or to another component."
§5.3 step 3: "Any Message the handler separately derives via MSG-1
forwardfrom the dispatch Message carries the write to the orchestrator and on to the originating client through any layer-2 transport that routes by those fields, as of the moment that Message is derived."What changed
set_cross_skill_contextwrites the entry as a shared bare key insession.intent_context.remove_cross_skill_contextremoves it.set_contextandset_cross_skill_contextaccept the §2 decay fieldsturns_remainingandexpires_at. Whenexpires_atis omitted, thecontext.timeoutdefault applies as before. That default is read from the configuration of the process that makes the call, which on a satellite is not the orchestrator's configuration.The private
set_contextandremove_contextpath sends the same messages as on dev.Second commit (
1e6e6af, review fix 5): the legacy listenershandle_set_cross_contextandhandle_remove_cross_contextno longer writesession.intent_context. See below.Compat emit, and what its listener does
The legacy topics
mycroft.skill.set_cross_contextandmycroft.skill.remove_cross_contextare still sent. A skill on an older ovos-workshop learns the entry only from these topics. The appendix retires the topics, so each call logs a deprecation that names the removal in 10.0.0.At the first commit the listener for those topics called
set_context, which writes a private entry insession.intent_context. That made the broadcast a second writer of the session: one call toset_cross_skill_contextproduced N+1 session pushes, one per receiving skill, each returned at handler completion (SESSION-2 §2.6). §5.0 removes exactly that class of mechanism, and its rationale names this race.The second commit routes both listeners to the path that touches only the pre-CONTEXT-1 adapt
session.contextfield, through the legacyadd_context/remove_contextmessage. A skill's ownset_contextandremove_contextcalls keep the private session write they have on dev; only the broadcast-driven path gives it up. The shared write inset_cross_skill_contextis then the onlysession.intent_contextmutation the call causes.Wire result (real messagebus, two skills)
A handler calls
set_cross_skill_context("Location", "lisbon")and then sends a Message made withforward.dev: the broadcast and the derived Message carry
intent_contextnull. Each receiving skill writes only a private copy.First commit: both Messages carry
"Location": {"value": "lisbon", ...}, beside a private<skill_id>:Locationcopy per receiving skill.Second commit: the private copies from the broadcast are gone. An in-process drive of the same two skills gives
intent_contextkeys['Location']only, where the reviewed head gave['Location', 'reader.skill:Location', 'setter.skill:Location']. The broadcast and the twoadd_contextmessages are still sent.On dev a shared context gate was never satisfied by this call, because ovos-spec-tools resolves a shared gate to the bare key only.
Test
The fix: context writes go to the session, never the bus; turns_remaining on set_context #605 unit tests on the dev source: 3 failed. On this branch: 89 passed.
test/unittestson this branch: 720 passed, 4 skipped. One file does not import, because ovoscope is not installed.The two listener tests now assert the §5.0 contract: the emitted compat message carries no resolved
key, andsession.intent_contextis untouched in the receiving skill.Missing cells (for the harness lane)
Every live cell needs re-running at
1e6e6af. A cell at722d3fameasures the behaviour the second commit changes.The end-of-handler event (SESSION-2 §2.6) that carries the shared key, and
test/end2end/test_context1_cross_skill_e2e.pywith ovoscope installed.Known residue, not closed here
add_contextis still emitted by each receiving skill, and a modern ovos-core folds the munged adapt key intosession.intent_contextin its own handler. That write predates this branch and is unchanged by it. Making the session the single writer for a pre-§5.0 core is core-side work, and it goes away with the 10.0.0 removal of these topics.🤖 Generated with Claude Code