test(skill,mcp): improve test coverage for writeSkillOutput and fileSystemReadFileTool

.agents/commands/git/pr-resolve-addressed.md

@@ -0,0 +1,24 @@
+# Resolve Addressed PR Review Comments
+
+Resolve review comments that have been **clearly addressed** in the code.
+
+## Important
+
+- Check each comment individually
+- Only resolve if the specific fix is confirmed in the code
+- Do NOT resolve comments that are unclear or still pending
+
+## Steps
+
+1. Get unresolved thread IDs from the PR
+2. For each comment, verify the fix exists in the code
+3. Resolve only confirmed threads using `resolveReviewThread` mutation
+
+```bash
+gh api graphql -f query='
+mutation {
+  resolveReviewThread(input: {threadId: "PRRT_xxx"}) {
+    thread { isResolved }
+  }
+}'
+```

tests/core/skill/writeSkillOutput.test.ts

@@ -1,6 +1,7 @@
 import path from 'node:path';
 import { describe, expect, test, vi } from 'vitest';
 import { writeSkillOutput } from '../../../src/core/skill/writeSkillOutput.js';
+import { RepomixError } from '../../../src/shared/errorHandle.js';
 
 describe('writeSkillOutput', () => {
   test('should create skill directory structure and write files', async () => {
@@ -74,4 +75,133 @@ describe('writeSkillOutput', () => {
       recursive: true,
     });
   });
+
+  test('should write tech-stack.md when techStack is provided', async () => {
+    const mockMkdir = vi.fn().mockResolvedValue(undefined);
+    const mockWriteFile = vi.fn().mockResolvedValue(undefined);
+
+    const output = {
+      skillMd: '# Skill',
+      references: {
+        summary: '# Summary',
+        structure: '# Structure',
+        files: '# Files',
+        techStack: '# Tech Stack\n\n- TypeScript\n- Node.js',
+      },
+    };
+
+    const skillDir = '/test/project/.claude/skills/test-skill';
+
+    await writeSkillOutput(output, skillDir, {
+      mkdir: mockMkdir as unknown as typeof import('node:fs/promises').mkdir,
+      writeFile: mockWriteFile as unknown as typeof import('node:fs/promises').writeFile,
+    });
+
+    // Check tech-stack.md was written
+    expect(mockWriteFile).toHaveBeenCalledWith(
+      path.join(skillDir, 'references', 'tech-stack.md'),
+      output.references.techStack,
+      'utf-8',
+    );
+  });
+
+  test('should throw RepomixError with permission message on EPERM error', async () => {
+    const mockMkdir = vi.fn().mockResolvedValue(undefined);
+    const permError = new Error('Permission denied') as NodeJS.ErrnoException;
+    permError.code = 'EPERM';
+    const mockWriteFile = vi.fn().mockRejectedValue(permError);
+
+    const output = {
+      skillMd: '# Skill',
+      references: {
+        summary: '# Summary',
+        structure: '# Structure',
+        files: '# Files',
+      },
+    };
+
+    const skillDir = '/test/project/.claude/skills/test-skill';
+
+    const promise = writeSkillOutput(output, skillDir, {
+      mkdir: mockMkdir as unknown as typeof import('node:fs/promises').mkdir,
+      writeFile: mockWriteFile as unknown as typeof import('node:fs/promises').writeFile,
+    });
+
+    await expect(promise).rejects.toThrow(RepomixError);
+    await expect(promise).rejects.toThrow(/Permission denied/);
+  });
+
+  test('should throw RepomixError with permission message on EACCES error', async () => {
+    const mockMkdir = vi.fn().mockResolvedValue(undefined);
+    const accessError = new Error('Access denied') as NodeJS.ErrnoException;
+    accessError.code = 'EACCES';
+    const mockWriteFile = vi.fn().mockRejectedValue(accessError);
+
+    const output = {
+      skillMd: '# Skill',
+      references: {
+        summary: '# Summary',
+        structure: '# Structure',
+        files: '# Files',
+      },
+    };
+
+    const skillDir = '/test/project/.claude/skills/test-skill';
+
+    const promise = writeSkillOutput(output, skillDir, {
+      mkdir: mockMkdir as unknown as typeof import('node:fs/promises').mkdir,
+      writeFile: mockWriteFile as unknown as typeof import('node:fs/promises').writeFile,
+    });
+
+    await expect(promise).rejects.toThrow(RepomixError);
+    await expect(promise).rejects.toThrow(/Permission denied/);
+  });
+
+  test('should throw RepomixError with generic message on other errors', async () => {
+    const mockMkdir = vi.fn().mockResolvedValue(undefined);
+    const genericError = new Error('Disk full');
+    const mockWriteFile = vi.fn().mockRejectedValue(genericError);
+
+    const output = {
+      skillMd: '# Skill',
+      references: {
+        summary: '# Summary',
+        structure: '# Structure',
+        files: '# Files',
+      },
+    };
+
+    const skillDir = '/test/project/.claude/skills/test-skill';
+
+    const promise = writeSkillOutput(output, skillDir, {
+      mkdir: mockMkdir as unknown as typeof import('node:fs/promises').mkdir,
+      writeFile: mockWriteFile as unknown as typeof import('node:fs/promises').writeFile,
+    });
+
+    await expect(promise).rejects.toThrow(RepomixError);
+    await expect(promise).rejects.toThrow(/Disk full/);
+  });
+
+  test('should handle non-Error objects in catch block', async () => {
+    const mockMkdir = vi.fn().mockResolvedValue(undefined);
+    const mockWriteFile = vi.fn().mockRejectedValue('string error');
+
+    const output = {
+      skillMd: '# Skill',
+      references: {
+        summary: '# Summary',
+        structure: '# Structure',
+        files: '# Files',
+      },
+    };
+
+    const skillDir = '/test/project/.claude/skills/test-skill';
+
+    await expect(
+      writeSkillOutput(output, skillDir, {
+        mkdir: mockMkdir as unknown as typeof import('node:fs/promises').mkdir,
+        writeFile: mockWriteFile as unknown as typeof import('node:fs/promises').writeFile,
+      }),
+    ).rejects.toThrow(RepomixError);
+  });
 });

