feat(skills,tools): schema↔serde roundtrip tests, triggers field, bus subscriber, registry integrity#2718
Conversation
… subscriber, registry integrity - cron/add.rs: add 7 Schedule serde roundtrip tests covering Cron/At/Every variants, optional tz field, missing-kind and unknown-kind rejection, and schema required-field contract. Guards the serde↔schema alignment gap that caused the window_days silent-failure class of bugs. - skills/ops_types.rs: add `triggers: Vec<String>` field to SkillFrontmatter between allowed_tools and extra. Each entry is a trigger pattern of the form "domain" or "domain/event_slug"; bare domain matches any event in that domain. Documented with full cross-references to skills::bus. - skills/bus.rs: replace 17-line no-op stub with full implementation: TriggerPattern (parse + matches), TriggeredSkillIndex (build, is_empty, len, domains, matching_skills), TriggeredSkillSubscriber (EventHandler impl), register_triggered_skill_subscriber(), register_skill_cleanup_subscriber() no-op kept for backward compat. Includes 20+ unit tests. - tool_registry/ops.rs: add all_registry_entries_have_non_empty_name_and_description integrity test — asserts every registered tool has a non-blank name and description, catching silent metadata gaps before they reach the LLM. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a ChangesTriggered Skills Event Bus & Quality Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/openhuman/skills/bus.rs (1)
152-154: ⚡ Quick winAlign the handler name with the repo's bus naming convention.
The subscriber type already carries the
Subscribersuffix, so the runtime name should stay at the stable<domain>::<purpose>form rather thanskills::triggered_skill_subscriber.Suggested change
fn name(&self) -> &str { - "skills::triggered_skill_subscriber" + "skills::triggered_skill" }As per coding guidelines,
src/openhuman/**/bus.rs: Register domain event handlers in<domain>/bus.rswith<Purpose>Subscriberpattern andname()returning'<domain>::<purpose>'.🤖 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 `@src/openhuman/skills/bus.rs` around lines 152 - 154, The runtime name currently returns "skills::triggered_skill_subscriber" which duplicates the Subscriber suffix; update the name() implementation on the subscriber type (fn name(&self) -> &str) to return the stable domain-purpose form "skills::triggered_skill" so it follows the '<domain>::<purpose>' convention while the type keeps the 'Subscriber' suffix.
🤖 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 `@src/openhuman/skills/bus.rs`:
- Around line 64-73: The matches() method currently ignores self.event_slug
causing event-specific patterns to match entire domains; update matches (in the
matches function) to reject slugged patterns until a stable event slug API
exists by returning false when self.event_slug.is_some(), e.g. after the domain
check: if self.event_slug.is_some() { return false }, and add a TODO referencing
DomainEvent::slug() so we can replace the rejection with a proper slug
comparison once DomainEvent exposes slug().
- Around line 95-107: The current filter_map drops malformed trigger strings
silently when mapping frontmatter.triggers via TriggerPattern::parse; update the
closure in the iterator that builds (skill.name.clone(), patterns) so that any
parse failure emits a structured warning (including skill.name and the raw
trigger text) and/or appends a message to the skill's warnings list instead of
swallowing it; specifically, when iterating skill.frontmatter.triggers call
TriggerPattern::parse, on Err log a grep-friendly warning (with skill.name and
trigger) and continue, and only collect successful parses into patterns so
callers still get the successful patterns while malformed entries are visible
via logs or skill warnings.
In `@src/openhuman/tools/impl/cron/add.rs`:
- Around line 822-835: The test cron_add_tool_schema_requires_name_and_schedule
currently asserts a hardcoded schema; replace that by calling
CronAddTool::parameters_schema() to obtain the real schema and then inspect its
"required" array for "name" and "schedule". Specifically, in the test, invoke
CronAddTool::parameters_schema(), parse or index into its "required" field the
same way you did for the hardcoded schema, and assert that the required array
contains "name" and "schedule".
---
Nitpick comments:
In `@src/openhuman/skills/bus.rs`:
- Around line 152-154: The runtime name currently returns
"skills::triggered_skill_subscriber" which duplicates the Subscriber suffix;
update the name() implementation on the subscriber type (fn name(&self) -> &str)
to return the stable domain-purpose form "skills::triggered_skill" so it follows
the '<domain>::<purpose>' convention while the type keeps the 'Subscriber'
suffix.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 762521c2-159a-4900-a0b4-7a50eb2ce874
📒 Files selected for processing (4)
src/openhuman/skills/bus.rssrc/openhuman/skills/ops_types.rssrc/openhuman/tool_registry/ops.rssrc/openhuman/tools/impl/cron/add.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds event-triggered skills support — a solid improvement to the skills subsystem. The code is well-structured, documented, and tested. However, there's one critical gap that needs fixing.
Key Changes
- Bus subscriber — Complete implementation of
TriggeredSkillSubscriberwithTriggerPatternparsing and skill indexing - Frontmatter field — New
triggers: Vec<String>field inSkillFrontmatter - Tests — Comprehensive roundtrip tests for serde schemas and registry integrity
Issues
[MAJOR] Incomplete trigger pattern matching
The TriggerPattern struct supports patterns like "domain/event_slug" (e.g., "composio/trigger_received"), documented and parsed correctly. However, the matches() function only checks the domain and ignores the event_slug field entirely.
This means:
- Pattern
"composio/trigger_received"will match all composio events, not justtrigger_received - Users will get silent behavior mismatch — the pattern looks specific but isn't
The code has a TODO comment acknowledging slug matching is pending, but the API claims to support it (in struct docs, pattern examples, and tests). This creates a footgun.
Fix options:
- Implement slug matching using
DomainEventdiscriminant (preferred, but needs DomainEvent API changes) - Remove slug support — only accept bare domain patterns like
"composio"(simpler, but breaks the documented API) - Add a clear warning in the struct docs that slugs are ignored pending a future API addition
I'd go with option 1 — the hard way is the right way. If that's not feasible, option 2 + update the docs to reflect what actually works.
What's Good
- Well-documented — clear comments on trigger format, cross-references to modules, intent explanation
- Comprehensive tests — parsing edge cases, index building, skill matching, serde roundtrips all covered
- Good patterns — Arc for shared ownership, OnceLock usage hints in docs, sorted outputs for determinism
- Backward compat —
register_skill_cleanup_subscriber()kept as no-op,triggers:field defaults to empty - Registry integrity check — catches silent metadata gaps before they hit the LLM
No Other Issues
No lint warnings, no unwraps without messages, no PII in logs, no test coverage gaps. The serde roundtrip tests are exactly what #2252 needed.
| } | ||
|
|
||
| /// Returns true when this pattern matches the given event. | ||
| pub fn matches(&self, event: &DomainEvent) -> bool { |
There was a problem hiding this comment.
[major] This function only checks domain and ignores event_slug. A pattern like "composio/trigger_received" will match all composio events, not just trigger_received.
The struct docs and tests claim slug support works, but it doesn't (pending a stable slug() method on DomainEvent). This creates silent behavior mismatch.
Either: (1) implement slug matching properly once DomainEvent exposes a slug method, (2) remove slug support and only accept bare domains, or (3) clearly document that slug patterns are ignored for now.
- bus.rs: slug-qualified trigger patterns now return false from matches() instead of silently matching the entire domain; guards against over-firing until DomainEvent::slug() exists - bus.rs: malformed trigger strings now emit log::warn instead of being swallowed silently by filter_map - bus.rs: EventHandler::name() corrected to "skills::triggered_skill" (dropped redundant _subscriber suffix per naming convention) - bus.rs: add test slugged_pattern_rejected_until_slug_api_exists to pin the intentional rejection behaviour - add.rs: replace hardcoded JSON fixture in schema test with a real CronAddTool::parameters_schema() call so future field renames break the test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@zahica1234 unresolved review feedback — please address before we review. |
|
Unresolved review feedback from coderabbitai[bot] — please address before we review. |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Summary
Four well-crafted changes closing the schema↔serde alignment gap and bringing the event-trigger pipeline from stub to working.
Prior Finding Resolved
Your fix for the slug-matching issue is the right call: reject slug-qualified patterns until DomainEvent exposes slug(), with explicit documentation, a test that verifies the safe default behavior, and a TODO for the proper implementation. This prevents silent overfiring and makes the limitation crystal clear. Approved.
Review
bus.rs: Full implementation of TriggeredSkillIndex and TriggeredSkillSubscriber. Pattern parsing is solid, index building is clean (sorted for determinism), and matching logic is safe. Logging is appropriate. Public API is clear with good usage example.
ops_types.rs: triggers field properly placed, well-documented with cross-references to skills::bus, defaults sensibly.
tool_registry/ops.rs: Defensive integrity test catches empty name/description metadata gaps before reaching the LLM surface. Good catch.
cron/add.rs: Comprehensive serde roundtrip tests covering all Schedule variants, optional tz, missing/unknown kind, and schema requirements. Prevents the #2252 class of silent field-name mismatches.
Test coverage: 20+ new tests covering parse edge cases, domain/slug matching, index building/sorting, skill name determinism, and schema validation. Cargo check, fmt, test all pass. Coverage gate pass (80%+ on diff).
CI: All 30 checks green. No conflicts.
Breaking changes: None. Bare domain patterns work immediately; slug patterns are safely deferred. New API is opt-in, returns None when no triggers declared.
This is solid work — ready to merge.
3 participants
Summary
Four independent, focused improvements that collectively close the
parameters_schema()↔serdealignment gap and bring the skills event-trigger pipeline from stub to working.1. Schedule serde roundtrip tests (
tools/impl/cron/add.rs)The
CronAddTooldeserializes itsscheduleargument withserde_json::from_value::<Schedule>. No test previously verified that the JSON shapes documented inparameters_schema()actually deserialize correctly — the same class of silent mismatch that caused issue #2252 (window_days).Added 7 tests:
Schedulevariant (Cron,At,Every) deserializes from the schema-documented shapetzfield round-trips correctlykindfails with a clear errorkindvalue fails with a clear errorrequiredarray containsnameandschedule2.
triggers:field inSkillFrontmatter(skills/ops_types.rs)Added
pub triggers: Vec<String>betweenallowed_toolsandextra. Each entry is a trigger pattern of the form"domain"or"domain/event_slug"(e.g."composio","cron","channel/inbound_message"). A bare domain (no slash) matches any event in that domain. Fully documented with cross-references toskills::bus.3. Full
TriggeredSkillSubscriberimplementation (skills/bus.rs)Replaced the 17-line no-op stub with a complete implementation:
TriggerPattern— parses"domain"or"domain/event_slug"strings; normalises to lowercase;"domain/*"treated as bare domainTriggeredSkillIndex— built from&[Skill]at startup;matching_skills(&event)returns names of skills whose patterns match; entries sorted for deterministic loggingTriggeredSkillSubscriber—EventHandlerimpl that logs matched skills on eachDomainEvent; activation handoff left to the integration layer (no harness context needed here)register_triggered_skill_subscriber()— public API, returnsNonewhen no skills declare triggersregister_skill_cleanup_subscriber()— kept as a safe no-op for call-site backward compatIncludes 20+ unit tests covering parse edge cases, domain matching, index build/sort/dedup, and the matching logic.
4. Tool registry integrity test (
tool_registry/ops.rs)Added
all_registry_entries_have_non_empty_name_and_description— asserts every entry produced byregistry_entries()has a non-blanknameanddescription. Catches silent metadata gaps (e.g. an MCP tool with an empty description) before they reach the LLM tool surface.Testing
All new tests are in-module
#[cfg(test)]blocks and run withcargo test.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests & Validation