Skip to content

feat(egress): CA rotation — follow-up to #35149#5

Closed
Bartok9 wants to merge 1 commit into
feat/iron-proxy-doctor-audit-anthropicfrom
feat/iron-proxy-ca-rotation
Closed

feat(egress): CA rotation — follow-up to #35149#5
Bartok9 wants to merge 1 commit into
feat/iron-proxy-doctor-audit-anthropicfrom
feat/iron-proxy-ca-rotation

Conversation

@Bartok9

@Bartok9 Bartok9 commented May 30, 2026

Copy link
Copy Markdown
Owner

Why this complements NousResearch#30179 and NousResearch#35149

PR NousResearch#30179 shipped the iron-proxy credential-injection firewall and explicitly cut one corner in its Scope cuts (v1) section:

The CA is a 10-year self-signed cert. Rotation is manual for now.

PR NousResearch#35149 then layered a doctor + audit + Anthropic-native pass on top, including a ca doctor check that fails on expiry. This PR closes the remaining gap: a first-class, audited rotation workflow so operators never have to hand-run openssl genrsa (and so an annual-hygiene reminder exists). It is additive — it does not touch the behaviour of either prior PR.

Stack: NousResearch#30179NousResearch#35149this PR (base: feat/iron-proxy-doctor-audit-anthropic).

What lands

  • hermes egress rotate-ca — archive the live CA, mint a fresh one (via the same atomic ensure_ca_cert(force=True) path), restart the daemon if running, and record a structured audit entry.
  • ca-rotation doctor check — additive entry in DOCTOR_CHECK_NAMES; an annual-hygiene warn tier distinct from feat(egress): doctor + audit + Anthropic native — follow-up to #30179 NousResearch/hermes-agent#35149's ca expiry check. Reads rotation-history.jsonl, falls back to the cert's notBefore when history is absent.
  • Docs — a new "CA rotation" section in the egress user guide + the rotate-ca command in the CLI reference.
  • 10 hermetic tests mirroring tests/test_iron_proxy_doctor.py.

Rotation steps, in order (so a crash never leaves a missing live CA):

  1. Archive current ca.crt~/.hermes/proxy/ca-archive/ca-<YYYYMMDD-HHMMSS>.crt, then prune to the 5 most recent.
  2. Mint a new CA + key (atomic 0o600 key write, os.replace).
  3. If running and not --no-restart: stop_proxy()start_proxy().
  4. Append to rotation-history.jsonl: {ts, old_fingerprint_sha256, new_fingerprint_sha256, reason, operator, subject, valid_until}.

New surfaces

Surface Kind Notes
hermes egress rotate-ca CLI command --dry-run, --reason TEXT, --no-restart, --force
ca-rotation doctor check additive; pass <365d, warn 365d–~9y; expiry stays ca's job
rotate_ca() / plan_ca_rotation() public API exported in __all__
RotationPlan dataclass shared by dry-run preview and the real path so they can't diverge
last_rotation_entry() / list_hermes_sandboxes() public API history read + labeled-sandbox discovery
~/.hermes/proxy/rotation-history.jsonl on-disk artifact append-only audit trail (0o644)
~/.hermes/proxy/ca-archive/ on-disk artifact 0o700, last-5 retention

Failure modes considered

Mode Handling
Daemon not running restart skipped silently; archive + mint still happen
No docker / no sandboxes list_hermes_sandboxes() returns [], never raises
openssl fingerprint failure redacted first-8/last-8 via _redact_fingerprint; rotation still completes (records null fingerprint)
Crash mid-rotation old CA is archived before the live cert is replaced; mint is atomic (os.replace)
Running sandboxes trust the old CA rotate-ca lists them and prompts by default (bypass with --force); docs + console output tell the operator to restart them
--dry-run plan_ca_rotation() is read-only; verified by stat-before/after in tests
--no-restart daemon left on the old key (staged rollout); not restarted
Corrupt/partial rotation-history.jsonl line last_rotation_entry() scans from the end, skips unparseable lines

Validation

