From ad17c6c80321e8bfae1604aaf6b5bcb32c2a7ee1 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Tue, 7 Apr 2026 13:36:32 +0000 Subject: [PATCH 1/2] fix: preserve Google Workspace MCP credentials between turns and add runner feedback route (#1222) Three fixes for issues reported in #1222: 1. **Google Workspace MCP auth**: Stop deleting the credentials file (`credentials.json`) in `clear_runtime_credentials()`. The workspace-mcp process reads from this file continuously; removing it between turns forces it into an inaccessible localhost OAuth flow. The file is overwritten with fresh tokens at the start of each run anyway. 2. **Credential refresh diagnostics**: After refreshing credentials, the `refresh_credentials` tool now checks MCP server auth status and includes diagnostic warnings in its response. This prevents false positives where the tool reports success but MCP servers still can't authenticate. 3. **Feedback endpoint auth**: Add a runner-accessible feedback route at `/api/projects/:projectName/agentic-sessions/:sessionName/runner/feedback` that bypasses `ValidateProjectContext()` middleware. In-session workflows (e.g. Amber) can now submit feedback using BOT_TOKEN without requiring a user OAuth token. The handler still performs its own SSAR auth check. Co-Authored-By: Claude Opus 4.6 --- components/backend/routes.go | 6 ++ .../ambient_runner/bridges/claude/tools.py | 41 +++++++++++-- .../ambient_runner/platform/auth.py | 19 +++---- .../tests/test_shared_session_credentials.py | 57 +++++++++++++++++++ 4 files changed, 107 insertions(+), 16 deletions(-) mode change 100644 => 100755 components/backend/routes.go mode change 100644 => 100755 components/runners/ambient-runner/ambient_runner/bridges/claude/tools.py mode change 100644 => 100755 components/runners/ambient-runner/tests/test_shared_session_credentials.py diff --git a/components/backend/routes.go b/components/backend/routes.go old mode 100644 new mode 100755 index 4d5bac93b..f93de52a2 --- a/components/backend/routes.go +++ b/components/backend/routes.go @@ -18,6 +18,12 @@ func registerRoutes(r *gin.Engine) { api.POST("/projects/:projectName/agentic-sessions/:sessionName/github/token", handlers.MintSessionGitHubToken) + // Runner-accessible feedback endpoint — accepts service account tokens + // (BOT_TOKEN) so that in-session workflows (e.g. Amber) can submit + // feedback without requiring a user OAuth token. The handler performs + // its own auth via GetK8sClientsForRequest + SSAR check. + api.POST("/projects/:projectName/agentic-sessions/:sessionName/runner/feedback", websocket.HandleAGUIFeedback) + projectGroup := api.Group("/projects/:projectName", handlers.ValidateProjectContext()) { projectGroup.GET("/models", handlers.ListModelsForProject) diff --git a/components/runners/ambient-runner/ambient_runner/bridges/claude/tools.py b/components/runners/ambient-runner/ambient_runner/bridges/claude/tools.py old mode 100644 new mode 100755 index 4742dc5f6..1838bd6f7 --- a/components/runners/ambient-runner/ambient_runner/bridges/claude/tools.py +++ b/components/runners/ambient-runner/ambient_runner/bridges/claude/tools.py @@ -22,6 +22,30 @@ logger = logging.getLogger(__name__) +# ------------------------------------------------------------------ +# Credential refresh helpers +# ------------------------------------------------------------------ + + +def _check_mcp_auth_after_refresh() -> str: + """Check MCP server auth status after a credential refresh. + + Returns a diagnostic string with any warnings about MCP servers + that may not be able to use the refreshed credentials. Empty + string means all known servers look healthy. + """ + from ambient_runner.bridges.claude.mcp import check_mcp_authentication + + warnings = [] + for server_name in ("google-workspace", "mcp-atlassian"): + is_auth, msg = check_mcp_authentication(server_name) + if is_auth is False: + warnings.append(f"{server_name}: {msg}") + elif is_auth is None and msg: + warnings.append(f"{server_name}: {msg}") + return "; ".join(warnings) + + # ------------------------------------------------------------------ # Credential refresh tool # ------------------------------------------------------------------ @@ -70,14 +94,23 @@ async def refresh_credentials_tool(args: dict) -> dict: integrations = get_active_integrations() summary = ", ".join(integrations) if integrations else "none detected" + + # Verify MCP server auth status after refresh to detect + # false positives (credentials written but MCP server can't use them). + diagnostics = _check_mcp_auth_after_refresh() + + parts = [ + f"Credentials refreshed successfully. " + f"Active integrations: {summary}.", + ] + if diagnostics: + parts.append(f"MCP diagnostics: {diagnostics}") + return { "content": [ { "type": "text", - "text": ( - f"Credentials refreshed successfully. " - f"Active integrations: {summary}." - ), + "text": "\n".join(parts), } ] } diff --git a/components/runners/ambient-runner/ambient_runner/platform/auth.py b/components/runners/ambient-runner/ambient_runner/platform/auth.py index f43aa0636..5d5645162 100755 --- a/components/runners/ambient-runner/ambient_runner/platform/auth.py +++ b/components/runners/ambient-runner/ambient_runner/platform/auth.py @@ -454,18 +454,13 @@ def clear_runtime_credentials() -> None: except OSError as e: logger.warning(f"Failed to remove token file {token_file}: {e}") - # Remove Google Workspace credential file if present (uses same hardcoded path as populate_runtime_credentials) - google_cred_file = _GOOGLE_WORKSPACE_CREDS_FILE - if google_cred_file.exists(): - try: - google_cred_file.unlink() - cleared.append("google_workspace_credentials_file") - # Clean up empty parent dirs - cred_dir = google_cred_file.parent - if cred_dir.exists() and not any(cred_dir.iterdir()): - cred_dir.rmdir() - except OSError as e: - logger.warning(f"Failed to remove Google credential file: {e}") + # NOTE: Google Workspace credential file is intentionally NOT deleted here. + # The workspace-mcp process runs as a long-lived child process of the Claude + # CLI and reads credentials from this file. Deleting it between turns causes + # workspace-mcp to lose its credentials and fall back to initiating a new + # OAuth flow (with an inaccessible localhost:8000 callback URL). + # The file is overwritten with fresh credentials at the start of each run + # by populate_runtime_credentials(), so staleness is not a concern. if cleared: logger.info(f"Cleared credentials: {', '.join(cleared)}") diff --git a/components/runners/ambient-runner/tests/test_shared_session_credentials.py b/components/runners/ambient-runner/tests/test_shared_session_credentials.py old mode 100644 new mode 100755 index de73d363e..0a60b2d24 --- a/components/runners/ambient-runner/tests/test_shared_session_credentials.py +++ b/components/runners/ambient-runner/tests/test_shared_session_credentials.py @@ -13,6 +13,7 @@ from ambient_runner.platform.auth import ( _GITHUB_TOKEN_FILE, _GITLAB_TOKEN_FILE, + _GOOGLE_WORKSPACE_CREDS_FILE, _fetch_credential, clear_runtime_credentials, populate_runtime_credentials, @@ -139,6 +140,27 @@ def test_does_not_crash_when_vars_absent(self): # Should not raise clear_runtime_credentials() + def test_preserves_google_credentials_file(self, tmp_path, monkeypatch): + """clear_runtime_credentials must NOT delete the Google credentials file. + + The workspace-mcp process reads credentials from this file. Deleting it + between turns causes workspace-mcp to fall back to an inaccessible + localhost OAuth flow (issue #1222). + """ + fake_cred_file = tmp_path / "credentials.json" + fake_cred_file.write_text('{"token": "test-access-token"}') + monkeypatch.setattr( + "ambient_runner.platform.auth._GOOGLE_WORKSPACE_CREDS_FILE", + fake_cred_file, + ) + + clear_runtime_credentials() + + assert fake_cred_file.exists(), ( + "Google credentials file must NOT be deleted — workspace-mcp needs it" + ) + assert fake_cred_file.read_text() == '{"token": "test-access-token"}' + def test_does_not_clear_unrelated_vars(self): try: os.environ["PATH_BACKUP_TEST"] = "keep-me" @@ -705,8 +727,43 @@ async def test_returns_success_on_successful_refresh(self): "ambient_runner.platform.utils.get_active_integrations", return_value=["github", "jira"], ), + patch( + "ambient_runner.bridges.claude.tools._check_mcp_auth_after_refresh", + return_value="", + ), ): result = await tool_fn({}) assert result.get("isError") is None or result.get("isError") is False assert "successfully" in result["content"][0]["text"].lower() + + @pytest.mark.asyncio + async def test_includes_mcp_diagnostics_on_auth_warning(self): + """refresh_credentials_tool includes MCP diagnostic warnings when auth issues are detected.""" + from ambient_runner.bridges.claude.tools import create_refresh_credentials_tool + + mock_context = MagicMock() + tool_fn = create_refresh_credentials_tool( + mock_context, self._make_tool_decorator() + ) + + with ( + patch( + "ambient_runner.platform.auth.populate_runtime_credentials", + new_callable=AsyncMock, + ), + patch( + "ambient_runner.platform.utils.get_active_integrations", + return_value=["github", "google"], + ), + patch( + "ambient_runner.bridges.claude.tools._check_mcp_auth_after_refresh", + return_value="google-workspace: Google OAuth token expired - re-authenticate", + ), + ): + result = await tool_fn({}) + + text = result["content"][0]["text"] + assert "successfully" in text.lower() + assert "MCP diagnostics:" in text + assert "google-workspace" in text From a124bc1dfecee33d73725c77d67d21cf0a7a1f00 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Tue, 7 Apr 2026 14:11:04 +0000 Subject: [PATCH 2/2] fix: skip redundant credential clear on first run to preserve MCP server env vars On first run, _setup_platform() already populates credentials and builds MCP servers with correctly expanded env vars. The subsequent clear_runtime_credentials() call in _initialize_run() would needlessly remove env vars like USER_GOOGLE_EMAIL before re-populating them, creating a brief window where workspace-mcp env state is inconsistent. Skip the clear-repopulate cycle on first run since _setup_platform() already handled it. Co-Authored-By: Claude Opus 4.6 --- .../ambient_runner/bridges/claude/bridge.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) mode change 100644 => 100755 components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py diff --git a/components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py b/components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py old mode 100644 new mode 100755 index 7ebee5f17..c726e9b5a --- a/components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py +++ b/components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py @@ -120,11 +120,18 @@ async def _initialize_run( await self._ensure_ready() - # Fresh credentials for this user on every run - clear_runtime_credentials() - await populate_runtime_credentials(self._context) - await populate_mcp_server_credentials(self._context) - self._last_creds_refresh = time.monotonic() + # Fresh credentials for this user on every run. + # On first run, _setup_platform() already populated credentials and + # built MCP servers with the correct env vars — skip the redundant + # clear-then-repopulate cycle to avoid briefly removing env vars + # (like USER_GOOGLE_EMAIL) that MCP servers depend on. + if self._first_run: + logger.info("First run: using credentials from _setup_platform()") + else: + clear_runtime_credentials() + await populate_runtime_credentials(self._context) + await populate_mcp_server_credentials(self._context) + self._last_creds_refresh = time.monotonic() # If the caller changed, destroy the worker and rebuild MCP servers + # adapter so the new ClaudeSDKClient gets fresh mcp_servers config.