feat(Claude extension): added Claude extension#1088
Open
gastoner wants to merge 8 commits intokortex-hub:mainfrom
Open
feat(Claude extension): added Claude extension#1088gastoner wants to merge 8 commits intokortex-hub:mainfrom
gastoner wants to merge 8 commits intokortex-hub:mainfrom
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
gastoner
force-pushed
the
claude_extension
branch
2 times, most recently
from
0cb245b to
3f061d6
Compare
gastoner
requested review from
benoitf and
bmahabirbu
and removed request for
a team
gastoner
force-pushed
the
claude_extension
branch
from
3f061d6 to
8613740
Compare
gastoner
force-pushed
the
claude_extension
branch
2 times, most recently
from
55dffcb to
e47b19e
Compare
gastoner
force-pushed
the
claude_extension
branch
from
e47b19e to
449f634
Compare
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
gastoner
force-pushed
the
claude_extension
branch
from
449f634 to
28d0847
Compare
📝 WalkthroughWalkthroughThe PR introduces a new Claude extension for the Kortex platform that registers skills stored in the user's home directory ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/main/src/plugin/extension/extension-loader.ts (1)
84-84: Use the/@/alias for this import.Line 84 adds a cross-module import from
plugin/extensionintoplugin/skill, so this should use the repo alias instead of a../path.As per coding guidelines, `**/{packages,extensions}/**/src/**/*.{ts,tsx,js,jsx}`: Use `/@/` path aliases for imports outside the current directory's module group instead of relative paths like `../plugin/provider-registry.js`.Suggested change
-import { SkillManager } from '../skill/skill-manager.js'; +import { SkillManager } from '/@/plugin/skill/skill-manager.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/extension/extension-loader.ts` at line 84, Replace the relative import of SkillManager with the repo path alias: in extension-loader.ts update the import that references SkillManager to use the "/@/..." alias for the plugin/skill module (locate the import statement importing SkillManager and change the module specifier to the corresponding "/@/..." path that points to packages/main/src/plugin/skill/skill-manager.js or its TS source) so the import follows the project's aliasing rule for cross-module imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/claude/src/claude-extension.ts`:
- Around line 35-40: The current call to extensionApi.skills.registerSkillFolder
(targetDisposable) is registering the user's Claude home (claudeSkillsDir) as a
managed folder which allows SkillManager.unregisterSkill() to recursively delete
it; change this to only register a dedicated Kortex-managed folder (create or
detect a subdirectory like claudeSkillsDir/kortex or a config-marked folder) or
register the folder as "discovered/read-only" so SkillManager does not treat it
as owned — i.e., stop passing the user's root claudeSkillsDir into
registerSkillFolder, instead create/use a Kortex-owned path and/or add
metadata/flag to the registration to indicate discovered (non-deletable) folders
and update unregisterSkill handling to skip deletion for folders marked
discovered.
In `@extensions/claude/src/extension.spec.ts`:
- Around line 66-75: Add an acceptance assertion that exercises deactivation:
after creating the ClaudeExtension (using ClaudeExtension and calling
ext.init()) and verifying the disposable was added to context.subscriptions,
call the extension's deactivation/dispose method (e.g., ext.deactivate() or
ext.dispose()—the actual method on ClaudeExtension) and assert that the
unregister function returned by extensionApi.skills.registerSkillFolder (the
mockDisposable.dispose or an unregister mock) was invoked; this requires mocking
registerSkillFolder to return a disposable whose dispose is a vi.fn() and then
expecting that dispose/unregister was called after deactivation.
In `@extensions/claude/vite.config.js`:
- Around line 60-63: The alias under the Vite config currently points
'@kortex-app/api' at a .js mock while the globalSetup writes a TypeScript mock;
update the alias value referenced in the alias object (the entry for
'@kortex-app/api') to point to the generated TypeScript mock file (replace the
.js target with the .ts target using the same PACKAGE_ROOT join expression) so
imports of '@kortex-app/api' resolve to '__mocks__/@kortex-app/api.ts'.
In `@packages/main/src/plugin/extension/extension-loader.ts`:
- Around line 1704-1712: registerSkillFolder currently calls
skillManager.registerSkillFolder which starts background discovery that can
continue after the returned Disposable is disposed, causing lingering
folder-backed skills; change the call so the background scan is cancelable and
tied to the returned Disposable: modify SkillManager.registerSkillFolder to
accept an AbortSignal/ cancellation token or to return a Disposable that cancels
its internal discovery, then in extension-loader.registerSkillFolder (the
function using instance.updateImage, resolvedIcon, extensionPath and pushing to
disposables) pass a new AbortController.signal (or wire the returned canceller)
and ensure the Disposable you push into disposables calls controller.abort() (or
the returned cancel method) so any in-flight discovery is aborted when the
extension is disabled.
- Around line 1714-1716: The wrapper method registerSkill in extension-loader
currently calls await this.skillManager.registerSkill(folderPath) and discards
the returned Disposable; change it to capture and return that Disposable and
also add it to the extension's lifecycle tracking (e.g., push the returned
Disposable into the extension instance's disposal collection such as
this.disposables or similar) so individually registered skills are disposed on
stop/reload; also update the public type/signature (including the
`@kortex-app/api` declaration) from Promise<void> to Promise<Disposable> if that
API still declares void.
---
Nitpick comments:
In `@packages/main/src/plugin/extension/extension-loader.ts`:
- Line 84: Replace the relative import of SkillManager with the repo path alias:
in extension-loader.ts update the import that references SkillManager to use the
"/@/..." alias for the plugin/skill module (locate the import statement
importing SkillManager and change the module specifier to the corresponding
"/@/..." path that points to packages/main/src/plugin/skill/skill-manager.js or
its TS source) so the import follows the project's aliasing rule for
cross-module imports.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 199b285f-e92f-4f10-b0d0-80d302bf98f7
⛔ Files ignored due to path filters (2)
extensions/claude/icon.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
extensions/claude/package.jsonextensions/claude/scripts/build.jsextensions/claude/src/claude-extension.tsextensions/claude/src/extension.spec.tsextensions/claude/src/extension.tsextensions/claude/tsconfig.jsonextensions/claude/vite.config.jspackages/extension-api/src/extension-api.d.tspackages/main/src/plugin/extension/extension-loader.spec.tspackages/main/src/plugin/extension/extension-loader.tspackages/main/src/plugin/skill/skill-manager.spec.tspackages/main/src/plugin/skill/skill-manager.ts
Comment on lines
+66
to
+75
| test('pushes disposable to extension context subscriptions', async () => { | ||
| const mockDisposable: Disposable = { dispose: vi.fn() }; | ||
| vi.mocked(extensionApi.skills.registerSkillFolder).mockReturnValue(mockDisposable); | ||
|
|
||
| const context = createMockExtensionContext(); | ||
| const ext = new ClaudeExtension(context); | ||
| await ext.init(); | ||
|
|
||
| expect(context.subscriptions).toContain(mockDisposable); | ||
| }); |
There was a problem hiding this comment.
Please add a deactivation/unregister assertion for the acceptance path.
This verifies subscription wiring, but it does not assert the disable/deactivation effect (registered skills becoming unregistered). Given the PR objective, add a test that exercises disposal/deactivation and checks unregister behavior is triggered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/claude/src/extension.spec.ts` around lines 66 - 75, Add an
acceptance assertion that exercises deactivation: after creating the
ClaudeExtension (using ClaudeExtension and calling ext.init()) and verifying the
disposable was added to context.subscriptions, call the extension's
deactivation/dispose method (e.g., ext.deactivate() or ext.dispose()—the actual
method on ClaudeExtension) and assert that the unregister function returned by
extensionApi.skills.registerSkillFolder (the mockDisposable.dispose or an
unregister mock) was invoked; this requires mocking registerSkillFolder to
return a disposable whose dispose is a vi.fn() and then expecting that
dispose/unregister was called after deactivation.
Comment on lines
+60
to
+63
| globalSetup: [join(PACKAGE_ROOT, '..', '..', '__mocks__', 'vitest-generate-api-global-setup.ts')], | ||
| alias: { | ||
| '@kortex-app/api': join(PACKAGE_ROOT, '..', '..', '__mocks__/@kortex-app/api.js'), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '56,63p' extensions/claude/vite.config.js
fd -a '^api\.(ts|js)$' __mocks__/@kortex-app
sed -n '96,124p' __mocks__/vitest-generate-api-global-setup.tsRepository: kortex-hub/kortex
Length of output: 507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if __mocks__ directory exists at all
if [ -d "__mocks__" ]; then
echo "=== __mocks__ directory structure ==="
find __mocks__ -type f -name "*.ts" -o -name "*.js" | sort
else
echo "__mocks__ directory not found at root"
fi
# Check vite.config.js for PACKAGE_ROOT and full path resolution
echo ""
echo "=== vite.config.js content (lines 1-70) ==="
head -70 extensions/claude/vite.config.jsRepository: kortex-hub/kortex
Length of output: 2157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== vitest-generate-api-global-setup.ts ==="
cat __mocks__/vitest-generate-api-global-setup.tsRepository: kortex-hub/kortex
Length of output: 5255
Fix alias extension: should be api.ts, not api.js.
The globalSetup generator explicitly writes to __mocks__/@kortex-app/api.ts, but this alias targets api.js. The mismatch will cause test failures when importing @kortex-app/api. Change the alias to:
'@kortex-app/api': join(PACKAGE_ROOT, '..', '..', '__mocks__/@kortex-app/api.ts'),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/claude/vite.config.js` around lines 60 - 63, The alias under the
Vite config currently points '@kortex-app/api' at a .js mock while the
globalSetup writes a TypeScript mock; update the alias value referenced in the
alias object (the entry for '@kortex-app/api') to point to the generated
TypeScript mock file (replace the .js target with the .ts target using the same
PACKAGE_ROOT join expression) so imports of '@kortex-app/api' resolve to
'__mocks__/@kortex-app/api.ts'.
Comment on lines
+1714
to
+1716
| registerSkill: async (folderPath: string): Promise<void> => { | ||
| await this.skillManager.registerSkill(folderPath); | ||
| }, |
There was a problem hiding this comment.
Return and track the disposer from registerSkill().
SkillManager.registerSkill() now gives back a Disposable, but this wrapper drops it. That leaves individually registered skills unattached to the extension lifecycle, so they can survive stop/reload.
Possible fix
- registerSkill: async (folderPath: string): Promise<void> => {
- await this.skillManager.registerSkill(folderPath);
- },
+ registerSkill: async (folderPath: string) => {
+ const disposable = await this.skillManager.registerSkill(folderPath);
+ disposables.push(disposable);
+ return disposable;
+ },If @kortex-app/api still types this as Promise<void>, update that declaration to match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/main/src/plugin/extension/extension-loader.ts` around lines 1714 -
1716, The wrapper method registerSkill in extension-loader currently calls await
this.skillManager.registerSkill(folderPath) and discards the returned
Disposable; change it to capture and return that Disposable and also add it to
the extension's lifecycle tracking (e.g., push the returned Disposable into the
extension instance's disposal collection such as this.disposables or similar) so
individually registered skills are disposed on stop/reload; also update the
public type/signature (including the `@kortex-app/api` declaration) from
Promise<void> to Promise<Disposable> if that API still declares void.
benoitf
reviewed
Apr 2, 2026
extensions/claude/src/extension.ts
Outdated
|
|
||
| import { ClaudeExtension } from './claude-extension'; | ||
|
|
||
| export async function activate(extensionContext: ExtensionContext): Promise<void> { |
Contributor
There was a problem hiding this comment.
IMHO new extensions should reuse the inversify pattern introduced in latest extensions
https://github.com/kortex-hub/kortex/blob/main/extensions/ramalama/src/ramalama-extension.ts
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
extensions/claude/src/manager/claude-skills-manager.ts (1)
39-54: Minor: potential double-dispose scenario.The disposable is both pushed to
extensionContext.subscriptions(Line 48) and stored in#skillFolderDisposable(Line 42) for explicit disposal indispose()(Line 52). If the extension context disposes its subscriptions during shutdown anddispose()is also called, the disposable'sdispose()method will execute twice. This should be fine if the underlying implementation is idempotent, but worth noting.🔧 Optional: Remove redundant subscription registration
If
ClaudeSkillsManager.dispose()is always called during deactivation (which it is viaClaudeExtension.deactivate()), you could skip pushing to subscriptions:this.#skillFolderDisposable = skills.registerSkillFolder({ label: 'Claude Skills', badge: 'Claude', icon: './icon.png', baseDirectory: claudeSkillsDir, }); - this.extensionContext.subscriptions.push(this.#skillFolderDisposable);Alternatively, rely solely on subscriptions and remove explicit
dispose()logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/claude/src/manager/claude-skills-manager.ts` around lines 39 - 54, The disposable returned by skills.registerSkillFolder is both stored on ClaudeSkillsManager.#skillFolderDisposable and also pushed into extensionContext.subscriptions, causing a potential double-dispose; fix by removing the redundant subscription registration in init(): omit extensionContext.subscriptions.push(this.#skillFolderDisposable) and keep the explicit disposal logic in dispose() (or alternatively remove the explicit dispose() and rely only on extensionContext.subscriptions) — update the init() method around the skills.registerSkillFolder call and the dispose() method accordingly.extensions/claude/src/claude-extension.ts (1)
36-48: RedundantgetAsynccall.
initBindings()(Line 38) already resolvesClaudeSkillsManagerviagetAsyncinternally (seeinversify-binding.ts:42), but the result isn't returned. Line 41 then callsgetAsyncagain to obtain the reference. While this works correctly due to singleton scoping, consider havinginitBindings()return the manager directly to avoid the extra container lookup.♻️ Optional: Return manager from initBindings
In
inversify-binding.ts:- await this.#container.getAsync(ClaudeSkillsManager); - return this.#container; + const manager = await this.#container.getAsync(ClaudeSkillsManager); + return { container: this.#container, claudeSkillsManager: manager };Then in
claude-extension.ts:- this.#container = await this.#inversifyBinding.initBindings(); - try { - this.#claudeSkillsManager = await this.getContainer()?.getAsync(ClaudeSkillsManager); - } catch (e) { - console.error('Error while creating the Claude skills manager', e); - throw e; - } + const { container, claudeSkillsManager } = await this.#inversifyBinding.initBindings(); + this.#container = container; + this.#claudeSkillsManager = claudeSkillsManager;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/claude/src/claude-extension.ts` around lines 36 - 48, initBindings currently calls getAsync(ClaudeSkillsManager) internally but does not return it, causing a redundant getAsync call in activate; modify InversifyBinding.initBindings to return the resolved ClaudeSkillsManager (or a tuple/object containing { container, claudeSkillsManager }) so callers get the manager directly, then update ClaudeExtension.activate to use the returned ClaudeSkillsManager instead of calling this.#container.getAsync(ClaudeSkillsManager) (remove the redundant lookup and adjust the try/catch/initialization flow around this.#claudeSkillsManager and its init call accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@extensions/claude/src/claude-extension.ts`:
- Around line 36-48: initBindings currently calls getAsync(ClaudeSkillsManager)
internally but does not return it, causing a redundant getAsync call in
activate; modify InversifyBinding.initBindings to return the resolved
ClaudeSkillsManager (or a tuple/object containing { container,
claudeSkillsManager }) so callers get the manager directly, then update
ClaudeExtension.activate to use the returned ClaudeSkillsManager instead of
calling this.#container.getAsync(ClaudeSkillsManager) (remove the redundant
lookup and adjust the try/catch/initialization flow around
this.#claudeSkillsManager and its init call accordingly).
In `@extensions/claude/src/manager/claude-skills-manager.ts`:
- Around line 39-54: The disposable returned by skills.registerSkillFolder is
both stored on ClaudeSkillsManager.#skillFolderDisposable and also pushed into
extensionContext.subscriptions, causing a potential double-dispose; fix by
removing the redundant subscription registration in init(): omit
extensionContext.subscriptions.push(this.#skillFolderDisposable) and keep the
explicit disposal logic in dispose() (or alternatively remove the explicit
dispose() and rely only on extensionContext.subscriptions) — update the init()
method around the skills.registerSkillFolder call and the dispose() method
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0166f3f5-6095-4979-995d-ed71dc1a0531
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
extensions/claude/package.jsonextensions/claude/src/claude-extension.spec.tsextensions/claude/src/claude-extension.tsextensions/claude/src/extension.spec.tsextensions/claude/src/extension.tsextensions/claude/src/inject/inversify-binding.spec.tsextensions/claude/src/inject/inversify-binding.tsextensions/claude/src/inject/symbol.tsextensions/claude/src/manager/_manager-module.tsextensions/claude/src/manager/claude-skills-manager.spec.tsextensions/claude/src/manager/claude-skills-manager.tsextensions/claude/tsconfig.jsonextensions/claude/vite.config.jspackage.json
✅ Files skipped from review due to trivial changes (5)
- extensions/claude/src/inject/symbol.ts
- extensions/claude/src/extension.spec.ts
- extensions/claude/tsconfig.json
- extensions/claude/vite.config.js
- extensions/claude/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/claude/src/extension.ts
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
gastoner
force-pushed
the
claude_extension
branch
from
7cf8105 to
517470f
Compare
Contributor
Author
|
@benoitf updated, just now the icon when creating the skills is not Claude one :( (but we can create a follow-up pr for this :D) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
extensions/claude/src/manager/claude-skills-manager.spec.ts (2)
67-72: Consider adding a re-init lifecycle test.A small test asserting repeated
init()does not create duplicate registrations would lock in expected behavior and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/claude/src/manager/claude-skills-manager.spec.ts` around lines 67 - 72, Add a re-init lifecycle test that calls claudeSkillsManager.init() twice and asserts that duplicate registrations are not created: invoke claudeSkillsManager.init() a first time, capture the registration/disposable (e.g. disposableMock or the container/registry method used during registration), call claudeSkillsManager.init() again, and assert that the registration method was called only once and no extra disposables were created (e.g. expect(container.register).toHaveBeenCalledTimes(1) or expect(disposableMock.dispose).not.toHaveBeenCalledDuringReinit). This ensures init() is idempotent and prevents duplicate skill registrations.
36-38: UsePick<Provider, 'registerSkill'>for more precise typing instead ofas unknown as Provider.The double-cast bypasses type checks and can mask interface changes. Since
registerSkillis the only used property, casting toPick<Provider, 'registerSkill'>provides better type safety without excess ceremony. Note: This pattern is used throughout the codebase; consider applying consistently if standardizing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/claude/src/manager/claude-skills-manager.spec.ts` around lines 36 - 38, Replace the unsafe double-cast on providerMock with a precise picked type: change its declaration so it uses Pick<Provider, 'registerSkill'> instead of "as unknown as Provider", keeping the existing vi.fn() for registerSkill; update any similar mocks in this file (claude-skills-manager.spec.ts) to use Pick<Provider, 'registerSkill'> to preserve correct typing for the registerSkill property and avoid masking interface changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/claude/src/manager/claude-skills-manager.ts`:
- Around line 38-45: init() currently re-registers the Claude skill on every
call, overwriting this.#skillDisposable so dispose() only cleans the last
registration; make init() idempotent by checking this.#skillDisposable before
calling this.claudeProvider.registerSkill (or by disposing the existing
disposable first) and only register when no active disposable exists, and ensure
dispose() clears this.#skillDisposable after disposing so subsequent init() can
register again; refer to init(), this.#skillDisposable,
this.claudeProvider.registerSkill, and dispose() when implementing the change.
---
Nitpick comments:
In `@extensions/claude/src/manager/claude-skills-manager.spec.ts`:
- Around line 67-72: Add a re-init lifecycle test that calls
claudeSkillsManager.init() twice and asserts that duplicate registrations are
not created: invoke claudeSkillsManager.init() a first time, capture the
registration/disposable (e.g. disposableMock or the container/registry method
used during registration), call claudeSkillsManager.init() again, and assert
that the registration method was called only once and no extra disposables were
created (e.g. expect(container.register).toHaveBeenCalledTimes(1) or
expect(disposableMock.dispose).not.toHaveBeenCalledDuringReinit). This ensures
init() is idempotent and prevents duplicate skill registrations.
- Around line 36-38: Replace the unsafe double-cast on providerMock with a
precise picked type: change its declaration so it uses Pick<Provider,
'registerSkill'> instead of "as unknown as Provider", keeping the existing
vi.fn() for registerSkill; update any similar mocks in this file
(claude-skills-manager.spec.ts) to use Pick<Provider, 'registerSkill'> to
preserve correct typing for the registerSkill property and avoid masking
interface changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70cda499-f53a-4b6b-8604-9f17308b44e7
📒 Files selected for processing (7)
extensions/claude/src/claude-extension.spec.tsextensions/claude/src/claude-extension.tsextensions/claude/src/inject/inversify-binding.spec.tsextensions/claude/src/inject/inversify-binding.tsextensions/claude/src/inject/symbol.tsextensions/claude/src/manager/claude-skills-manager.spec.tsextensions/claude/src/manager/claude-skills-manager.ts
✅ Files skipped from review due to trivial changes (1)
- extensions/claude/src/inject/symbol.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- extensions/claude/src/inject/inversify-binding.spec.ts
- extensions/claude/src/claude-extension.spec.ts
- extensions/claude/src/inject/inversify-binding.ts
- extensions/claude/src/claude-extension.ts
Comment on lines
+38
to
+45
| async init(): Promise<void> { | ||
| const claudeSkillsDir = ClaudeSkillsManager.getClaudeSkillsDir(); | ||
|
|
||
| this.#skillDisposable = this.claudeProvider.registerSkill({ | ||
| label: 'Claude', | ||
| path: claudeSkillsDir, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Make init() idempotent to avoid duplicate skill registrations.
Calling init() more than once re-registers the same skill and overwrites the previous Disposable, so only the latest registration is cleaned up in dispose().
Suggested fix
async init(): Promise<void> {
+ if (this.#skillDisposable) {
+ return;
+ }
const claudeSkillsDir = ClaudeSkillsManager.getClaudeSkillsDir();
this.#skillDisposable = this.claudeProvider.registerSkill({
label: 'Claude',
path: claudeSkillsDir,
});
}📝 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
| async init(): Promise<void> { | |
| const claudeSkillsDir = ClaudeSkillsManager.getClaudeSkillsDir(); | |
| this.#skillDisposable = this.claudeProvider.registerSkill({ | |
| label: 'Claude', | |
| path: claudeSkillsDir, | |
| }); | |
| } | |
| async init(): Promise<void> { | |
| if (this.#skillDisposable) { | |
| return; | |
| } | |
| const claudeSkillsDir = ClaudeSkillsManager.getClaudeSkillsDir(); | |
| this.#skillDisposable = this.claudeProvider.registerSkill({ | |
| label: 'Claude', | |
| path: claudeSkillsDir, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/claude/src/manager/claude-skills-manager.ts` around lines 38 - 45,
init() currently re-registers the Claude skill on every call, overwriting
this.#skillDisposable so dispose() only cleans the last registration; make
init() idempotent by checking this.#skillDisposable before calling
this.claudeProvider.registerSkill (or by disposing the existing disposable
first) and only register when no active disposable exists, and ensure dispose()
clears this.#skillDisposable after disposing so subsequent init() can register
again; refer to init(), this.#skillDisposable,
this.claudeProvider.registerSkill, and dispose() when implementing the change.
jeffmaury
reviewed
Apr 2, 2026
Contributor
jeffmaury
left a comment
jeffmaury
left a comment
There was a problem hiding this comment.
I tested it and it showed the skills from the local Claude skills folder but I don't think we should enable delete in this case as it deleted the skill folder in the local .Claude skills folder which seems weird to me. @slemeur WDYT ?
benoitf
approved these changes
Apr 3, 2026
Contributor
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
follow-up: I think we might enhance API like adding icons, having a way to prevent the removal, etc.
But I see that as follow-up
Contributor
Author
Yep aggree, looks like the manager was not working properly, the idea was as you wrote basically delete ONLY skills in |
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.
Adds in-built claude extension that registeres skills from claude/skills folder
Closes #1047
You can test this by creating some skill in claude/skills folder
Then enable extension (the skill should be visible in the list of skills)
Disable extension, all registered skills should now be unregistered (not visible)