Feat/iii directory support global skills#162
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements SKILLS.md filesystem aliasing, updates the skills API to normalize ChangesSKILLS.md alias and file-path link migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skill-check — worker0 verified, 10 skipped (no docs/).
Three for three. Nicely done. |
The PR-checks workflow requires iii-directory/skill.md per
.github/scripts/validate_worker.py (BOOTSTRAP_WORKERS). skill.md is
rendered from docs/{intro,quickstart,companions}.md via iii-skill-render,
which becomes the source-of-truth going forward — README and
skills/index.md remain unchanged for now to preserve existing content.
read.feature additions: - SKILLS.md at namespace root aliased to <ns>/index - nested SKILLS.md aliased to <ns>/<sub>/index - directory::skills::get accepts <id>.md file-path form - directory::skills::get accepts SKILLS.md filename - directory::skills::get accepts iii:// + .md combined form Updated the existing index scenario to assert the new file-path pointer (Read [<ns>/index.md](<ns>/index.md)) and explicitly check that the rendered body no longer contains the iii:// scheme. docs/intro.md: replaced em dashes with parens/periods to pass the skill-check Vale rule (Terminology.EmDash). skill.md re-rendered.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 21-23: Remove the duplicated install command block containing "iii
worker add iii-directory" that appears after the configuration discussion;
locate the second occurrence of the fenced bash block with that exact command
and delete the entire code fence so the only remaining instance is the one in
the Install section, leaving the surrounding headers (e.g., "## Quickstart")
intact.
🪄 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: 95b6e36e-0430-45e8-9f1f-cd8f4a4404e2
⛔ Files ignored due to path filters (1)
iii-directory/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
iii-directory/README.mdiii-directory/docs/companions.mdiii-directory/docs/intro.mdiii-directory/docs/quickstart.mdiii-directory/examples/test_registry.rsiii-directory/skill.mdiii-directory/skills/index.mdiii-directory/src/fs_source.rsiii-directory/src/functions/registry.rsiii-directory/tests/features/read.feature
💤 Files with no reviewable changes (1)
- iii-directory/skills/index.md
✅ Files skipped from review due to trivial changes (5)
- iii-directory/skill.md
- iii-directory/docs/intro.md
- iii-directory/docs/companions.md
- iii-directory/src/functions/registry.rs
- iii-directory/docs/quickstart.md
🚧 Files skipped from review as they are similar to previous changes (2)
- iii-directory/examples/test_registry.rs
- iii-directory/src/fs_source.rs
| ```bash | ||
| iii worker add iii-directory | ||
| ``` |
There was a problem hiding this comment.
Remove duplicate install command.
The iii worker add iii-directory command block is already present in the Install section (lines 13-15). This duplicate appears out of context after the configuration discussion and should be removed.
🧹 Proposed fix
`directory::skills::download` pulls bundles from the public workers registry (`api.workers.iii.dev` by default). For self-hosted setups, repoint `registry_url` in `config.yaml` at your own registry. The `directory::registry::*` proxies share their envelope with `directory::engine::workers::*`, so a single parser handles both local and remote worker discovery.
-```bash
-iii worker add iii-directory
-```
-
## Quickstart📝 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.
| ```bash | |
| iii worker add iii-directory | |
| ``` |
🤖 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 21 - 23, Remove the duplicated install
command block containing "iii worker add iii-directory" that appears after the
configuration discussion; locate the second occurrence of the fenced bash block
with that exact command and delete the entire code fence so the only remaining
instance is the one in the Install section, leaving the surrounding headers
(e.g., "## Quickstart") intact.
| And the index response body contains "Read [`iii://team-a/index`](iii://team-a/index) for the full worker reference." | ||
| And the index response body contains "Read [`iii://team-b/index`](iii://team-b/index) for the full worker reference." | ||
| And the index response body contains "Read [`team-a/index.md`](team-a/index.md) (legacy `iii://team-a/index`) for the full worker reference." | ||
| And the index response body contains "Read [`team-b/index.md`](team-b/index.md) (legacy `iii://team-b/index`) for the full worker reference." |
There was a problem hiding this comment.
can we have tests to ensure iii:// still works?
| @@ -1,67 +0,0 @@ | |||
| --- | |||
There was a problem hiding this comment.
why did remove this? this is critical
There was a problem hiding this comment.
It was not me, was the pipeline, see this commit:
6198786
Let me check why
There was a problem hiding this comment.
I see what happened. Since this project does not have a docs folder, the pipeline was never triggered, as soon i move the files, it generates this commit. This ci pipeline was added here
There was a problem hiding this comment.
let's disable this pipeline then, it cannot delete this md, it's important
…s::index Output now reads: Read [`<id>.md`](<id>.md) (legacy `iii://<id>`) for the full worker reference. Both forms travel together so harnesses that grep for the old URI scheme keep working alongside new consumers that prefer the markdown link target. The parser already accepted all three input forms (bare id, .md path, iii:// URI), so dual-emit closes the loop for back-compat with existing prompts and tooling. Updated unit test (render_index_emits_both_file_path_and_iii_pointer) and BDD scenario (read.feature) to assert both forms appear in the rendered body.
…input round-trip Adds explicit coverage for the legacy iii:// URI form on the input side and proves the dual-emit output round-trips: Unit tests (functions::skills): - normalize_iii_prefix_with_skills_md_aliases_to_index - normalize_iii_prefix_with_nested_skills_md_aliases_to_index - normalize_iii_prefix_round_trips_with_render_emitted_id BDD scenarios (read.feature): - directory::skills::get accepts the iii:// + SKILLS.md combined form - the legacy iii:// pointer emitted by skills::index round-trips through skills::get (proves the dual-emit output remains a valid get input)
…index.md skill-check ran iii-hq/skills-and-validation@v0.4 with write: true, which auto-committed a re-render that deletes any skills/ artifact the renderer classifies as stale. On this PR it removed iii-directory/skills/index.md (content was hand-maintained, not generated from docs/) and rewrote the README. Set write: false so the action runs in report-only mode: the sticky PR comment still surfaces diffs, but no commit lands automatically. The contents permission drops to read since the action no longer pushes.
guibeira
force-pushed
the
feat/iii-directory-support-global-skills
branch
from
b32ea08 to
92828f0
Compare
The skill-check action (iii-hq/skills-and-validation@v0.4) skips a worker entirely when `<worker>/docs/intro.md` is missing — that gates the renderer's drift detection. Without those partials, the action no longer races to re-render and remove skills/index.md or rewrite the README on every push. skill.md stays in tree (required by BOOTSTRAP_WORKERS in the worker validator) but loses the "DO NOT EDIT" header and the now-broken docs/* pointer. The content was carried over verbatim and updated to mention the dual-emit pointer behavior added in this PR.
| @@ -0,0 +1,11 @@ | |||
| # iii-directory | |||
There was a problem hiding this comment.
i dont think we need this
2 participants
Summary by CodeRabbit
Release Notes
New Features
.mdextension, and legacy URI format.SKILLS.mdfiles are now recognized as skill index aliases.Documentation
Tests
SKILLS.mdaliasing.What this PR does
Two coordinated changes in
iii-directory, with the old surface kept fully working:SKILLS.mdbecomes a namespace entry-point. The filesystem scanner (fs_source::rel_to_id) and thedirectory::skills::getinput parser (normalize_get_id) both alias the literal filenameSKILLS.mdtoindex.md, so a file at<ns>/SKILLS.mdproduces the id<ns>/index. The download path on the registry side was already byte-passthrough, so the new convention round-trips end to end without touching the registry.directory::skills::indexdual-emits the pointer. The rendered overview now readsRead [<ns>/index.md](<ns>/index.md) (legacy iii://<ns>/index). New consumers grab the markdown link target; harnesses that still grep foriii://keep finding it. The input parser accepts all three forms (<id>,<id>.md,iii://<id>), so any caller can use whichever it prefers.Function URIs in skill bodies (
iii://fn/<dotted>, inhow_to.rs) are deliberately out of scope, as they point at function ids, not files, so the file-path mapping does not apply.End-to-end demo
Publish a
SKILLS.mdpayload to the local registry, then run the bundled smoke example. It downloads the file, runsscan_skills, and asserts the id surfaces as<ns>/indexwith no uppercase leaks.Unit-test coverage
New tests in
fs_source(alias + duplicate handling) andfunctions::skills(parser + emitter dual-emit). Full lib suite is green (172/172).BDD coverage
Five new scenarios in
tests/features/read.featureexercise the alias onlist, the.mdpath form onget(includingiii://+.mdcomposition), and the dual-emit pointer in the renderedindexbody.Back-compat matrix
directory::skills::get { id: "hello/index" }directory::skills::get { id: "hello/index.md" }.mdstripped)directory::skills::get { id: "hello/SKILLS.md" }hello/index)directory::skills::get { id: "iii://hello/index" }directory::skills::get { id: "iii://hello/index.md" }directory::skills::indexoutputRead [iii://...](iii://...)Read [<id>.md](<id>.md) (legacy iii://<id>)— both forms emittedTest plan
cargo test --lib(172/172 green)cargo test --test bdd -- --tags @pure(pure scenarios green;@enginescenarios require a live engine and run in operator setups)cargo run --example test_registryagainst the local registry withSKILLS.mdpublished, smoke assertions passcargo fmt --all -- --checkclean