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.
This commit is contained in:
kshitijk4poor 2026-07-04 13:52:37 +05:30 committed by kshitij
parent 0c8441c880
commit af0ce1cf8e
2 changed files with 48 additions and 15 deletions

View file

@ -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)

View file

@ -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.