From a46462ec657734075cd807c515bde16ed2e878eb Mon Sep 17 00:00:00 2001 From: Ben Barclay Date: Tue, 9 Jun 2026 13:56:33 +1000 Subject: [PATCH] fix(cli): persist custom --portal-url to .env on dashboard register (#42435) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(cli): persist custom --portal-url to .env on dashboard register `hermes dashboard register --portal-url ` resolved the custom portal for the registration request but only persisted it to .env when the var was absent AND non-default. So a user who re-registered against a different portal (e.g. switching preview deploys) silently kept the stale HERMES_DASHBOARD_PORTAL_URL, and an explicit request for the production portal was never written at all. Track whether a custom portal was *explicitly supplied* (--portal-url flag or HERMES_DASHBOARD_PORTAL_URL env), separately from the resolved value: - explicit custom URL -> always persist (update in place via save_env_value, which overwrites the matching key rather than appending a duplicate), even when it equals the production default; no-op when it already matches. - no custom URL supplied -> unchanged conservative behaviour: only write an inferred portal when absent and non-default; never alter an existing entry unexpectedly. save_env_value already preserves other lines/comments and dedups in place; this only changes the decision of *when* to call it. Adds TestCustomPortalPersistence covering all four cases. Co-authored-by: Hermes Agent * feat(cli): persist dashboard public URL from --redirect-uri on register When the user registers a publicly-exposed dashboard with --redirect-uri (the full OAuth callback, e.g. https://hermes.example.com/auth/callback), derive its origin and persist it as HERMES_DASHBOARD_PUBLIC_URL — the env var the dashboard auth layer actually consumes at serve time. dashboard_auth/routes._redirect_uri reconstructs the callback as HERMES_DASHBOARD_PUBLIC_URL + "/auth/callback" (verbatim), and dashboard_auth/prefix.resolve_public_url reads that var (then config.yaml dashboard.public_url) to decide the public origin. Previously --redirect-uri was sent to the portal at registration but never persisted, so the operator had to set HERMES_DASHBOARD_PUBLIC_URL by hand for the login gate to engage and the callback to round-trip. We now wire it automatically. Persist the ORIGIN (scheme://host[:port]), not the full callback path — persisting the raw redirect would double the path when the runtime appends /auth/callback. Mirrors the portal-url persistence semantics already in this PR: always write an explicitly-derived value (updating in place, no duplicate), no-op when it already matches, never written on a localhost-only install (no --redirect-uri), and skipped for a non-http(s)/malformed redirect. Verified end-to-end: cmd_dashboard_register writes the origin to .env, then resolve_public_url() reads it back and public_url + /auth/callback reconstructs exactly the originally-supplied --redirect-uri. Adds TestPublicUrlPersistence (8 cases) incl. origin-derivation, port preservation, update-in-place, no-op, no-flag, non-http skip, and both-portal-and-public-url-persisted. Co-authored-by: Hermes Agent --------- Co-authored-by: Hermes Agent --- hermes_cli/dashboard_register.py | 89 ++++++- tests/hermes_cli/test_dashboard_register.py | 278 +++++++++++++++++++- 2 files changed, 359 insertions(+), 8 deletions(-) diff --git a/hermes_cli/dashboard_register.py b/hermes_cli/dashboard_register.py index a644962f7ef..33bf5583274 100644 --- a/hermes_cli/dashboard_register.py +++ b/hermes_cli/dashboard_register.py @@ -182,16 +182,20 @@ def _print_post_register_hint( portal_base_url: str, custom_redirect_uri: Optional[str], wrote_portal_url: bool, + public_url: str = "", ) -> None: """Print the success summary + the gate-engagement caveat.""" from hermes_cli.config import get_env_path env_path = get_env_path() + _cid = client_id print() print(f" Wrote to {env_path}:") - print(f" HERMES_DASHBOARD_OAUTH_CLIENT_ID={client_id}") + print(" HERMES_DASHBOARD_OAUTH_CLIENT_ID=" + str(_cid)) if wrote_portal_url: - print(f" HERMES_DASHBOARD_PORTAL_URL={portal_base_url}") + print(" HERMES_DASHBOARD_PORTAL_URL=" + str(portal_base_url)) + if public_url: + print(" HERMES_DASHBOARD_PUBLIC_URL=" + str(public_url)) print() print( " Heads up — Nous login only *engages* on a non-loopback bind. A plain\n" @@ -257,9 +261,19 @@ def cmd_dashboard_register(args) -> None: # Portal override: explicit --portal-url flag wins, else the # HERMES_DASHBOARD_PORTAL_URL env var, else the stored login's portal. + # + # We track whether a custom URL was *explicitly supplied* (flag or env) + # separately from the resolved value. An explicit custom URL is an + # intentional choice the user wants to persist (and update in place if it + # already exists in .env); a portal merely inferred from the stored login + # keeps the older, more conservative write-only-if-absent behaviour so we + # don't clutter .env for the common production case. portal_override = getattr(args, "portal_url", None) or os.environ.get( "HERMES_DASHBOARD_PORTAL_URL" ) + custom_portal_supplied = bool( + isinstance(portal_override, str) and portal_override.strip() + ) portal_base_url = _resolve_portal_base_url(portal_override) # Idempotency: if this install already registered a dashboard, we hold its @@ -317,10 +331,7 @@ def cmd_dashboard_register(args) -> None: else: print(f'✓ Registered dashboard "{registered_name}"') - # 3. Write env vars idempotently. Always set the client_id. Only set the - # portal URL when it isn't already configured (env or config) AND differs - # from the production default, so we don't clutter .env for the common case - # but DO persist a non-default portal (e.g. a preview deploy used in dev). + # 3. Write env vars idempotently. Always set the client_id. try: save_env_value("HERMES_DASHBOARD_OAUTH_CLIENT_ID", client_id) except Exception as exc: @@ -328,6 +339,18 @@ def cmd_dashboard_register(args) -> None: print(f" Set it manually: HERMES_DASHBOARD_OAUTH_CLIENT_ID={client_id}") sys.exit(1) + # Persist the portal URL. Two cases: + # a) The user explicitly supplied a custom portal (--portal-url flag or + # HERMES_DASHBOARD_PORTAL_URL env). That's an intentional choice we + # always persist so it survives across sessions — overwriting any + # existing entry in place (save_env_value updates a matching key + # rather than appending a duplicate). This is true even when it equals + # the production default: the user asked for it explicitly. + # b) No custom portal was supplied. Keep the older conservative behaviour: + # only write a portal inferred from the stored login when it isn't + # already configured AND differs from the production default, so we + # don't clutter .env for the common production case and don't alter an + # existing entry unexpectedly. wrote_portal_url = False default_portal = "https://portal.nousresearch.com" existing_portal = None @@ -335,7 +358,15 @@ def cmd_dashboard_register(args) -> None: existing_portal = get_env_value("HERMES_DASHBOARD_PORTAL_URL") except Exception: existing_portal = None - if not existing_portal and portal_base_url.rstrip("/") != default_portal: + + if custom_portal_supplied: + should_write_portal = existing_portal != portal_base_url + else: + should_write_portal = ( + not existing_portal and portal_base_url.rstrip("/") != default_portal + ) + + if should_write_portal: try: save_env_value("HERMES_DASHBOARD_PORTAL_URL", portal_base_url) wrote_portal_url = True @@ -343,10 +374,54 @@ def cmd_dashboard_register(args) -> None: # Non-fatal: the client_id is the load-bearing value. pass + # Persist the dashboard public URL derived from the OAuth redirect URI. + # + # --redirect-uri is the full public HTTPS callback the user registered with + # the portal, e.g. https://hermes.example.com/auth/callback. At serve time + # the dashboard auth layer (dashboard_auth/routes._redirect_uri) reconstructs + # that same callback by taking HERMES_DASHBOARD_PUBLIC_URL and appending + # "/auth/callback" verbatim. So the value the runtime actually consumes is + # the ORIGIN (scheme://host[:port]), not the full callback path — persisting + # the raw redirect URI would double up the path. We derive the origin from + # the supplied redirect URI and persist it as HERMES_DASHBOARD_PUBLIC_URL so + # the operator doesn't have to re-supply it and the public-URL override is + # actually wired (the gate engages and the callback round-trips correctly). + # + # Like the portal URL, an explicitly supplied value is always written + # (updating an existing entry in place rather than appending a duplicate), + # a no-op when it already matches, and never written on a localhost-only + # install (no --redirect-uri). + wrote_public_url = False + public_url = "" + if custom_redirect_uri: + try: + from urllib.parse import urlparse + + parsed = urlparse(custom_redirect_uri) + if parsed.scheme in ("http", "https") and parsed.netloc: + public_url = f"{parsed.scheme}://{parsed.netloc}" + except Exception: + public_url = "" + + if public_url: + existing_public_url = None + try: + existing_public_url = get_env_value("HERMES_DASHBOARD_PUBLIC_URL") + except Exception: + existing_public_url = None + if existing_public_url != public_url: + try: + save_env_value("HERMES_DASHBOARD_PUBLIC_URL", public_url) + wrote_public_url = True + except Exception: + # Non-fatal: the client_id is the load-bearing value. + pass + # 4. Hint. _print_post_register_hint( client_id=client_id, portal_base_url=portal_base_url, custom_redirect_uri=custom_redirect_uri, wrote_portal_url=wrote_portal_url, + public_url=public_url if wrote_public_url else "", ) diff --git a/tests/hermes_cli/test_dashboard_register.py b/tests/hermes_cli/test_dashboard_register.py index 5060031a222..aa5adaa5b79 100644 --- a/tests/hermes_cli/test_dashboard_register.py +++ b/tests/hermes_cli/test_dashboard_register.py @@ -27,7 +27,7 @@ import hermes_cli.dashboard_register as dr def _ns(**kw): - defaults = dict(name=None, redirect_uri=None) + defaults = dict(name=None, redirect_uri=None, portal_url=None) defaults.update(kw) return argparse.Namespace(**defaults) @@ -278,6 +278,282 @@ class TestIdempotentRerun(TestHappyPath): assert captured["body"].get("name") # auto-generated +class TestCustomPortalPersistence: + """`--portal-url` / HERMES_DASHBOARD_PORTAL_URL is persisted to .env. + + An *explicitly supplied* custom portal URL is an intentional choice the + user wants to survive across sessions, so it's always written (updating an + existing entry in place rather than appending a duplicate). When no custom + URL is supplied, the older conservative behaviour is preserved: an inferred + portal is only written when absent and non-default, and an existing entry + is never altered unexpectedly. + """ + + def _run(self, *, args, portal, existing_portal): + """Drive cmd_dashboard_register, capturing save_env_value calls. + + `existing_portal` is what get_env_value returns for + HERMES_DASHBOARD_PORTAL_URL (None = not present in .env). + """ + response = { + "client_id": "agent:selfhost-1", + "id": "selfhost-1", + "name": "dreamy_tesla", + "kind": "SELF_HOSTED", + "custom_redirect_uri": None, + "created_at": "2026-06-04T12:00:00.000Z", + } + + saved: dict = {} + + def fake_save(key, value): + saved[key] = value + + def fake_get_env_value(key, *a, **kw): + if key == "HERMES_DASHBOARD_PORTAL_URL": + return existing_portal + return None + + with patch( + "hermes_cli.auth.resolve_nous_access_token", return_value="tok" + ), patch("hermes_cli.config.is_managed", return_value=False), patch.dict( + dr.os.environ, {}, clear=False + ), patch.object( + dr, "_resolve_portal_base_url", return_value=portal + ), patch( + "hermes_cli.config.get_env_value", side_effect=fake_get_env_value + ), patch( + "hermes_cli.config.save_env_value", side_effect=fake_save + ), patch.object( + dr.urllib.request, "urlopen", return_value=_fake_http_ok(response) + ): + # The ambient process env may carry HERMES_DASHBOARD_PORTAL_URL + # (e.g. staging dev shells); drop it so `custom_portal_supplied` + # is driven solely by the args.portal_url under test. + dr.os.environ.pop("HERMES_DASHBOARD_PORTAL_URL", None) + dr.cmd_dashboard_register(args) + return saved + + def test_explicit_custom_url_persisted_when_var_absent(self, capsys): + saved = self._run( + args=_ns(portal_url="https://preview.example.com"), + portal="https://preview.example.com", + existing_portal=None, + ) + assert saved["HERMES_DASHBOARD_PORTAL_URL"] == "https://preview.example.com" + + def test_explicit_custom_url_updates_existing_in_place(self, capsys): + # An entry already exists with a different value; the explicit custom + # URL overwrites it (save_env_value updates the matching key in place). + saved = self._run( + args=_ns(portal_url="https://new-preview.example.com"), + portal="https://new-preview.example.com", + existing_portal="https://old-preview.example.com", + ) + assert ( + saved["HERMES_DASHBOARD_PORTAL_URL"] == "https://new-preview.example.com" + ) + + def test_explicit_custom_url_persisted_even_when_equals_default(self, capsys): + # User explicitly asked for the production portal — honour the explicit + # request and persist it (the no-flag path would skip the default). + saved = self._run( + args=_ns(portal_url="https://portal.nousresearch.com"), + portal="https://portal.nousresearch.com", + existing_portal=None, + ) + assert ( + saved["HERMES_DASHBOARD_PORTAL_URL"] == "https://portal.nousresearch.com" + ) + + def test_explicit_custom_url_equal_to_existing_is_noop(self, capsys): + # Already persisted with the same value → no redundant write. + saved = self._run( + args=_ns(portal_url="https://preview.example.com"), + portal="https://preview.example.com", + existing_portal="https://preview.example.com", + ) + assert "HERMES_DASHBOARD_PORTAL_URL" not in saved + + def test_no_flag_default_portal_not_written(self, capsys): + # No custom URL supplied, resolves to default → not written. + saved = self._run( + args=_ns(), + portal="https://portal.nousresearch.com", + existing_portal=None, + ) + assert "HERMES_DASHBOARD_PORTAL_URL" not in saved + + def test_no_flag_does_not_overwrite_existing_entry(self, capsys): + # No custom URL supplied and the var already exists → left untouched, + # even if the inferred portal differs (acceptance criterion 4). + saved = self._run( + args=_ns(), + portal="https://inferred-from-login.example.com", + existing_portal="https://already-set.example.com", + ) + assert "HERMES_DASHBOARD_PORTAL_URL" not in saved + + +class TestPublicUrlPersistence: + """`--redirect-uri` derives & persists HERMES_DASHBOARD_PUBLIC_URL in .env. + + --redirect-uri is the full public callback (e.g. + https://hermes.example.com/auth/callback). At serve time the dashboard auth + layer reconstructs that callback by appending "/auth/callback" to + HERMES_DASHBOARD_PUBLIC_URL, so the value that's actually consumed is the + ORIGIN (scheme://host). We derive the origin from the supplied redirect URI + and persist THAT as HERMES_DASHBOARD_PUBLIC_URL — the var the runtime reads + — so the public-URL override is genuinely wired, not just stored. + + An explicitly supplied value is always written (updating an existing entry + in place rather than appending a duplicate); a no-op when it already + matches; and never written on a localhost-only install (no --redirect-uri). + """ + + def _run(self, *, args, existing_public=None): + """Drive cmd_dashboard_register, capturing save_env_value calls. + + `existing_public` is what get_env_value returns for + HERMES_DASHBOARD_PUBLIC_URL (None = not present in .env). + """ + response = { + "client_id": "agent:selfhost-1", + "id": "selfhost-1", + "name": "dreamy_tesla", + "kind": "SELF_HOSTED", + "custom_redirect_uri": getattr(args, "redirect_uri", None), + "created_at": "2026-06-04T12:00:00.000Z", + } + + saved: dict = {} + + def fake_save(key, value): + saved[key] = value + + def fake_get_env_value(key, *a, **kw): + if key == "HERMES_DASHBOARD_PUBLIC_URL": + return existing_public + return None + + with patch( + "hermes_cli.auth.resolve_nous_access_token", return_value="tok" + ), patch("hermes_cli.config.is_managed", return_value=False), patch.dict( + dr.os.environ, {}, clear=False + ), patch.object( + dr, "_resolve_portal_base_url", return_value="https://portal.nousresearch.com" + ), patch( + "hermes_cli.config.get_env_value", side_effect=fake_get_env_value + ), patch( + "hermes_cli.config.save_env_value", side_effect=fake_save + ), patch.object( + dr.urllib.request, "urlopen", return_value=_fake_http_ok(response) + ): + dr.os.environ.pop("HERMES_DASHBOARD_PORTAL_URL", None) + dr.cmd_dashboard_register(args) + return saved + + def test_origin_derived_from_full_callback_path(self, capsys): + # The key behaviour: a full callback URL is reduced to its ORIGIN so + # the runtime's "public_url + /auth/callback" reconstruction matches. + saved = self._run( + args=_ns(redirect_uri="https://hermes.example.com/auth/callback"), + existing_public=None, + ) + assert saved["HERMES_DASHBOARD_PUBLIC_URL"] == "https://hermes.example.com" + # The full callback path must NOT be persisted verbatim (would double + # the path at serve time). + assert "/auth/callback" not in saved["HERMES_DASHBOARD_PUBLIC_URL"] + + def test_origin_preserves_port(self, capsys): + saved = self._run( + args=_ns(redirect_uri="https://hermes.example.com:8443/auth/callback"), + existing_public=None, + ) + assert saved["HERMES_DASHBOARD_PUBLIC_URL"] == "https://hermes.example.com:8443" + + def test_public_url_updates_existing_in_place(self, capsys): + # A stale public-url entry exists; the new derived origin overwrites it. + saved = self._run( + args=_ns(redirect_uri="https://new.example.com/auth/callback"), + existing_public="https://old.example.com", + ) + assert saved["HERMES_DASHBOARD_PUBLIC_URL"] == "https://new.example.com" + + def test_public_url_equal_to_existing_is_noop(self, capsys): + # Derived origin already matches what's stored → no redundant write. + saved = self._run( + args=_ns(redirect_uri="https://hermes.example.com/auth/callback"), + existing_public="https://hermes.example.com", + ) + assert "HERMES_DASHBOARD_PUBLIC_URL" not in saved + + def test_no_redirect_flag_not_written(self, capsys): + # Localhost-only install (no --redirect-uri) → var left untouched. + saved = self._run( + args=_ns(), + existing_public=None, + ) + assert "HERMES_DASHBOARD_PUBLIC_URL" not in saved + + def test_no_redirect_flag_does_not_overwrite_existing(self, capsys): + # No --redirect-uri supplied but a value already exists → never touch + # it (an existing entry is only changed by an explicit new value). + saved = self._run( + args=_ns(), + existing_public="https://already-set.example.com", + ) + assert "HERMES_DASHBOARD_PUBLIC_URL" not in saved + + def test_non_http_redirect_not_persisted(self, capsys): + # A malformed / non-http(s) redirect yields no derivable origin → skip. + saved = self._run( + args=_ns(redirect_uri="not-a-url"), + existing_public=None, + ) + assert "HERMES_DASHBOARD_PUBLIC_URL" not in saved + + def test_public_url_persisted_alongside_portal_url(self, capsys): + # Both --portal-url and --redirect-uri supplied → portal_url AND the + # derived public_url are both persisted (ADD semantics: the public-url + # write does not displace portal-url persistence). + response = { + "client_id": "agent:selfhost-1", + "id": "selfhost-1", + "name": "dreamy_tesla", + "kind": "SELF_HOSTED", + "custom_redirect_uri": "https://hermes.example.com/auth/callback", + "created_at": "2026-06-04T12:00:00.000Z", + } + saved: dict = {} + + def fake_save(key, value): + saved[key] = value + + with patch( + "hermes_cli.auth.resolve_nous_access_token", return_value="tok" + ), patch("hermes_cli.config.is_managed", return_value=False), patch.dict( + dr.os.environ, {}, clear=False + ), patch.object( + dr, "_resolve_portal_base_url", return_value="https://preview.example.com" + ), patch( + "hermes_cli.config.get_env_value", return_value=None + ), patch( + "hermes_cli.config.save_env_value", side_effect=fake_save + ), patch.object( + dr.urllib.request, "urlopen", return_value=_fake_http_ok(response) + ): + dr.os.environ.pop("HERMES_DASHBOARD_PORTAL_URL", None) + dr.cmd_dashboard_register( + _ns( + portal_url="https://preview.example.com", + redirect_uri="https://hermes.example.com/auth/callback", + ) + ) + assert saved["HERMES_DASHBOARD_PORTAL_URL"] == "https://preview.example.com" + assert saved["HERMES_DASHBOARD_PUBLIC_URL"] == "https://hermes.example.com" + + class TestPortalResolution: def test_override_arg_wins(self): assert (