Use FALLBACK-1 skill-addressed ping and pong topics

[goldyfruit]

🤖 Auto-generated by Claude Fable 5 (claude-fable-5) via Claude Code — NOT human-reviewed. Verify before acting.

Fixes #464 by removing the shared fallback-pong channel that made request-level echoing necessary.

Architecture

• Register each fallback skill on <skill_id>.fallback.ping.
• Reply on the matching <skill_id>.fallback.pong topic.
• Remove those exact handlers during shutdown.

This is the skill-side half of OpenVoiceOS/ovos-core#808. It follows the FALLBACK-1 topic contract directly and keeps traffic owned by the fallback skill; it does not add request IDs, a compatibility bridge, or a generic agent hook.

Validation

• 11 focused fallback-skill tests pass.
• The companion Core fallback suite passes against this local package.
• Ruff passes.
• All GitHub checks and CodeRabbit pass.

Summary by CodeRabbit

• Bug Fixes
  • Improved fallback skill communication by supporting both skill-specific and broadcast ping/pong messages.
  • Ensured responses are routed to the correct destination, preventing addressed requests from generating unintended broadcast responses.
  • Enhanced asynchronous handling for more reliable fallback responses.
  • Preserved compatibility with existing broadcast-based fallback interactions.

---

Rework after adversarial audit (2026-08-12)

🤖 Auto-generated by Claude Fable 5 (claude-fable-5) via Claude Code — NOT human-reviewed. Verify before acting.

