fix: redirect interactive stderr away from TUI - #52441
Conversation
Use the configured oauth.redirect_host for callback URI generation and listener binding. Add focused tests for localhost/IPv6 redirect URIs, dual-stack port selection, and SSH hint output.
Related: this PR bundles two changes. The MCP OAuth |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing terminal corruption and adding focused coverage. The stdio-MCP portion of the reported problem is already addressed on current main by 379b2273d: tools/mcp_tool.py:2097-2105 passes a profile-local log file as stdio_client(..., errlog=...).
Problems
- The new
redirect_stderr_to_log()context around the entire interactive app (cli.py:14475in this PR) also hides deliberate user-facing OAuth output.tools/mcp_oauth.py:567writes the authorization URL tosys.stderr, andtools/mcp_oauth.py:673writes the paste fallback prompt; both would go only to the side log. - The changed URI construction at
tools/mcp_oauth.py:744directly interpolates::1, yieldinghttp://::1:<port>/callback. The added test itself expects the bracketed formhttp://[::1]:8771/callback; the pre-registered-client path has the same construction.
Suggested changes
- Scope redirection to the identified non-interactive producer rather than replacing process-wide stderr during
app.run(). - Centralize URI-host formatting and use it in both OAuth metadata paths.
- Consider splitting the redirect-host work noted in the existing #21482 discussion.
Automated hermes-sweeper review.
| with patch_stdout(): | ||
| from hermes_logging import redirect_stderr_to_log | ||
|
|
||
| with redirect_stderr_to_log(), patch_stdout(): |
There was a problem hiding this comment.
This replaces sys.stderr for the full interactive session, but MCP OAuth intentionally prints its authorization URL and paste fallback to stderr (tools/mcp_oauth.py:567,673). Those user-required instructions would be silently diverted to the side log; scope the redirect to the noisy producer instead.
| client_name = cfg.get("client_name", "Hermes Agent") | ||
| scope = cfg.get("scope") | ||
| redirect_uri = f"http://127.0.0.1:{port}/callback" | ||
| redirect_uri = f"http://{_oauth_uri_host}:{port}/callback" |
There was a problem hiding this comment.
For the allowed ::1 host this constructs http://::1:<port>/callback, while the added test expects http://[::1]:8771/callback. Format IPv6 URI authorities with brackets here and in the pre-registered-client path.
Summary:
Why:
Verification: