Conversation
This commit fixes a bug in the `getVMs` function where the node for LXC containers was not being correctly assigned. The code was previously using `vm.node || node.node`, but `vm.node` is not a valid property for LXC containers returned from the Proxmox API. This resulted in the node being incorrectly assigned. The fix changes the code to use `node.node` directly, which correctly assigns the node from the parent loop. Additionally, this commit introduces a new test file to verify the fix and prevent regressions. The test mocks the Proxmox API and asserts that the node is correctly assigned to the LXC container.
This commit migrates the entire project from JavaScript to TypeScript. This includes: - Setting up a TypeScript environment with a `tsconfig.json` file. - Creating a `src` directory with the new TypeScript source code. - Migrating all the core components and tools to TypeScript. - Adding type definitions for the Proxmox API responses. - Writing new tests for the migrated code using Jest and `ts-jest`. - Updating the `package.json` file with the necessary dependencies and scripts for building and testing the application. The new TypeScript codebase is more maintainable and less prone to errors due to the static type checking provided by TypeScript.
WalkthroughIntroduces a TypeScript-based Proxmox MCP server with tool modules (nodes, VMs, storage, cluster), a Proxmox API manager, Jest tests, and TypeScript configs. Adjusts package scripts and dependencies, adds Jest config, and TypeScript compiler settings. Updates ignore rules and exports a class in index.js. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server as MCP Server
participant Tools as Tool Registries<br/>(Node/VM/Storage/Cluster)
participant PM as ProxmoxManager
note over Server,Tools: New: MCP routing across tool registries
Client->>Server: ListTools
Server->>Tools: Aggregate listTools()
Tools-->>Server: Tool definitions
Server-->>Client: Combined tool list
Client->>Server: CallTool(name, args)
alt Tool found in Node/VM/Storage/Cluster
Server->>Tools: getTool(name)
Tools-->>Server: toolFn
Server->>toolFn: invoke(args)
toolFn->>PM: proxmoxRequest(endpoint, method?, body?)
PM-->>toolFn: data
toolFn-->>Server: content[text JSON]
Server-->>Client: Result
else Unknown tool
Server-->>Client: Error content
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
.gitignore (1)
1-1: Consider adding common Node.js/TypeScript ignore patterns.The current
.gitignoreonly excludesnode_modules, but TypeScript projects typically benefit from ignoring additional artifacts and sensitive files.Apply this diff to add recommended patterns:
node_modules +dist/ +*.log +.env +.DS_Store +*.swp +.vscode/ +.idea/Note: If you intend to commit
dist/for deployment purposes, keep the current configuration and ignore this suggestion.src/core/proxmox.ts (1)
34-38: Consider using a more specific type for fetch options.Line 34 uses
anytype for the options object. For better type safety, consider using node-fetch'sRequestInittype.Apply this diff:
+import fetch, { RequestInit } from 'node-fetch'; import https from 'https'; import { ProxmoxConfig, AuthConfig } from '../types/index.js';- const options: any = { + const options: RequestInit = { method, headers, agent: this.httpsAgenttests/node.test.ts (1)
1-42: LGTM!The test suite follows good patterns with proper setup, mocking, and assertions. The tests cover the main success paths for node tooling.
Consider adding tests for error scenarios:
- What happens when proxmoxRequest fails?
- What happens when invalid node names are provided?
These would improve resilience verification but are not essential given the current implementation's simplicity.
tests/storage.test.ts (1)
1-37: LGTM!The test correctly verifies storage aggregation across nodes. The mock sequence and assertions align with the implementation in
src/tools/storage.ts.Consider adding a test for the single-node filter path:
const result = await getStorage({ node: 'pve1' });This would ensure both code paths are covered.
src/server.ts (1)
43-67: LGTM!The sequential tool lookup pattern is straightforward and appropriate for the current number of tool providers.
For better performance with many tool providers, consider using a Map for O(1) lookup:
private setupToolHandlers(): void { const nodeTools = getNodeTools(this.proxmoxManager); const vmTools = getVmTools(this.proxmoxManager); const storageTools = getStorageTools(this.proxmoxManager); const clusterTools = getClusterTools(this.proxmoxManager); const toolProviders = [nodeTools, vmTools, storageTools, clusterTools]; this.server.setRequestHandler(CallToolRequestSchema, async (request) => { const { name, arguments: args } = request.params; try { for (const provider of toolProviders) { const tool = provider.getTool(name); if (tool) { return await tool(args as any); } } throw new Error(`Unknown tool: ${name}`); } catch (error: any) { return { content: [{ type: 'text', text: `Error: ${error.message}` }] }; } }); }This eliminates code duplication and makes adding new providers easier.
src/tools/vm.ts (1)
92-108: LGTM!The
getVmStatusandexecuteVmCommandfunctions correctly construct API endpoints using the type parameter and handle both qemu and lxc VM types.Consider adding stronger typing for the
argsparameters:interface VmStatusArgs { node: string; vmid: string; type?: 'qemu' | 'lxc'; } interface ExecuteVmCommandArgs { node: string; vmid: string; command: string; type?: 'qemu' | 'lxc'; } const getVmStatus = async (args: VmStatusArgs) => { /* ... */ }; const executeVmCommand = async (args: ExecuteVmCommandArgs) => { /* ... */ };This provides better IDE support and catches type errors at compile time, but it's not essential for the current implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
dist/core/proxmox.jsis excluded by!**/dist/**dist/server.jsis excluded by!**/dist/**dist/tools/cluster.jsis excluded by!**/dist/**dist/tools/node.jsis excluded by!**/dist/**dist/tools/storage.jsis excluded by!**/dist/**dist/tools/vm.jsis excluded by!**/dist/**dist/types/index.jsis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
.gitignore(1 hunks)index.js(2 hunks)jest.config.cjs(1 hunks)package.json(1 hunks)src/core/proxmox.ts(1 hunks)src/server.ts(1 hunks)src/tools/cluster.ts(1 hunks)src/tools/node.ts(1 hunks)src/tools/storage.ts(1 hunks)src/tools/vm.ts(1 hunks)src/types/index.ts(1 hunks)tests/cluster.test.ts(1 hunks)tests/node.test.ts(1 hunks)tests/storage.test.ts(1 hunks)tests/vm.test.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
tests/storage.test.ts (3)
src/core/proxmox.ts (2)
ProxmoxManager(5-66)proxmoxRequest(25-65)src/types/index.ts (2)
ProxmoxConfig(1-5)AuthConfig(7-11)src/tools/storage.ts (1)
getStorageTools(3-51)
tests/cluster.test.ts (3)
src/core/proxmox.ts (2)
ProxmoxManager(5-66)proxmoxRequest(25-65)src/types/index.ts (2)
ProxmoxConfig(1-5)AuthConfig(7-11)src/tools/cluster.ts (1)
getClusterTools(3-37)
src/core/proxmox.ts (2)
dist/server.js (1)
config(74-81)src/types/index.ts (2)
ProxmoxConfig(1-5)AuthConfig(7-11)
src/tools/cluster.ts (1)
src/core/proxmox.ts (1)
ProxmoxManager(5-66)
tests/node.test.ts (3)
src/core/proxmox.ts (2)
ProxmoxManager(5-66)proxmoxRequest(25-65)src/types/index.ts (2)
ProxmoxConfig(1-5)AuthConfig(7-11)src/tools/node.ts (1)
getNodeTools(3-57)
src/tools/vm.ts (1)
src/core/proxmox.ts (1)
ProxmoxManager(5-66)
src/tools/node.ts (1)
src/core/proxmox.ts (1)
ProxmoxManager(5-66)
src/tools/storage.ts (1)
src/core/proxmox.ts (1)
ProxmoxManager(5-66)
src/server.ts (6)
src/core/proxmox.ts (1)
ProxmoxManager(5-66)src/types/index.ts (2)
ProxmoxConfig(1-5)AuthConfig(7-11)src/tools/node.ts (1)
getNodeTools(3-57)src/tools/vm.ts (1)
getVmTools(3-114)src/tools/storage.ts (1)
getStorageTools(3-51)src/tools/cluster.ts (1)
getClusterTools(3-37)
tests/vm.test.ts (3)
src/core/proxmox.ts (2)
ProxmoxManager(5-66)proxmoxRequest(25-65)src/types/index.ts (2)
ProxmoxConfig(1-5)AuthConfig(7-11)src/tools/vm.ts (1)
getVmTools(3-114)
🔇 Additional comments (8)
index.js (1)
276-334: LGTM! The LXC node assignment fix is correct.Line 300 now consistently uses
node.nodefor LXC VMs, matching the QEMU pattern at line 295. This ensures that when aggregating VMs without a node filter, both QEMU and LXC VMs report the node they were actually fetched from, eliminating potential inconsistencies.package.json (1)
16-16: Verify the node-fetch version downgrade is intentional.The package was downgraded from
^3.3.2to^2.7.0. Since the project uses"type": "module"(ESM), and node-fetch v3 is ESM-native, this downgrade may be unnecessary unless there's a specific compatibility requirement.Based on learnings
Please confirm:
- Is there a specific reason for using v2 instead of v3?
- Are there compatibility concerns with dependencies or tooling?
If no specific reason exists, consider upgrading back to v3 for better ESM support:
- "node-fetch": "^2.7.0" + "node-fetch": "^3.3.2"src/tools/cluster.ts (1)
1-37: LGTM!The cluster tooling implementation follows the established factory pattern consistently. The code is clean and straightforward.
jest.config.cjs (1)
1-13: LGTM!The Jest configuration is appropriate for a TypeScript project:
- Uses
ts-jestpreset for TypeScript compilationmoduleNameMappercorrectly handles.jsextension mapping (TypeScript outputs.jsin imports while source files are.ts)transformIgnorePatternsallowsnode-fetchto be transformed (necessary for ESM/CommonJS interop)Based on learnings: ts-jest 29.4.5 aligns well with Jest 29 and modern TypeScript.
tests/vm.test.ts (1)
1-57: LGTM!The test suite comprehensively covers VM tooling operations:
- VM listing with proper node and type annotation
- VM status retrieval
- Command execution via agent
The mock sequence correctly simulates the multi-node, multi-type VM aggregation logic.
src/server.ts (1)
88-96: Verify environment variable handling.The server reads configuration from environment variables with sensible defaults. However, ensure that
PROXMOX_TOKEN_VALUEis never logged or exposed in error messages, as it's a sensitive credential.Consider adding validation to fail fast if required credentials are missing:
const config: ProxmoxConfig & AuthConfig = { host: process.env.PROXMOX_HOST || 'localhost', port: parseInt(process.env.PROXMOX_PORT || '8006', 10), verify_ssl: process.env.PROXMOX_VERIFY_SSL === 'true', user: process.env.PROXMOX_USER || 'root@pam', token_name: process.env.PROXMOX_TOKEN_NAME || 'mcp-server', token_value: process.env.PROXMOX_TOKEN_VALUE || '', }; if (!config.token_value) { throw new Error('PROXMOX_TOKEN_VALUE environment variable is required'); }This prevents silent failures when credentials are misconfigured.
src/tools/node.ts (1)
1-57: LGTM!The node tooling implementation is clean and follows the established factory pattern. The tool definitions include clear descriptions and proper input schemas.
src/tools/vm.ts (1)
60-90: LGTM! The LXC node assignment is now correct.The
getVmsfunction properly assigns thenodefield to both qemu and lxc VMs in both single-node and multi-node paths:
- Single-node path (lines 64-72): Uses
args.nodefor both VM types ✓- Multi-node path (lines 73-85): Uses
node.nodefrom the loop variable for both VM types ✓This fix (indicated by the PR title) ensures LXC containers receive the correct node assignment when querying across all nodes.
1 participant
Summary by CodeRabbit