Modify the setup skill to avoid credential handling#23
Modify the setup skill to avoid credential handling#23
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies the mongodb-mcp-setup skill by removing credential handling from the LLM context. Instead of having the agent collect and write credentials directly, the skill now instructs users to add credentials to their shell profiles themselves. It also removes the dual "Interactive Mode" / "Documentation Mode" approach in favor of a single interactive guide flow.
Changes:
- Removed credential collection/handling logic and the two execution modes (Interactive/Documentation), replacing them with a single step-by-step guide where users edit their own shell profiles
- Simplified and streamlined instructions throughout (connection string setup, service account setup, shell profile configuration, troubleshooting)
- Removed the
SETUP.mdfile creation fallback (Documentation Mode)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
dacharyc
left a comment
dacharyc
left a comment
There was a problem hiding this comment.
Solid wins here; the credential handling removal simplifies the flow, and consolidating the troubleshooting/error sections addresses the token efficiency concerns from the last round. Looks good.
One minor gap: Steps 1 and 5.1 assume the agent can run shell commands, but there's no fallback if it can't. The old Documentation Mode covered this (albeit imperfectly). A single line like "If the agent cannot run shell commands, ask the user for their shell type and profile path, then skip directly to Step 5.2" would close the loop. Not a blocker; just worth noting for a future pass.
skills/mongodb-mcp-setup/SKILL.md
Outdated
| Run this command to check for existing configuration: | ||
|
|
||
| ```bash | ||
| env | grep -E "MDB_MCP_(CONNECTION_STRING|API_CLIENT_ID|API_CLIENT_SECRET|READ_ONLY)" |
There was a problem hiding this comment.
Won't this expose the credential to the LLM after it was just told that it wouldn't handle any credentials on line 29?
skills/mongodb-mcp-setup/SKILL.md
Outdated
| 3. Add current IP or 0.0.0.0/0 for testing (not for production) | ||
| 1. On the service account details page, find **API Access List** | ||
| 2. Click **Add Access List Entry** | ||
| 3. Add current IP address (or `0.0.0.0/0` for testing only — not for production) |
There was a problem hiding this comment.
I know that you have stated that 0.0.0.0/0 is for testing only, but I would emphasize that using it is a security concern. The end user needs to know that it is a risk and probably not necessary.
skills/mongodb-mcp-setup/SKILL.md
Outdated
| ### 5.2: Show the Exact Snippet to Add | ||
|
|
||
| Check if variables already exist in the profile file. If so, replace rather than duplicate. | ||
| Tell the user to open their profile file (e.g. `code ~/.zshrc`, `nano ~/.zshrc`, or their preferred editor) and add the following lines at the end. Make sure to adapt the profile path in the instructions to match their shell. |
There was a problem hiding this comment.
The shell rc files are normally group and world readable by default most of the time. I think it might be better guidance to would be to add the connection strings into a dedicated ~/.mcp-env file (chmod 600) and then source that file from the shell rc file. This would help protect accidental perm changes to the shell rc file. This would also help prevent users accidentally committing the connection string to git if it was in their shell rc file.
skills/mongodb-mcp-setup/SKILL.md
Outdated
|
|
||
| ```bash | ||
| export MDB_MCP_CONNECTION_STRING="<paste-your-connection-string-here>" | ||
| env | grep MDB_MCP |
There was a problem hiding this comment.
We are exposing the credentials to the LLM again after stating that they wouldn't be.
skills/mongodb-mcp-setup/SKILL.md
Outdated
|
|
||
| ```bash | ||
| export MDB_MCP_READ_ONLY="true" | ||
| chmod 600 ~/.zshrc # adjust path as needed |
There was a problem hiding this comment.
See note above about using a dedicated ~/.mcp-env file.
There was a problem hiding this comment.
Pull request overview
Updates the mongodb-mcp-setup skill instructions to avoid credential collection/handling in the LLM context by guiding users to configure secrets directly in their local environment.
Changes:
- Reworks Step 1 environment-variable checks to mask values in output.
- Replaces credential “collection” steps with instructions for users to add secrets themselves (via a dedicated env file + shell profile sourcing).
- Refines service-account guidance and troubleshooting notes with added security warnings.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -358,24 +276,11 @@ Proceed to Step 6 (Next Steps). | |||
| - List deployments: `atlas-local-list-deployments` | |||
| - Use standard database operations once connected | |||
|
|
|||
| ## Important Notes | |||
|
|
|||
| - **Security**: Never commit credentials to version control. Set shell profile permissions to 600. | |||
|
|
|||
| - **Troubleshooting** if MCP server doesn't work after restart: | |||
| - Verify variables: `env | grep MDB_MCP` | |||
| - Confirm full restart (not reload) | |||
| - Test credentials directly (mongosh for connection string, API call for service account) | |||
| - For read-only mode, verify `MDB_MCP_READ_ONLY=true` | |||
| - Check MCP server logs for errors | |||
|
|
|||
| ## Error Handling | |||
|
|
|||
| Common issues: | |||
| ## Troubleshooting | |||
|
|
|||
| - **Bash permission denied**: Automatically switch to Documentation Mode (Step 5b) | |||
| - **Invalid connection string**: Provide correct format guidance | |||
| - **Profile file doesn't exist**: Create it (Interactive) or explain where to create it (Documentation) | |||
| - **Permission denied on profile**: Fix permissions (Interactive) or include instructions (Documentation) | |||
| - **Variables not loading**: Check shell type and profile path | |||
| - **Invalid service account credentials**: Direct to Atlas to verify/regenerate | |||
| - **Variables not appearing after `source`**: Check the profile file path and confirm the file was saved | |||
| - **Client doesn't pick up variables**: Ensure full restart (quit + reopen), not just a reload | |||
| - **Invalid connection string format**: Re-check the format; must start with `mongodb://` or `mongodb+srv://` | |||
| - **Atlas Admin API errors (Option B)**: Verify your IP is in the service account's API Access List | |||
| - **Read-only mode not working**: Check `MDB_MCP_READ_ONLY=true` is set — run `env | grep ^MDB_MCP_READ_ONLY` | |||
| - **fish/PowerShell**: Syntax differs — use `set -x` (fish) or `$env:` (PowerShell) instead of `export` | |||
There was a problem hiding this comment.
The verification command in Step 5.3 masks all values, which is good for secrets, but it also prevents confirming that non-secret flags are correctly set (e.g., read-only needs MDB_MCP_READ_ONLY=true, not just “set”). Consider either checking MDB_MCP_READ_ONLY explicitly without masking, or adding a targeted boolean check alongside the masked output.
| Run this command to check for existing configuration (masking values to avoid exposing credentials): | ||
|
|
||
| ```bash | ||
| env | grep -E "MDB_MCP_(CONNECTION_STRING|API_CLIENT_ID|API_CLIENT_SECRET|READ_ONLY)" | ||
| env | grep "^MDB_MCP" | sed 's/=.*/=[set]/' | ||
| ``` | ||
|
|
||
| **Interpretation (if Bash succeeded):** | ||
| **Interpretation:** | ||
|
|
||
| - If `MDB_MCP_CONNECTION_STRING` is set, the user has connection string auth configured | ||
| - If both `MDB_MCP_API_CLIENT_ID` and `MDB_MCP_API_CLIENT_SECRET` are set, the user has service account auth configured. If only one of these is set, the configuration is incomplete and treat it as if neither is set. | ||
| - If `MDB_MCP_READ_ONLY` is set to `true`, the user has read-only mode enabled | ||
| - If `MDB_MCP_CONNECTION_STRING` is set → connection string auth is configured | ||
| - If both `MDB_MCP_API_CLIENT_ID` and `MDB_MCP_API_CLIENT_SECRET` are set → service account auth is configured. If only one is set, treat it as incomplete. | ||
| - If `MDB_MCP_READ_ONLY=true` → read-only mode is enabled |
There was a problem hiding this comment.
Step 1 masks all values with sed 's/=.*/=[set]/', so you can no longer tell whether MDB_MCP_READ_ONLY is specifically true vs just set to some other value. The subsequent interpretation bullet (“If MDB_MCP_READ_ONLY=true → …”) becomes unverifiable with the provided command. Consider either exempting MDB_MCP_READ_ONLY from masking, or adding a separate check that only reveals the boolean (since it’s not a secret).
This modifies the instructions in the
mongodb-mcp-setupskill to eliminate the need for credential passing into the LLM context.