Add Skills feature: Custom Skills in Skills.md#111
Conversation
Add Skills feature: Custom Skills in Skills.md
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🚀 Thanks for opening a Pull Request! A maintainer will review this soon. Meanwhile: Your contribution helps make this project better! |
There was a problem hiding this comment.
Pull request overview
Adds a persistent “skills” layer to agent prompting via Skills.md, distinguishes it from task-specific guidance (Coderrr.md), and enforces a new prompt precedence order.
Changes:
- Load and prepend a persistent skills prompt from
Skills.mdinAgent.chat()before task guidance and user input. - Protect
Skills.mdfrom agent-driven deletion by adding it to protected paths. - Add unit tests for
Skills.mdloading and prompt ordering behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/agent.js |
Introduces skillsPrompt, loads Skills.md, and updates prompt construction ordering. |
src/fileOps.js |
Adds Skills.md to the protected paths list to prevent deletion. |
tests/unit/skills.test.js |
Adds unit tests for skills/task prompt loading and intended priority ordering. |
| const PROTECTED_PATHS = [ | ||
| 'Coderrr.md', | ||
| 'Skills.md', | ||
| '.coderrr' | ||
| ]; |
There was a problem hiding this comment.
isProtectedPath() checks protected filenames with a case-sensitive match (PROTECTED_PATHS.includes(basename)). On case-insensitive file systems (e.g., Windows/macOS default), a deletion request for skills.md can still resolve to Skills.md and bypass protection. Consider normalizing both basename/relativePath and PROTECTED_PATHS to a consistent case (e.g., lower-case) before comparing so the protection can't be bypassed by casing.
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| // Create a temporary directory for testing | ||
| tempDir = path.join(originalCwd, 'test-temp-skills'); | ||
| if (!fs.existsSync(tempDir)) { | ||
| fs.mkdirSync(tempDir, { recursive: true }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The test temp directory uses a fixed path (test-temp-skills) under the repo root. This can collide across parallel Jest runs and can leave artifacts in the working tree if a test crashes before cleanup. Prefer creating a unique temp dir via fs.mkdtempSync(path.join(os.tmpdir(), ...)) and clean it up with fs.rmSync(tempDir, { recursive: true, force: true }).
| // Simulate prompt construction (from chat method logic) | ||
| let enhancedPrompt = 'User request'; | ||
|
|
||
| // Task guidance first (will be wrapped by skills) | ||
| if (agent.customPrompt) { | ||
| enhancedPrompt = `[TASK GUIDANCE]\n${agent.customPrompt}\n\n[USER REQUEST]\n${enhancedPrompt}`; | ||
| } | ||
|
|
||
| // Skills prepended (comes before everything else) | ||
| if (agent.skillsPrompt) { | ||
| enhancedPrompt = `[SKILLS]\n${agent.skillsPrompt}\n\n${enhancedPrompt}`; | ||
| } | ||
|
|
||
| // Verify priority order: Skills comes first | ||
| expect(enhancedPrompt.startsWith('[SKILLS]')).toBe(true); | ||
| expect(enhancedPrompt.indexOf('[SKILLS]')).toBeLessThan(enhancedPrompt.indexOf('[TASK GUIDANCE]')); | ||
| expect(enhancedPrompt.indexOf('[TASK GUIDANCE]')).toBeLessThan(enhancedPrompt.indexOf('[USER REQUEST]')); |
There was a problem hiding this comment.
The prompt priority tests rebuild enhancedPrompt manually instead of exercising the production Agent.chat() logic. This duplicates implementation details and can keep passing even if chat() prompt construction changes. Consider calling agent.chat('User request', …) with scanOnFirstRequest: false and a mocked axios.post, then assert against the actual requestPayload.prompt passed to axios (order of [SKILLS], [TASK GUIDANCE], and [USER REQUEST]).
| loadSkillsPrompt() { | ||
| try { | ||
| const skillsPath = path.join(this.workingDir, 'Skills.md'); | ||
| if (fs.existsSync(skillsPath)) { | ||
| this.skillsPrompt = fs.readFileSync(skillsPath, 'utf8').trim(); | ||
| ui.info('Loaded skills from Skills.md'); | ||
| } |
There was a problem hiding this comment.
loadSkillsPrompt() logs "Loaded skills from Skills.md" even when the file is present but trims down to an empty string. Since chat() later checks if (this.skillsPrompt) (truthy) before prepending, an empty Skills.md results in a "loaded" log but no skills section in the prompt. Consider treating an all-whitespace file as absent (e.g., set skillsPrompt back to null and/or only log when the trimmed content is non-empty) to keep behavior consistent.
1 participant
This pull request introduces a new persistent skills prompt feature to the agent, allowing guidance from a
Skills.mdfile to be included in every conversation. It also clarifies the distinction between persistent skills (Skills.md) and task-specific guidance (Coderrr.md), and ensures the correct priority order when constructing prompts. Comprehensive unit tests are added to verify the loading and priority of these prompts. Additionally,Skills.mdis now protected from accidental deletion.Agent prompt enhancements:
Skills.md, which is prepended to every user request if present. The prompt construction now prioritizes: system prompt, then skills (Skills.md), then task guidance (Coderrr.md), then the user request. (src/agent.js) [1] [2] [3] [4]File protection:
Skills.mdto the list of protected files that cannot be deleted by the agent. (src/fileOps.js)Testing:
Skills.md, interaction withCoderrr.md, prompt construction order, and edge cases (file absence, empty file, etc.). (tests/unit/skills.test.js)