[Critical] tsc reports 3 type errors here: Type 'number' is not assignable to type 'string'. Lines 459, 460, and 462 all pass numeric values where string props are expected. These cause npm run build to fail.

— qwen3.7-max via Qwen Code /review

Already fixed in 6dc3b33 (landed on this branch before this review was filed) — String(matchedCount) / String(totalCount) are present on the affected lines. tsc --noEmit is clean against the latest tip.

[Critical] Race condition: if the user presses Esc while skills are still loading (skills === null, selectedKeys === null), this line creates new Set([]) (empty), the loop over unlockedSkills (also empty) produces nextDisabled = [], and settings.setValue(...) writes undefined — silently clearing all workspace-level disabled entries from .qwen/settings.json. The success toast "Skills configuration saved." still fires.

The useKeypress handler at line 349 has { isActive: true } unconditionally, so Esc is captured even during the "Loading skills..." state.

— qwen3.7-max via Qwen Code /review

Fixed in a0fd5d6.

handleSaveAndClose now early-returns when skills === null || selectedKeys === null, so pressing Esc while still on the "Loading skills…" state just closes the dialog without ever entering persistChanges. The would-be Set([]) → empty nextDisabled → setValue(..., undefined) path is gone.

[Critical] If skills.disabled contains a non-string entry (e.g. from manual JSON editing: "disabled": [42]), .trim() throws TypeError: n.trim is not a function. This provider is consumed by validateToolParams (skill.ts:304) and execute (skill.ts:419) — neither wraps getDisabledSkillNames() in a try/catch (unlike refreshSkills at line 115 which does). The unhandled exception bricks every skill tool invocation until the settings file is manually repaired.

Also consider adding items: { type: 'string' } to the schema in settingsSchema.ts for defense in depth.

— qwen3.7-max via Qwen Code /review

Fixed in a0fd5d6.

buildDisabledSkillNamesProvider now filters non-strings before normalizing:

return new Set(
  list
    .filter((n): n is string => typeof n === 'string')
    .map((n) => n.trim().toLowerCase())
    .filter(Boolean),
);

A stray [42] no longer throws out of the provider, so validateToolParams and execute can't be bricked by hand-edited settings.

On the schema side: I left settingsSchema.ts as-is because the generator already emits items: { type: 'string' } for typeless arrays (see scripts/generate-settings-schema.ts:134), and the regenerated settings.schema.json in this commit shows that. Happy to add an explicit items to the TS schema for defense in depth if you'd prefer — let me know.

[Critical] activeValue is populated only by MultiSelect.onHighlight (arrow-key navigation). Two failure modes:

1. On mount: useSelectionList's INITIALIZE sets pendingHighlight: false, so onHighlight never fires on initial render. activeValue stays null — pressing Enter immediately after the dialog opens is a no-op.
2. After search filtering: When the user types a query that filters out the previously highlighted item, INITIALIZE fires again with pendingHighlight: false. activeValue still points to the now-invisible skill. Pressing Enter invokes handlePick on a stale (filtered-out) skill.

Consider initializing activeValue from filteredUnlocked[0] when skills load, and re-deriving when the filtered list changes and the current activeValue is no longer present.

— qwen3.7-max via Qwen Code /review

Fixed in a0fd5d6.

Added a useEffect that:

• seeds activeValue from filteredUnlocked[0] on initial mount (cures the on-mount "Enter is a no-op" path); and
• re-derives activeValue to filteredUnlocked[0] whenever the previously-highlighted skill is no longer in the filtered list (cures the after-filter "Enter invokes a stale skill" path).

Depends on both filteredUnlocked and activeValue so arrow-key navigation (which fires onHighlight → setActiveValue) doesn't fight against it — when the user moves the highlight to another visible row, stillVisible is true and the effect is a no-op.

[Critical] When a disabled skill shares its name with a model-invocable command and commandExecutor returns non-null content, this branch logs SkillLaunchEvent(name, true) and calls onSkillLoaded(name) — even though no skill was loaded from disk. This produces misleading telemetry (dashboards report a successful skill launch) and pollutes loadedSkillNames (used by /context token attribution).

Consider using a distinct returnDisplay (e.g. "Delegated to command: ${name}"), skipping onSkillLoaded (no skill body was loaded), and either omitting SkillLaunchEvent or emitting it with success: false to distinguish delegation from actual skill execution.

— qwen3.7-max via Qwen Code /review

Fixed in a0fd5d6.

The disabled-skill → command-delegation branch in skill.ts no longer:

• emits SkillLaunchEvent (so skill-launch dashboards aren't inflated by command runs); or
• calls onSkillLoaded (so /context skill-token attribution doesn't count command bytes against skill bodies).

returnDisplay is now Delegated to command: ${this.params.skill} (matching your suggestion) so the UI/transcript clearly distinguishes pass-through from real skill execution. The corresponding assertion in skill.test.ts was updated.

Scope note: I deliberately did NOT touch the parallel pass-through inside the not-found-skill branch (skill.ts:451-465) — that's pre-existing behavior unrelated to disabled-skill semantics, and changing it felt like scope creep for this PR. Happy to do it as a follow-up if you'd like consistent telemetry/UX across both pass-through paths.

[Suggestion] This 553-line component has zero test coverage. Key correctness properties that are entirely unverified:

1. persistChanges ordering invariant (lines 282-293): the comment says "ORDER MATTERS — must NOT be Promise.all" but no test asserts reloadCommands completes before notifyConfigChanged.
2. Locked-exclusion invariant: locked names are never re-emitted into workspace scope on confirm — no assertion.
3. Pick semantic: Enter fills setInputBuffer without submitting — untested.
4. Untrusted workspace guard: returns false and shows ERROR — untested.
5. Search filtering over both name and description — untested.

Ink component tests exist elsewhere in the tree (e.g. PermissionsDialog, McpDialog), so there's a well-established pattern to follow.

— qwen3.7-max via Qwen Code /review

Acknowledged but not addressed in this round — flagging explicitly so it isn't lost.

Each of the 5 specific properties you listed is real and worth covering, but adding ~5 Ink-component tests against a 553-line dialog is meaningfully larger than the rest of this review combined. I'd rather land it as a follow-up PR scoped specifically to dialog tests, modeled after the PermissionsDialog / McpDialog patterns you mentioned.

The other 11 review comments (5 [Critical] + 6 [Suggestion]) are all fixed in a0fd5d6. Tested locally:

• core: 297 passed
• cli: 383 passed (1 skipped) — including all skill-related and the previously-failing acpAgent/gemini suites
• eslint: clean on touched files
• tsc --noEmit: clean on cli + core
• npm run generate:settings-schema produces the same diff CI was expecting

Let me know if you'd prefer the dialog tests in this PR rather than a follow-up.

[Suggestion] persistChanges unconditionally writes skills.disabled to disk and triggers reloadCommands() + notifyConfigChanged() on every Esc/Enter — even when no toggles were made. Opening /skills and pressing Esc immediately still writes to disk and rebuilds the entire command surface.

Consider a quick equality check against initialWorkspaceDisabled before writing:

const unchanged = previousWorkspace.length === nextDisabled.length &&
  previousWorkspace.every((n, i) => lower(n) === lower(nextDisabled[i]));
if (unchanged) return true;

— qwen3.7-max via Qwen Code /review

Fixed in a0fd5d6.

persistChanges now compares the normalized previous and next workspace lists before deciding to write:

const prevNormalized = normalizeNames(previousWorkspace).sort();
const nextNormalized = normalizeNames(nextDisabled).sort();
const unchanged =
  prevNormalized.length === nextNormalized.length &&
  prevNormalized.every((n, i) => n === nextNormalized[i]);
if (unchanged) return 'ok';

When unchanged: no setValue, no reloadCommands(), no notifyConfigChanged(). Opening /skills and immediately pressing Esc is a pure no-op now. Normalizing both sides means whitespace/case-only edits in the JSON file are also treated as no-changes (consistent with [[lower]] / [[buildDisabledSkillNamesProvider]] above).

[Suggestion] settings.setValue(...) is outside the try/catch that wraps reloadCommands / notifyConfigChanged below. If the write throws (disk full, permission denied, corrupted settings), the error propagates out of persistChanges and both callers (handleSaveAndClose, handlePick) discard the promise with void. The dialog stays open with no user-visible error — Esc presses keep re-throwing.

Either wrap settings.setValue in the existing try/catch (surface a MessageType.ERROR), or add try { ... } finally { onClose(); } in handleSaveAndClose so the dialog can always be dismissed.

— qwen3.7-max via Qwen Code /review

Fixed in a0fd5d6.

settings.setValue now lives in its own try/catch that surfaces the error as MessageType.ERROR with a new i18n key 'Failed to save skills configuration: {{error}}' and returns 'error' from persistChanges. The function's return type is now 'ok' | 'untrusted' | 'error' so callers can disambiguate.

Both callers always close the dialog after persistChanges resolves:

• handleSaveAndClose calls onClose() unconditionally at the end
• handlePick calls onClose() unconditionally and only fills the input buffer when result === 'ok'

So a re-throwing Esc is no longer reachable — the dialog can always be dismissed.

[Suggestion] This t() key — 'All available skills are disabled. Edit ~/.qwen/settings.json or .qwen/settings.json (skills.disabled) to re-enable.' — is missing from all 9 locale files (en.js, ca.js, de.js, fr.js, ja.js, pt.js, ru.js, zh.js, zh-TW.js) and from mustTranslateKeys.ts. Every other new t() key introduced by this PR is present in all locale files — this is the sole outlier. The t() fallback returns the key verbatim so English works, but the string will never translate.

— qwen3.7-max via Qwen Code /review

Fixed in a0fd5d6.

Added the missing key to all 9 locale files (en.js, ca.js, de.js, fr.js, ja.js, pt.js, ru.js, zh.js, zh-TW.js):

'All available skills are disabled. Edit ~/.qwen/settings.json or .qwen/settings.json (skills.disabled) to re-enable.'

Also added the new 'Failed to save skills configuration: {{error}}' key (introduced by the setValue try/catch fix on the dialog side) to all 9.

Did NOT add either to mustTranslateKeys.ts — that list looks curated rather than exhaustive (it currently has ~4 of the ~20 skills strings the dialog uses). If you'd prefer them in the must-translate set, let me know and I'll move them in.

[Suggestion] handlePick doesn't check whether the picked skill is currently enabled (in selectedKeys). A user can: (1) uncheck a skill (toggling it off), (2) keep the cursor on that row, (3) press Enter. Result: persistChanges saves it as disabled, the dialog closes, and /<disabled-skill-name> is placed in the input buffer. On submit, the user gets the "Skill is disabled" error.

Consider guarding: if (selectedKeys && !selectedKeys.includes(skill.name)) { onClose(); return; } before filling the input buffer.

— qwen3.7-max via Qwen Code /review

Fixed in a0fd5d6.

handlePick now gates on:

const isEnabled =
  selectedKeys !== null &&
  selectedKeys.includes(skill.name) &&
  !higher.set.has(lower(skill.name));
if (!isEnabled) {
  onClose();
  return;
}

So the three "should not fill" cases all close the dialog without writing to the input buffer:

1. selectedKeys still loading (null)
2. user just toggled the row off (not in selectedKeys)
3. row is locked at higher scope (defense in depth — shouldn't be reachable through MultiSelect, but activeValue could in theory hold a stale reference)

Picking still requires a real, currently-enabled skill — no more /<disabled-skill> landing in the buffer.

[Critical] buildDisabledSkillNamesProvider (and the parallel namesFromScope in SkillsManagerDialog.tsx:103) assume skills.disabled is always an array. The ?? [] fallback only catches null/undefined — if the user edits settings.json and sets "disabled": "all" or "disabled": 42, the value passes through ?? and .filter(...) throws TypeError: list.filter is not a function.

The prior fix (a0fd5d6) added per-element typeof n === 'string' filtering for mixed-type arrays, but the value itself can be a non-array. This crashes the CLI on cold-start (Config.initialize() → warmAll() → SkillTool → getDisabledSkillNames()) and the /skills dialog on open.

Same gap exists in SkillsManagerDialog.tsx:103 (namesFromScope) and line 305 (persistChanges builds previousMap from the raw array — lower(42) throws TypeError: n.trim is not a function).

— qwen3.7-max via Qwen Code /review

Fixed in 17498e7.

Both buildDisabledSkillNamesProvider (config.ts) and namesFromScope (SkillsManagerDialog.tsx) now guard with Array.isArray(raw) before iterating:

const raw = loadedSettings.merged.skills?.disabled;
const list = Array.isArray(raw) ? raw : [];

The previousMap build in persistChanges also adds a .filter((n): n is string => typeof n === 'string') over previousWorkspace before calling lower(), so lower(42) can never happen even if a stray integer survives the Array.isArray gate.

[Suggestion] Both disabled-skill error messages (here and at line 440) reference /skills manage, but the manage subcommand was dropped in this PR — bare /skills now opens the dialog directly. The model will see this stale text and may try to invoke a non-existent /skills manage command.

Apply the same change at line 440.

— qwen3.7-max via Qwen Code /review

Fixed in 17498e7.

Both error messages (validateToolParams:305 and execute:440) now say:

Re-enable it via /skills or remove it from skills.disabled.

replace_all replaced both occurrences at once.

[Suggestion] Command.ACCEPT_SUGGESTION matches both Tab and Enter (see keyBindings.ts). When the accepted suggestion has submitOnAccept: true (e.g. /skills), both keys trigger handleSubmitAndClear. Typing /skil<Tab> will immediately submit /skills and open the dialog instead of just filling the buffer — breaking the standard shell convention where Tab means "complete without executing."

The codebase itself acknowledges this asymmetry at lines 861-862:

// Use explicit key.name === 'tab' instead of ACCEPT_SUGGESTION matcher,
// because ACCEPT_SUGGESTION also matches Enter which must fall through to SUBMIT.

Gate the auto-submit on Enter only:

— qwen3.7-max via Qwen Code /review

Fixed in 17498e7.

Added && key.name === 'return' gate:

if (accepted?.submitOnAccept && key.name === 'return') {
  handleSubmitAndClear(\`/\${accepted.value}\`);
}

Tab now fills the buffer with /<suggestion> but does NOT submit — matches the standard shell "complete without executing" convention the codebase already acknowledges at lines 861-862.

[Suggestion] The commandExecutor call in the disabled branch is not wrapped in a try/catch, but the identical call in the non-disabled path (line 453) IS inside a try/catch block (line 444). If the MCP server throws (network error, timeout, protocol violation), the disabled branch propagates an unhandled rejection out of execute(), while the non-disabled path degrades gracefully.

Disabling a skill can paradoxically make the system LESS robust to MCP failures for same-named commands.

— qwen3.7-max via Qwen Code /review

Fixed in 17498e7.

The disabled-branch commandExecutor call is now wrapped in try/catch matching the non-disabled path:

try {
  const content = await this.commandExecutor(this.params.skill);
  if (content !== null) { ... }
} catch {
  // Fall through to the disabled-error message below.
}

If the MCP server throws, the user now sees the standard disabled-skill error rather than an unhandled rejection propagating out of execute().

[Suggestion] await reloadCommands() here triggers a full CommandService rebuild. Then await skillManager.notifyConfigChanged() on the next line fires all registered change listeners — including the one at slashCommandProcessor.ts:416 which calls reloadCommands() again fire-and-forget (the listener is () => void, not () => Promise<void>, so notifyChangeListeners's Promise.allSettled treats it as instantly resolved).

Result: every /skills save triggers two CommandService rebuilds — one awaited, one orphaned. The ordering-invariant comment carefully documents why steps 1 and 2 must be serial, but is silent about the phantom step-3 rebuild that step 2 triggers as a side effect.

Two options:

1. Remove the explicit await reloadCommands() and rely on notifyConfigChanged() to trigger it via the change listener — but also fix the listener to return the Promise: addChangeListener(async () => { await reloadCommands(); }).
2. Keep both calls but suppress the listener-triggered reload (e.g. a skipNextReload flag).

— qwen3.7-max via Qwen Code /review

Fixed in 17498e7 — went with a variant of option 2.

Added a one-shot suppression flag on SkillManager:

// SkillManager
suppressNextSlashReload(): void { ... }
consumeSlashReloadSuppression(): boolean { ... }  // read-and-clear

The dialog calls skillManager.suppressNextSlashReload() immediately before notifyConfigChanged(). In slashCommandProcessor's listener:

return skillManager.addChangeListener(() => {
  if (skillManager.consumeSlashReloadSuppression()) return;
  reloadCommands();
});

This preserves the explicit-reload-first ordering invariant (which guarantees SkillTool.refreshSkills sees the new provider) while eliminating the redundant listener-driven second rebuild. External changes (filesystem watch, settings reload from outside the dialog) leave the flag false and continue reloading normally.

I considered option 1 (remove the explicit reloadCommands, rely on the listener) but it breaks the ordering: notifyChangeListeners runs listeners in parallel via Promise.allSettled, so SkillTool.refreshSkills would race against the listener's reloadCommands and potentially read the OLD modelInvocableCommandsProvider. The flag is a more surgical fix.

[Suggestion] When isEnabled is false, handlePick calls onClose() without first calling persistChanges(). Any toggle changes the user made to other skills during the dialog session are silently discarded.

This fires in a realistic scenario: user toggles a skill off, cursor stays on the row, user presses Enter. The !isEnabled branch fires and all other pending toggles are lost — inconsistent with handleSaveAndClose (Esc), which always calls persistChanges() when selectedKeys !== null.

— qwen3.7-max via Qwen Code /review

Fixed in 17498e7.

The `!isEnabled` branch now persists before closing:

if (!isEnabled) {
  if (skills !== null && selectedKeys !== null) {
    await persistChanges();
  }
  onClose();
  return;
}

Keeps Enter consistent with Esc (handleSaveAndClose) — both always persist pending toggles when data has loaded. The only difference is Enter-on-enabled ALSO fills the input buffer.

[Suggestion] The description was changed from 'List available skills.' to 'Open the skills panel (browse, search, toggle, pick).', but the old translation key 'List available skills.' remains in all 9 locale files (en.js, ca.js, de.js, fr.js, ja.js, pt.js, ru.js, zh.js, zh-TW.js) as dead code — it's no longer referenced anywhere in source.

Remove the orphaned entry from each locale file to keep them clean.

— qwen3.7-max via Qwen Code /review

Fixed in 17498e7.

Removed the orphaned `'List available skills.'` entry from all 9 locale files (en, ca, de, fr, ja, pt, ru, zh, zh-TW). Verified with `grep -rn "List available skills" packages/cli/src/i18n/locales/` — zero hits.

[Suggestion] j and k are deferred to MultiSelect for vim-style navigation (SELECTION_DOWN/SELECTION_UP in keyBindings.ts), but this also blocks them from the search filter. Users cannot search for skills whose names contain j or k (e.g., "json", "jwt", "kotlin", "jdk") — these extremely common letters are silently consumed by navigation instead.

Other search-enabled dialogs in the codebase don't combine MultiSelect with a dialog-level text search, so this is a unique conflict.

Also pass isFocused={!query} to the <MultiSelect> component (around line 600) so vim navigation is disabled when the user is actively searching:

<MultiSelect
  items={items}
  isFocused={!query}
  selectedKeys={selectedKeys ?? []}
  ...

— qwen3.7-max via Qwen Code /review

Fixed in b72e5c0.

Two changes:

1. j/k deferral is now conditional on !query:

if ((key.name === 'j' || key.name === 'k') && !query) {
  return;
}

2. MultiSelect receives isFocused={!query} so its internal vim-key handler is disabled during search:

<MultiSelect items={items} isFocused={!query} ... />

Behavior matrix:

• Query empty + j/k → dialog defers, MultiSelect (isFocused=true) handles vim nav
• Query non-empty + j/k → dialog appends to search, MultiSelect (isFocused=false) ignores

Users can now search for "json", "jwt", "kotlin" etc. without j/k being swallowed.

[Critical] isFocused={!query} deactivates ALL of MultiSelect's key handlers while a search query is active. Verified in MultiSelect.tsx (space handler: isActive: isFocused) and useSelectionList.ts (arrows/Enter: isActive: !!(isFocused && items.length > 0)). Meanwhile the outer useKeypress (lines 491-495) still unconditionally returns early for up/down/space/return — deferring to a now-inactive MultiSelect. Result: arrow keys, space, and Enter are completely dead while searching. Users cannot navigate the filtered list, toggle checkboxes, or pick a skill.

The j/k search fix (conditional deferral + isFocused={!query}) correctly solves the j/k-as-filter-character problem, but the isFocused approach over-disables. Suggested fix: keep MultiSelect focused and add a targeted way to suppress only j/k vim-nav during search, e.g. a disableVimNav prop on useSelectionList that skips the j/k key matchers while keeping arrows/space/Enter active.

— qwen3.7-max via Qwen Code /review

Fixed in 42e5b06.

Replaced isFocused={!query} (which over-disabled ALL of MultiSelect's handlers) with a targeted disableVimNav prop:

useSelectionList.ts — new disableVimNav?: boolean option:

if (keyMatchers[Command.SELECTION_UP](key)) {
  if (disableVimNav && key.name === 'k' && !key.ctrl) {
    // Skip bare 'k' — let caller's printable-char handler use it
  } else {
    dispatch({ type: 'MOVE_UP', payload: { items } });
    return;
  }
}
// (same for SELECTION_DOWN + 'j')

MultiSelect.tsx — threads disableVimNav to useSelectionList.

SkillsManagerDialog.tsx — disableVimNav={!!query} instead of isFocused={!query}:

<MultiSelect items={items} disableVimNav={!!query} ... />

Behavior matrix:

• Arrows/space/Enter: always work (navigate, toggle, pick) — isFocused stays true
• j/k when idle (!query): vim-nav (handled by useSelectionList)
• j/k when searching (!!query): filter chars (useSelectionList skips them, outer handler appends to query)
• ctrl+p/n: always work for navigation (not suppressed by disableVimNav)

[Suggestion] Dead buffer, property — not in the UIActions interface and no consumer reads actions.buffer from the context. The first hunk (line 3489) correctly adds setInputBuffer: buffer.setText,; this second hunk appears to be a copy-paste artifact that passes the entire buffer object instead.

— qwen3.7-max via Qwen Code /review

No source change needed — this line (3570) is in the useMemo dependency array, not the returned object. `buffer` is a correct dep: the useMemo callback (line 3489) reads `buffer.setText`, so React needs to know when `buffer` changes to recompute the memo.

The object literal at line 3489 already has the proper `setInputBuffer: buffer.setText,`. No duplication or dead code here — just two sections (callback vs deps) that look similar because they share the same variable names.