Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces name-only active-skill handling with a pinned-skill model, adds richer per-skill metadata, introduces skill inspection and conversation-scoped draft-management APIs/tools, persists a Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent (LLM)
participant ToolMgr as AgentToolManager
participant SkillTools as SkillTools
participant SkillPres as SkillPresenter
participant Config as ConfigPresenter
participant FS as FileSystem
Agent->>ToolMgr: invoke tool (skill_view | skill_manage) with args
ToolMgr->>SkillTools: route to handleSkillView/handleSkillManage(args, conversationId)
SkillTools->>Config: read skillDraftSuggestionsEnabled?
SkillTools->>SkillPres: call viewSkill(name, filePath?, conversationId) / manageDraftSkill(conversationId, request)
SkillPres->>FS: validate paths, read/write files, enumerate linked files, enforce limits
FS-->>SkillPres: file contents / errors
SkillPres-->>SkillTools: return SkillViewResult / SkillManageResult
SkillTools-->>ToolMgr: return tool result
ToolMgr-->>Agent: return tool output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/presenter/agentRuntimePresenter/index.ts (1)
2121-2131:⚠️ Potential issue | 🟠 MajorInclude skill metadata in the system-prompt fingerprint.
The prompt now renders each skill's description/category/platforms, but the cache key still hashes only skill names. If a
SKILL.mdchanges metadata without renaming the skill, this session can keep serving a stale prompt until the cache is invalidated for some unrelated reason.Suggested fix
- const fingerprint = this.buildSystemPromptFingerprint({ + const fingerprint = this.buildSystemPromptFingerprint({ providerId, modelId, workdir, basePrompt: normalizedBase, skillsEnabled, - availableSkillNames: normalizedAvailableSkills.map((skill) => skill.name), + availableSkills: normalizedAvailableSkills.map((skill) => ({ + name: skill.name, + description: skill.description, + category: skill.category ?? null, + platforms: skill.platforms ?? [] + })), activeSkillNames: normalizedActiveSkills, toolSignature: this.buildToolSignature(toolDefinitions), skillDraftSuggestionsEnabled })private buildSystemPromptFingerprint(params: { providerId: string modelId: string workdir: string | null basePrompt: string skillsEnabled: boolean - availableSkillNames: string[] + availableSkills: Array<{ + name: string + description: string + category?: string | null + platforms?: string[] + }> activeSkillNames: string[] toolSignature: string[] skillDraftSuggestionsEnabled: boolean }): string { return JSON.stringify({ providerId: params.providerId, modelId: params.modelId, workdir: params.workdir ?? '', basePrompt: params.basePrompt, skillsEnabled: params.skillsEnabled, - availableSkillNames: params.availableSkillNames, + availableSkills: params.availableSkills, activeSkillNames: params.activeSkillNames, toolSignature: params.toolSignature, skillDraftSuggestionsEnabled: params.skillDraftSuggestionsEnabled }) }Also applies to: 2366-2387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentRuntimePresenter/index.ts` around lines 2121 - 2131, The fingerprint currently only includes available skill names (availableSkillNames) so changes to skill metadata (description/category/platforms) won’t invalidate the cache; update the call to buildSystemPromptFingerprint (and the data you construct before it) to include a deterministic signature of each skill's metadata instead of just skill.name — e.g., replace availableSkillNames with a normalizedAvailableSkillsSignature built from normalizedAvailableSkills (including description, category, platforms, and name) or add a new parameter that contains that metadata, and update buildSystemPromptFingerprint to incorporate that metadata when computing the hash (also apply the same change in the other occurrence around the 2366-2387 region).test/main/presenter/skillPresenter/skillPresenter.test.ts (1)
66-95:⚠️ Potential issue | 🔴 CriticalUse real
path.posixsemantics in this traversal test mock.This hand-rolled
resolve()never normalizes./.., so security tests with embedded traversal patterns (e.g.,notes/../../../etc/passwd) will fail to detect real exploits. The simple../secrets.txtcase still fails correctly by accident, but more sophisticated attacks bypass the mock's check.Suggested test fix
-vi.mock('path', () => ({ - default: { - join: vi.fn((...args: string[]) => args.join('/')), - dirname: vi.fn((p: string) => p.split('/').slice(0, -1).join('/')), - basename: vi.fn((p: string) => p.split('/').pop() || ''), - extname: vi.fn((p: string) => { - const base = p.split('/').pop() || '' - const idx = base.lastIndexOf('.') - return idx >= 0 ? base.slice(idx) : '' - }), - resolve: vi.fn((...args: string[]) => { - let resolved = '' - for (const part of args.filter(Boolean)) { - if (part.startsWith('/')) { - resolved = part - continue - } - resolved = resolved ? `${resolved.replace(/\/+$/, '')}/${part}` : `/${part}` - } - return resolved || '/' - }), - relative: vi.fn((from: string, to: string) => { - if (to.startsWith(from)) { - return to.substring(from.length + 1) - } - return '../' + to - }), - isAbsolute: vi.fn((p: string) => p.startsWith('/')), - sep: '/' - } -})) +vi.mock('path', async () => { + const actual = await vi.importActual<typeof import('path')>('path') + return { + default: { + ...actual.posix, + join: vi.fn(actual.posix.join), + dirname: vi.fn(actual.posix.dirname), + basename: vi.fn(actual.posix.basename), + extname: vi.fn(actual.posix.extname), + resolve: vi.fn(actual.posix.resolve), + relative: vi.fn(actual.posix.relative), + isAbsolute: vi.fn(actual.posix.isAbsolute), + sep: '/' + } + } +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/skillPresenter/skillPresenter.test.ts` around lines 66 - 95, The mocked path module in the test defines a custom resolve()/relative() that doesn't handle "."/".." normalization, allowing traversal payloads like "notes/../../../etc/passwd" to bypass checks; replace the hand-rolled implementations by delegating to the real POSIX semantics (e.g., use path.posix.resolve and path.posix.relative) when creating the mock for resolve, relative, dirname, basename, extname and isAbsolute so the mock normalizes "." and ".." correctly and the traversal security tests behave like the real runtime; update the vi.mock('path', ...) block to call into the actual path.posix functions instead of the inline implementations for the named symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/skillPresenter/index.ts`:
- Around line 1804-1824: The conversationId is used directly as a filesystem
segment in createDraftDirectory and resolveDraftPath which allows path
traversal; validate or sanitize it before use (in both createDraftDirectory and
resolveDraftPath). Specifically, ensure conversationId is not absolute, does not
contain path separators or ".." (e.g., conversationId ===
path.basename(conversationId) and does not include path.sep or '/' or '\\'), and
optionally restrict to a safe character class (like /^[A-Za-z0-9._-]+$/); if
invalid, throw or return null. Apply this check at the start of both methods and
only join conversationId into paths after it passes validation so draftsRoot
cannot be escaped.
- Around line 540-554: The code in skill presenter reads metadata.path directly
(in the block returning content) and bypasses the
SKILL_CONFIG.SKILL_FILE_MAX_SIZE guard; before calling
fs.readFileSync(metadata.path, 'utf-8') inside the method that returns the skill
view, add the same size check used by loadSkillContent()/readSkillFile(): stat
the file, compare to SKILL_CONFIG.SKILL_FILE_MAX_SIZE, and if it exceeds the
limit return the same error/response shape used elsewhere (e.g., success: false
or a trimmed payload) so large SKILL.md files are rejected consistently; update
the branch that constructs the returned object (which uses replacePathVariables
and listSkillLinkedFiles) to only read content after the size check passes.
- Around line 1489-1518: The watcher handlers currently unconditionally
overwrite metadataCache on add/change; modify the logic in the
watcher.on('change') and watcher.on('add') blocks after parseSkillMetadata so
that if metadataCache already contains metadata.name (and the stored entry's
path/name differs from the new metadata) you do not overwrite the existing entry
but instead emit the same duplicate-name warning/keep-first behavior used by
full discovery: log or send a warning event via eventBus (e.g., a new
SKILL_EVENTS.DUPLICATE_NAME or reuse existing warning flow) and keep the
original metadataCache entry; only set metadataCache.set(metadata.name,
metadata) when there is no existing conflict (or when the existing entry is the
same skill being renamed), and ensure metadataCache.delete(previousName) logic
in watcher.on('change') still correctly handles renames without clobbering other
skills.
In `@src/main/presenter/toolPresenter/agentTools/agentToolManager.ts`:
- Around line 262-279: The skill_manage schema currently marks draftPath,
content, filePath, and fileContent as optional for all actions, so invalid
combinations pass safeParse; replace the single generic object with a
discriminated union (z.discriminatedUnion or z.union of action-specific
z.objects) keyed on action to enforce required fields per action (e.g., for
action 'edit'|'write_file'|'remove_file'|'delete' require draftPath; for
'create' require content; for 'write_file' require filePath and fileContent; for
'remove_file'/'delete' require filePath as appropriate), and apply the same
change to the duplicate schema instance (the other skill_manage definition
referenced) so validations fail fast in safeParse.
In `@src/renderer/settings/components/skills/SkillsSettings.vue`:
- Around line 185-187: The assignment to draftSuggestionsEnabled.value is
storing a Promise because configPresenter.getSkillDraftSuggestionsEnabled is
async; update the onMounted callback to await the presenter getter before
assigning (e.g., const enabled = await
configPresenter.getSkillDraftSuggestionsEnabled?.();
draftSuggestionsEnabled.value = enabled ?? false) and keep the existing await
skillsStore.loadSkills() call; ensure you handle missing method (optional
chaining) and provide a boolean fallback.
In `@src/renderer/src/i18n/pt-BR/settings.json`:
- Around line 1311-1313: Replace the English word "agent" with the Portuguese
"agente" inside the description string for the "Sugerir rascunhos de skill"
entry so the locale reads consistently; locate the JSON object where "title":
"Sugerir rascunhos de skill" and update its "description" value to use "agente"
instead of "agent".
---
Outside diff comments:
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Around line 2121-2131: The fingerprint currently only includes available skill
names (availableSkillNames) so changes to skill metadata
(description/category/platforms) won’t invalidate the cache; update the call to
buildSystemPromptFingerprint (and the data you construct before it) to include a
deterministic signature of each skill's metadata instead of just skill.name —
e.g., replace availableSkillNames with a normalizedAvailableSkillsSignature
built from normalizedAvailableSkills (including description, category,
platforms, and name) or add a new parameter that contains that metadata, and
update buildSystemPromptFingerprint to incorporate that metadata when computing
the hash (also apply the same change in the other occurrence around the
2366-2387 region).
In `@test/main/presenter/skillPresenter/skillPresenter.test.ts`:
- Around line 66-95: The mocked path module in the test defines a custom
resolve()/relative() that doesn't handle "."/".." normalization, allowing
traversal payloads like "notes/../../../etc/passwd" to bypass checks; replace
the hand-rolled implementations by delegating to the real POSIX semantics (e.g.,
use path.posix.resolve and path.posix.relative) when creating the mock for
resolve, relative, dirname, basename, extname and isAbsolute so the mock
normalizes "." and ".." correctly and the traversal security tests behave like
the real runtime; update the vi.mock('path', ...) block to call into the actual
path.posix functions instead of the inline implementations for the named
symbols.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20c6a097-97ff-4165-835f-975f740812a1
📒 Files selected for processing (30)
src/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/skillPresenter/index.tssrc/main/presenter/skillPresenter/skillExecutionService.tssrc/main/presenter/skillPresenter/skillTools.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tssrc/main/presenter/toolPresenter/index.tssrc/renderer/settings/components/skills/SkillsSettings.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/types/skill.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/skillPresenter/skillPresenter.test.tstest/main/presenter/skillPresenter/skillTools.test.tstest/main/presenter/toolPresenter/agentTools/agentToolManagerSettings.test.ts
src/main/presenter/toolPresenter/agentTools/agentToolManager.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/skillPresenter/index.ts (1)
320-326:⚠️ Potential issue | 🟠 MajorApply the manifest size guard before metadata parsing.
parseSkillMetadata()still does an unconditionalreadFileSync(). A large localSKILL.mdwill be read during discovery/hot reload beforeloadSkillContent()orviewSkill()ever get a chance to enforceSKILL_FILE_MAX_SIZE.Suggested fix
private async parseSkillMetadata( skillPath: string, dirName: string ): Promise<SkillMetadata | null> { try { + const stats = fs.statSync(skillPath) + if (stats.size > SKILL_CONFIG.SKILL_FILE_MAX_SIZE) { + logger.warn('[SkillPresenter] Skill manifest too large to parse metadata', { + skillPath, + size: stats.size + }) + return null + } + const content = fs.readFileSync(skillPath, 'utf-8') const { data } = matter(content)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/skillPresenter/index.ts` around lines 320 - 326, The parseSkillMetadata function currently calls fs.readFileSync unconditionally; add a file-size guard using SKILL_FILE_MAX_SIZE before reading to avoid loading huge SKILL.md files during discovery/hot-reload. Use fs.statSync or fs.promises.stat on skillPath to check size and return null (or skip parsing) if the size exceeds SKILL_FILE_MAX_SIZE, so that loadSkillContent() and viewSkill() remain the places that enforce content size limits; update parseSkillMetadata to perform this pre-check and only call fs.readFileSync and matter() when the file is within the allowed size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/skillPresenter/index.ts`:
- Around line 283-301: The duplicate-name resolution depends on filesystem
traversal order; make it deterministic by sorting the list returned from
collectSkillManifestPaths(this.skillsDir) before iterating. Replace the direct
for-loop over collectSkillManifestPaths(this.skillsDir) with an iteration over a
sorted array (e.g., Array.from(...) or
[...collectSkillManifestPaths(...)].sort()) so that
parseSkillMetadata(skillPath, dirName) and the metadataCache.set(metadata.name,
metadata) always see a stable order when checking
this.metadataCache.has(metadata.name).
- Around line 581-583: The code currently returns absolute temp-backed draft
paths (created via createDraftDirectory and written with atomicWriteFile) which
lets agents bypass skill_manage restrictions; change the API to never expose
filesystem paths: have createDraftDirectory (or introduce createDraftHandle)
store the draft under an internal location or move drafts outside the generic
allowlist and instead return an opaque draft ID/handle (e.g., draftId) from the
presenter methods that currently return draftPath (locations around the calls to
createDraftDirectory/atomicWriteFile and where responses return
draftPath/skillName). Update any consumers to resolve drafts via a new internal
lookup method (e.g., getDraftPathForId) that enforces conversation/scan checks
and does not expose the real path to the agent.
- Around line 1879-1884: The draft folder naming uses Date.now() which can
collide; update the draftPath creation in skillPresenter (around
conversationRoot, safeSlug, draftPath) to include a collision-resistant token
such as crypto.randomUUID() (or a UUID/nanoid) appended to or replacing
Date.now() so each create call produces a unique directory name; ensure the
chosen token is generated via a secure library (e.g., Node's
crypto.randomUUID()) and concatenate it with safeSlug when forming draftPath
before calling fs.mkdirSync.
---
Outside diff comments:
In `@src/main/presenter/skillPresenter/index.ts`:
- Around line 320-326: The parseSkillMetadata function currently calls
fs.readFileSync unconditionally; add a file-size guard using SKILL_FILE_MAX_SIZE
before reading to avoid loading huge SKILL.md files during discovery/hot-reload.
Use fs.statSync or fs.promises.stat on skillPath to check size and return null
(or skip parsing) if the size exceeds SKILL_FILE_MAX_SIZE, so that
loadSkillContent() and viewSkill() remain the places that enforce content size
limits; update parseSkillMetadata to perform this pre-check and only call
fs.readFileSync and matter() when the file is within the allowed size.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7631c128-3b52-47a2-b5a8-a45ac7b366d0
📒 Files selected for processing (6)
src/main/presenter/skillPresenter/index.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tssrc/renderer/settings/components/skills/SkillsSettings.vuesrc/renderer/src/i18n/pt-BR/settings.jsontest/main/presenter/skillPresenter/skillPresenter.test.tstest/main/presenter/toolPresenter/agentTools/agentToolManagerSettings.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/renderer/src/i18n/pt-BR/settings.json
🚧 Files skipped from review as they are similar to previous changes (3)
- test/main/presenter/toolPresenter/agentTools/agentToolManagerSettings.test.ts
- src/renderer/settings/components/skills/SkillsSettings.vue
- test/main/presenter/skillPresenter/skillPresenter.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/toolPresenter/agentTools/agentToolManager.ts (1)
1511-1545:⚠️ Potential issue | 🟡 MinorHide
skill_managewhen there is no conversation context.
skill_manageis defined unconditionally here, butsrc/main/presenter/skillPresenter/skillTools.ts:52-65rejects it without aconversationId. That leaves sessions with no conversation context holding a tool that can only fail. Gate this definition the same wayskill_runis gated, or splitgetSkillToolDefinitions()soskill_manageis only exposed whencontext.conversationIdexists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/toolPresenter/agentTools/agentToolManager.ts` around lines 1511 - 1545, The tool definition for skill_manage is currently always included even when no conversation context exists; update getSkillToolDefinitions (or the block adding skill_manage) to only push/return the skill_manage tool when context.conversationId is present (same gating used for skill_run), or split getSkillToolDefinitions so the draft-management tool (skill_manage / schemas.skill_manage) is only exposed when context.conversationId exists; ensure the server/function entry for skill_manage is omitted otherwise so callers in skillPresenter/skillTools.ts no longer receive a tool that will reject without a conversationId.
🧹 Nitpick comments (1)
src/shared/types/skill.ts (1)
146-152: ModelSkillManageRequestas the same discriminated union the runtime accepts.This flat interface still allows impossible states like
{ action: 'edit' }or{ action: 'write_file', draftId }at compile time, even thoughagentToolManagernow rejects those at runtime. Making the shared type action-specific keeps presenters, tool handlers, and tests aligned.♻️ Suggested typing change
-export interface SkillManageRequest { - action: SkillManageAction - draftId?: string - content?: string - filePath?: string - fileContent?: string -} +export type SkillManageRequest = + | { + action: 'create' + content: string + } + | { + action: 'edit' + draftId: string + content: string + } + | { + action: 'write_file' + draftId: string + filePath: string + fileContent: string + } + | { + action: 'remove_file' + draftId: string + filePath: string + } + | { + action: 'delete' + draftId: string + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types/skill.ts` around lines 146 - 152, The SkillManageRequest interface allows invalid combinations; replace it with a discriminated union keyed by action (SkillManageAction) that mirrors the runtime checks in agentToolManager. Define one union member per SkillManageAction value, e.g. for actions that require a draftId (like "edit"/"delete") include draftId: string and disallow filePath/fileContent; for create/edit include content: string as required; for file operations (like "write_file") require filePath: string and fileContent: string and disallow draftId/content; and for any other actions add appropriate required/forbidden fields. Update the exported SkillManageRequest type and ensure all callers compile against the new action-specific variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/skillPresenter/index.ts`:
- Around line 1992-2021: cleanupExpiredDrafts() currently uses the draft
directory's mtime to decide expiry which misses updates to nested files; change
expiry to use an explicit last-activity marker or the draft root's mtime that
you update on every draft mutation: update cleanupExpiredDrafts() to read the
marker file (e.g., ".lastActivity") timestamp or fs.statSync(draftDir +
"/.lastActivity").mtimeMs instead of stats.mtimeMs, and ensure all functions
that mutate drafts (the methods that create/update/delete draft contents — e.g.,
saveDraft/updateDraft/any writer that touches references/checklist.md) either
touch the draft root (fs.utimesSync) or write/update the marker file after
successful mutations so active drafts are not erroneously deleted; keep
SKILL_CONFIG.DRAFT_RETENTION_MS as the retention threshold.
- Around line 495-541: Wrap the filesystem access in both branches (the branch
handling options?.filePath and the other branch around lines 547-558) in
try/catch blocks so any fs.existsSync/statSync/readFileSync errors are caught
and converted into a SkillViewResult { success: false, error: string } instead
of letting exceptions bubble; specifically, around resolveSkillRelativePath,
fs.statSync, fs.readFileSync and calls to isBinaryLikeFile (and checks against
SKILL_CONFIG.MAX_LINKED_FILE_SIZE) catch errors and return a structured failure
result with a clear error message (avoid throwing) so skill_view always returns
the { success: boolean, error? } shape expected by the tool API.
- Around line 1727-1757: collectSkillManifestPaths currently lets fs.readdirSync
errors escape and abort discovery; wrap the readdirSync call in a try/catch
inside collectSkillManifestPaths, log the error (using the class logger
instance, e.g. this.processLogger or this.logger) with context including the
currentDir, then return/continue so siblings are still processed; keep the
existing recursion using shouldIgnoreSkillsRootEntry and SKILL.md detection and
ensure you still respect SKILL_CONFIG.FOLDER_TREE_MAX_DEPTH.
---
Outside diff comments:
In `@src/main/presenter/toolPresenter/agentTools/agentToolManager.ts`:
- Around line 1511-1545: The tool definition for skill_manage is currently
always included even when no conversation context exists; update
getSkillToolDefinitions (or the block adding skill_manage) to only push/return
the skill_manage tool when context.conversationId is present (same gating used
for skill_run), or split getSkillToolDefinitions so the draft-management tool
(skill_manage / schemas.skill_manage) is only exposed when
context.conversationId exists; ensure the server/function entry for skill_manage
is omitted otherwise so callers in skillPresenter/skillTools.ts no longer
receive a tool that will reject without a conversationId.
---
Nitpick comments:
In `@src/shared/types/skill.ts`:
- Around line 146-152: The SkillManageRequest interface allows invalid
combinations; replace it with a discriminated union keyed by action
(SkillManageAction) that mirrors the runtime checks in agentToolManager. Define
one union member per SkillManageAction value, e.g. for actions that require a
draftId (like "edit"/"delete") include draftId: string and disallow
filePath/fileContent; for create/edit include content: string as required; for
file operations (like "write_file") require filePath: string and fileContent:
string and disallow draftId/content; and for any other actions add appropriate
required/forbidden fields. Update the exported SkillManageRequest type and
ensure all callers compile against the new action-specific variants.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7aa91f55-ff88-4c3c-832c-8f32dc432038
📒 Files selected for processing (6)
src/main/presenter/skillPresenter/index.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tssrc/shared/types/skill.tstest/main/presenter/skillPresenter/skillPresenter.test.tstest/main/presenter/skillPresenter/skillTools.test.tstest/main/presenter/toolPresenter/agentTools/agentToolManagerSettings.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/main/presenter/toolPresenter/agentTools/agentToolManagerSettings.test.ts
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
1 participant
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.
Summary by CodeRabbit
New Features
UI Changes
Localization
Tests