pytest tests/test_iron_proxy*.py -q
Metric Before this PR (NousResearch#35149 base) After
iron-proxy tests passed 155 165
iron-proxy tests skipped 1 1
New tests added 10 (tests/test_iron_proxy_rotate_ca.py)
Failures 0 0

New tests: writes/archives with no daemon; restart order (stopstart); dry-run no-mtime-change; reason recorded with valid 64-hex fingerprints (old≠new); archive pruning (7→5); --no-restart leaves daemon alone; doctor pass/warn/notBefore-fallback; fingerprint redaction.

Manual end-to-end smoke (isolated HERMES_HOME): --dry-run changed nothing; real rotate-ca --reason "annual rotation" archived the old cert, minted a 10-year CA, wrote a parseable history line, and the ca-rotation doctor check returned pass.

Coverage gaps

  • No verify-trust subcommand (verifying live sandboxes still trust the new CA after restart). Deliberately deferred — documented as a known follow-up in both the PR and the user guide.
  • Interactive confirmation prompt path is exercised manually, not unit-tested (the underlying rotate_ca engine and list_hermes_sandboxes discovery are).
  • Sandbox restart is the operator's responsibility; we surface the list but don't auto-recreate containers (would couple egress to the docker backend lifecycle).

Ambiguity flags

  1. ca vs ca-rotation overlap on hard expiry. Chose option (b) from the brief — a separate additive check — and explicitly made ca-rotation not duplicate the expiry failure (that stays the ca check's job; ca-rotation only adds an age/hygiene warn). This keeps feat(egress): doctor + audit + Anthropic native — follow-up to #30179 NousResearch/hermes-agent#35149's tested ca behaviour untouched.
  2. History anchor when rotation-history.jsonl is missing. Fell back to the cert's notBefore (when was this CA minted?) rather than treating "no history" as a warn — a freshly-setup CA shouldn't nag on day one.
  3. subject field shape. openssl x509 -subject emits e.g. subject=CN=hermes iron-proxy CA; recorded verbatim rather than parsing out a bare CN, to stay faithful to the source and avoid a brittle parser.

This PR was opened by Bartok at Daniel's request, in response to Teknium's invitation to Catalin for a security review of NousResearch#30179.

"How much of the capabilities do you think you have to tradeoff for a truly secure hermes..." — Teknium


Note

Medium Risk
Rotation replaces the TLS-intercept CA and signing key; mis-timed restarts or unrestarted sandboxes can break trust until containers are recreated, though ordering (archive before mint) limits leaving the host without a live CA.

Overview
Adds a first-class iron-proxy CA rotation workflow on top of existing ensure_ca_cert(force=True), replacing the prior manual-only rotation story.

hermes egress rotate-ca archives the live ca.crt under ~/.hermes/proxy/ca-archive/ (keeps five), mints a new CA/key atomically, optionally restarts the managed daemon (stop_proxystart_proxy), and appends audit lines to rotation-history.jsonl. plan_ca_rotation / RotationPlan share logic with --dry-run; the CLI lists labeled hermes.sandbox=true containers, prompts before rotate unless --force, and supports --reason, --no-restart.

Doctor gains ca-rotation: pass if last rotation (or cert notBefore when no history) is under 365 days, warn otherwise—separate from the existing ca expiry check. New helpers cover fingerprints (with redaction), sandbox discovery, and history parsing; __all__ exports the public API.

Docs describe the command and flags; 10 hermetic tests cover archive/restart ordering, dry-run immutability, history, pruning, and doctor tiers.

Reviewed by Cursor Bugbot for commit 3650b23. Bugbot is set up for automated code reviews on this repo. Configure here.

Add `hermes egress rotate-ca` plus a `ca-rotation` doctor check, closing
PR NousResearch#30179's scope cut: "The CA is a 10-year self-signed cert. Rotation is
manual for now."

- rotate_ca(): archive old ca.crt -> ca-archive/ (keep 5), mint fresh CA via
  ensure_ca_cert(force=True), restart daemon if running, append a structured
  entry to rotation-history.jsonl.
- CLI flags: --dry-run, --reason, --no-restart, --force.
- New additive doctor check 'ca-rotation' (annual-hygiene warn tier); does
  not mutate NousResearch#35149's 'ca' expiry check.
- Fingerprints redacted first-8/last-8 in error paths (_redact_fingerprint).
- 10 new hermetic tests; full iron-proxy suite stays green (155 -> 165).

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Daemon restart after rotation loses bitwarden secrets
    • CA rotation now carries Bitwarden refresh options through CLI validation and daemon restart so restarted proxies fetch Bitwarden-backed secrets.
  • ✅ Fixed: Duplicated date-parsing logic in _ca_not_before and _ca_not_after
    • The duplicated openssl certificate date parsing was consolidated into a shared helper used by both notBefore and notAfter readers.

Create PR

Or push these changes by commenting:

@cursor push db1a23a751
Preview (db1a23a751)
diff --git a/agent/proxy_sources/iron_proxy.py b/agent/proxy_sources/iron_proxy.py
--- a/agent/proxy_sources/iron_proxy.py
+++ b/agent/proxy_sources/iron_proxy.py
@@ -981,6 +981,8 @@
     reason: Optional[str] = None,
     restart: bool = True,
     operator: Optional[str] = None,
+    refresh_secrets_from_bitwarden: bool = False,
+    bitwarden_config: Optional[Dict] = None,
 ) -> RotationPlan:
     """Generate a fresh CA, archive the old one, and (optionally) restart.
 
@@ -1040,7 +1042,10 @@
     # 3. Restart the daemon if it was running and the caller wants it.
     if restart and plan.daemon_running:
         stop_proxy()
-        start_proxy()
+        start_proxy(
+            refresh_secrets_from_bitwarden=refresh_secrets_from_bitwarden,
+            bitwarden_config=bitwarden_config,
+        )
 
     # 4. Append the audit record.
     record = {
@@ -2503,24 +2508,20 @@
     return "****" + value[-4:]
 
 
-def _ca_not_after(ca_crt: Path) -> Optional[datetime]:
-    """Return the CA cert's notAfter as an aware UTC datetime, or None.
-
-    Uses ``openssl x509 -enddate`` (already a hard dep for CA generation)
-    so we don't pull in ``cryptography`` just to read one field.
-    """
+def _ca_x509_date(ca_crt: Path, flag: str) -> Optional[datetime]:
+    """Return an openssl x509 date field as an aware UTC datetime, or None."""
     if shutil.which("openssl") is None:
         return None
     try:
         res = subprocess.run(  # noqa: S603 -- openssl trusted PATH lookup
-            ["openssl", "x509", "-enddate", "-noout", "-in", str(ca_crt)],
+            ["openssl", "x509", flag, "-noout", "-in", str(ca_crt)],
             capture_output=True, text=True, timeout=10,
         )
     except (OSError, subprocess.TimeoutExpired):
         return None
     if res.returncode != 0:
         return None
-    # Output: ``notAfter=Jun  1 12:00:00 2035 GMT``
+    # Output: ``notAfter=...`` or ``notBefore=...``.
     line = res.stdout.strip()
     if "=" not in line:
         return None
@@ -2534,6 +2535,15 @@
     return None
 
 
+def _ca_not_after(ca_crt: Path) -> Optional[datetime]:
+    """Return the CA cert's notAfter as an aware UTC datetime, or None.
+
+    Uses ``openssl x509 -enddate`` (already a hard dep for CA generation)
+    so we don't pull in ``cryptography`` just to read one field.
+    """
+    return _ca_x509_date(ca_crt, "-enddate")
+
+
 def _ca_not_before(ca_crt: Path) -> Optional[datetime]:
     """Return the CA cert's notBefore as an aware UTC datetime, or None.
 
@@ -2541,29 +2551,7 @@
     ``rotation-history.jsonl`` is absent (a CA generated by ``setup`` but
     never explicitly rotated).
     """
-    if shutil.which("openssl") is None:
-        return None
-    try:
-        res = subprocess.run(  # noqa: S603 -- openssl trusted PATH lookup
-            ["openssl", "x509", "-startdate", "-noout", "-in", str(ca_crt)],
-            capture_output=True, text=True, timeout=10,
-        )
-    except (OSError, subprocess.TimeoutExpired):
-        return None
-    if res.returncode != 0:
-        return None
-    # Output: ``notBefore=Jun  1 12:00:00 2025 GMT``
-    line = res.stdout.strip()
-    if "=" not in line:
-        return None
-    raw = line.split("=", 1)[1].strip()
-    for fmt in ("%b %d %H:%M:%S %Y %Z", "%b %d %H:%M:%S %Y"):
-        try:
-            dt = datetime.strptime(raw, fmt)
-            return dt.replace(tzinfo=timezone.utc)
-        except ValueError:
-            continue
-    return None
+    return _ca_x509_date(ca_crt, "-startdate")
 
 
 def _check_ca_rotation(ca_crt: Path) -> DoctorCheck:

diff --git a/hermes_cli/proxy_cli.py b/hermes_cli/proxy_cli.py
--- a/hermes_cli/proxy_cli.py
+++ b/hermes_cli/proxy_cli.py
@@ -19,7 +19,7 @@
 import argparse
 import os
 from pathlib import Path
-from typing import List
+from typing import Dict, List, Optional, Tuple
 
 from rich.console import Console
 from rich.panel import Panel
@@ -203,6 +203,64 @@
 # ---------------------------------------------------------------------------
 
 
+def _bitwarden_start_options(cfg: Dict) -> Tuple[bool, Optional[Dict]]:
+    proxy_cfg = cfg.get("proxy") or {}
+    credential_source = proxy_cfg.get("credential_source", "env")
+    bw_cfg = (cfg.get("secrets") or {}).get("bitwarden")
+    refresh_bw = (
+        credential_source == "bitwarden"
+        and bw_cfg is not None
+        and bool(bw_cfg.get("enabled"))
+    )
+    # Pass the proxy-side allow_env_fallback opt-in through to
+    # start_proxy.  This is a deliberate, documented escape hatch: when
+    # set, the daemon silently falls back to host env if BWS is
+    # unreachable, instead of raising.  Default is strict (raise).
+    if refresh_bw and bw_cfg is not None:
+        bw_cfg = dict(bw_cfg)
+        bw_cfg["allow_env_fallback"] = bool(
+            proxy_cfg.get("allow_env_fallback", False)
+        )
+    return refresh_bw, bw_cfg
+
+
+def _bitwarden_start_ready(
+    *, refresh_bw: bool, bw_cfg: Optional[Dict], console: Console,
+) -> bool:
+    # stephenschoettler #1: when `credential_source: bitwarden`, the
+    # operator picked BWS specifically to get the rotation guarantee —
+    # silently falling back to parent-env at start_proxy time reintroduces
+    # exactly the bug class the BW mode is supposed to defeat (host env
+    # is stale / mismatched).  Pre-check at the wizard layer so we fail
+    # loud with actionable error messages BEFORE start_proxy degrades.
+    if not refresh_bw:
+        return True
+    bw_access_env = (bw_cfg or {}).get("access_token_env", "BWS_ACCESS_TOKEN")
+    if not os.environ.get(bw_access_env, "").strip():
+        console.print(
+            f"[red]✗ Refusing to start: credential_source=bitwarden but "
+            f"{bw_access_env} is not set in the environment.[/red]"
+        )
+        console.print(
+            "  Either export the access token, or run "
+            "`hermes egress setup --no-bitwarden` to switch back to "
+            "env-based credentials."
+        )
+        return False
+    if not (bw_cfg or {}).get("project_id"):
+        console.print(
+            "[red]✗ Refusing to start: credential_source=bitwarden but "
+            "secrets.bitwarden.project_id is empty.[/red]"
+        )
+        console.print(
+            "  Run `hermes secrets bitwarden setup` to configure the "
+            "project, or switch back via `hermes egress setup "
+            "--no-bitwarden`."
+        )
+        return False
+    return True
+
+
 def cmd_install(args: argparse.Namespace) -> int:
     console = Console()
     try:
@@ -567,22 +625,7 @@
     # rotation guarantee that distinguishes ``credential_source:
     # bitwarden`` from ``credential_source: env``.  Without it, rotating
     # a key in the Bitwarden web app doesn't reach the proxy.
-    credential_source = proxy_cfg.get("credential_source", "env")
-    bw_cfg = (cfg.get("secrets") or {}).get("bitwarden")
-    refresh_bw = (
-        credential_source == "bitwarden"
-        and bw_cfg is not None
-        and bool(bw_cfg.get("enabled"))
-    )
-    # Pass the proxy-side allow_env_fallback opt-in through to
-    # start_proxy.  This is a deliberate, documented escape hatch: when
-    # set, the daemon silently falls back to host env if BWS is
-    # unreachable, instead of raising.  Default is strict (raise).
-    if refresh_bw and bw_cfg is not None:
-        bw_cfg = dict(bw_cfg)
-        bw_cfg["allow_env_fallback"] = bool(
-            proxy_cfg.get("allow_env_fallback", False)
-        )
+    refresh_bw, bw_cfg = _bitwarden_start_options(cfg)
 
     # fail_on_uncovered_providers: when true, refuse to start if any
     # LLM-specific non-bearer providers (Anthropic native, Azure OpenAI,
@@ -610,36 +653,10 @@
             )
             return 1
 
-    # stephenschoettler #1: when `credential_source: bitwarden`, the
-    # operator picked BWS specifically to get the rotation guarantee —
-    # silently falling back to parent-env at start_proxy time reintroduces
-    # exactly the bug class the BW mode is supposed to defeat (host env
-    # is stale / mismatched).  Pre-check at the wizard layer so we fail
-    # loud with actionable error messages BEFORE start_proxy degrades.
-    if refresh_bw:
-        bw_access_env = (bw_cfg or {}).get("access_token_env", "BWS_ACCESS_TOKEN")
-        if not os.environ.get(bw_access_env, "").strip():
-            console.print(
-                f"[red]✗ Refusing to start: credential_source=bitwarden but "
-                f"{bw_access_env} is not set in the environment.[/red]"
-            )
-            console.print(
-                "  Either export the access token, or run "
-                "`hermes egress setup --no-bitwarden` to switch back to "
-                "env-based credentials."
-            )
-            return 1
-        if not (bw_cfg or {}).get("project_id"):
-            console.print(
-                "[red]✗ Refusing to start: credential_source=bitwarden but "
-                "secrets.bitwarden.project_id is empty.[/red]"
-            )
-            console.print(
-                "  Run `hermes secrets bitwarden setup` to configure the "
-                "project, or switch back via `hermes egress setup "
-                "--no-bitwarden`."
-            )
-            return 1
+    if not _bitwarden_start_ready(
+        refresh_bw=refresh_bw, bw_cfg=bw_cfg, console=console,
+    ):
+        return 1
 
     try:
         status = ip.start_proxy(
@@ -847,9 +864,19 @@
 
     # ------------------------------------------------------------- do it
     restart = not getattr(args, "no_restart", False)
+    cfg = load_config()
+    refresh_bw, bw_cfg = _bitwarden_start_options(cfg)
+    if restart and plan.daemon_running and not _bitwarden_start_ready(
+        refresh_bw=refresh_bw, bw_cfg=bw_cfg, console=console,
+    ):
+        return 1
     try:
-        result = ip.rotate_ca(reason=getattr(args, "reason", None),
-                              restart=restart)
+        result = ip.rotate_ca(
+            reason=getattr(args, "reason", None),
+            restart=restart,
+            refresh_secrets_from_bitwarden=refresh_bw,
+            bitwarden_config=bw_cfg,
+        )
     except Exception as exc:  # noqa: BLE001 -- user-facing error funnel
         console.print(f"[red]✗ CA rotation failed:[/red] {exc}")
         return 1

diff --git a/tests/test_iron_proxy_cli.py b/tests/test_iron_proxy_cli.py
--- a/tests/test_iron_proxy_cli.py
+++ b/tests/test_iron_proxy_cli.py
@@ -304,6 +304,77 @@
     assert captured.get("refresh_secrets_from_bitwarden") is False
 
 
+def test_cmd_rotate_ca_passes_bitwarden_refresh_when_restarting(
+    hermes_home, monkeypatch,
+):
+    from hermes_cli.config import load_config, save_config
+    cfg = load_config()
+    cfg.setdefault("proxy", {})["credential_source"] = "bitwarden"
+    cfg.setdefault("secrets", {})["bitwarden"] = {
+        "enabled": True,
+        "project_id": "test-proj-id",
+        "access_token_env": "BWS_ACCESS_TOKEN",
+    }
+    save_config(cfg)
+    monkeypatch.setenv("BWS_ACCESS_TOKEN", "bwsk-test-access-token")
+
+    plan = ip.RotationPlan(
+        ca_crt=hermes_home / "proxy" / "ca.crt",
+        ca_key=hermes_home / "proxy" / "ca.key",
+        archive_path=hermes_home / "proxy" / "ca-archive" / "ca-test.crt",
+        old_fingerprint="deadbeef" * 8,
+        daemon_running=True,
+        sandboxes=[],
+    )
+    captured: dict = {}
+
+    def fake_rotate_ca(**kw):
+        captured.update(kw)
+        plan.new_fingerprint = "feedface" * 8
+        plan.new_valid_until = "2035-06-01T12:00:00+00:00"
+        return plan
+
+    monkeypatch.setattr(ip, "plan_ca_rotation", lambda: plan)
+    monkeypatch.setattr(ip, "rotate_ca", fake_rotate_ca)
+
+    rc = proxy_cli.cmd_rotate_ca(_args())
+    assert rc == 0
+    assert captured.get("refresh_secrets_from_bitwarden") is True
+    assert captured.get("bitwarden_config") is not None
+
+
+def test_cmd_rotate_ca_refuses_when_bitwarden_token_missing(
+    hermes_home, monkeypatch,
+):
+    from hermes_cli.config import load_config, save_config
+    cfg = load_config()
+    cfg.setdefault("proxy", {})["credential_source"] = "bitwarden"
+    cfg.setdefault("secrets", {})["bitwarden"] = {
+        "enabled": True,
+        "project_id": "test-proj-id",
+        "access_token_env": "BWS_ACCESS_TOKEN",
+    }
+    save_config(cfg)
+    monkeypatch.delenv("BWS_ACCESS_TOKEN", raising=False)
+
+    plan = ip.RotationPlan(
+        ca_crt=hermes_home / "proxy" / "ca.crt",
+        ca_key=hermes_home / "proxy" / "ca.key",
+        archive_path=hermes_home / "proxy" / "ca-archive" / "ca-test.crt",
+        daemon_running=True,
+        sandboxes=[],
+    )
+    monkeypatch.setattr(ip, "plan_ca_rotation", lambda: plan)
+
+    def must_not_rotate(**kw):
+        pytest.fail("rotate_ca should not be invoked when BWS token missing")
+
+    monkeypatch.setattr(ip, "rotate_ca", must_not_rotate)
+
+    rc = proxy_cli.cmd_rotate_ca(_args())
+    assert rc == 1
+
+
 # ---------------------------------------------------------------------------
 # cmd_stop, cmd_status, cmd_disable, cmd_config
 # ---------------------------------------------------------------------------

diff --git a/tests/test_iron_proxy_rotate_ca.py b/tests/test_iron_proxy_rotate_ca.py
--- a/tests/test_iron_proxy_rotate_ca.py
+++ b/tests/test_iron_proxy_rotate_ca.py
@@ -105,6 +105,38 @@
     assert calls == ["stop", "start"]
 
 
+def test_rotate_running_daemon_preserves_bitwarden_start_options(
+    hermes_home, monkeypatch,
+):
+    """rotate-ca restarts the daemon with the same Bitwarden refresh options."""
+    _write_ca(hermes_home)
+    monkeypatch.setattr(ip, "_proxy_is_running", lambda: True)
+    monkeypatch.setattr(ip, "list_hermes_sandboxes", lambda: [])
+    calls = []
+    captured = {}
+    monkeypatch.setattr(ip, "stop_proxy", lambda: calls.append("stop"))
+
+    def fake_start_proxy(**kw):
+        calls.append("start")
+        captured.update(kw)
+
+    monkeypatch.setattr(ip, "start_proxy", fake_start_proxy)
+    bw_cfg = {"enabled": True, "project_id": "test-proj-id"}
+
+    ip.rotate_ca(
+        reason="test",
+        restart=True,
+        refresh_secrets_from_bitwarden=True,
+        bitwarden_config=bw_cfg,
+    )
+
+    assert calls == ["stop", "start"]
+    assert captured == {
+        "refresh_secrets_from_bitwarden": True,
+        "bitwarden_config": bw_cfg,
+    }
+
+
 def test_dry_run_plan_does_not_touch_filesystem(hermes_home, monkeypatch):
     """plan_ca_rotation (the dry-run engine) must not change any mtimes."""
     crt = _write_ca(hermes_home)

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 3650b23. Configure here.

# 3. Restart the daemon if it was running and the caller wants it.
if restart and plan.daemon_running:
stop_proxy()
start_proxy()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Daemon restart after rotation loses bitwarden secrets

Medium Severity

rotate_ca calls start_proxy() with no arguments when restarting the daemon. Since start_proxy defaults refresh_secrets_from_bitwarden=False and bitwarden_config=None, operators using credential_source: bitwarden will get a proxy that starts without fetching upstream API keys from bitwarden. The proxy will be running but unable to perform credential injection, silently degrading functionality until the operator manually restarts with the proper bitwarden flags.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3650b23. Configure here.

return dt.replace(tzinfo=timezone.utc)
except ValueError:
continue
return None

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated date-parsing logic in _ca_not_before and _ca_not_after

Low Severity

_ca_not_before is a near-exact copy of the existing _ca_not_after function, differing only in the openssl flag (-startdate vs -enddate) and one comment line. Both share identical openssl-availability checks, subprocess invocation patterns, output parsing, and date-format iteration. A single parameterized helper accepting the flag/field name would eliminate ~25 lines of duplicated logic.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3650b23. Configure here.

@Bartok9

Bartok9 commented May 30, 2026

Copy link
Copy Markdown
Owner Author

Superseded by upstream PR NousResearch#35188 — retargeted to NousResearch/hermes-agent feat/iron-proxy branch with Bugbot fixes baked in. Companion ca-rotation doctor check deferred to follow-up since DoctorCheck lives on the doctor+audit stacked branch.

@Bartok9 Bartok9 closed this May 30, 2026
Bartok9 added a commit that referenced this pull request Jun 6, 2026
…w goals (NousResearch#34196, NousResearch#34197)

Two related /goal bugs:

(review/reflect/suggest/analyze) unless the assistant uses a magic
phrase like 'goal complete'. The synthetic continuation loop escalates
reflection into producing concrete artifacts that the goal only listed
as *examples* of possible help.

untracked` even when the user did not ask for staging/commit/push.
This races with preflight compression and survives session split,
turning a scoped 'done' answer into out-of-scope artifact production.

Both bugs converge on the goal-judge prompt machinery in
`hermes_cli/goals.py`. The fix is layered, minimal, and reviewable:

1. Tighten JUDGE_SYSTEM_PROMPT with three new explicit guardrails:
   - EXPLORATORY goals (review/reflect/suggest/analyze) are completable
     by a substantive synthesis — do NOT require additional artifacts.
   - Do NOT infer incompletion from untracked / unstaged / uncommitted
     files unless the goal explicitly required staging/commit/push.
   - Do NOT require a magic phrase like 'goal complete'.
   - Treat 'for example' / 'maybe' / 'you could' items as illustrative,
     NOT as required deliverables.
   - Scope-narrow goals (one file, one section, one specific change)
     are DONE when that exact scope is confirmed done — do not expand.

2. Add a transparent keyword classifier `_classify_goal_shape(goal)`
   that returns 'exploratory', 'illustrative', or 'concrete'. Cheap,
   reviewer-friendly substring detection — the LLM judge still makes
   the final DONE/CONTINUE call, but it now sees what kind of goal it
   is. Kept intentionally simple so behaviour is easy to audit and
   tune from issue feedback.

3. Append a corresponding goal-shape hint to the user-prompt template
   when the goal is exploratory or illustrative. The hint reminds the
   judge that for those shapes, a high-quality synthesis IS the
   deliverable. Concrete goals get the original strict template
   unchanged.

4. The with-subgoals template (already enforces strict per-criterion
   evidence) deliberately does NOT receive the shape hint — the
   user's explicit /subgoal criteria take precedence over goal-shape
   heuristics.

Why prompt-level vs adding new state:

This intentionally avoids adding new GoalState fields, new gateway
plumbing, or new compression-lifecycle coupling. The goal judge is the
single point where 'should we continue?' is decided; teaching it to
read goal shape correctly is the smallest change that addresses both
issues' root cause without touching the compression race window
described in NousResearch#34197 #2-#5. Those lifecycle concerns are real and
documented in the issue's 'Proposed fixes #3-#6' — they belong in a
separate gateway-side change. This PR fixes the judge's bad
'continue' verdict that triggers the lifecycle problem in the first
place. Without the bad verdict, the race window in NousResearch#34197 has nothing
to race over.

Tests (17 new, all 67 in test_goals.py pass):
- TestClassifyGoalShape (9 tests): exploratory/illustrative/concrete
  classification including the sanitized NousResearch#34196 repro goal text.
- TestJudgeSystemPromptGuardrails (4 tests): system prompt mentions
  exploratory goals, warns about untracked files, warns about
  requiring magic phrases, warns about illustrative examples.
- TestJudgePromptIncludesGoalShapeHint (4 tests): the user prompt
  receives the shape hint for exploratory/illustrative goals, does
  NOT for concrete goals, and the with-subgoals template skips the
  hint to preserve its strict per-criterion evidence rule.

Refs: NousResearch#34196 NousResearch#34197
Closes: NousResearch#34196 NousResearch#34197

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant