Skip to content

feat(Skills manager): added skills manager backend#1077

Open
gastoner wants to merge 2 commits intokortex-hub:mainfrom
gastoner:skills_manager
Open

feat(Skills manager): added skills manager backend#1077
gastoner wants to merge 2 commits intokortex-hub:mainfrom
gastoner:skills_manager

Conversation

@gastoner
Copy link
Contributor

@gastoner gastoner commented Mar 6, 2026

Added skills manager that stores the route to skill folders in config

Creates skill directory in kortex home dir

Allows to:

  • registerSkill(folderPath) — Register an existing skill by pointing to a folder that contains a SKILL.md. Parses the frontmatter to extract metadata, validates it, and persists the registration.
  • createSkill(options) — Create a new skill from scratch by providing name, description, and content. Generates the folder and SKILL.md file in the app's skills directory, then registers it.
  • unregisterSkill(name) — Remove a skill from the registry by name.
  • listSkills() — List all registered skills (returns name, description, content, path for each).
  • getSkillContent(name) — Read and return the full SKILL.md content from disk for a given skill.
  • listSkillFolderContent(name) — List all files/directories inside a skill's folder.

Closes #1033

Added tests for the skill-manager

@gastoner gastoner changed the title feat(Skills manager): added skills manager feat(Skills manager): added skills manager backend Mar 6, 2026
@gastoner gastoner marked this pull request as ready for review March 6, 2026 20:37
@gastoner gastoner requested a review from a team as a code owner March 6, 2026 20:37
@gastoner gastoner requested review from MarsKubeX, benoitf and bmahabirbu and removed request for a team March 6, 2026 20:37
@coderabbitai
Copy link

coderabbitai bot commented Mar 6, 2026

📝 Walkthrough

Walkthrough

Adds skill management: data contracts, directory accessors, a filesystem-backed SkillManager with IPC wiring and DI registration, preload APIs, and comprehensive unit tests. No changes outside skill-management surfaces.

Changes

Cohort / File(s) Summary
Skill Data Contracts
packages/api/src/skill/skill-info.ts
Adds SkillInfo, SkillMetadata, SkillCreateOptions interfaces and constants SKILL_SECTION, SKILL_ENABLED, SKILL_FILE_NAME.
Directory Management
packages/main/src/plugin/directories.ts, packages/main/src/plugin/directories-legacy.ts, packages/main/src/plugin/directories-linux-xdg.ts
Adds getSkillsDirectory(): string to the Directories interface and implementations; stores skills directory path.
SkillManager Implementation
packages/main/src/plugin/skill/skill-manager.ts
New SkillManager class: discovery, SKILL.md parsing/validation, register/create/enable/disable/unregister, content listing, config persistence, IPC handlers, and lifecycle disposal.
SkillManager Tests
packages/main/src/plugin/skill/skill-manager.spec.ts
Comprehensive unit tests with filesystem/config/IPC mocks covering init, discovery, parsing, registration, creation, enable/disable, unregistration, listing, content access, and config persistence.
Plugin System Integration
packages/main/src/plugin/index.ts
Binds SkillManager as a singleton in DI container and initializes it during plugin startup (retrieval and init() calls).
Preload API Exposure
packages/preload/src/index.ts
Exposes preload APIs that invoke IPC channels: listSkills, registerSkill, disableSkill, enableSkill, unregisterSkill, createSkill, getSkillContent, listSkillFolderContent (imports SkillInfo/SkillCreateOptions).

Sequence Diagram

sequenceDiagram
    participant Renderer as Renderer
    participant Preload as Preload API
    participant IPC as IPC / Main
    participant SkillMgr as SkillManager
    participant FS as FileSystem
    participant Config as Config Registry

    Renderer->>Preload: registerSkill(folderPath)
    Preload->>IPC: invoke skill-manager:registerSkill
    IPC->>SkillMgr: registerSkill(folderPath)
    SkillMgr->>FS: read SKILL.md from source
    FS-->>SkillMgr: SKILL.md content
    SkillMgr->>SkillMgr: parse & validate frontmatter
    SkillMgr->>FS: copy folder -> managed skills directory
    FS-->>SkillMgr: copy complete
    SkillMgr->>Config: update enabled skill names
    Config-->>SkillMgr: persist ack
    SkillMgr->>IPC: post skill update notification
    IPC-->>Preload: return SkillInfo
    Preload-->>Renderer: SkillInfo
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding a skills manager backend component with full lifecycle management capabilities.
Description check ✅ Passed The description clearly relates to the changeset, detailing the skills manager implementation, its operations, and references the closed issue #1033.
Linked Issues check ✅ Passed The PR fully implements issue #1033 requirements: skill manager backend with name/path/description attributes, registration, creation, and lifecycle management capabilities.
Out of Scope Changes check ✅ Passed All changes are in-scope: skill manager implementation, directory configuration, IPC APIs, and comprehensive tests directly address issue #1033 objectives.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/main/src/plugin/directories-linux-xdg.ts (1)

42-62: ⚠️ Potential issue | 🔴 Critical

registerSkill() must call mkdir before cp to ensure skills directory exists.

createSkill() safely calls mkdir(skillDir, { recursive: true }) before writing (line 239). However, registerSkill() calls cp(resolvedPath, targetDir, { recursive: true }) without explicitly creating the parent skillsDir beforehand (line 205). Node.js cp does not auto-create parent directories; it only copies recursively. On a clean XDG profile, the first call to registerSkill() to register an external skill will fail because skillsDir does not exist.

Add await mkdir(skillsDir, { recursive: true }); before the cp call in registerSkill() at line 204.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/main/src/plugin/directories-linux-xdg.ts` around lines 42 - 62,
registerSkill() can fail on a fresh XDG profile because it calls
cp(resolvedPath, targetDir, { recursive: true }) without ensuring the parent
skills directory exists; add an explicit await mkdir(skillsDir, { recursive:
true }) (or await mkdir(this.skillsDirectory, { recursive: true }) depending on
local variable names) immediately before the cp call in registerSkill(),
mirroring createSkill() which already calls mkdir(skillDir, { recursive: true
}), so the skillsDirectory/skillsDir is created before copying.
🧹 Nitpick comments (4)
packages/api/src/skill/skill-info.ts (1)

19-23: Separate list metadata from full skill content.

Including content in the base SkillInfo contract makes listSkills() load and IPC-transfer every SKILL.md, even though getSkillContent(name) already exists for the detailed read path.

♻️ Suggested contract split
-export interface SkillInfo extends SkillMetadata {
-  content: string;
+export interface SkillSummary extends SkillMetadata {
   path: string;
   enabled: boolean;
 }
+
+export interface SkillInfo extends SkillSummary {
+  content: string;
+}

Then keep listSkills() on SkillSummary[] and reserve SkillInfo/getSkillContent() for detail views.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/api/src/skill/skill-info.ts` around lines 19 - 23, Extract content
out of the base SkillInfo contract by introducing a new SkillSummary interface
(e.g., interface SkillSummary extends SkillMetadata { path: string; enabled:
boolean }) and keep SkillInfo as the detailed contract that includes content
(e.g., interface SkillInfo extends SkillMetadata { content: string; path:
string; enabled: boolean }); then change listSkills() to return SkillSummary[]
and leave getSkillContent(name) / any code that needs the full SKILL.md to use
SkillInfo/getSkillContent, updating any type annotations and IPC payloads to use
SkillSummary for list operations and SkillInfo only for detail fetches.
packages/main/src/plugin/skill/skill-manager.ts (2)

394-398: Consider using for...of loop instead of forEach to avoid callback return value issue.

The forEach callback should not return a value. If dispose() returns something, it's being silently ignored. Using a for...of loop is cleaner and avoids this lint warning.

Suggested fix
   `@preDestroy`()
   dispose(): void {
-    this.disposables.forEach(d => d.dispose());
+    for (const d of this.disposables) {
+      d.dispose();
+    }
     this.disposables = [];
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/main/src/plugin/skill/skill-manager.ts` around lines 394 - 398, The
dispose method currently calls this.disposables.forEach(d => d.dispose()), which
silences any return value from individual disposables and triggers the lint
warning; change it to iterate with a for...of loop over this.disposables (e.g.,
for (const d of this.disposables) { d.dispose(); }) to ensure any return value
is not discarded and to satisfy the linter while keeping the method semantics
(then clear this.disposables = [] as before).

330-330: Add explicit type annotation to avoid implicit any.

The entries variable lacks a type annotation, resulting in implicit any. This was flagged by static analysis.

Suggested fix
-    let entries;
+    let entries: Awaited<ReturnType<typeof readdir>> | undefined;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/main/src/plugin/skill/skill-manager.ts` at line 330, The variable
`entries` in skill-manager.ts is declared without a type causing implicit `any`;
locate the `let entries;` declaration and give it an explicit type that matches
how it's later used (for example the result of Object.entries => `[string,
Skill][]`, or a `Record<string, Skill>` or specific array/tuple type used by
methods around `SkillManager`), update the declaration to that type and ensure
any subsequent assignments conform to it so static analysis errors are resolved.
packages/main/src/plugin/skill/skill-manager.spec.ts (1)

267-267: Use regular strings instead of template literals when interpolation is not needed.

Lines 267 and 277 use template literals without any interpolation. These can be simplified to regular strings.

Suggested fix
-  vi.mocked(readFile).mockResolvedValue(`---\nname: My_Skill\ndescription: Valid\n---\n`);
+  vi.mocked(readFile).mockResolvedValue('---\nname: My_Skill\ndescription: Valid\n---\n');
-  vi.mocked(readFile).mockResolvedValue(`---\nname: my-claude-skill\ndescription: Valid\n---\n`);
+  vi.mocked(readFile).mockResolvedValue('---\nname: my-claude-skill\ndescription: Valid\n---\n');

Also applies to: 277-277

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/main/src/plugin/skill/skill-manager.spec.ts` at line 267, Replace
unnecessary template literals used in the test mocks with plain string literals:
locate the vi.mocked(readFile).mockResolvedValue(...) calls in
skill-manager.spec.ts (the two occurrences using backtick-delimited strings) and
change them to use regular quoted strings (e.g., "...\n...") since there is no
interpolation; this applies to the mockResolvedValue calls for readFile at both
places around the earlier reported lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 241-242: The code in skill-manager.ts builds YAML frontmatter via
string interpolation (fileContent using metadata.name, metadata.description, and
options.content), which can break when description or other fields contain
colons, quotes, or newlines; replace the inline string assembly with a proper
YAML serialization: create a frontmatter object (e.g., { name: metadata.name,
description: metadata.description, ... }), use js-yaml's dump() to serialize it
safely, then concatenate the dumped YAML between the '---' markers and append
options.content before calling writeFile to SKILL_FILE_NAME so the resulting
file is valid for parsing.

---

Outside diff comments:
In `@packages/main/src/plugin/directories-linux-xdg.ts`:
- Around line 42-62: registerSkill() can fail on a fresh XDG profile because it
calls cp(resolvedPath, targetDir, { recursive: true }) without ensuring the
parent skills directory exists; add an explicit await mkdir(skillsDir, {
recursive: true }) (or await mkdir(this.skillsDirectory, { recursive: true })
depending on local variable names) immediately before the cp call in
registerSkill(), mirroring createSkill() which already calls mkdir(skillDir, {
recursive: true }), so the skillsDirectory/skillsDir is created before copying.

---

Nitpick comments:
In `@packages/api/src/skill/skill-info.ts`:
- Around line 19-23: Extract content out of the base SkillInfo contract by
introducing a new SkillSummary interface (e.g., interface SkillSummary extends
SkillMetadata { path: string; enabled: boolean }) and keep SkillInfo as the
detailed contract that includes content (e.g., interface SkillInfo extends
SkillMetadata { content: string; path: string; enabled: boolean }); then change
listSkills() to return SkillSummary[] and leave getSkillContent(name) / any code
that needs the full SKILL.md to use SkillInfo/getSkillContent, updating any type
annotations and IPC payloads to use SkillSummary for list operations and
SkillInfo only for detail fetches.

In `@packages/main/src/plugin/skill/skill-manager.spec.ts`:
- Line 267: Replace unnecessary template literals used in the test mocks with
plain string literals: locate the vi.mocked(readFile).mockResolvedValue(...)
calls in skill-manager.spec.ts (the two occurrences using backtick-delimited
strings) and change them to use regular quoted strings (e.g., "...\n...") since
there is no interpolation; this applies to the mockResolvedValue calls for
readFile at both places around the earlier reported lines.

In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 394-398: The dispose method currently calls
this.disposables.forEach(d => d.dispose()), which silences any return value from
individual disposables and triggers the lint warning; change it to iterate with
a for...of loop over this.disposables (e.g., for (const d of this.disposables) {
d.dispose(); }) to ensure any return value is not discarded and to satisfy the
linter while keeping the method semantics (then clear this.disposables = [] as
before).
- Line 330: The variable `entries` in skill-manager.ts is declared without a
type causing implicit `any`; locate the `let entries;` declaration and give it
an explicit type that matches how it's later used (for example the result of
Object.entries => `[string, Skill][]`, or a `Record<string, Skill>` or specific
array/tuple type used by methods around `SkillManager`), update the declaration
to that type and ensure any subsequent assignments conform to it so static
analysis errors are resolved.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 581326eb-3092-4e31-88de-4ac87e7ec78b

