fix: pass plugin mcp env overrides#386
Conversation
Signed-off-by: laplace young <yangqk12@whu.edu.cn>
|
Someone is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPlugin ChangesMCP Server Environment Variable Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (1)
358-359: 💤 Low valueConsider adding a concrete example of setting environment variables.
The documentation clearly explains what to do, but developers unfamiliar with environment variable configuration might benefit from a concrete example of how to launch Claude Code with these variables set.
📝 Suggested documentation enhancement
For remote or protected deployments, launch Claude Code with `AGENTMEMORY_URL` and `AGENTMEMORY_SECRET` set. The plugin passes both values through to its bundled MCP server, with `http://localhost:3111` as the default URL. + +Example (macOS/Linux): +```bash +AGENTMEMORY_URL=https://my-server.example.com:3111 \ +AGENTMEMORY_SECRET=my-secret-token \ +claude +``` + +Example (Windows PowerShell): +```powershell +$env:AGENTMEMORY_URL="https://my-server.example.com:3111" +$env:AGENTMEMORY_SECRET="my-secret-token" +claude +```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 358 - 359, Add concrete examples showing how to set AGENTMEMORY_URL and AGENTMEMORY_SECRET when launching Claude Code: show a POSIX shell example using inline env vars (AGENTMEMORY_URL=... AGENTMEMORY_SECRET=... claude or export ...; claude) and a Windows PowerShell example using $env:AGENTMEMORY_URL="..." and $env:AGENTMEMORY_SECRET="..." followed by invoking claude; reference the environment variable names AGENTMEMORY_URL and AGENTMEMORY_SECRET and the command claude so readers can copy/paste the snippets.test/codex-plugin.test.ts (1)
49-65: 💤 Low valueLGTM! Consider adding runtime integration test coverage.
The test correctly validates that the environment variable template strings are present in the configuration. However, it doesn't verify the actual runtime behavior—whether MCP clients successfully substitute these variables when they're set or fall back to defaults when unset.
While this config validation is appropriate for a unit test, consider documenting in the test plan how the runtime behavior is verified (e.g., manual testing with
AGENTMEMORY_URL=https://remote-server:3111 claude ...or integration tests that spawn actual MCP sessions).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/codex-plugin.test.ts` around lines 49 - 65, The current unit test in the "plugin MCP server inherits remote agentmemory environment overrides" case only asserts template strings in the .mcp.json (the mcp variable produced by readJson) but doesn't exercise runtime substitution; add an integration test that spawns or simulates an MCP client/server using the same .mcp.json (reuse the mcp variable from test/codex-plugin.test.ts) and verifies two runtime scenarios: (1) with AGENTMEMORY_URL and AGENTMEMORY_SECRET set in process.env the running MCP process resolves those into concrete values, and (2) with them unset the MCP process falls back to the default template values; implement this by launching the MCP command from mcp.mcpServers.agentmemory.command (or a small helper that loads the same config) and asserting the effective environment seen by the child process or IPC message matches expected resolved URLs/secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/.mcp.json`:
- Around line 6-9: The env entries in plugin/.mcp.json use shell-style
"${VAR:-default}" expansion which is not portable across MCP clients; update the
AGENTMEMORY_URL and AGENTMEMORY_SECRET entries to use a cross-client compatible
variable syntax (e.g. the VS Code-style "${env:AGENTMEMORY_URL}" /
"${env:AGENTMEMORY_SECRET}") and remove the ":-default" fallback from the JSON,
and implement any defaults in the plugin initialization code (where
AGENTMEMORY_URL and AGENTMEMORY_SECRET are read) so that functions referencing
those keys (AGENTMEMORY_URL, AGENTMEMORY_SECRET) get a safe default at runtime
instead of relying on shell expansion.
---
Nitpick comments:
In `@README.md`:
- Around line 358-359: Add concrete examples showing how to set AGENTMEMORY_URL
and AGENTMEMORY_SECRET when launching Claude Code: show a POSIX shell example
using inline env vars (AGENTMEMORY_URL=... AGENTMEMORY_SECRET=... claude or
export ...; claude) and a Windows PowerShell example using
$env:AGENTMEMORY_URL="..." and $env:AGENTMEMORY_SECRET="..." followed by
invoking claude; reference the environment variable names AGENTMEMORY_URL and
AGENTMEMORY_SECRET and the command claude so readers can copy/paste the
snippets.
In `@test/codex-plugin.test.ts`:
- Around line 49-65: The current unit test in the "plugin MCP server inherits
remote agentmemory environment overrides" case only asserts template strings in
the .mcp.json (the mcp variable produced by readJson) but doesn't exercise
runtime substitution; add an integration test that spawns or simulates an MCP
client/server using the same .mcp.json (reuse the mcp variable from
test/codex-plugin.test.ts) and verifies two runtime scenarios: (1) with
AGENTMEMORY_URL and AGENTMEMORY_SECRET set in process.env the running MCP
process resolves those into concrete values, and (2) with them unset the MCP
process falls back to the default template values; implement this by launching
the MCP command from mcp.mcpServers.agentmemory.command (or a small helper that
loads the same config) and asserting the effective environment seen by the child
process or IPC message matches expected resolved URLs/secrets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36f307a8-6612-4f7c-9cf6-968db4860c16
📒 Files selected for processing (3)
README.mdplugin/.mcp.jsontest/codex-plugin.test.ts
| "env": { | ||
| "AGENTMEMORY_URL": "${AGENTMEMORY_URL:-http://localhost:3111}", | ||
| "AGENTMEMORY_SECRET": "${AGENTMEMORY_SECRET:-}" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for documentation of env var syntax in MCP-related files and comments
# Check if there are any other .mcp.json examples in the repo
echo "=== Other MCP config examples in repo ==="
fd -e json -e md . | xargs rg -l 'mcpServers.*env' -A5 -B5
# Check README and docs for MCP env var documentation
echo "=== MCP env var documentation in README ==="
rg -C5 'AGENTMEMORY_URL.*localhost.*3111' README.md
# Check if there are integration tests that actually exercise this
echo "=== Tests that might validate runtime behavior ==="
rg -C3 'AGENTMEMORY_URL|AGENTMEMORY_SECRET' test/Repository: rohitg00/agentmemory
Length of output: 1967
🌐 Web query:
MCP Model Context Protocol environment variable syntax specification
💡 Result:
There is no single, protocol-level “MCP Model Context Protocol environment variable syntax specification” in the upstream MCP spec itself. What is standardized in MCP: 1) The MCP protocol spec defines an HTTP authorization framework for HTTP-based transports, but it explicitly does not require stdio clients/servers to follow that HTTP auth scheme and instead notes that credentials should come from the environment for stdio implementations. [1] What “environment variable syntax” does exist for MCP today: A) MCP client configuration formats (e.g., VS Code / Cursor style mcp.json, and other MCP client tools) each implement their own string interpolation / env expansion rules. - VS Code’s MCP configuration reference shows that the env field is “Environment variables for the server” and also supports an envFile field. [2] - A VS Code issue discussing env-var placeholders in mcp.json shows the commonly used pattern ${env:VARIABLE_NAME} referenced by VS Code’s variable system, and reports that behavior can differ depending on where the config comes from (user mcp.json vs plugin-provided). [3] B) Some MCP client libraries / wrappers don’t expand ${VAR} placeholders at all (they pass them through as literal strings), which is not an MCP spec guarantee. - A LangChain MCP adapters issue states that ${VAR} syntax in the env block is “widely-used” but that their adapter passes those values through without resolving them. [4] C) MCP-related runtimes and deployment tools introduce their own environment-variable templating syntax (again, not MCP core). - Example: code/config templates for an MCP server can use ${VAR}-style substitution in some tooling; one MCP Hub docs page documents ${...} expansion and ${VAR_NAME:-default} defaults as supported by that product. [5] - Example: 1mcp-app/agent documents a set of environment variables for configuring the agent itself (prefixed ONE_MCP_), which is tool-specific rather than part of MCP protocol syntax. [6] So what syntax should you use? - If you are editing an MCP client config file such as VS Code’s mcp.json, use the syntax that that specific client supports. In VS Code discussions, ${env:VARIABLE_NAME} is the referenced form. [3] - If you’re using another MCP client/runtime, follow its docs; some support ${VAR} expansion, others require ${env:VAR}, and some don’t expand placeholders when passed in the env map. [2][4] If you tell me which exact MCP client/tool you’re using (e.g., VS Code Copilot MCP, Cursor, Claude Desktop, a specific SDK like openai-agents-js, or a specific MCP gateway), I can look up that tool’s exact environment-variable placeholder grammar and confirm with citations.
Citations:
- 1: https://openai.github.io/openai-agents-js/guides/mcp
- 2: https://github.com/cyanheads/model-context-protocol-resources/blob/main/guides/mcp-server-development-guide.md
- 3: https://modelcontextprotocol.io/specification/2025-06-18/server/resources
- 4: https://docs.docker.com/ai/gordon/mcp
- 5: https://www.codebuddy.ai/docs/cli/mcp
- 6: https://www.sitepoint.com/mcp-model-context-protocol-complete-developer-integration-guide/
Verify MCP client compatibility for environment variable syntax ${VAR:-default}.
The ${VAR:-default} syntax uses shell parameter expansion, not standard JSON. Web search confirms that MCP clients do NOT have a shared standard for environment variable substitution—each client implements its own rules:
- VS Code/Cursor use
${env:VARIABLE_NAME}syntax - Some MCP clients (e.g., LangChain adapters) do not expand
${VAR}placeholders at all, treating them as literal strings - A few tools support
${VAR:-default}, but coverage is not universal
This configuration may break when deployed to clients that don't recognize this syntax. Test with your target MCP clients (Claude Code, Cursor, Cline, Windsurf) or use the ${env:VARIABLE_NAME} syntax documented in VS Code's MCP configuration reference if cross-client compatibility is required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/.mcp.json` around lines 6 - 9, The env entries in plugin/.mcp.json use
shell-style "${VAR:-default}" expansion which is not portable across MCP
clients; update the AGENTMEMORY_URL and AGENTMEMORY_SECRET entries to use a
cross-client compatible variable syntax (e.g. the VS Code-style
"${env:AGENTMEMORY_URL}" / "${env:AGENTMEMORY_SECRET}") and remove the
":-default" fallback from the JSON, and implement any defaults in the plugin
initialization code (where AGENTMEMORY_URL and AGENTMEMORY_SECRET are read) so
that functions referencing those keys (AGENTMEMORY_URL, AGENTMEMORY_SECRET) get
a safe default at runtime instead of relying on shell expansion.
|
Solid fix and the README addition is great. One small concern before merge: the Could you simplify to: "env": {
"AGENTMEMORY_URL": "${AGENTMEMORY_URL}",
"AGENTMEMORY_SECRET": "${AGENTMEMORY_SECRET}"
}The shim already defaults to |
|
Updated the plugin MCP env templates to use plain Verified locally with: npm test -- --run test/codex-plugin.test.ts |
|
Thanks @LaplaceYoung — plugin .mcp.json was indeed missing the env-passthrough that the connect adapters already had (util.ts AGENTMEMORY_MCP_BLOCK). Cleanly closes the remote-deployment gap. Merged. Closes #375. |
2 participants
Summary
AGENTMEMORY_URLandAGENTMEMORY_SECRETthrough the bundled plugin MCP server config.http://localhost:3111as the default MCP server target whenAGENTMEMORY_URLis unset.Fixes #375
Test plan
npm test -- --run test/codex-plugin.test.tsnpx tsdowngit diff --checkNotes
npm run buildreaches the tsdown compile step on Windows, then the shell copy tail exits ontruebecause the script uses Unix shell syntax.npm teston Windows reports path-sensitive failures intest/obsidian-export.test.tsandtest/compress-file.test.ts; 894 tests pass, includingtest/codex-plugin.test.ts.Summary by CodeRabbit