From 52ae9d9f022a69f377a1fa5f7c54a541bfca4302 Mon Sep 17 00:00:00 2001 From: Ben Barclay Date: Tue, 9 Jun 2026 13:19:35 +1000 Subject: [PATCH] feat(dashboard): make `hermes dashboard register` idempotent (#42455) Re-running `hermes dashboard register` now updates the existing dashboard record in nous-account-service instead of creating a duplicate. The stable key is the client_id this install already persisted in HERMES_DASHBOARD_OAUTH_CLIENT_ID on a prior run: - No stored client_id -> first registration -> create a fresh client with an auto-generated name (unchanged behavior). - Stored client_id present -> re-send it as `client_id` so the portal updates that row in place. Without an explicit --name, the name is omitted so the portal-stored name isn't churned to a new random value on every re-run. - Prints "Updated dashboard" vs "Registered dashboard" based on whether the portal echoed back the same client_id. A stale/deleted id safely falls through to a fresh create server-side. Requires the matching nous-account-service change (POST /api/oauth/self-hosted-client accepting an optional client_id + optional name). Tests: 7 new TestIdempotentRerun cases (key sent, name preserved/overridden, Updated message, persisted id, stale-id fall-through, blank-id first-run); existing create-path tests unchanged (23 pass). --- hermes_cli/dashboard_register.py | 62 +++++++++- tests/hermes_cli/test_dashboard_register.py | 125 +++++++++++++++++++- 2 files changed, 180 insertions(+), 7 deletions(-) diff --git a/hermes_cli/dashboard_register.py b/hermes_cli/dashboard_register.py index 4ece93d3fc2..a644962f7ef 100644 --- a/hermes_cli/dashboard_register.py +++ b/hermes_cli/dashboard_register.py @@ -94,19 +94,36 @@ def _register_self_hosted_client( *, access_token: str, portal_base_url: str, - name: str, + name: Optional[str], custom_redirect_uri: Optional[str], + existing_client_id: Optional[str] = None, timeout: float = 15.0, ) -> dict: """POST to the portal's self-hosted-client endpoint and return the JSON body. + When ``existing_client_id`` is provided (the client_id this install + persisted on a prior run), it is sent so the portal updates that existing + dashboard record in place instead of minting a duplicate — this is what + makes re-running ``hermes dashboard register`` idempotent. The portal + falls back to creating a fresh client if the id no longer resolves to a row + in the caller's org (stale/deleted), so passing it is always safe. + + ``name`` may be ``None`` on the idempotent update path (re-run without an + explicit ``--name``): omitting it tells the portal to keep the name it + already stored rather than overwriting it. It is required on the create + path; the caller guarantees a value there. + Raises RuntimeError with a user-facing message on any non-2xx response or transport failure. """ url = f"{portal_base_url.rstrip('/')}/api/oauth/self-hosted-client" - body: dict[str, str] = {"name": name} + body: dict[str, str] = {} + if name: + body["name"] = name if custom_redirect_uri: body["custom_redirect_uri"] = custom_redirect_uri + if existing_client_id: + body["client_id"] = existing_client_id data = json.dumps(body).encode("utf-8") req = urllib.request.Request( @@ -245,7 +262,33 @@ def cmd_dashboard_register(args) -> None: ) portal_base_url = _resolve_portal_base_url(portal_override) - name = getattr(args, "name", None) or _generate_dashboard_name() + # Idempotency: if this install already registered a dashboard, we hold its + # client_id locally (HERMES_DASHBOARD_OAUTH_CLIENT_ID). Re-send it so the + # portal UPDATES that existing record instead of creating a duplicate. No + # stored client_id -> this is a first registration -> create a fresh one + # (the original behavior). This mirrors the portal's rule: no client id = + # new dashboard; client id present = the stable key of the row to modify. + existing_client_id = None + try: + existing_client_id = get_env_value("HERMES_DASHBOARD_OAUTH_CLIENT_ID") + except Exception: + existing_client_id = None + if isinstance(existing_client_id, str): + existing_client_id = existing_client_id.strip() or None + else: + existing_client_id = None + + explicit_name = getattr(args, "name", None) + # Auto-generate a random name ONLY for a first registration. On a re-run + # (we hold a client_id) without an explicit --name, keep the name the + # portal already stored rather than churning it to a new random value + # every time — so leave `name` unset and let the portal preserve it. + if explicit_name: + name = explicit_name + elif existing_client_id: + name = None + else: + name = _generate_dashboard_name() custom_redirect_uri = getattr(args, "redirect_uri", None) # 2. Register with the portal. @@ -255,15 +298,24 @@ def cmd_dashboard_register(args) -> None: portal_base_url=portal_base_url, name=name, custom_redirect_uri=custom_redirect_uri, + existing_client_id=existing_client_id, ) except RuntimeError as exc: print(f"✗ Registration failed: {exc}") sys.exit(1) client_id = str(result["client_id"]) - registered_name = str(result.get("name") or name) + registered_name = str(result.get("name") or name or "") - print(f'✓ Registered dashboard "{registered_name}"') + # Distinguish create vs update for the user: the portal echoes back the + # same client_id we sent when it updated in place. + updated_existing = bool( + existing_client_id and client_id == existing_client_id + ) + if updated_existing: + print(f'✓ Updated dashboard "{registered_name}"') + 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 diff --git a/tests/hermes_cli/test_dashboard_register.py b/tests/hermes_cli/test_dashboard_register.py index b3bf9029737..5060031a222 100644 --- a/tests/hermes_cli/test_dashboard_register.py +++ b/tests/hermes_cli/test_dashboard_register.py @@ -76,7 +76,7 @@ def _fake_http_ok(payload: dict): class TestHappyPath: def _run(self, *, args, account_token="tok_abc", portal="https://portal.nousresearch.com", - response=None, captured=None): + response=None, captured=None, existing_client_id=None): response = response or { "client_id": "agent:selfhost-1", "id": "selfhost-1", @@ -98,12 +98,21 @@ class TestHappyPath: def fake_save(key, value): saved[key] = value + # get_env_value is consulted twice: once for the stored client_id + # (idempotency key) and once for HERMES_DASHBOARD_PORTAL_URL. Route by + # key so a test can seed a prior client_id while keeping the portal + # unset (the default-portal-not-persisted path). + def fake_get_env(key): + if key == "HERMES_DASHBOARD_OAUTH_CLIENT_ID": + return existing_client_id + return None + with patch( "hermes_cli.auth.resolve_nous_access_token", return_value=account_token ), patch("hermes_cli.config.is_managed", return_value=False), patch.object( dr, "_resolve_portal_base_url", return_value=portal ), patch( - "hermes_cli.config.get_env_value", return_value=None + "hermes_cli.config.get_env_value", side_effect=fake_get_env ), patch( "hermes_cli.config.save_env_value", side_effect=fake_save ), patch.object( @@ -157,6 +166,118 @@ class TestHappyPath: ) +class TestIdempotentRerun(TestHappyPath): + """Re-running with a stored client_id updates instead of creating. + + Inherits ``_run`` from TestHappyPath; the only new lever is + ``existing_client_id`` (the HERMES_DASHBOARD_OAUTH_CLIENT_ID a prior run + persisted), which the CLI re-sends so the portal updates that row. + """ + + def test_stored_client_id_is_sent_as_idempotency_key(self, capsys): + captured: dict = {} + # Portal echoes back the SAME id -> it updated in place. + self._run( + args=_ns(), + existing_client_id="agent:selfhost-1", + 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", + }, + captured=captured, + ) + assert captured["body"]["client_id"] == "agent:selfhost-1" + + def test_rerun_without_name_omits_name_to_preserve_stored(self, capsys): + # No --name on a re-run: don't churn the portal-stored name. The CLI + # leaves `name` out of the body so the portal keeps what it has. + captured: dict = {} + self._run( + args=_ns(), + existing_client_id="agent:selfhost-1", + captured=captured, + ) + assert "name" not in captured["body"] + assert captured["body"]["client_id"] == "agent:selfhost-1" + + def test_rerun_with_explicit_name_still_sends_name(self, capsys): + captured: dict = {} + self._run( + args=_ns(name="renamed_box"), + existing_client_id="agent:selfhost-1", + captured=captured, + ) + assert captured["body"]["name"] == "renamed_box" + assert captured["body"]["client_id"] == "agent:selfhost-1" + + def test_rerun_prints_updated_when_same_id_returned(self, capsys): + self._run( + args=_ns(), + existing_client_id="agent:selfhost-1", + 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", + }, + ) + out = capsys.readouterr().out + assert "Updated dashboard" in out + assert "Registered dashboard" not in out + + def test_rerun_persists_returned_client_id(self, capsys): + saved = self._run( + args=_ns(), + existing_client_id="agent:selfhost-1", + ) + # Same id round-trips into .env -> idempotent, one record. + assert saved["HERMES_DASHBOARD_OAUTH_CLIENT_ID"] == "agent:selfhost-1" + + def test_stale_id_falls_through_to_create_prints_registered(self, capsys): + # Stored id no longer resolves server-side -> portal created a fresh + # row and returns a DIFFERENT id. The CLI treats that as a create and + # persists the new id (re-run stays safe, never worse than first run). + captured: dict = {} + saved = self._run( + args=_ns(name="seed_name"), + existing_client_id="agent:selfhost-stale", + response={ + "client_id": "agent:selfhost-new", + "id": "selfhost-new", + "name": "seed_name", + "kind": "SELF_HOSTED", + "custom_redirect_uri": None, + "created_at": "2026-06-04T12:00:00.000Z", + }, + captured=captured, + ) + # The stale id is still SENT (portal decides create-vs-update). + assert captured["body"]["client_id"] == "agent:selfhost-stale" + # Returned id differs from what we sent -> message is "Registered". + out = capsys.readouterr().out + assert "Registered dashboard" in out + assert "Updated dashboard" not in out + assert saved["HERMES_DASHBOARD_OAUTH_CLIENT_ID"] == "agent:selfhost-new" + + def test_blank_stored_client_id_treated_as_first_run(self, capsys): + # A blank/whitespace stored value is not a usable key: treat as a + # first registration (auto-generate a name, don't send client_id). + captured: dict = {} + self._run( + args=_ns(), + existing_client_id=" ", + captured=captured, + ) + assert "client_id" not in captured["body"] + assert captured["body"].get("name") # auto-generated + + class TestPortalResolution: def test_override_arg_wins(self): assert (