src/commands/skills.ts (1)

348-374: Consider extracting shared subprocess spawning logic.

The subprocess spawning pattern here duplicates the logic in installViaAddSkill from skill-installer.ts. Both functions spawn bunx, pipe output, and capture stdout/stderr.

Consider extracting a shared helper (e.g., spawnBunxAddSkill(args: string[])) that both consumers can use, reducing duplication and ensuring consistent error handling.

src/commands/skills.test.ts (4)

190-250: Consider adding a test for conflicting --local and --global flags.

The tests cover individual flags well, but there's no test for what happens when both --local and --global are specified together. Based on the implementation in skills.ts, both flags would be set to true, which could lead to unexpected behaviour in buildAddSkillArgs.

📝 Suggested additional test case

test('handles both --local and --global specified', () => {
  const result = parseInstallArgs(['--local', '--global']);
  // Document expected behaviour - both true, or one takes precedence?
  expect(result.local).toBe(true);
  expect(result.global).toBe(true);
});

---

332-350: Consider adding a note about why install tests are minimal.

The install command tests are intentionally minimal since the actual installation is delegated to the external bunx add-skill CLI. Consider adding a brief comment explaining this design decision, which would help future maintainers understand why more comprehensive install tests aren't present here.

📝 Suggested documentation

 describe('skills install command', () => {
+  // NOTE: Actual installation tests are limited because the command delegates
+  // to the external `bunx add-skill` CLI. Unit tests for argument parsing and
+  // output parsing are in the parseInstallArgs and parseAddSkillOutput blocks.
   let consoleSpy: ReturnType<typeof spyOn>;

---

352-415: Good coverage of output parsing, consider adding edge cases for mixed failures.

The tests thoroughly cover the main scenarios including the ELOOP-only failure handling (which is important per the PR objectives). Consider adding tests for:

1. Mixed ELOOP and non-ELOOP failures - to verify eloopOnly is correctly false when both error types exist
2. Partial/malformed output - to verify graceful handling when some patterns match but others don't

📝 Suggested additional test cases

test('handles mixed ELOOP and non-ELOOP failures', () => {
  const output = `Found 2 skills
Detected 2 agents
Installing to: Claude Code, OpenCode
Installation complete
Failed to install 2
ELOOP: too many symbolic links encountered
ENOENT: no such file or directory`;
  const result = parseAddSkillOutput(output);
  expect(result.failureCount).toBe(2);
  expect(result.eloopOnly).toBe(false); // Not ELOOP-only since ENOENT present
});

test('handles partial output with only skill count', () => {
  const output = `Found 3 skills`;
  const result = parseAddSkillOutput(output);
  expect(result.skillCount).toBe(3);
  expect(result.agentCount).toBe(0);
  expect(result.installed).toBe(false);
});

---

77-89: Unused consoleErrorSpy in test setup.

The consoleErrorSpy is set up at line 83 and restored at line 88, but it's not used in any of the tests within this describe block. Either remove it to reduce test setup overhead, or add a test for the unknown subcommand error case (which logs to console.error per the implementation).

📝 Option 1: Remove unused spy

 describe('executeSkillsCommand', () => {
   let consoleSpy: ReturnType<typeof spyOn>;
-  let consoleErrorSpy: ReturnType<typeof spyOn>;

   beforeEach(() => {
     consoleSpy = spyOn(console, 'log').mockImplementation(() => {});
-    consoleErrorSpy = spyOn(console, 'error').mockImplementation(() => {});
   });

   afterEach(() => {
     consoleSpy.mockRestore();
-    consoleErrorSpy.mockRestore();
   });

📝 Option 2: Add test for unknown subcommand

test('shows error for unknown subcommand', async () => {
  const exitSpy = spyOn(process, 'exit').mockImplementation(() => { throw new Error('exit'); });
  
  try {
    await executeSkillsCommand(['unknown']);
  } catch (e) {
    // Expected due to process.exit mock
  }
  
  expect(consoleErrorSpy).toHaveBeenCalled();
  const errorOutput = consoleErrorSpy.mock.calls[0][0];
  expect(errorOutput).toContain('Unknown subcommand');
  
  exitSpy.mockRestore();
});

src/commands/skills.ts (1)

231-239: Avoid temporal wording in comments.

“Now the default” introduces time‑dependent wording; rephrase to an evergreen statement.

♻️ Suggested tweak

-    // Accepted for backwards compat but is now the default
+    // Accepted for backwards compat; default behaviour installs all skills

As per coding guidelines, avoid temporal context in comments.