Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,6 @@ jobs:
- name: Run tests with coverage
run: bun run test:coverage

# - name: Prepare coverage report in markdown
# if: github.event_name == 'pull_request'
# id: coverage
# uses: fingerprintjs/action-coverage-report-md@v2
# with:
# textReportPath: './coverage.txt'
# srcBasePath: './src'

# - name: Comment coverage report on PR
# if: github.event_name == 'pull_request'
# uses: marocchino/sticky-pull-request-comment@v2
# with:
# message: ${{ steps.coverage.outputs.markdownReport }}
# header: Coverage Report

- name: Upload coverage to Codecov
uses: codecov/codecov-action@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "mcpland",
"version": "0.3.0",
"version": "0.3.1",
"private": false,
"type": "module",
"description": "Building blocks for implementing Model Context Protocol tools.",
Expand Down
81 changes: 54 additions & 27 deletions src/core/mcp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,47 @@ export abstract class McpTool {
return fileText;
}

/**
* Check if a file is likely binary by examining its content
*/
private async isBinaryFile(filePath: string): Promise<boolean> {
const { openSync, readSync, closeSync } = await import('node:fs');
try {
// Read first 8KB only
const fd = openSync(filePath, 'r');
const buffer = Buffer.allocUnsafe(8192);
const bytesRead = readSync(fd, buffer, 0, 8192, 0);
closeSync(fd);
const chunk = buffer.subarray(0, bytesRead);

Comment on lines +196 to +205

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

-	private async isBinaryFile(filePath: string): Promise<boolean> {
-		const { openSync, readSync, closeSync } = await import('node:fs');
-		try {
-			// Read first 8KB only
-			const fd = openSync(filePath, 'r');
-			const buffer = Buffer.allocUnsafe(8192);
-			const bytesRead = readSync(fd, buffer, 0, 8192, 0);
-			closeSync(fd);
-			const chunk = buffer.subarray(0, bytesRead);
+	private async isBinaryFile(filePath: string): Promise<boolean> {
+		const { openSync, readSync, closeSync } = await import('node:fs');
+		let fd: number | undefined;
+		try {
+			// Read first 8KB only
+			fd = openSync(filePath, 'r');
+			const buffer = Buffer.allocUnsafe(8192);
+			const bytesRead = readSync(fd, buffer, 0, 8192, 0);
+			const chunk = buffer.subarray(0, bytesRead);
@@
-		} catch {
-			// If we can't read the file, assume it's binary to be safe
-			return true;
-		}
+		} catch {
+			// If we can't read the file, assume it's binary to be safe
+			return true;
+		} finally {
+			if (fd !== undefined) {
+				try { closeSync(fd); } catch {}
+			}
+		}

Also applies to: 228-231

🤖 Prompt for AI Agents
In src/core/mcp.ts around lines 196 to 205 the file descriptor opened with
openSync may not be closed if readSync throws; change the function to ensure
closeSync is always called by moving closeSync into a finally block (track the
fd variable outside try, check fd !== undefined before closing) so the
descriptor is closed on success and on error. Apply the same pattern to the
related code at lines 228-231 to guard against fd leaks.

// Check for null bytes (common in binary files)
if (chunk.indexOf(0) !== -1) {
return true;
}

// Check ratio of non-printable characters
let nonPrintableCount = 0;
for (let i = 0; i < chunk.length; i++) {
const byte = chunk[i];
// Consider bytes outside printable ASCII range (excluding common whitespace)
/* c8 ignore start */
if (byte < 32 && byte !== 9 && byte !== 10 && byte !== 13) {
nonPrintableCount++;
} else if (byte > 126) {
nonPrintableCount++;
}
/* c8 ignore stop */
}

// If more than 30% of characters are non-printable, consider it binary
const nonPrintableRatio = nonPrintableCount / chunk.length;
return nonPrintableRatio > 0.3;
} catch {
// If we can't read the file, assume it's binary to be safe
return true;
}
}
Comment thread
stewones marked this conversation as resolved.

protected async fetchFromDirectory(): Promise<string> {
// Build context from contextDir (recursive text files) when provided
let docsText: string = '';
Expand All @@ -198,25 +239,9 @@ export abstract class McpTool {
const dirToRead = `${baseDir}/${this.spec.contextDir}`;
const { readdirSync, statSync, readFileSync } = await import('node:fs');
const pathMod = await import('node:path');
const TEXT_EXTS = new Set([
'.txt',
'.md',
'.mdx',
'.markdown',
'.json',
'.yml',
'.yaml',
'.ini',
'.cfg',
'.conf',
'.toml',
'.csv',
'.tsv',
'.html',
'.htm',
]);

const files: string[] = [];
const walk = (dir: string) => {
const walk = async (dir: string) => {
let entries: string[] = [];
try {
entries = readdirSync(dir);
Expand All @@ -227,25 +252,27 @@ export abstract class McpTool {
const full = pathMod.join(dir, entry);
try {
const st = statSync(full);
if (st.isDirectory()) walk(full);
else if (st.isFile()) {
const ext = pathMod.extname(entry).toLowerCase();
if (TEXT_EXTS.has(ext)) files.push(full);
if (st.isDirectory()) {
await walk(full);
} else if (st.isFile()) {
// Skip files that are likely binary
const isBinary = await this.isBinaryFile(full, readFileSync);
if (!isBinary) {
files.push(full);
}
}
// eslint-disable-next-line no-empty
} catch {}
}
};
walk(dirToRead);
await walk(dirToRead);
const pieces: string[] = [];
for (const f of files) {
try {
const rel = pathMod.relative(dirToRead, f);
const content = readFileSync(f, 'utf-8');
pieces.push(`=== ${rel} ===\n\n${content}`);
} catch {
// ignore read errors
}
} /* c8 ignore next - file might be binary despite our check */ catch {}
}
docsText = pieces.join('\n\n');
}
Expand Down Expand Up @@ -309,7 +336,7 @@ export abstract class McpTool {
)
.join('\n\n');

const serverResult:ServerResult = {
const serverResult: ServerResult = {
content: [
{
type: 'text',
Expand Down
50 changes: 44 additions & 6 deletions test/src/core/mcp.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,18 +113,51 @@ 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('/');
}
};

vi.doMock('node:fs', () => ({
readdirSync: readdirMock,
statSync: statMock,
readFileSync: readFileMock,
}));

vi.doMock('node:path', () => pathMock);

class DirTool extends McpTool {
constructor() {
super({
Expand Down Expand Up @@ -156,15 +189,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 () => {
Expand Down
Loading