fix(mcp): wrap npx-based servers to ensure node is on PATH

[johnae]

No description provided.

[claude]

Claude finished @johnae's task —— View job

---

Code Review for PR #1715

Todo List:

• Read repository guidelines (CLAUDE.md if present)
• Review changes in users/profiles/bizniz.nix
• Review changes in users/profiles/mcp.nix
• Analyze code quality and best practices
• Check for potential bugs or issues
• Consider performance implications
• Evaluate security concerns
• Assess test coverage needs
• Provide comprehensive review feedback

---

Review Summary

The PR wraps npx-based MCP servers in shell scripts to ensure Node.js is properly on the PATH. This is a good defensive fix for environment issues.

✅ Strengths

1. Proper PATH management: The change correctly ensures Node.js is available in PATH before executing npx commands
2. Consistent approach: Both files use the same wrapping pattern with writeShellScriptBin
3. Arguments preservation: Uses "$@" to properly pass all arguments to the wrapped command
4. Nix best practices: Uses pkgs.lib.getExe to get the executable path from the derivation

🔍 Code Quality & Best Practices

The implementation follows Nix conventions well. The use of writeShellScriptBin is appropriate for creating wrapper scripts, and pkgs.lib.getExe is the correct way to reference the executable.

🐛 Potential Issues

1. Missing description: The PR lacks a description explaining why this change is needed. Consider adding context about what environment issues this fixes.

2. Inconsistent quoting: In users/profiles/mcp.nix:24, the arguments after npx are unquoted:

   exec npx @playwright/mcp@latest --browser chromium --executable-path ${pkgs.chromium}/bin/chromium "$@"

   While this works, consider quoting ${pkgs.chromium}/bin/chromium for consistency and to handle potential spaces in paths:

   exec npx @playwright/mcp@latest --browser chromium --executable-path "${pkgs.chromium}/bin/chromium" "$@"

⚡ Performance Considerations

• The wrapper scripts add minimal overhead (shell script startup)
• Using exec ensures the wrapper process is replaced, avoiding an extra process

🔒 Security Concerns

No significant security issues identified:

• PATH is explicitly set rather than appended, avoiding PATH injection
• Uses exec to replace the shell process
• Properly quotes "$@" to handle arguments safely

🧪 Test Coverage

Since this is a Nix configuration change:

• Consider testing that the MCP servers still function correctly after this change
• Verify that the servers work in environments where Node.js isn't already in PATH
• No automated tests appear necessary for this configuration change

📝 Recommendations

1. Add a PR description explaining the motivation for this change
2. Consider adding the missing quotes in the playwright command for consistency
3. Test the changes in an environment where Node.js is not in the system PATH to verify the fix works as intended

Overall, this is a solid fix that properly addresses PATH issues for npx-based MCP servers. The implementation is clean and follows Nix best practices.