fix(cli): persist custom --portal-url to .env on dashboard register (#42435)

* fix(cli): persist custom --portal-url to .env on dashboard register `hermes dashboard register --portal-url <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 <agent@nousresearch.com> * 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 <agent@nousresearch.com> --------- Co-authored-by: Hermes Agent <agent@nousresearch.com>
2026-06-09 13:56:33 +10:00
parent b23184cad4
commit a46462ec65
2 changed files with 359 additions and 8 deletions
@@ -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 (