feat: define skill placement and provenance policy#748
Conversation
|
Analysis Failed
Troubleshooting
Retry: |
📝 WalkthroughWalkthroughThis PR establishes a comprehensive skill placement and provenance policy framework by adding documentation, a JSON schema for provenance validation, and updating CI validators to enforce curated versus generated/imported skill distinctions across repository directories. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
Greptile SummaryThis PR establishes a formal skill placement and provenance policy by adding Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Skill] --> B{Classification}
B -->|Curated| C["skills/<name>/ (repo)"]
B -->|Learned| D["~/.claude/skills/learned/<name>/"]
B -->|Imported| E["~/.claude/skills/imported/<name>/"]
B -->|Evolved| F["~/.claude/homunculus/evolved/skills/<name>/"]
C --> G[No .provenance.json required]
C --> H[Validated by validate-skills.js]
C --> I[In install manifests]
C --> J[Shipped]
D --> K[.provenance.json required]
E --> K
F --> L[Provenance inherited from instinct source]
K --> M[validateProvenance in provenance.js]
M --> N[Schema: provenance.schema.json]
H --> O[validate-install-manifests.js checks all paths exist]
P[validate-commands.js] -->|Skip warn for| Q["reservedSkillRoots: learned, imported"]
P -->|Warn if missing| R[Other skill refs not in skills/]
Reviews (1): Last reviewed commit: "feat: define skill placement and provena..." | Re-trigger Greptile |
| | Curated | `skills/` (repo) | Yes | Not required | | ||
| | Learned | `~/.claude/skills/learned/` | No | Required | | ||
| | Imported | `~/.claude/skills/imported/` | No | Required | | ||
| | Evolved | `~/.claude/homunculus/evolved/skills/` (global) or `projects/<hash>/evolved/skills/` (per-project) | No | Inherits from instinct source | |
There was a problem hiding this comment.
The per-project path in the table is missing the ~/.claude/homunculus/ prefix. It currently reads projects/<hash>/evolved/skills/, but the detailed section below (line 45) correctly shows the full path as ~/.claude/homunculus/projects/<hash>/evolved/skills/. This inconsistency could cause contributors to create skills in the wrong location.
| | Evolved | `~/.claude/homunculus/evolved/skills/` (global) or `projects/<hash>/evolved/skills/` (per-project) | No | Inherits from instinct source | | |
| | Evolved | `~/.claude/homunculus/evolved/skills/` (global) or `~/.claude/homunculus/projects/<hash>/evolved/skills/` (per-project) | No | Inherits from instinct source | |
|
|
||
| // Check skill directory references (e.g., "skills/tdd-workflow/") | ||
| // learned and imported are reserved roots (~/.claude/skills/); no local dir expected | ||
| const reservedSkillRoots = new Set(['learned', 'imported']); |
There was a problem hiding this comment.
reservedSkillRoots instantiated on every loop iteration
reservedSkillRoots is declared inside the for (const file of files) loop, so a new Set is created for every command file processed. Since the set is constant and does not depend on any per-file state, it should be hoisted outside the loop so it is created only once.
| const reservedSkillRoots = new Set(['learned', 'imported']); | |
| const reservedSkillRoots = new Set(['learned', 'imported']); | |
| for (const file of files) { |
Then remove the inner declaration at line 103.
| } | ||
| }, | ||
| "required": ["source", "created_at", "confidence", "author"], | ||
| "additionalProperties": true |
There was a problem hiding this comment.
additionalProperties: true allows unvalidated fields silently
"additionalProperties": true is the JSON Schema default and means any extra or misspelled field (e.g. "confidance": 0.9) will pass schema validation without any error. Since this is a provenance schema with a fixed set of required fields, using false would make schema validation strictly enforce the contract and catch typos early.
| "additionalProperties": true | |
| "additionalProperties": false |
There was a problem hiding this comment.
3 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/SKILL-PLACEMENT-POLICY.md">
<violation number="1" location="docs/SKILL-PLACEMENT-POLICY.md:12">
P2: The policy document defines conflicting per-project evolved skill paths, which can cause skills to be read/written in the wrong location.</violation>
<violation number="2" location="docs/SKILL-PLACEMENT-POLICY.md:28">
P2: Documentation implies configurable learned skill location, but health/provenance scripts default to fixed `~/.claude/skills/learned` unless separately overridden, causing inconsistent behavior for custom paths.</violation>
<violation number="3" location="docs/SKILL-PLACEMENT-POLICY.md:31">
P2: Policy text claims provenance is currently mandatory, but the roadmap and code indicate enforcement is not yet implemented.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| Location: `~/.claude/skills/learned/<skill-name>/`. | ||
|
|
||
| Created by continuous-learning (evaluate-session hook, /learn command). Default path is configurable via `skills/continuous-learning/config.json` → `learned_skills_path`. |
There was a problem hiding this comment.
P2: Documentation implies configurable learned skill location, but health/provenance scripts default to fixed ~/.claude/skills/learned unless separately overridden, causing inconsistent behavior for custom paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/SKILL-PLACEMENT-POLICY.md, line 28:
<comment>Documentation implies configurable learned skill location, but health/provenance scripts default to fixed `~/.claude/skills/learned` unless separately overridden, causing inconsistent behavior for custom paths.</comment>
<file context>
@@ -0,0 +1,104 @@
+
+Location: `~/.claude/skills/learned/<skill-name>/`.
+
+Created by continuous-learning (evaluate-session hook, /learn command). Default path is configurable via `skills/continuous-learning/config.json` → `learned_skills_path`.
+
+- Not in repo. Not shipped.
</file context>
| Created by continuous-learning (evaluate-session hook, /learn command). Default path is configurable via `skills/continuous-learning/config.json` → `learned_skills_path`. | |
| Created by continuous-learning (evaluate-session hook, /learn command). Write path is configurable via `skills/continuous-learning/config.json` → `learned_skills_path`; note that health/provenance tooling defaults to `~/.claude/skills/learned` unless `--learned-root`/`--home` overrides are provided. |
| Created by continuous-learning (evaluate-session hook, /learn command). Default path is configurable via `skills/continuous-learning/config.json` → `learned_skills_path`. | ||
|
|
||
| - Not in repo. Not shipped. | ||
| - Must have `.provenance.json` sibling to `SKILL.md`. |
There was a problem hiding this comment.
P2: Policy text claims provenance is currently mandatory, but the roadmap and code indicate enforcement is not yet implemented.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/SKILL-PLACEMENT-POLICY.md, line 31:
<comment>Policy text claims provenance is currently mandatory, but the roadmap and code indicate enforcement is not yet implemented.</comment>
<file context>
@@ -0,0 +1,104 @@
+Created by continuous-learning (evaluate-session hook, /learn command). Default path is configurable via `skills/continuous-learning/config.json` → `learned_skills_path`.
+
+- Not in repo. Not shipped.
+- Must have `.provenance.json` sibling to `SKILL.md`.
+- Loaded at runtime when directory exists.
+
</file context>
| | Curated | `skills/` (repo) | Yes | Not required | | ||
| | Learned | `~/.claude/skills/learned/` | No | Required | | ||
| | Imported | `~/.claude/skills/imported/` | No | Required | | ||
| | Evolved | `~/.claude/homunculus/evolved/skills/` (global) or `projects/<hash>/evolved/skills/` (per-project) | No | Inherits from instinct source | |
There was a problem hiding this comment.
P2: The policy document defines conflicting per-project evolved skill paths, which can cause skills to be read/written in the wrong location.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/SKILL-PLACEMENT-POLICY.md, line 12:
<comment>The policy document defines conflicting per-project evolved skill paths, which can cause skills to be read/written in the wrong location.</comment>
<file context>
@@ -0,0 +1,104 @@
+| Curated | `skills/` (repo) | Yes | Not required |
+| Learned | `~/.claude/skills/learned/` | No | Required |
+| Imported | `~/.claude/skills/imported/` | No | Required |
+| Evolved | `~/.claude/homunculus/evolved/skills/` (global) or `projects/<hash>/evolved/skills/` (per-project) | No | Inherits from instinct source |
+
+Curated skills live in the repo under `skills/`. Install manifests reference only curated paths. Generated and imported skills live under the user home directory and are never shipped.
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/SKILL-PLACEMENT-POLICY.md`:
- Line 12: The per-project evolved skill path is inconsistent: update the entry
that currently shows `projects/<hash>/evolved/skills/` so it matches the other
section by including the `~/.claude/homunculus/` prefix (i.e., use
`~/.claude/homunculus/projects/<hash>/evolved/skills/`), and make the same
change wherever the shorter form appears (ensure both occurrences are consistent
with the `~/.claude/homunculus/evolved/skills/` global path).
- Line 65: The docs claim that validateProvenance performs schema-based
validation but the function in scripts/lib/skill-evolution/provenance.js
currently uses hard-coded checks; either update validateProvenance to load and
apply schemas/provenance.schema.json (e.g., import/require the JSON schema,
compile it with your project's validator like Ajv, replace the manual property
checks in validateProvenance with a schema validate/validate.errors flow and
return/report those errors) or change the doc line to state that
validateProvenance uses explicit hard-coded checks rather than schema
compilation; reference validateProvenance and schemas/provenance.schema.json
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77ddaca4-590a-44f6-a0f9-b417a67134c1
📒 Files selected for processing (6)
CLAUDE.mddocs/SKILL-PLACEMENT-POLICY.mdschemas/provenance.schema.jsonscripts/ci/validate-commands.jsscripts/ci/validate-install-manifests.jsscripts/ci/validate-skills.js
| | Curated | `skills/` (repo) | Yes | Not required | | ||
| | Learned | `~/.claude/skills/learned/` | No | Required | | ||
| | Imported | `~/.claude/skills/imported/` | No | Required | | ||
| | Evolved | `~/.claude/homunculus/evolved/skills/` (global) or `projects/<hash>/evolved/skills/` (per-project) | No | Inherits from instinct source | |
There was a problem hiding this comment.
Per-project evolved path is inconsistent across sections.
Line 12 omits the ~/.claude/homunculus/ prefix that Line 45 includes, which can mislead path resolution.
Suggested doc fix
-| Evolved | `~/.claude/homunculus/evolved/skills/` (global) or `projects/<hash>/evolved/skills/` (per-project) | No | Inherits from instinct source |
+| Evolved | `~/.claude/homunculus/evolved/skills/` (global) or `~/.claude/homunculus/projects/<hash>/evolved/skills/` (per-project) | No | Inherits from instinct source |Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/SKILL-PLACEMENT-POLICY.md` at line 12, The per-project evolved skill
path is inconsistent: update the entry that currently shows
`projects/<hash>/evolved/skills/` so it matches the other section by including
the `~/.claude/homunculus/` prefix (i.e., use
`~/.claude/homunculus/projects/<hash>/evolved/skills/`), and make the same
change wherever the shorter form appears (ensure both occurrences are consistent
with the `~/.claude/homunculus/evolved/skills/` global path).
| | confidence | number | 0–1 | | ||
| | author | string | Who or what produced the skill | | ||
|
|
||
| Schema: `schemas/provenance.schema.json`. Validation: `scripts/lib/skill-evolution/provenance.js` → `validateProvenance`. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether provenance validator actually loads/uses the schema file.
rg -n "provenance\\.schema\\.json|validateProvenance|Ajv|compile\\(" \
scripts/lib/skill-evolution/provenance.js \
schemas/provenance.schema.json \
docs/SKILL-PLACEMENT-POLICY.mdRepository: affaan-m/everything-claude-code
Length of output: 476
🏁 Script executed:
cat -n scripts/lib/skill-evolution/provenance.js | sed -n '98,130p'Repository: affaan-m/everything-claude-code
Length of output: 1218
🏁 Script executed:
ls -la schemas/provenance.schema.json 2>/dev/null || echo "Schema file not found"Repository: affaan-m/everything-claude-code
Length of output: 153
🏁 Script executed:
head -50 schemas/provenance.schema.json 2>/dev/null || echo "Schema file not readable"Repository: affaan-m/everything-claude-code
Length of output: 952
Validation claim is stronger than current implementation.
This line states schema-based validation is performed by validateProvenance, but the function uses hard-coded checks instead of loading and applying schemas/provenance.schema.json. The validation rules in the code (lines 98-130) mirror the schema requirements, but the implementation directly validates properties rather than compiling and using the schema. Wire schema validation into validateProvenance or reword this line to accurately describe the validation approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/SKILL-PLACEMENT-POLICY.md` at line 65, The docs claim that
validateProvenance performs schema-based validation but the function in
scripts/lib/skill-evolution/provenance.js currently uses hard-coded checks;
either update validateProvenance to load and apply
schemas/provenance.schema.json (e.g., import/require the JSON schema, compile it
with your project's validator like Ajv, replace the manual property checks in
validateProvenance with a schema validate/validate.errors flow and return/report
those errors) or change the doc line to state that validateProvenance uses
explicit hard-coded checks rather than schema compilation; reference
validateProvenance and schemas/provenance.schema.json when making the change.
|
Interesting addition. Will review. 🦞 |
codeCraft-Ritik
left a comment
There was a problem hiding this comment.
Excellent contribution! The addition of a clear skill placement policy brings much-needed structure to the project
fix : #422
What Changed
Why This Change
Testing Done
Type of Change
[x] feat: New feature
[x] docs: Documentation
[x] ci: CI/CD changes
Security & Quality Checklist
[x] No secrets or API keys committed (ghp_, sk-, AKIA, xoxb, xoxp patterns checked)
[x] JSON files validate cleanly
[x] No sensitive data exposed in logs or output
[x] Follows conventional commits format
Documentation
[x] Updated relevant documentation
[x] Added comments for complex logic
Summary by CodeRabbit