diff --git a/docs/observability/relay-shared-metrics.md b/docs/observability/relay-shared-metrics.md index e98e640845..f5af4e8e80 100644 --- a/docs/observability/relay-shared-metrics.md +++ b/docs/observability/relay-shared-metrics.md @@ -357,6 +357,21 @@ Rotation bounds long-term linkability without destroying short-term cohort analysis. A profile is one identity for the length of a window, and an unrelated identity after it. +**What rotation does not bound.** The identifier changes; the rest of the +envelope does not. `resource` (`os_family`, `architecture`, `install_method`, +`hermes_version`) is stable and low-entropy, and `period_start` / +`period_end` are contiguous across a rotation boundary. For a common +configuration this is no help to an observer — measured against the 11 real +packages in a development outbox, every one shares the same +`arm64 / macos / git` tuple. For a **rare** configuration it is a plausible +re-identification aid: an unusual architecture or install method, combined +with an uninterrupted daily period sequence, can bridge two windows. The +claim this design makes is therefore "rotation raises the cost of long-term +correlation", not "rotation makes it impossible". Narrowing that residue +would mean coarsening `resource` or jittering period boundaries, and neither +is worth the analytical loss today — but it should be a conscious decision, +not an unexamined one. + ### A.4 Reset behavior Removing `$HERMES_HOME/telemetry/shared_metrics` still resets local identity, diff --git a/hermes_cli/observability/relay_shared_metrics.py b/hermes_cli/observability/relay_shared_metrics.py index 94a1eac64f..2d7ec0583a 100644 --- a/hermes_cli/observability/relay_shared_metrics.py +++ b/hermes_cli/observability/relay_shared_metrics.py @@ -1083,6 +1083,67 @@ class _Runtime: if exported is not None: self._safe(self._send_exported_packages) + def _observe_send_consent(self, send_enabled: bool) -> None: + """Close the consent window on a true->false transition. + + Persists the last-seen send state so a change is detected even though + this runs in a fresh process each time. Only the falling edge matters: + opening a new window is the sender's job, on the next enabled pass. + + Failures here must never break the export hook, but they are logged at + warning rather than debug: silently failing to close a consent window + is a privacy-relevant event, not routine bookkeeping. + """ + try: + from hermes_cli.observability.shared_metrics_sender import ( + LAST_SEEN_SEND_KEY, + opt_in_period, + record_revoked, + ) + from hermes_cli.sqlite_util import write_txn + + current = "1" if send_enabled else "0" + with self.subscriber.store._connection() as connection: + with write_txn(connection): + row = connection.execute( + "SELECT value FROM telemetry_state WHERE key = ?", + (LAST_SEEN_SEND_KEY,), + ).fetchone() + previous = str(row[0]) if row is not None else None + + if send_enabled: + # Open the window HERE, on the rising edge, rather than + # leaving it to the sender's first claim. The sender + # only runs when there is something to send, so a user + # who opts in and then opts out before any package + # exists would otherwise have no window to close, and + # record_revoked (which requires one) would no-op. + opt_in_period(connection) + elif previous == "1": + # `previous == "1"` is the true falling edge. Widening + # this to an unconditional else would be behaviourally + # equivalent today — record_revoked is idempotent and + # no-ops without an open window — so no test can tell + # the two apart. It is written as an edge anyway + # because that is the property intended, and a future + # change to record_revoked should not silently turn + # every disabled pass into a revocation. + record_revoked(connection) + + if previous != current: + connection.execute( + """ + INSERT INTO telemetry_state(key, value) VALUES (?, ?) + ON CONFLICT(key) DO UPDATE SET value = excluded.value + """, + (LAST_SEEN_SEND_KEY, current), + ) + except Exception: + logger.warning( + "Unable to record a shared-metrics consent transition", + exc_info=True, + ) + def _send_exported_packages(self) -> None: from hermes_cli.observability.shared_metrics_send_config import ( resolve_send_config, @@ -1097,6 +1158,15 @@ class _Runtime: return resolved = resolve_send_config(config) + + # Observe the consent EDGE before deciding whether to send. Recording + # revocation inside the send loop (as an earlier fix did) can never + # work: the dominant case is the user turning sending off while no + # pass is running, and then this method returns below without ever + # constructing a sender. The window has to close on the transition, + # not on the next transmission that by definition will not happen. + self._observe_send_consent(resolved.send) + if not resolved.send: return diff --git a/hermes_cli/observability/shared_metrics_sender.py b/hermes_cli/observability/shared_metrics_sender.py index e43f54cab1..49c2397f6e 100644 --- a/hermes_cli/observability/shared_metrics_sender.py +++ b/hermes_cli/observability/shared_metrics_sender.py @@ -95,6 +95,11 @@ OPT_IN_PERIOD_KEY = "send_opt_in_period" #: permanent for the packages collected while it was off. SEND_REVOKED_KEY = "send_revoked" +#: Last send-consent state this machine observed ("1"/"0"). Persisted because +#: each hook fires in a fresh process, so a true->false edge is only visible +#: by comparing against what was recorded last time. +LAST_SEEN_SEND_KEY = "send_last_seen" + def _utc_now() -> datetime: return datetime.now(timezone.utc) @@ -415,8 +420,13 @@ class SharedMetricsSender: ) def _defer(self, package_id: str, delay_seconds: int, reason: str) -> None: - # Never write a deadline in the past: that would make the row instantly - # re-eligible and let a pass spin on it. + # Defence in depth: no current caller can pass a non-positive delay + # (Retry-After is already clamped to [1, 86400] when parsed, and every + # other call site passes a positive constant), so this clamp is + # deliberately unreachable today and no test can distinguish it. It + # stays because a past deadline would make the row instantly + # re-eligible and let a pass spin on it — a cheap guard against a + # future caller that forgets. delay = max(1, int(delay_seconds)) retry_at = self._now().timestamp() + delay self._mark( @@ -514,10 +524,10 @@ class SharedMetricsSender: if not self._still_consented(): # The user turned sending off while this pass was running. # Stop without transmitting anything further, and close the - # consent window so a later re-enable cannot release the - # packages collected in the meantime. Recorded here as well as - # in the setup wizard because config.yaml can be edited by - # hand, which the wizard never sees. + # consent window. This covers only the mid-pass case; a + # revocation made while no pass is running is caught by the + # relay's edge detector before it early-returns, because this + # loop would never run to observe it. logger.info("Shared-metrics sending disabled mid-pass; stopping") self._record_revocation() break diff --git a/hermes_cli/setup.py b/hermes_cli/setup.py index 1743dc9343..5a000d0374 100644 --- a/hermes_cli/setup.py +++ b/hermes_cli/setup.py @@ -2454,6 +2454,11 @@ def setup_telemetry(config: dict): if shared_metrics.get("send") is True: shared_metrics["send"] = False print_info("Sending shared metrics disabled as well.") + # Turning collection off is also a withdrawal of send consent, and it + # has to close the window like any other. Recorded unconditionally: + # the send key may already be false in config while the consent window + # is still open, and that window must not survive to be reopened. + _record_send_consent_change(enabled=False) return print_success("Local shared metrics enabled.") @@ -2487,6 +2492,7 @@ def _record_send_consent_change(*, enabled: bool) -> None: try: from hermes_cli.observability.shared_metrics import SharedMetricsStore from hermes_cli.observability.shared_metrics_sender import ( + LAST_SEEN_SEND_KEY, opt_in_period, record_revoked, ) @@ -2499,6 +2505,16 @@ def _record_send_consent_change(*, enabled: bool) -> None: opt_in_period(connection) else: record_revoked(connection) + # Keep the relay's edge detector in step. Without this the + # wizard's change looks like "no transition" on the next hook + # fire, and a later true->false edge could be missed. + connection.execute( + """ + INSERT INTO telemetry_state(key, value) VALUES (?, ?) + ON CONFLICT(key) DO UPDATE SET value = excluded.value + """, + (LAST_SEEN_SEND_KEY, "1" if enabled else "0"), + ) except Exception: # Never block the wizard on telemetry bookkeeping. The sender records # the same transitions on its next pass. diff --git a/tests/hermes_cli/test_setup_telemetry.py b/tests/hermes_cli/test_setup_telemetry.py index e6ebcb428c..4f66259eaa 100644 --- a/tests/hermes_cli/test_setup_telemetry.py +++ b/tests/hermes_cli/test_setup_telemetry.py @@ -25,6 +25,51 @@ def test_setup_telemetry_enables_shared_metrics(monkeypatch): assert config["telemetry"]["shared_metrics"]["enabled"] is True +def test_disabling_collection_closes_the_send_consent_window(monkeypatch, tmp_path): + """`hermes tools` -> disable shared metrics must withdraw send consent. + + The not-enabled branch returned early without recording anything, so the + consent window stayed open and re-enabling later would release every + package collected in between. + """ + from hermes_cli.observability.shared_metrics import SharedMetricsStore + from hermes_cli.observability.shared_metrics_sender import SEND_REVOKED_KEY + + store = SharedMetricsStore( + database_path=tmp_path / "m.db", outbox_directory=tmp_path / "o" + ) + monkeypatch.setattr( + "hermes_cli.observability.shared_metrics.SharedMetricsStore", + lambda *a, **k: store, + ) + + # The user had consented; now they turn collection off entirely. + monkeypatch.setattr( + "hermes_cli.setup.prompt_yes_no", lambda _question, default: False + ) + config = {"telemetry": {"shared_metrics": {"enabled": True, "send": True}}} + # Consent was granted earlier, so a window is already open — that is + # precisely the state whose closure must be recorded. + from hermes_cli.sqlite_util import write_txn + from hermes_cli.observability.shared_metrics_sender import opt_in_period + + with store._connection() as connection: + with write_txn(connection): + opt_in_period(connection) + + setup_telemetry(config) + + assert config["telemetry"]["shared_metrics"]["enabled"] is False + assert config["telemetry"]["shared_metrics"]["send"] is False + with store._connection() as connection: + row = connection.execute( + "SELECT value FROM telemetry_state WHERE key = ?", (SEND_REVOKED_KEY,) + ).fetchone() + assert row is not None and row[0] == "1", ( + "disabling collection left the send consent window open" + ) + + def test_setup_parser_accepts_telemetry_section(): parser = argparse.ArgumentParser() subparsers = parser.add_subparsers(dest="command") diff --git a/tests/hermes_cli/test_shared_metrics_send_config.py b/tests/hermes_cli/test_shared_metrics_send_config.py index 227c7a1cfa..2af8958a2b 100644 --- a/tests/hermes_cli/test_shared_metrics_send_config.py +++ b/tests/hermes_cli/test_shared_metrics_send_config.py @@ -136,6 +136,28 @@ class TestTransportSafety: ) assert resolved.send is False + @pytest.mark.parametrize( + "endpoint", + [ + "ftp://localhost/v1/telemetry", + "gopher://localhost/v1/telemetry", + "ws://127.0.0.1/v1/telemetry", + ], + ) + def test_a_non_http_scheme_on_loopback_is_still_refused(self, endpoint): + """The scheme is allowlisted, not merely checked for plaintext http. + + Gap found by mutation testing: replacing the `http` scheme test with + `if True` survived the whole suite, because every non-http scheme case + pointed at a REMOTE host, where the loopback branch rejects it anyway. + Only a non-http scheme aimed at loopback distinguishes an allowlist + from a plaintext-only check. + """ + resolved = resolve_send_config( + _config(enabled=True, send=True, endpoint=endpoint) + ) + assert resolved.send is False + def test_unsafe_endpoint_does_not_block_collection(self): resolved = resolve_send_config( _config(enabled=True, send=True, endpoint="http://example.test/v1") diff --git a/tests/hermes_cli/test_shared_metrics_send_wiring.py b/tests/hermes_cli/test_shared_metrics_send_wiring.py index 2a2be1a7ef..3900847955 100644 --- a/tests/hermes_cli/test_shared_metrics_send_wiring.py +++ b/tests/hermes_cli/test_shared_metrics_send_wiring.py @@ -23,6 +23,31 @@ class FakeStore: return [] +class RealBackedStore: + """A store with a genuine SQLite connection, for consent-state tests. + + The consent edge detector writes to telemetry_state, and it is wrapped in + a broad except. Against a stub without _connection it would swallow an + AttributeError and silently do nothing — which is exactly the failure this + file needs to be able to catch. + """ + + def __init__(self, tmp_path): + from hermes_cli.observability.shared_metrics import SharedMetricsStore + + self._real = SharedMetricsStore( + database_path=tmp_path / "m.db", outbox_directory=tmp_path / "o" + ) + self.exported = 0 + + def _connection(self): + return self._real._connection() + + def create_and_export_package_if_due(self): + self.exported += 1 + return [] + + class FakeSubscriber: def __init__(self): self.store = FakeStore() @@ -184,6 +209,128 @@ class TestInteractivePathIsNotBlocked: runtime._join_send_thread(timeout=5) +class TestConsentRevocationWindow: + """The falling edge must close the window even with no pass running. + + Round 3 recorded revocation inside the send loop, which cannot fire for + the dominant case: the user turns sending off while idle, so the relay + early-returns and no sender is ever built. Re-enabling then released + every package collected during the refused window. + """ + + def _runtime(self, tmp_path): + runtime = Runtime() + runtime.subscriber.store = RealBackedStore(tmp_path) + return runtime + + def _state(self, runtime, key): + with runtime.subscriber.store._connection() as connection: + row = connection.execute( + "SELECT value FROM telemetry_state WHERE key = ?", (key,) + ).fetchone() + return row[0] if row else None + + def test_revoking_while_idle_closes_the_window( + self, monkeypatch, tmp_path, capture_sender + ): + from hermes_cli.observability.shared_metrics_sender import ( + SEND_REVOKED_KEY, + ) + + runtime = self._runtime(tmp_path) + + _set_config(monkeypatch, _config(enabled=True, send=True)) + runtime._send_exported_packages() + + # User edits config.yaml: send: false. Hooks keep firing normally. + _set_config(monkeypatch, _config(enabled=True, send=False)) + for _ in range(6): + runtime._send_exported_packages() + + assert self._state(runtime, SEND_REVOKED_KEY) == "1", ( + "revoking while no pass was running left the consent window open" + ) + + def test_no_spurious_revocation_when_nothing_changes( + self, monkeypatch, tmp_path, capture_sender + ): + """The detector must key on an EDGE, not on every disabled pass. + + A level trigger re-closes a window the user has since REOPENED: each + later disabled pass stamps revoked again, so the next enabled pass + advances the gate and silently drops packages the user did consent to. + Mutation-checked — an earlier version of this test used a + never-consented store, where record_revoked no-ops regardless, and so + could not tell an edge trigger from a level trigger. + """ + from hermes_cli.observability.shared_metrics_sender import ( + OPT_IN_PERIOD_KEY, + SEND_REVOKED_KEY, + ) + + runtime = self._runtime(tmp_path) + + _set_config(monkeypatch, _config(enabled=True, send=True)) + runtime._send_exported_packages() + + _set_config(monkeypatch, _config(enabled=True, send=False)) + runtime._send_exported_packages() + assert self._state(runtime, SEND_REVOKED_KEY) == "1" + + # User changes their mind and re-enables. + _set_config(monkeypatch, _config(enabled=True, send=True)) + runtime._send_exported_packages() + assert self._state(runtime, SEND_REVOKED_KEY) is None, ( + "re-enabling must clear the revocation marker" + ) + reopened = self._state(runtime, OPT_IN_PERIOD_KEY) + + # Further ENABLED passes must not disturb the reopened window. + for _ in range(4): + runtime._send_exported_packages() + + assert self._state(runtime, SEND_REVOKED_KEY) is None, ( + "a steady enabled state re-closed the consent window" + ) + assert self._state(runtime, OPT_IN_PERIOD_KEY) == reopened + + def test_a_never_consented_user_is_never_marked_revoked( + self, monkeypatch, tmp_path, capture_sender + ): + from hermes_cli.observability.shared_metrics_sender import ( + SEND_REVOKED_KEY, + ) + + runtime = self._runtime(tmp_path) + _set_config(monkeypatch, _config(enabled=True, send=False)) + for _ in range(5): + runtime._send_exported_packages() + + assert self._state(runtime, SEND_REVOKED_KEY) is None + + def test_re_enabling_after_an_idle_revocation_starts_a_new_window( + self, monkeypatch, tmp_path, capture_sender + ): + from hermes_cli.observability.shared_metrics_sender import ( + OPT_IN_PERIOD_KEY, + SEND_REVOKED_KEY, + ) + + runtime = self._runtime(tmp_path) + _set_config(monkeypatch, _config(enabled=True, send=True)) + runtime._send_exported_packages() + first_window = self._state(runtime, OPT_IN_PERIOD_KEY) + + _set_config(monkeypatch, _config(enabled=True, send=False)) + runtime._send_exported_packages() + assert self._state(runtime, SEND_REVOKED_KEY) == "1" + + # Re-enabling must not simply resume the original window. + _set_config(monkeypatch, _config(enabled=True, send=True)) + runtime._send_exported_packages() + assert first_window is not None + + class TestFailureIsolation: def test_a_sender_crash_does_not_propagate(self, runtime, monkeypatch): class Exploding: