Add support for proxy env var aliases

[jdalton]

No description provided.

[claude]

Claude finished @jdalton's task —— View job

---

PR Review: Add support for proxy env var aliases

This PR adds support for standard proxy environment variable aliases (HTTPS_PROXY and https_proxy) alongside existing Socket-specific proxy environment variables, improving compatibility with corporate environments and standard tooling.

Todo List

• Read and analyze changed files
• Review constants.mts changes
• Review sdk.mts changes
• Review npm-fix.mts changes
• Review test changes
• Provide comprehensive feedback

---

📋 Code Review Feedback

✅ Strengths

1. Standards Compliance: Excellent addition of HTTPS_PROXY and https_proxy support - these are widely recognized standard environment variables used across many tools.

2. Backward Compatibility: Well-designed fallback chain preserves all existing Socket-specific environment variables while adding standard alternatives.

3. Priority Order: The environment variable precedence is logical:

   • SOCKET_CLI_API_PROXY (highest priority)
   • SOCKET_SECURITY_API_PROXY (legacy support)
   • HTTPS_PROXY (standard uppercase)
   • https_proxy (standard lowercase)

4. Corporate-Friendly: This change significantly improves usability in enterprise environments where proxy settings are typically configured via standard environment variables.

🔍 Areas for Improvement

High Priority Issues

1. Missing HTTP_PROXY Support (src/constants.mts:339-340)

   // Current implementation only supports HTTPS proxies
   envAsString(env['HTTPS_PROXY']) ||
   envAsString(env['https_proxy']),

   Issue: Standard proxy conventions include both HTTP_PROXY/http_proxy and HTTPS_PROXY/https_proxy. Many tools check HTTP_PROXY as a fallback when HTTPS_PROXY isn't set.

   Recommendation: Add HTTP_PROXY and http_proxy to the fallback chain:

   envAsString(env['HTTPS_PROXY']) ||
   envAsString(env['https_proxy']) ||
   envAsString(env['HTTP_PROXY']) ||
   envAsString(env['http_proxy']),

2. Inconsistent Proxy Protocol Handling (src/utils/sdk.mts:104-106)

   const ProxyAgent = proxy?.startsWith('http:')
     ? HttpProxyAgent
     : HttpsProxyAgent

   Issue: This assumes all non-HTTP URLs are HTTPS, which may not be correct for other protocols or malformed URLs.

   Recommendation: Add explicit HTTPS protocol check:

   const ProxyAgent = proxy?.startsWith('https:')
     ? HttpsProxyAgent
     : HttpProxyAgent

Medium Priority Issues

3. No NO_PROXY Support
   Issue: Standard proxy configurations often include NO_PROXY or no_proxy to specify hosts that should bypass the proxy. This is missing from the implementation.

   Recommendation: Consider adding NO_PROXY support in a future iteration to complete the standard proxy environment variable support.

4. Limited Documentation
   Issue: The code comments are good but there's no visible documentation about the new proxy environment variable aliases.

   Recommendation: Update documentation to mention the new supported environment variables.

Low Priority Issues

5. Type Safety in Agent Selection (src/utils/sdk.mts:104-106)
   Issue: The current implementation could benefit from better type safety and error handling for edge cases.

   Recommendation: Consider adding URL validation before agent selection.

🧪 Testing Considerations

6. Test Coverage Gap
   Issue: The changes to test/socket-npm.test.mts appear minimal (+1/-0) and may not adequately test the new proxy environment variable functionality.

   Recommendation: Add specific tests for:

   • Each supported proxy environment variable
   • Priority order verification
   • HTTP vs HTTPS proxy agent selection
   • Invalid proxy URL handling

🔐 Security Considerations

7. Proxy URL Validation
   Positive: The existing isUrl() function provides good validation for proxy URLs.

   Note: Consider logging or warning when proxy environment variables contain potentially unsafe URLs.

📈 Performance Impact

8. Environment Variable Lookup
   Assessment: The additional environment variable checks have minimal performance impact as they're evaluated lazily and cached.

🏗️ Architecture Assessment

9. Design Pattern
   Positive: The fallback pattern using the || operator is clean and follows established patterns in the codebase.

   Positive: The separation of concerns between proxy URL resolution and agent creation is well-maintained.

---

🎯 Summary

This is a solid, well-implemented feature that significantly improves Socket CLI's compatibility with standard proxy conventions. The main areas for improvement are:

1. Add HTTP_PROXY/http_proxy support for complete standard compliance
2. Fix the proxy protocol detection logic to properly handle HTTPS URLs
3. Enhance test coverage for the new functionality

The changes demonstrate good understanding of the existing codebase patterns and maintain backward compatibility while adding valuable functionality for enterprise users.

Overall Assessment: ✅ Approve with suggestions for improvement