tests/mcp/tools/fileSystemReadFileTool.test.ts

@@ -1,12 +1,24 @@
-import { promises as fs } from 'node:fs';
+import fs from 'node:fs/promises';
 import path from 'node:path';
 import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
 import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js';
 import { beforeEach, describe, expect, test, vi } from 'vitest';
 import { registerFileSystemReadFileTool } from '../../../src/mcp/tools/fileSystemReadFileTool.js';
 
-vi.mock('node:fs');
+vi.mock('node:fs/promises', () => ({
+  default: {
+    access: vi.fn(),
+    stat: vi.fn(),
+    readFile: vi.fn(),
+  },
+}));
 vi.mock('node:path');
+vi.mock('../../../src/core/security/workers/securityCheckWorker.js', () => ({
+  createSecretLintConfig: vi.fn().mockReturnValue({}),
+  runSecretLint: vi.fn().mockResolvedValue(null),
+}));
+
+import { runSecretLint } from '../../../src/core/security/workers/securityCheckWorker.js';
 
 describe('FileSystemReadFileTool', () => {
   const mockServer = {
@@ -66,4 +78,142 @@ describe('FileSystemReadFileTool', () => {
       ],
     });
   });
+
+  test('should handle directory path error', async () => {
+    const testPath = '/some/directory';
+    vi.mocked(path.isAbsolute).mockReturnValue(true);
+    vi.mocked(fs.access).mockResolvedValueOnce(undefined);
+    vi.mocked(fs.stat).mockResolvedValueOnce({
+      isDirectory: () => true,
+    } as unknown as Awaited<ReturnType<typeof fs.stat>>);
+
+    const result = await toolHandler({ path: testPath });
+
+    expect(result).toEqual({
+      isError: true,
+      content: [
+        {
+          type: 'text',
+          text: JSON.stringify(
+            {
+              errorMessage: `Error: The specified path is a directory, not a file: ${testPath}. Use file_system_read_directory for directories.`,
+            },
+            null,
+            2,
+          ),
+        },
+      ],
+    });
+  });
+
+  test('should successfully read file and return content', async () => {
+    const testPath = '/test/file.txt';
+    const fileContent = 'Line 1\nLine 2\nLine 3';
+    vi.mocked(path.isAbsolute).mockReturnValue(true);
+    vi.mocked(fs.access).mockResolvedValueOnce(undefined);
+    vi.mocked(fs.stat)
+      .mockResolvedValueOnce({
+        isDirectory: () => false,
+      } as unknown as Awaited<ReturnType<typeof fs.stat>>)
+      .mockResolvedValueOnce({
+        size: 21,
+      } as unknown as Awaited<ReturnType<typeof fs.stat>>);
+    vi.mocked(fs.readFile).mockResolvedValueOnce(fileContent);
+    vi.mocked(runSecretLint).mockResolvedValueOnce(null);
+
+    const result = await toolHandler({ path: testPath });
+
+    const expectedContent = {
+      path: testPath,
+      content: fileContent,
+      size: 21,
+      encoding: 'utf8',
+      lines: 3,
+    };
+    expect(result).toEqual({
+      content: [
+        {
+          type: 'text',
+          text: JSON.stringify(expectedContent, null, 2),
+        },
+      ],
+      structuredContent: expectedContent,
+    });
+  });
+
+  test('should block file when security check fails', async () => {
+    const testPath = '/test/secrets.txt';
+    const fileContent = 'API_KEY=secret123';
+    vi.mocked(path.isAbsolute).mockReturnValue(true);
+    vi.mocked(fs.access).mockResolvedValueOnce(undefined);
+    vi.mocked(fs.stat)
+      .mockResolvedValueOnce({
+        isDirectory: () => false,
+      } as unknown as Awaited<ReturnType<typeof fs.stat>>)
+      .mockResolvedValueOnce({
+        size: 17,
+      } as unknown as Awaited<ReturnType<typeof fs.stat>>);
+    vi.mocked(fs.readFile).mockResolvedValueOnce(fileContent);
+    vi.mocked(runSecretLint).mockResolvedValueOnce({
+      filePath: testPath,
+      messages: ['Found potential secret'],
+      type: 'file',
+    });
+
+    const result = await toolHandler({ path: testPath });
+
+    expect(result).toEqual({
+      isError: true,
+      content: [
+        {
+          type: 'text',
+          text: JSON.stringify(
+            {
+              errorMessage: `Error: Security check failed. The file at ${testPath} may contain sensitive information.`,
+            },
+            null,
+            2,
+          ),
+        },
+      ],
+    });
+  });
+
+  test('should handle general errors during file reading', async () => {
+    const testPath = '/test/file.txt';
+    vi.mocked(path.isAbsolute).mockReturnValue(true);
+    vi.mocked(fs.access).mockResolvedValueOnce(undefined);
+    vi.mocked(fs.stat).mockRejectedValueOnce(new Error('Disk I/O error'));
+
+    const result = await toolHandler({ path: testPath });
+
+    expect(result).toEqual({
+      isError: true,
+      content: [
+        {
+          type: 'text',
+          text: JSON.stringify({ errorMessage: 'Error reading file: Disk I/O error' }, null, 2),
+        },
+      ],
+    });
+  });
+
+  test('should handle non-Error objects in catch block', async () => {
+    const testPath = '/test/file.txt';
+    vi.mocked(path.isAbsolute).mockReturnValue(true);
+    vi.mocked(fs.access).mockResolvedValueOnce(undefined);
+    vi.mocked(fs.stat).mockRejectedValueOnce('string error');
+
+    const result = await toolHandler({ path: testPath });
+
+    expect(result).toEqual({
+      isError: true,
+      content: [
+        {
+          type: 'text',
+          text: JSON.stringify({ errorMessage: 'Error reading file: string error' }, null, 2),
+        },
+      ],
+    });
+  });
 });