feat(skills): catalog-driven run_skill flow + mid-session skill refresh#3722
Conversation
Surface installed skills via the compact `## Installed Skills` catalogue and run them in an isolated worker, instead of the brittle per-turn SKILL.md body injection that polluted chat history. - Orchestrator catalogue steers to `run_skill` (isolated `skill_executor` worker) with handoff-plan guidance, not `run_workflow`. - `skill_executor` does what it can with its bounded tools and emits a `## Handoff Plan` for steps needing tools it lacks (integrations, memory); the orchestrator completes those under the approval gate. - Remove the keyword-matched SKILL.md body injection (left full skill text permanently in chat history); delete the now-unused `workflows::inject` module. Discovery is the compact catalogue; execution is via `run_skill`. - Mid-session skill refresh, mirroring the integration/MCP pattern: a new `DomainEvent::WorkflowsChanged` (install/uninstall/create) drives `refresh_workflows` at the turn boundary plus a one-shot `[skills update]` user-turn announcement, so a skill installed mid-conversation appears in the catalogue and is runnable without a restart. Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces the per-turn SKILL.md body injection model with an event-driven live-refresh mechanism. Workflow create/install/uninstall operations now publish a ChangesLive Skill Refresh and inject.rs Removal
Sequence Diagram(s)sequenceDiagram
participant User
participant AgentTurn as Agent::turn()
participant EventBus as DomainEvent Bus
participant WorkflowOps as ops_install / ops_create
participant WorkflowStore as Disk / Workflow Metadata
WorkflowOps->>EventBus: publish WorkflowsChanged { reason }
User->>AgentTurn: next user message
AgentTurn->>AgentTurn: ensure_skill_events_listener()
AgentTurn->>EventBus: drain_skill_events() → changed=true
AgentTurn->>WorkflowStore: refresh_workflows("event") → load metadata
WorkflowStore-->>AgentTurn: updated workflow list
AgentTurn->>AgentTurn: enqueue new IDs → pending_skill_announcement
AgentTurn->>AgentTurn: skill_announcement_note(pending_skills) → prepend to user content
AgentTurn-->>User: turn executes with refreshed skill catalogue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
feature
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b358dcaea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| log::info!( | ||
| "[agent_loop] installed-skills set changed ({trigger}): {} -> {} skills; refreshing catalogue (system prompt section rebuilds next turn)", | ||
| self.workflows.len(), | ||
| latest.len() | ||
| ); | ||
| self.workflows = latest; |
There was a problem hiding this comment.
When a workflow change is observed after the first turn, this only replaces self.workflows; it does not update the already-stored system message that contains ## Installed Skills. In the inspected Agent::turn flow, the system prompt is pushed only when history is empty and later turns deliberately reuse it, so uninstalling a skill leaves it advertised to the model and installing/updating one does not actually refresh the catalog the comment/log claim is being rebuilt. This can make live sessions call removed skills or miss changed skills until a new session starts; the refreshed catalog needs to be surfaced through the stored prompt or a per-turn injected catalog/removal note.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/agent/harness/session/turn/core.rs`:
- Around line 174-184: The skills event listener is not initialized early enough
in the turn lifecycle, causing `WorkflowsChanged` events to be missed before the
first `drain_skill_events()` call at line 182. Arm the skills event listener at
the beginning of turn initialization (next to where the composio listener is set
up) rather than conditionally in the drain call, so that the listener is
subscribed and ready to capture workflow changes from the start of the session.
This ensures that mid-session skill installs/uninstalls are reliably detected
even on the first turn when `drain_skill_events()` is first invoked.
In `@src/openhuman/agent/harness/session/turn/tools.rs`:
- Around line 249-305: Add comprehensive unit tests for the new stateful session
behavior introduced by the functions `ensure_skill_events_listener`,
`drain_skill_events`, and `refresh_workflows`. Create tests that cover lag
handling (TryRecvError::Lagged case), closed receiver handling
(TryRecvError::Closed case), no-op set comparisons when no changes occur, and
one-shot announcement queuing behavior. Follow the coding guidelines by
implementing these tests either as inline tests using #[cfg(test)] mod tests or
in a sibling *_tests.rs file.
In `@src/openhuman/skill_runtime/agent/skill_executor/prompt.md`:
- Line 35: The fenced code block in the prompt.md file is missing a language
specification, which violates markdownlint rule MD040. Add the language tag
`text` immediately after the opening triple backticks (``` becomes ```text) on
the line before the "## Completed" section to properly specify the code block
language.
In `@src/openhuman/workflows/ops_create.rs`:
- Around line 386-392: Add a regression test to verify that the
`create_workflow_inner` function publishes a `WorkflowsChanged` event with
reason "create" when a workflow is successfully created. Create the test either
inline in `src/openhuman/workflows/ops_create.rs` using a `#[cfg(test)] mod
tests` block or in a sibling `ops_create_tests.rs` file following the project's
coding guidelines. The test should mock or verify the `publish_global` call to
ensure the `DomainEvent::WorkflowsChanged` event is emitted during workflow
creation, preventing regression of this live-refresh behavior.
🪄 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: 02e5f62b-6476-44b1-ae21-44e4b69003c5
📒 Files selected for processing (14)
src/core/event_bus/events.rssrc/openhuman/agent/harness/session/builder/setters.rssrc/openhuman/agent/harness/session/turn/core.rssrc/openhuman/agent/harness/session/turn/mod.rssrc/openhuman/agent/harness/session/turn/tools.rssrc/openhuman/agent/harness/session/types.rssrc/openhuman/agent_registry/agents/orchestrator/prompt.mdsrc/openhuman/agent_registry/agents/orchestrator/prompt.rssrc/openhuman/skill_runtime/agent/skill_executor/prompt.mdsrc/openhuman/workflows/README.mdsrc/openhuman/workflows/inject.rssrc/openhuman/workflows/mod.rssrc/openhuman/workflows/ops_create.rssrc/openhuman/workflows/ops_install.rs
💤 Files with no reviewable changes (1)
- src/openhuman/workflows/inject.rs
| /// Lazily attach this session to the global event bus so it can observe | ||
| /// [`crate::core::event_bus::DomainEvent::WorkflowsChanged`] (skill | ||
| /// install / uninstall / create). Mirror of | ||
| /// [`Self::ensure_composio_integrations_listener`]. | ||
| pub(in super::super) fn ensure_skill_events_listener(&mut self) { | ||
| if self.skill_events_rx.is_some() { | ||
| return; | ||
| } | ||
| if let Some(bus) = crate::core::event_bus::global() { | ||
| self.skill_events_rx = Some(bus.raw_receiver()); | ||
| log::debug!( | ||
| "[agent_loop] armed installed-skills listener for session='{}'", | ||
| self.event_session_id | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// Drain pending [`crate::core::event_bus::DomainEvent::WorkflowsChanged`] | ||
| /// events. Returns `true` when at least one was observed (or the listener | ||
| /// lagged) and the caller should re-scan the installed skill set via | ||
| /// [`Self::refresh_workflows`]. Mirror of | ||
| /// [`Self::drain_composio_integrations_changed_events`]. | ||
| pub(in super::super) fn drain_skill_events(&mut self) -> bool { | ||
| self.ensure_skill_events_listener(); | ||
| let Some(rx) = self.skill_events_rx.as_mut() else { | ||
| return false; | ||
| }; | ||
| use tokio::sync::broadcast::error::TryRecvError; | ||
|
|
||
| let mut saw_signal = false; | ||
| let mut closed = false; | ||
| loop { | ||
| match rx.try_recv() { | ||
| Ok(crate::core::event_bus::DomainEvent::WorkflowsChanged { reason }) => { | ||
| saw_signal = true; | ||
| log::info!("[agent_loop] received installed-skills changed event ({reason})"); | ||
| } | ||
| Ok(_) => {} | ||
| Err(TryRecvError::Empty) => break, | ||
| Err(TryRecvError::Lagged(skipped)) => { | ||
| saw_signal = true; | ||
| log::warn!( | ||
| "[agent_loop] installed-skills listener lagged by {} event(s); forcing catalogue re-check", | ||
| skipped | ||
| ); | ||
| } | ||
| Err(TryRecvError::Closed) => { | ||
| closed = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if closed { | ||
| self.skill_events_rx = None; | ||
| } | ||
| saw_signal | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add focused tests for skill-event drain + workflow-refresh behavior.
These new flows (ensure_skill_events_listener, drain_skill_events, refresh_workflows) introduce stateful session behavior but ship without targeted tests for lag/closed handling, no-op set comparisons, and one-shot announcement queuing.
As per coding guidelines, “Write Rust tests inline with #[cfg(test)] mod tests or in sibling *_tests.rs files when new/changed behavior is added.”
Also applies to: 360-407
🤖 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/agent/harness/session/turn/tools.rs` around lines 249 - 305,
Add comprehensive unit tests for the new stateful session behavior introduced by
the functions `ensure_skill_events_listener`, `drain_skill_events`, and
`refresh_workflows`. Create tests that cover lag handling (TryRecvError::Lagged
case), closed receiver handling (TryRecvError::Closed case), no-op set
comparisons when no changes occur, and one-shot announcement queuing behavior.
Follow the coding guidelines by implementing these tests either as inline tests
using #[cfg(test)] mod tests or in a sibling *_tests.rs file.
Source: Coding guidelines
…resh - render_installed_skills: lists skills + steers to run_skill; empty omitted - skill_announcement_note: empty -> None; non-empty mentions ids + run_skill - refresh_workflows: discovers an on-disk skill, parks it for announcement, idempotent - drain_skill_events / ensure_skill_events_listener: WorkflowsChanged drains once - DomainEvent::WorkflowsChanged: domain() = workflow, variant_name() = WorkflowsChanged Co-Authored-By: Claude <noreply@anthropic.com>
…-field access) Keeps session/tests.rs to its public-surface-only convention. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…atalogue docs Addresses CodeRabbit review on tinyhumansai#3722: - Arm ensure_skill_events_listener() at turn start (next to the composio listener) so WorkflowsChanged events during turn 1 aren't missed before the first drain (broadcast subscriptions are not retroactive). - Correct the comment/log/doc that claimed the system-prompt '## Installed Skills' block is rebuilt each turn. It is frozen at turn 1 for KV-cache stability; the user-turn announcement is what surfaces a mid-session install to the live session (mirroring the MCP mechanism). refresh_workflows updates the tracked set for the next diff + a fresh session's catalogue. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks @coderabbitai — addressed in the latest commits: 1. Arm the skills event listener before first-turn steady state (core.rs) — ✅ Fixed. 2. Refresh the prompt-visible skills catalog (tools.rs) — Good catch on the inaccurate wording; corrected the comment/log/doc. You're right that the system-prompt 3. Add focused tests for skill-event drain + workflow-refresh (tools.rs) — ✅ Already added in a later commit (your review predated it): |
|
Tip For best results, initiate chat on the files or code changes.
On the removal retraction limitation you flagged — would you like me to open a follow-up issue tracking "emit a (ᵔ ᵕ ᵔ)🐇 ✏️ Learnings added
|
|
@coderabbitai review All review comments have been addressed in the latest commits (listener arming at turn start, corrected the frozen-catalogue docs/log, and the requested tests were added). CI is fully green (incl. Coverage Gate ≥ 80%). Please re-review to clear the stale changes-requested verdict if everything looks good. |
|
(ᵔ ᵕ ᵔ)🐇 ✅ Action performedFull review finished. |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Review — PR #3722
Net-negative simplification (~-453 LOC): drops the brittle per-turn SKILL.md body injection (workflows::inject, 830 LOC, polluted chat history permanently) for a compact ## Installed Skills catalogue + run_skill delegate in an isolated worker, plus a WorkflowsChanged mid-session refresh mirroring the composio/MCP announcement mechanism. Well-documented, sound design.
Note: the carried-over P1 listener-arming race is already FIXED in head — core.rs arms ensure_skill_events_listener unconditionally before the first-turn branch, so a turn-1 WorkflowsChanged is buffered and drained. Not a remaining blocker. Verified clean: dangling callers of deleted workflows::inject = NONE.
Requesting changes on the items below.
Major
- M1 — emission wiring untested. The 3 new
publish_global(WorkflowsChanged)calls (ops_create.rs:392,ops_install.rs:342+~516) are the trigger half of the feature and nothing asserts they fire. Therefresh_workflowstest writes a skill straight to disk and bypasses create/install — deleting any emission line stays green while silently killing mid-session refresh. Add one regression test drivingcreate_workflow_inner(cheapest, no network) assertingWorkflowsChangedlands on a bus receiver. - M2 —
drain_skill_eventslag/closed arms untested (tools.rs:344-378). Only the happyOk+Emptypath is covered.Lagged(returns true, forces re-check) andClosed(nulls the rx) carry real behaviour — a missedLagged→truedrops a refresh under load. Cover by flooding >64 events into the bounded bus to force lag.
Minor
- m1 — MD040 (
skill_executor/prompt.md:~35): the## Handoff Planfenced block opens bare; add a language tag (e.g.```text). - m2 — uninstall has no symmetric catalogue retraction. Install is surfaced via the
[skills update]note; uninstall is not, so an uninstalled skill stays advertised mid-session andrun_skillagainst it fails at the worker. Acceptable for this PR, but track it (don't leave it as a PR comment).
Questions
- No linked tracking issue — per workflow norms, file one for this replacement (and the m2 uninstall-retraction follow-up) before merge.
… (review) Addresses oxoxDev review on tinyhumansai#3722: - M1: create_workflow_inner_emits_workflows_changed asserts the publish_global emission fires (the refresh_workflows test wrote to disk directly and bypassed create/install, so a dropped emission would have stayed green). - M2: skill_listener_treats_lag_as_signal (floods 256 events past the 64-slot bus -> Lagged returns true) and skill_listener_closed_channel_nulls_rx_and_is_ not_a_signal (injected dropped-sender receiver -> Closed returns false + nulls rx), via two #[cfg(test)] helpers. - m1 (MD040): add 'text' language tag to the Handoff Plan fence. - m2: note the uninstall-retraction gap in code, tracked in tinyhumansai#3738 (filed). Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks @oxoxDev — all items addressed in M1 — emission wiring untested ✅ Added M2 —
m1 — MD040 ✅ Added a m2 — uninstall has no symmetric retraction ✅ Tracked, not left as a comment: filed #3738 and referenced it in a code comment at the Question — linked tracking issue ✅ #3738 covers the uninstall-retraction follow-up. (Happy to also file a broader tracking issue for the injection→catalogue replacement itself if you'd prefer one separate from the PR.) All 4 new tests pass locally; pushing the branch now triggers CI. |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Re-review of 186814456 — addresses the prior REQUEST_CHANGES. Majors resolved; approving.
M1 (emission untested) — mostly resolved.
create_workflow_inner_emits_workflows_changed(ops_create.rs:687) callscreate_workflow_innerdirectly and assertsWorkflowsChanged{reason:"create"}lands on the bus — dropping the emit (ops_create.rs:388) now goes red. Real coverage, not a disk-write bypass.- Residual (non-blocking): the
install(ops_install.rs:347) anduninstall(ops_install.rs:521) emits are still droppable-green — no test goes through those two paths. The consumer side is now fully covered so the wiring is exercised end-to-end; worth a symmetric pin when those async paths get a test harness.
M2 (drain lag/closed arms) — resolved.
skill_listener_treats_lag_as_signal(tests.rs) floods 256 past the 64-slot bus → forcesLagged→ asserts drain returns true. Pins lag-is-signal.skill_listener_closed_channel_nulls_rx_and_is_not_a_signalinjects a closed rx viaset_skill_events_rx_for_test, asserts drain returns false AND rx nulled so the next drain re-arms. Covers theClosedarm + null. Both#[cfg(test)]helpers are clean.
m1 (MD040) — fixed. prompt.md fence is now ```text.
m2 (uninstall retraction) — tracked. tools.rs comment documents that removals aren't retracted yet and links #3738 (open, matching title). Doubles as the tracking issue the prior round flagged as missing.
Nice work on the closed-channel injection helper — that arm is awkward to reach without it.
|
@coderabbitai resolve All three original concerns are addressed (you confirmed this), the human maintainer @oxoxDev has approved, and CI is fully green. Please resolve your review threads so the standing changes-requested verdict clears for merge. |
|
@coderabbitai review |
2 participants
Summary
## Installed Skillscatalogue and run them in an isolatedskill_executorworker viarun_skill— instead of the brittle per-turnSKILL.mdbody injection.skill_executordoes what its bounded tools allow and emits a## Handoff Planfor steps it can't perform (integrations, memory); the orchestrator completes those under the approval gate.SKILL.mdinjection (it baked full skill text permanently into chat history) and delete the now-unusedworkflows::injectmodule.DomainEvent::WorkflowsChanged(install/uninstall/create) drivesrefresh_workflowsat the turn boundary + a one-shot[skills update]user-turn note — mirroring the existing integration/MCP mid-session pattern (upstream/main added the identical mechanism for MCP servers in parallel).Problem
Skills were surfaced two ways, both flawed:
SKILL.mdbody onto the user turn and stored it — so full skill text lingered permanently in chat history (microcompact only clears tool results, not user messages), and it fired unintentionally on incidental wording.run_workflow(the heavy autonomous orchestrator run) rather than the bounded, isolatedskill_executor.Separately,
## Installed Skillsis a session-build snapshot: a skill installed mid-conversation didn't appear until restart.Solution
run_skill→skill_executor(isolated worker).skill_executorlacks, it returns a## Handoff Plan; the orchestrator executes those steps under the approval gate.## Installed Skillsblock is frozen at turn 1 for KV-cache stability, so — like the MCP mechanism — the user-turn[skills update]announcement surfaces a mid-session install to the live session;run_skillloads/runs it fresh from disk. Event-gated byWorkflowsChangedso the hot path does no extra disk scans.Validated end-to-end on the managed
openhumanprovider (live app): catalogue →run_skill→skill_executor(loaded SKILL.md, resolved the managed Python runtime, ran a shell step under the approval gate) → real result. Also exercised the capability-gap path with a Gmail skill (google-workspace):skill_executorreturned a## Handoff Plan, and the orchestrator completed the read viaintegrations_agent→GMAIL_FETCH_EMAILS.Submission Checklist
render_installed_skills(lists + steers torun_skill; empty omitted),skill_announcement_note(empty→None; mentions ids +run_skill),refresh_workflows(discovers an on-disk skill, parks it, idempotent),drain_skill_events/ensure_skill_events_listener(drainsWorkflowsChangedonce),DomainEvent::WorkflowsChanged(domain/variant_name). All passing locally.run_skillsteering,refresh_workflows, the event drain, the announcement note, and the new event variant; field/init lines are exercised via agent build + the existing turn tests.N/A: behaviour-only change to existing agent-harness internals; no new feature row.## Related—N/A: no matrix rows touched.N/A: no release-cut surface changed (agent-harness internals + prompts).N/A: no tracking issue.Impact
SKILL.mdin user turns). No UI/schema changes. Note: theclaude-codeinference provider still can't run skills due to a separate staleDISALLOWED_CC_BUILTINSlist (out of scope); the managed provider works end-to-end.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes