Implement core MCP tools for file operation#24
Merged
Patrick-Ehimen merged 1 commit intomainfrom Oct 18, 2025
Merged
Conversation
Reviewer's GuideThis PR introduces two MCP tools for file upload and download—LighthouseUploadFileTool and LighthouseFetchFileTool—complete with reusable definitions, parameter validation, progress tracking, error handling, and integration into the MCP server registry; it refactors server registration to leverage these tool classes, adds shared types and exports, and updates documentation accordingly. Sequence diagram for MCP server tool registration (refactored)sequenceDiagram
participant Server as "LighthouseMCPServer"
participant Registry
participant UploadTool as "LighthouseUploadFileTool"
participant FetchTool as "LighthouseFetchFileTool"
Server->>UploadTool: new LighthouseUploadFileTool(service, logger)
Server->>FetchTool: new LighthouseFetchFileTool(service, logger)
Server->>Registry: register(UploadTool.getDefinition(), UploadTool.execute)
Server->>Registry: register(FetchTool.getDefinition(), FetchTool.execute)
Server->>Registry: register(datasetTool, datasetService.createDataset)
Registry->>Server: listTools()
Server->>Server: log toolCount, toolNames
Class diagram for new MCP file toolsclassDiagram
class LighthouseUploadFileTool {
- ILighthouseService service
- Logger logger
+ constructor(service: ILighthouseService, logger?: Logger)
+ static getDefinition(): MCPToolDefinition
- validateParams(params: UploadFileParams): Promise<string | null>
+ execute(args: Record<string, unknown>): Promise<ProgressAwareToolResult>
}
class LighthouseFetchFileTool {
- ILighthouseService service
- Logger logger
+ constructor(service: ILighthouseService, logger?: Logger)
+ static getDefinition(): MCPToolDefinition
- isValidCID(cid: string): boolean
- validateParams(params: FetchFileParams): Promise<string | null>
+ execute(args: Record<string, unknown>): Promise<ProgressAwareToolResult>
}
class ILighthouseService {
<<interface>>
+ uploadFile(params)
+ fetchFile(params)
+ getFileInfo(cid)
}
class Logger {
+ info(...)
+ warn(...)
+ error(...)
+ getInstance(...)
}
class ProgressAwareToolResult {
+ success: boolean
+ data?: unknown
+ error?: string
+ metadata?: Record<string, unknown>
+ operationId?: string
+ isRunning?: boolean
+ executionTime: number
}
LighthouseUploadFileTool --> ILighthouseService
LighthouseUploadFileTool --> Logger
LighthouseFetchFileTool --> ILighthouseService
LighthouseFetchFileTool --> Logger
LighthouseUploadFileTool ..> ProgressAwareToolResult
LighthouseFetchFileTool ..> ProgressAwareToolResult
ILighthouseService <|.. LighthouseUploadFileTool
ILighthouseService <|.. LighthouseFetchFileTool
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `apps/mcp-server/src/server.ts:138-147` </location>
<code_context>
+ if (datasetTool) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Silent fallback if dataset tool is missing may obscure configuration errors.
Consider adding a warning log when the tool is missing to improve visibility and make debugging configuration issues easier.
</issue_to_address>
### Comment 2
<location> `apps/mcp-server/src/server.ts:160` </location>
<code_context>
+
this.logger.info("All tools registered", {
- toolCount: LIGHTHOUSE_MCP_TOOLS.length,
+ toolCount: registeredTools.length,
+ toolNames: registeredTools.map((t) => t.name),
registrationTime,
</code_context>
<issue_to_address>
**suggestion:** Switching toolCount to registeredTools.length may not reflect intended tool coverage.
Reporting only registeredTools.length may misrepresent tool coverage if some tools fail to register. Consider including both the total available and registered tool counts, or update the log message for clarity.
</issue_to_address>
### Comment 3
<location> `apps/mcp-server/src/tools/LighthouseFetchFileTool.ts:72-81` </location>
<code_context>
+ private isValidCID(cid: string): boolean {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** CID validation logic may reject valid CIDs or allow invalid ones.
Consider using a library like 'multiformats/cid' for CID validation to ensure compatibility with all valid formats and reduce the risk of incorrect validation.
Suggested implementation:
```typescript
import { CID } from "multiformats/cid";
/**
* Validate CID format using multiformats/cid library
*/
private isValidCID(cid: string): boolean {
if (typeof cid !== "string" || cid.length === 0) return false;
try {
CID.parse(cid);
return true;
} catch {
return false;
}
}
```
Make sure to add `multiformats` to your dependencies if it is not already installed:
```
npm install multiformats
```
Remove any unused imports or code related to the old validation logic if present elsewhere in the file.
</issue_to_address>
### Comment 4
<location> `apps/mcp-server/src/tools/__tests__/LighthouseUploadFileTool.test.ts:179-192` </location>
<code_context>
+ expect(result.error).toContain("Path is not a file");
+ });
+
+ it("should fail when file is too large", async () => {
+ mockFs.stat.mockResolvedValue({
+ isFile: () => true,
+ size: 200 * 1024 * 1024, // 200MB
+ } as any);
+
+ const result = await tool.execute({
+ filePath: "/test/huge-file.txt",
+ });
+
+ expect(result.success).toBe(false);
+ expect(result.error).toContain("File too large");
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for a file exactly at the size limit (100MB).
Please add a test for a file with a size of exactly 100MB to verify correct handling of the boundary condition.
```suggestion
+ it("should succeed when file is exactly at the size limit (100MB)", async () => {
+ mockFs.stat.mockResolvedValue({
+ isFile: () => true,
+ size: 100 * 1024 * 1024, // 100MB
+ } as any);
+
+ const result = await tool.execute({
+ filePath: "/test/exact-limit-file.txt",
+ });
+
+ expect(result.success).toBe(true);
+ expect(result.error).toBeUndefined();
+ });
+
+ it("should fail when file is too large", async () => {
+ mockFs.stat.mockResolvedValue({
+ isFile: () => true,
+ size: 200 * 1024 * 1024, // 200MB
+ } as any);
+
+ const result = await tool.execute({
+ filePath: "/test/huge-file.txt",
+ });
+
+ expect(result.success).toBe(false);
+ expect(result.error).toContain("File too large");
+ });
+
```
</issue_to_address>
### Comment 5
<location> `apps/mcp-server/src/tools/__tests__/LighthouseUploadFileTool.test.ts:204-206` </location>
<code_context>
+ expect(result.error).toContain("encrypt must be a boolean");
+ });
+
+ it("should fail when access conditions are provided without encryption", async () => {
+ mockFs.stat.mockResolvedValue({
+ isFile: () => true,
+ size: 1024,
+ } as any);
+
+ const result = await tool.execute({
+ filePath: "/test/file.txt",
+ encrypt: false,
+ accessConditions: [
+ {
+ type: "token_balance",
+ condition: ">=",
+ value: "1000",
+ },
+ ],
+ });
+
+ expect(result.success).toBe(false);
+ expect(result.error).toContain("Access conditions require encryption");
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for accessConditions with encrypt set to undefined.
Please add a test case where accessConditions are set but encrypt is undefined, to verify that the validation logic enforces encryption as required.
```suggestion
expect(result.success).toBe(false);
expect(result.error).toContain("encrypt must be a boolean");
});
it("should fail when access conditions are provided and encrypt is undefined", async () => {
mockFs.stat.mockResolvedValue({
isFile: () => true,
size: 1024,
} as any);
const result = await tool.execute({
filePath: "/test/file.txt",
// encrypt is intentionally omitted (undefined)
accessConditions: [
{
type: "token_balance",
condition: ">=",
value: "1000",
},
],
});
expect(result.success).toBe(false);
expect(result.error).toContain("Access conditions require encryption");
});
```
</issue_to_address>
### Comment 6
<location> `apps/mcp-server/src/tools/__tests__/LighthouseFetchFileTool.test.ts:81-90` </location>
<code_context>
+ it("should download file successfully with minimal parameters", async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for outputPath pointing to a directory instead of a file.
Testing this scenario will help verify that the tool behaves as expected when given a directory path for outputPath.
</issue_to_address>
### Comment 7
<location> `apps/mcp-server/src/tools/__tests__/LighthouseFetchFileTool.test.ts:263-270` </location>
<code_context>
+ expect(result.error).toContain("Output file already exists");
+ });
+
+ it("should fail when decrypt is not boolean", async () => {
+ const result = await tool.execute({
+ cid: "QmTestCID123456789012345678901234567890123456",
+ decrypt: "yes",
+ });
+
+ expect(result.success).toBe(false);
+ expect(result.error).toContain("decrypt must be a boolean");
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for decrypt being undefined when downloading an encrypted file.
Add a test case where decrypt is undefined for an encrypted file to verify correct default behavior.
Suggested implementation:
```typescript
expect(result.success).toBe(false);
expect(result.error).toContain("Output file already exists");
});
it("should handle decrypt being undefined for an encrypted file", async () => {
// Mock the tool to simulate an encrypted file
vi.spyOn(tool, "isFileEncrypted").mockReturnValue(true);
const result = await tool.execute({
cid: "QmTestCID123456789012345678901234567890123456",
// decrypt is intentionally undefined
});
// Adjust the expectation below to match the intended default behavior.
// If the default is to decrypt, expect success and decrypted output.
// If the default is to throw an error, expect failure and error message.
// Example expectation for default decrypt:
expect(result.success).toBe(true);
expect(result.output).toContain("decrypted"); // Adjust as needed for your output
});
```
- You may need to adjust the mock for `isFileEncrypted` and the expectations to match your actual implementation and default behavior.
- If your tool uses a different method to determine encryption, update the mock accordingly.
- If the default behavior is to throw an error when `decrypt` is undefined, change the expectation to `expect(result.success).toBe(false)` and check for the appropriate error message.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+138
to
147
| if (datasetTool) { | ||
| this.registry.register(datasetTool, async (args) => { | ||
| const result = await this.datasetService.createDataset({ | ||
| name: args.name as string, | ||
| description: args.description as string | undefined, | ||
| files: args.files as string[], | ||
| metadata: args.metadata as Record<string, unknown> | undefined, | ||
| encrypt: args.encrypt as boolean | undefined, | ||
| }); | ||
|
|
There was a problem hiding this comment.
suggestion (bug_risk): Silent fallback if dataset tool is missing may obscure configuration errors.
Consider adding a warning log when the tool is missing to improve visibility and make debugging configuration issues easier.
|
|
||
| this.logger.info("All tools registered", { | ||
| toolCount: LIGHTHOUSE_MCP_TOOLS.length, | ||
| toolCount: registeredTools.length, |
There was a problem hiding this comment.
suggestion: Switching toolCount to registeredTools.length may not reflect intended tool coverage.
Reporting only registeredTools.length may misrepresent tool coverage if some tools fail to register. Consider including both the total available and registered tool counts, or update the log message for clarity.
Comment on lines
+72
to
+81
| private isValidCID(cid: string): boolean { | ||
| // Basic CID validation - should start with Qm (v0) or b (v1 base32) and have proper length | ||
| if (typeof cid !== "string" || cid.length === 0) return false; | ||
|
|
||
| // CID v0 (base58, starts with Qm, 46 characters) - strict validation | ||
| if (cid.startsWith("Qm") && cid.length === 46) { | ||
| return /^Qm[1-9A-HJ-NP-Za-km-z]{44}$/.test(cid); | ||
| } | ||
|
|
||
| // CID v1 (multibase, various encodings) |
There was a problem hiding this comment.
suggestion (bug_risk): CID validation logic may reject valid CIDs or allow invalid ones.
Consider using a library like 'multiformats/cid' for CID validation to ensure compatibility with all valid formats and reduce the risk of incorrect validation.
Suggested implementation:
import { CID } from "multiformats/cid";
/**
* Validate CID format using multiformats/cid library
*/
private isValidCID(cid: string): boolean {
if (typeof cid !== "string" || cid.length === 0) return false;
try {
CID.parse(cid);
return true;
} catch {
return false;
}
}Make sure to add multiformats to your dependencies if it is not already installed:
npm install multiformats
Remove any unused imports or code related to the old validation logic if present elsewhere in the file.
Comment on lines
+204
to
+206
| expect(result.success).toBe(false); | ||
| expect(result.error).toContain("encrypt must be a boolean"); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Add a test for accessConditions with encrypt set to undefined.
Please add a test case where accessConditions are set but encrypt is undefined, to verify that the validation logic enforces encryption as required.
Suggested change
| expect(result.success).toBe(false); | |
| expect(result.error).toContain("encrypt must be a boolean"); | |
| }); | |
| expect(result.success).toBe(false); | |
| expect(result.error).toContain("encrypt must be a boolean"); | |
| }); | |
| it("should fail when access conditions are provided and encrypt is undefined", async () => { | |
| mockFs.stat.mockResolvedValue({ | |
| isFile: () => true, | |
| size: 1024, | |
| } as any); | |
| const result = await tool.execute({ | |
| filePath: "/test/file.txt", | |
| // encrypt is intentionally omitted (undefined) | |
| accessConditions: [ | |
| { | |
| type: "token_balance", | |
| condition: ">=", | |
| value: "1000", | |
| }, | |
| ], | |
| }); | |
| expect(result.success).toBe(false); | |
| expect(result.error).toContain("Access conditions require encryption"); | |
| }); |
Comment on lines
+81
to
+90
| it("should download file successfully with minimal parameters", async () => { | ||
| const mockResult: DownloadResult = { | ||
| filePath: "./downloaded_QmTestCID123456789012345678901234567890123456", | ||
| cid: "QmTestCID123456789012345678901234567890123456", | ||
| size: 1024, | ||
| decrypted: false, | ||
| downloadedAt: new Date("2023-01-01T00:00:00.000Z"), | ||
| hash: "QmTestCID123456789012345678901234567890123456", | ||
| }; | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for outputPath pointing to a directory instead of a file.
Testing this scenario will help verify that the tool behaves as expected when given a directory path for outputPath.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implemented core MCP tools for file upload and download operations, integrating with the unified SDK wrapper and providing comprehensive parameter validation, progress tracking, and error handling.
Features Implemented
🔧 lighthouse_upload_file Tool
📥 lighthouse_fetch_file Tool
🧪 Comprehensive Testing
Requirements Fulfilled
✅ MCP tools integrated with unified SDK wrapper
✅ File download functionality implemented
✅ SDK wrapper integration for actual Lighthouse operations
✅ Upload file tool with parameter validation
✅ Download functionality with streaming support
Testing Results
Usage Examples
Upload File
{ "method": "tools/call", "params": { "name": "lighthouse_upload_file", "arguments": { "filePath": "/path/to/file.txt", "encrypt": true, "tags": ["document", "important"] } } }Download File
{ "method": "tools/call", "params": { "name": "lighthouse_fetch_file", "arguments": { "cid": "QmXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", "outputPath": "./downloads/file.txt", "decrypt": true } } }Checklist
Summary by Sourcery
Implement and integrate core MCP tools for file upload and download operations using the unified LighthouseService wrapper, adding comprehensive parameter validation, progress tracking, and error handling.
New Features:
Enhancements:
Documentation:
Tests: