Enforce skills_dir requires strict: false#7
Conversation
- Add dependentRequired in schema: skills_dir needs strict to be present - Add check_strict_consistency validation in script - Update descriptions to clarify strict mode semantics Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Code auto-discovers skills in .claude/skills/ without needing strict: false or skills_dir. These fields are only needed when the marketplace must be the sole authority for plugin definition. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 17 minutes and 5 seconds. ⌛ 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: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe diff adds ARCHITECTURE.md and README updates; updates registry schema and registry.yaml; removes Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Issues
Security/CVEs: no specific CVEs are applicable to these config/schema edits. Focus is on input validation and authority-of-truth enforcement (CWE-20, CWE-284). 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ARCHITECTURE.md (1)
10-24: Add language specifiers to fenced code blocks.markdownlint flags code blocks without language specifiers (MD040). For ASCII diagrams, use
textorplaintext:♻️ Example fix for one block
-``` +```text skills-registry ┌──────────────────────────────────────────────────────┐Apply similarly to blocks at lines 28, 47, 72, 120, and 146.
Also applies to: 28-43, 47-65, 72-93, 120-139, 146-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ARCHITECTURE.md` around lines 10 - 24, The fenced ASCII diagram blocks (containing items like "skills-registry", "registry.yaml", "marketplace.json", "catalog.md", and the box/diagram art) need language specifiers to satisfy markdownlint MD040; update each triple-backtick fence for the diagrams to use a plain text specifier such as ```text (or ```plaintext) for the blocks shown and the other occurrences referenced (the blocks around the sections with "registry.yaml", "marketplace.json", "catalog.md" and the similar diagrams at the other locations), ensuring all code fences at the listed diagram locations include the language token.schema/registry.schema.json (1)
50-52: Schema constraint is structurally correct but semantically incomplete.
dependentRequiredonly enforces thatstrictmust be present whenskills_diris used—it doesn't enforcestrict: false. The Python validation incheck_strict_consistency()fills this gap.If you want full schema-level enforcement, an
if/then/elseblock could check that whenskills_diris present,strictmust equalfalse:♻️ Optional: Full schema-level enforcement
"dependentRequired": { "skills_dir": ["strict"] - }, + }, + "if": { + "required": ["skills_dir"] + }, + "then": { + "properties": { + "strict": { "const": false } + } + },Current two-layer approach (schema + Python) is acceptable and provides clearer error messages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schema/registry.schema.json` around lines 50 - 52, The JSON Schema uses "dependentRequired" to require presence of "strict" when "skills_dir" exists but doesn't enforce that "strict" must be false; update the schema by replacing/augmenting the "dependentRequired" rule with an if/then/else that checks presence of "skills_dir" in the instance and then requires "strict" to be false (i.e., in the then branch assert "strict": {"const": false}), and keep or remove the Python helper check_strict_consistency() accordingly to avoid duplicate validation; reference the existing "dependentRequired", "skills_dir", "strict" and the helper function name "check_strict_consistency()" when making the change so you enforce the semantic constraint entirely in the schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ARCHITECTURE.md`:
- Around line 10-24: The fenced ASCII diagram blocks (containing items like
"skills-registry", "registry.yaml", "marketplace.json", "catalog.md", and the
box/diagram art) need language specifiers to satisfy markdownlint MD040; update
each triple-backtick fence for the diagrams to use a plain text specifier such
as ```text (or ```plaintext) for the blocks shown and the other occurrences
referenced (the blocks around the sections with "registry.yaml",
"marketplace.json", "catalog.md" and the similar diagrams at the other
locations), ensuring all code fences at the listed diagram locations include the
language token.
In `@schema/registry.schema.json`:
- Around line 50-52: The JSON Schema uses "dependentRequired" to require
presence of "strict" when "skills_dir" exists but doesn't enforce that "strict"
must be false; update the schema by replacing/augmenting the "dependentRequired"
rule with an if/then/else that checks presence of "skills_dir" in the instance
and then requires "strict" to be false (i.e., in the then branch assert
"strict": {"const": false}), and keep or remove the Python helper
check_strict_consistency() accordingly to avoid duplicate validation; reference
the existing "dependentRequired", "skills_dir", "strict" and the helper function
name "check_strict_consistency()" when making the change so you enforce the
semantic constraint entirely in the schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: da9ac3c5-841f-4820-8129-486e02aa9f31
📒 Files selected for processing (5)
.claude-plugin/marketplace.jsonARCHITECTURE.mdregistry.yamlschema/registry.schema.jsonscripts/validate_registry.py
💤 Files with no reviewable changes (2)
- registry.yaml
- .claude-plugin/marketplace.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 64-66: The fenced code block containing the URL needs an explicit
language identifier to satisfy markdownlint MD040; update the block surrounding
the URL (the triple-backtick fenced block showing
"https://raw.githubusercontent.com/opendatahub-io/skills-registry/main/registry.yaml")
to include the language token `text` (i.e., replace ``` with ```text) so the
markdown linter recognizes the block language.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8459d7e5-ad79-4698-9504-9bc058eec611
📒 Files selected for processing (1)
README.md
| ``` | ||
| https://raw.githubusercontent.com/opendatahub-io/skills-registry/main/registry.yaml | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced block at Line 64 to satisfy markdownlint MD040.
Use an explicit language (text) for the URL block to keep lint clean.
Proposed fix
-```
+```text
https://raw.githubusercontent.com/opendatahub-io/skills-registry/main/registry.yaml</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 64-64: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 64 - 66, The fenced code block containing the URL
needs an explicit language identifier to satisfy markdownlint MD040; update the
block surrounding the URL (the triple-backtick fenced block showing
"https://raw.githubusercontent.com/opendatahub-io/skills-registry/main/registry.yaml")
to include the language token `text` (i.e., replace ``` with ```text) so the
markdown linter recognizes the block language.
astefanutti
force-pushed
the
enforce-strict-skills-dir
branch
from
2cae1b4 to
0aeecfe
Compare
Fixes regression from PR #7 which incorrectly removed these fields. Plugins without plugin.json require strict: false and skills_dir for Claude Code to discover skills when installed via marketplace. Auto-discovery only works locally, not for marketplace installs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes regression from PR #7 which incorrectly removed these fields. Plugins without plugin.json require strict: false and skills_dir for Claude Code to discover skills when installed via marketplace. Auto-discovery only works locally, not for marketplace installs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
skills_dirrequiresstrictto be present (dependentRequired)check_strict_consistencyvalidation thatskills_diris only used withstrict: falsestrict: falseandskills_dirfromrfe-creatorandtest-plan(Claude Code auto-discovers.claude/skills/)ARCHITECTURE.mddocumenting the registry architectureContext
strict: falsemeans the marketplace entry is the entire plugin definition. Whenstrict: true(default), Claude Code auto-discovers skills in default locations like.claude/skills/— noskills_dirneeded. Settingskills_dirwithoutstrict: falsehas no effect and is misleading.Test plan
python3 scripts/validate_registry.pypassesskills_dirwithoutstrict: falsefails validationmarketplace.jsonandcatalog.mdare in sync🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores