fix(skills): revert dep-array misread + add test coverage for review

callmeYe · claude · callmeYe · commit fe243152145d · 2026-05-27T10:08:33.000+08:00
Review comment 3303143705 pointed at `buffer,` in the useMemo
dependency array, not the object literal. `buffer` is a correct dep
(the useMemo callback references `buffer.setText`). The object at
line 3489 already has the proper `setInputBuffer: buffer.setText`.
No source change needed — reply will clarify.

Test coverage (review comments 3304330911/17/23/27):
- useSelectionList.test.ts: `describe('disableVimNav')` — bare j/k
  suppression, ctrl+n pass-through, arrow-key navigation.
- slashCommandProcessor.test.ts: reload skipped when
  `consumeSlashReloadSuppression()` returns true.
- skill.test.ts (core): commandExecutor throws in disabled branch →
  graceful fallback to disabled-error message.
- config.integration.test.ts: `buildDisabledSkillNamesProvider` unit
  tests covering normal arrays, non-array inputs, mixed-type arrays,
  whitespace trimming, and empty-after-trim filtering.

Co-Authored-By: Claude Opus 4.6 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/packages/cli/src/config/config.integration.test.ts b/packages/cli/src/config/config.integration.test.ts
@@ -415,3 +415,50 @@ describe('Configuration Integration Tests', () => {
     });
   });
 });
+
+describe('buildDisabledSkillNamesProvider', async () => {
+  const { buildDisabledSkillNamesProvider } = await import('./config.js');
+
+  function fakeSettings(disabled: unknown) {
+    return { merged: { skills: { disabled } } } as never;
+  }
+
+  it('returns a normalized set from a normal array', () => {
+    const provider = buildDisabledSkillNamesProvider(
+      fakeSettings(['Foo', ' BAR ', 'baz']),
+    );
+    const result = provider();
+    expect(result).toEqual(new Set(['foo', 'bar', 'baz']));
+  });
+
+  it('returns empty set for non-array values (string)', () => {
+    const provider = buildDisabledSkillNamesProvider(fakeSettings('all'));
+    expect(provider()).toEqual(new Set());
+  });
+
+  it('returns empty set for non-array values (number)', () => {
+    const provider = buildDisabledSkillNamesProvider(fakeSettings(42));
+    expect(provider()).toEqual(new Set());
+  });
+
+  it('returns empty set for null/undefined', () => {
+    const provider = buildDisabledSkillNamesProvider(fakeSettings(null));
+    expect(provider()).toEqual(new Set());
+    const provider2 = buildDisabledSkillNamesProvider(fakeSettings(undefined));
+    expect(provider2()).toEqual(new Set());
+  });
+
+  it('filters non-string elements from a mixed-type array', () => {
+    const provider = buildDisabledSkillNamesProvider(
+      fakeSettings([42, null, 'valid', undefined, true, '  TRIMMED  ']),
+    );
+    expect(provider()).toEqual(new Set(['valid', 'trimmed']));
+  });
+
+  it('excludes empty-after-trim strings', () => {
+    const provider = buildDisabledSkillNamesProvider(
+      fakeSettings(['  ', '', 'keep']),
+    );
+    expect(provider()).toEqual(new Set(['keep']));
+  });
+});
diff --git a/packages/cli/src/ui/hooks/slashCommandProcessor.test.ts b/packages/cli/src/ui/hooks/slashCommandProcessor.test.ts
@@ -1102,6 +1102,65 @@ describe('useSlashCommandProcessor', () => {
       }
     });
 
+    it('should skip reload when consumeSlashReloadSuppression returns true', async () => {
+      const removeListener = vi.fn();
+      const addChangeListener = vi.fn().mockReturnValue(removeListener);
+      const fakeSkillManager = {
+        addChangeListener,
+        consumeSlashReloadSuppression: vi.fn(() => true),
+      };
+      const skillManagerSpy = vi
+        .spyOn(mockConfig, 'getSkillManager')
+        .mockReturnValue(
+          fakeSkillManager as unknown as ReturnType<
+            typeof mockConfig.getSkillManager
+          >,
+        );
+      try {
+        mockBuiltinLoadCommands.mockResolvedValue([]);
+        mockFileLoadCommands.mockResolvedValue([]);
+        mockMcpLoadCommands.mockResolvedValue([]);
+
+        const { unmount } = renderHook(() =>
+          useSlashCommandProcessor(
+            mockConfig,
+            mockSettings,
+            mockAddItem,
+            mockClearItems,
+            mockLoadHistory,
+            vi.fn(),
+            vi.fn(),
+            false,
+            vi.fn(),
+            { current: true },
+            vi.fn(),
+            createMockActions(),
+            new Map(),
+            true,
+            null,
+          ),
+        );
+
+        await waitFor(() => expect(addChangeListener).toHaveBeenCalledTimes(1));
+        await waitFor(() =>
+          expect(BuiltinCommandLoader).toHaveBeenCalledTimes(1),
+        );
+
+        const listener = addChangeListener.mock.calls[0][0] as () => void;
+        await act(async () => {
+          listener();
+        });
+
+        // When suppression is consumed, the listener should NOT trigger
+        // a second load — BuiltinCommandLoader stays at 1 call.
+        expect(BuiltinCommandLoader).toHaveBeenCalledTimes(1);
+
+        unmount();
+      } finally {
+        skillManagerSpy.mockRestore();
+      }
+    });
+
     it('should register SkillManager listener after config initialization', async () => {
       const removeListener = vi.fn();
       const addChangeListener = vi.fn().mockReturnValue(removeListener);
diff --git a/packages/cli/src/ui/hooks/useSelectionList.test.ts b/packages/cli/src/ui/hooks/useSelectionList.test.ts
@@ -1027,4 +1027,61 @@ describe('useSelectionList', () => {
       expect(mockOnSelect).not.toHaveBeenCalled();
     });
   });
+
+  describe('disableVimNav', () => {
+    it('bare j does NOT dispatch MOVE_DOWN when disableVimNav is true', () => {
+      const { result } = renderHook(() =>
+        useSelectionList({
+          items,
+          onSelect: mockOnSelect,
+          disableVimNav: true,
+        }),
+      );
+      expect(result.current.activeIndex).toBe(0);
+      pressKey('j');
+      expect(result.current.activeIndex).toBe(0);
+    });
+
+    it('bare k does NOT dispatch MOVE_UP when disableVimNav is true', () => {
+      const { result } = renderHook(() =>
+        useSelectionList({
+          items,
+          onSelect: mockOnSelect,
+          initialIndex: 2,
+          disableVimNav: true,
+        }),
+      );
+      expect(result.current.activeIndex).toBe(2);
+      pressKey('k');
+      expect(result.current.activeIndex).toBe(2);
+    });
+
+    it('Ctrl+N still dispatches MOVE_DOWN when disableVimNav is true', () => {
+      const { result } = renderHook(() =>
+        useSelectionList({
+          items,
+          onSelect: mockOnSelect,
+          disableVimNav: true,
+        }),
+      );
+      expect(result.current.activeIndex).toBe(0);
+      pressKey('n', 'n', { ctrl: true });
+      expect(result.current.activeIndex).toBe(2);
+    });
+
+    it('arrow keys still work when disableVimNav is true', () => {
+      const { result } = renderHook(() =>
+        useSelectionList({
+          items,
+          onSelect: mockOnSelect,
+          disableVimNav: true,
+        }),
+      );
+      expect(result.current.activeIndex).toBe(0);
+      pressKey('down');
+      expect(result.current.activeIndex).toBe(2);
+      pressKey('up');
+      expect(result.current.activeIndex).toBe(0);
+    });
+  });
 });
diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts
@@ -928,6 +928,26 @@ describe('SkillTool', () => {
       expect(llmText).toMatch(/is disabled/);
     });
 
+    it('falls through to disabled-error when commandExecutor throws', async () => {
+      vi.mocked(config.getDisabledSkillNames).mockReturnValue(
+        new Set(['mytool']),
+      );
+      const executor = vi.fn().mockRejectedValue(new Error('MCP timeout'));
+      vi.mocked(config.getModelInvocableCommandsExecutor).mockReturnValue(
+        executor,
+      );
+
+      const invocation = (
+        skillTool as SkillToolWithProtectedMethods
+      ).createInvocation({ skill: 'mytool' });
+      const result = await invocation.execute();
+
+      expect(executor).toHaveBeenCalledWith('mytool');
+      expect(mockSkillManager.loadSkillForRuntime).not.toHaveBeenCalled();
+      const llmText = partToString(result.llmContent);
+      expect(llmText).toMatch(/is disabled/);
+    });
+
     it('does not affect a skill that is not disabled', async () => {
       // Sanity check: with skills.disabled empty, the original
       // loadSkillForRuntime → executor fallback ordering still applies.
