From af0ce1cf8e9dc728ecea7d1e804b7c8817822772 Mon Sep 17 00:00:00 2001
From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com>
Date: Sat, 4 Jul 2026 13:52:37 +0530
Subject: [PATCH] refactor(mcp): DRY the non-interactive OAuth guard +
 positive-control test

Follow-up to the salvaged #58000 fix.

- Extract _raise_if_non_interactive(lead) so the shared 'hermes mcp login'
  next-step wording lives in one place across both OAuth boundaries
  (_redirect_handler, _wait_for_callback), rather than two copy-pasted
  inline raises. Boundary-specific lead sentences preserved verbatim, so
  existing message-match tests stay green.
- Add a positive-control test asserting the guard does NOT over-fire on the
  interactive path (valid/refreshable tokens keep working), satisfying an
  explicit regression-coverage line from #57836.
---
 tests/tools/test_mcp_oauth.py | 24 +++++++++++++++++++++
 tools/mcp_oauth.py            | 39 +++++++++++++++++++++--------------
 2 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/tests/tools/test_mcp_oauth.py b/tests/tools/test_mcp_oauth.py
index 2f06a542cf0..8f3f58ca9fb 100644
--- a/tests/tools/test_mcp_oauth.py
+++ b/tests/tools/test_mcp_oauth.py
@@ -679,6 +679,30 @@ class TestNonInteractiveFailFastAtCallbackBoundary:
         with pytest.raises(OAuthNonInteractiveError, match="hermes mcp login"):
             asyncio.run(mod._wait_for_callback())
 
+    def test_guard_does_not_fire_on_interactive_redirect(self, monkeypatch, capsys):
+        """Positive control: the fail-fast guard is scoped to the auth-code path.
+
+        #57836 regression coverage asks that valid/refreshable OAuth keeps
+        working non-interactively — a good token never reaches these handlers,
+        so the guard must be inert once a real flow is in progress. Assert the
+        interactive path still prints the URL and does not raise, proving the
+        guard does not over-fire and swallow legitimate authorization.
+        """
+        import tools.mcp_oauth as mod
+        import asyncio
+
+        monkeypatch.setattr(mod, "_is_interactive", lambda: True)
+        # Local (non-SSH) interactive session with no browser available, so the
+        # handler falls through to the manual-URL print without opening a tab.
+        monkeypatch.delenv("SSH_CLIENT", raising=False)
+        monkeypatch.delenv("SSH_TTY", raising=False)
+        monkeypatch.setattr(mod, "_can_open_browser", lambda: False)
+
+        asyncio.run(mod._redirect_handler("https://idp.example.com/authorize?x=9"))
+
+        err = capsys.readouterr().err
+        assert "https://idp.example.com/authorize?x=9" in err
+
 
 # ---------------------------------------------------------------------------
 # Extracted helper tests (Task 3 of MCP OAuth consolidation)
diff --git a/tools/mcp_oauth.py b/tools/mcp_oauth.py
index 49331cbc915..eac9d28f5ad 100644
--- a/tools/mcp_oauth.py
+++ b/tools/mcp_oauth.py
@@ -167,6 +167,21 @@ def _is_interactive() -> bool:
         return False
 
 
+def _raise_if_non_interactive(lead: str) -> None:
+    """Raise ``OAuthNonInteractiveError`` unless an interactive session exists.
+
+    ``lead`` is the boundary-specific first sentence; this helper appends the
+    shared, actionable ``hermes mcp login`` next-step so the guidance wording
+    lives in one place across every non-interactive OAuth boundary (#57836).
+    """
+    if not _is_interactive():
+        raise OAuthNonInteractiveError(
+            f"{lead} "
+            "Run `hermes mcp login <server>` interactively to (re)authorize, "
+            "then restart or reload the gateway."
+        )
+
+
 @contextmanager
 def force_interactive_oauth():
     """Treat the current execution context as interactive despite no TTY.
@@ -539,13 +554,10 @@ async def _redirect_handler(authorization_url: str) -> None:
     # promptly and the caller can skip this server with an actionable warning.
     # This intentionally re-checks interactivity here rather than trusting the
     # token-file existence guard alone. See #57836.
-    if not _is_interactive():
-        raise OAuthNonInteractiveError(
-            "MCP OAuth requires browser authorization but no interactive "
-            "session is available (non-interactive/background context). "
-            "Run `hermes mcp login <server>` interactively to (re)authorize, "
-            "then restart or reload the gateway."
-        )
+    _raise_if_non_interactive(
+        "MCP OAuth requires browser authorization but no interactive "
+        "session is available (non-interactive/background context)."
+    )
 
     msg = (
         f"\n  MCP OAuth: authorization required.\n"
@@ -628,14 +640,11 @@ async def _wait_for_callback() -> tuple[str, str | None]:
     # gateway startup independent of an unusable optional MCP server. This
     # guard holds "regardless of whether a token file exists" — the point the
     # build_oauth_auth token-file guard cannot cover. See #57836.
-    if not _is_interactive():
-        raise OAuthNonInteractiveError(
-            "OAuth callback requires an interactive session but none is "
-            "available (non-interactive/background context); skipping browser "
-            "authorization without binding a callback listener. "
-            "Run `hermes mcp login <server>` interactively to (re)authorize, "
-            "then restart or reload the gateway."
-        )
+    _raise_if_non_interactive(
+        "OAuth callback requires an interactive session but none is "
+        "available (non-interactive/background context); skipping browser "
+        "authorization without binding a callback listener."
+    )
 
     # The callback server is already running (started in build_oauth_auth).
     # We just need to poll for the result.