main
3 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Ben Barclay
|
613849c190 |
fix(telemetry): close the consent window on the config transition
Fourth independent review. Two more consent leaks, both reproduced through the real relay entry point before and after the fix. Both are failures of my own round-3 fix, which recorded revocation in the wrong place. BLOCKER 1 - revoking while idle recorded nothing. _record_revocation lived inside send_pending's loop, but _send_exported_packages returns early when send is false, before a sender is ever constructed. The dominant case is a user turning sending off while no pass is running, so the loop that was meant to observe the revocation could never run. Reproduced: 6 periods collected during a refused window were transmitted on re-enable. The window now closes on the observed config EDGE, before the early return. Last-seen send state is persisted because each hook fires in a fresh process, so a true->false transition is only visible by comparison. The rising edge also opens the window explicitly: the sender only runs when there is something to send, so a user who opts in and out before any package exists would otherwise have no window for record_revoked to close. BLOCKER 2 - turning COLLECTION off never recorded revocation. The not-enabled branch in setup.py force-set send=false and returned without calling _record_send_consent_change, so `hermes tools` -> disable shared metrics silently dropped consent while leaving the window open. Same retroactive release on re-enable. Both consent surfaces now record, and setup keeps the relay's edge detector in step. Also, from the same review's mutation sweep: - the scheme check is now pinned as an allowlist. Replacing the http test with `if True` survived the entire suite, because every non-http case targeted a REMOTE host where the loopback branch rejects anyway. Only a non-http scheme on loopback distinguishes the two. Shipped behaviour was already correct; nothing guarded it. - A.3 no longer claims rotation bounds long-term linkability outright. Measured against 11 real packages: resource is a stable low-entropy tuple and periods are contiguous across a rotation, so for a RARE configuration those can bridge windows. The honest claim is that rotation raises the cost, not that it makes correlation impossible. Two mutants are documented as unkillable rather than papered over with tests that only appear to cover them: the _defer clamp is unreachable from any current caller, and widening the falling-edge check to an unconditional else is behaviourally equivalent because record_revoked is idempotent and no-ops without an open window. An earlier version of the anti-spurious-revocation test could not fail either - it used a never-consented store, where record_revoked no-ops regardless. Rewritten to opt in, revoke, re-enable, and then assert that a steady enabled state does not re-close the reopened window. 259 tests pass. Staging E2E re-run: both packages 202. |
||
|
Ben Barclay
|
49757d5e39 |
fix(telemetry): address review findings on the shared-metrics sender
Independent review found the claim mechanism did not work. Reproduced against the real store: two senders POSTed the same package. The claim wrote next_attempt_at = now, but selection requires next_attempt_at <= now, so a concurrent pass matched the same row immediately. It now writes a LEASE INTO THE FUTURE (_CLAIM_LEASE_SECONDS), which is what actually excludes another pass, and expires by itself if a process dies mid-send. _mark is additionally guarded on send_state so a straggler whose lease lapsed cannot overwrite a completed send back to pending. The old concurrency test could not fail: it raised AssertionError from inside a transport, and _send_one catches every exception as a retryable transport error. It now records what the second pass saw. Also from review: - shutdown() never joined the send thread; the join was only wired into deactivate(). A short-lived CLI therefore killed an in-flight send at exit, on the only cadence this feature has. - Removed HERMES_TELEMETRY_ENDPOINT. AGENTS.md reserves HERMES_* for secrets, and a behavioural override here was a consent hazard: an inherited variable could silently redirect telemetry a user agreed to send to Nous. The staging E2E writes the endpoint into its throwaway profile instead, which also exercises the real config path. - Added the shared-metrics toggle that AGENTS.md requires as the third opt-in surface, delegating to the setup prompt so the consent rules stay in one place. - Non-429 4xx (401/403/404/413/422) are now permanent. Only 400 was, so a wrong path or oversized body retried every 15 minutes for 30 days until retention pruned it. - The opt-in day is stamped when the user consents, not on the first send pass, which silently dropped the opt-in day whenever the next export crossed midnight UTC. - gzip now uses mtime=0. The embedded timestamp made two sends of one package differ on the wire, so the 'byte-identical retry' E2E was comparing parsed bodies and could not have caught it. It now compares raw request bytes. - Reconciled the three stale claims in relay-shared-metrics.md that said no remote-delivery path exists. 233 tests pass (was 213). Staging E2E re-run through the config path: both packages 202, and the service logged both objects written to S3. |
||
|
Ben Barclay
|
e5180ab3df |
feat(telemetry): add opt-in send config and send-state columns
Step 1+2 of the shared-metrics exporter. Config: telemetry.shared_metrics.send (default false) and .endpoint (default production), resolved by a new shared_metrics_send_config module. Precedence is HERMES_TELEMETRY_ENDPOINT > config > default; the env var exists so the live staging E2E never has to mutate a user's config. send requires enabled and never implies it — that combination is a misconfiguration the user believes is working, so it logs an ERROR once per process rather than silently doing nothing. Plaintext endpoints are refused unless the host is loopback, so a typo cannot send telemetry in clear text. Per AGENTS.md, outbound telemetry needs a user-facing opt-in, so setup_telemetry now prompts for sending as a second, separate question and force-disables send when collection is turned off. Storage: six additive nullable columns on package_outbox for send bookkeeping. The store schema version deliberately does NOT move — _ensure_schema_in_transaction raises on any version it does not recognise and has no forward-compatibility branch, so bumping it would hard-fail an older Hermes, a second profile on an older build, or a rollback, against the same file. Old readers select named columns and never SELECT *, so the additions are invisible to them. Also corrects the two places that promised telemetry is never uploaded (config_defaults comment and cli-config.yaml.example); leaving them would make them false privacy statements once sending ships. Tests: 26 covering config precedence, the enabled/send relationship, transport safety, fresh-database creation, upgrade from a pre-send database (rows preserved, version pinned, idempotent), and that the shipped export query still runs. Mutation-checked: bumping the schema version fails 5 of them. |