Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an NPM distribution mechanism for the cicd-mcp-server Go binary using a meta-package and platform-specific sub-packages. The feedback highlights several critical improvements for the installation scripts: addressing binary naming mismatches that would cause failures on Windows, ensuring target directories are created before file operations, and utilizing require.resolve for more robust path resolution of dependencies. Additionally, a correction was suggested for the npx command in the MCP configuration to match the defined binary name.
| "url": "git+https://github.com/gemini-cli-extensions/cicd.git" | ||
| }, | ||
| "bin": { | ||
| "cicd-mcp-server": "bin/cicd-mcp-server" |
There was a problem hiding this comment.
There is a mismatch for Windows users here. The postinstall.js script creates bin/cicd-mcp-server.exe on Windows, but this bin field points to bin/cicd-mcp-server. NPM will fail to create the necessary shims on Windows because the source file name doesn't match.
Additionally, to support npx @google-cloud/cicd-mcp directly, the key should be cicd-mcp (matching the package name suffix).
Recommendation: Use a small JavaScript wrapper as the entry point in bin/. This wrapper can detect the platform and spawn the correct binary from the optional dependency at runtime. This is the industry standard (used by esbuild, swc, etc.) and avoids the fragility of postinstall scripts (which can be skipped with --ignore-scripts).
| "cicd-mcp-server": "bin/cicd-mcp-server" | |
| "cicd-mcp": "bin/cicd-mcp-server" |
| { | ||
| "mcpServers": { | ||
| "google-cicd": { | ||
| "command": "npx @google-cloud/cicd-mcp" |
There was a problem hiding this comment.
The command npx @google-cloud/cicd-mcp will attempt to execute a binary named cicd-mcp (matching the package name suffix). However, the package.json for this package defines the binary as cicd-mcp-server. This will result in a 'command not found' error. You should either update this command to specify the binary name or change the binary key in package.json to cicd-mcp.
| "command": "npx @google-cloud/cicd-mcp" | |
| "command": "npx @google-cloud/cicd-mcp cicd-mcp-server" |
| // In a typical install, the sub-package will be in the same node_modules/@google-cloud directory | ||
| // __dirname is node_modules/@google-cloud/cicd-mcp/scripts | ||
| // We go up 3 levels to reach node_modules | ||
| const sourcePath = path.join(__dirname, '..', '..', '..', subPackage, 'bin', binaryName); |
There was a problem hiding this comment.
Using relative path traversal (../../../) to locate optional dependencies is fragile and depends on a flat node_modules structure. It may fail with certain package managers (like pnpm) or nested installations. Using require.resolve is a more robust way to find the installation path of the sub-packages.
| const sourcePath = path.join(__dirname, '..', '..', '..', subPackage, 'bin', binaryName); | |
| const sourcePath = path.join(path.dirname(require.resolve(`${subPackage}/package.json`)), 'bin', binaryName); |
| try { | ||
| // Check if source exists | ||
| if (!fs.existsSync(sourcePath)) { | ||
| console.error(`Source binary not found at ${sourcePath}`); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
The bin directory might not exist in the published package since it is excluded by .gitignore. You should ensure the target directory exists before attempting to create a link or copy the binary into it.
| try { | |
| // Check if source exists | |
| if (!fs.existsSync(sourcePath)) { | |
| console.error(`Source binary not found at ${sourcePath}`); | |
| process.exit(1); | |
| } | |
| try { | |
| const targetDir = path.dirname(targetPath); | |
| if (!fs.existsSync(targetDir)) { | |
| fs.mkdirSync(targetDir, { recursive: true }); | |
| } | |
| // Check if source exists | |
| if (!fs.existsSync(sourcePath)) { | |
| console.error(`Source binary not found at ${sourcePath}`); | |
| process.exit(1); | |
| } |
Added platform specific NPM packages. This will simplify distributing the
cicd-mcp-serverGo binary.