feat: skills worker#70
Merged
sergiofilhowz merged 1 commit intomainfrom May 4, 2026
Merged
Conversation
📝 WalkthroughWalkthroughIntroduces a new ChangesSkills Registry Worker
Sequence Diagram(s)sequenceDiagram
actor User
participant Client
participant Skills Worker
participant State Service
participant Subscriber Function
User->>Client: Call skills::register {id, skill}
Client->>Skills Worker: TriggerRequest
Skills Worker->>Skills Worker: Validate id & body
Skills Worker->>State Service: state::set(scope="skills", key=id, value=StoredSkill)
State Service-->>Skills Worker: OK
Skills Worker->>Skills Worker: Dispatch skills::on-change {op:"register", id}
Skills Worker-->>Skills Worker: Trigger subscribers async
Subscriber Function->>Skills Worker: Receives on-change event
Subscriber Function->>Subscriber Function: Handle change notification
Skills Worker-->>Client: {id, registered_at}
Client-->>User: Success
sequenceDiagram
actor User
participant Client
participant Skills Worker
participant State Service
participant URI Resolver
User->>Client: Call skills::resources-read {uri: "iii://brain/brain::summarize"}
Client->>Skills Worker: TriggerRequest
Skills Worker->>Skills Worker: Parse URI to Skill(brain) + Section{skill_id,function_id}
Skills Worker->>State Service: state::get(scope="skills", key="brain")
State Service-->>Skills Worker: StoredSkill {body: markdown}
Skills Worker->>Skills Worker: Guard against is_always_hidden("brain::summarize")
Skills Worker->>URI Resolver: Trigger "brain::summarize" with {}
URI Resolver-->>Skills Worker: {content: "summary text"}
Skills Worker->>Skills Worker: Normalize to (text, mime) = ("summary text", "text/markdown")
Skills Worker-->>Client: {contents: [{mimeType: "text/markdown", text: "summary text"}]}
Client-->>User: Resource content
sequenceDiagram
actor User
participant Client
participant Prompts Worker
participant State Service
participant Handler Function
User->>Client: Call prompts::mcp-get {name: "email-draft", arguments: {to: "alice@example"}}
Client->>Prompts Worker: TriggerRequest
Prompts Worker->>State Service: state::get(scope="prompts", key="email-draft")
State Service-->>Prompts Worker: StoredPrompt {function_id: "user-handler-1"}
Prompts Worker->>Prompts Worker: Guard against is_always_hidden("user-handler-1")
Prompts Worker->>Handler Function: Trigger "user-handler-1" {to: "alice@example"}
Handler Function-->>Prompts Worker: {messages: [{role: "assistant", content: {type: "text", text: "Draft..."}}]}
Prompts Worker->>Prompts Worker: Normalize output to MCP shape
Prompts Worker-->>Client: {description: "...", messages: [...]}
Client-->>User: Prompt response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
13-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the
skillsworker to the Modules table.This PR introduces the
skillsworker but doesn't add it to the## Modulestable, leaving the registry incomplete and the worker undiscoverable from the top-level README.📝 Proposed addition
| [`mcp`](mcp/) | Rust | Model Context Protocol surface — stdio + HTTP JSON-RPC, exposes iii functions tagged `mcp.expose` as MCP tools. | +| [`skills`](skills/) | Rust | Agentic content registry — skills + prompts + `iii://` resource resolver bridged to the `mcp` worker. | | [`proof`](proof/) | Node | AI-driven browser testing — diffs changes, generates test plans, drives Playwright. |🤖 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 `@README.md` around lines 13 - 21, Add a new row for the skills worker to the "## Modules" Markdown table in README.md so the module registry is complete and discoverable: insert an entry with the display name `skills` (folder `skills/`) and a short summary like "Node | Skills worker providing reusable action primitives" (or your preferred concise description) into the existing table alongside other workers such as `iii-lsp`, `image-resize`, `mcp`, etc.; ensure the row follows the same pipe-delimited table format and style used by the other entries so rendering and ordering remain consistent.
🧹 Nitpick comments (5)
skills/src/manifest.rs (1)
21-27: ⚡ Quick win
default_configduplicatesSkillsConfig::default()— two sources of truth.The hardcoded
serde_json::json!({ "scopes": ..., "state_timeout_ms": 10_000 })is a manual copy ofSkillsConfig's defaults. If the defaults inconfig.rschange,manifest.rssilently diverges.♻️ Proposed fix: derive from the canonical defaults
+use crate::config::SkillsConfig; + pub fn build_manifest() -> ModuleManifest { ModuleManifest { // ... - default_config: serde_json::json!({ - "scopes": { - "skills": "skills", - "prompts": "prompts" - }, - "state_timeout_ms": 10_000 - }), + default_config: serde_json::to_value(SkillsConfig::default()) + .expect("SkillsConfig::default() is always serializable"), // ... } }🤖 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 `@skills/src/manifest.rs` around lines 21 - 27, The hardcoded default_config JSON duplicates SkillsConfig::default(); replace the literal with a canonical serialization of the config default so there is a single source of truth: call serde_json::to_value(SkillsConfig::default()) (or equivalent) to produce the Value for default_config in manifest.rs, and add the necessary use/import for SkillsConfig and serde_json::to_value so the manifest now derives its defaults from SkillsConfig::default() instead of the manual serde_json! literal.skills/tests/bdd.rs (1)
33-37: 💤 Low valueVerify
shared()is alwaysSomewhenget_or_init()returnsSome.
world.cfgis only populated whencommon::workers::shared()returnsSome. If there is any initialization path where the engine connection succeeds but the shared worker state isn't set (e.g., a registration failure insideget_or_init),world.cfgsilently stays at its default and scenarios that rely on it will fail with an opaque error rather than a clear "workers not initialized" message.Consider asserting or logging when
shared()returnsNoneinside this branch:if let Some(iii) = common::engine::get_or_init().await { world.iii = Some(iii.clone()); if let Some(shared) = common::workers::shared() { world.cfg = shared.cfg.clone(); + } else { + tracing::warn!("engine available but shared worker state not initialised — cfg will be default"); }🤖 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 `@skills/tests/bdd.rs` around lines 33 - 37, When get_or_init() returns Some(iii) ensure common::workers::shared() is also present instead of silently leaving world.cfg default; in the branch handling the successful engine init (the block that sets world.iii from common::engine::get_or_init()), call common::workers::shared() and if it returns None either assert/panic or log an explicit error and fail the test so failures surface as "workers not initialized" rather than causing opaque downstream errors; reference the symbols common::engine::get_or_init, common::workers::shared, world.iii and world.cfg when making the change.skills/tests/common/workers.rs (1)
34-53: 💤 Low valueConsider
get_or_try_initto makeregister_allrace-free.The current
get()→ work →set()pattern ontokio::sync::OnceCellhas a TOCTOU window: two concurrent callers can both seeNone, both calltrigger_types::register_allandfunctions::register_all(double-registering every function), and then race onset()— with the loser silently returning a differentArc<Shared>. In sequential BDD execution this is harmless, but usingget_or_try_initeliminates the gap cleanly:♻️ Proposed refactor
-pub async fn register_all(iii: &Arc<III>) -> Result<Arc<Shared>> { - if let Some(s) = SHARED.get() { - return Ok(s.clone()); - } - - let cfg = Arc::new(SkillsConfig::default()); - let registered = trigger_types::register_all(iii); - functions::register_all(iii, &cfg, ®istered); - - tokio::time::sleep(std::time::Duration::from_millis(150)).await; - - let shared = Arc::new(Shared { - cfg, - triggers: Arc::new(registered), - }); - let _ = SHARED.set(shared.clone()); - Ok(shared) -} +pub async fn register_all(iii: &Arc<III>) -> Result<Arc<Shared>> { + let iii = iii.clone(); + SHARED + .get_or_try_init(|| async move { + let cfg = Arc::new(SkillsConfig::default()); + let registered = trigger_types::register_all(&iii); + functions::register_all(&iii, &cfg, ®istered); + tokio::time::sleep(std::time::Duration::from_millis(150)).await; + Ok(Arc::new(Shared { + cfg, + triggers: Arc::new(registered), + })) + }) + .await + .cloned() +}🤖 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 `@skills/tests/common/workers.rs` around lines 34 - 53, The register_all function currently races by calling SHARED.get() then doing work and SHARED.set(), so replace that pattern with SHARED.get_or_try_init(...) to perform the registration atomically: move creation of cfg, call trigger_types::register_all and functions::register_all, the tokio::time::sleep, and construction of Arc<Shared> into the async init closure passed to SHARED.get_or_try_init so only one initializer runs; then return the cloned Arc from the get_or_try_init result. Update the function signature and error propagation as needed to use the Result returned by get_or_try_init and remove the manual SHARED.set/get usage.skills/src/functions/prompts.rs (1)
294-294: 💤 Low valueOption comparison in sort may have unexpected behavior.
a["name"].as_str().cmp(&b["name"].as_str())comparesOption<&str>values. While this works (None < Some), if a malformed entry somehow lacks anamefield, it would sort to the front rather than causing an obvious error.Since entries are constructed from
StoredPromptwith a requiredname: Stringfield, this is likely safe in practice, but consider using.expect()or explicit handling for defensive coding.♻️ More explicit sort key extraction
- prompts.sort_by(|a, b| a["name"].as_str().cmp(&b["name"].as_str())); + prompts.sort_by(|a, b| { + let a_name = a["name"].as_str().unwrap_or(""); + let b_name = b["name"].as_str().unwrap_or(""); + a_name.cmp(b_name) + });🤖 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 `@skills/src/functions/prompts.rs` at line 294, The sort currently uses prompts.sort_by(|a, b| a["name"].as_str().cmp(&b["name"].as_str())), which compares Option<&str> and can silently place missing names first; change to explicitly extract the string key (e.g., via a["name"].as_str().expect("prompt missing name") or by mapping to a required &str and handling errors) before calling sort_by to ensure a missing or malformed `name` panics or is handled deterministically — update the call site where `prompts` is sorted in prompts.rs to use the explicit extraction (expect or explicit match) so the comparator works on &str not Option<&str>.skills/tests/steps/notifications.rs (1)
102-126: 💤 Low valueObserver index retrieval may race under parallel execution.
The observer is pushed to the global
OBSERVERSvec, then the index is retrieved aslen() - 1. If scenarios run concurrently, another scenario could push an observer between these two operations, causing the stored index to point to the wrong observer.If parallel scenario execution is intended in the future, consider returning the index directly from
new_observerorregister_observerinstead of computing it separately.♻️ Suggested fix to return index from new_observer
-fn new_observer(_world: &IiiSkillsWorld) -> Arc<Observer> { +fn new_observer(_world: &IiiSkillsWorld) -> (Arc<Observer>, usize) { let o = Arc::new(Observer { counter: Arc::new(Mutex::new(0)), }); - observers().lock().unwrap().push(o.clone()); - o + let mut vec = observers().lock().unwrap(); + let idx = vec.len(); + vec.push(o.clone()); + (o, idx) }🤖 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 `@skills/tests/steps/notifications.rs` around lines 102 - 126, The current subscribe_skills and subscribe_prompts compute the observer index with observers().lock().unwrap().len() - 1 which races if other scenarios push into OBSERVERS; change register_observer (or new_observer) to return the created observer's index (or return a handle containing the index) and update subscribe_skills and subscribe_prompts to use that returned idx when storing "notifications_obs_idx" (avoid computing len()-1), or alternatively ensure register_observer itself locks OBSERVERS, pushes the observer and returns the index atomically so callers don't need to call observers().lock() themselves; update references to OBS_COUNT_SLOT and notifications_obs_idx accordingly.
🤖 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 `@skills/README.md`:
- Around line 454-485: The example uses top-level await with iii.trigger (and
the `@iii.register_function` coroutines) which causes a SyntaxError in normal
Python scripts; wrap the async parts into an async entrypoint (e.g., create
async def main(): move the two await iii.trigger(...) calls and any async
registration logic into main) and invoke it with asyncio.run(main()) so the
register_worker/register_function coroutines and iii.trigger calls run inside an
event loop.
In `@skills/src/config.rs`:
- Around line 93-97: Rename the existing test function malformed_yaml_errors to
something like missing_config_file_errors to reflect that it exercises
load_config when std::fs::read_to_string fails, and add a new unit test that
calls load_config with a path to a temporary file containing invalid YAML to
assert that the returned Result is Err due to serde_yaml deserialization
(exercise serde_yaml::from_str path). Update references to the test name if
needed and ensure the new test writes invalid YAML to a temp file (or uses
std::fs::write to a tempfile) before calling load_config to trigger the parse
error.
In `@skills/src/functions/mod.rs`:
- Line 25: The log message in tracing::info currently hardcodes counts ("skills
registered 5 skills::* and 5 prompts::* functions"), which will become incorrect
as handlers change; update the log in skills::register (or wherever
tracing::info is called) to either remove the literal counts altogether or
compute them dynamically by counting the registered entries from
skills::register and prompts::register (or the collections they populate) and
include those computed values in the message so it always reflects the real
number of handlers.
In `@skills/src/trigger_types.rs`:
- Around line 115-127: The current calls to iii.register_trigger_type (using let
_ = ...) drop the Result and always emit tracing::info!, which hides failures;
update the registration in the function that registers all trigger types (where
register_trigger_type and RegisterTriggerType::new are used) to handle the
Result: call iii.register_trigger_type(...) for both SKILLS_ON_CHANGE and
PROMPTS_ON_CHANGE, check the returned Result, and on Err either propagate the
error (return Err from the register_all function) or at minimum log the failure
with tracing::error! including the error details and the trigger name
(SKILLS_ON_CHANGE / PROMPTS_ON_CHANGE), and only emit tracing::info! on Ok so
the success log is conditional; keep the use of SkillsTriggerHandler::new(...)
and RegisterTriggerType::new(...) but don't discard the Result.
In `@skills/tests/common/engine.rs`:
- Around line 54-63: The worker registration error is being swallowed by .ok()?
inside the ENGINE.get_or_init closure; update the closure in get_or_init so
register_all's Err is explicitly handled: call
crate::common::workers::register_all(&iii).await and on Err(e) emit a clear
skip/diagnostic message (same style as try_connect_raw's skip output) and return
None, and on Ok(_) return Some(iii); this ensures ENGINE only stores Some(iii)
when registration succeeds and that registration failures are logged instead of
silently dropped.
In `@skills/tests/common/world.rs`:
- Around line 56-60: scoped_id currently truncates the combined string which can
drop the unique_id suffix; change it to build the suffix first (using
self.unique_id and the "-" separator), then truncate the base to at most (64 -
suffix.len()) characters (but not negative) and return format!("{}{}",
truncated_base, suffix) so the unique suffix is always preserved and the final
id respects skills::validate_id's 64-char limit.
In `@skills/tests/steps/prompts_get.rs`:
- Around line 104-114: The step attributes on seed_content and seed_messages are
being parsed as Cucumber Expression parameters; change their attribute strings
to regex-style patterns so the braces are treated literally (e.g. replace the
plain string in the #[given(...)] for seed_content and seed_messages with a
regex pattern that matches the literal text "a prompt handler that returns a
{content} object" and "a prompt handler that returns a {messages: [...]} object"
respectively); no changes needed to the function signatures—keep
register_handler_returning and register_prompt_pointing_at usage the same.
---
Outside diff comments:
In `@README.md`:
- Around line 13-21: Add a new row for the skills worker to the "## Modules"
Markdown table in README.md so the module registry is complete and discoverable:
insert an entry with the display name `skills` (folder `skills/`) and a short
summary like "Node | Skills worker providing reusable action primitives" (or
your preferred concise description) into the existing table alongside other
workers such as `iii-lsp`, `image-resize`, `mcp`, etc.; ensure the row follows
the same pipe-delimited table format and style used by the other entries so
rendering and ordering remain consistent.
---
Nitpick comments:
In `@skills/src/functions/prompts.rs`:
- Line 294: The sort currently uses prompts.sort_by(|a, b|
a["name"].as_str().cmp(&b["name"].as_str())), which compares Option<&str> and
can silently place missing names first; change to explicitly extract the string
key (e.g., via a["name"].as_str().expect("prompt missing name") or by mapping to
a required &str and handling errors) before calling sort_by to ensure a missing
or malformed `name` panics or is handled deterministically — update the call
site where `prompts` is sorted in prompts.rs to use the explicit extraction
(expect or explicit match) so the comparator works on &str not Option<&str>.
In `@skills/src/manifest.rs`:
- Around line 21-27: The hardcoded default_config JSON duplicates
SkillsConfig::default(); replace the literal with a canonical serialization of
the config default so there is a single source of truth: call
serde_json::to_value(SkillsConfig::default()) (or equivalent) to produce the
Value for default_config in manifest.rs, and add the necessary use/import for
SkillsConfig and serde_json::to_value so the manifest now derives its defaults
from SkillsConfig::default() instead of the manual serde_json! literal.
In `@skills/tests/bdd.rs`:
- Around line 33-37: When get_or_init() returns Some(iii) ensure
common::workers::shared() is also present instead of silently leaving world.cfg
default; in the branch handling the successful engine init (the block that sets
world.iii from common::engine::get_or_init()), call common::workers::shared()
and if it returns None either assert/panic or log an explicit error and fail the
test so failures surface as "workers not initialized" rather than causing opaque
downstream errors; reference the symbols common::engine::get_or_init,
common::workers::shared, world.iii and world.cfg when making the change.
In `@skills/tests/common/workers.rs`:
- Around line 34-53: The register_all function currently races by calling
SHARED.get() then doing work and SHARED.set(), so replace that pattern with
SHARED.get_or_try_init(...) to perform the registration atomically: move
creation of cfg, call trigger_types::register_all and functions::register_all,
the tokio::time::sleep, and construction of Arc<Shared> into the async init
closure passed to SHARED.get_or_try_init so only one initializer runs; then
return the cloned Arc from the get_or_try_init result. Update the function
signature and error propagation as needed to use the Result returned by
get_or_try_init and remove the manual SHARED.set/get usage.
In `@skills/tests/steps/notifications.rs`:
- Around line 102-126: The current subscribe_skills and subscribe_prompts
compute the observer index with observers().lock().unwrap().len() - 1 which
races if other scenarios push into OBSERVERS; change register_observer (or
new_observer) to return the created observer's index (or return a handle
containing the index) and update subscribe_skills and subscribe_prompts to use
that returned idx when storing "notifications_obs_idx" (avoid computing
len()-1), or alternatively ensure register_observer itself locks OBSERVERS,
pushes the observer and returns the index atomically so callers don't need to
call observers().lock() themselves; update references to OBS_COUNT_SLOT and
notifications_obs_idx accordingly.
🪄 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: 2d74395a-7be5-4cfc-80bd-638a2c0b3b5a
⛔ Files ignored due to path filters (1)
skills/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
README.mdskills/Cargo.tomlskills/README.mdskills/build.rsskills/config.yamlskills/iii.worker.yamlskills/src/config.rsskills/src/functions/mod.rsskills/src/functions/prompts.rsskills/src/functions/skills.rsskills/src/lib.rsskills/src/main.rsskills/src/manifest.rsskills/src/state.rsskills/src/trigger_types.rsskills/tests/bdd.rsskills/tests/common/engine.rsskills/tests/common/mod.rsskills/tests/common/workers.rsskills/tests/common/world.rsskills/tests/features/markdown.featureskills/tests/features/mcp_bridge.featureskills/tests/features/notifications.featureskills/tests/features/prompts_get.featureskills/tests/features/prompts_register.featureskills/tests/features/skills_register.featureskills/tests/features/skills_resources.featureskills/tests/steps/markdown.rsskills/tests/steps/mcp_bridge.rsskills/tests/steps/mod.rsskills/tests/steps/notifications.rsskills/tests/steps/prompts_get.rsskills/tests/steps/prompts_register.rsskills/tests/steps/skills_register.rsskills/tests/steps/skills_resources.rsworker-readme.md
This was referenced May 4, 2026
Merged
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.
Summary by CodeRabbit
New Features
iii://scheme