feat(iii-directory): global skills home, auto-download, install-filtered reads, LLM-friendly docs#211
Merged
Conversation
…red reads, LLM-friendly docs Squash of the directory-skills-home-autodownload branch. Core: - Global skills home with auto-download; explicit download_from_registry and download_from_repo entry points plus a flexible download alias. Downloading is the only write path; file-by-file overwrite preserves hand-edited siblings; emits skills/prompts on-change events. - Install-filtered reads: list/index/get only surface skills for registered (installed) workers, plus the always-visible directory and iii namespaces. Cold-start reconcile and daemon-down fallback to the unfiltered set; skill-less installed workers fall back to the engine overview. - Agent-facing bare worker ids: get/list/index return the bare worker name (iii-sandbox, not iii-sandbox/index); get gains colloquial-name and function-shaped-path fallbacks that redirect to the worker overview. - LLM-friendly errors: prose recovery messages (Dxxx class: problem. Did you mean ... Next: call ...) for skills/prompts/registry misses. - Registry hardening: 60s per-input cache, pre-install schema source via registry::workers::info, pagination contract. - prompts honor local_skills_folder (wired scan_prompts_merged). - Rewrote iii-directory/skills/SKILL.md for weak-LLM clarity and aligned it with the above behavior. Tests: - New e2e harness under tests/e2e covering auto-download, boot reconcile, startup fs-health scan, real git-clone source, prompt fixtures, and fault injection (unreadable skill file). Removed the legacy directory.rs/how_to.rs monoliths and stale feature files. Harness: - Steer agents to web::fetch via agent_trigger for HTTP(S) instead of shell curl/wget (parsed envelope, size/timeout caps, SSRF protection).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRemoves the directory::engine wrappers and refactors iii-directory into skills, prompts, and registry surfaces; adds merged local/global skill scanning and aliasing, registered-worker caching, boot-time auto-download and reconcile flows, registry/git input validation, and a comprehensive E2E test harness and docs updates. Changesiii-directory structural refactor and API surface changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
skill-check — worker0 verified, 13 skipped (no docs/).
Note 17 stale rendered artifact(s) detected on main, unrelated to this PR. This PR is fine; the drift was already there. A maintainer should open a chore PR to re-render these.
|
CI runs clippy with -D warnings; rank_suggestions_in tripped clippy::unnecessary_sort_by. Swap the manual descending comparator for sort_by_key with std::cmp::Reverse on the Copy i32 score. Behavior is identical (stable, score-descending).
![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)
iii-directory/README.md (1)
181-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFunction-surface section is internally inconsistent about engine wrappers.
Line 181 says “Sixteen functions, all under
directory::*”, and Line 203 says introspection is no longer wrapped; however this PR stack keepsdirectory::engine::functions::infoas a proxy. Please make this section consistent (counts + wrapper statement) so agents/users don’t call the wrong IDs.🤖 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 `@iii-directory/README.md` around lines 181 - 216, The documentation is inconsistent: update the "Function-surface" section so the count and the statement about engine wrappers match reality—either remove any mention of directory::engine::* proxies (e.g., delete or replace references to directory::engine::functions::info) and state that engine introspection must be called via native engine IDs (keeping "Sixteen functions, all under directory::*" if that’s the final count), or explicitly list the engine wrapper functions under directory::engine::* and adjust the total function count and the wrapper statement accordingly; ensure references to directory::skills::*, directory::prompts::*, and any engine wrapper names (like directory::engine::functions::info) are consistent with the codebase and update the count and the sentence on Line 181/203 to reflect the chosen approach.
🧹 Nitpick comments (4)
iii-directory/src/functions/mod.rs (1)
71-88: ⚡ Quick win
register_all_with_cacheduplicatesregister_allalmost verbatim.The only difference is
skills::register_with_cachevsskills::register; prompts/download/registry/engine_fn registration and the log line are identical. Consider havingregister_alldelegate toregister_all_with_cachewith a freshly-built cache, or extract the shared tail into a private helper to avoid the two drifting over time.🤖 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 `@iii-directory/src/functions/mod.rs` around lines 71 - 88, register_all_with_cache duplicates register_all; fix by factoring the shared registrations into a single helper and have register_all either call register_all_with_cache with a newly constructed cache or both call a private helper. Specifically, extract the common tail (prompts::register, Subscribers::from + download::register, registry::register, engine_fn::register, and the tracing::info call) into a private function (e.g., fn register_common(iii: &Arc<III>, cfg: &Arc<SkillsConfig>, subs_opt: Option<&Subscribers>) or fn register_common(iii: &Arc<III>, cfg: &Arc<SkillsConfig>, subs: &Subscribers)); then modify register_all_with_cache to call skills::register_with_cache(...) followed by register_common, and modify register_all to call skills::register(...) then register_common (or have register_all build an Arc<skills::RegisteredWorkersCache> and call register_all_with_cache). Use the existing function names register_all_with_cache, register_all, skills::register_with_cache, skills::register, prompts::register, download::register, registry::register, engine_fn::register and Subscribers::from to locate the code.iii-directory/tests/e2e/README.md (1)
39-39: 💤 Low valueAdd a language to the fenced code block.
markdownlint (MD040) flags this fence. Use
textto silence it.📝 Proposed tweak
-``` +```text run-tests.sh the asserting suite (one command, exits 0/1)🤖 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 `@iii-directory/tests/e2e/README.md` at line 39, Add a language tag to the fenced code block that contains "run-tests.sh the asserting suite" to satisfy markdownlint MD040; edit the triple-backtick fence in the README's code block and change it from ``` to ```text so the block becomes a text code fence.iii-directory/tests/e2e/run-tests.sh (1)
14-15: 💤 Low valueMinor: guard the
cdand drop or use the unusedHERE.
- Line 14:
cd "$ROOT_DIR"is unguarded (SC2164); add|| exit 1so a failedcddoesn't run the laterrm -rf "$GLOBAL"in the wrong directory.- Line 15:
HEREis assigned but never referenced anywhere in the script (the comment claims the assertion body uses it). Either drop it or remove the stale comment.♻️ Proposed tweak
-ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"; cd "$ROOT_DIR" -HERE="$ROOT_DIR" # assertion body refers to $HERE / $GLOBAL +ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"; cd "$ROOT_DIR" || exit 1🤖 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 `@iii-directory/tests/e2e/run-tests.sh` around lines 14 - 15, Guard the unprotected directory change and clean up the unused HERE variable: update the ROOT_DIR assignment so the subsequent cd "$ROOT_DIR" is followed by a fail-safe (e.g., add "|| exit 1" or similar) to prevent running later commands in the wrong location if cd fails, and remove the unused HERE="$ROOT_DIR" assignment and its stale comment (or instead use HERE where appropriate) to eliminate the dead variable in run-tests.sh; reference ROOT_DIR, cd "$ROOT_DIR", HERE, and GLOBAL when making the fixes.iii-directory/src/main.rs (1)
278-278: 💤 Low valueMinor: Doc comment says "~30s" but actual backoff totals ~42s.
With 6 attempts and
attempt * 2backoff (2+4+6+8+10+12 = 42s cumulative sleep), plus up to 10s timeout per attempt, the actual budget is closer to ~100s worst-case. Consider updating the comment to reflect the actual retry budget.🤖 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 `@iii-directory/src/main.rs` at line 278, The doc comment is inaccurate: with 6 attempts and backoff computed as `attempt * 2` the sleeps sum to ~42s (2+4+6+8+10+12), and with up to 10s timeout per attempt the worst‑case total is ≈100–102s; update the comment near the retry loop to state both the cumulative backoff (~42s) and the approximate worst‑case retry budget (~100s) and reference the `attempt` variable, the `attempt * 2` backoff formula, and the per‑attempt 10s timeout so readers see how the numbers are derived.
🤖 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 `@iii-directory/README.md`:
- Line 10: Update the README docs to remove the now-removed "description" field
from the public directory::skills::get response contract and ensure the
documented response shape matches the implementation (directory::skills::get
should list only { id, title, type, body, modified_at }); edit the table row for
"Skills" and the detailed section that previously claimed "description" (also
referenced around the second occurrence) to reflect the corrected fields and
note that title prefers YAML frontmatter and type may be null.
In `@iii-directory/skills/SKILL.md`:
- Around line 103-105: The fenced code block containing the message starting
with "D110 not_found: skill \"iii-sanbox\"..." in SKILL.md is unlabeled and
triggers MD040; edit that fence to add a language tag (e.g., change ``` to
```text or ```console) so the block is labeled and the markdown linter passes.
- Around line 168-169: Update the trigger introspection IDs so agents call the
existing functions: replace any occurrences of engine::trigger-types::* with
engine::registered-triggers::* for registered trigger instances, and ensure
trigger type calls use engine::triggers::* (e.g. engine::triggers::list for
types and engine::registered-triggers::list for instances); search for
references to engine::trigger-types:: and engine::triggers:: in SKILL.md and
correct them to the documented engine::triggers::* (types) and
engine::registered-triggers::* (instances) IDs.
In `@iii-directory/src/config.rs`:
- Around line 139-146: The fallback branch that runs when dirs::home_dir()
returns None currently joins the literal raw string (e.g., "~/.iii/skills") to
cwd resulting in paths with a literal '~'; update the None arm so you strip a
leading "~/" or a lone "~" from raw before joining (e.g., remove the "~/" prefix
or if raw == "~" treat it as empty) and then join the sanitized remainder to
cwd; modify the code around the cwd and raw variables in the fallback branch to
perform this prefix removal so the resulting path is CWD-relative without a
literal '~'.
In `@iii-directory/tests/e2e/config.yaml`:
- Around line 15-26: The top comment is incorrect about auto_download; update
the comment above the workers block to state that filter_unregistered is OFF but
auto_download is ON (driven by the engine worker add event / boot-reconcile),
clarifying that only filter_unregistered requires the iii-worker-manager daemon
while auto_download (the auto_download key) is handled by boot-reconcile and the
__on_worker_added handler; edit the comment to accurately reflect
"filter_unregistered: false" and "auto_download: true" and mention the harness
reliance on boot-reconcile/run-tests.sh rather than the daemon.
In `@iii-directory/tests/e2e/run-tests.sh`:
- Around line 583-599: The permission-based tests that set chmod 000 on fixtures
(the mkdir/printf/chmod sequence creating "$GLOBAL/permns/prompts/p.md" and
"$GLOBAL/permskill/index.md") should be skipped when running as root, because
root bypasses permission bits; wrap the unreadable-prompt and unreadable-skill
blocks (the chmod 000 + subsequent trig directory::prompts::list and trig
directory::skills::get calls and their jtrue/iserr assertions) in a guard like
if [ "$(id -u)" != "0" ]; then ... fi so the chmod 000 and assertions around
directory::prompts::list and directory::skills::get (id=permskill/index) only
run for non-root runners, and ensure the fixtures are restored (chmod 644) in
the else branch or after the guard so teardown can clean up.
- Around line 19-22: The assignment to the III variable contains a hardcoded
developer-specific fallback path; remove that path and change the logic in the
III assignment (in run-tests.sh) to prefer command -v iii and
$HOME/.local/bin/iii only, and if neither is found print a clear error and exit
(fail fast) so tests stop with a helpful message rather than using a
non-portable path; update the III variable resolution and add an explicit
error/exit when III is empty.
---
Outside diff comments:
In `@iii-directory/README.md`:
- Around line 181-216: The documentation is inconsistent: update the
"Function-surface" section so the count and the statement about engine wrappers
match reality—either remove any mention of directory::engine::* proxies (e.g.,
delete or replace references to directory::engine::functions::info) and state
that engine introspection must be called via native engine IDs (keeping "Sixteen
functions, all under directory::*" if that’s the final count), or explicitly
list the engine wrapper functions under directory::engine::* and adjust the
total function count and the wrapper statement accordingly; ensure references to
directory::skills::*, directory::prompts::*, and any engine wrapper names (like
directory::engine::functions::info) are consistent with the codebase and update
the count and the sentence on Line 181/203 to reflect the chosen approach.
---
Nitpick comments:
In `@iii-directory/src/functions/mod.rs`:
- Around line 71-88: register_all_with_cache duplicates register_all; fix by
factoring the shared registrations into a single helper and have register_all
either call register_all_with_cache with a newly constructed cache or both call
a private helper. Specifically, extract the common tail (prompts::register,
Subscribers::from + download::register, registry::register, engine_fn::register,
and the tracing::info call) into a private function (e.g., fn
register_common(iii: &Arc<III>, cfg: &Arc<SkillsConfig>, subs_opt:
Option<&Subscribers>) or fn register_common(iii: &Arc<III>, cfg:
&Arc<SkillsConfig>, subs: &Subscribers)); then modify register_all_with_cache to
call skills::register_with_cache(...) followed by register_common, and modify
register_all to call skills::register(...) then register_common (or have
register_all build an Arc<skills::RegisteredWorkersCache> and call
register_all_with_cache). Use the existing function names
register_all_with_cache, register_all, skills::register_with_cache,
skills::register, prompts::register, download::register, registry::register,
engine_fn::register and Subscribers::from to locate the code.
In `@iii-directory/src/main.rs`:
- Line 278: The doc comment is inaccurate: with 6 attempts and backoff computed
as `attempt * 2` the sleeps sum to ~42s (2+4+6+8+10+12), and with up to 10s
timeout per attempt the worst‑case total is ≈100–102s; update the comment near
the retry loop to state both the cumulative backoff (~42s) and the approximate
worst‑case retry budget (~100s) and reference the `attempt` variable, the
`attempt * 2` backoff formula, and the per‑attempt 10s timeout so readers see
how the numbers are derived.
In `@iii-directory/tests/e2e/README.md`:
- Line 39: Add a language tag to the fenced code block that contains
"run-tests.sh the asserting suite" to satisfy markdownlint MD040; edit the
triple-backtick fence in the README's code block and change it from ``` to
```text so the block becomes a text code fence.
In `@iii-directory/tests/e2e/run-tests.sh`:
- Around line 14-15: Guard the unprotected directory change and clean up the
unused HERE variable: update the ROOT_DIR assignment so the subsequent cd
"$ROOT_DIR" is followed by a fail-safe (e.g., add "|| exit 1" or similar) to
prevent running later commands in the wrong location if cd fails, and remove the
unused HERE="$ROOT_DIR" assignment and its stale comment (or instead use HERE
where appropriate) to eliminate the dead variable in run-tests.sh; reference
ROOT_DIR, cd "$ROOT_DIR", HERE, and GLOBAL when making the fixes.
🪄 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: 0aa9b533-b767-4572-8298-d991cb01ddef
⛔ Files ignored due to path filters (1)
iii-directory/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (55)
console/README.mdconsole/web/src/hooks/use-functions-catalog.tsconsole/web/src/lib/functions-catalog.tsconsole/web/src/vite-env.d.tsharness/src/turn-orchestrator/system-prompt.tsiii-directory/Cargo.tomliii-directory/README.mdiii-directory/skill.mdiii-directory/skills/SKILL.mdiii-directory/skills/directory/engine/functions/info.mdiii-directory/skills/directory/engine/functions/list.mdiii-directory/skills/directory/engine/registered-triggers/info.mdiii-directory/skills/directory/engine/registered-triggers/list.mdiii-directory/skills/directory/engine/triggers/info.mdiii-directory/skills/directory/engine/triggers/list.mdiii-directory/skills/directory/engine/workers/info.mdiii-directory/skills/directory/engine/workers/list.mdiii-directory/skills/directory/prompts.mdiii-directory/skills/directory/registry/workers/info.mdiii-directory/skills/directory/registry/workers/list.mdiii-directory/skills/directory/skills/download.mdiii-directory/skills/directory/skills/get.mdiii-directory/skills/directory/skills/index.mdiii-directory/skills/directory/skills/list.mdiii-directory/skills/index.mdiii-directory/src/config.rsiii-directory/src/fs_source.rsiii-directory/src/functions/directory.rsiii-directory/src/functions/download.rsiii-directory/src/functions/engine_fn.rsiii-directory/src/functions/error.rsiii-directory/src/functions/mod.rsiii-directory/src/functions/prompts.rsiii-directory/src/functions/registry.rsiii-directory/src/functions/skills.rsiii-directory/src/how_to.rsiii-directory/src/lib.rsiii-directory/src/main.rsiii-directory/src/sources/git.rsiii-directory/src/sources/registry.rsiii-directory/tests/e2e/.gitignoreiii-directory/tests/e2e/README.mdiii-directory/tests/e2e/config.yamliii-directory/tests/e2e/reports/.gitkeepiii-directory/tests/e2e/run-tests.shiii-directory/tests/features/directory_functions.featureiii-directory/tests/features/directory_triggers.featureiii-directory/tests/features/directory_workers.featureiii-directory/tests/features/read.featureiii-directory/tests/features/registry_worker_info.featureiii-directory/tests/features/registry_worker_list.featureiii-directory/tests/steps/directory.rsiii-directory/tests/steps/mod.rsiii-directory/tests/steps/read.rsiii-directory/tests/steps/registry.rs
💤 Files with no reviewable changes (24)
- iii-directory/skills/directory/prompts.md
- iii-directory/skills/directory/skills/index.md
- iii-directory/skills/directory/registry/workers/info.md
- iii-directory/skills/directory/skills/list.md
- iii-directory/skills/directory/skills/download.md
- iii-directory/tests/features/directory_triggers.feature
- iii-directory/tests/features/directory_workers.feature
- iii-directory/skills/directory/skills/get.md
- iii-directory/skills/directory/engine/functions/list.md
- iii-directory/skills/directory/engine/registered-triggers/list.md
- iii-directory/skills/directory/engine/functions/info.md
- iii-directory/skills/directory/engine/registered-triggers/info.md
- iii-directory/skills/directory/engine/triggers/list.md
- iii-directory/skills/directory/engine/workers/list.md
- iii-directory/skills/index.md
- iii-directory/skills/directory/registry/workers/list.md
- iii-directory/tests/steps/mod.rs
- iii-directory/skills/directory/engine/workers/info.md
- iii-directory/tests/steps/directory.rs
- iii-directory/skills/directory/engine/triggers/info.md
- iii-directory/src/functions/directory.rs
- iii-directory/tests/steps/read.rs
- iii-directory/tests/features/directory_functions.feature
- iii-directory/src/how_to.rs
|
|
||
| | Surface | What clients see | When to use it | | ||
| |---|---|---| | ||
| | **Skills** (`directory::skills::*`) | Enriched listing via `directory::skills::list` (`{ id, title, type, description, bytes, modified_at }` per row), a single-skill reader `directory::skills::get { id }` returning `{ id, title, type, description, body, modified_at }`, and `directory::skills::index` which renders a short per-worker overview document (one `## <title>` + first paragraph + `read more` link per `type: index` skill). `title` prefers the YAML frontmatter `title:` over the body H1; `type` is lifted from frontmatter `type:` (e.g. `index`, `how-to`, `reference`) and serialised as `null` when absent. | Orientation: "when and why to use my worker's tools" | |
There was a problem hiding this comment.
Public skills::get response shape is documented incorrectly.
Line 10 and Line 191 still claim directory::skills::get returns description, but this PR’s feature updates explicitly remove description from get. Please align both tables/sections to the actual response contract to avoid client-side parsing bugs.
Suggested doc patch
-| **Skills** (`directory::skills::*`) | ... `directory::skills::get { id }` returning `{ id, title, type, description, body, modified_at }` ...
+| **Skills** (`directory::skills::*`) | ... `directory::skills::get { id }` returning `{ id, title, type, body, modified_at }` ...-| `directory::skills::get` | Fetch one skill by id. Returns `{ id, title, type, description, body, modified_at }` ...
+| `directory::skills::get` | Fetch one skill by id. Returns `{ id, title, type, body, modified_at }` ...Also applies to: 181-192
🤖 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 `@iii-directory/README.md` at line 10, Update the README docs to remove the
now-removed "description" field from the public directory::skills::get response
contract and ensure the documented response shape matches the implementation
(directory::skills::get should list only { id, title, type, body, modified_at
}); edit the table row for "Skills" and the detailed section that previously
claimed "description" (also referenced around the second occurrence) to reflect
the corrected fields and note that title prefers YAML frontmatter and type may
be null.
Comment on lines
+103
to
+105
| ``` | ||
| D110 not_found: skill "iii-sanbox" does not exist. Did you mean: iii-sandbox. Next: call directory::skills::list to browse skill ids; or directory::skills::index to see the per-worker overview. | ||
| ``` |
There was a problem hiding this comment.
Add a language to this fenced code block to satisfy markdown lint.
The unlabeled fence at Line 103 triggers MD040; add text (or appropriate language) to keep docs lint clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 103-103: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@iii-directory/skills/SKILL.md` around lines 103 - 105, The fenced code block
containing the message starting with "D110 not_found: skill \"iii-sanbox\"..."
in SKILL.md is unlabeled and triggers MD040; edit that fence to add a language
tag (e.g., change ``` to ```text or ```console) so the block is labeled and the
markdown linter passes.
Comment on lines
+168
to
+169
| - `engine::triggers::list` / `engine::trigger-types::list` — registered trigger instances and types. | ||
|
|
There was a problem hiding this comment.
Trigger introspection function IDs appear incorrect.
Line 168/Line 174 use engine::trigger-types::*, but this PR’s own docs elsewhere use engine::triggers::* (types) and engine::registered-triggers::* (instances). As written, agents will likely call non-existent IDs.
Also applies to: 174-174
🤖 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 `@iii-directory/skills/SKILL.md` around lines 168 - 169, Update the trigger
introspection IDs so agents call the existing functions: replace any occurrences
of engine::trigger-types::* with engine::registered-triggers::* for registered
trigger instances, and ensure trigger type calls use engine::triggers::* (e.g.
engine::triggers::list for types and engine::registered-triggers::list for
instances); search for references to engine::trigger-types:: and
engine::triggers:: in SKILL.md and correct them to the documented
engine::triggers::* (types) and engine::registered-triggers::* (instances) IDs.
Comment on lines
+139
to
+146
| None => { | ||
| tracing::warn!( | ||
| path = %raw, | ||
| "dirs::home_dir() returned None; treating '~' path as CWD-relative" | ||
| ); | ||
| let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); | ||
| cwd.join(raw) | ||
| } |
There was a problem hiding this comment.
Fallback path on home_dir() failure includes the literal ~ character.
When dirs::home_dir() returns None, the code joins the raw path (e.g., ~/.iii/skills) to the CWD, producing a path like /current/dir/~/.iii/skills. This creates a directory with a literal ~ in its name, which is almost certainly unintended.
Consider stripping the ~ prefix before joining, or failing more explicitly:
🐛 Proposed fix
None => {
tracing::warn!(
path = %raw,
"dirs::home_dir() returned None; treating '~' path as CWD-relative"
);
let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("."));
- cwd.join(raw)
+ // Strip the tilde so we don't create a literal "~" directory.
+ cwd.join(tail)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| None => { | |
| tracing::warn!( | |
| path = %raw, | |
| "dirs::home_dir() returned None; treating '~' path as CWD-relative" | |
| ); | |
| let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); | |
| cwd.join(raw) | |
| } | |
| None => { | |
| tracing::warn!( | |
| path = %raw, | |
| "dirs::home_dir() returned None; treating '~' path as CWD-relative" | |
| ); | |
| let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); | |
| // Strip the tilde so we don't create a literal "~" directory. | |
| cwd.join(tail) | |
| } |
🤖 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 `@iii-directory/src/config.rs` around lines 139 - 146, The fallback branch that
runs when dirs::home_dir() returns None currently joins the literal raw string
(e.g., "~/.iii/skills") to cwd resulting in paths with a literal '~'; update the
None arm so you strip a leading "~/" or a lone "~" from raw before joining
(e.g., remove the "~/" prefix or if raw == "~" treat it as empty) and then join
the sanitized remainder to cwd; modify the code around the cwd and raw variables
in the fallback branch to perform this prefix removal so the resulting path is
CWD-relative without a literal '~'.
Comment on lines
+15
to
+26
| # filter_unregistered / auto_download are OFF so the run is self-contained | ||
| # (both need the iii-worker-manager daemon). | ||
| workers: | ||
| - name: iii-directory | ||
| config: | ||
| skills_folder: __E2E_DIR__/skills-home | ||
| local_skills_folder: __E2E_DIR__/.iii/skills | ||
| registry_url: https://api.workers.iii.dev | ||
| download_timeout_ms: 60000 | ||
| registry_cache_ttl_ms: 60000 | ||
| filter_unregistered: false | ||
| auto_download: true |
There was a problem hiding this comment.
Comment contradicts the actual auto_download value.
The comment states both filter_unregistered and auto_download are OFF, but Line 26 sets auto_download: true. The harness deliberately relies on this (boot-reconcile in run-tests.sh §0 and the __on_worker_added handler tests in §11), so the comment is misleading. Note that auto_download here is driven directly by the engine worker add event / boot-reconcile task and does not require the daemon, unlike filter_unregistered.
📝 Proposed comment fix
-# filter_unregistered / auto_download are OFF so the run is self-contained
-# (both need the iii-worker-manager daemon).
+# filter_unregistered is OFF so the run is self-contained (it needs the
+# iii-worker-manager daemon). auto_download is ON: boot-reconcile and the
+# __on_worker_added handler are driven directly by the harness (no daemon).🤖 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 `@iii-directory/tests/e2e/config.yaml` around lines 15 - 26, The top comment is
incorrect about auto_download; update the comment above the workers block to
state that filter_unregistered is OFF but auto_download is ON (driven by the
engine worker add event / boot-reconcile), clarifying that only
filter_unregistered requires the iii-worker-manager daemon while auto_download
(the auto_download key) is handled by boot-reconcile and the __on_worker_added
handler; edit the comment to accurately reflect "filter_unregistered: false" and
"auto_download: true" and mention the harness reliance on
boot-reconcile/run-tests.sh rather than the daemon.
Comment on lines
+19
to
+22
| # Prefer iii on PATH, then the conventional install dir, then a local build. | ||
| III="${III:-$(command -v iii 2>/dev/null \ | ||
| || { [ -x "$HOME/.local/bin/iii" ] && echo "$HOME/.local/bin/iii"; } \ | ||
| || echo /Users/andersonleal/projetos/motia/motia/target/release/iii)}" |
There was a problem hiding this comment.
Remove the hardcoded developer-specific fallback path.
The final fallback /Users/andersonleal/projetos/motia/motia/target/release/iii is a personal, non-portable path that will never resolve on other machines or CI, and leaks a developer's local layout into the repo. Prefer failing fast with a clear message when iii cannot be located.
♻️ Proposed fix
-# Prefer iii on PATH, then the conventional install dir, then a local build.
-III="${III:-$(command -v iii 2>/dev/null \
- || { [ -x "$HOME/.local/bin/iii" ] && echo "$HOME/.local/bin/iii"; } \
- || echo /Users/andersonleal/projetos/motia/motia/target/release/iii)}"
+# Prefer iii on PATH, then the conventional install dir.
+III="${III:-$(command -v iii 2>/dev/null \
+ || { [ -x "$HOME/.local/bin/iii" ] && echo "$HOME/.local/bin/iii"; })}"
+[ -n "$III" ] && [ -x "$III" ] || { echo "iii engine not found on PATH or \$HOME/.local/bin (set III=/path/to/iii)"; exit 1; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Prefer iii on PATH, then the conventional install dir, then a local build. | |
| III="${III:-$(command -v iii 2>/dev/null \ | |
| || { [ -x "$HOME/.local/bin/iii" ] && echo "$HOME/.local/bin/iii"; } \ | |
| || echo /Users/andersonleal/projetos/motia/motia/target/release/iii)}" | |
| # Prefer iii on PATH, then the conventional install dir. | |
| III="${III:-$(command -v iii 2>/dev/null \ | |
| || { [ -x "$HOME/.local/bin/iii" ] && echo "$HOME/.local/bin/iii"; })}" | |
| [ -n "$III" ] && [ -x "$III" ] || { echo "iii engine not found on PATH or \$HOME/.local/bin (set III=/path/to/iii)"; exit 1; } |
🤖 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 `@iii-directory/tests/e2e/run-tests.sh` around lines 19 - 22, The assignment to
the III variable contains a hardcoded developer-specific fallback path; remove
that path and change the logic in the III assignment (in run-tests.sh) to prefer
command -v iii and $HOME/.local/bin/iii only, and if neither is found print a
clear error and exit (fail fast) so tests stop with a helpful message rather
than using a non-portable path; update the III variable resolution and add an
explicit error/exit when III is empty.
Comment on lines
+583
to
+599
| # unreadable prompt (perm 000) -> scan_prompts records a read SkipReason | ||
| mkdir -p "$GLOBAL/permns/prompts" | ||
| printf -- '---\ndescription: x\n---\nbody\n' > "$GLOBAL/permns/prompts/p.md"; chmod 000 "$GLOBAL/permns/prompts/p.md" | ||
| # list + index + prompts::list force a full scan over all of the above (skip arms) | ||
| out=$(trig directory::skills::list --json '{}') | ||
| jtrue "skills::list healthy with fault fixtures (scan skips the bad ones)" "$out" '.skills | length > 0' | ||
| out=$(trig directory::skills::index --json '{}') | ||
| jtrue "skills::index still renders with fault fixtures present" "$out" '.body | length > 0' | ||
| out=$(trig directory::prompts::list --json '{}') | ||
| jtrue "prompts::list healthy with an unreadable prompt present" "$out" '.prompts | type == "array"' | ||
| chmod 644 "$GLOBAL/permns/prompts/p.md" 2>/dev/null || true # restore so teardown can clean | ||
| # unreadable skill file (perm 000) -> the body read fails on get (read-error arm) | ||
| mkdir -p "$GLOBAL/permskill" | ||
| printf -- '---\ntype: index\n---\n# P\nbody\n' > "$GLOBAL/permskill/index.md"; chmod 000 "$GLOBAL/permskill/index.md" | ||
| out=$(trig directory::skills::get id=permskill/index) | ||
| iserr "get permskill/index (unreadable file, perm 000) -> rejected" "$out" | ||
| chmod 644 "$GLOBAL/permskill/index.md" 2>/dev/null || true |
There was a problem hiding this comment.
Permission-based fault tests assume a non-root runner.
The chmod 000 fixtures (Lines 585, 596) rely on the OS denying reads, but root bypasses permission bits, so get permskill/index and the unreadable-prompt scan would read successfully and these assertions would flip to FAIL when the suite runs as root (e.g., some CI containers). Consider skipping these two checks when [ "$(id -u)" = 0 ].
🤖 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 `@iii-directory/tests/e2e/run-tests.sh` around lines 583 - 599, The
permission-based tests that set chmod 000 on fixtures (the mkdir/printf/chmod
sequence creating "$GLOBAL/permns/prompts/p.md" and
"$GLOBAL/permskill/index.md") should be skipped when running as root, because
root bypasses permission bits; wrap the unreadable-prompt and unreadable-skill
blocks (the chmod 000 + subsequent trig directory::prompts::list and trig
directory::skills::get calls and their jtrue/iserr assertions) in a guard like
if [ "$(id -u)" != "0" ]; then ... fi so the chmod 000 and assertions around
directory::prompts::list and directory::skills::get (id=permskill/index) only
run for non-root runners, and ensure the fixtures are restored (chmod 644) in
the else branch or after the guard so teardown can clean up.
ytallo
approved these changes
Jun 2, 2026
ytallo
approved these changes
Jun 2, 2026
Comment on lines
+5
to
+16
| functions: | ||
| - directory::skills::list | ||
| - directory::skills::get | ||
| - directory::skills::index | ||
| - directory::skills::download_from_registry | ||
| - directory::skills::download_from_repo | ||
| - directory::skills::download | ||
| - directory::prompts::list | ||
| - directory::prompts::get | ||
| - directory::registry::workers::list | ||
| - directory::registry::workers::info | ||
| - directory::engine::functions::info |
sergiofilhowz
approved these changes
Jun 2, 2026
…ectory::skills::get Two bugs in directory::skills::index, both in render_index_markdown. Q1 — index showed only workers whose overview declared frontmatter `type: index`. Most installed bundles ship their overview as a bare `<ns>/index.md` with legacy `name:`/`description:` frontmatter and no `type:`, so they were silently excluded (e.g. iii, iii-http, iii-sandbox, shell) even though those workers are registered and running. Net effect: 2 of ~13 workers visible. Fix: treat a namespace-root overview doc (`<ns>/index`) as a worker overview even without `type: index` (is_index_overview / is_worker_overview), keyed off a new internal on_disk_id on SkillEntry (not serialized; list output unchanged). Q2 — each block ended with "Read <id>.md" (a file path the agent can't open) and "Dive deeper: https://workers.iii.dev/..." (an external site), never naming the in-engine way to read the doc. Fix: the pointer now says `call directory::skills::get { "id": "<id>" }`, keeping the legacy `iii://<id>` token for back-compat and dropping the external URL. Tests: rewrote the render-index unit tests for the new pointer wording, added regressions for untyped namespace-root overview classification. 242 unit tests pass; clippy/fmt clean. The registered-filter is unchanged: a worker still must be installed to appear (e.g. `web` stays hidden until connected).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
iii-directory/skills/SKILL.md (1)
168-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify trigger introspection function IDs against engine implementation.
Line 168 lists
engine::trigger-types::list, but according to previous review feedback, the correct IDs should beengine::triggers::*for trigger types andengine::registered-triggers::*for registered instances. The description says "registered trigger instances and types", so both are intended, but the function IDs may be incorrect.Similarly, line 174 references
trigger-types::infowhich may need correction.If agents call these IDs as documented, they will receive "not found" errors if the IDs don't match the engine's actual registration.
Run the following script to verify the actual engine function IDs:
#!/bin/bash # Search for trigger-related function registrations in the engine # Find trigger function registrations rg -n -C3 --type=rust 'register.*trigger.*list|register.*trigger.*info' -g '!target/**' # Find the actual function IDs for triggers rg -n --type=rust '"engine::triggers::|"engine::registered-triggers::|"engine::trigger-types::' -g '!target/**'Also applies to: 174-174
🤖 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 `@iii-directory/skills/SKILL.md` at line 168, The documentation lists incorrect trigger function IDs; run the provided grep script to extract the actual registered function IDs from the engine and then update the SKILL.md entries: replace the current `engine::trigger-types::list`/`engine::triggers::list` usage with the exact IDs returned (likely `engine::triggers::*` for trigger types and `engine::registered-triggers::*` for registered instances), and similarly correct the `trigger-types::info` reference to the engine's actual ID (e.g., `engine::trigger-types::info` → exact `engine::...::info` from the search); ensure the text at the lines referencing `engine::triggers::list`, `engine::trigger-types::list`, and `trigger-types::info` matches the engine's registered function IDs verbatim.
🤖 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.
Duplicate comments:
In `@iii-directory/skills/SKILL.md`:
- Line 168: The documentation lists incorrect trigger function IDs; run the
provided grep script to extract the actual registered function IDs from the
engine and then update the SKILL.md entries: replace the current
`engine::trigger-types::list`/`engine::triggers::list` usage with the exact IDs
returned (likely `engine::triggers::*` for trigger types and
`engine::registered-triggers::*` for registered instances), and similarly
correct the `trigger-types::info` reference to the engine's actual ID (e.g.,
`engine::trigger-types::info` → exact `engine::...::info` from the search);
ensure the text at the lines referencing `engine::triggers::list`,
`engine::trigger-types::list`, and `trigger-types::info` matches the engine's
registered function IDs verbatim.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6631cc96-9c52-49aa-88c7-cd0552af2e18
📒 Files selected for processing (2)
iii-directory/skills/SKILL.mdiii-directory/src/functions/skills.rs
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
3 participants
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
Makes
iii-directorya global, self-provisioning skills home: it auto-downloads worker skill bundles, only surfaces skills for workers that are actually installed, hands agents forgiving bare-name ids, and returns prose error messages a weak LLM can self-correct from. The user-facingskills/SKILL.mdis rewritten to match.What changed
Skills home & downloads
download_from_registryanddownload_from_repoentry points plus a flexibledownloadalias. Downloading is the only write path; file-by-file overwrite preserves hand-edited siblings; emitsskills/promptson-change events.Install-filtered reads
list/index/getonly surface skills for registered (installed) workers, plus the always-visibledirectoryandiiinamespaces. Cold-start reconcile and daemon-down fallback to the unfiltered set; an installed-but-skill-less worker falls back to the engine overview.Agent-facing ids & errors
get/list/indexreturn the bare worker name (iii-sandbox, notiii-sandbox/index).getgains colloquial-name (sandbox→iii-sandbox) and function-shaped-path fallbacks that redirect to the worker overview with a> Note:banner.Dxxx <class>: <problem>. Did you mean … Next: call …) for skills / prompts / registry misses (D110/D111/D112/D210/D310).Registry
registry::workers::info, opaque-cursor pagination.Prompts
promptshonorlocal_skills_folder(wiredscan_prompts_merged).Docs
iii-directory/skills/SKILL.mdfor weak-LLM clarity and aligned it with the behavior above.Harness
web::fetchviaagent_triggerfor HTTP(S) instead of shellcurl/wget(parsed envelope, size/timeout caps, SSRF protection).Cleanup
directory.rs/how_to.rsmonoliths and the staletests/features/directory_*.feature+tests/steps/directory.rscucumber suite.Test plan
cargo test -p iii-directory(unit + remaining cucumber features)cargo fmt --all --checkandcargo clippy -p iii-directoryiii-directory/tests/e2e/run-tests.sh(auto-download, boot reconcile, fs-health scan, real git-clone source, prompt fixtures, unreadable-skill fault injection)directory::skills::indexon a fresh skills home →download_from_registry { worker }→ worker appears;get { id: "sandbox" }redirects toiii-sandboxoverviewweb::fetch, not shellcurlSummary by CodeRabbit
New Features
Changes
Documentation