Re-implement outdated pull request

[Dhravya]

Re-implements the --env feature from PR #30 to allow users to pass environment variables when installing MCP servers.

Changes:

• Add --env option to CLI that accepts key-value pairs
• Add parseEnvVars() function to convert array format to object
• Update setServerConfig() to properly handle env vars for different clients:
  • Goose: uses 'envs' field instead of 'env'
  • Zed: uses 'env' field
  • All other clients: uses 'env' field
• Apply env vars to URL-based and command-based installations
• Update Warp config output to include env vars

Usage:
npx install-mcp --client --env KEY1 value1 --env KEY2 value2

Closes #30

[claude]

Claude finished @Dhravya's task —— View job

---

Code Review Complete

I've completed a thorough review of PR #34's implementation of the --env feature. Here's my feedback:

Todo List

• Read the modified file to understand the implementation
• Analyze code quality and best practices
• Check for potential bugs and security concerns
• Assess performance considerations
• Review test coverage needs
• Provide comprehensive feedback

✅ Strengths

Good Implementation Choices:

• Clean separation of parseEnvVars() function with clear responsibility
• Proper client-specific handling (Goose uses envs, others use env)
• Consistent application to both URL-based and command-based installations
• Type safety with InstallArgv interface extension

Code Quality:

• Clear and descriptive function naming
• Good error handling for edge cases (undefined/empty arrays)
• Follows existing code patterns and conventions

⚠️ Areas for Improvement

1. Bug Risk in Environment Variable Parsing src/commands/install.ts:219

The current parsing logic assumes pairs but doesn't validate:

for (let i = 0; i < envArray.length; i += 2) {
  const key = envArray[i]
  const value = envArray[i + 1] // Could be undefined for odd-length arrays
  if (key && value !== undefined) {
    envObj[key] = value
  }
}

Issue: If users provide an odd number of arguments (--env KEY1 value1 --env KEY2), the last key gets silently ignored.
Recommendation: Add validation to warn about incomplete pairs.

2. Missing Environment Variable Validation

envObj[key] = value // No validation of key format

Security Concern: No validation that keys are valid environment variable names (should match /^[A-Za-z_][A-Za-z0-9_]*$/).
Recommendation: Add key format validation to prevent issues.

3. OpenCode Client Missing Environment Variables src/commands/install.ts:82-90

The OpenCode client branch doesn't handle environment variables:

} else {
  // For local MCP servers
  servers[serverName] = {
    type: 'local',
    command: serverConfig.command,
    args: serverConfig.args || [],
    enabled: true,
    environment: {}, // Hard-coded empty object, ignores env vars
  }
}

Bug: Environment variables are lost for OpenCode local servers.
Fix: Should be environment: serverConfig.env || {}

4. Inconsistent Property Names

Different clients use different property names for environment variables:

• Goose: envs
• OpenCode: environment
• Others: env

This creates potential confusion and maintenance burden.

📋 Performance Considerations

Acceptable Performance: The implementation is efficient with O(n) parsing and no performance concerns for typical CLI usage.

🧪 Test Coverage Assessment

Critical Gap: No test files found in the repository (**/*.test.*, **/test/** directories missing).

Recommended Test Cases:

1. Valid env var parsing: ['KEY1', 'value1', 'KEY2', 'value2'] → {KEY1: 'value1', KEY2: 'value2'}
2. Odd number handling: ['KEY1', 'value1', 'KEY2']
3. Empty array handling: [] → undefined
4. Client-specific config generation for each client type
5. Integration test with actual config file writing

🔒 Security Review

Low Risk: Environment variables are handled safely without shell injection risks. The values are stored as-is without interpretation.

📝 Recommendations

1. Immediate Fixes Needed:

   • Fix OpenCode environment variable handling
   • Add environment variable key validation
   • Improve error handling for malformed env pairs

2. Future Enhancements:

   • Add comprehensive test suite
   • Consider standardizing env property names across clients
   • Add CLI validation feedback for malformed --env arguments

Overall Assessment

Verdict: Good implementation that successfully adds the requested feature, but has one critical bug (OpenCode) and several areas for robustness improvements. The core functionality works correctly for most clients.

---

[sentry]

Bug: The OpenCode client configuration handler silently discards environment variables from serverConfig.env, hardcoding an empty environment object for local servers and omitting it for remote ones.
_{Severity: HIGH | Confidence: High}

🔍 Detailed Analysis

When installing a server for the OpenCode client using the --env flag, the provided environment variables are silently ignored. For local servers, the environment property is hardcoded to an empty object ({}). For remote servers, the property is omitted entirely. This behavior is inconsistent with other clients like Zed and Warp, which correctly apply the environment variables from serverConfig.env. This leads to a silent failure where users believe their configuration is applied, but it is not, potentially causing misconfigured server setups.

💡 Suggested Fix

In the OpenCode handler for local servers, replace the hardcoded environment: {} with environment: serverConfig.env || {} to correctly pass through the environment variables provided by the user.

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/commands/install.ts#L30-L54

Potential issue: When installing a server for the OpenCode client using the `--env`
flag, the provided environment variables are silently ignored. For local servers, the
`environment` property is hardcoded to an empty object (`{}`). For remote servers, the
property is omitted entirely. This behavior is inconsistent with other clients like Zed
and Warp, which correctly apply the environment variables from `serverConfig.env`. This
leads to a silent failure where users believe their configuration is applied, but it is
not, potentially causing misconfigured server setups.

_{Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8130120}