📥 Commits

Reviewing files that changed from the base of the PR and between fd1f158 and df50843.

📒 Files selected for processing (8)
  • packages/api/src/skill/skill-info.ts
  • packages/main/src/plugin/directories-legacy.ts
  • packages/main/src/plugin/directories-linux-xdg.ts
  • packages/main/src/plugin/directories.ts
  • packages/main/src/plugin/index.ts
  • packages/main/src/plugin/skill/skill-manager.spec.ts
  • packages/main/src/plugin/skill/skill-manager.ts
  • packages/preload/src/index.ts

Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
packages/main/src/plugin/skill/skill-manager.ts (1)

241-242: ⚠️ Potential issue | 🟠 Major

Unsafe YAML emission can make a newly created skill unreadable.

Line 241 interpolates raw description text into YAML. Inputs containing : , quotes, or newlines can produce invalid frontmatter, so createSkill() can succeed and still write a SKILL.md that parseSkillFile() cannot read back reliably. Please serialize/escape the frontmatter instead of concatenating it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/main/src/plugin/skill/skill-manager.ts` around lines 241 - 242, The
frontmatter is built by raw string interpolation (metadata.name /
metadata.description) which can produce invalid YAML; in createSkill() in
skill-manager.ts (where SKILL_FILE_NAME is written) replace the manual template
with a proper YAML serialization of the metadata (e.g., use a YAML emitter such
as js-yaml's safeDump/dump) and then append the markdown body, so
parseSkillFile() can reliably read the file; ensure you serialize the entire
metadata object (not just description) and preserve the separation
(---\n<yaml>\n---\n\n<content>).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 359-371: The code currently treats any skill as disabled when
there are any entries in enabledNames unless its name is present, which makes
newly discovered folders appear disabled; change the logic in the skill
discovery block (around isEnabled, enabledNames, this.skills,
newSkillDiscovered, metadata, folderPath) to distinguish between "known" skills
and truly disabled ones: check enabledNames.has(metadata.name) to decide
explicit disabled/enabled, and if the name is not present at all treat it as a
new/unknown skill and default enabled = true (and mark newSkillDiscovered), then
add the skill name to the persisted known/enabled map (or maintain a separate
persisted knownSkills set or enabledState map) so future discoveries are not
treated as disabled; ensure persistence is updated after adding the name.
- Around line 152-156: The metadata.name validation currently allows ":" but
that value is later used as a filesystem folder name (via mkdir/cp), which fails
on Windows; either reject ":" at validation or sanitize when creating
directories. Fix by tightening the validation for metadata.name (e.g., change
the regex to disallow colons and other OS-reserved characters such as <>:"/\\|?*
and update the error message) or, if you prefer to allow ":" logically, ensure
every place that derives a filesystem path from metadata.name (the code that
calls mkdir and cp using metadata.name) maps/sanitizes the name to a safe folder
name (e.g., replace ":" with "-" or use a slugify function) before filesystem
operations; update both the validation/regex around metadata.name and the code
paths that construct filesystem paths so they are consistent.
- Around line 80-109: The IPC handlers for the skill-manager channels are
currently registered inside the SkillManager class; move those ipcHandle
registrations into the central plugin index module and have them delegate to the
SkillManager instance instead (i.e., in plugin index call
ipcHandle('skill-manager:listSkills', () => skillManager.listSkills()),
ipcHandle('skill-manager:registerSkill', (_e, p) =>
skillManager.registerSkill(p)), and likewise for disableSkill, enableSkill,
unregisterSkill, createSkill, getSkillContent, and listSkillFolderContent) so
SkillManager only exposes methods (listSkills, registerSkill, disableSkill,
enableSkill, unregisterSkill, createSkill, getSkillContent,
listSkillFolderContent) and all ipcHandle(...) calls live in the plugin index
using the shared SkillManager instance.

---

Duplicate comments:
In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 241-242: The frontmatter is built by raw string interpolation
(metadata.name / metadata.description) which can produce invalid YAML; in
createSkill() in skill-manager.ts (where SKILL_FILE_NAME is written) replace the
manual template with a proper YAML serialization of the metadata (e.g., use a
YAML emitter such as js-yaml's safeDump/dump) and then append the markdown body,
so parseSkillFile() can reliably read the file; ensure you serialize the entire
metadata object (not just description) and preserve the separation
(---\n<yaml>\n---\n\n<content>).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c38f9874-13bc-4f7a-b206-8564e6de99ed

📥 Commits

Reviewing files that changed from the base of the PR and between df50843 and 2e4b2d5.

📒 Files selected for processing (8)
  • packages/api/src/skill/skill-info.ts
  • packages/main/src/plugin/directories-legacy.ts
  • packages/main/src/plugin/directories-linux-xdg.ts
  • packages/main/src/plugin/directories.ts
  • packages/main/src/plugin/index.ts
  • packages/main/src/plugin/skill/skill-manager.spec.ts
  • packages/main/src/plugin/skill/skill-manager.ts
  • packages/preload/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/preload/src/index.ts
  • packages/main/src/plugin/directories-linux-xdg.ts

Comment on lines +359 to +371
const isEnabled = enabledNames.has(metadata.name);

this.skills = [
...this.skills,
{
...metadata,
path: folderPath,
enabled: isEnabled || !enabledNames.size,
},
];

if (!isEnabled) {
newSkillDiscovered = true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

New folders are treated as disabled once skills.enabled has any entry.

Line 366 only defaults to enabled when the stored set is empty. After a user has any persisted skill, a brand-new folder is indistinguishable from an intentionally disabled one and comes up disabled on discovery, which contradicts the method contract here. This needs a persisted “known skills” concept or an explicit enabled-state map rather than only the enabled-name array.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/main/src/plugin/skill/skill-manager.ts` around lines 359 - 371, The
code currently treats any skill as disabled when there are any entries in
enabledNames unless its name is present, which makes newly discovered folders
appear disabled; change the logic in the skill discovery block (around
isEnabled, enabledNames, this.skills, newSkillDiscovered, metadata, folderPath)
to distinguish between "known" skills and truly disabled ones: check
enabledNames.has(metadata.name) to decide explicit disabled/enabled, and if the
name is not present at all treat it as a new/unknown skill and default enabled =
true (and mark newSkillDiscovered), then add the skill name to the persisted
known/enabled map (or maintain a separate persisted knownSkills set or
enabledState map) so future discoveries are not treated as disabled; ensure
persistence is updated after adding the name.

@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 86.63366% with 27 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/main/src/plugin/skill/skill-manager.ts 90.22% 17 Missing ⚠️
packages/preload/src/index.ts 50.00% 8 Missing ⚠️
packages/main/src/plugin/directories-legacy.ts 33.33% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Comment on lines +52 to +55
@inject(ApiSenderType) private apiSender: ApiSenderType,
@inject(IConfigurationRegistry) private configurationRegistry: IConfigurationRegistry,
@inject(Directories) private directories: Directories,
@inject(IPCHandle) private readonly ipcHandle: IPCHandle,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: should all of them be readonly ? (looks like it's coming from copy/paste)

if (metadata.name.length > 64) {
throw new Error(`'name' exceeds 64 characters in ${filePath}`);
}
if (!/^[a-z0-9:-]+$/.test(metadata.name)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: what about comment from coderabbit ?


await mkdir(skillDir, { recursive: true });

const fileContent = `---\nname: ${metadata.name}\ndescription: ${metadata.description}\n---\n\n${options.content}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: I would defer the serialization/writing to js-yaml library

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we copy the skill folder rather than just reference it in an index file ?

if (existsSync(targetDir)) {
throw new Error(`Skill directory already exists: ${targetDir}`);
}
await cp(resolvedPath, targetDir, { recursive: true });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a fresh install where the skills directory doesn't exist yet, registerSkill calls cp() without ensuring the parent skillsDir is created first. Unlike createSkill, which uses mkdir(skillDir, { recursive: true }), cp({ recursive: true }) only copies the source tree. Consider a verification before cp call to be sure if the skills directory exists and if not, create it.


const isEnabled = enabledNames.has(metadata.name);

this.skills = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment for this.skills can cause a race condition. Since the .allSettled makes promises run concurrently, earlier writes can be overwritten by later ones. Consider using a local array to store discovered skills that can be added to this.skills after all promises are resolved.

this.apiSender.send('skill-manager-update');
}

listSkills(): SkillInfo[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Since SkillInfo includes content (which could be large) and it's only needed in the detail view, consider introducing a lighter interface without it for listSkills(). This way the list response stays lean, and getSkillContent() handles the full content when needed.

Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
packages/main/src/plugin/skill/skill-manager.ts (2)

196-203: ⚠️ Potential issue | 🟠 Major

Create the managed skills directory before copying into it.

createSkill() builds the directory tree first, but registerSkill() goes straight to cp(). On a fresh install, registering an external skill can fail before anything has created skillsDir.

🛠️ Proposed fix
     const skillsDir = this.directories.getSkillsDirectory();
     const targetDir = join(skillsDir, metadata.name);

     if (resolvedPath !== targetDir) {
+      await mkdir(skillsDir, { recursive: true });
       if (existsSync(targetDir)) {
         throw new Error(`Skill directory already exists: ${targetDir}`);
       }
       await cp(resolvedPath, targetDir, { recursive: true });
     }
In Node.js `fs.promises.cp`, when copying a directory recursively, does `cp(src, dest, { recursive: true })` create a missing parent directory for `dest`, or must the parent already exist?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/main/src/plugin/skill/skill-manager.ts` around lines 196 - 203,
registerSkill() assumes the skills directory exists before calling
cp(resolvedPath, targetDir, { recursive: true }) which can fail on fresh
installs; before computing targetDir or invoking cp, ensure the parent managed
directory (this.directories.getSkillsDirectory() / skillsDir) is created (use
mkdir or fs.promises.mkdir with { recursive: true }) or check and create it if
missing, similar to createSkill(), then continue with the existsSync(targetDir)
check and cp call to safely copy the skill.

63-72: ⚠️ Potential issue | 🟠 Major

skills.enabled alone cannot represent the discovery state correctly.

Persisting only enabled names conflates three cases: never seen, explicitly disabled, and “all skills disabled”. That breaks both directions here: a brand-new folder is treated as disabled once any other skill is persisted, and disabling every skill is lost on the next restart because [] is interpreted as “enable everything”. Please persist an explicit known/enabled state instead of inferring from array emptiness. The discovery spec covering the non-empty skills.enabled case will need to change with this fix.

Also applies to: 360-382, 385-392

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/main/src/plugin/skill/skill-manager.ts` around lines 63 - 72, The
current preferences schema/key `${SKILL_SECTION}.${SKILL_ENABLED}` (in
skill-manager.ts) stores only an array of enabled skill names, which conflates
"never seen", "explicitly disabled", and "all disabled" states; change
persistence to store explicit known/enabled state per skill (for example a
map/object like { "<skillId>": { known: true, enabled: false } } or two explicit
lists `known` and `enabled`) instead of inferring from array emptiness, update
read/write logic in the functions that use `${SKILL_SECTION}.${SKILL_ENABLED}`
to read the new structure and to write both known and enabled flags when
discovering or toggling skills, and ensure the discovery handling/exports
referenced by the discovery spec are adjusted to use the new explicit state
representation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 196-203: registerSkill() assumes the skills directory exists
before calling cp(resolvedPath, targetDir, { recursive: true }) which can fail
on fresh installs; before computing targetDir or invoking cp, ensure the parent
managed directory (this.directories.getSkillsDirectory() / skillsDir) is created
(use mkdir or fs.promises.mkdir with { recursive: true }) or check and create it
if missing, similar to createSkill(), then continue with the
existsSync(targetDir) check and cp call to safely copy the skill.
- Around line 63-72: The current preferences schema/key
`${SKILL_SECTION}.${SKILL_ENABLED}` (in skill-manager.ts) stores only an array
of enabled skill names, which conflates "never seen", "explicitly disabled", and
"all disabled" states; change persistence to store explicit known/enabled state
per skill (for example a map/object like { "<skillId>": { known: true, enabled:
false } } or two explicit lists `known` and `enabled`) instead of inferring from
array emptiness, update read/write logic in the functions that use
`${SKILL_SECTION}.${SKILL_ENABLED}` to read the new structure and to write both
known and enabled flags when discovering or toggling skills, and ensure the
discovery handling/exports referenced by the discovery spec are adjusted to use
the new explicit state representation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 19b531bc-32de-4164-886d-60b6f1bfd646

📥 Commits

Reviewing files that changed from the base of the PR and between 2e4b2d5 and 58ef4cd.

📒 Files selected for processing (2)
  • packages/main/src/plugin/skill/skill-manager.spec.ts
  • packages/main/src/plugin/skill/skill-manager.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create a skill manager

4 participants