mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-09 08:21:50 +00:00
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).
This commit is contained in:
parent
1e5ff4a577
commit
52ae9d9f02
2 changed files with 180 additions and 7 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 (
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue