chore: simplify skills publishing pipeline#4448
Merged
Merged
Conversation
Removes the catalog-skills allowlist, exporter script, drift gate, and nightly refresh automation. Publishing a user-facing skill is now: copy .agents/skills/nemoclaw-user-<name>/ into skills/, commit, comment /nvskills-ci, merge. NVSkills CI signs one skill per PR (the existing timeout constraint), and skills/ itself is the source of truth for what is published. Drift between source (.agents/skills/) and published (skills/) is now verified on demand by asking an agent to diff the two trees, rather than enforced on every commit. The previous gate forced allowlist and export to stay in lockstep, which conflicts with NVSkills CI's per-skill signing cadence and required staged migration PRs to bypass the hook. Removed: - .agents/catalog-skills.yaml (allowlist) - scripts/export-catalog-skills.py (exporter + checker) - test/catalog-skills-export.test.ts (test for the exporter) - .github/workflows/catalog-skills-refresh.yaml (nightly refresh bot) - .github/pr-bodies/catalog-skills-refresh.md (bot PR body template) - .pre-commit-config.yaml catalog-skills-export hook - .github/workflows/pr.yaml "Verify catalog skills export" step - .github/CODEOWNERS entry for the deleted allowlist Replaced: - .github/catalog-skills-signing-flow.md rewritten as a 40-line manual workflow doc (cp -r, /nvskills-ci, merge). This unblocks the per-skill migration PRs (#4438 and the 9 to follow) that are currently blocked by the gate's allowlist-vs-export equality check. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
12 tasks
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoves the automated catalog skills export system (config, script, workflow, tests, PR body) and updates CI checks, pre-commit hooks, documentation, and CODEOWNERS to reflect a manual, comment-triggered signing flow using ChangesRemove catalog skills export automation
Sequence Diagram(s)sequenceDiagram
participant Maintainer
participant GitHubCommentListener
participant SigningCI
participant Repo
Maintainer->>Repo: add or refresh skill in `skills/` and open PR
Maintainer->>GitHubCommentListener: comment `/nvskills-ci`
GitHubCommentListener->>SigningCI: trigger signing workflow
SigningCI->>Repo: publish signed artifacts to `/.agents/skills/`
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 0 needs attention, 6 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/catalog-skills-signing-flow.md (1)
21-22: ⚡ Quick winConsider clarifying the one-skill-per-PR constraint.
The phrase "Repeat per skill — NVSkills CI signs one at a time" implies a sequential process but doesn't explicitly state that each skill requires its own PR. Based on the PR objectives stating "NVSkills CI signs one skill per PR," consider rewording for operational clarity.
📝 Suggested rewording
-Open the PR, comment `/nvskills-ci`, wait for the signing job to push back -`skill.oms.sig` and `skill-card.md`, then merge. Repeat per skill — NVSkills -CI signs one at a time. +Open a PR, comment `/nvskills-ci`, wait for the signing job to push back +`skill.oms.sig` and `skill-card.md`, then merge. NVSkills CI signs one skill +per PR; repeat this process for each additional skill.🤖 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/catalog-skills-signing-flow.md around lines 21 - 22, Update the sentence containing "skill.oms.sig and skill-card.md, then merge. Repeat per skill — NVSkills CI signs one at a time." to explicitly state the one-skill-per-PR constraint: mention that each skill must be submitted as a separate pull request and that NVSkills CI will sign only one skill per PR (e.g., "Create a separate PR for each skill; NVSkills CI will sign one skill per PR"). Ensure this replaces the ambiguous "Repeat per skill — NVSkills CI signs one at a time" phrasing so readers clearly understand the operational requirement.
🤖 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/catalog-skills-signing-flow.md:
- Line 37: Paths in the document are inconsistent: some entries use leading
slashes ("/.agents/skills/", "/skills/") while others use no leading slash
(".agents/skills/"); update all occurrences to a single consistent notation —
e.g., change "/.agents/skills/" and "/skills/" to ".agents/skills/" and
"skills/" (or vice versa if you prefer absolute paths) so every reference to the
agent and published skill directories uses the same form throughout the file.
---
Nitpick comments:
In @.github/catalog-skills-signing-flow.md:
- Around line 21-22: Update the sentence containing "skill.oms.sig and
skill-card.md, then merge. Repeat per skill — NVSkills CI signs one at a time."
to explicitly state the one-skill-per-PR constraint: mention that each skill
must be submitted as a separate pull request and that NVSkills CI will sign only
one skill per PR (e.g., "Create a separate PR for each skill; NVSkills CI will
sign one skill per PR"). Ensure this replaces the ambiguous "Repeat per skill —
NVSkills CI signs one at a time" phrasing so readers clearly understand the
operational requirement.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 94b4f824-b2f7-4dd1-a149-5118554bef01
📒 Files selected for processing (9)
.agents/catalog-skills.yaml.github/CODEOWNERS.github/catalog-skills-signing-flow.md.github/pr-bodies/catalog-skills-refresh.md.github/workflows/catalog-skills-refresh.yaml.github/workflows/pr.yaml.pre-commit-config.yamlscripts/export-catalog-skills.pytest/catalog-skills-export.test.ts
💤 Files with no reviewable changes (8)
- test/catalog-skills-export.test.ts
- .github/workflows/catalog-skills-refresh.yaml
- .agents/catalog-skills.yaml
- .github/CODEOWNERS
- .pre-commit-config.yaml
- scripts/export-catalog-skills.py
- .github/pr-bodies/catalog-skills-refresh.md
- .github/workflows/pr.yaml
| - `dry_run=false` to create or update `automation/catalog-skills-refresh`. | ||
| - `request_nvskills_ci=true` to attempt the `/nvskills-ci` comment after opening/updating the PR. | ||
| - scheduled no-op/refresh behavior using the same exporter. | ||
| Source (`/.agents/skills/`) and published (`/skills/`) can drift if a |
Contributor
There was a problem hiding this comment.
Fix path notation inconsistency.
Line 37 uses leading slashes (/.agents/skills/ and /skills/) while line 40 omits them (.agents/skills/). Use consistent notation throughout the document to avoid confusion about absolute vs. relative paths.
🔧 Proposed fix
-Source (`/.agents/skills/`) and published (`/skills/`) can drift if a
+Source (`.agents/skills/`) and published (`skills/`) can drift if a
source-side edit lands without a corresponding refresh PR. To check, ask📝 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
| Source (`/.agents/skills/`) and published (`/skills/`) can drift if a | |
| Source (`.agents/skills/`) and published (`skills/`) can drift if a |
🤖 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/catalog-skills-signing-flow.md at line 37, Paths in the document are
inconsistent: some entries use leading slashes ("/.agents/skills/", "/skills/")
while others use no leading slash (".agents/skills/"); update all occurrences to
a single consistent notation — e.g., change "/.agents/skills/" and "/skills/" to
".agents/skills/" and "skills/" (or vice versa if you prefer absolute paths) so
every reference to the agent and published skill directories uses the same form
throughout the file.
miyoungc
approved these changes
May 28, 2026
Address PR Review Advisor 'worth checking' finding on the new catalog-skills-signing-flow.md: - mkdir -p skills before initial cp so first publication works when the skills/ directory does not yet exist. - cp -R for portability (works identically to -r on GNU/macOS but is the POSIX-defined recursive flag). - git add -A skills/<name> in the refresh flow so newly added files in the refreshed skill copy are staged alongside the deletions picked up by 'git commit -a'. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
|
Actionable comments posted: 0 |
wscurran
added
enhancement: skill
This was referenced May 28, 2026
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
Removes the catalog-skills allowlist, exporter script, drift gate, and nightly refresh automation. Publishing a user-facing skill is now:
skills/is the source of truth for what is published. NVSkills CI signs one skill per PR (which is the existing timeout constraint anyway).Why
The current gate (
scripts/export-catalog-skills.py --check --allow-missing) requires.agents/catalog-skills.yamlandskills/to be in lockstep on every commit. NVSkills CI's per-skill signing cadence means we land published skills incrementally over many PRs, so allowlist and export are intentionally out of sync for the duration of the migration. The gate fights that workflow and forced #4438 to skip pre-commit hooks just to push.The system as a whole had three sources of truth (allowlist, source
.agents/skills/, exportedskills/) all needing to agree. Collapsing to two (source + published) eliminates the class of inconsistencies the gate was built to catch — because there's nothing left to be inconsistent.What replaces drift detection
On-demand verification: ask an agent to diff
skills/<name>/against.agents/skills/<name>/and report any content differences (ignoringskill.oms.sigandskill-card.md). Documented in the rewritten.github/catalog-skills-signing-flow.md.The nightly refresh bot's job (regenerating from source) is no longer meaningful since there's no allowlist driving regeneration — each published skill is now an explicit human decision per PR.
Removed
.agents/catalog-skills.yaml(allowlist)scripts/export-catalog-skills.py(exporter + checker, 460 lines)test/catalog-skills-export.test.ts(test for the deleted script).github/workflows/catalog-skills-refresh.yaml(nightly refresh bot).github/pr-bodies/catalog-skills-refresh.md(bot PR body template).pre-commit-config.yamlcatalog-skills-exporthook.github/workflows/pr.yaml"Verify catalog skills export" step.github/CODEOWNERSline for the deleted allowlistRewritten
.github/catalog-skills-signing-flow.md— was a Mermaid diagram of the seven-step generator/checker/refresh flow. Now a 40-line manual workflow doc.Downstream
#4438 (and the 9 per-skill PRs to follow) need to rebase onto this. After rebase, Miyoung's PR becomes a plain
cp -rof two skill directories with no manifest and no SPDX-stripping — strictly "as authored" content underskills/.Type of Change
Verification
Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
Chores
Documentation
Tests