Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/server/src/skills/SkillsManager.ts`:
- Around line 167-199: writeSkillConfigEntry currently reads and then
unconditionally writes configPath which can race with concurrent toggles; to
fix, serialize updates around configPath: acquire a per-config lock
(process-level mutex or file-based lock like creating/locking a .lock file) at
the start of the update, then re-read the file after acquiring the lock, apply
the existing regex cleanup and enabled/disabled append logic (use the same
cleaned variable approach), write changes to a temp file and atomically rename
it to configPath (or use fs.rename) to avoid partial writes, and finally release
the lock; reference the function writeSkillConfigEntry, the configPath variable,
the entryRe regex, and the writeFile calls that should be replaced by this
read-after-lock, temp-write, atomic-rename pattern.
- Around line 309-311: Add an abortable timeout to the server-side fetch to
prevent indefinite hangs: create an AbortController, pass controller.signal into
the existing fetch(url) call (the symbols involved are url, res, fetch), and set
a short timeout (e.g. 3–10s) that calls controller.abort(); ensure you clear the
timeout after the response arrives and convert aborts into a proper error path
(catch the aborted fetch and throw a descriptive Error or handle it the same way
you handle non-ok responses) so the UI spinner won't remain stuck.
- Around line 196-198: The TOML entry is written using skillMdPath directly,
which can produce invalid TOML on Windows due to unescaped backslashes; before
building entry in SkillsManager.ts (around the variables skillMdPath, entry,
cleaned, configPath) escape backslashes and any double quotes in skillMdPath
(e.g., replace "\" with "\\" and `"` with `\"`) or otherwise serialize it as a
TOML-safe basic string, then use that escaped value when constructing entry so
the written TOML remains valid cross-platform.
- Around line 382-383: The regex that strips YAML frontmatter in
SkillsManager.ts uses raw.replace(...) and only matches LF line endings, so
SKILL.md files with CRLF won't strip; update the frontmatter-stripping regex
used to produce content (the raw.replace call) to accept optional CR characters
by using \r?\n (or equivalent) for all newline matches (both after the opening
--- and before/after the closing ---) so CRLF and LF frontmatter are removed
correctly.

In `@apps/web/src/composer-editor-mentions.ts`:
- Line 15: The CHIP_TOKEN_REGEX currently uses a lookahead `(?=\s)` which
prevents matching chips at end-of-input; update the regex used in
composer-editor-mentions (CHIP_TOKEN_REGEX) so it also matches when the token is
at string end (e.g., change the lookahead to `(?=\s|$)` or equivalent) so that
splitPromptIntoComposerSegments() correctly recognizes trailing `@...` and
`$...` chips and they don't revert to plain text on re-render.

In `@apps/web/src/lib/skillsReactQuery.ts`:
- Around line 14-17: The current query/mutation options call ensureNativeApi()
(e.g., in the queryFn that calls api.skills.list()) which throws when the
desktop bridge is absent; wrap each use of ensureNativeApi()/api.skills.* with a
guard that detects whether the native bridge is available and return a safe
fallback instead of letting ensureNativeApi() throw. Concretely, before calling
ensureNativeApi() in the list/create/update/get/delete helpers (the occurrences
around the shown queryFn and the other ranges), check a bridge-available
predicate (or try/catch around ensureNativeApi()) and return an appropriate safe
response (e.g., Promise.resolve([]) for list queries, a resolved null/undefined
or a controlled error object for gets/mutations) so web surfaces gracefully
hide/disable skills rather than erroring.

In `@apps/web/src/routes/_chat.skills.tsx`:
- Around line 521-524: The loading check currently only compares
installMutation.variables?.source to result.source, causing cards with the same
source to all show as installing; update the isInstalling condition used in the
component (where installMutation is referenced and the row key uses
"source:name") to scope to the exact result by matching both source and name
(e.g., compare installMutation.variables?.source === result.source &&
installMutation.variables?.name === result.name or build and compare the
combined `${source}:${name}` key) so only the specific row with that source:name
shows the pending state.

---

Outside diff comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 1027-1034: The empty-state text in ChatView.tsx currently falls
back to workspace/command copy; update the conditional that renders when
props.items.length === 0 to handle props.triggerKind === "skill" explicitly:
when props.triggerKind === "skill" show the skills-specific loading and empty
copy (separate messages for props.isLoading true and false), otherwise keep the
existing "path" and default command messages; modify the ternary/conditional
around props.isLoading and props.triggerKind in the block that references
props.items, props.isLoading, and props.triggerKind to include the new "skill"
branch so the /skills picker shows appropriate copy.

---

Nitpick comments:
In `@apps/server/src/skills/SkillsManager.ts`:
- Around line 203-217: resolveSkillAgents currently calls
readSkillsConfigEntries for every platform on each skill, causing N×M reparses;
instead pre-read each platform config once and reuse it. Modify the caller
(listSkills) to build a Map from platform.configPath (or platform.name) to the
entries returned by readSkillsConfigEntries for each SKILLS_PLATFORMS entry,
then change resolveSkillAgents to accept that preloaded Map (or use a
module-level cache keyed by configPath) and consult entries.get(skillMdPath)
instead of calling readSkillsConfigEntries repeatedly; keep function names
resolveSkillAgents, listSkills, readSkillsConfigEntries and constant
SKILLS_PLATFORMS to locate changes.

In `@apps/web/src/components/Sidebar.tsx`:
- Around line 1103-1121: The className logic on the SidebarMenuButton duplicates
active-state handling; remove the conditional expression `${pathname ===
"/skills" ? "" : "text-muted-foreground"}` and instead always include
"text-muted-foreground" in the base class string so the
data-[active=true]:text-foreground rule (driven by isActive prop on
SidebarMenuButton) overrides it when pathname === "/skills"; update the
SidebarMenuButton instance (render prop on SidebarMenuButton, isActive prop, and
its className) to reflect this simplified base style.

In `@packages/contracts/src/skills.ts`:
- Around line 39-44: The shared schema currently defines
SkillSearchResultEntry.installs as a String which forces preformatted text;
change the schema so installs is a Number (update Schema.Struct to use
Schema.Number for installs) and then update the producer code in
apps/server/src/skills/SkillsManager.ts to return the raw numeric s.installs
value (instead of a formatted "123 installs" string) so UIs can format, sort,
and localize counts themselves.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/server/src/skills/SkillsManager.ts`:
- Around line 371-388: installSkill (and similarly uninstallSkill) must validate
input.skillName before calling runCommand/npx to avoid CLI flag injection; call
the existing assertValidSkillSlug(input.skillName) at the start of installSkill
and in uninstallSkill, and if it throws return an Effect that resolves to a
failed SkillsInstallResult (success: false, message: error.message) instead of
invoking runCommand; ensure you place the validation before building
agentFlags/runCommand and mirror the same validation+error-to-result behavior
used in the function’s catch handler.
- Around line 177-182: The path regex currently stops at the first raw quote and
fails for escaped quotes; update the path parsing in SkillsManager.ts (the
pathMatch capture) to accept escaped characters inside the TOML basic string
(e.g. use a pattern that captures ((?:\\.|[^"\\])*) instead of (.+?)), then
continue to unescape sequences (backslashes and \" ) as you already do;
specifically modify the pathMatch assignment and keep the unescapedPath logic so
skills with escaped quotes round-trip correctly when using
escapeTomlBasicString().

In `@apps/web/src/routes/_chat.skills.tsx`:
- Around line 305-320: The effect is using raw search which allows trailing
spaces to bypass local matches; normalize the input by trimming before any
checks or setting debouncedSearch. Inside the useEffect that references search,
compute a trimmed value (e.g., const normalized = search.trim()), use
normalized.length for the length check, call setDebouncedSearch(normalized) in
the timeout, and clear timeouts via debounceRef as before; also ensure any
installed-skill filtering logic that reads debouncedSearch (or directly uses
search elsewhere, e.g., the installed-skill filter around the other block) uses
the trimmed value so comparisons use normalized strings.
- Around line 324-356: The three mutation handlers (handleToggle,
handleUninstall, handleInstall) lack onError callbacks so network/API failures
never surface to users; update each to pass an onError option to
toggleMutation.mutate, uninstallMutation.mutate, and installMutation.mutate
respectively that calls toastManager.add with an error toast (include a brief
message and the error.message) and, for uninstallMutation.onError and
installMutation.onError, ensure any UI state adjustments mirror success/error
paths (e.g., do not clear selectedSkillName unless success). Reference the
functions handleToggle, handleUninstall, handleInstall and the mutation objects
toggleMutation, uninstallMutation, installMutation and use toastManager.add to
display the error feedback.

---

Nitpick comments:
In `@apps/web/src/routes/_chat.skills.tsx`:
- Around line 98-114: Extract the duplicated agent icon/fallback loop into a
small shared component (e.g., AgentBadges or AgentBadgeList) that accepts
agents: string[]; inside it reuse PLATFORM_ICONS to render PlatformIcon when
present and otherwise render the same fallback <span> with the exact className,
key={agent} and aria-label behavior; then replace the inline map in SkillCard
and SkillDetailView with <AgentBadges agents={skill.agents} /> and add
appropriate prop typing (agents: string[]) and export so both usages (the block
shown and the one around lines 216-232) use the shared component.

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@apps/server/src/skills/SkillsManager.ts`:
- Around line 280-287: The code in SkillsManager.ts currently uses synchronous
fs.readFileSync to read SKILL.md into the local variable content (when
skillMdPath exists), which blocks the event loop; replace that sync call with
the async API used elsewhere (e.g., await fsp.readFile(skillMdPath, "utf-8"))
inside the same try/catch so content gets assigned the awaited result, and keep
the catch to handle read failures; ensure fsp (fs.promises) is imported/used
consistently with the earlier fsp.readdir usage.
- Around line 206-211: The code builds a RegExp from variable input
(escapedPath) in SkillsManager (see escapeTomlBasicString, tomlSafeSkillMdPath,
escapedPath, entryRe), which static analysis flagged for ReDoS; to fix, stop
constructing a regex from untrusted/variable content: instead build a literal
TOML snippet using tomlSafeSkillMdPath and perform a safe string search
(String.prototype.includes or indexOf) to locate `[[skills.config]]\npath =
"..."` entries, or if you must use a regex, enforce input validation via
assertValidSkillSlug and a strict max-length check before escaping with a
dedicated escapeRegExp utility so the RegExp constructor only receives safe,
size-limited content.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 1814-1815: The skills query wired in ChatView (isSkillTrigger ->
skillsQuery using skillsListQueryOptions) calls ensureNativeApi (from
skillsReactQuery.ts) which breaks in the browser; fix by using a web-safe query
or guarding the query: either replace usage of skillsListQueryOptions with a
web-transport-aware variant provided by the apps/web layer, or change the
enabled flag to isSkillTrigger && ensureNativeApi() (or an apps/web-provided
isNativeAvailable flag) so the query never runs in non-native builds; reference
symbols: isSkillTrigger, skillsQuery, skillsListQueryOptions, and
ensureNativeApi to locate the code to update.

---

Outside diff comments:
In `@apps/web/src/components/ComposerPromptEditor.tsx`:
- Around line 896-909: initialConfig's nodes array only registers
ComposerMentionNode but not ComposerSkillNode, so Lexical won't
serialize/deserialize or handle copy/paste for skill chips; update the useMemo
that builds initialConfig (the InitialConfigType returned by initialConfig in
the useMemo) to include ComposerSkillNode alongside ComposerMentionNode in the
nodes array so both custom node types (ComposerMentionNode and
ComposerSkillNode) are registered with the editor config and available for
serialization/deserialization and runtime use.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/server/src/skills/SkillsManager.ts`:
- Around line 351-358: The Effect.catch usage in the uninstall pipeline (same
pattern as in installSkill) is likely invalid; update the error handler to use
the correct Effect error combinator (use Effect.catchAll instead of
Effect.catch) in the pipeline where the anonymous error handler is registered,
e.g., the uninstall pipeline that returns a SkillsUninstallResult, and keep the
handler signature accepting the error (typed as SkillsError) and returning
Effect.succeed({...} satisfies SkillsUninstallResult); mirror the same fix you
applied for installSkill so the uninstall error path compiles and behaves
consistently.

---

Duplicate comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 1814-1815: The UI currently shows the `/skills` command even when
skillsQuery (created via skillsListQueryOptions() in the skillsQuery variable)
can never run outside Electron; update the code that determines visibility and
data fetching so `/skills` only appears when skills are actually available:
either change the enabled flag on skillsQuery to "enabled: isSkillTrigger &&
isElectron" (preserving the existing isElectron guard) and hide/disable the
command when isElectron is false, or implement a web-safe transport for
skillsQuery so it can run in apps/web; specifically adjust the logic around
composerTriggerKind/isSkillTrigger and skillsQuery (and any UI that lists the
`/skills` command) so the command is not surfaced in the browser unless
skillsQuery can populate.

---

Nitpick comments:
In `@apps/server/src/skills/SkillsManager.ts`:
- Around line 200-206: Replace the blocking fs.readFileSync call in the
SkillsManager flow with the async promise-based read to avoid blocking the event
loop: where skillMdPath is checked and content is assigned, use
fs.promises.readFile (or imported fsp.readFile) with await inside the existing
try/catch so content = await fs.promises.readFile(skillMdPath, "utf-8") (or
equivalent), preserving the error handling path; ensure the surrounding function
is async or properly awaits this operation so the rest of SkillsManager
(references to content and SKILL.md processing) continues to work correctly.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/server/src/skills/SkillsManager.ts`:
- Around line 302-305: The code passes raw input.source into runCommand invoking
"npx" "skills add" which lets values like "--help" be treated as flags; validate
and sanitize input.source before calling runCommand: in SkillsManager (where
input.source and runCommand are used), implement a whitelist/regex check for
allowed source patterns (owner/repo, HTTP/HTTPS/git URLs, absolute/local file
paths) and reject or normalize any value that looks like an option (starts with
"-" ) or fails the patterns; if supporting local paths, normalize them to safe
forms (e.g., resolve to absolute path) or explicitly prefix with "./" when
appropriate, and return/throw a clear validation error instead of invoking
runCommand with an unsafe source.

---

Duplicate comments:
In `@apps/server/src/skills/SkillsManager.ts`:
- Around line 311-316: The recovery currently uses a non-existent Effect.catch
combinator; replace it with the correct Effect recovery combinator for this
codepath—use Effect.catchAll to handle any error and map it to Effect.succeed({
success: false, message: error.message } as SkillsInstallResult), or if you need
to narrow by error type use Effect.catchTag("SkillsError", ...). Update both
occurrences around the SkillsManager handlers (the Effect pipeline that returns
SkillsInstallResult at the shown sites) so the TypeScript types align and bun
typecheck/lint pass.

---

Nitpick comments:
In `@apps/web/src/components/ComposerPromptEditor.tsx`:
- Around line 578-642: The plugin names still use "Mention" but the logic also
handles "skill" chips; rename ComposerMentionArrowPlugin,
ComposerMentionSelectionNormalizePlugin, and ComposerMentionBackspacePlugin to
ComposerChipArrowPlugin, ComposerChipSelectionNormalizePlugin, and
ComposerChipBackspacePlugin respectively (update their function declarations and
any imports/exports/usages) so names accurately reflect handling of both chip
types and ensure all references in the file (and other modules) are updated to
the new identifiers.
- Around line 685-694: The local function removeMentionNode should be replaced
with a consistent helper that uses isComposerChipNode instead of instanceof
checks and be renamed to removeChipNode; keep the existing logic of computing
mentionStart via getAbsoluteOffsetForPoint(candidate, 0), calling
candidate.remove(), restoring selection with
$setSelectionAtComposerOffset(mentionStart) and event?.preventDefault(), and
return true/false as before. Update all callers that referenced
removeMentionNode to call removeChipNode so the chip-typing logic
(ComposerMentionNode/ComposerSkillNode and any future chip types) is handled via
isComposerChipNode uniformly.