feat(mcp): Add context7 and deepwiki MCP support#856
Merged
Gkrumbach07 merged 6 commits intoambient-code:mainfrom Mar 12, 2026
Merged
feat(mcp): Add context7 and deepwiki MCP support#856Gkrumbach07 merged 6 commits intoambient-code:mainfrom
Gkrumbach07 merged 6 commits intoambient-code:mainfrom
Conversation
syntaxsdev
force-pushed
the
1-add-mcp-tools
branch
from
d490943 to
b0d1944
Compare
syntaxsdev
changed the title
WalkthroughThis PR adds MCP (Model Context Protocol) server credential management: backend handlers and Secret-backed storage, frontend UI, API and queries, runner-side credential population, and two new MCP server entries (context7, deepwiki). Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend UI
participant Backend as Backend API
participant Secret as Kubernetes Secret
participant Runner as Runner Process
User->>Frontend: Click Connect MCP Server
Frontend->>Frontend: Validate form fields
Frontend->>Backend: POST /auth/mcp/{serverName}/connect (fields)
activate Backend
Backend->>Backend: Validate token, user, serverName
Backend->>Secret: Upsert credentials at key {serverName}:{userID}
activate Secret
Secret-->>Backend: Secret updated
deactivate Secret
Backend-->>Frontend: Success (200)
deactivate Backend
Frontend->>Frontend: Invalidate status & integrations queries
Frontend->>Frontend: Show success toast, clear form
rect rgba(100, 150, 200, 0.5)
Note over Runner,Backend: Runner credential fetch flow
end
Runner->>Runner: Start up, read .mcp.json
Runner->>Backend: GET /auth/mcp/{serverName}/status
activate Backend
Backend->>Secret: Read credentials for {serverName}:{runnerUserID}
activate Secret
Secret-->>Backend: Return fields
deactivate Secret
Backend-->>Runner: Return MCPServerStatus with fieldNames
deactivate Backend
Runner->>Runner: Populate MCP_{SERVER}_{FIELD}=value env vars
sequenceDiagram
actor User
participant Frontend as Frontend UI
participant Backend as Backend API
participant Secret as Kubernetes Secret
User->>Frontend: View Integrations page
Frontend->>Backend: GET /integrations/status
activate Backend
Backend->>Secret: Read all MCP credential entries for userID
activate Secret
Secret-->>Backend: Return credentials map
deactivate Secret
Backend->>Backend: Build mcpServers status summary
Backend-->>Frontend: IntegrationsStatus { mcpServers: {...} }
deactivate Backend
Frontend->>Frontend: Render IntegrationsClient
Frontend->>Frontend: Merge predefined + backend mcpServers
Frontend->>Frontend: Render MCPCredentialCard for each server
Frontend-->>User: Display cards with connect/disconnect status
User->>Frontend: Click Disconnect on server
Frontend->>Backend: DELETE /auth/mcp/{serverName}/disconnect
activate Backend
Backend->>Secret: Delete credentials at {serverName}:{userID}
activate Secret
Secret-->>Backend: Deleted
deactivate Secret
Backend-->>Frontend: Success
deactivate Backend
Frontend->>Frontend: Invalidate queries, refresh UI
Frontend-->>User: Show disconnected state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/mcp_credentials.go`:
- Around line 249-257: The helper functions (starting with storeMCPCredentials)
are using the package-level K8sClient which elevates requests to the backend
service account; change these helpers to accept a request-scoped Secret client
(the Kubernetes corev1.SecretInterface) as a parameter and replace all uses of
K8sClient.CoreV1().Secrets(Namespace) with that injected secret client; update
all callers (those that call storeMCPCredentials and the other helpers covering
the ranges ~306-313, ~331-339, ~365-366) to obtain the secret client from
GetK8sClientsForRequest(c) and pass it through so every Secret read/write is
performed with the request-scoped client.
- Around line 313-316: getMCPCredentials() currently propagates a Kubernetes
NotFound error from K8sClient.CoreV1().Secrets(...).Get which causes
GetMCPCredentialsForSession() to return a 500; treat a missing Secret as "not
configured" by detecting a NotFound error (errors.IsNotFound(err) /
k8serrors.IsNotFound(err)) and return (nil, nil) instead of the error. Update
the getMCPCredentials function to check the Get() error, return nil,nil for
NotFound, and only return actual errors for other failure cases so
GetMCPCredentialsForSession() follows the normal no-credentials path.
- Around line 45-46: mcpSecretKey currently uses a colon which is invalid for
Secret.data keys and several helpers use the package-level K8sClient instead of
request-scoped clients; change mcpSecretKey(serverName, userID) to build a key
using only allowed characters (e.g., join with "-" or "." and sanitize/escape
any disallowed chars) so it matches Kubernetes key rules, update
storeMCPCredentials, getMCPCredentials, deleteMCPCredentials, and
getMCPServerStatusForUser to accept and use a passed-in reqK8s (obtained from
GetK8sClientsForRequest(c)) instead of referencing K8sClient, and in
getMCPCredentials normalize the “Secret not found” path by returning a
not-found/empty credentials result (instead of propagating the raw error that
causes a 500) so it matches the existing key-not-found handling.
In `@components/frontend/src/components/mcp-credential-card.tsx`:
- Around line 194-200: The Cancel button closes the form with setShowForm(false)
but does not clear the form state, so sensitive data in fieldValues persists;
update the onClick handler for the Cancel button (the component using
setShowForm and fieldValues) to also reset/clear the form state (e.g., call the
state setter that controls fieldValues or reset to the initial form values and
clear any validation errors) before or after calling setShowForm(false), and
ensure this behavior respects connectMutation.isPending to avoid disrupting
in-flight requests.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 301-302: populate_mcp_server_credentials currently only sets MCP_*
env vars and can leave stale values around; before calling
populate_mcp_server_credentials (the call next to populate_runtime_credentials
in the bridge startup), clear existing environment variables whose names start
with "MCP_" so revoked/removed fields are removed from os.environ, then call
populate_mcp_server_credentials to repopulate; ensure check_mcp_authentication
reads the refreshed os.environ values after this change.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py`:
- Around line 310-321: The code performs blocking file I/O inside the async flow
using config_path.exists() and open(config_path, "r"); change these to run on a
thread to avoid blocking the event loop by wrapping the blocking calls with
asyncio.to_thread (or switch to aiofiles) — check the mcp_config_file /
config_path existence and read the file contents via asyncio.to_thread and then
_json.load on the resulting string, then parse mcp_servers from the loaded
config; keep the same error handling and logger.warning behavior around the
try/except that currently surrounds the open/_json.load operations.
- Around line 350-352: The loop that sets environment variables currently
assigns raw field_value to os.environ which can raise TypeError for non-string
values; update the loop that iterates over fields.items() (using sanitized_name
to build env_key) to convert each field_value to a string before assignment
(e.g., env_value = "" if field_value is None else str(field_value)) and then set
os.environ[env_key] = env_value so all environment values are guaranteed to be
strings.
In `@specs/1-add-mcp-tools/plan.md`:
- Around line 191-205: The test expectation uses the wrong env var name — adjust
the test table to match the env var format produced by
populate_mcp_server_credentials: MCP_{server_name.upper().replace('-',
'_')}_{field_name.upper()}; for server_name "test-server" the correct variable
is MCP_TEST_SERVER_APIKEY, so update the assertions/expected values that
reference the old name (including the other occurrences referenced in the
comment) to MCP_TEST_SERVER_APIKEY.
In `@specs/1-add-mcp-tools/research.md`:
- Around line 100-104: The fenced code block under "Decision 8: Env var naming
convention for credential injection" that contains the example
"MCP_{UPPERCASED_SERVER_NAME}_{UPPERCASED_FIELD_NAME}" is missing a language
identifier; update that fenced block (the one showing
MCP_{UPPERCASED_SERVER_NAME}_{UPPERCASED_FIELD_NAME}) to include a language
specifier such as "text" or "bash" (e.g., ```text) so tooling and syntax
highlighting recognize it.
In `@specs/1-add-mcp-tools/spec.md`:
- Line 207: The mitigation text currently suggests "One secret per server" which
conflicts with Decision 6's chosen approach of a single Secret named
mcp-server-credentials; update the mitigation to align with Decision 6 by
replacing or augmenting the option with "Use a single Kubernetes Secret named
mcp-server-credentials for all MCP servers (or, if needed, partition by server
name when exceeding 1MB)" and mention the Decision 6 reference so the spec and
research.md are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8579eb45-2c92-4130-ba3d-5b14cbefc61a
⛔ Files ignored due to path filters (2)
components/frontend/public/logos/context7.svgis excluded by!**/*.svgcomponents/frontend/public/logos/deepwiki.pngis excluded by!**/*.png
📒 Files selected for processing (18)
components/backend/handlers/integrations_status.gocomponents/backend/handlers/mcp_credentials.gocomponents/backend/routes.gocomponents/frontend/src/app/integrations/IntegrationsClient.tsxcomponents/frontend/src/components/mcp-credential-card.tsxcomponents/frontend/src/components/navigation.tsxcomponents/frontend/src/services/api/integrations.tscomponents/frontend/src/services/api/mcp-credentials.tscomponents/frontend/src/services/queries/use-mcp-credentials.tscomponents/runners/ambient-runner/.mcp.jsoncomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.pyspecs/1-add-mcp-tools/checklists/requirements.mdspecs/1-add-mcp-tools/plan.mdspecs/1-add-mcp-tools/research.mdspecs/1-add-mcp-tools/spec.mdspecs/1-add-mcp-tools/tasks.md
Comment on lines
+45
to
+46
| func mcpSecretKey(serverName, userID string) string { | ||
| return serverName + ":" + userID |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What characters are allowed in Kubernetes Secret.data keys in the core/v1 Secret API?
💡 Result:
In the core/v1 Secret API, the keys in Secret.data (and Secret.stringData) must consist only of:
- Alphanumeric characters (
[A-Za-z0-9]) - Hyphen (
-) - Underscore (
_) - Dot (
.)
Sources: Kubernetes docs and the Go API type comment for Secret.Data. [1] [2]
🏁 Script executed:
cat -n components/backend/handlers/mcp_credentials.go | head -100Repository: ambient-code/platform
Length of output: 3623
🏁 Script executed:
cat -n components/backend/handlers/mcp_credentials.go | tail -200Repository: ambient-code/platform
Length of output: 7927
Secret key format violates Kubernetes API specification and token auth must use request-scoped clients.
The colon (:) in mcpSecretKey() is not allowed in core/v1.Secret.data keys (only alphanumeric, hyphen, underscore, dot are valid). Additionally, storeMCPCredentials(), getMCPCredentials(), deleteMCPCredentials(), and getMCPServerStatusForUser() use the package-level backend service account (K8sClient) instead of passing request-scoped clients from GetK8sClientsForRequest(c), violating the requirement that user-facing API operations must use token auth. Finally, getMCPCredentials() returns a raw error when the Secret doesn't exist (line 313–316), causing a 500 response on first session fetch; it should normalize this to a "not found" state matching the key-not-found path (line 318–319).
Suggested fix
import (
"context"
+ "encoding/base64"
"encoding/json"
"fmt"
"log"
@@
func mcpSecretKey(serverName, userID string) string {
- return serverName + ":" + userID
+ encodedUserID := base64.RawURLEncoding.EncodeToString([]byte(userID))
+ return serverName + "." + encodedUserID
}
@@
- suffix := ":" + userID
+ suffix := "." + base64.RawURLEncoding.EncodeToString([]byte(userID))Also requires passing request-scoped clients (reqK8s) to helper functions and normalizing missing-Secret errors.
Also applies to: 371–375, 249–303, 305–328, 330–362, 364–382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/mcp_credentials.go` around lines 45 - 46,
mcpSecretKey currently uses a colon which is invalid for Secret.data keys and
several helpers use the package-level K8sClient instead of request-scoped
clients; change mcpSecretKey(serverName, userID) to build a key using only
allowed characters (e.g., join with "-" or "." and sanitize/escape any
disallowed chars) so it matches Kubernetes key rules, update
storeMCPCredentials, getMCPCredentials, deleteMCPCredentials, and
getMCPServerStatusForUser to accept and use a passed-in reqK8s (obtained from
GetK8sClientsForRequest(c)) instead of referencing K8sClient, and in
getMCPCredentials normalize the “Secret not found” path by returning a
not-found/empty credentials result (instead of propagating the raw error that
causes a 500) so it matches the existing key-not-found handling.
Comment on lines
+249
to
+257
| func storeMCPCredentials(ctx context.Context, creds *MCPServerCredentials) error { | ||
| if creds == nil || creds.UserID == "" || creds.ServerName == "" { | ||
| return fmt.Errorf("invalid credentials payload") | ||
| } | ||
|
|
||
| key := mcpSecretKey(creds.ServerName, creds.UserID) | ||
|
|
||
| for i := 0; i < 3; i++ { | ||
| secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, mcpCredentialsSecretName, v1.GetOptions{}) |
There was a problem hiding this comment.
Do not switch these user-facing flows back to the backend service account.
The handlers authenticate with GetK8sClientsForRequest(c), but every Secret read/write in these helpers goes through the package-level K8sClient. That turns MCP credential CRUD and session fetch into backend-privileged operations instead of request-scoped ones. Pass the request-scoped Secret client into these helpers and use it end-to-end. Based on learnings: not applicable. As per coding guidelines "All user-facing API operations must use GetK8sClientsForRequest(c) for token auth, never the backend service account".
Also applies to: 306-313, 331-339, 365-366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/mcp_credentials.go` around lines 249 - 257, The
helper functions (starting with storeMCPCredentials) are using the package-level
K8sClient which elevates requests to the backend service account; change these
helpers to accept a request-scoped Secret client (the Kubernetes
corev1.SecretInterface) as a parameter and replace all uses of
K8sClient.CoreV1().Secrets(Namespace) with that injected secret client; update
all callers (those that call storeMCPCredentials and the other helpers covering
the ranges ~306-313, ~331-339, ~365-366) to obtain the secret client from
GetK8sClientsForRequest(c) and pass it through so every Secret read/write is
performed with the request-scoped client.
Comment on lines
+313
to
+316
| secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, mcpCredentialsSecretName, v1.GetOptions{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Treat a missing backing Secret as “not configured.”
getMCPCredentials() only returns nil when the entry is absent inside an existing Secret. On a fresh cluster, Secrets().Get() returns NotFound, which GetMCPCredentialsForSession() currently turns into a 500 instead of the normal no-credentials path.
Suggested fix
secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, mcpCredentialsSecretName, v1.GetOptions{})
if err != nil {
- return nil, err
+ if errors.IsNotFound(err) {
+ return nil, nil
+ }
+ return nil, fmt.Errorf("failed to get Secret %q: %w", mcpCredentialsSecretName, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, mcpCredentialsSecretName, v1.GetOptions{}) | |
| if err != nil { | |
| return nil, err | |
| } | |
| secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, mcpCredentialsSecretName, v1.GetOptions{}) | |
| if err != nil { | |
| if errors.IsNotFound(err) { | |
| return nil, nil | |
| } | |
| return nil, fmt.Errorf("failed to get Secret %q: %w", mcpCredentialsSecretName, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/mcp_credentials.go` around lines 313 - 316,
getMCPCredentials() currently propagates a Kubernetes NotFound error from
K8sClient.CoreV1().Secrets(...).Get which causes GetMCPCredentialsForSession()
to return a 500; treat a missing Secret as "not configured" by detecting a
NotFound error (errors.IsNotFound(err) / k8serrors.IsNotFound(err)) and return
(nil, nil) instead of the error. Update the getMCPCredentials function to check
the Get() error, return nil,nil for NotFound, and only return actual errors for
other failure cases so GetMCPCredentialsForSession() follows the normal
no-credentials path.
Comment on lines
+194
to
+200
| <Button | ||
| variant="outline" | ||
| onClick={() => setShowForm(false)} | ||
| disabled={connectMutation.isPending} | ||
| > | ||
| Cancel | ||
| </Button> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clear form state on cancel to avoid retaining sensitive input.
When the user cancels the form, fieldValues retains the entered data (including potential API keys). If the form is reopened, the previous values remain visible. Consider clearing the state on cancel for better security hygiene.
🔒 Proposed fix
<Button
variant="outline"
- onClick={() => setShowForm(false)}
+ onClick={() => {
+ setShowForm(false)
+ setFieldValues({})
+ }}
disabled={connectMutation.isPending}
>
Cancel
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button | |
| variant="outline" | |
| onClick={() => setShowForm(false)} | |
| disabled={connectMutation.isPending} | |
| > | |
| Cancel | |
| </Button> | |
| <Button | |
| variant="outline" | |
| onClick={() => { | |
| setShowForm(false) | |
| setFieldValues({}) | |
| }} | |
| disabled={connectMutation.isPending} | |
| > | |
| Cancel | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/components/mcp-credential-card.tsx` around lines 194
- 200, The Cancel button closes the form with setShowForm(false) but does not
clear the form state, so sensitive data in fieldValues persists; update the
onClick handler for the Cancel button (the component using setShowForm and
fieldValues) to also reset/clear the form state (e.g., call the state setter
that controls fieldValues or reset to the initial form values and clear any
validation errors) before or after calling setShowForm(false), and ensure this
behavior respects connectMutation.isPending to avoid disrupting in-flight
requests.
Comment on lines
301
to
+302
| await populate_runtime_credentials(self._context) | ||
| await populate_mcp_server_credentials(self._context) |
There was a problem hiding this comment.
Clear stale MCP_* vars before refreshing credentials.
populate_mcp_server_credentials() only adds MCP_{SERVER}_* keys to os.environ. Once this call runs, disconnecting an MCP integration or removing a field leaves the old values live for the lifetime of the runner, so later runs can still authenticate with revoked credentials and check_mcp_authentication() will keep reporting the server as configured.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`
around lines 301 - 302, populate_mcp_server_credentials currently only sets
MCP_* env vars and can leave stale values around; before calling
populate_mcp_server_credentials (the call next to populate_runtime_credentials
in the bridge startup), clear existing environment variables whose names start
with "MCP_" so revoked/removed fields are removed from os.environ, then call
populate_mcp_server_credentials to repopulate; ensure check_mcp_authentication
reads the refreshed os.environ values after this change.
Comment on lines
+310
to
+321
| mcp_config_file = os.getenv("MCP_CONFIG_FILE", "/app/ambient-runner/.mcp.json") | ||
| config_path = Path(mcp_config_file) | ||
| if not config_path.exists(): | ||
| return | ||
|
|
||
| try: | ||
| with open(config_path, "r") as f: | ||
| config = _json.load(f) | ||
| mcp_servers = config.get("mcpServers", {}) | ||
| except Exception as e: | ||
| logger.warning(f"Failed to read MCP config for credential population: {e}") | ||
| return |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Blocking file I/O in async function is acceptable here but could be improved.
Path.exists() and open() are blocking calls. Since this runs once at session startup (not on a hot path), the impact is minimal. However, for consistency with async patterns, consider using asyncio.to_thread() or aiofiles if this becomes a performance concern in the future.
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 312-312: Async functions should not use pathlib.Path methods, use trio.Path or anyio.path
(ASYNC240)
[warning] 316-316: Async functions should not open files with blocking methods like open
(ASYNC230)
[warning] 316-316: Unnecessary mode argument
Remove mode argument
(UP015)
[warning] 319-319: Do not catch blind exception: Exception
(BLE001)
[warning] 320-320: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py` around
lines 310 - 321, The code performs blocking file I/O inside the async flow using
config_path.exists() and open(config_path, "r"); change these to run on a thread
to avoid blocking the event loop by wrapping the blocking calls with
asyncio.to_thread (or switch to aiofiles) — check the mcp_config_file /
config_path existence and read the file contents via asyncio.to_thread and then
_json.load on the resulting string, then parse mcp_servers from the loaded
config; keep the same error handling and logger.warning behavior around the
try/except that currently surrounds the open/_json.load operations.
Comment on lines
+350
to
+352
| for field_name, field_value in fields.items(): | ||
| env_key = f"MCP_{sanitized_name}_{field_name.upper()}" | ||
| os.environ[env_key] = field_value |
There was a problem hiding this comment.
Ensure credential field values are strings before setting as environment variables.
os.environ requires string values. If the backend returns non-string field values (e.g., integers or booleans in JSON), this will raise a TypeError. Consider explicit string conversion.
🛡️ Proposed fix
for field_name, field_value in fields.items():
env_key = f"MCP_{sanitized_name}_{field_name.upper()}"
- os.environ[env_key] = field_value
+ os.environ[env_key] = str(field_value) if field_value is not None else ""
logger.info(f"Set {env_key} for MCP server {server_name}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for field_name, field_value in fields.items(): | |
| env_key = f"MCP_{sanitized_name}_{field_name.upper()}" | |
| os.environ[env_key] = field_value | |
| for field_name, field_value in fields.items(): | |
| env_key = f"MCP_{sanitized_name}_{field_name.upper()}" | |
| os.environ[env_key] = str(field_value) if field_value is not None else "" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py` around
lines 350 - 352, The loop that sets environment variables currently assigns raw
field_value to os.environ which can raise TypeError for non-string values;
update the loop that iterates over fields.items() (using sanitized_name to build
env_key) to convert each field_value to a string before assignment (e.g.,
env_value = "" if field_value is None else str(field_value)) and then set
os.environ[env_key] = env_value so all environment values are guaranteed to be
strings.
specs/1-add-mcp-tools/plan.md
Outdated
Comment on lines
+191
to
+205
| async def populate_mcp_server_credentials(context: RunnerContext, mcp_servers: dict): | ||
| """For each MCP server with env vars referencing MCP_* pattern, fetch and inject credentials.""" | ||
| for server_name, config in mcp_servers.items(): | ||
| env_block = config.get("env", {}) | ||
| # Find env vars that use ${MCP_*} pattern | ||
| mcp_vars = [v for v in env_block.values() if "${MCP_" in str(v)] | ||
| if not mcp_vars: | ||
| continue | ||
|
|
||
| try: | ||
| creds = await _fetch_mcp_credentials(context, server_name) | ||
| if creds and creds.get("fields"): | ||
| for field_name, field_value in creds["fields"].items(): | ||
| env_var = f"MCP_{server_name.upper().replace('-', '_')}_{field_name.upper()}" | ||
| os.environ[env_var] = field_value |
There was a problem hiding this comment.
The expected env var name in the test table does not match Step 4.
Step 4 builds MCP_{server_name.upper().replace('-', '_')}_{field_name.upper()}. For test-server, that becomes MCP_TEST_SERVER_APIKEY, so the expectation on Line 293 is currently pointing at a different variable.
Also applies to: 291-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/1-add-mcp-tools/plan.md` around lines 191 - 205, The test expectation
uses the wrong env var name — adjust the test table to match the env var format
produced by populate_mcp_server_credentials:
MCP_{server_name.upper().replace('-', '_')}_{field_name.upper()}; for
server_name "test-server" the correct variable is MCP_TEST_SERVER_APIKEY, so
update the assertions/expected values that reference the old name (including the
other occurrences referenced in the comment) to MCP_TEST_SERVER_APIKEY.
specs/1-add-mcp-tools/research.md
Outdated
Comment on lines
+100
to
+104
| ### Decision 8: Env var naming convention for credential injection | ||
|
|
||
| **Decision**: Convention-based mapping: `MCP_{UPPERCASED_SERVER_NAME}_{UPPERCASED_FIELD_NAME}`. | ||
|
|
||
| Example: Server `context7` with field `apiKey` → env var `MCP_CONTEXT7_APIKEY`. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language specifier to fenced code block.
The code block starting at line 100 is missing a language identifier. This helps with syntax highlighting and documentation tooling.
-```
+```text
MCP_{UPPERCASED_SERVER_NAME}_{UPPERCASED_FIELD_NAME}🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 100-100: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/1-add-mcp-tools/research.md` around lines 100 - 104, The fenced code
block under "Decision 8: Env var naming convention for credential injection"
that contains the example "MCP_{UPPERCASED_SERVER_NAME}_{UPPERCASED_FIELD_NAME}"
is missing a language identifier; update that fenced block (the one showing
MCP_{UPPERCASED_SERVER_NAME}_{UPPERCASED_FIELD_NAME}) to include a language
specifier such as "text" or "bash" (e.g., ```text) so tooling and syntax
highlighting recognize it.
specs/1-add-mcp-tools/spec.md
Outdated
| |--------------------------------------------------------|------------|--------|---------------------------------------------------------| | ||
| | External service downtime blocks documentation lookups | Medium | Low | Agent falls back to training data; session not blocked | | ||
| | Network policies block outbound connections | Medium | High | Document network requirements; verify in staging | | ||
| | K8s Secret size limit (1MB) constrains credential count | Low | Medium | One secret per server, or partition by server name | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inconsistency with research.md on Secret structure.
The risk mitigation mentions "One secret per server" as an option, but research.md (Decision 6) chose a single Secret named mcp-server-credentials for all MCP servers. Consider aligning the documentation to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/1-add-mcp-tools/spec.md` at line 207, The mitigation text currently
suggests "One secret per server" which conflicts with Decision 6's chosen
approach of a single Secret named mcp-server-credentials; update the mitigation
to align with Decision 6 by replacing or augmenting the option with "Use a
single Kubernetes Secret named mcp-server-credentials for all MCP servers (or,
if needed, partition by server name when exceeding 1MB)" and mention the
Decision 6 reference so the spec and research.md are consistent.
Contributor
Review Queue — Blockers Found
|
syntaxsdev
force-pushed
the
1-add-mcp-tools
branch
from
102ab2d to
735997e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
components/backend/handlers/mcp_credentials.go (3)
313-316:⚠️ Potential issue | 🟠 MajorTreat a missing backing Secret as “not configured”.
On a fresh cluster,
Secrets().Get()returnsNotFoundhere, which makes first-use status/session fetches fail with a 500 instead of the normal disconnected path. NormalizeNotFoundto(nil, nil).Suggested fix
secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, mcpCredentialsSecretName, v1.GetOptions{}) if err != nil { - return nil, err + if errors.IsNotFound(err) { + return nil, nil + } + return nil, fmt.Errorf("failed to get Secret %q: %w", mcpCredentialsSecretName, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/mcp_credentials.go` around lines 313 - 316, The current call to K8sClient.CoreV1().Secrets(Namespace).Get(...) treats a missing Secret as an error; change the error handling in the function containing that Get call so that if the error is a Kubernetes NotFound (use errors.IsNotFound from k8s.io/apimachinery/pkg/api/errors) you return (nil, nil) to indicate "not configured", otherwise return the original error; also add the import for the k8s API errors package if missing and reference Namespace and mcpCredentialsSecretName in the updated handling.
45-46:⚠️ Potential issue | 🔴 CriticalUse a Secret key encoding that Kubernetes accepts.
mcpSecretKey()writesserverName:userIDdirectly intoSecret.Data.:is not a valid Secret data key character, and raw user IDs commonly contain characters like@, so storing credentials will fail for normal users. Encode the user ID and switch to a safe separator, then update the suffix scan ingetMCPServerStatusForUser()to match.What characters are allowed in Kubernetes Secret.data keys in the core/v1 Secret API?Suggested fix
import ( "context" + "encoding/base64" "encoding/json" "fmt" @@ func mcpSecretKey(serverName, userID string) string { - return serverName + ":" + userID + encodedUserID := base64.RawURLEncoding.EncodeToString([]byte(userID)) + return serverName + "." + encodedUserID }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/mcp_credentials.go` around lines 45 - 46, mcpSecretKey currently concatenates serverName + ":" + userID which creates Kubernetes Secret.Data keys with invalid characters (":" and raw user IDs like "@"); update mcpSecretKey to encode the userID using a Kubernetes-safe encoding (e.g., base64 URL-safe or hex) and use a safe separator (e.g., "-" or "__") between serverName and the encoded userID, and then update getMCPServerStatusForUser to look for the new suffix/prefix form and decode the userID accordingly when scanning Secret keys (adjust any suffix scan logic to match the chosen separator and decoding scheme).
249-257:⚠️ Potential issue | 🟠 MajorStop routing credential CRUD through the backend service account.
The handlers already authenticate with
GetK8sClientsForRequest(c), but every Secret read/write below still uses the package-levelK8sClient. That bypasses request-scoped RBAC for user-facing MCP credential operations. Pass the request-scoped Secret client intostoreMCPCredentials,getMCPCredentials,deleteMCPCredentials, andgetMCPServerStatusForUser, and use it end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/mcp_credentials.go` around lines 249 - 257, The credential handlers (storeMCPCredentials, getMCPCredentials, deleteMCPCredentials, getMCPServerStatusForUser) currently call the package-level K8sClient and bypass request-scoped RBAC; change each function signature to accept and use the request-scoped Secret client returned by GetK8sClientsForRequest(c) (e.g. the CoreV1().Secrets(Namespace) SecretInterface) and replace all uses of K8sClient and its Secret calls (including where mcpCredentialsSecretName is read/updated) with that injected Secret client; update all call sites in the handlers to obtain the request-scoped client via GetK8sClientsForRequest(c) and pass it through so every Secret read/write is performed with the request-scoped Secret client.components/runners/ambient-runner/ambient_runner/platform/auth.py (1)
338-355:⚠️ Potential issue | 🟠 MajorClear stale per-server
MCP_*vars before repopulating.This helper only overwrites fields that still exist. If a user disconnects an MCP integration or removes one field, the old
MCP_{SERVER}_*entries stay inos.environfor the rest of the runner lifetime, so later runs can keep authenticating with revoked credentials andcheck_mcp_authentication()will still report the server as configured.Suggested fix
try: + sanitized_name = server_name.upper().replace("-", "_") + prefix = f"MCP_{sanitized_name}_" + for env_key in [k for k in os.environ if k.startswith(prefix)]: + del os.environ[env_key] + data = await _fetch_mcp_credentials(context, server_name) fields = data.get("fields", {}) if not fields: logger.warning( f"No MCP credentials found for server {server_name} — " f"tools requiring auth may not work" ) continue # Set env vars using convention: MCP_{SERVER_NAME}_{FIELD_NAME} - sanitized_name = server_name.upper().replace("-", "_") for field_name, field_value in fields.items(): env_key = f"MCP_{sanitized_name}_{field_name.upper()}" os.environ[env_key] = field_value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/platform/auth.py` around lines 338 - 355, Before repopulating per-server MCP env vars, clear any stale MCP_{SERVER}_* entries for that server so removed fields don't linger; in the block that calls _fetch_mcp_credentials (and in the branch where fields is empty), compute sanitized_name = server_name.upper().replace("-", "_") and remove all existing os.environ keys that start with f"MCP_{sanitized_name}_" (use os.environ.pop(key, None) or del if present) before setting new env vars, and keep the existing logging behavior; this ensures check_mcp_authentication() won't see stale credentials.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/frontend/src/app/integrations/IntegrationsClient.tsx`:
- Around line 13-19: The FieldDefinition type is duplicated between
IntegrationsClient.tsx and mcp-credential-card.tsx; remove the duplicate and
centralize the definition by exporting type FieldDefinition from
mcp-credential-card.tsx (or a new shared types file like components/types.ts)
and then import that type into IntegrationsClient.tsx using an `import type {
FieldDefinition } from '...'` statement; update any usages in both files to
reference the shared type and delete the local duplicate declaration to prevent
drift.
---
Duplicate comments:
In `@components/backend/handlers/mcp_credentials.go`:
- Around line 313-316: The current call to
K8sClient.CoreV1().Secrets(Namespace).Get(...) treats a missing Secret as an
error; change the error handling in the function containing that Get call so
that if the error is a Kubernetes NotFound (use errors.IsNotFound from
k8s.io/apimachinery/pkg/api/errors) you return (nil, nil) to indicate "not
configured", otherwise return the original error; also add the import for the
k8s API errors package if missing and reference Namespace and
mcpCredentialsSecretName in the updated handling.
- Around line 45-46: mcpSecretKey currently concatenates serverName + ":" +
userID which creates Kubernetes Secret.Data keys with invalid characters (":"
and raw user IDs like "@"); update mcpSecretKey to encode the userID using a
Kubernetes-safe encoding (e.g., base64 URL-safe or hex) and use a safe separator
(e.g., "-" or "__") between serverName and the encoded userID, and then update
getMCPServerStatusForUser to look for the new suffix/prefix form and decode the
userID accordingly when scanning Secret keys (adjust any suffix scan logic to
match the chosen separator and decoding scheme).
- Around line 249-257: The credential handlers (storeMCPCredentials,
getMCPCredentials, deleteMCPCredentials, getMCPServerStatusForUser) currently
call the package-level K8sClient and bypass request-scoped RBAC; change each
function signature to accept and use the request-scoped Secret client returned
by GetK8sClientsForRequest(c) (e.g. the CoreV1().Secrets(Namespace)
SecretInterface) and replace all uses of K8sClient and its Secret calls
(including where mcpCredentialsSecretName is read/updated) with that injected
Secret client; update all call sites in the handlers to obtain the
request-scoped client via GetK8sClientsForRequest(c) and pass it through so
every Secret read/write is performed with the request-scoped Secret client.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py`:
- Around line 338-355: Before repopulating per-server MCP env vars, clear any
stale MCP_{SERVER}_* entries for that server so removed fields don't linger; in
the block that calls _fetch_mcp_credentials (and in the branch where fields is
empty), compute sanitized_name = server_name.upper().replace("-", "_") and
remove all existing os.environ keys that start with f"MCP_{sanitized_name}_"
(use os.environ.pop(key, None) or del if present) before setting new env vars,
and keep the existing logging behavior; this ensures check_mcp_authentication()
won't see stale credentials.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45e4b4f8-1c00-4977-82ce-2264bafe1b21
⛔ Files ignored due to path filters (2)
components/frontend/public/logos/context7.svgis excluded by!**/*.svgcomponents/frontend/public/logos/deepwiki.pngis excluded by!**/*.png
📒 Files selected for processing (13)
components/backend/handlers/integrations_status.gocomponents/backend/handlers/mcp_credentials.gocomponents/backend/routes.gocomponents/frontend/src/app/integrations/IntegrationsClient.tsxcomponents/frontend/src/components/mcp-credential-card.tsxcomponents/frontend/src/components/navigation.tsxcomponents/frontend/src/services/api/integrations.tscomponents/frontend/src/services/api/mcp-credentials.tscomponents/frontend/src/services/queries/use-mcp-credentials.tscomponents/runners/ambient-runner/.mcp.jsoncomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.py
Comment on lines
+13
to
+19
| type FieldDefinition = { | ||
| name: string | ||
| label: string | ||
| type: 'text' | 'password' | ||
| placeholder?: string | ||
| helpText?: string | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared FieldDefinition type.
This type is duplicated in mcp-credential-card.tsx (lines 16-22). Sharing the type definition would prevent drift and improve maintainability.
♻️ Suggested approach
Export FieldDefinition from mcp-credential-card.tsx and import it here, or create a shared types file:
// In mcp-credential-card.tsx or a shared types file
export type FieldDefinition = {
name: string
label: string
type: 'text' | 'password'
placeholder?: string
helpText?: string
}Then import in IntegrationsClient.tsx:
import type { FieldDefinition } from '@/components/mcp-credential-card'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/app/integrations/IntegrationsClient.tsx` around lines
13 - 19, The FieldDefinition type is duplicated between IntegrationsClient.tsx
and mcp-credential-card.tsx; remove the duplicate and centralize the definition
by exporting type FieldDefinition from mcp-credential-card.tsx (or a new shared
types file like components/types.ts) and then import that type into
IntegrationsClient.tsx using an `import type { FieldDefinition } from '...'`
statement; update any usages in both files to reference the shared type and
delete the local duplicate declaration to prevent drift.
syntaxsdev
force-pushed
the
1-add-mcp-tools
branch
from
735997e to
93b78fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
components/backend/handlers/mcp_credentials.go (2)
313-316:⚠️ Potential issue | 🟠 MajorNormalize a missing Secret to the no-credentials path.
If
mcp-server-credentialsdoes not exist yet, thisGet()returns NotFound andGetMCPCredentialsForSession()currently converts that into a 500. Fresh clusters should treat this as “not configured” and returnnil, nilhere instead.🩹 Proposed fix
secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, mcpCredentialsSecretName, v1.GetOptions{}) if err != nil { - return nil, err + if errors.IsNotFound(err) { + return nil, nil + } + return nil, fmt.Errorf("failed to get Secret %q: %w", mcpCredentialsSecretName, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/mcp_credentials.go` around lines 313 - 316, GetMCPCredentialsForSession currently treats a missing Kubernetes Secret as an error; change the error handling after calling K8sClient.CoreV1().Secrets(Namespace).Get(...) for mcpCredentialsSecretName so that if the returned error is a NotFound (use k8serrors.IsNotFound or equivalent) the function returns nil, nil to represent “no credentials configured”, otherwise return the original error; keep the rest of the function logic unchanged.
45-46:⚠️ Potential issue | 🔴 CriticalUse a Secret-safe storage key.
mcpSecretKey()currently generatesserverName:userID. That key shape is invalid forSecret.data, so the first MCP credential write can be rejected before anything is persisted. Encode or hash the user id into an allowed alphabet and keep the suffix lookup ingetMCPServerStatusForUser()aligned with the same format.🔧 Suggested direction
+import "encoding/base64" + func mcpSecretKey(serverName, userID string) string { - return serverName + ":" + userID + return serverName + "." + base64.RawURLEncoding.EncodeToString([]byte(userID)) }Apply the same encoded suffix in
getMCPServerStatusForUser().In Kubernetes core/v1 Secret API, what characters are allowed in Secret.data keys?Also applies to: 371-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/mcp_credentials.go` around lines 45 - 46, mcpSecretKey currently returns "serverName:userID" which contains characters invalid for Kubernetes Secret.data keys; change mcpSecretKey to produce a Secret-safe suffix by encoding or hashing userID (e.g., hex or URL-safe base64 or sha256-hex) and concatenate that to serverName with an allowed separator, and update getMCPServerStatusForUser() to use the identical encoding/decoding logic so lookups match the stored key format; ensure the chosen encoding only uses characters allowed by Secret.data keys (letters, digits, '_', '-', '.').components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py (1)
300-303:⚠️ Potential issue | 🟠 MajorStale MCP credentials persist after disconnection.
populate_mcp_server_credentials()only setsMCP_*environment variables but never clears them. If a user disconnects an MCP integration, the old credentials remain inos.environfor the runner's lifetime, allowing continued authentication with revoked credentials.Additionally,
_refresh_credentials_if_stale()(context snippet 4) only callspopulate_runtime_credentials, notpopulate_mcp_server_credentials, so MCP credentials are never refreshed during the session.🔒 Proposed fix - clear stale MCP vars before population
# Populate credentials before building system prompt (prompt checks env vars) await populate_runtime_credentials(self._context) + # Clear stale MCP credentials before repopulating + for key in list(os.environ.keys()): + if key.startswith("MCP_") and key not in ("MCP_CONFIG_FILE",): + del os.environ[key] await populate_mcp_server_credentials(self._context)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py` around lines 300 - 303, populate_mcp_server_credentials currently only sets MCP_* env vars and never clears them, causing stale/revoked MCP credentials to persist; update populate_mcp_server_credentials to first clear any existing MCP_* keys from os.environ (e.g., remove keys with prefix "MCP_") before writing new values, and update _refresh_credentials_if_stale to call populate_mcp_server_credentials in addition to populate_runtime_credentials so MCP credentials are refreshed during the session; reference the functions populate_mcp_server_credentials and _refresh_credentials_if_stale and ensure you update the call site where populate_runtime_credentials is invoked so both credential population functions run and old MCP env vars are removed prior to setting new ones.components/frontend/src/components/mcp-credential-card.tsx (1)
194-200:⚠️ Potential issue | 🟡 MinorClear form state on cancel to prevent sensitive data persistence.
The Cancel button closes the form but doesn't clear
fieldValues, leaving entered credentials (API keys, tokens) in memory. If the form is reopened, previous values remain visible.🔒 Proposed fix
<Button variant="outline" - onClick={() => setShowForm(false)} + onClick={() => { + setShowForm(false) + setFieldValues({}) + }} disabled={connectMutation.isPending} > Cancel </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/components/mcp-credential-card.tsx` around lines 194 - 200, The Cancel button currently only calls setShowForm(false) leaving sensitive form state in memory; update the onClick handler used on the Button (the one that also checks connectMutation.isPending) to clear fieldValues by calling setFieldValues back to the initial/empty state (and clear any related validation/errors) before or after calling setShowForm(false) so reopened forms do not show previous API keys/tokens.components/frontend/src/app/integrations/IntegrationsClient.tsx (1)
13-27: 🧹 Nitpick | 🔵 TrivialConsider extracting shared types.
FieldDefinitionis duplicated inmcp-credential-card.tsx. Sharing the type definition would prevent drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/integrations/IntegrationsClient.tsx` around lines 13 - 27, Extract the duplicated FieldDefinition type into a single shared types module and update both IntegrationsClient.tsx and mcp-credential-card.tsx to import it instead of redefining it: create and export a shared type (e.g., export type FieldDefinition = { name: string; label: string; type: 'text' | 'password'; placeholder?: string; helpText?: string }) alongside any related types (you can export MCPCredentialServer or keep it local but reference FieldDefinition), remove the duplicate definition in mcp-credential-card.tsx, update imports to reference the new shared symbol FieldDefinition, and run the TypeScript type-check to ensure no references remain to the old duplicate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/mcp_credentials.go`:
- Around line 249-303: storeMCPCredentials currently packs all users' MCP
credentials into a single Secret (mcpCredentialsSecretName) causing global
conflicts and O(N) reads; change to shard storage per (server,user) by using
mcpSecretKey(creds.ServerName, creds.UserID) as the Secret name (or append it to
mcpCredentialsSecretName) and store the marshaled creds as the Secret.Data (not
as an entry in a shared map). Update the flow in storeMCPCredentials: compute
per-secret name via mcpSecretKey, Create/Get that single Secret (with same
labels/Type), set Secret.Data to the marshaled creds (or a single key like
"credentials"), and Update; preserve retry-on-conflict logic. Also update
getMCPServerStatusForUser (and any other reader) to read the per-(server,user)
Secret by the same mcpSecretKey instead of scanning the large shared secret.
In `@components/runners/ambient-runner/.mcp.json`:
- Around line 3-10: Remove "context7" and "deepwiki" from the default .mcp.json
entries and instead gate them behind an explicit opt-in admin/config flag (e.g.,
a feature flag or ADMIN_ENABLE_THIRD_PARTY_MCP setting). Update the runner
initialization code that reads .mcp.json to only load these two entries when the
opt-in flag is true, or move their definitions into a separate opt-in config
object (named e.g., "thirdPartyMCP" or "optInMCP") and document that the runtime
will only include "context7" and "deepwiki" when the admin flag is enabled;
ensure the keys "context7" and "deepwiki" are the referenced symbols so
reviewers can locate and verify the change.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py`:
- Around line 323-353: The MCP credential population loop is never triggered
because it only looks for ${MCP_*} placeholders (mcp_env_pattern and
needs_creds) but your .mcp.json uses JIRA_* and GOOGLE_* keys, so the exported
MCP_{SERVER}_{FIELD} env vars (env_key built from sanitized_name) are never
created/consumed; to fix, change the detection and mapping: for each server in
mcp_servers iterate env_block and, instead of requiring ${MCP_*}, support
explicit per-server mappings (e.g., map JIRA_* or GOOGLE_* names to returned
fields) or accept non-braced keys by adjusting the regex/condition, then call
_fetch_mcp_credentials and export each retrieved field under the correct target
names (use sanitized_name, field_name) so the runner will receive the
JIRA_*/GOOGLE_* variables expected by mcp-atlassian.
---
Duplicate comments:
In `@components/backend/handlers/mcp_credentials.go`:
- Around line 313-316: GetMCPCredentialsForSession currently treats a missing
Kubernetes Secret as an error; change the error handling after calling
K8sClient.CoreV1().Secrets(Namespace).Get(...) for mcpCredentialsSecretName so
that if the returned error is a NotFound (use k8serrors.IsNotFound or
equivalent) the function returns nil, nil to represent “no credentials
configured”, otherwise return the original error; keep the rest of the function
logic unchanged.
- Around line 45-46: mcpSecretKey currently returns "serverName:userID" which
contains characters invalid for Kubernetes Secret.data keys; change mcpSecretKey
to produce a Secret-safe suffix by encoding or hashing userID (e.g., hex or
URL-safe base64 or sha256-hex) and concatenate that to serverName with an
allowed separator, and update getMCPServerStatusForUser() to use the identical
encoding/decoding logic so lookups match the stored key format; ensure the
chosen encoding only uses characters allowed by Secret.data keys (letters,
digits, '_', '-', '.').
In `@components/frontend/src/app/integrations/IntegrationsClient.tsx`:
- Around line 13-27: Extract the duplicated FieldDefinition type into a single
shared types module and update both IntegrationsClient.tsx and
mcp-credential-card.tsx to import it instead of redefining it: create and export
a shared type (e.g., export type FieldDefinition = { name: string; label:
string; type: 'text' | 'password'; placeholder?: string; helpText?: string })
alongside any related types (you can export MCPCredentialServer or keep it local
but reference FieldDefinition), remove the duplicate definition in
mcp-credential-card.tsx, update imports to reference the new shared symbol
FieldDefinition, and run the TypeScript type-check to ensure no references
remain to the old duplicate.
In `@components/frontend/src/components/mcp-credential-card.tsx`:
- Around line 194-200: The Cancel button currently only calls setShowForm(false)
leaving sensitive form state in memory; update the onClick handler used on the
Button (the one that also checks connectMutation.isPending) to clear fieldValues
by calling setFieldValues back to the initial/empty state (and clear any related
validation/errors) before or after calling setShowForm(false) so reopened forms
do not show previous API keys/tokens.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 300-303: populate_mcp_server_credentials currently only sets MCP_*
env vars and never clears them, causing stale/revoked MCP credentials to
persist; update populate_mcp_server_credentials to first clear any existing
MCP_* keys from os.environ (e.g., remove keys with prefix "MCP_") before writing
new values, and update _refresh_credentials_if_stale to call
populate_mcp_server_credentials in addition to populate_runtime_credentials so
MCP credentials are refreshed during the session; reference the functions
populate_mcp_server_credentials and _refresh_credentials_if_stale and ensure you
update the call site where populate_runtime_credentials is invoked so both
credential population functions run and old MCP env vars are removed prior to
setting new ones.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63dd4224-478d-40cc-8dc7-7f6b3f8141f4
⛔ Files ignored due to path filters (2)
components/frontend/public/logos/context7.svgis excluded by!**/*.svgcomponents/frontend/public/logos/deepwiki.pngis excluded by!**/*.png
📒 Files selected for processing (13)
components/backend/handlers/integrations_status.gocomponents/backend/handlers/mcp_credentials.gocomponents/backend/routes.gocomponents/frontend/src/app/integrations/IntegrationsClient.tsxcomponents/frontend/src/components/mcp-credential-card.tsxcomponents/frontend/src/components/navigation.tsxcomponents/frontend/src/services/api/integrations.tscomponents/frontend/src/services/api/mcp-credentials.tscomponents/frontend/src/services/queries/use-mcp-credentials.tscomponents/runners/ambient-runner/.mcp.jsoncomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.py
Comment on lines
+249
to
+303
| func storeMCPCredentials(ctx context.Context, creds *MCPServerCredentials) error { | ||
| if creds == nil || creds.UserID == "" || creds.ServerName == "" { | ||
| return fmt.Errorf("invalid credentials payload") | ||
| } | ||
|
|
||
| key := mcpSecretKey(creds.ServerName, creds.UserID) | ||
|
|
||
| for i := 0; i < 3; i++ { | ||
| secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, mcpCredentialsSecretName, v1.GetOptions{}) | ||
| if err != nil { | ||
| if errors.IsNotFound(err) { | ||
| secret = &corev1.Secret{ | ||
| ObjectMeta: v1.ObjectMeta{ | ||
| Name: mcpCredentialsSecretName, | ||
| Namespace: Namespace, | ||
| Labels: map[string]string{ | ||
| "app": "ambient-code", | ||
| "ambient-code.io/provider": "mcp", | ||
| }, | ||
| }, | ||
| Type: corev1.SecretTypeOpaque, | ||
| Data: map[string][]byte{}, | ||
| } | ||
| if _, cerr := K8sClient.CoreV1().Secrets(Namespace).Create(ctx, secret, v1.CreateOptions{}); cerr != nil && !errors.IsAlreadyExists(cerr) { | ||
| return fmt.Errorf("failed to create Secret: %w", cerr) | ||
| } | ||
| secret, err = K8sClient.CoreV1().Secrets(Namespace).Get(ctx, mcpCredentialsSecretName, v1.GetOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to fetch Secret after create: %w", err) | ||
| } | ||
| } else { | ||
| return fmt.Errorf("failed to get Secret: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if secret.Data == nil { | ||
| secret.Data = map[string][]byte{} | ||
| } | ||
|
|
||
| b, err := json.Marshal(creds) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal credentials: %w", err) | ||
| } | ||
| secret.Data[key] = b | ||
|
|
||
| if _, uerr := K8sClient.CoreV1().Secrets(Namespace).Update(ctx, secret, v1.UpdateOptions{}); uerr != nil { | ||
| if errors.IsConflict(uerr) { | ||
| continue | ||
| } | ||
| return fmt.Errorf("failed to update Secret: %w", uerr) | ||
| } | ||
| return nil | ||
| } | ||
| return fmt.Errorf("failed to update Secret after retries") | ||
| } |
There was a problem hiding this comment.
Do not pack every user's MCP credentials into one Secret.
Every connect/disconnect rewrites the same mcp-server-credentials object, and getMCPServerStatusForUser() later reads and scans that whole object to answer one user's Integrations page. That makes the write path a global conflict hotspot and turns status lookup into O(total stored MCP credentials). Please shard storage per user or per (server,user).
Also applies to: 365-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/mcp_credentials.go` around lines 249 - 303,
storeMCPCredentials currently packs all users' MCP credentials into a single
Secret (mcpCredentialsSecretName) causing global conflicts and O(N) reads;
change to shard storage per (server,user) by using
mcpSecretKey(creds.ServerName, creds.UserID) as the Secret name (or append it to
mcpCredentialsSecretName) and store the marshaled creds as the Secret.Data (not
as an entry in a shared map). Update the flow in storeMCPCredentials: compute
per-secret name via mcpSecretKey, Create/Get that single Secret (with same
labels/Type), set Secret.Data to the marshaled creds (or a single key like
"credentials"), and Update; preserve retry-on-conflict logic. Also update
getMCPServerStatusForUser (and any other reader) to read the per-(server,user)
Secret by the same mcpSecretKey instead of scanning the large shared secret.
Comment on lines
+3
to
+10
| "context7": { | ||
| "type": "http", | ||
| "url": "https://mcp.context7.com/mcp" | ||
| }, | ||
| "deepwiki": { | ||
| "type": "http", | ||
| "url": "https://mcp.deepwiki.com/mcp" | ||
| }, |
There was a problem hiding this comment.
Gate these third-party MCP servers behind explicit opt-in.
Adding context7 and deepwiki to the default runner config enables outbound tool traffic to external services in every workspace. That is a data-egress/compliance change, not just a local UI feature. Please make these servers admin-configurable or feature-flagged before shipping them as defaults.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/.mcp.json` around lines 3 - 10, Remove
"context7" and "deepwiki" from the default .mcp.json entries and instead gate
them behind an explicit opt-in admin/config flag (e.g., a feature flag or
ADMIN_ENABLE_THIRD_PARTY_MCP setting). Update the runner initialization code
that reads .mcp.json to only load these two entries when the opt-in flag is
true, or move their definitions into a separate opt-in config object (named
e.g., "thirdPartyMCP" or "optInMCP") and document that the runtime will only
include "context7" and "deepwiki" when the admin flag is enabled; ensure the
keys "context7" and "deepwiki" are the referenced symbols so reviewers can
locate and verify the change.
Comment on lines
+323
to
+353
| mcp_env_pattern = re.compile(r"\$\{(MCP_[A-Z0-9_]+)") | ||
|
|
||
| for server_name, server_config in mcp_servers.items(): | ||
| env_block = server_config.get("env", {}) | ||
| if not env_block: | ||
| continue | ||
|
|
||
| # Check if any env value references ${MCP_*} pattern | ||
| needs_creds = any( | ||
| isinstance(v, str) and mcp_env_pattern.search(v) | ||
| for v in env_block.values() | ||
| ) | ||
| if not needs_creds: | ||
| continue | ||
|
|
||
| try: | ||
| data = await _fetch_mcp_credentials(context, server_name) | ||
| fields = data.get("fields", {}) | ||
| if not fields: | ||
| logger.warning( | ||
| f"No MCP credentials found for server {server_name} — " | ||
| f"tools requiring auth may not work" | ||
| ) | ||
| continue | ||
|
|
||
| # Set env vars using convention: MCP_{SERVER_NAME}_{FIELD_NAME} | ||
| sanitized_name = server_name.upper().replace("-", "_") | ||
| for field_name, field_value in fields.items(): | ||
| env_key = f"MCP_{sanitized_name}_{field_name.upper()}" | ||
| os.environ[env_key] = field_value | ||
| logger.info(f"Set {env_key} for MCP server {server_name}") |
There was a problem hiding this comment.
The new MCP credential population path is unreachable with the current config.
This loop only activates when an env value contains ${MCP_*}, but components/runners/ambient-runner/.mcp.json currently uses JIRA_* and GOOGLE_* placeholders and contains no ${MCP_*} values. needs_creds stays false for every configured server, and the later MCP_{SERVER}_{FIELD} exports would not satisfy mcp-atlassian's JIRA_* env names anyway. As written, the new MCP credential store is never consumed by the runner. Either align .mcp.json to the generic convention or add an explicit per-server env mapping here.
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 343-344: Logging statement uses f-string
(G004)
[warning] 353-353: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py` around
lines 323 - 353, The MCP credential population loop is never triggered because
it only looks for ${MCP_*} placeholders (mcp_env_pattern and needs_creds) but
your .mcp.json uses JIRA_* and GOOGLE_* keys, so the exported
MCP_{SERVER}_{FIELD} env vars (env_key built from sanitized_name) are never
created/consumed; to fix, change the detection and mapping: for each server in
mcp_servers iterate env_block and, instead of requiring ${MCP_*}, support
explicit per-server mappings (e.g., map JIRA_* or GOOGLE_* names to returned
fields) or accept non-braced keys by adjusting the regex/condition, then call
_fetch_mcp_credentials and export each retrieved field under the correct target
names (use sanitized_name, field_name) so the runner will receive the
JIRA_*/GOOGLE_* variables expected by mcp-atlassian.
Gkrumbach07
approved these changes
Mar 11, 2026
mprpic
approved these changes
Mar 11, 2026
Contributor
|
GHA skipped E2E on this one since it's from a fork. |
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
4 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.
Uh oh!
There was an error while loading. Please reload this page.