refactor(build_skills_payload): update skill resolution logic and add unit tests#215
Merged
Conversation
… unit tests - Changed the mapping of skill paths to keys in `build_skills_payload.py` to use "SKILL.md" instead of "index.md" for better clarity. - Updated the `_resolve_top_skill` function's docstring for accuracy. - Added a new test file `test_build_skills_payload.py` with unit tests for the `collect_skills` function, covering various scenarios including single skill, legacy index, nested skills, and empty markdown handling. - Removed outdated `index.md` and `coder.md` files, consolidating documentation into the new `SKILL.md` format. - Bumped `iii-directory` version to 0.8.0 in `Cargo.lock`.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMake SKILL.md the canonical worker overview key across payloads and registry output, add tests and registry dedupe logic, introduce publish workflows plus a CLI for worker input parsing, and update/relocate coder skill docs and console documentation. ChangesSkill Registry Migration to SKILL.md
Publish Workflows & CLI
Console Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 |
Contributor
skill-check — worker0 verified, 14 skipped (no docs/).
Four for four. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/build_skills_payload.py (1)
56-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNo fallback when the highest-priority overview file is empty.
_resolve_top_skillalways returns the highest-priority candidate (SKILL.md), even if it is whitespace-only. In that case_read_nonemptyreturnsNone, so no top overview is published — and becausecollect_skills(lines 84-85) also skipsskills_index, a non-empty legacyindex.mdfallback is silently dropped. During the migration, a placeholder/emptySKILL.mdalongside a realindex.mdwould lose the overview entirely.Consider resolving to the first non-empty candidate.
🛠️ Proposed fix
- winner_label, winner_path = present[0] - for label, _ in present[1:]: - print( - f"::warning::{worker_root.name}: both {label} and " - f"{winner_label} present; using {winner_label} as the top-of-tree." - ) - return _read_nonempty(winner_path), winner_path + for winner_label, winner_path in present: + body = _read_nonempty(winner_path) + if body is None: + continue + for label, _ in present: + if label != winner_label: + print( + f"::warning::{worker_root.name}: both {label} and " + f"{winner_label} present; using {winner_label} as the top-of-tree." + ) + return body, winner_path + return None, None🤖 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 @.github/scripts/build_skills_payload.py around lines 56 - 62, _resolve_top_skill currently chooses the highest-priority candidate (e.g., SKILL.md) even if its contents are empty (so _read_nonempty returns None) which prevents falling back to a non-empty legacy index.md; change _resolve_top_skill to iterate over the ordered present candidates (present list) and call _read_nonempty for each until it finds the first non-empty content, use that candidate as the winner (emit warnings only for lower-priority files when they exist), and return its content and path; this ensures collect_skills and skills_index will receive a non-empty overview by falling back from SKILL.md to index.md when appropriate.
🤖 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 @.github/workflows/database-e2e.yml:
- Around line 46-49: The "Install iii engine (next)" step is calling the
installer without the --next flag so it installs stable; update the run command
invoked in the "Install iii engine (next)" step to pass the next-channel flag to
the installer (for example invoke sh with -s -- --next or otherwise append
--next to the installer invocation) so the installer receives --next and
actually installs the next channel; ensure the change is applied to the run
block that currently pipes https://install.iii.dev/iii/main/install.sh into sh.
In `@coder/skills/SKILL.md`:
- Around line 16-17: Clarify the documented behavior of non_accessible_globs by
explicitly stating that matches are exposed in directory listings (e.g., shown
by coder::list-folder with non_accessible: true) but are treated as inaccessible
for direct operations (reads, writes, deletes) which return "not found" or
access-denied semantics; update the ambiguous sentences around
`non_accessible_globs`, the “denied path is folded with 'not found'” text, and
the example mentioning coder::list-folder so they consistently explain the
distinction (visible in listings with a non_accessible flag vs.
hidden/inaccessible for direct probing).
- Line 60: The sentence describing batched mutators and coder::update-file is
missing a terminal period; update the markdown in SKILL.md so the final sentence
ends with a period (add "." at the end of the sentence that begins "The batched
mutators return one result per input entry..." and references
coder::update-file, line ops, and atomic commit via temp file plus rename) to
correct punctuation and finish the sentence.
---
Outside diff comments:
In @.github/scripts/build_skills_payload.py:
- Around line 56-62: _resolve_top_skill currently chooses the highest-priority
candidate (e.g., SKILL.md) even if its contents are empty (so _read_nonempty
returns None) which prevents falling back to a non-empty legacy index.md; change
_resolve_top_skill to iterate over the ordered present candidates (present list)
and call _read_nonempty for each until it finds the first non-empty content, use
that candidate as the winner (emit warnings only for lower-priority files when
they exist), and return its content and path; this ensures collect_skills and
skills_index will receive a non-empty overview by falling back from SKILL.md to
index.md when appropriate.
🪄 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: e6434959-40ae-4f1c-9bcc-e2aa5e2f9f2f
⛔ Files ignored due to path filters (1)
iii-directory/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/scripts/build_skills_payload.py.github/scripts/test_build_skills_payload.py.github/workflows/database-e2e.ymlcoder/skills/SKILL.mdcoder/skills/coder.mdcoder/skills/index.mdiii-directory/skill.mdiii-directory/src/sources/registry.rs
💤 Files with no reviewable changes (3)
- iii-directory/skill.md
- coder/skills/index.md
- coder/skills/coder.md
Comment on lines
46
to
+49
| - name: Install iii engine (next) | ||
| run: | | ||
| curl -fsSL --retry 3 --retry-connrefused --retry-delay 5 \ | ||
| https://install.iii.dev/iii/main/install.sh | sh -s -- --next | ||
| https://install.iii.dev/iii/main/install.sh | sh |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
url="https://install.iii.dev/iii/main/install.sh"
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
curl -fsSL "$url" > "$tmp"
echo "Inspecting installer defaults from: $url"
grep -nE 'next|stable|channel|default|--next' "$tmp" || trueRepository: iii-hq/workers
Length of output: 1538
Fix “Install iii engine (next)” to actually install the “next” channel
The workflow runs https://install.iii.dev/iii/main/install.sh | sh without --next, but the installer defaults use_next=false and only switches to next when --next is provided—so CI will install the stable engine while the step is labeled “(next)”. Restore --next (e.g., pass -s -- --next to sh) or rename the step to match the installed channel.
🤖 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 @.github/workflows/database-e2e.yml around lines 46 - 49, The "Install iii
engine (next)" step is calling the installer without the --next flag so it
installs stable; update the run command invoked in the "Install iii engine
(next)" step to pass the next-channel flag to the installer (for example invoke
sh with -s -- --next or otherwise append --next to the installer invocation) so
the installer receives --next and actually installs the next channel; ensure the
change is applied to the run block that currently pipes
https://install.iii.dev/iii/main/install.sh into sh.
Comment on lines
+16
to
+17
| (`.env`, `*.pem`, anything under `secrets/`) visible to directory listings but | ||
| unreadable, unwritable, and unsearchable. |
There was a problem hiding this comment.
Clarify the visibility behavior of non_accessible_globs matches.
The documentation contains contradictory statements about whether non-accessible files are visible:
- Lines 16-17 state files are "visible to directory listings but unreadable, unwritable, and unsearchable"
- Line 53 confirms
coder::list-foldershows them with anon_accessible: trueflag - Lines 43-44 state "a denied path is folded with 'not found' so callers can't probe for its existence"
If non-accessible files appear in directory listings (as lines 16-17 and 53 indicate), then callers CAN probe for their existence by listing the directory. This contradicts the claim in lines 43-44 that denied paths are hidden to prevent probing.
Please clarify the intended behavior: Are non-accessible files visible in directory listings or completely hidden? If they're visible in listings but treated as "not found" for direct access operations (read/write/delete), this distinction should be explicitly stated.
Also applies to: 43-44, 53-54
🤖 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 `@coder/skills/SKILL.md` around lines 16 - 17, Clarify the documented behavior
of non_accessible_globs by explicitly stating that matches are exposed in
directory listings (e.g., shown by coder::list-folder with non_accessible: true)
but are treated as inaccessible for direct operations (reads, writes, deletes)
which return "not found" or access-denied semantics; update the ambiguous
sentences around `non_accessible_globs`, the “denied path is folded with 'not
found'” text, and the example mentioning coder::list-folder so they consistently
explain the distinction (visible in listings with a non_accessible flag vs.
hidden/inaccessible for direct probing).
| - `coder::update-file` — batched `insert` / `remove` / `update_lines` / regex `replace` ops across one or more files. | ||
| - `coder::delete-file` — batched removal; `recursive: true` is required for non-empty directories and missing paths are idempotent successes. | ||
|
|
||
| The batched mutators return one result per input entry so a single bad path never aborts the rest of the call, `coder::update-file` line ops are 1-based and inclusive and applied bottom-up so each op still references the caller's original line numbers, and every file commits atomically via a temp file plus rename. |
There was a problem hiding this comment.
Add missing period at end of sentence.
The final sentence is missing its closing punctuation.
📝 Proposed fix
-The batched mutators return one result per input entry so a single bad path never aborts the rest of the call, `coder::update-file` line ops are 1-based and inclusive and applied bottom-up so each op still references the caller's original line numbers, and every file commits atomically via a temp file plus rename.
+The batched mutators return one result per input entry so a single bad path never aborts the rest of the call, `coder::update-file` line ops are 1-based and inclusive and applied bottom-up so each op still references the caller's original line numbers, and every file commits atomically via a temp file plus rename.📝 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
| The batched mutators return one result per input entry so a single bad path never aborts the rest of the call, `coder::update-file` line ops are 1-based and inclusive and applied bottom-up so each op still references the caller's original line numbers, and every file commits atomically via a temp file plus rename. | |
| The batched mutators return one result per input entry so a single bad path never aborts the rest of the call, `coder::update-file` line ops are 1-based and inclusive and applied bottom-up so each op still references the caller's original line numbers, and every file commits atomically via a temp file plus rename. |
🤖 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 `@coder/skills/SKILL.md` at line 60, The sentence describing batched mutators
and coder::update-file is missing a terminal period; update the markdown in
SKILL.md so the final sentence ends with a period (add "." at the end of the
sentence that begins "The batched mutators return one result per input entry..."
and references coder::update-file, line ops, and atomic commit via temp file
plus rename) to correct punctuation and finish the sentence.
andersonleal
approved these changes
Jun 2, 2026
- Introduced a new workflow `_publish-worker-skills.yml` for publishing skills to the registry. - Created a `parse_publish_workers_input.py` script to parse worker inputs into a matrix format for GitHub Actions. - Added tests for the new parsing script in `test_parse_publish_workers_input.py`. - Implemented a `publish-worker-skills.yml` workflow to handle worker skills publishing via a workflow dispatch event. - Updated `create-tag.yml` to remove deprecated worker entries and ensure consistency across workflows.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/publish-worker-skills.yml (2)
30-30: ⚡ Quick winConsider disabling credential persistence.
The checkout action persists Git credentials by default. Since this workflow only reads the repo, consider adding
persist-credentials: false.🔒 Proposed security enhancement
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + persist-credentials: false🤖 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 @.github/workflows/publish-worker-skills.yml at line 30, The checkout step currently uses "uses: actions/checkout@v4" and persists Git credentials by default; update that checkout step to include a with block setting "persist-credentials: false" (i.e., modify the checkout step that references actions/checkout@v4 to add with: persist-credentials: false) so credentials are not stored in the runner, and confirm no downstream steps require the persisted credentials before making the change.
24-39: ⚡ Quick winAdd explicit permissions to the setup job.
The setup job uses default permissions. For better security posture, explicitly set
permissions: { contents: read }.🔒 Proposed security enhancement
setup: name: Parse workers runs-on: ubuntu-latest + permissions: + contents: read outputs: matrix: ${{ steps.parse.outputs.matrix }}🤖 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 @.github/workflows/publish-worker-skills.yml around lines 24 - 39, The GitHub Actions "setup" job (name: Parse workers) currently relies on default permissions; explicitly add a permissions block to the job with "contents: read" to tighten security. Update the job definition for the "setup" job (the job that defines outputs.matrix and runs the "Build worker matrix" step with id "parse") by inserting a top-level permissions: { contents: read } entry so the job only has read access to repository contents..github/workflows/_publish-worker-skills.yml (1)
30-30: ⚡ Quick winConsider disabling credential persistence.
The checkout action persists Git credentials by default, which could be exposed if later steps are compromised. Since this workflow only needs to read the repo and doesn't push changes, consider adding
persist-credentials: false.🔒 Proposed security enhancement
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + persist-credentials: false🤖 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 @.github/workflows/_publish-worker-skills.yml at line 30, The checkout step currently uses actions/checkout@v4 which persists Git credentials by default; update the checkout step to set persist-credentials: false so credentials are not stored for later steps (since the workflow only requires read access), ensuring the action invocation includes the persist-credentials: false option alongside uses: actions/checkout@v4.
🤖 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 `@console/docs/custom-function-call-message.md`:
- Line 79: Update the broken internal link fragments in the sentence containing
the two anchors: change `#11-scale-beyond-hand-wiring` to
`#12-scale-beyond-hand-wiring` and change
`#7-wire-the-family-into-functioncallmessagetsx` to
`#9-wire-the-family-into-functioncallmessagetsx` so the cross-references point
to the correct sections; locate the anchors in the same sentence that currently
reads "see [Scale beyond hand-wiring](`#11-scale-beyond-hand-wiring`) ... follow
the [minimal wiring](`#7-wire-the-family-into-functioncallmessagetsx`)" and update
them accordingly.
---
Nitpick comments:
In @.github/workflows/_publish-worker-skills.yml:
- Line 30: The checkout step currently uses actions/checkout@v4 which persists
Git credentials by default; update the checkout step to set persist-credentials:
false so credentials are not stored for later steps (since the workflow only
requires read access), ensuring the action invocation includes the
persist-credentials: false option alongside uses: actions/checkout@v4.
In @.github/workflows/publish-worker-skills.yml:
- Line 30: The checkout step currently uses "uses: actions/checkout@v4" and
persists Git credentials by default; update that checkout step to include a with
block setting "persist-credentials: false" (i.e., modify the checkout step that
references actions/checkout@v4 to add with: persist-credentials: false) so
credentials are not stored in the runner, and confirm no downstream steps
require the persisted credentials before making the change.
- Around line 24-39: The GitHub Actions "setup" job (name: Parse workers)
currently relies on default permissions; explicitly add a permissions block to
the job with "contents: read" to tighten security. Update the job definition for
the "setup" job (the job that defines outputs.matrix and runs the "Build worker
matrix" step with id "parse") by inserting a top-level permissions: { contents:
read } entry so the job only has read access to repository contents.
🪄 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: f5936170-b6a3-42c3-a7d8-e9688e7b878c
📒 Files selected for processing (7)
.github/scripts/parse_publish_workers_input.py.github/scripts/tests/test_parse_publish_workers_input.py.github/workflows/_publish-worker-skills.yml.github/workflows/create-tag.yml.github/workflows/publish-worker-skills.yml.github/workflows/release.ymlconsole/docs/custom-function-call-message.md
💤 Files with no reviewable changes (2)
- .github/workflows/create-tag.yml
- .github/workflows/release.yml
| : null | ||
| ``` | ||
|
|
||
| > **Note on scale.** Five families chained by hand is fine; it is explicit and tree-shakes cleanly. There is no plugin *registry* yet — see [Scale beyond hand-wiring](#11-scale-beyond-hand-wiring) for the refactor sketch if the chain grows unwieldy. Until then, follow the [minimal wiring](#7-wire-the-family-into-functioncallmessagetsx) below. |
There was a problem hiding this comment.
Fix broken internal link fragments.
Both cross-references on this line point to incorrect section numbers:
#11-scale-beyond-hand-wiringshould be#12-scale-beyond-hand-wiring(section 12 starts at line 518)#7-wire-the-family-into-functioncallmessagetsxshould be#9-wire-the-family-into-functioncallmessagetsx(section 9 starts at line 331)
🔗 Proposed fix
-> **Note on scale.** Five families chained by hand is fine; it is explicit and tree-shakes cleanly. There is no plugin *registry* yet — see [Scale beyond hand-wiring](`#11-scale-beyond-hand-wiring`) for the refactor sketch if the chain grows unwieldy. Until then, follow the [minimal wiring](`#7-wire-the-family-into-functioncallmessagetsx`) below.
+> **Note on scale.** Five families chained by hand is fine; it is explicit and tree-shakes cleanly. There is no plugin *registry* yet — see [Scale beyond hand-wiring](`#12-scale-beyond-hand-wiring`) for the refactor sketch if the chain grows unwieldy. Until then, follow the [minimal wiring](`#9-wire-the-family-into-functioncallmessagetsx`) below.📝 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
| > **Note on scale.** Five families chained by hand is fine; it is explicit and tree-shakes cleanly. There is no plugin *registry* yet — see [Scale beyond hand-wiring](#11-scale-beyond-hand-wiring) for the refactor sketch if the chain grows unwieldy. Until then, follow the [minimal wiring](#7-wire-the-family-into-functioncallmessagetsx) below. | |
| > **Note on scale.** Five families chained by hand is fine; it is explicit and tree-shakes cleanly. There is no plugin *registry* yet — see [Scale beyond hand-wiring](`#12-scale-beyond-hand-wiring`) for the refactor sketch if the chain grows unwieldy. Until then, follow the [minimal wiring](`#9-wire-the-family-into-functioncallmessagetsx`) below. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 79-79: Link fragments should be valid
(MD051, link-fragments)
[warning] 79-79: Link fragments should be valid
(MD051, link-fragments)
🤖 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 `@console/docs/custom-function-call-message.md` at line 79, Update the broken
internal link fragments in the sentence containing the two anchors: change
`#11-scale-beyond-hand-wiring` to `#12-scale-beyond-hand-wiring` and change
`#7-wire-the-family-into-functioncallmessagetsx` to
`#9-wire-the-family-into-functioncallmessagetsx` so the cross-references point
to the correct sections; locate the anchors in the same sentence that currently
reads "see [Scale beyond hand-wiring](`#11-scale-beyond-hand-wiring`) ... follow
the [minimal wiring](`#7-wire-the-family-into-functioncallmessagetsx`)" and update
them accordingly.
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
2 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.
build_skills_payload.pyto use "SKILL.md" instead of "index.md" for better clarity._resolve_top_skillfunction's docstring for accuracy.test_build_skills_payload.pywith unit tests for thecollect_skillsfunction, covering various scenarios including single skill, legacy index, nested skills, and empty markdown handling.index.mdandcoder.mdfiles, consolidating documentation into the newSKILL.mdformat.iii-directoryversion to 0.8.0 inCargo.lock.Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests