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

Inline comments:
In `@apps/controller/openapi.json`:
- Around line 5436-5448: Update the JSON schema for the response properties
"error" and "errorCode" to OpenAPI 3.1 null-preserving syntax: replace the
"nullable": true usage with a type array that includes "null" (i.e., set "type"
to ["string","null"] for "error" and set "type" to ["string","null"] plus
preserve the "enum" for "errorCode"); then regenerate the client types so
apps/web/lib/api/types.gen.ts reflects the nullable types.

In `@apps/controller/src/routes/skillhub-routes.ts`:
- Around line 195-197: The uninstall currently races with active executors
because container.skillhubService.queue.cancel(slug) only marks cancellation;
you must not call container.skillhubService.catalog.uninstallSkill(slug) until
running jobs have stopped. Change the flow to either (a) after
queue.cancel(slug) await an existing wait method (e.g., queue.waitForStop(slug)
/ queue.awaitIdle(slug) / queue.abortAndWait(slug)) to perform an abortable
shutdown, or (b) if no wait API exists, check a running flag (e.g.,
queue.isActive(slug) / queue.hasRunningJobs(slug)) and reject the uninstall with
a 409/appropriate error until the slug is inactive; ensure you reference and use
container.skillhubService.queue.cancel, the chosen wait/check method, and
container.skillhubService.catalog.uninstallSkill so uninstall only runs after
active jobs have fully stopped.
- Around line 152-154: Move the slug pattern from the ad-hoc SLUG_REGEX check
into the shared Zod schema so all routes use the same validation: update
skillhubSlugSchema (the shared schema in packages/shared/src/schemas/) to
include .regex(SLUG_REGEX or the equivalent pattern) instead of only min(1),
remove the inline SLUG_REGEX.test(slug) guard in the route handler
(apps/controller/src/routes/skillhub-routes.ts) and rely on the validated
path/body param schema for POST /install, DELETE /uninstall and GET
/skills/{slug} so OpenAPI/SDK and all routes enforce the same slug rules.

In `@apps/controller/src/services/skillhub-service.ts`:
- Around line 54-59: InstallQueue's callbacks must not violate its error
contract: make onComplete non-throwing by catching any errors from
skillDb.recordInstall(slug, source) (log the failure but do not rethrow), and
change onCancelled to await catalogManager.uninstallSkill(slug) and inspect its
return value; if the result is { ok: false } then throw an error (or return a
rejected promise) so the queue sees the failure. Reference: onComplete,
skillDb.recordInstall, onCancelled, catalogManager.uninstallSkill, and
InstallQueue.

In `@apps/controller/src/services/skillhub/catalog-manager.ts`:
- Around line 282-309: The code canonicalizes the slug inside executeInstall
(slug = SLUG_CORRECTIONS[rawSlug] ?? rawSlug) but the InstallQueue bookkeeping
still uses the original raw slug, causing ledger drift; fix by normalizing
before queue operations or returning/plumbing the canonical slug into the
completion path: either canonicalize rawSlug earlier (where items are enqueued)
so enqueue uses the normalized value, or modify executeInstall/onComplete to
pass the resolved slug back to InstallQueue.complete (or whichever method
records completion) so the ledger records the canonical slug (reference:
executeInstall, SLUG_CORRECTIONS, InstallQueue, onComplete).
- Around line 316-319: The current getCuratedSlugsToEnqueue() uses only
this.db.getAllInstalled() so migrated-but-uninstalled ledger entries are treated
as never-seen; change the logic to exclude any CURATED_SKILL_SLUGS that have any
ledger record instead of only installed rows. Update getCuratedSlugsToEnqueue()
to call the ledger-history API on the DB (e.g. this.db.getLedgerHistory() /
this.db.getLedgerEntriesForSlug(slug) or a new this.db.hasLedgerRecord(slug))
and filter out slugs with any ledger record before returning the list from
CURATED_SKILL_SLUGS. Ensure you reference getCuratedSlugsToEnqueue,
this.db.getAllInstalled, and CURATED_SKILL_SLUGS when making the change.

In `@apps/controller/src/services/skillhub/skill-dir-watcher.ts`:
- Around line 38-43: The syncNow() implementation treats errors from
scanDiskSlugs() as an empty scan which can incorrectly mark installed skills as
uninstalled or stop reconciliation; wrap the call to this.scanDiskSlugs() in a
try/catch inside syncNow(), log or capture the error, and return early on
failure (do not proceed to diff/marking logic). Ensure the catch targets
failures when reading this.skillsDir so syncNow() aborts safely if the directory
disappears or becomes unreadable.

In `@apps/web/src/pages/skills.tsx`:
- Around line 60-64: The current isBusy calculation treats queue activity as a
local pending action and disables the cancel/uninstall UI; update the logic so
queued/downloading/installing-deps do not block the cancel path: either remove
isQueueActive from isBusy (only use pendingAction !== null) or keep
isQueueActive but explicitly enable the uninstall/cancel controls when
queueStatus is queued/downloading/installing-deps so handleToggle(false) /
queue.cancel(slug) remains reachable; apply the same change in the companion
component CommunitySkillCard (look for isQueueActive, isBusy, handleToggle and
queue.cancel references) so queued installs can always be cancelled.

In `@tests/desktop/skill-db.test.ts`:
- Around line 164-165: The guard using the deprecated isRemovedByUser
(referenced in curated-skills.ts around the logic in copyStaticSkills and
resolveCuratedSkillsToInstall) is ineffective and should be removed or replaced:
either delete the conditional checks that rely on isRemovedByUser so the
existing disk-existence fallbacks handle copies/enqueues, or implement a proper
persisted removal-tracking mechanism (e.g., a removal registry or config) and
consult that registry in copyStaticSkills and resolveCuratedSkillsToInstall
instead of isRemovedByUser to honor user-deletions across restarts.

---

Outside diff comments:
In `@apps/controller/src/services/skillhub/curated-skills.ts`:
- Around line 74-77: The guard using skillDb.isRemovedByUser(slug) is
ineffective because isRemovedByUser currently always returns false; instead,
update getCuratedSlugsToEnqueue (and the curated-skills guards) to consult the
ledger and explicitly exclude slugs whose ledger entry status is "uninstalled"
(in addition to "installed" handling). Concretely, modify
getCuratedSlugsToEnqueue to fetch ledger entries (use getAll or the ledger
accessor used in skill-db), filter out any entries where entry.status ===
"uninstalled" before building the enqueue list, and/or implement isRemovedByUser
to return true when the ledger has an "uninstalled" record for the slug so
existing checks in curated-skills.ts and catalog-manager.ts work as intended.

---

Nitpick comments:
In `@apps/controller/tests/skillhub-service.test.ts`:
- Around line 210-227: Add a test in
apps/controller/tests/skillhub-service.test.ts that creates a preexisting ledger
file at env.skillDbPath inside the temp root (use the same mkdtemp/rootDir
setup) before calling start() on the SkillHub service so the
existsSync(env.skillDbPath) branch is exercised; ensure mocks like
mockSkillDbCreate and mockCopyStaticSkills reflect a restart (e.g.,
mockSkillDbCreate not called or skipped) and assert the initialization path was
skipped (check relevant calls/flags on SkillHubService.start, ledger
initialization code, and any "skip initialization" behavior). Also add the same
preexisting-ledger scenario to the other start() tests referenced in the 271-434
range so subsequent restarts are covered consistently.

In `@apps/web/src/hooks/use-community-catalog.ts`:
- Around line 30-35: The code is casting the SDK response away from its
generated types (removing type safety) in the queryFn of use-community-catalog
(the useQuery block using getApiV1SkillhubCatalog and SkillhubCatalogData);
remove the "as unknown as SkillhubCatalogData" and any manual "as { ... }" casts
and instead use the generated SDK types from apps/web/lib/api/types.gen.ts for
the API response, or explicitly map the SDK shape to your local DTO (e.g.,
convert SDK queue items that include retries and the SDK source enum into your
local QueueItem shape) inside the queryFn before returning; update the return
type of the queryFn (and any downstream consumers) to the proper SDK type or the
mapped local type so you no longer hide schema drift between
getApiV1SkillhubCatalog and your local types.

In `@docs/plans/2026-03-23-skillhub-install-queue.md`:
- Around line 167-169: Update the plan examples to stop using the removed
"curated" SkillSource: change calls like queue.enqueue("a", "curated") to use
the current valid source (e.g., "managed") so examples match the implementation;
search for occurrences of queue.enqueue(...) and any other examples referencing
"curated" in this document and replace them with the canonical SkillSource value
used in the codebase.
- Around line 1306-1311: Add a language identifier to the fenced code block that
shows state transitions so the markdown linter (MD040) is satisfied; change the
opening fence from ``` to ```text (or ```plaintext) for the block containing
"queued → downloading → done ..." and ensure the closing fence remains ``` so
the block is properly fenced.

In `@tests/desktop/skill-dir-watcher.test.ts`:
- Around line 60-190: Add tests that assert the watcher’s race guards: create a
SkillDirWatcher with a small debounceMs and a fake/real skillDb, then trigger a
sync and while that sync is running verify that the watcher reports the slug as
in-flight (isSlugInFlight for the skill) and that a concurrent sync attempt does
not record a second install; also add a test that starts the watcher, writes a
SKILL.md to schedule a debounced sync, then immediately calls stop() and assert
the pending sync never runs (the skill is not recorded). Reference
SkillDirWatcher, start(), stop(), syncNow(), debounceMs and isSlugInFlight when
locating where to add the assertions.

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

Inline comments:
In `@apps/web/src/pages/skills.tsx`:
- Around line 267-279: The current de-duplication Set shownErrorSlugs in the
useEffect keys only by item.slug so a repeat failure for the same slug in a
later attempt is suppressed; update the logic to deduplicate by attempt (e.g.,
combine item.slug + item.enqueuedAt or item.attemptId) or remove the slug from
shownErrorSlugs when the item leaves the "failed" state. Locate the useEffect
that iterates data?.queue and change the key used for shownErrorSlugs.current
(or add logic to clear entries when item.status !== "failed") so each new
enqueue attempt will surface its toast.

---

Duplicate comments:
In `@apps/controller/openapi.json`:
- Around line 6660-6671: The OpenAPI 3.1 schema is still using the 3.0
`nullable: true` form for the "error" and "errorCode" properties; replace that
with JSON Schema 2020-12 style nullability so generated clients preserve null:
for the simple nullable string property "error" change its schema to allow both
string and null (e.g., type as ["string","null"] or an anyOf/oneOf of string and
{"type":"null"}), and for the nullable enum "errorCode" either include null as
one of the enum values (e.g., enum: ["skill_not_found","rate_limit","unknown",
null]) or use a oneOf/anyOf combining the enum and {"type":"null"} so null is a
valid branch.

In `@apps/web/src/pages/skills.tsx`:
- Around line 63-67: The current logic sets isQueueActive (queueStatus ===
"queued" || "downloading" || "installing-deps") and then uses it to compute
isBusy, which hides/disables the uninstall/cancel control when a job is
queued/downloads/installing-deps; change the logic so
queued/download/install-deps remain cancellable: keep isQueueActive for UI
status but compute isBusy without treating those cancellable queue states as
busy (e.g., create isCancellableQueue = queueStatus === "queued" || queueStatus
=== "downloading" || queueStatus === "installing-deps" and set isBusy =
pendingAction !== null || (queueStatus not in cancellable states && other
long-running states)), ensuring the switch/uninstall control renders/enabled
when isCancellableQueue is true while preserving busy state for truly blocking
statuses.

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

Inline comments:
In `@apps/controller/src/services/skillhub/catalog-manager.ts`:
- Around line 754-759: refreshCatalog() currently persists meta.skillCount from
the raw skills list before the post-processing that applies CATALOG_BLOCKLIST
and SLUG_CORRECTIONS, causing meta.skillCount to disagree with the filtered
result returned by the reader; update refreshCatalog() to apply the same
filtering/canonicalization (use CATALOG_BLOCKLIST and SLUG_CORRECTIONS) to the
skills array before computing/persisting meta.skillCount, or else compute
meta.skillCount from the final filtered array you intend to cache/return so the
persisted meta.skillCount equals the length of the filtered skills.
- Around line 292-307: The code currently logs raw child-process output from
execFileAsync (stdout/stderr) when invoking clawHubBin which can leak sensitive
URLs or tokens; change the logging in the block that calls execFileAsync so that
instead of this.log("info", `install stdout ${slug}: ${stdout.trim()}`) and
this.log("warn", `install stderr ${slug}: ${stderr.trim()}`) you produce a
sanitized, structured summary: log success/failure for the slug, include a
truncated length or redacted indicator (e.g., "<redacted-output>" or first N
chars only), remove or mask any URLs or tokens, and preserve the existing log
levels via this.log; keep exec errors intact but ensure any attached output is
sanitized before logging.

In `@apps/controller/src/services/skillhub/install-queue.ts`:
- Around line 255-295: When the executor rejects, first check and handle
cancelled installs before any retry/failure logic: if item.cancelled is true,
clear the cancelled flag, call item.onCancelled(), set item.status to
"cancelled" (or appropriate state), remove it from this.active, push it to
this.completed, call this.scheduleCleanup(item) and this.drain(), then return —
do this at the top of the (err: unknown) => { ... } handler so rate-limit
branches (parseRateLimitPauseMs/classifyError/retries/pauseQueue) never run for
cancelled jobs.

---

Duplicate comments:
In `@apps/controller/src/routes/skillhub-routes.ts`:
- Around line 191-193: The current code calls
container.skillhubService.queue.cancel(slug) then immediately
container.skillhubService.catalog.uninstallSkill(slug), which can delete files
while executeInstall() / npm install is still running; change this to detect an
active install and either (A) wait for the queue item to become inactive/settled
(use the queue API to poll or await the item’s completion) before calling
catalog.uninstallSkill(slug), or (B) immediately throw new HTTPException(409,
"Skill is currently installing; cancel requested and uninstall deferred") so
onCancelled can finish cleanup; ensure you reference and use
container.skillhubService.queue.cancel, the queue item status check,
executeInstall/onCancelled semantics, and throw HTTPException with status and
contextual message if choosing the 409 path.

---

Nitpick comments:
In `@apps/controller/tests/skillhub-service.test.ts`:
- Around line 366-381: The test should assert that SkillhubService canonicalizes
slugs before enqueueing: update the mock catalog to return ["alpha","beta"] from
getCuratedSlugsToEnqueue and also mock catalog.canonicalizeSlug to rewrite those
values (e.g., "alpha" -> "ALPHA", "beta" -> "BETA"), then call
SkillhubService.create(...) and start() and assert that installQueue.enqueue was
called with the canonicalized slugs (e.g., "ALPHA","BETA") and "managed"; also
apply the same canonicalization rewrite/assertion to the other test block around
lines 438-449 that covers enqueueInstall()/related behavior so the regression is
pinned.

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

Inline comments:
In `@apps/controller/src/services/skillhub/skill-dir-watcher.ts`:
- Around line 98-113: The watcher setup using watch(...) with recursive: true
can silently fail on Node.js 20.0–20.12 and the error handler on this.watcher
only logs errors without recovery; update project metadata to require a
compatible Node.js by adding "engines": { "node": ">=20.13" } to
apps/controller/package.json (or add clear documentation about the limitation),
and add watcher error recovery in skill-dir-watcher.ts: when this.watcher emits
"error" call this.watcher.close() (or clear this.watcher), log the failure via
this.log, and schedule a restart of the watcher (e.g., setTimeout to recreate
the watch using the same watch(...) call and reattach the "error" handler) so
scheduleSync and the watch lifecycle are restored after transient failures.

In `@apps/web/src/pages/skills.tsx`:
- Around line 160-175: The new visible strings "Uninstall" and "Installing…" are
not localized; update the JSX in the conditional that uses isInstalled,
isQueueActive, isMutating and handleToggle to call the translation helper (t)
for both labels — e.g. replace the literal "Uninstall" with
t('skills.uninstall') (or the appropriate key used in your app) and replace
"Installing…" with t('skills.installing') — ensure the component imports and
uses the same t function/namespace as other strings in this file so the button
label and fallback span render localized text.

---

Duplicate comments:
In `@apps/web/src/pages/skills.tsx`:
- Around line 267-279: The current dedupe uses shownErrorSlugs (and
shownErrorSlugs.current) which never clears, so repeated failed installs for the
same slug in one session won't show a new toast; update the logic in the
useEffect around data?.queue to deduplicate per install attempt instead of per
slug by using an attempt-specific key (e.g., item.attemptId or item.createdAt)
when available (use shownErrorAttempts instead of shownErrorSlugs and key by
item.attemptId/item.createdAt), or if the queue has no attempt ID then remove
the slug from the set when that item leaves the failed state (detect item.status
!== "failed" and delete item.slug from shownErrorSlugs.current) before adding on
new failures; adjust references to shownErrorSlugs, useEffect, data?.queue,
item.status, item.slug, and toast.error accordingly.

---

Nitpick comments:
In `@apps/web/src/pages/skills.tsx`:
- Around line 41-53: The file repeats the same queue-status string literals
(e.g., in the prop type for the Skills card and elsewhere) that must stay
aligned with ACTIVE_QUEUE_STATUSES in use-community-catalog.ts; extract a single
shared QueueStatus type (string union or enum) and an ACTIVE_QUEUE_STATUSES
helper in a common module (or export the existing one from
use-community-catalog.ts), replace the inline literals in the Skills component
(props: queueStatus) and any other occurrences (e.g., card state checks and
polling logic) to import and use the shared QueueStatus type and
ACTIVE_QUEUE_STATUSES constant so all places reference the same source of truth.