-
Notifications
You must be signed in to change notification settings - Fork 1
add binary file detection #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ coverage.txt | |
| .env.production.local | ||
|
|
||
| # Data | ||
| .sqlite | ||
| *.sqlite | ||
|
|
||
| # Others | ||
| TODO.md | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,18 +113,87 @@ describe('McpTool base class', () => { | |
| const isDirectory = () => /(^|\/)docs(\/)?$/.test(p) || /\/docs\/sub$/.test(p); | ||
| return { isFile, isDirectory } as any; | ||
| }); | ||
| const readFileMock = vi.fn((p: string) => { | ||
| if (/a\.md$/.test(p)) return 'Content A'; | ||
| if (/b\.txt$/.test(p)) return 'Content B'; | ||
| throw new Error('should not read binaries'); | ||
| const readFileMock = vi.fn((p: string, options?: any) => { | ||
| // Handle binary detection reads (with encoding: null) | ||
| if (options && options.encoding === null) { | ||
| if (/img\.png$/.test(p)) { | ||
| // Return a buffer with null bytes to simulate binary content | ||
| return Buffer.from([0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, 0x00]); // PNG header with null byte | ||
| } | ||
| if (/bin\.bin$/.test(p)) { | ||
| // Return a buffer with null bytes to simulate binary content | ||
| return Buffer.from([0x00, 0x01, 0x02, 0x03]); | ||
| } | ||
| // For text files during binary detection | ||
| if (/a\.md$/.test(p)) return Buffer.from('Content A', 'utf-8'); | ||
| if (/b\.txt$/.test(p)) return Buffer.from('Content B', 'utf-8'); | ||
| } else { | ||
| // Handle regular text reads | ||
| if (/a\.md$/.test(p)) return 'Content A'; | ||
| if (/b\.txt$/.test(p)) return 'Content B'; | ||
| } | ||
| throw new Error('should not read this file: ' + p); | ||
| }); | ||
|
|
||
| // Mock path module as well | ||
| const pathMock = { | ||
| join: (...parts: string[]) => parts.join('/'), | ||
| relative: (from: string, to: string) => { | ||
| // Simplified relative path calculation for test | ||
| if (to.includes('a.md')) return 'a.md'; | ||
| if (to.includes('b.txt')) return 'sub/b.txt'; | ||
| return 'unknown'; | ||
| }, | ||
| dirname: (path: string) => { | ||
| const parts = path.split('/'); | ||
| return parts.slice(0, -1).join('/'); | ||
| } | ||
| }; | ||
|
|
||
| // Mock the new fs methods used by isBinaryFile | ||
| const openSyncMock = vi.fn((path: string) => { | ||
| // Return a fake file descriptor | ||
| return 123; | ||
| }); | ||
| const readSyncMock = vi.fn((fd: number, buffer: Buffer, offset: number, length: number, position: number) => { | ||
| // Simulate reading file content into buffer based on the current path being tested | ||
| // We need to track the path, so we'll use a closure to remember the last opened path | ||
| const mockPath = openSyncMock.mock.calls[openSyncMock.mock.calls.length - 1]?.[0] || ''; | ||
|
|
||
| if (fd === 123) { // Our fake fd | ||
| let content: Buffer; | ||
| if (/img\.png$/.test(mockPath)) { | ||
| content = Buffer.from([0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, 0x00]); // PNG with null byte | ||
| } else if (/bin\.bin$/.test(mockPath)) { | ||
| content = Buffer.from([0x00, 0x01, 0x02, 0x03]); // Binary with null bytes | ||
| } else if (/a\.md$/.test(mockPath)) { | ||
| content = Buffer.from('Content A', 'utf-8'); | ||
| } else if (/b\.txt$/.test(mockPath)) { | ||
| content = Buffer.from('Content B', 'utf-8'); | ||
| } else { | ||
| content = Buffer.from('Default content', 'utf-8'); | ||
| } | ||
| const bytesToCopy = Math.min(content.length, length); | ||
| content.copy(buffer, offset, 0, bytesToCopy); | ||
| return bytesToCopy; | ||
| } | ||
| return 0; | ||
| }); | ||
| const closeSyncMock = vi.fn(() => { | ||
| // No-op for closing file descriptor | ||
| }); | ||
|
|
||
| vi.doMock('node:fs', () => ({ | ||
| readdirSync: readdirMock, | ||
| statSync: statMock, | ||
| readFileSync: readFileMock, | ||
| openSync: openSyncMock, | ||
| readSync: readSyncMock, | ||
| closeSync: closeSyncMock, | ||
| })); | ||
|
|
||
| vi.doMock('node:path', () => pathMock); | ||
|
|
||
|
Comment on lines
186
to
+196
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doMock after importing McpTool won’t affect it — reset modules and re-import These tests mock node:fs/node:path with vi.doMock, but McpTool was imported earlier at file top. Since doMock isn’t hoisted, the mocks won’t be applied to the already-loaded module, causing flakiness and leaking real FS. Reset and re-import the SUT, then define classes against that import. Minimal fix inside each test after doMock: vi.resetModules();
const { McpTool: LocalMcpTool } = await import('../../../src/core/mcp');
// then: class DirTool extends LocalMcpTool { ... }Also applies to: 250-255, 289-294, 328-333 🤖 Prompt for AI Agents |
||
| class DirTool extends McpTool { | ||
| constructor() { | ||
| super({ | ||
|
|
@@ -156,15 +225,20 @@ describe('McpTool base class', () => { | |
| expect(arg).toContain('=== sub/b.txt ==='); | ||
| expect(arg).toContain('Content B'); | ||
|
|
||
| // Ensure binaries were not read | ||
| expect(readFileMock).toHaveBeenCalledTimes(2); | ||
| // Ensure binaries were detected and skipped (not read for text content) | ||
| // The readFileMock will be called for binary detection (with encoding: null) and text reads | ||
| expect(readFileMock).toHaveBeenCalled(); | ||
|
|
||
| // Ingestion should include dir meta | ||
| expect(ingestSpy).toHaveBeenCalledWith( | ||
| { id: 'source-1', meta: { name: 'dirtool', url: undefined, file: undefined, dir: 'docs' } }, | ||
| ['c1', 'c2'], | ||
| { mcpId: 'foo', toolId: 'dirtool' } | ||
| ); | ||
|
|
||
| // Clean up mocks | ||
| vi.doUnmock('node:fs'); | ||
| vi.doUnmock('node:path'); | ||
| }); | ||
|
|
||
| it('contextDir handles readdirSync errors gracefully', async () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close the file descriptor on all paths (use finally)
If readSync throws, closeSync isn’t reached, leaking the fd. Wrap closeSync in a finally and guard it.
Also applies to: 228-231
🤖 Prompt for AI Agents