fix: [#701] surface unresolved MCP env-var warnings to user via toast#806
Open
VJ-yadav wants to merge 1 commit intoAltimateAI:mainfrom
Open
fix: [#701] surface unresolved MCP env-var warnings to user via toast#806VJ-yadav wants to merge 1 commit intoAltimateAI:mainfrom
VJ-yadav wants to merge 1 commit intoAltimateAI:mainfrom
Conversation
… via toast
An unresolved `${VAR}` in an MCP server's env/headers block silently
became `""`, causing the server to launch with empty credentials and
fail downstream with a confusing error. The warning was logged but
most users never see the log.
This adds a per-server accumulator in `mcp/discover.ts` that captures
unresolved env-var references during discovery, plus a one-shot
`consumeUnresolvedEnvVars()` API consumed by `MCP.state` initializer,
which publishes a `TuiEvent.ToastShow` warning naming the affected
servers and missing variables and pointing users at `${VAR:-default}`
for explicit fallbacks. Adds an `mcp_unresolved_env_vars` telemetry
event to track recovery rate.
Scope: external MCP configs only (`.vscode/mcp.json`, `.cursor/mcp.json`,
`.mcp.json`, `~/.claude.json`, etc.), where each server has direct
attribution. Unresolved vars in `opencode.json`'s `mcp.environment`
go through `ConfigPaths.substitute()` at file level and are tracked
in existing telemetry; surfacing those to a toast needs file→server
attribution and is left as follow-up.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
📝 WalkthroughWalkthroughThis PR implements user-facing warnings for unresolved MCP environment-variable references. The discovery module now accumulates unresolved ChangesMCP Unresolved Environment Variables Warning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 (1)
packages/opencode/src/mcp/index.ts (1)
190-190: ⚡ Quick winConsider simplifying the type annotation.
The type
Awaited<ReturnType<typeof import("./discover").consumeUnresolvedEnvVars>>is verbose. SinceUnresolvedEnvVarRecordis already exported from./discover, you could add a top-level import and use it directly:import type { UnresolvedEnvVarRecord } from "./discover"Then at line 190:
- let unresolvedEnvVars: Awaited<ReturnType<typeof import("./discover").consumeUnresolvedEnvVars>> = [] + let unresolvedEnvVars: UnresolvedEnvVarRecord[] = []🤖 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 `@packages/opencode/src/mcp/index.ts` at line 190, Replace the verbose type annotation on the unresolvedEnvVars variable by importing the exported type UnresolvedEnvVarRecord from "./discover" and using it directly; add a top-level line import type { UnresolvedEnvVarRecord } from "./discover" and change the declaration for unresolvedEnvVars (the variable assigned from consumeUnresolvedEnvVars) to use UnresolvedEnvVarRecord[] instead of Awaited<ReturnType<typeof import("./discover").consumeUnresolvedEnvVars>> so the code reads simply with the existing consumeUnresolvedEnvVars usage.
🤖 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 `@packages/opencode/test/mcp/discover-unresolved-env.test.ts`:
- Around line 10-28: Replace manual temp dir lifecycle by removing the top-level
tempDir variable and the beforeEach/afterEach hooks; instead in each test use
the tmpdir fixture with "await using cleanTmp = await tmpdir()" to create a
temporary directory that auto-cleans, and call consumeUnresolvedEnvVars() and
explicit deletes of UNRESOLVED_TEST_VAR_A/B/TOKEN inside each test setup as
needed; also update the test that currently creates a second temp dir (lines
~229–258) to use a nested "await using" (e.g., "await using secondTmp = await
tmpdir()") rather than mkdtemp/rm so cleanup is automatic.
---
Nitpick comments:
In `@packages/opencode/src/mcp/index.ts`:
- Line 190: Replace the verbose type annotation on the unresolvedEnvVars
variable by importing the exported type UnresolvedEnvVarRecord from "./discover"
and using it directly; add a top-level line import type { UnresolvedEnvVarRecord
} from "./discover" and change the declaration for unresolvedEnvVars (the
variable assigned from consumeUnresolvedEnvVars) to use UnresolvedEnvVarRecord[]
instead of Awaited<ReturnType<typeof
import("./discover").consumeUnresolvedEnvVars>> so the code reads simply with
the existing consumeUnresolvedEnvVars usage.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5c3ca9cb-5d2b-4c9c-ba41-ec9e1075a00b
📒 Files selected for processing (4)
packages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/mcp/discover.tspackages/opencode/src/mcp/index.tspackages/opencode/test/mcp/discover-unresolved-env.test.ts
Comment on lines
+10
to
+28
| let tempDir: string | ||
|
|
||
| beforeEach(async () => { | ||
| tempDir = await mkdtemp(path.join(tmpdir(), "mcp-unresolved-env-")) | ||
| // Drain anything left over from prior tests so we start clean. | ||
| consumeUnresolvedEnvVars() | ||
| delete process.env["UNRESOLVED_TEST_VAR_A"] | ||
| delete process.env["UNRESOLVED_TEST_VAR_B"] | ||
| delete process.env["UNRESOLVED_TEST_TOKEN"] | ||
| }) | ||
|
|
||
| afterEach(async () => { | ||
| await rm(tempDir, { recursive: true, force: true }) | ||
| // Empty the accumulator so a failed test doesn't poison the next one. | ||
| consumeUnresolvedEnvVars() | ||
| delete process.env["UNRESOLVED_TEST_VAR_A"] | ||
| delete process.env["UNRESOLVED_TEST_VAR_B"] | ||
| delete process.env["UNRESOLVED_TEST_TOKEN"] | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace manual temp directory management with the tmpdir fixture.
The coding guidelines require using the tmpdir function from fixture/fixture.ts with await using syntax for automatic cleanup. As per coding guidelines, this ensures proper cleanup even when tests fail and follows the project's established testing patterns.
♻️ Refactor to use tmpdir fixture
Remove the global tempDir variable and beforeEach/afterEach hooks. Instead, use await using in each test:
import { describe, test, expect, beforeEach, afterEach } from "bun:test"
-import { mkdtemp, rm, mkdir, writeFile } from "fs/promises"
-import { tmpdir } from "os"
+import { mkdir, writeFile } from "fs/promises"
import path from "path"
+import { tmpdir } from "../fixture/fixture"
import { consumeUnresolvedEnvVars, discoverExternalMcp } from "../../src/mcp/discover"
-let tempDir: string
-
beforeEach(async () => {
- tempDir = await mkdtemp(path.join(tmpdir(), "mcp-unresolved-env-"))
// Drain anything left over from prior tests so we start clean.
consumeUnresolvedEnvVars()
delete process.env["UNRESOLVED_TEST_VAR_A"]
delete process.env["UNRESOLVED_TEST_VAR_B"]
delete process.env["UNRESOLVED_TEST_TOKEN"]
})
afterEach(async () => {
- await rm(tempDir, { recursive: true, force: true })
// Empty the accumulator so a failed test doesn't poison the next one.
consumeUnresolvedEnvVars()
delete process.env["UNRESOLVED_TEST_VAR_A"]
delete process.env["UNRESOLVED_TEST_VAR_B"]
delete process.env["UNRESOLVED_TEST_VAR_TOKEN"]
})
describe("consumeUnresolvedEnvVars (issue `#701`)", () => {
test("captures unresolved ${VAR} in a local server's env block", async () => {
+ await using tmp = await tmpdir()
- await mkdir(path.join(tempDir, ".vscode"), { recursive: true })
+ await mkdir(path.join(tmp.path, ".vscode"), { recursive: true })
await writeFile(
- path.join(tempDir, ".vscode/mcp.json"),
+ path.join(tmp.path, ".vscode/mcp.json"),
// ... rest of test
)
- await discoverExternalMcp(tempDir)
+ await discoverExternalMcp(tmp.path)
// ...
})
// Apply the same pattern to all other tests
})Apply this pattern to all 11 tests. The test at lines 229-258 that creates a second temp directory should use a nested await using cleanTmp = await tmpdir() instead of manual mkdtemp/rm.
As per coding guidelines: "Use the tmpdir function from fixture/fixture.ts to create temporary directories for tests with automatic cleanup in test files. Always use await using syntax with tmpdir() for automatic cleanup when the variable goes out of scope."
🤖 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 `@packages/opencode/test/mcp/discover-unresolved-env.test.ts` around lines 10 -
28, Replace manual temp dir lifecycle by removing the top-level tempDir variable
and the beforeEach/afterEach hooks; instead in each test use the tmpdir fixture
with "await using cleanTmp = await tmpdir()" to create a temporary directory
that auto-cleans, and call consumeUnresolvedEnvVars() and explicit deletes of
UNRESOLVED_TEST_VAR_A/B/TOKEN inside each test setup as needed; also update the
test that currently creates a second temp dir (lines ~229–258) to use a nested
"await using" (e.g., "await using secondTmp = await tmpdir()") rather than
mkdtemp/rm so cleanup is automatic.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. cc @VJ-yadav |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #701.
An unresolved
${VAR}in an MCP server'senvorheadersblock silently became\"\", the server then launched with empty credentials, and the user saw a confusing downstream error. The warning was logged vialog.warnbut most users never check the log.This wires the existing log warning into the existing TUI toast mechanism so the user sees a single warning toast at startup naming each affected server, the source file, and the missing env var(s) — and pointing them at
\${VAR:-default}for explicit fallbacks.What changed
mcp/discover.ts— added a per-discovery accumulator.resolveServerEnvVarsnow records unresolved vars (deduped per block) into module state, in addition to the existinglog.warn. New exports:UnresolvedEnvVarRecordtype andconsumeUnresolvedEnvVars()(one-shot drain). The accumulator is reset at the start of eachdiscoverExternalMcp()call so a re-discovery does not show stale warnings.mcp/index.ts— the state initializer now drainsconsumeUnresolvedEnvVars()alongsideconsumeDiscoveryResult(). If non-empty, publishes aTuiEvent.ToastShowwithvariant: \"warning\", grouped by server, and tracks anmcp_unresolved_env_varstelemetry event. Reuses the same Bus + Toast plumbing the discovery toast already uses (PR fix: resolve env-var references in MCP server environment config (closes #656) #666 / existing OAuth flow), so no new UI components.altimate/telemetry/index.ts— added themcp_unresolved_env_varsevent type.test/mcp/discover-unresolved-env.test.ts— 11 new unit tests covering capture, dedupe, multi-var, default-suffix passthrough, escape (\$\${VAR}) passthrough, set-var passthrough, env+headers separation, one-shot drain, and reset-on-new-discovery.Scope
Limited to external MCP configs (
.vscode/mcp.json,.cursor/mcp.json,.github/copilot/mcp.json,.mcp.json,~/.claude.json,~/.gemini/settings.json) where each server has direct attribution.Unresolved vars in
opencode.json'smcp.environmentgo throughConfigPaths.substitute()at file-level and are already tracked inconfig_env_interpolationtelemetry. Surfacing those to a toast needs file→server attribution and is left as a follow-up — happy to do it in a separate PR if reviewers want both paths covered here.Test plan
bun test test/mcp/discover-unresolved-env.test.ts— 11/11 passbun turbo typecheck— clean across all 5 packagesbun test test/mcp/— same 29 pre-existing failures asmain(all caused by the suite reading the user's real~/.pencil/,~/.gemini/,~/.claude.json— unrelated to this change). My branch adds 11 passing tests on top.main(c859b57ec)Manual verification (toast appearance)
The new toast reuses the existing
TuiEvent.ToastShowvariant: \"warning\"path already used by the MCP OAuth needs-auth toast atmcp/index.ts:475and the OAuth client-registration failure toast atmcp/index.ts:486. No new UI; same renderer, same styling. To see it in a live TUI, drop a.mcp.jsonlike:{ \"mcpServers\": { \"snowflake\": { \"command\": \"snowflake-mcp\", \"env\": { \"SNOWFLAKE_PASSWORD\": \"\${SNOWFLAKE_PASSWORD}\" } } } }…with
SNOWFLAKE_PASSWORDunset, and start a session. The toast appears once at MCP startup with title "MCP env vars unresolved" and a 12s duration.Summary by cubic
Show a warning toast at MCP startup when unresolved ${VAR} references are found in server env or headers, so users can fix missing vars before servers fail. Addresses #701 by surfacing clear, grouped warnings and tracking telemetry.
mcp/discover.ts; newconsumeUnresolvedEnvVars()drains it and resets each discovery.mcp/index.tsshows a single warningTuiEvent.ToastShow, grouped by server with source path and var names; suggests${VAR:-default}.mcp_unresolved_env_varstelemetry inaltimate/telemetry..vscode/mcp.json,.cursor/mcp.json,.github/copilot/mcp.json,.mcp.json,~/.claude.json,~/.gemini/settings.json).Written for commit 1a40b4f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests