feat(chat): add skill support and multi-command input for slash commands#466
Draft
shikihane wants to merge 10 commits intositeboon:mainfrom
Draft
feat(chat): add skill support and multi-command input for slash commands#466shikihane wants to merge 10 commits intositeboon:mainfrom
shikihane wants to merge 10 commits intositeboon:mainfrom
Conversation
- Add scanUserSkills() and scanPluginSkills() to server/routes/commands.js - Expand security checks to allow .claude/skills and .claude/plugins - Add isSkill flag detection in /execute endpoint - Inject skill content via systemPrompt.append in claude-sdk.js - Change slash command selection to autocomplete behavior (insert name instead of execute) - Add multi-command parsing with regex extraction in handleSubmit - Add skill-loaded collapsible card rendering (purple theme) - Support sequential skill loading with combined content - Auto-submit with remaining text after skill loading
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds backend skill discovery/loading and system-prompt skill injection, frontend multi-command slash parsing with sequential skill loading and skill-loaded UI cards, removes onExecuteCommand from slash hook integration, and adds localized "skillLoaded" strings for four locales. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Composer as useChatComposerState
participant Slash as useSlashCommands
participant Backend as server/routes/commands.js
participant SDK as server/claude-sdk.js
participant Claude as Claude Engine
User->>Composer: Type input with one or more /commands
Composer->>Slash: Request parse & detection
Slash->>Backend: /load for each detected command
Backend->>Backend: Scan commands, user skills, plugin skills
Backend-->>Slash: Return command metadata (+ isSkill, SKILL.md content)
Slash-->>Composer: Emit loaded command(s)
Composer->>Composer: Store pending skillContent, render skill-loaded card(s)
alt has userArgs or remaining text
Composer->>SDK: Submit Claude request with skillContent
SDK->>Claude: Inject skillContent into systemPrompt (CLAUDE.md) and send
Claude-->>SDK: Return result
SDK-->>Composer: Deliver response
else no args
Composer->>User: Focus input for user to enter args
end
Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/chat/hooks/useSlashCommands.ts (1)
217-219:⚠️ Potential issue | 🟠 MajorMouse command selection is blocked without a selected project.
At Line 217,
handleCommandSelectexits whenselectedProjectis null, so click selection fails even though command fetch now supports no project and keyboard selection still works.✅ Suggested fix
- if (!command || !selectedProject) { + if (!command) { return; } @@ - [selectedProject, input, slashPosition, setInput, resetCommandMenuState, textareaRef, trackCommandUsage, setSelectedCommandIndex], + [input, slashPosition, setInput, resetCommandMenuState, textareaRef, trackCommandUsage, setSelectedCommandIndex],Also applies to: 247-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useSlashCommands.ts` around lines 217 - 219, The click-selection early return is blocking mouse selection when no project is chosen; in handleCommandSelect remove the guard that requires selectedProject (keep the check for command existence) so clicks can select commands when commands are fetched without a project; apply the same change to the analogous guard at the other location (the similar early-return around line 247) so mouse selection behavior matches keyboard selection while preserving any other validation.server/routes/commands.js (1)
662-685:⚠️ Potential issue | 🔴 CriticalResolve and read real path in
/executeto block symlink escapes.At Line 662 and Line 684, ACL checks are performed on
path.resolve(commandPath)but file content is read fromcommandPath. A symlink under an allowed directory can bypass intended restrictions.🔐 Suggested fix
- const resolvedPath = path.resolve(commandPath); + const resolvedPath = await fs.realpath(path.resolve(commandPath)); @@ - const content = await fs.readFile(commandPath, 'utf8'); + const content = await fs.readFile(resolvedPath, 'utf8');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/commands.js` around lines 662 - 685, The ACL currently checks the resolved path (resolvedPath) but then reads the original commandPath, allowing symlink escapes; fix by resolving the real filesystem path (using fs.realpath or fs.promises.realpath) into a variable (e.g., realPath) and use that same realPath for the isUnder checks (instead of path.resolve(commandPath)) and for reading the file (fs.readFile(realPath, 'utf8')); update references to resolvedPath/commandPath in the isSkill determination and the ACL condition to use the realPath and preserve projectBase/isUnder logic.src/components/chat/hooks/useChatComposerState.ts (1)
848-874:⚠️ Potential issue | 🟡 MinorMissing
handleCustomCommandin dependency array.
handleCustomCommandis called insideloadAllCommands(line 605) but is not included in thehandleSubmitdependency array. This creates a stale closure wherehandleCustomCommandmay reference outdated values ofsetChatMessagesortextareaRef.🐛 Proposed fix
[ attachedImages, claudeModel, codexModel, currentSessionId, cursorModel, executeCommand, geminiModel, + handleCustomCommand, isLoading, onSessionActive,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatComposerState.ts` around lines 848 - 874, The dependency array for handleSubmit is missing handleCustomCommand which is invoked inside loadAllCommands and can cause a stale closure (affecting setChatMessages, textareaRef, etc.); update the dependency array used where handleSubmit is declared to include handleCustomCommand so the handler is recreated when handleCustomCommand changes (locate handleSubmit, loadAllCommands, and handleCustomCommand in useChatComposerState and add handleCustomCommand to the listed dependencies).
🧹 Nitpick comments (1)
src/components/chat/hooks/useChatComposerState.ts (1)
631-635: HardcodedsetTimeout(100)delay is fragile.Using a fixed 100ms delay to wait for React state updates before auto-submitting is unreliable. React's batching behavior isn't guaranteed to complete within this timeframe, especially under load.
Consider using a callback pattern,
queueMicrotask, orrequestAnimationFramefor more reliable timing.♻️ Proposed fix using queueMicrotask
- setTimeout(() => { - if (handleSubmitRef.current) { - handleSubmitRef.current(createFakeSubmitEvent()); - } - }, 100); + // Use queueMicrotask for more reliable timing after state updates + queueMicrotask(() => { + requestAnimationFrame(() => { + if (handleSubmitRef.current) { + handleSubmitRef.current(createFakeSubmitEvent()); + } + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatComposerState.ts` around lines 631 - 635, Replace the fragile setTimeout(100) auto-submit with a microtask or frame-based scheduling so submission runs after React state updates: instead of setTimeout in useChatComposerState, invoke queueMicrotask (or requestAnimationFrame) to call handleSubmitRef.current(createFakeSubmitEvent()) once state updates settle; update the block that references handleSubmitRef and createFakeSubmitEvent to use queueMicrotask(() => { if (handleSubmitRef.current) handleSubmitRef.current(createFakeSubmitEvent()); }) (or requestAnimationFrame) to ensure reliable timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-03-01-upstream-pr-contribution.md`:
- Line 146: Update the checklist item text to use a hyphenated compound
adjective: change “emoji generated lines” to “emoji-generated lines” in the list
entry that currently reads "No `Co-Authored-By` or emoji generated lines in
commits" so the item becomes "No `Co-Authored-By` or emoji-generated lines in
commits".
In `@server/routes/commands.js`:
- Around line 581-588: The current ACL uses substring checks on resolvedPath
which can be bypassed; replace it with explicit containment checks against
canonical allowed directories: compute allowed dirs with
path.resolve(os.homedir(), '.claude/commands'), path.resolve(os.homedir(),
'.claude/skills'), and path.resolve(os.homedir(), '.claude/plugins'), then
verify resolvedPath is inside one of those using a robust containment test
(e.g., path.relative(allowedDir, resolvedPath) and ensure it does not start with
'..' and is not equal to '' in an unexpected way, or check
resolvedPath.startsWith(allowedDir + path.sep) or equals allowedDir). Update the
if that currently references resolvedPath, path.resolve(os.homedir()), and
includes() to deny access unless resolvedPath is contained within one of the
three allowed resolved directories.
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 647-658: The call to loadAllCommands() is fire-and-forget and can
race with clearing UI state/pendingSkillContentRef; modify the
useChatComposerState flow so loadAllCommands is awaited or guarded: either await
loadAllCommands() before continuing to clear UI state (so pendingSkillContentRef
cannot be overwritten mid-load) or introduce a guard like isLoadingCommandsRef /
isLoadingCommands flag checked/set around the loadAllCommands invocation (set
true before, false after, and skip concurrent loads) and ensure
pendingSkillContentRef updates are only allowed when the load is complete;
update the code around the loadAllCommands() call in useChatComposerState
accordingly.
In `@src/components/chat/view/subcomponents/MessageComponent.tsx`:
- Around line 113-119: The UI branch in MessageComponent.tsx uses hardcoded text
and unsafe (message as any) casts for skillName/skillDescription; instead define
and use a typed message shape (e.g., interface SkillLoadedMessage { type:
"skill-loaded"; skillName: string; skillDescription?: string }) and narrow the
union by checking message.type === "skill-loaded" so you can access
message.skillName and message.skillDescription without any, and replace the
hardcoded "Loaded skill:" label with a translation call (e.g.,
t('skillLoaded.label')) and any small text like description fallbacks via i18n
keys (e.g., t('skillLoaded.untitled')) using your existing translation
hook/function to ensure type-safety and localization.
In `@task_plan.md`:
- Around line 1-112: The task_plan.md (Scheduled Task Management System) is out
of scope for this PR and must be removed from the current change set and moved
to its own PR: revert or remove task_plan.md from this branch/commit (so it no
longer appears in the current PR), create a new branch that adds task_plan.md
(and any related planning docs) and open a dedicated PR titled something like
"RFC: Scheduled Task Management System / Cron Scheduler" that contains the plan
and discussion only, and update the original PR description to reference the new
PR/branch and keep this PR focused on slash-command/skills; use the filename
task_plan.md to locate the file to move and ensure related references in
README/PR body are updated accordingly.
---
Outside diff comments:
In `@server/routes/commands.js`:
- Around line 662-685: The ACL currently checks the resolved path (resolvedPath)
but then reads the original commandPath, allowing symlink escapes; fix by
resolving the real filesystem path (using fs.realpath or fs.promises.realpath)
into a variable (e.g., realPath) and use that same realPath for the isUnder
checks (instead of path.resolve(commandPath)) and for reading the file
(fs.readFile(realPath, 'utf8')); update references to resolvedPath/commandPath
in the isSkill determination and the ACL condition to use the realPath and
preserve projectBase/isUnder logic.
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 848-874: The dependency array for handleSubmit is missing
handleCustomCommand which is invoked inside loadAllCommands and can cause a
stale closure (affecting setChatMessages, textareaRef, etc.); update the
dependency array used where handleSubmit is declared to include
handleCustomCommand so the handler is recreated when handleCustomCommand changes
(locate handleSubmit, loadAllCommands, and handleCustomCommand in
useChatComposerState and add handleCustomCommand to the listed dependencies).
In `@src/components/chat/hooks/useSlashCommands.ts`:
- Around line 217-219: The click-selection early return is blocking mouse
selection when no project is chosen; in handleCommandSelect remove the guard
that requires selectedProject (keep the check for command existence) so clicks
can select commands when commands are fetched without a project; apply the same
change to the analogous guard at the other location (the similar early-return
around line 247) so mouse selection behavior matches keyboard selection while
preserving any other validation.
---
Nitpick comments:
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 631-635: Replace the fragile setTimeout(100) auto-submit with a
microtask or frame-based scheduling so submission runs after React state
updates: instead of setTimeout in useChatComposerState, invoke queueMicrotask
(or requestAnimationFrame) to call
handleSubmitRef.current(createFakeSubmitEvent()) once state updates settle;
update the block that references handleSubmitRef and createFakeSubmitEvent to
use queueMicrotask(() => { if (handleSubmitRef.current)
handleSubmitRef.current(createFakeSubmitEvent()); }) (or requestAnimationFrame)
to ensure reliable timing.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
docs/plans/2026-03-01-task4-slash-commands-design.mddocs/plans/2026-03-01-upstream-pr-contribution-design.mddocs/plans/2026-03-01-upstream-pr-contribution.mdserver/claude-sdk.jsserver/routes/commands.jssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useSlashCommands.tssrc/components/chat/view/subcomponents/MessageComponent.tsxtask_plan.md
| - [ ] `npm run build` passes (run `npm install` first if dependencies changed) | ||
| - [ ] Commit messages follow Conventional Commits | ||
| - [ ] UI changes include screenshots in PR body | ||
| - [ ] No `Co-Authored-By` or emoji generated lines in commits |
Contributor
There was a problem hiding this comment.
Minor wording cleanup in checklist item.
At Line 146, “emoji generated lines” reads better as “emoji-generated lines”.
✏️ Suggested edit
-- [ ] No `Co-Authored-By` or emoji generated lines in commits
+- [ ] No `Co-Authored-By` or emoji-generated lines in commits📝 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
| - [ ] No `Co-Authored-By` or emoji generated lines in commits | |
| - [ ] No `Co-Authored-By` or emoji-generated lines in commits |
🧰 Tools
🪛 LanguageTool
[grammar] ~146-~146: Use a hyphen to join words.
Context: ... body - [ ] No Co-Authored-By or emoji generated lines in commits
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-01-upstream-pr-contribution.md` at line 146, Update the
checklist item text to use a hyphenated compound adjective: change “emoji
generated lines” to “emoji-generated lines” in the list entry that currently
reads "No `Co-Authored-By` or emoji generated lines in commits" so the item
becomes "No `Co-Authored-By` or emoji-generated lines in commits".
Comment on lines
+113
to
+119
| <div className="text-sm font-medium text-purple-900 dark:text-purple-100"> | ||
| Loaded skill: {(message as any).skillName || 'skill'} | ||
| </div> | ||
| {(message as any).skillDescription && ( | ||
| <div className="text-xs text-purple-700 dark:text-purple-300 mt-0.5 truncate"> | ||
| {(message as any).skillDescription} | ||
| </div> |
Contributor
There was a problem hiding this comment.
Localize skill-card labels and avoid any casts in this branch.
At Line 114 and Line 116, the new UI uses hardcoded text and (message as any). This introduces avoidable i18n drift and type-safety loss in a new message type path. Prefer translated keys and a typed skill-loaded message shape.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/view/subcomponents/MessageComponent.tsx` around lines 113
- 119, The UI branch in MessageComponent.tsx uses hardcoded text and unsafe
(message as any) casts for skillName/skillDescription; instead define and use a
typed message shape (e.g., interface SkillLoadedMessage { type: "skill-loaded";
skillName: string; skillDescription?: string }) and narrow the union by checking
message.type === "skill-loaded" so you can access message.skillName and
message.skillDescription without any, and replace the hardcoded "Loaded skill:"
label with a translation call (e.g., t('skillLoaded.label')) and any small text
like description fallbacks via i18n keys (e.g., t('skillLoaded.untitled')) using
your existing translation hook/function to ensure type-safety and localization.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
docs/plans/2026-03-01-upstream-pr-contribution.md (1)
101-101:⚠️ Potential issue | 🟡 MinorMinor wording cleanup in checklist item.
The static analysis tool correctly identifies that "emoji generated lines" should be hyphenated as "emoji-generated lines" to form a compound adjective.
✏️ Suggested fix
-- [ ] No `Co-Authored-By` or emoji generated lines in commits +- [ ] No `Co-Authored-By` or emoji-generated lines in commits🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-01-upstream-pr-contribution.md` at line 101, Update the checklist line to use the compound adjective form by changing "emoji generated lines" to "emoji-generated lines" (so the line reads: No `Co-Authored-By` or emoji-generated lines in commits); ensure the backticks around Co-Authored-By remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-03-01-task4-slash-commands-design.md`:
- Around line 26-28: Update the design doc to explicitly document
content-security measures for skill injection: state that scanUserSkills() and
scanPluginSkills() must call a new validateSkillContent()/sanitizeSkillContent()
routine before passing text to systemPrompt.append, describe reasonable
constraints (e.g., MAX_SKILL_SIZE bytes, allowed Markdown subset or schema, max
heading/section counts), explain how malformed or malicious content is handled
(reject+log, strip disallowed constructs, or load in read-only sandbox) and note
that security checks beyond path validation (content validation, size limits,
logging, and failure modes) are required and where they integrate (during
scanning and before setting isSkill on execute responses).
- Around line 76-84: Add concrete security validations when implementing
scanUserSkills(skillsDir) and scanPluginSkills(pluginsDir): resolve each
SKILL.md to an absolute path and ensure it stays inside the allowed base
(prevent path traversal), reject or carefully validate symlinks (deny links that
resolve outside the allowed dir), enforce a maximum file size (e.g., 100KB) and
read files with size checks, and catch/read errors to return sanitized errors
(no system paths). When updating the /list handler, call both scanUserSkills and
scanPluginSkills and merge results while filtering out any entries that failed
validation; for /load expand the allowlist to include .claude/skills/ and
.claude/plugins/ but validate requested paths by canonicalizing and ensuring
they remain within those directories before loading; for /execute detect isSkill
by checking that the canonicalized path is under the skills or plugins skill
dirs and include that flag in the response. Ensure all path checks use
canonicalized/realpath resolution and never trust raw input paths.
- Around line 151-160: Update the "Checklist Per PR Rules" section so it
includes the same "emoji-generated lines" constraint text used in the main
contribution plan (the document titled 2026-03-01-upstream-pr-contribution.md)
to ensure identical wording; modify the checklist under the heading "Checklist
Per PR Rules" to add the missing checkbox line exactly matching the main plan's
phrasing and formatting so both documents remain consistent.
In `@docs/plans/2026-03-01-upstream-pr-contribution.md`:
- Around line 91-102: The two checklists are inconsistent: upstream PR checklist
in docs/plans/2026-03-01-upstream-pr-contribution.md contains "No
`Co-Authored-By` or emoji-generated lines in commits" but
docs/plans/2026-03-01-task4-slash-commands-design.md only says "No
`Co-Authored-By` lines"; make them consistent by either adding "or
emoji-generated lines" to the Task 4 checklist in
docs/plans/2026-03-01-task4-slash-commands-design.md to match the upstream
checklist, or remove the emoji constraint from the upstream checklist and
include a short rationale comment in one of the files if the discrepancy is
intentional.
---
Duplicate comments:
In `@docs/plans/2026-03-01-upstream-pr-contribution.md`:
- Line 101: Update the checklist line to use the compound adjective form by
changing "emoji generated lines" to "emoji-generated lines" (so the line reads:
No `Co-Authored-By` or emoji-generated lines in commits); ensure the backticks
around Co-Authored-By remain unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/plans/2026-03-01-task4-slash-commands-design.mddocs/plans/2026-03-01-upstream-pr-contribution.md
Comment on lines
+26
to
+28
| **Files:** | ||
| - `server/routes/commands.js` — Add `scanUserSkills()` for `~/.claude/skills/`, `scanPluginSkills()` for `~/.claude/plugins/`, `isSkill` flag on execute response, security checks for new paths | ||
| - `server/claude-sdk.js` — Add `systemPrompt.append = options.skillContent` (do NOT include `taskOutputFallbackHint` — that belongs to Task 5) |
Contributor
There was a problem hiding this comment.
Consider documenting security measures for skill content injection.
Enhancement 1 introduces scanning and loading of SKILL.md files from ~/.claude/skills/ and ~/.claude/plugins/, with the content being injected into the system prompt via systemPrompt.append. However, the design doc does not mention validation, sanitization, or security checks on the skill content before injection.
While the files are read from user-controlled directories (which may imply trust), it would be valuable to document:
- Whether skill content undergoes any validation or sanitization before injection
- Size limits or format constraints on
SKILL.mdfiles - How the system handles malformed or malicious skill content
- Whether the security checks mentioned in Step 2 (line 82) extend beyond path validation to content validation
This is especially important since the injected content becomes part of the system prompt that influences AI behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-01-task4-slash-commands-design.md` around lines 26 - 28,
Update the design doc to explicitly document content-security measures for skill
injection: state that scanUserSkills() and scanPluginSkills() must call a new
validateSkillContent()/sanitizeSkillContent() routine before passing text to
systemPrompt.append, describe reasonable constraints (e.g., MAX_SKILL_SIZE
bytes, allowed Markdown subset or schema, max heading/section counts), explain
how malformed or malicious content is handled (reject+log, strip disallowed
constructs, or load in read-only sandbox) and note that security checks beyond
path validation (content validation, size limits, logging, and failure modes)
are required and where they integrate (during scanning and before setting
isSkill on execute responses).
Comment on lines
+76
to
+84
|
|
||
| Add skill scanning functions and integrate into endpoints: | ||
|
|
||
| 1. Add `scanUserSkills(skillsDir)` function (~40 lines) — scans `~/.claude/skills/` for `SKILL.md` files | ||
| 2. Add `scanPluginSkills(pluginsDir)` function (~60 lines) — reads `installed_plugins.json`, scans each plugin's `skills/` dir | ||
| 3. Modify `/list` endpoint — call both scan functions, append results to command list | ||
| 4. Modify `/load` endpoint — expand security check to allow `.claude/skills/` and `.claude/plugins/` | ||
| 5. Modify `/execute` endpoint — detect `isSkill` based on path, include in response | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Implementation steps should reference security best practices.
Step 2 mentions expanding security checks to allow .claude/skills/ and .claude/plugins/ paths (line 82), but the design doesn't specify what security validations are performed during scanning. Consider adding guidance on:
- Path traversal prevention (ensuring
SKILL.mdfiles don't reference paths outside allowed directories) - Symbolic link handling
- File size limits during scanning
- Error handling for inaccessible or malformed files
These details would help ensure the implementation is secure and robust.
📝 Suggested addition to Step 2
Add a security considerations subsection:
**Security considerations for Step 2:**
- Validate that resolved paths remain within `.claude/skills/` or `.claude/plugins/`
- Reject symbolic links pointing outside allowed directories
- Enforce maximum file size limit (e.g., 100KB per SKILL.md)
- Handle read errors gracefully without exposing system paths🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-01-task4-slash-commands-design.md` around lines 76 - 84,
Add concrete security validations when implementing scanUserSkills(skillsDir)
and scanPluginSkills(pluginsDir): resolve each SKILL.md to an absolute path and
ensure it stays inside the allowed base (prevent path traversal), reject or
carefully validate symlinks (deny links that resolve outside the allowed dir),
enforce a maximum file size (e.g., 100KB) and read files with size checks, and
catch/read errors to return sanitized errors (no system paths). When updating
the /list handler, call both scanUserSkills and scanPluginSkills and merge
results while filtering out any entries that failed validation; for /load expand
the allowlist to include .claude/skills/ and .claude/plugins/ but validate
requested paths by canonicalizing and ensuring they remain within those
directories before loading; for /execute detect isSkill by checking that the
canonicalized path is under the skills or plugins skill dirs and include that
flag in the response. Ensure all path checks use canonicalized/realpath
resolution and never trust raw input paths.
Comment on lines
+151
to
+160
| ## Checklist Per PR Rules | ||
|
|
||
| - [ ] Branch based on latest `main` | ||
| - [ ] No `debug:` commits | ||
| - [ ] No cross-feature changes (no Gemini removal, no scroll fixes, no background task code) | ||
| - [ ] `npm run build` passes | ||
| - [ ] `npm run typecheck` passes | ||
| - [ ] Commit messages follow Conventional Commits | ||
| - [ ] No `Co-Authored-By` lines | ||
|
|
Contributor
There was a problem hiding this comment.
Checklist differs from main contribution plan.
This checklist omits the "emoji-generated lines" constraint that appears in the main contribution plan document (docs/plans/2026-03-01-upstream-pr-contribution.md line 101). For consistency across planning documents, both checklists should use identical wording.
🔄 Align with main plan checklist
-- [ ] No `Co-Authored-By` lines
+- [ ] No `Co-Authored-By` or emoji-generated lines in commits🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-01-task4-slash-commands-design.md` around lines 151 - 160,
Update the "Checklist Per PR Rules" section so it includes the same
"emoji-generated lines" constraint text used in the main contribution plan (the
document titled 2026-03-01-upstream-pr-contribution.md) to ensure identical
wording; modify the checklist under the heading "Checklist Per PR Rules" to add
the missing checkbox line exactly matching the main plan's phrasing and
formatting so both documents remain consistent.
Comment on lines
+91
to
+102
| ## Checklist per PR | ||
|
|
||
| Before pushing each PR, verify: | ||
|
|
||
| - [ ] Branch is based on latest `main` (which tracks `upstream/main`) | ||
| - [ ] No `debug:` commits included | ||
| - [ ] No cross-feature changes leaked in | ||
| - [ ] `npm run build` passes (run `npm install` first if dependencies changed) | ||
| - [ ] Commit messages follow Conventional Commits | ||
| - [ ] UI changes include screenshots in PR body | ||
| - [ ] No `Co-Authored-By` or emoji generated lines in commits | ||
| - [ ] Use `git add <specific files>` — never `git add -A` |
Contributor
There was a problem hiding this comment.
Inconsistent checklist wording with Task 4 design doc.
The checklist at line 101 states "No Co-Authored-By or emoji-generated lines in commits", but the corresponding checklist in docs/plans/2026-03-01-task4-slash-commands-design.md at line 159 only mentions "No Co-Authored-By lines" without the emoji-generated constraint. Since both documents serve as checklists for the same PR contribution process, they should use identical wording to avoid confusion.
🔄 Recommended alignment
Either update line 159 of the task design doc to match this checklist, or document why the emoji constraint applies globally but not to Task 4 specifically.
🧰 Tools
🪛 LanguageTool
[grammar] ~101-~101: Use a hyphen to join words.
Context: ... body - [ ] No Co-Authored-By or emoji generated lines in commits - [ ] Use `gi...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-01-upstream-pr-contribution.md` around lines 91 - 102, The
two checklists are inconsistent: upstream PR checklist in
docs/plans/2026-03-01-upstream-pr-contribution.md contains "No `Co-Authored-By`
or emoji-generated lines in commits" but
docs/plans/2026-03-01-task4-slash-commands-design.md only says "No
`Co-Authored-By` lines"; make them consistent by either adding "or
emoji-generated lines" to the Task 4 checklist in
docs/plans/2026-03-01-task4-slash-commands-design.md to match the upstream
checklist, or remove the emoji constraint from the upstream checklist and
include a short rationale comment in one of the files if the discrepancy is
intentional.
shikihane
force-pushed
the
feat/slash-commands-skills
branch
from
68f5272 to
074993d
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/plans/2026-03-01-upstream-pr-contribution.md (1)
109-109:⚠️ Potential issue | 🟡 MinorHyphenation: "emoji generated" → "emoji-generated".
Compound adjectives should be hyphenated. Change "emoji generated lines" to "emoji-generated lines".
✏️ Suggested fix
-- [ ] No `Co-Authored-By` or emoji generated lines in commits +- [ ] No `Co-Authored-By` or emoji-generated lines in commits🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-01-upstream-pr-contribution.md` at line 109, Update the checklist item that reads "No `Co-Authored-By` or emoji generated lines in commits" by hyphenating the compound adjective to "emoji-generated" so it reads "No `Co-Authored-By` or emoji-generated lines in commits"; locate the exact string "- [ ] No `Co-Authored-By` or emoji generated lines in commits" and replace "emoji generated" with "emoji-generated".
🧹 Nitpick comments (1)
docs/plans/2026-03-01-upstream-pr-contribution.md (1)
37-42: Add language specification to code block.The fenced code block is missing a language identifier, which reduces readability and disables syntax highlighting. Consider adding
bashortextafter the opening fence.📝 Suggested improvement
-``` +```bash git checkout main 然后执行 Task 5 brainstorming:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-01-upstream-pr-contribution.md` around lines 37 - 42, The fenced code block containing the lines starting with "git checkout main" and the subsequent "然后执行 Task 5 brainstorming:" is missing a language tag; update the opening triple backticks to include a language (e.g., ```bash or ```text) so the block becomes ```bash and retains the same inner content, improving readability and enabling syntax highlighting for the command snippet and Task 5 instructions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/plans/2026-03-01-upstream-pr-contribution.md`:
- Line 109: Update the checklist item that reads "No `Co-Authored-By` or emoji
generated lines in commits" by hyphenating the compound adjective to
"emoji-generated" so it reads "No `Co-Authored-By` or emoji-generated lines in
commits"; locate the exact string "- [ ] No `Co-Authored-By` or emoji generated
lines in commits" and replace "emoji generated" with "emoji-generated".
---
Nitpick comments:
In `@docs/plans/2026-03-01-upstream-pr-contribution.md`:
- Around line 37-42: The fenced code block containing the lines starting with
"git checkout main" and the subsequent "然后执行 Task 5 brainstorming:" is missing a
language tag; update the opening triple backticks to include a language (e.g.,
```bash or ```text) so the block becomes ```bash and retains the same inner
content, improving readability and enabling syntax highlighting for the command
snippet and Task 5 instructions.
Add comprehensive design document for migrating background tasks management feature from feature/personal-enhancements to upstream/main. Design includes: - Architecture overview with at-least-once event delivery - Migration strategy with adaptation points - Frontend integration with WebSocket state management - Data flow and state synchronization - Error handling and edge cases - Complete implementation checklist Estimated effort: 8-12 hours
- Replace substring-based ACL in /load endpoint with isUnder() path validation matching /execute endpoint pattern - Add fs.realpath() to both /load and /execute to prevent symlink escapes - Remove unnecessary selectedProject null guard in handleCommandSelect - Replace setTimeout(100) with queueMicrotask+requestAnimationFrame - Add missing handleCustomCommand to handleSubmit dependency array - Remove task_plan.md (out of scope for this PR) - Internationalize "Loaded skill:" text in MessageComponent
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/chat/hooks/useSlashCommands.ts (1)
194-199:⚠️ Potential issue | 🟠 MajorGuard against
slashPosition === -1before replacing input text.When the command menu is opened without an active slash query,
slashPositioncan be-1. At Line 194 and Line 229 this causes slicing from the end of the string and corrupts the composer content.💡 Proposed fix
- const textBeforeSlash = input.slice(0, slashPosition); - const textAfterSlash = input.slice(slashPosition); + const effectiveSlashPos = + slashPosition >= 0 ? slashPosition : (textareaRef.current?.selectionStart ?? input.length); + const textBeforeSlash = input.slice(0, effectiveSlashPos); + const textAfterSlash = input.slice(effectiveSlashPos);Also applies to: 229-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useSlashCommands.ts` around lines 194 - 199, In useSlashCommands, guard against slashPosition === -1 before slicing/rewriting the input: if slashPosition is -1, skip the replacement logic (do not compute textBeforeSlash/textAfterSlash or construct newInput) and return/abort the command insertion early; apply the same guard to the second replacement block that constructs newInput around lines where textBeforeSlash/textAfterSlash are used (the other occurrence that replaces input when a command is selected) so slices are only performed when slashPosition is a valid index.
♻️ Duplicate comments (2)
src/components/chat/hooks/useChatComposerState.ts (1)
649-660:⚠️ Potential issue | 🟠 Major
loadAllCommands()should be awaited to avoid state races.At Line 649,
loadAllCommands()is fire-and-forget, but UI/input state is immediately cleared afterward (Lines 651-660). This can race with pending skill-content updates and auto-submit behavior.💡 Proposed fix
- loadAllCommands(); + await loadAllCommands(); @@ - // Clear UI state + // Clear UI state after command/skill loading is settled🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatComposerState.ts` around lines 649 - 660, The call to loadAllCommands() is currently fire-and-forget and races with the subsequent UI clears (setAttachedImages, setUploadingImages, setImageErrors, resetCommandMenuState, setIsTextareaExpanded and resetting textareaRef.current.style.height); change the call to await loadAllCommands() and ensure the enclosing function is marked async (or propagate async up) so that command loading completes before clearing UI state to prevent skill-content/update races and unintended auto-submit behavior.server/routes/commands.js (1)
571-572:⚠️ Potential issue | 🟠 Major
/loadno longer allows project command files.Line 571 only reads
commandPath, and Lines 589-593 restrict access to user-level bases. This breaks loading project commands listed from.claude/commandsunder the selected project.💡 Proposed fix
- const { commandPath } = req.body; + const { commandPath, projectPath } = req.body; @@ - if (!(isUnder(userCommandsBase) || isUnder(userSkillsBase) || isUnder(userPluginsBase))) { + const projectCommandsBase = projectPath + ? path.resolve(path.join(projectPath, '.claude', 'commands')) + : null; + + if ( + !( + isUnder(userCommandsBase) || + isUnder(userSkillsBase) || + isUnder(userPluginsBase) || + (projectCommandsBase && isUnder(projectCommandsBase)) + ) + ) { return res.status(403).json({Also applies to: 589-593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/commands.js` around lines 571 - 572, The /load handler currently only reads commandPath and then enforces that bases are user-level, which prevents loading project-scoped command files (e.g., .claude/commands in a selected project). Update the /load request handling to accept and preserve project-scoped command locations (inspect req.body for project identifier or allow commandPath values that resolve under the selected project), and change the access-check logic (the block that currently restricts to user bases) to also permit project bases when a project is selected (e.g., allow base === 'user' OR base denotes the active project or matches the provided projectId/projectKey). Ensure you reference and update commandPath resolution and the authorization condition in the /load route handler so project commands under .claude/commands can be loaded.
🧹 Nitpick comments (1)
docs/plans/2026-03-01-task5-background-tasks-design.md (1)
1-1006: Split this design doc into a separate PR to keep scope focused.This file is about background-task migration, while this PR is scoped to slash commands/skills. Mixing unrelated design docs makes review and rollback harder.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-01-task5-background-tasks-design.md` around lines 1 - 1006, This design doc for Background Tasks Management is unrelated to the current slash-commands/skills PR; move it into its own focused PR by removing or reverting this document from the current changeset and creating a new branch/PR that contains the design doc and its listed artifacts (references: "Background Tasks Management - Design Document", new file names ws-clients.js, claude-sdk.js, index.js, BackgroundTasksPopover.tsx, BackgroundTasksPage.tsx and the i18n additions); ensure the original PR only contains slash-commands/skills changes and add a short PR description referencing the separate design-doc PR for reviewers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-03-01-task5-background-tasks-design.md`:
- Around line 89-90: The document contains conflicting SDK version references
("0.2.59" vs "0.1.71") that can cause migration errors; choose a single
authoritative baseline version (or explicitly mark the older version as
"historical/legacy") and update all mentions accordingly (including the SDK
bullet, the `claude-sdk.js` session-management note, and the other referenced
occurrences at the indicated sections), and add a short note where appropriate
explaining why the older version is mentioned (e.g., "legacy behavior in
0.1.71") so readers clearly know which version to target for current guidance.
- Around line 37-77: Add explicit language identifiers (e.g., text) to the
fenced code blocks that contain ASCII diagrams and plain text sections so
markdownlint MD040 is satisfied; update the diagram block that starts with the
frontend/backend box art, the block listing "AppContent.tsx", and the numbered
"1. Background Task Start" block (and the other similar blocks noted in the
review) by changing ``` to ```text for each fenced block. Ensure all
plain-text/diagram fences throughout the document (including the other affected
ranges) are consistently marked as text.
- Line 814: Replace the British phrasing "in future" with US-English "in the
future" in the sentence that currently reads "If SDK supports termination API in
future, update this logic" (update this exact string to "If SDK supports
termination API in the future, update this logic") so the document uses
consistent US-English phrasing.
In `@server/routes/commands.js`:
- Around line 71-73: The current extraction of pluginName using
pluginKey.split('@')[0] fails for scoped package keys like `@scope/pkg`@1.2.3;
update the extraction in the Object.entries(plugins) loop to strip only the
trailing version suffix (the last @ and everything after) so scoped names retain
their scope (use pluginKey.lastIndexOf('@') to find the final '@' and slice
before it, falling back to the whole pluginKey when no version suffix exists).
Replace the pluginName assignment that currently uses pluginKey.split('@')[0]
with this safer logic referencing pluginKey and pluginName.
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 603-606: The code treats any non-skill command as a custom command
and skips built-ins; update the branch that checks result.isSkill to explicitly
handle result.type === 'builtin' (the API can return type: 'builtin') by
dispatching to a built-in handler (e.g., implement or call
handleBuiltInCommand/executeBuiltInCommand) and returning early, otherwise
continue to call handleCustomCommand({ ...result, userArgs: remainingText });
reference result.isSkill, result.type, and handleCustomCommand in your fix.
---
Outside diff comments:
In `@src/components/chat/hooks/useSlashCommands.ts`:
- Around line 194-199: In useSlashCommands, guard against slashPosition === -1
before slicing/rewriting the input: if slashPosition is -1, skip the replacement
logic (do not compute textBeforeSlash/textAfterSlash or construct newInput) and
return/abort the command insertion early; apply the same guard to the second
replacement block that constructs newInput around lines where
textBeforeSlash/textAfterSlash are used (the other occurrence that replaces
input when a command is selected) so slices are only performed when
slashPosition is a valid index.
---
Duplicate comments:
In `@server/routes/commands.js`:
- Around line 571-572: The /load handler currently only reads commandPath and
then enforces that bases are user-level, which prevents loading project-scoped
command files (e.g., .claude/commands in a selected project). Update the /load
request handling to accept and preserve project-scoped command locations
(inspect req.body for project identifier or allow commandPath values that
resolve under the selected project), and change the access-check logic (the
block that currently restricts to user bases) to also permit project bases when
a project is selected (e.g., allow base === 'user' OR base denotes the active
project or matches the provided projectId/projectKey). Ensure you reference and
update commandPath resolution and the authorization condition in the /load route
handler so project commands under .claude/commands can be loaded.
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 649-660: The call to loadAllCommands() is currently
fire-and-forget and races with the subsequent UI clears (setAttachedImages,
setUploadingImages, setImageErrors, resetCommandMenuState, setIsTextareaExpanded
and resetting textareaRef.current.style.height); change the call to await
loadAllCommands() and ensure the enclosing function is marked async (or
propagate async up) so that command loading completes before clearing UI state
to prevent skill-content/update races and unintended auto-submit behavior.
---
Nitpick comments:
In `@docs/plans/2026-03-01-task5-background-tasks-design.md`:
- Around line 1-1006: This design doc for Background Tasks Management is
unrelated to the current slash-commands/skills PR; move it into its own focused
PR by removing or reverting this document from the current changeset and
creating a new branch/PR that contains the design doc and its listed artifacts
(references: "Background Tasks Management - Design Document", new file names
ws-clients.js, claude-sdk.js, index.js, BackgroundTasksPopover.tsx,
BackgroundTasksPage.tsx and the i18n additions); ensure the original PR only
contains slash-commands/skills changes and add a short PR description
referencing the separate design-doc PR for reviewers.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/plans/2026-03-01-task5-background-tasks-design.mdserver/routes/commands.jssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useSlashCommands.tssrc/components/chat/view/subcomponents/MessageComponent.tsxsrc/i18n/locales/en/chat.jsonsrc/i18n/locales/ja/chat.jsonsrc/i18n/locales/ko/chat.jsonsrc/i18n/locales/zh-CN/chat.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/chat/view/subcomponents/MessageComponent.tsx
Comment on lines
+37
to
+77
| ``` | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ Frontend │ | ||
| │ ┌──────────────────┐ ┌─────────────────────────┐ │ | ||
| │ │ BackgroundTasks │◄────────┤ WebSocketContext │ │ | ||
| │ │ Popover (Drawer) │ │ (subscribe/sendMessage)│ │ | ||
| │ └──────────────────┘ └─────────────────────────┘ │ | ||
| │ ┌──────────────────┐ │ | ||
| │ │ BackgroundTasks │ │ | ||
| │ │ Page (Standalone)│ │ | ||
| │ └──────────────────┘ │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| │ WebSocket | ||
| ▼ | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ Backend (server/) │ | ||
| │ ┌──────────────────────────────────────────────────────┐ │ | ||
| │ │ ws-clients.js (NEW) │ │ | ||
| │ │ - connectedClients registry │ │ | ||
| │ │ - pendingEvents queue (at-least-once delivery) │ │ | ||
| │ │ - emitTaskEvent() / ackEvent() / syncPendingEvents() │ │ | ||
| │ └──────────────────────────────────────────────────────┘ │ | ||
| │ │ │ | ||
| │ ┌──────────────────────────▼───────────────────────────┐ │ | ||
| │ │ claude-sdk.js (ENHANCED) │ │ | ||
| │ │ - monitorSubagentCompletion() (fd-based polling) │ │ | ||
| │ │ - monitorBackgroundBash() (fd-based polling) │ │ | ||
| │ │ - backgroundTasks Map (taskId → task info) │ │ | ||
| │ │ - backgroundTaskOutputs Map (taskId → output cache) │ │ | ||
| │ └───────────────────────────────────────────────────────┘ │ | ||
| │ │ │ | ||
| │ ┌──────────────────────────▼───────────────────────────┐ │ | ||
| │ │ index.js (ENHANCED) │ │ | ||
| │ │ - WebSocket message handlers: │ │ | ||
| │ │ • query-task-output │ │ | ||
| │ │ • kill-background-task │ │ | ||
| │ │ • sync-background-events │ │ | ||
| │ │ • ack-event │ │ | ||
| │ └───────────────────────────────────────────────────────┘ │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to satisfy markdownlint (MD040).
The affected blocks should use an explicit language (for example text for diagrams/tree output).
Suggested patch
-```
+```text
┌─────────────────────────────────────────────────────────────┐
...
-```
+```
-```
+```text
AppContent.tsx
...
-```
+```
-```
+```text
1. Background Task Start
...
-```
+```Also applies to: 246-253, 489-532
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-01-task5-background-tasks-design.md` around lines 37 - 77,
Add explicit language identifiers (e.g., text) to the fenced code blocks that
contain ASCII diagrams and plain text sections so markdownlint MD040 is
satisfied; update the diagram block that starts with the frontend/backend box
art, the block listing "AppContent.tsx", and the numbered "1. Background Task
Start" block (and the other similar blocks noted in the review) by changing ```
to ```text for each fenced block. Ensure all plain-text/diagram fences
throughout the document (including the other affected ranges) are consistently
marked as text.
Comment on lines
+89
to
+90
| - SDK upgraded to 0.2.59 (added model usage logging) | ||
| - `claude-sdk.js` session management refactored |
Contributor
There was a problem hiding this comment.
Resolve conflicting SDK version assumptions in the design.
The doc states upstream is on 0.2.59 but later cites a current limitation in 0.1.71. Keep one authoritative version baseline (or clearly mark historical context) to avoid migration mistakes.
Also applies to: 106-107, 130-130, 789-790
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-01-task5-background-tasks-design.md` around lines 89 - 90,
The document contains conflicting SDK version references ("0.2.59" vs "0.1.71")
that can cause migration errors; choose a single authoritative baseline version
(or explicitly mark the older version as "historical/legacy") and update all
mentions accordingly (including the SDK bullet, the `claude-sdk.js`
session-management note, and the other referenced occurrences at the indicated
sections), and add a short note where appropriate explaining why the older
version is mentioned (e.g., "legacy behavior in 0.1.71") so readers clearly know
which version to target for current guidance.
| - Tooltip explains: "Remove from list (task continues running)" | ||
|
|
||
| **Future Improvement**: | ||
| - If SDK supports termination API in future, update this logic |
Contributor
There was a problem hiding this comment.
Use US-English phrasing for consistency: “in the future”.
Update “in future” to “in the future”.
🧰 Tools
🪛 LanguageTool
[locale-violation] ~814-~814: The phrase ‘in future’ is British English. Did you mean: “in the future”?
Context: ...nt**: - If SDK supports termination API in future, update this logic - For bash tasks, ca...
(IN_FUTURE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-01-task5-background-tasks-design.md` at line 814, Replace
the British phrasing "in future" with US-English "in the future" in the sentence
that currently reads "If SDK supports termination API in future, update this
logic" (update this exact string to "If SDK supports termination API in the
future, update this logic") so the document uses consistent US-English phrasing.
- Remove unrelated background-tasks design doc from PR scope - Guard against slashPosition === -1 before slicing input in both selectCommandFromKeyboard and handleCommandSelect - Allow /load endpoint to accept project-scoped commands (.claude/commands in project directory), matching /execute endpoint behavior - Await loadAllCommands() before clearing UI state to prevent race conditions with skill content loading
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/components/chat/hooks/useSlashCommands.ts (2)
104-106: Remove stray formatting artifacts.Lines 104-106 have unusual formatting with a trailing bracket, empty line, and standalone semicolon. This appears to be a merge artifact or accidental edit.
🧹 Proposed cleanup
...((data.custom || []) as SlashCommand[]).map((command) => ({ ...command, type: 'custom', })), - ] - -; + ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useSlashCommands.ts` around lines 104 - 106, The snippet in useSlashCommands (file src/components/chat/hooks/useSlashCommands.ts) contains stray formatting artifacts: an extraneous closing bracket, an empty line, and a standalone semicolon likely left from a bad merge; remove those stray characters so the function or array returns/assignments (e.g., whatever variable or array defined around the end of useSlashCommands) have proper syntax and formatting, ensuring the surrounding code (function closing brace, export statements or returned value from useSlashCommands) remains intact and the file compiles.
192-255: Consider extracting shared command insertion logic.
selectCommandFromKeyboard(lines 197-213) andhandleCommandSelect(lines 231-252) contain nearly identical logic for computing the new input string and positioning the cursor. This duplication could be consolidated into a helper function.♻️ Example helper extraction
const insertCommandAtSlashPosition = useCallback( (command: SlashCommand) => { if (slashPosition === -1) return; const textBeforeSlash = input.slice(0, slashPosition); const textAfterSlash = input.slice(slashPosition); const spaceIndex = textAfterSlash.indexOf(' '); const textAfterQuery = spaceIndex !== -1 ? textAfterSlash.slice(spaceIndex) : ''; const newInput = `${textBeforeSlash}${command.name} ${textAfterQuery}`; setInput(newInput); resetCommandMenuState(); setTimeout(() => { if (textareaRef.current) { const cursorPos = textBeforeSlash.length + command.name.length + 1; textareaRef.current.setSelectionRange(cursorPos, cursorPos); textareaRef.current.focus(); } }, 0); }, [input, slashPosition, setInput, resetCommandMenuState, textareaRef], );Then both
selectCommandFromKeyboardandhandleCommandSelectcan call this helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useSlashCommands.ts` around lines 192 - 255, Extract the duplicated input-plus-cursor logic from selectCommandFromKeyboard and handleCommandSelect into a single helper (e.g., insertCommandAtSlashPosition) that accepts a SlashCommand, checks slashPosition !== -1, computes textBeforeSlash/textAfterSlash/spaceIndex/textAfterQuery, builds newInput, calls setInput and resetCommandMenuState, and moves the caret using textareaRef.setSelectionRange inside setTimeout; then replace the duplicated blocks in selectCommandFromKeyboard and handleCommandSelect to call insertCommandAtSlashPosition(command) (keep trackCommandUsage and hover handling in handleCommandSelect as-is). Ensure the helper is memoized with useCallback and includes the dependencies input, slashPosition, setInput, resetCommandMenuState, and textareaRef.src/components/chat/hooks/useChatComposerState.ts (1)
856-856: RemoveexecuteCommandfrom the dependency array.The
executeCommandcallback is included in the dependency array at line 856 but is not called withinhandleSubmit. The function uses directauthenticatedFetchcalls to/api/commands/executeinstead. Removing this unused dependency will clean up the code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatComposerState.ts` at line 856, The dependency array for the effect/handler incorrectly includes executeCommand even though handleSubmit does not call it; remove executeCommand from the dependency array to avoid unnecessary re-renders and confusion. Edit the useChatComposerState hook (remove executeCommand from the dependency list where handleSubmit is defined/registered) and ensure the dependency array only contains variables actually referenced by handleSubmit (e.g., authenticatedFetch, form state, and any setters), leaving executeCommand referenced elsewhere intact. Run tests/lint to confirm no unused variable warnings remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Line 856: The dependency array for the effect/handler incorrectly includes
executeCommand even though handleSubmit does not call it; remove executeCommand
from the dependency array to avoid unnecessary re-renders and confusion. Edit
the useChatComposerState hook (remove executeCommand from the dependency list
where handleSubmit is defined/registered) and ensure the dependency array only
contains variables actually referenced by handleSubmit (e.g.,
authenticatedFetch, form state, and any setters), leaving executeCommand
referenced elsewhere intact. Run tests/lint to confirm no unused variable
warnings remain.
In `@src/components/chat/hooks/useSlashCommands.ts`:
- Around line 104-106: The snippet in useSlashCommands (file
src/components/chat/hooks/useSlashCommands.ts) contains stray formatting
artifacts: an extraneous closing bracket, an empty line, and a standalone
semicolon likely left from a bad merge; remove those stray characters so the
function or array returns/assignments (e.g., whatever variable or array defined
around the end of useSlashCommands) have proper syntax and formatting, ensuring
the surrounding code (function closing brace, export statements or returned
value from useSlashCommands) remains intact and the file compiles.
- Around line 192-255: Extract the duplicated input-plus-cursor logic from
selectCommandFromKeyboard and handleCommandSelect into a single helper (e.g.,
insertCommandAtSlashPosition) that accepts a SlashCommand, checks slashPosition
!== -1, computes textBeforeSlash/textAfterSlash/spaceIndex/textAfterQuery,
builds newInput, calls setInput and resetCommandMenuState, and moves the caret
using textareaRef.setSelectionRange inside setTimeout; then replace the
duplicated blocks in selectCommandFromKeyboard and handleCommandSelect to call
insertCommandAtSlashPosition(command) (keep trackCommandUsage and hover handling
in handleCommandSelect as-is). Ensure the helper is memoized with useCallback
and includes the dependencies input, slashPosition, setInput,
resetCommandMenuState, and textareaRef.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/routes/commands.jssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useSlashCommands.ts
… mode
- Use lastIndexOf('@') for plugin key parsing to correctly handle scoped
packages like @scope/pkg@1.2.3 (split('@')[0] returns empty for these)
- Dispatch builtin commands via handleBuiltInCommand in multi-command
execution loop, preventing them from being silently skipped
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/commands.js`:
- Around line 582-594: The ACL currently allows any file under
userPluginsBase/userSkillsBase/userCommandsBase/projectBase; restrict reads to
markdown skill docs by updating the isUnder check and the route guards: ensure
resolvedPath is inside the allowed base AND its extension is .md or .markdown
(e.g., path.extname(resolvedPath) === '.md' || '.markdown'), and for plugin
paths additionally require the file be in a skills subfolder or match a
skill-doc filename pattern (e.g., contains path.sep + 'skills' + path.sep or
filename endsWith 'skill.md'); apply the same tightened checks where similar ACL
logic appears (the other block around lines 673-688) and use the existing
symbols userCommandsBase, userSkillsBase, userPluginsBase, projectBase, isUnder
and resolvedPath to locate and change the code.
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 564-573: The API call to '/api/commands/execute' is sending args:
[] which drops user-provided arguments; update the POST body in the
authenticatedFetch calls (the one building { commandName: command.name,
commandPath: command.path, args: [], context: {...} } and the similar block at
the other occurrence) to forward the actual argument array (e.g., use the
existing args variable or command.args from the surrounding scope) instead of an
empty array so $ARGUMENTS/$1... substitutions receive the user text.
- Around line 815-819: The pendingSkillContentRef.current is only cleared in the
Claude branch causing stale skill content to survive provider switches; update
the submit flow in useChatComposerState (the submit/handleSubmit logic where
skillContent is read) so that pendingSkillContentRef.current is cleared exactly
once per submit cycle regardless of provider branch—read skillContent =
pendingSkillContentRef.current, then immediately set
pendingSkillContentRef.current = null (or ensure nulling occurs in a
finally/common path) before any provider-specific attachment logic so stale
content cannot leak across providers.
- Around line 536-537: The current slashCommandPattern and its use in
foundCommands exclude "@" and internal "/" characters so scoped names like
"@scope/skill" and nested commands like "/foo/bar" are missed; update the regex
used by slashCommandPattern to allow "@" and additional "/" inside the token
(e.g. allow characters like @ and / in the group after the leading slash) so
that functions that iterate foundCommands will capture scoped and nested command
names correctly, and verify the pattern still anchors with (^|\s) and (?=\s|$)
to avoid partial matches.
Comment on lines
+582
to
594
| const userCommandsBase = path.resolve(path.join(os.homedir(), '.claude', 'commands')); | ||
| const userSkillsBase = path.resolve(path.join(os.homedir(), '.claude', 'skills')); | ||
| const userPluginsBase = path.resolve(path.join(os.homedir(), '.claude', 'plugins')); | ||
| const projectBase = projectPath | ||
| ? path.resolve(path.join(projectPath, '.claude', 'commands')) | ||
| : null; | ||
| const isUnder = (base) => { | ||
| const rel = path.relative(base, resolvedPath); | ||
| return rel !== '' && !rel.startsWith('..') && !path.isAbsolute(rel); | ||
| }; | ||
|
|
||
| if (!(isUnder(userCommandsBase) || isUnder(userSkillsBase) || isUnder(userPluginsBase) || (projectBase && isUnder(projectBase)))) { | ||
| return res.status(403).json({ |
Contributor
There was a problem hiding this comment.
Restrict plugin-path reads to skill markdown files only.
Current ACL permits any file under ~/.claude/plugins (and similarly broad under skills). That expands /load and /execute into general file-read endpoints within plugin trees, not just command/skill docs.
🔒 Tighten file-type + location checks
- if (!(isUnder(userCommandsBase) || isUnder(userSkillsBase) || isUnder(userPluginsBase) || (projectBase && isUnder(projectBase)))) {
+ const isMarkdownFile = resolvedPath.toLowerCase().endsWith('.md');
+ const isSkillFile = path.basename(resolvedPath).toLowerCase() === 'skill.md';
+ const isPluginSkillFile = /[\\/]skills[\\/][^\\/]+[\\/]SKILL\.md$/i.test(resolvedPath);
+
+ const allowedCommandFile =
+ isMarkdownFile && (isUnder(userCommandsBase) || (projectBase && isUnder(projectBase)));
+ const allowedUserSkillFile = isSkillFile && isUnder(userSkillsBase);
+ const allowedPluginSkillFile = isPluginSkillFile && isUnder(userPluginsBase);
+
+ if (!(allowedCommandFile || allowedUserSkillFile || allowedPluginSkillFile)) {
return res.status(403).json({
error: 'Access denied',
message: 'Command must be in .claude/commands, .claude/skills, or .claude/plugins directory'
});
}Also applies to: 673-688
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes/commands.js` around lines 582 - 594, The ACL currently allows
any file under userPluginsBase/userSkillsBase/userCommandsBase/projectBase;
restrict reads to markdown skill docs by updating the isUnder check and the
route guards: ensure resolvedPath is inside the allowed base AND its extension
is .md or .markdown (e.g., path.extname(resolvedPath) === '.md' || '.markdown'),
and for plugin paths additionally require the file be in a skills subfolder or
match a skill-doc filename pattern (e.g., contains path.sep + 'skills' +
path.sep or filename endsWith 'skill.md'); apply the same tightened checks where
similar ACL logic appears (the other block around lines 673-688) and use the
existing symbols userCommandsBase, userSkillsBase, userPluginsBase, projectBase,
isUnder and resolvedPath to locate and change the code.
Comment on lines
+536
to
+537
| const slashCommandPattern = /(^|\s)(\/[a-zA-Z0-9_:-]+)(?=\s|$)/g; | ||
| const foundCommands: { command: SlashCommand; match: string }[] = []; |
Contributor
There was a problem hiding this comment.
Multi-command regex misses valid command names (@scope/... and nested /.../...).
The pattern excludes @ and internal /, so scoped plugin skills and nested command names won’t be detected/executed.
💡 Broaden token matching for slash commands
- const slashCommandPattern = /(^|\s)(\/[a-zA-Z0-9_:-]+)(?=\s|$)/g;
+ const slashCommandPattern = /(^|\s)(\/[^\s]+)(?=\s|$)/g;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatComposerState.ts` around lines 536 - 537,
The current slashCommandPattern and its use in foundCommands exclude "@" and
internal "/" characters so scoped names like "@scope/skill" and nested commands
like "/foo/bar" are missed; update the regex used by slashCommandPattern to
allow "@" and additional "/" inside the token (e.g. allow characters like @ and
/ in the group after the leading slash) so that functions that iterate
foundCommands will capture scoped and nested command names correctly, and verify
the pattern still anchors with (^|\s) and (?=\s|$) to avoid partial matches.
Comment on lines
+564
to
+573
| const response = await authenticatedFetch('/api/commands/execute', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| commandName: command.name, | ||
| commandPath: command.path, | ||
| args: [], | ||
| context: { |
Contributor
There was a problem hiding this comment.
Custom-command arguments are dropped in the multi-command path.
/api/commands/execute is called with args: [], so $ARGUMENTS/$1... substitution never receives user text for non-skill commands.
💡 Preserve args for non-skill commands
+ const isSkillNamespace =
+ command.namespace === 'user-skill' ||
+ String(command.namespace || '').startsWith('plugin:');
+ const commandArgs =
+ !isSkillNamespace && remainingText ? remainingText.split(/\s+/) : [];
+
const response = await authenticatedFetch('/api/commands/execute', {
method: 'POST',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify({
commandName: command.name,
commandPath: command.path,
- args: [],
+ args: commandArgs,
context: {Also applies to: 608-609
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatComposerState.ts` around lines 564 - 573,
The API call to '/api/commands/execute' is sending args: [] which drops
user-provided arguments; update the POST body in the authenticatedFetch calls
(the one building { commandName: command.name, commandPath: command.path, args:
[], context: {...} } and the similar block at the other occurrence) to forward
the actual argument array (e.g., use the existing args variable or command.args
from the surrounding scope) instead of an empty array so $ARGUMENTS/$1...
substitutions receive the user text.
Comment on lines
+815
to
+819
| // Check if there's a pending skill to attach | ||
| const skillContent = pendingSkillContentRef.current; | ||
| if (skillContent) { | ||
| pendingSkillContentRef.current = null; // Clear after using | ||
| } |
Contributor
There was a problem hiding this comment.
pendingSkillContentRef can leak across provider switches.
It is cleared only in the Claude branch. If a skill is loaded while using another provider, stale skill content can be attached later when switching back to Claude.
💡 Clear pending skill content once per submit cycle
- if (provider === 'cursor') {
+ const skillContent = pendingSkillContentRef.current;
+ pendingSkillContentRef.current = null;
+
+ if (provider === 'cursor') {
sendMessage({
type: 'cursor-command',
command: messageContent,
@@
- } else {
- // Check if there's a pending skill to attach
- const skillContent = pendingSkillContentRef.current;
- if (skillContent) {
- pendingSkillContentRef.current = null; // Clear after using
- }
-
+ } else {
sendMessage({
type: 'claude-command',
command: messageContent,Also applies to: 821-835
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatComposerState.ts` around lines 815 - 819,
The pendingSkillContentRef.current is only cleared in the Claude branch causing
stale skill content to survive provider switches; update the submit flow in
useChatComposerState (the submit/handleSubmit logic where skillContent is read)
so that pendingSkillContentRef.current is cleared exactly once per submit cycle
regardless of provider branch—read skillContent =
pendingSkillContentRef.current, then immediately set
pendingSkillContentRef.current = null (or ensure nulling occurs in a
finally/common path) before any provider-specific attachment logic so stale
content cannot leak across providers.
Collaborator
|
@shikihane Thanks for this PR. This is great feature and was requested by a lot of people. However, I'm facing the following issue: If I attach the skill and click send, it is also sending the complete skill file content as input in the UI. This shouldn't be the case since the skill content can be extremely long. Rather, the user should see a response that the skill was loaded and they can preview the contents of the skill if they want to. For reference, look to something like PR 456 which does a similar thing. |
Collaborator
|
The coderabbit comments also look valid. Can you check them out when you have the time? |
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
2 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.
Summary
This PR adds comprehensive skill support and multi-command parsing to the slash command system:
~/.claude/skills/and~/.claude/plugins/for SKILL.md files/skill1 /skill2 do something)systemPrompt.appendChanges
Backend (
server/)routes/commands.js: AddedscanUserSkills()andscanPluginSkills()functions, expanded security checks, addedisSkillflagclaude-sdk.js: Added skill content injection viaoptions.skillContentFrontend (
src/)hooks/useSlashCommands.ts: Changed to autocomplete behavior (insert command name, don't execute)hooks/useChatComposerState.ts: Added multi-command regex parsing, skill loading flow,pendingSkillContentRefMessageComponent.tsx: Added skill-loaded collapsible card renderingTest Plan
~/.claude/skills/and~/.claude/plugins//skill1 /skill2 prompt text)Related
Part of upstream contribution plan - see #461, #462, #463 for related PRs.
Summary by CodeRabbit
New Features
Documentation
Localization