The flag-day defect. This PR bound ONLY the skill-addressed
<skill_id>.fallback.ping topic, dropping the broadcast
ovos.skills.fallback.ping that every released ovos-core (pre
ovos-core#808) still polls. Executed result: this branch + any released
ovos-core = fallback dead (the released core never gets its poll
answered). FALLBACK-1 §6.1 makes the addressed topics normative but
explicitly sanctions the broadcast poll as an observably-equivalent
optimisation, so a dual window is conformant.

The fix. Bind BOTH ping topics during the migration window
(kill-switch #837 conventions):

• Keep the new <skill_id>.fallback.ping binding.
• Re-add ovos.skills.fallback.ping, same handler.
• Reply on the topic family matching the ping received: a general ping
  gets a general ovos.skills.fallback.pong (the shape old cores
  understand); a skill-addressed ping gets a skill-addressed
  <skill_id>.fallback.pong.
• New unit tests: one per ping family asserting the correct pong topic +
  payload, and that a general ping does NOT produce a skill-addressed
  pong (and vice versa).

Executed matrix (probe-free harness, real ovoscope + real
ovos-skill-fallback-unknown, no fake ping/pong): fixed-core (#808) +
fixed-workshop (#465), fixed-core + released dev workshop, unfixed dev
core + fixed-workshop, and fixed-core + PyPI ovos-workshop==9.3.9a1 are
all GREEN (ping answered, dispatched exactly once, skill spoke exactly
once). Full ovos-workshop unit suite green (552 passed).

Verified claims: model re-check only (this session), not
human-reviewed -- matrix cells, red-before-fix on the dual-binding tests
(revert-code-keep-test technique, no git stash), and the full unit
suite were all actually executed in this session, not asserted from
memory.

Fixes #464 -> unchanged; this defect and its fix are local to the
skill-side ping/pong binding and don't affect the shared-pong-channel
removal #464 addresses.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b9adb7a2-8c1d-4723-aa44-0548bedfaf39

📥 Commits

Reviewing files that changed from the base of the PR and between a09f276 and 0dc6d46.

📒 Files selected for processing (2)

• ovos_workshop/skills/fallback.py
• test/unittests/skills/test_fallback_skill.py

---

📝 Walkthrough

Walkthrough

FallbackSkill now supports both skill-specific and legacy broadcast fallback pings. It routes each acknowledgement to the matching pong event and tests asynchronous handling, payloads, and lifecycle event names.

Changes

Fallback event registration and routing

Layer / File(s)	Summary
Register and route fallback events ovos_workshop/skills/fallback.py	FallbackSkill registers addressed and broadcast ping events. It emits the matching pong event after one can_answer check.
Validate fallback event routing test/unittests/skills/test_fallback_skill.py	Tests cover addressed and broadcast acknowledgements, payloads, asynchronous handling, single can_answer invocation, and fixed registration and deregistration events.

Estimated code review effort: 2 (Simple) | ~10 minutes

Mergeability Score: ⚪ Minimal · up to 0dc6d

The PR changes fallback topic routing and includes focused validation; no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs

• OpenVoiceOS/ovos-workshop#515: Both changes update FallbackSkill._handle_fallback_ack and its tests.

Suggested reviewers: jarbasal

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR does not implement [#464]'s requirement to echo fallback_request_id values in scoped pong responses.	Implement fallback_request_id echoing for scoped pings and preserve legacy behavior when no request ID is present, or update the linked issue and scope.
Out of Scope Changes check	⚠️ Warning	The PR mainly implements a FALLBACK-1 topic migration and compatibility handlers, which are outside the directly linked request-ID issue [#464].	Limit the changes to request-ID echoing, or link and document the issue that authorizes the FALLBACK-1 topic migration.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change to use FALLBACK-1 skill-addressed ping and pong topics.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

ovos_workshop/skills/fallback.py (1)

113-127: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Correctly implements request-id echo with legacy fallback preserved.

Logic matches the PR objective: echoes fallback_request_id from data or context into both data and context of the pong, and leaves the shape unchanged when absent.

One minor nit: using or to chain the lookups means a falsy-but-present fallback_request_id (e.g., empty string) in data would silently fall through to context. Unlikely in practice given request ids are non-empty strings, but message.data.get(...) if ... is not None else message.context.get(...) would be more precise if that ever becomes a concern.

🤖 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_workshop/skills/fallback.py` around lines 113 - 127, The request-id echo
logic in the fallback pong should preserve a falsy-but-present
fallback_request_id from message.data instead of falling through to
message.context. Update the lookup in the fallback handler that builds data and
context so it distinguishes “missing” from “present but empty,” while keeping
the existing behavior of copying the ID into both payloads in the fallback
response emitted by the fallback method.

🤖 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.

Nitpick comments:
In `@ovos_workshop/skills/fallback.py`:
- Around line 113-127: The request-id echo logic in the fallback pong should
preserve a falsy-but-present fallback_request_id from message.data instead of
falling through to message.context. Update the lookup in the fallback handler
that builds data and context so it distinguishes “missing” from “present but
empty,” while keeping the existing behavior of copying the ID into both payloads
in the fallback response emitted by the fallback method.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e13afc0a-7332-43a9-8f1b-a172b7bbd015

📥 Commits

Reviewing files that changed from the base of the PR and between 72993f3 and e67ec8c.

📒 Files selected for processing (2)

• ovos_workshop/skills/fallback.py
• test/unittests/test_fallback_skill.py

[JarbasAl]

This is the correct skill-side counterpart to ovos-core#808: echo fallback_request_id back into the pong's data+context only when present, so legacy pings still produce the identical old pong shape. The pair is order-independent and mutually back-compatible (old skill+new core → no id → core tolerates; new skill+old core → legacy shape). Using if "fallback_request_id" in message.data membership (not or-chaining) correctly handles the falsy empty-string id — and it's tested.

The red CI is not your change: the failing build_tests breakage is an unrelated test_intent_layers_e2e regression that was dev-wide at the time and is now green on dev; your new fallback tests pass. Re-run to clear. Same non-blocking spec note as #808 (FALLBACK-1 §6.1 dotted topics are the longer-term direction). Looks mergeable alongside #808.

---

Review written by Claude Opus 4.8 (claude-opus-4-8) without human oversight.

[JarbasAl]

Correction to my earlier review — reject as-is, same spec-compliance reason as ovos-core#808.

Echoing fallback_request_id extends the legacy broadcast pong, but that field isn't in FALLBACK-1 §6.1, which prescribes per-skill dotted topics (<skill_id>.fallback.ping/pong). Dotted addressing is inherently request-scoped, so the id echo is unnecessary under the spec-compliant design. The architecture spec is prescriptive; the correct skill-side change is to respond on the skill's own dotted .fallback.pong topic (payload skill_id + can_handle), paired with the core migration. Recommend reworking toward §6.1 rather than merging the id-echo approach.

---

Review written by Claude Opus 4.8 (claude-opus-4-8) without human oversight.

[coderabbitai]

🧹 Nitpick comments (1)

test/unittests/skills/test_fallback_skill.py (1)

66-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise the new ping route through FakeBus.

The registration test checks only the event name, and the acknowledgment test calls _handle_fallback_ack directly. Emit a <skill_id>.fallback.ping message and assert that the <skill_id>.fallback.pong listener receives the response. This checks the changed registration and reply path together.

Also applies to: 81-82

🤖 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/skills/test_fallback_skill.py` at line 66, Extend the fallback
skill tests to exercise the ping route through FakeBus: publish a
<skill_id>.fallback.ping message, then assert that the registered
<skill_id>.fallback.pong listener receives the response. Update the relevant
registration and acknowledgment tests without relying on a direct
_handle_fallback_ack call, so the changed registration and reply path are tested
together.

🤖 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.

Nitpick comments:
In `@test/unittests/skills/test_fallback_skill.py`:
- Line 66: Extend the fallback skill tests to exercise the ping route through
FakeBus: publish a <skill_id>.fallback.ping message, then assert that the
registered <skill_id>.fallback.pong listener receives the response. Update the
relevant registration and acknowledgment tests without relying on a direct
_handle_fallback_ack call, so the changed registration and reply path are tested
together.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1d7cfdc8-8170-45cb-a5d9-75f1195fd58c

📥 Commits

Reviewing files that changed from the base of the PR and between e67ec8c and 427607d.

📒 Files selected for processing (2)

• ovos_workshop/skills/fallback.py
• test/unittests/skills/test_fallback_skill.py

[JarbasAl]

🤖 Auto-generated by Claude Fable 5 (claude-fable-5) via Claude Code — NOT human-reviewed. Verify before acting.

Closed by owner ruling (2026-08-13): this pair predates the ratified FALLBACK-1 broadcast-contest shape. Fallback will be reworked on the broadcast pattern established by ovos-core#863 + ovos-workshop#534 (one broadcast question per round, parallel answers, explicit declines, early close) once that pair lands — one migration instead of two. Nothing is broken by the closure: the pair was never merged.