From 87ce0d5e522d648dc29acfbaea01a072dd715cc2 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 20:32:28 -0300 Subject: [PATCH 01/35] Redact OTel trace_id/span_id before hashing the error dedup fingerprint. A W3C span_id (16 hex chars) fell under the existing _HEX redaction threshold (20+ chars) and leaked into stack_sig(), so the same recurring error got a different fingerprint on every occurrence and never deduped. trace_id (32 hex chars) was already caught by _HEX; only span_id needed a targeted fix. --- src/agent_cli/errors.py | 4 ++++ tests/test_errors.py | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/src/agent_cli/errors.py b/src/agent_cli/errors.py index 85d1147..9a734c1 100644 --- a/src/agent_cli/errors.py +++ b/src/agent_cli/errors.py @@ -28,6 +28,8 @@ r'(?i)("[^"]*(?:password|secret|token|api[_-]?key|access[_-]?token|client[_-]?secret|authorization|passwd|access_key)[^"]*"\s*:\s*")[^"]*(")' ) _HEX = re.compile(r"\b[a-fA-F0-9]{20,}\b") +_OTEL_TRACE_ID = re.compile(r"\btrace_id=[0-9a-fA-F]{32}\b") +_OTEL_SPAN_ID = re.compile(r"\bspan_id=[0-9a-fA-F]{16}\b") _SECRET = re.compile( r"(?i)(? str: out = _AKIA.sub("[redacted]", out) out = _JWT.sub("[redacted]", out) out = _EMAIL.sub("[redacted]", out) + out = _OTEL_TRACE_ID.sub("trace_id=[redacted]", out) + out = _OTEL_SPAN_ID.sub("span_id=[redacted]", out) out = _HEX.sub("[redacted]", out) return out diff --git a/tests/test_errors.py b/tests/test_errors.py index 04b0b6c..7356cef 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -168,6 +168,13 @@ def test_redact_and_fingerprint() -> None: fp = fingerprint(service="api", error_class="TimeoutError", stack_sig=sig, environment="prod") assert fp.startswith("api|TimeoutError|") assert fp.endswith("|prod") + trace_id = "a" * 32 + span_id = "b" * 16 + otel = redact(f"TimeoutError boom trace_id={trace_id} span_id={span_id}") + assert trace_id not in otel + assert span_id not in otel + assert "trace_id=[redacted]" in otel + assert "span_id=[redacted]" in otel def test_scan_inserts_once_then_enriches(tmp_path: Path) -> None: @@ -208,6 +215,40 @@ def fetch(_cfg: dict, _cursor: str | None) -> tuple[list[dict], str | None]: assert "line_fingerprint" not in payload +def test_scan_dedups_across_different_otel_ids(tmp_path: Path) -> None: + store = Store(tmp_path) + _runner_session(store) + _write_config(tmp_path) + line1 = ( + "TimeoutError boom " + "trace_id=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa " + "span_id=bbbbbbbbbbbbbbbb" + ) + line2 = ( + "TimeoutError boom " + "trace_id=cccccccccccccccccccccccccccccccc " + "span_id=dddddddddddddddd" + ) + calls = {"n": 0} + + def fetch(_cfg: dict, _cursor: str | None) -> tuple[list[dict], str | None]: + calls["n"] += 1 + if calls["n"] == 1: + return ([{"ts": "2026-08-23T16:00:00Z", "line": line1}], "2026-08-23T16:00:00Z") + return ([{"ts": "2026-08-23T16:00:01Z", "line": line2}], "2026-08-23T16:00:01Z") + + created, enriched = scan_errors(store, fetch) + assert enriched == [] + assert len(created) == 1 + + created2, enriched2 = scan_errors(store, fetch) + assert created2 == [] + assert enriched2 == created + again = store.row("activity", created[0]) + assert again is not None + assert again["payload"]["count"] == 2 + + def test_line_fingerprint_is_sha256_of_server_container_line() -> None: import hashlib From d3dae2556623d4d34b0b09a82cfc52484e34f946 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 20:32:32 -0300 Subject: [PATCH 02/35] Push newly-created local activity to the hub at the end of each knock scan cycle. cmd_knock's scan loop wrote new/enriched activity rows locally but never called into the hub-push path; _sync_once() only ran on websocket (re)connect or in response to an inbound hub message, so a stable idle connection left local writes unpushed indefinitely. Extracted the scan cycle into _knock_scan_cycle() and call _sync_once() at the end of it when the device is paired. --- src/agent_cli/main.py | 132 ++++++++++++++++++--------------- tests/test_knock_scan_cycle.py | 65 ++++++++++++++++ 2 files changed, 136 insertions(+), 61 deletions(-) create mode 100644 tests/test_knock_scan_cycle.py diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index f85449b..23213c0 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -15,7 +15,7 @@ import webbrowser from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path -from typing import Any +from typing import Any, Callable from urllib.parse import urlparse from websockets.exceptions import WebSocketException @@ -2904,6 +2904,75 @@ def cmd_lane(args: list[str]) -> None: raise SystemExit(2) +def _knock_scan_cycle(store: Store, run_argv: Callable) -> None: + from .pending import scan_pending + + try: + usage_id = scan_usage(store) + if usage_id: + print(f"usage.snapshot {usage_id}") + except AuthStale: + pass + except StoreError as exc: + print(f"usage.snapshot error: {exc}", file=sys.stderr) + try: + created, skipped = scan_merged(store, run_argv) + for activity_id in created: + print(f"pr.merged {activity_id}") + if skipped: + print(f"watch skipped {skipped} pr.open rows", file=sys.stderr) + except StoreError as exc: + print(f"pr.merged error: {exc}", file=sys.stderr) + hub_url = store.meta("hub_url") + hub_token = store.meta("device_token") + if hub_url and hub_token: + hub = Hub(hub_url, hub_token) + try: + lines = scan_pending(store, hub) + for line in lines: + print(line) + except (HubError, StoreError) as exc: + print(f"pending error: {exc}", file=sys.stderr) + finally: + hub.close() + from .github_act import scan_github + from .mail_act import scan_mail + + try: + for line in scan_github(store, run_argv): + print(line) + except StoreError as exc: + print(f"github pending error: {exc}", file=sys.stderr) + try: + for line in scan_mail(store, run_argv): + print(line) + except StoreError as exc: + print(f"mail pending error: {exc}", file=sys.stderr) + from .errors import config_path, default_fetch, scan_errors + + if config_path(store.home).is_file(): + try: + created, enriched = scan_errors(store, default_fetch) + for activity_id in created: + print(f"error.seen {activity_id}") + for activity_id in enriched: + print(f"error.seen enrich {activity_id}") + except StoreError as exc: + print(f"error.seen error: {exc}", file=sys.stderr) + from .error_fix_act import scan_error_fix + + try: + for line in scan_error_fix(store, run_argv): + print(line) + except StoreError as exc: + print(f"error.fix error: {exc}", file=sys.stderr) + if hub_url and hub_token: + try: + _sync_once(store) + except (HubError, StoreError) as exc: + print(f"sync error: {exc}", file=sys.stderr) + + def cmd_knock(args: list[str]) -> None: once = "--once" in args store = open_store() @@ -2913,71 +2982,12 @@ def cmd_knock(args: list[str]) -> None: for activity_id, status in knock_drain(store, runtime): print(f"knock {activity_id} {status}") return - from .pending import scan_pending from .runtime import run_argv last_poll: float | None = None while True: if usage_poll_due(last_poll, time.monotonic()): - try: - usage_id = scan_usage(store) - if usage_id: - print(f"usage.snapshot {usage_id}") - except AuthStale: - pass - except StoreError as exc: - print(f"usage.snapshot error: {exc}", file=sys.stderr) - try: - created, skipped = scan_merged(store, run_argv) - for activity_id in created: - print(f"pr.merged {activity_id}") - if skipped: - print(f"watch skipped {skipped} pr.open rows", file=sys.stderr) - except StoreError as exc: - print(f"pr.merged error: {exc}", file=sys.stderr) - hub_url = store.meta("hub_url") - hub_token = store.meta("device_token") - if hub_url and hub_token: - hub = Hub(hub_url, hub_token) - try: - lines = scan_pending(store, hub) - for line in lines: - print(line) - except (HubError, StoreError) as exc: - print(f"pending error: {exc}", file=sys.stderr) - finally: - hub.close() - from .github_act import scan_github - from .mail_act import scan_mail - - try: - for line in scan_github(store, run_argv): - print(line) - except StoreError as exc: - print(f"github pending error: {exc}", file=sys.stderr) - try: - for line in scan_mail(store, run_argv): - print(line) - except StoreError as exc: - print(f"mail pending error: {exc}", file=sys.stderr) - from .errors import config_path, default_fetch, scan_errors - - if config_path(store.home).is_file(): - try: - created, enriched = scan_errors(store, default_fetch) - for activity_id in created: - print(f"error.seen {activity_id}") - for activity_id in enriched: - print(f"error.seen enrich {activity_id}") - except StoreError as exc: - print(f"error.seen error: {exc}", file=sys.stderr) - from .error_fix_act import scan_error_fix - - try: - for line in scan_error_fix(store, run_argv): - print(line) - except StoreError as exc: - print(f"error.fix error: {exc}", file=sys.stderr) + _knock_scan_cycle(store, run_argv) last_poll = time.monotonic() activity_id = knock_listen(store, runtime, timeout=30.0) if activity_id: diff --git a/tests/test_knock_scan_cycle.py b/tests/test_knock_scan_cycle.py new file mode 100644 index 0000000..8686162 --- /dev/null +++ b/tests/test_knock_scan_cycle.py @@ -0,0 +1,65 @@ +from __future__ import annotations + +import os +from pathlib import Path + +import pytest + +from agent_cli import main as main_mod +from agent_cli.main import open_store + + +def _init_paired_store(tmp_path: Path) -> None: + os.environ["AGENT_HOME"] = str(tmp_path) + main_mod.main(["init"]) + store = open_store() + try: + store.set_meta("hub_url", "https://hub.example") + store.set_meta("device_token", "fake-token") + finally: + store.close() + + +def _stub_scans(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(main_mod, "scan_usage", lambda store: None) + monkeypatch.setattr(main_mod, "scan_merged", lambda store, run_argv: ([], 0)) + monkeypatch.setattr("agent_cli.pending.scan_pending", lambda store, hub: []) + monkeypatch.setattr("agent_cli.github_act.scan_github", lambda store, run_argv: []) + monkeypatch.setattr("agent_cli.mail_act.scan_mail", lambda store, run_argv: []) + monkeypatch.setattr("agent_cli.error_fix_act.scan_error_fix", lambda store, run_argv: []) + + +def test_knock_scan_cycle_pushes_when_paired( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + calls: list[int] = [] + monkeypatch.setattr(main_mod, "_sync_once", lambda store: calls.append(1)) + _stub_scans(monkeypatch) + _init_paired_store(tmp_path) + + store = open_store() + try: + main_mod._knock_scan_cycle(store, lambda _argv: None) + finally: + store.close() + + assert calls == [1] + + +def test_knock_scan_cycle_skips_sync_when_unpaired( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + calls: list[int] = [] + monkeypatch.setattr(main_mod, "_sync_once", lambda store: calls.append(1)) + _stub_scans(monkeypatch) + + os.environ["AGENT_HOME"] = str(tmp_path) + main_mod.main(["init"]) + + store = open_store() + try: + main_mod._knock_scan_cycle(store, lambda _argv: None) + finally: + store.close() + + assert calls == [] From 36d18868b71b20cf17783b0fbfe9d513237c6bb1 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 20:47:57 -0300 Subject: [PATCH 03/35] Address PR review: convention fix, cursor regression guard, daemon crash guard. Import Callable from collections.abc instead of typing and parametrize it, matching every other run_argv/runner callback signature in this codebase. Give mark_pushed() the same monotonic guard mark_origin() already has, so a concurrent push from cmd_knock and cmd_sync --follow cannot regress the push cursor. Catch the bare SystemExit _sync_once() raises on a malformed hub pull response at the new knock call site, so a bad response logs and continues instead of killing the daemon. --- src/agent_cli/main.py | 8 +++++--- src/agent_cli/store.py | 4 +++- tests/test_knock_scan_cycle.py | 23 +++++++++++++++++++++++ tests/test_store.py | 7 +++++++ 4 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 23213c0..96df6c8 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -13,9 +13,10 @@ import time import uuid import webbrowser +from collections.abc import Callable from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path -from typing import Any, Callable +from typing import Any from urllib.parse import urlparse from websockets.exceptions import WebSocketException @@ -36,6 +37,7 @@ from .lane import LANE_ROLES, LANE_VENDORS, LaneResult, launch from .pg import PgError, cluster_exists, cluster_running, ensure_cluster, require_loopback_dsn, stop_cluster from .runtime import ( + Completed, Runtime, grok_model, grok_new_session_id, @@ -2904,7 +2906,7 @@ def cmd_lane(args: list[str]) -> None: raise SystemExit(2) -def _knock_scan_cycle(store: Store, run_argv: Callable) -> None: +def _knock_scan_cycle(store: Store, run_argv: Callable[[list[str]], Completed]) -> None: from .pending import scan_pending try: @@ -2969,7 +2971,7 @@ def _knock_scan_cycle(store: Store, run_argv: Callable) -> None: if hub_url and hub_token: try: _sync_once(store) - except (HubError, StoreError) as exc: + except (HubError, StoreError, SystemExit) as exc: print(f"sync error: {exc}", file=sys.stderr) diff --git a/src/agent_cli/store.py b/src/agent_cli/store.py index 3c5030a..e88db7c 100644 --- a/src/agent_cli/store.py +++ b/src/agent_cli/store.py @@ -522,7 +522,9 @@ def pending_events(self) -> list[dict[str, Any]]: @_wrap_pg_errors def mark_pushed(self, seq: int) -> None: - self.sync_set("pushed_origin_seq", str(seq)) + current = int(self.sync_get("pushed_origin_seq", "0") or "0") + if seq > current: + self.sync_set("pushed_origin_seq", str(seq)) def origin_cursor(self, origin: str) -> int: raw = self.sync_get(f"origin:{origin}", "0") diff --git a/tests/test_knock_scan_cycle.py b/tests/test_knock_scan_cycle.py index 8686162..844b9a0 100644 --- a/tests/test_knock_scan_cycle.py +++ b/tests/test_knock_scan_cycle.py @@ -63,3 +63,26 @@ def test_knock_scan_cycle_skips_sync_when_unpaired( store.close() assert calls == [] + + +def test_knock_scan_cycle_logs_and_continues_on_malformed_pull_response( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: _sync_once() calls die() (bare SystemExit, not a HubError/ + StoreError) when the hub returns a malformed pull payload. A narrow except that + only caught (HubError, StoreError) would let that SystemExit propagate and kill + the whole cmd_knock daemon loop instead of logging and moving on like every + other scan in this function.""" + + def _raise(_store: object) -> None: + raise SystemExit("agent: pull response missing events") + + monkeypatch.setattr(main_mod, "_sync_once", _raise) + _stub_scans(monkeypatch) + _init_paired_store(tmp_path) + + store = open_store() + try: + main_mod._knock_scan_cycle(store, lambda _argv: None) + finally: + store.close() diff --git a/tests/test_store.py b/tests/test_store.py index 279dfee..376767b 100644 --- a/tests/test_store.py +++ b/tests/test_store.py @@ -45,6 +45,13 @@ def test_write_emits_seq_and_blocks_foreign(tmp_path: Path) -> None: assert pending[0]["origin_seq"] == 1 +def test_mark_pushed_does_not_regress_the_cursor(tmp_path: Path) -> None: + store = Store(tmp_path) + store.mark_pushed(10) + store.mark_pushed(3) + assert store.sync_get("pushed_origin_seq", "0") == "10" + + def test_remote_gap_fail_closed(tmp_path: Path) -> None: store = Store(tmp_path) with pytest.raises(StoreError, match="gap"): From 421cb03ca24a0af9d42b89b2f97c175bb85e7f94 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 20:58:11 -0300 Subject: [PATCH 04/35] Document the new knock hub push and assert the SystemExit test's log line. README.md and DESIGN.md described the knock daemon's poll list without mentioning the hub push added at the end of each paired cycle. Also assert on the actual stderr text in the malformed-pull-response regression test, matching the existing test_knock_daemon_polls_usage pattern, instead of only checking that SystemExit doesn't propagate. --- DESIGN.md | 2 +- README.md | 4 ++-- tests/test_knock_scan_cycle.py | 6 +++++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 661c739..b8fc691 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -40,7 +40,7 @@ The AI session talks **only** to the local database. Scripts perform every actio | Runtime | This public client. Team-specific rules live elsewhere and must not ship a second store binary. | | Session mail | Addressed to a **session id**. Delivery does not require a subscription. | | TUI knock | Script wakes the session with only `da ist Post id `. The agent reads that row from local Postgres. | -| Device daemon | Always-on user service on this device. `agent init` installs and starts it with knock (`LISTEN` plus usage / pending / github pending / mail pending / `pr.merged` polls) and the local dashboard; daemon `sync --follow` starts only after `agent pair`, once `device.json` has token and hub URL. | +| Device daemon | Always-on user service on this device. `agent init` installs and starts it with knock (`LISTEN` plus usage / pending / github pending / mail pending / `pr.merged` polls, plus a hub push of new local activity each cycle once paired) and the local dashboard; daemon `sync --follow` starts only after `agent pair`, once `device.json` has token and hub URL. | | Outside facts | Scripts notice GitHub (and other outside) state. The agent is not told by a human and does not poll GitHub. Example: a recorded PR merges → script writes `pr.merged` on that session and knocks. | | AI vs scripts | The AI inserts local intent. Scripts perform every side effect that leaves the machine. Model text is never a state transition. | | Checks and gates | A **check** records a fact (`agent check record`). A **gate** is a policy verdict over evidence (`agent gate record`). A model claim is neither. Confidence is not proof. | diff --git a/README.md b/README.md index dbcf15e..1a0a017 100644 --- a/README.md +++ b/README.md @@ -97,14 +97,14 @@ agent watch assigned [--follow] # allowlisted assignments; needs `gh` and `$AGE agent watch errors # one scan; $AGENT_HOME/error-fix.json; no log host in this package agent watch error-fix # one scan; find-or-create implement task + isolated worktree agent supervise --session ID [--repo OWNER/REPO --number N] [--once|--follow] -# agent knock (daemon, no --once) polls grok-usage, pending, pr.merged, github pending, mail pending, errors, and error-fix every 60s +# agent knock (daemon, no --once) polls grok-usage, pending, pr.merged, github pending, mail pending, errors, and error-fix every 60s, then pushes any new local activity to the hub when paired ``` `agent supervise` posts a short status line to Telegram when both `TELEGRAM_BOT_TOKEN` and `TELEGRAM_CHAT_ID` are set in the environment. The follow CLI does not ask closed questions. Working vs not working for paging is whether the Grok tmux session exists: it posts `not working` only when that session is gone, not when the prompt is idle between turns. The TUI working probe (`Thinking…`, `Waiting for response`, `Preparing …`, `[stop]`, `Esc:cancel`, `command still running`, queued `Enter to send now`) is for the follow loop, not for Telegram. A send failure is printed to stderr and does not stop the loop. Credentials stay out of git. The error-fix executor find-or-creates the implement task and isolated worktree; `agent github pending` still opens draft pull requests. -`agent watch grok-usage` uses the existing Grok login token from the Grok auth file, does not start a Grok session, and does not knock the TUI. Each `usage.snapshot` includes the account email, provider, and subscription tier. Under the device daemon, the knock child records those snapshots (and scans pending, `pr.merged`, github pending, mail pending, errors when `$AGENT_HOME/error-fix.json` exists, and pending `error.fix`) on the same interval. `agent daemon --install` / `--uninstall` manage the user service; `agent init` already installs and starts it. +`agent watch grok-usage` uses the existing Grok login token from the Grok auth file, does not start a Grok session, and does not knock the TUI. Each `usage.snapshot` includes the account email, provider, and subscription tier. Under the device daemon, the knock child records those snapshots (and scans pending, `pr.merged`, github pending, mail pending, errors when `$AGENT_HOME/error-fix.json` exists, and pending `error.fix`) on the same interval, then pushes any new local activity to the hub when the device is paired. `agent daemon --install` / `--uninstall` manage the user service; `agent init` already installs and starts it. `agent watch assigned` reads `$AGENT_HOME/watch.json`: diff --git a/tests/test_knock_scan_cycle.py b/tests/test_knock_scan_cycle.py index 844b9a0..1045629 100644 --- a/tests/test_knock_scan_cycle.py +++ b/tests/test_knock_scan_cycle.py @@ -66,7 +66,7 @@ def test_knock_scan_cycle_skips_sync_when_unpaired( def test_knock_scan_cycle_logs_and_continues_on_malformed_pull_response( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch + tmp_path: Path, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch ) -> None: """Regression test: _sync_once() calls die() (bare SystemExit, not a HubError/ StoreError) when the hub returns a malformed pull payload. A narrow except that @@ -80,9 +80,13 @@ def _raise(_store: object) -> None: monkeypatch.setattr(main_mod, "_sync_once", _raise) _stub_scans(monkeypatch) _init_paired_store(tmp_path) + capsys.readouterr() store = open_store() try: main_mod._knock_scan_cycle(store, lambda _argv: None) finally: store.close() + + captured = capsys.readouterr() + assert "sync error: agent: pull response missing events" in captured.err From bf4e7a8087d15cdc4fad0f4afdb32bef60834a57 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 21:05:25 -0300 Subject: [PATCH 05/35] Fix docs to say hub sync (push+pull), not push, and align a test name. _sync_once() pushes then pulls; the doc lines describing the new per-cycle hub call said only 'push', underselling what actually happens. Also update DESIGN.md's operator checklist (the second description of what agent init's daemon does) to match the contract table, and rename a test to use the same verb as its sibling. --- DESIGN.md | 4 ++-- README.md | 4 ++-- tests/test_knock_scan_cycle.py | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index b8fc691..d726887 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -40,7 +40,7 @@ The AI session talks **only** to the local database. Scripts perform every actio | Runtime | This public client. Team-specific rules live elsewhere and must not ship a second store binary. | | Session mail | Addressed to a **session id**. Delivery does not require a subscription. | | TUI knock | Script wakes the session with only `da ist Post id `. The agent reads that row from local Postgres. | -| Device daemon | Always-on user service on this device. `agent init` installs and starts it with knock (`LISTEN` plus usage / pending / github pending / mail pending / `pr.merged` polls, plus a hub push of new local activity each cycle once paired) and the local dashboard; daemon `sync --follow` starts only after `agent pair`, once `device.json` has token and hub URL. | +| Device daemon | Always-on user service on this device. `agent init` installs and starts it with knock (`LISTEN` plus usage / pending / github pending / mail pending / `pr.merged` polls, plus a hub sync — push then pull — each cycle once paired) and the local dashboard; daemon `sync --follow` starts only after `agent pair`, once `device.json` has token and hub URL. | | Outside facts | Scripts notice GitHub (and other outside) state. The agent is not told by a human and does not poll GitHub. Example: a recorded PR merges → script writes `pr.merged` on that session and knocks. | | AI vs scripts | The AI inserts local intent. Scripts perform every side effect that leaves the machine. Model text is never a state transition. | | Checks and gates | A **check** records a fact (`agent check record`). A **gate** is a policy verdict over evidence (`agent gate record`). A model claim is neither. Confidence is not proof. | @@ -469,7 +469,7 @@ These are not silent defaults in code; they are human steps after merge: 2. Create a GitHub OAuth App whose callback is `{public-url}/auth/github/callback`. 3. Deploy `agent-core` with every `AGENT_CORE_*` variable set. 4. Add GitHub logins to `teams.yaml` via pull request. -5. On each laptop: PostgreSQL 15+ (`initdb`/`pg_ctl` on `PATH`, or `AGENT_PG_BIN` / `AGENT_PG_DSN`), `pip install -e .`, `agent init` (installs and starts the user-service daemon for knock, usage, pending, github pending, mail pending, `pr.merged`, and the local dashboard; daemon `sync --follow` starts only after pair, once `device.json` has token and hub URL), `agent pair --hub …`. Do not leave a separate `agent knock` or `agent sync --follow` as the always-on path; one-shot `agent sync` remains fine after pairing. +5. On each laptop: PostgreSQL 15+ (`initdb`/`pg_ctl` on `PATH`, or `AGENT_PG_BIN` / `AGENT_PG_DSN`), `pip install -e .`, `agent init` (installs and starts the user-service daemon for knock, usage, pending, github pending, mail pending, `pr.merged`, a per-cycle hub sync once paired, and the local dashboard; daemon `sync --follow` starts only after pair, once `device.json` has token and hub URL), `agent pair --hub …`. Do not leave a separate `agent knock` or `agent sync --follow` as the always-on path; one-shot `agent sync` remains fine after pairing. Later product work (not required to operate v1 after merge): diff --git a/README.md b/README.md index 1a0a017..9e59283 100644 --- a/README.md +++ b/README.md @@ -97,14 +97,14 @@ agent watch assigned [--follow] # allowlisted assignments; needs `gh` and `$AGE agent watch errors # one scan; $AGENT_HOME/error-fix.json; no log host in this package agent watch error-fix # one scan; find-or-create implement task + isolated worktree agent supervise --session ID [--repo OWNER/REPO --number N] [--once|--follow] -# agent knock (daemon, no --once) polls grok-usage, pending, pr.merged, github pending, mail pending, errors, and error-fix every 60s, then pushes any new local activity to the hub when paired +# agent knock (daemon, no --once) polls grok-usage, pending, pr.merged, github pending, mail pending, errors, and error-fix every 60s, then syncs (push + pull) with the hub when paired ``` `agent supervise` posts a short status line to Telegram when both `TELEGRAM_BOT_TOKEN` and `TELEGRAM_CHAT_ID` are set in the environment. The follow CLI does not ask closed questions. Working vs not working for paging is whether the Grok tmux session exists: it posts `not working` only when that session is gone, not when the prompt is idle between turns. The TUI working probe (`Thinking…`, `Waiting for response`, `Preparing …`, `[stop]`, `Esc:cancel`, `command still running`, queued `Enter to send now`) is for the follow loop, not for Telegram. A send failure is printed to stderr and does not stop the loop. Credentials stay out of git. The error-fix executor find-or-creates the implement task and isolated worktree; `agent github pending` still opens draft pull requests. -`agent watch grok-usage` uses the existing Grok login token from the Grok auth file, does not start a Grok session, and does not knock the TUI. Each `usage.snapshot` includes the account email, provider, and subscription tier. Under the device daemon, the knock child records those snapshots (and scans pending, `pr.merged`, github pending, mail pending, errors when `$AGENT_HOME/error-fix.json` exists, and pending `error.fix`) on the same interval, then pushes any new local activity to the hub when the device is paired. `agent daemon --install` / `--uninstall` manage the user service; `agent init` already installs and starts it. +`agent watch grok-usage` uses the existing Grok login token from the Grok auth file, does not start a Grok session, and does not knock the TUI. Each `usage.snapshot` includes the account email, provider, and subscription tier. Under the device daemon, the knock child records those snapshots (and scans pending, `pr.merged`, github pending, mail pending, errors when `$AGENT_HOME/error-fix.json` exists, and pending `error.fix`) on the same interval, then syncs (push + pull) with the hub when the device is paired. `agent daemon --install` / `--uninstall` manage the user service; `agent init` already installs and starts it. `agent watch assigned` reads `$AGENT_HOME/watch.json`: diff --git a/tests/test_knock_scan_cycle.py b/tests/test_knock_scan_cycle.py index 1045629..3b899d7 100644 --- a/tests/test_knock_scan_cycle.py +++ b/tests/test_knock_scan_cycle.py @@ -29,7 +29,7 @@ def _stub_scans(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr("agent_cli.error_fix_act.scan_error_fix", lambda store, run_argv: []) -def test_knock_scan_cycle_pushes_when_paired( +def test_knock_scan_cycle_syncs_when_paired( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: calls: list[int] = [] From b9d289f2bd490a5e5e9aaef439bbe381f90cd302 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 21:30:20 -0300 Subject: [PATCH 06/35] Harden hub-response handling and widen OTel redaction; document the cursor TOCTOU. Hub.request let a raw json.JSONDecodeError leak out on a malformed 2xx body instead of raising HubError like every other hub-communication failure. _sync_once then assumed a successful pull always returns a dict with well-formed events, so a None response or an event missing origin_device_id/origin_seq raised a bare AttributeError/KeyError that the new SystemExit catch in _knock_scan_cycle could not see - the exact crash-the-daemon failure mode that catch was meant to close. Both are now validated the same way the existing 'pull response missing events' check already works: a controlled die() that the caller's except clause catches. Also make the OTel trace_id/span_id redaction case-insensitive and add a traceparent header rule, and add a comment documenting why the mark_pushed/mark_origin cursor guard's residual cross-process race is accepted rather than locked (ledger_event is append-only and the hub treats repeated events as idempotent, so the worst case is a redundant push, not data loss). --- src/agent_cli/errors.py | 6 ++++-- src/agent_cli/hub.py | 6 +++++- src/agent_cli/main.py | 4 ++++ src/agent_cli/store.py | 5 +++++ tests/test_errors.py | 7 +++++++ tests/test_hub.py | 21 +++++++++++++++++++++ tests/test_pending.py | 32 ++++++++++++++++++++++++++++++++ 7 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 tests/test_hub.py diff --git a/src/agent_cli/errors.py b/src/agent_cli/errors.py index 9a734c1..ab8c7a0 100644 --- a/src/agent_cli/errors.py +++ b/src/agent_cli/errors.py @@ -28,8 +28,9 @@ r'(?i)("[^"]*(?:password|secret|token|api[_-]?key|access[_-]?token|client[_-]?secret|authorization|passwd|access_key)[^"]*"\s*:\s*")[^"]*(")' ) _HEX = re.compile(r"\b[a-fA-F0-9]{20,}\b") -_OTEL_TRACE_ID = re.compile(r"\btrace_id=[0-9a-fA-F]{32}\b") -_OTEL_SPAN_ID = re.compile(r"\bspan_id=[0-9a-fA-F]{16}\b") +_OTEL_TRACE_ID = re.compile(r"(?i)\btrace_id=[0-9a-fA-F]{32}\b") +_OTEL_SPAN_ID = re.compile(r"(?i)\bspan_id=[0-9a-fA-F]{16}\b") +_OTEL_TRACEPARENT = re.compile(r"(?i)\btraceparent:\s*[0-9a-fA-F]{2}-[0-9a-fA-F]{32}-[0-9a-fA-F]{16}-[0-9a-fA-F]{2}\b") _SECRET = re.compile( r"(?i)(? str: out = _EMAIL.sub("[redacted]", out) out = _OTEL_TRACE_ID.sub("trace_id=[redacted]", out) out = _OTEL_SPAN_ID.sub("span_id=[redacted]", out) + out = _OTEL_TRACEPARENT.sub("traceparent: [redacted]", out) out = _HEX.sub("[redacted]", out) return out diff --git a/src/agent_cli/hub.py b/src/agent_cli/hub.py index 43b2282..acc1e6e 100644 --- a/src/agent_cli/hub.py +++ b/src/agent_cli/hub.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json from typing import Any from urllib.parse import urljoin @@ -40,7 +41,10 @@ def request(self, method: str, path: str, **kwargs: Any) -> Any: detail = _detail(response) raise HubError(f"hub {method} {path} → HTTP {response.status_code}: {detail}") if response.content: - return response.json() + try: + return response.json() + except (json.JSONDecodeError, UnicodeDecodeError, ValueError) as exc: + raise HubError(f"hub {method} {path} → invalid JSON response") from exc return None def prepare(self, device_id: str, challenge: str, device_name: str) -> dict[str, Any]: diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 96df6c8..395d4ce 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1735,10 +1735,14 @@ def _sync_once(store: Store) -> None: hub.push(pending) store.mark_pushed(pending[-1]["origin_seq"]) pulled = hub.pull(store.all_cursors()) + if not isinstance(pulled, dict): + die("pull response is not an object") events = pulled.get("events") if not isinstance(events, list): die("pull response missing events") for event in events: + if not isinstance(event, dict) or "origin_device_id" not in event or "origin_seq" not in event: + die("pull event missing origin_device_id/origin_seq") store.apply_remote(event) store.mark_origin(event["origin_device_id"], int(event["origin_seq"])) snapshots = ( diff --git a/src/agent_cli/store.py b/src/agent_cli/store.py index e88db7c..060f005 100644 --- a/src/agent_cli/store.py +++ b/src/agent_cli/store.py @@ -522,6 +522,11 @@ def pending_events(self) -> list[dict[str, Any]]: @_wrap_pg_errors def mark_pushed(self, seq: int) -> None: + # Read-then-write is not atomic across the separate cmd_knock/cmd_sync + # --follow processes; a fully concurrent interleaving can still lose an + # update. Accepted: ledger_event is append-only and the hub treats the + # same event id as idempotent, so the worst case is a redundant re-push, + # not data loss. mark_origin below has the identical shape. current = int(self.sync_get("pushed_origin_seq", "0") or "0") if seq > current: self.sync_set("pushed_origin_seq", str(seq)) diff --git a/tests/test_errors.py b/tests/test_errors.py index 7356cef..54c61cb 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -175,6 +175,13 @@ def test_redact_and_fingerprint() -> None: assert span_id not in otel assert "trace_id=[redacted]" in otel assert "span_id=[redacted]" in otel + otel_upper = redact(f"TimeoutError boom TRACE_ID={trace_id.upper()} SPAN_ID={span_id.upper()}") + assert trace_id.upper() not in otel_upper + assert span_id.upper() not in otel_upper + traceparent = redact(f"TimeoutError boom traceparent: 00-{trace_id}-{span_id}-01") + assert trace_id not in traceparent + assert span_id not in traceparent + assert "traceparent: [redacted]" in traceparent def test_scan_inserts_once_then_enriches(tmp_path: Path) -> None: diff --git a/tests/test_hub.py b/tests/test_hub.py new file mode 100644 index 0000000..0dd78c0 --- /dev/null +++ b/tests/test_hub.py @@ -0,0 +1,21 @@ +from __future__ import annotations + +import httpx +import pytest + +from agent_cli.hub import Hub, HubError + + +def test_request_invalid_json_response_raises_hub_error_not_json_decode_error() -> None: + """Regression test: a 2xx response with a non-JSON body used to leak a raw + json.JSONDecodeError out of Hub.request, which callers like _sync_once (via + hub.pull) do not catch, killing whatever loop called it instead of logging a + HubError like every other hub-communication failure.""" + + def handler(_request: httpx.Request) -> httpx.Response: + return httpx.Response(200, text="not-json") + + with httpx.Client(transport=httpx.MockTransport(handler)) as client: + hub = Hub("https://hub.example", "tok", client=client) + with pytest.raises(HubError, match="invalid JSON"): + hub.pull({}) diff --git a/tests/test_pending.py b/tests/test_pending.py index 6d08c30..2d3b985 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -247,6 +247,38 @@ def test_sync_once_applies_subscription_snapshots(tmp_path: Path, monkeypatch: p assert row["_origin_device_id"] == "other-device" +def test_sync_once_dies_on_non_dict_pull_response(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Regression test: Hub.request returns None for a 2xx response with an empty + body. _sync_once used to call pulled.get("events") straight on that, raising a + raw AttributeError instead of a catchable HubError/SystemExit - which would + have escaped _knock_scan_cycle's (HubError, StoreError, SystemExit) guard and + killed the whole knock daemon.""" + store = Store(tmp_path) + store.set_meta("hub_url", "http://hub.example") + store.set_meta("device_token", "tok") + hub = FakeHub() + hub.pull_body = None # type: ignore[assignment] + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(SystemExit, match="pull response is not an object"): + _sync_once(store) + + +def test_sync_once_dies_on_event_missing_origin_fields( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: a pull event missing origin_device_id/origin_seq used to + raise a raw KeyError from event["origin_device_id"], same uncaught-crash risk + as the non-dict pull response above.""" + store = Store(tmp_path) + store.set_meta("hub_url", "http://hub.example") + store.set_meta("device_token", "tok") + hub = FakeHub() + hub.pull_body = {"events": [{"table": "activity", "row_id": "x"}]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(SystemExit, match="origin_device_id/origin_seq"): + _sync_once(store) + + def test_watch_pending_skips_other_executable_types(tmp_path: Path) -> None: store = Store(tmp_path) _owned_session(store) From 0fd5b39334bb252a3381973447879db6d23110ec Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 21:45:14 -0300 Subject: [PATCH 07/35] Fully validate pull-response shape in _sync_once and raise HubError consistently. The prior guard only checked pulled is a dict and each event has origin_device_id/origin_seq before calling apply_remote(). Everything downstream - apply_remote's own indexing of table/op/row_id/payload/ occurred_at, int(origin_seq) on a possibly non-numeric value, and apply_replica_row's indexing of the same fields on snapshot rows - was still unguarded, so a malformed-but-well-typed hub response could still raise a raw KeyError/TypeError/ValueError past every catch clause. Validate the full required field set for both events and snapshot rows up front, and raise HubError instead of calling die() for every malformed-response case in this function: HubError is already caught by both cmd_knock's _knock_scan_cycle and cmd_sync --follow's reconnect loop, so this closes the daemon-crash risk in both callers instead of just the one this PR added, without touching either loop's except clause. --- src/agent_cli/main.py | 43 +++++++++++++++------- tests/test_pending.py | 85 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 97 insertions(+), 31 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 395d4ce..be35951 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1727,7 +1727,15 @@ def lookup_session() -> dict | None: store.close() +_PULL_EVENT_FIELDS = ("origin_device_id", "origin_seq", "table", "op", "row_id", "payload", "occurred_at") +_PULL_ROW_FIELDS = ("table", "origin_device_id", "row_id", "payload", "updated_at") + + def _sync_once(store: Store) -> None: + # Every malformed-hub-response check below raises HubError (not die()'s bare + # SystemExit): both cmd_knock's _knock_scan_cycle and cmd_sync --follow's + # reconnect loop already catch HubError, so a bad response logs/retries in + # whichever loop is calling instead of killing that process. hub = _hub_from_store(store) try: pending = store.pending_events() @@ -1736,25 +1744,32 @@ def _sync_once(store: Store) -> None: store.mark_pushed(pending[-1]["origin_seq"]) pulled = hub.pull(store.all_cursors()) if not isinstance(pulled, dict): - die("pull response is not an object") + raise HubError("pull response is not an object") events = pulled.get("events") if not isinstance(events, list): - die("pull response missing events") + raise HubError("pull response missing events") for event in events: - if not isinstance(event, dict) or "origin_device_id" not in event or "origin_seq" not in event: - die("pull event missing origin_device_id/origin_seq") + if not isinstance(event, dict) or any(field not in event for field in _PULL_EVENT_FIELDS): + raise HubError("pull event is missing required fields") + try: + origin_seq = int(event["origin_seq"]) + except (TypeError, ValueError) as exc: + raise HubError("pull event has a non-numeric origin_seq") from exc store.apply_remote(event) - store.mark_origin(event["origin_device_id"], int(event["origin_seq"])) - snapshots = ( - list(pulled.get("inbox") or []) - + list(pulled.get("pings") or []) - + list(pulled.get("subscriptions") or []) - ) + store.mark_origin(event["origin_device_id"], origin_seq) + snapshots: list[dict[str, Any]] = [] + for key in ("inbox", "pings", "subscriptions"): + value = pulled.get(key) + if value is None: + continue + if not isinstance(value, list): + raise HubError(f"pull response {key} is not a list") + snapshots.extend(value) for row in snapshots: - if not isinstance(row, dict): - die("pull snapshot is not an object") - sessions = [r for r in snapshots if isinstance(r, dict) and r.get("table") == "session"] - rest = [r for r in snapshots if not (isinstance(r, dict) and r.get("table") == "session")] + if not isinstance(row, dict) or any(field not in row for field in _PULL_ROW_FIELDS): + raise HubError("pull snapshot is missing required fields") + sessions = [r for r in snapshots if r.get("table") == "session"] + rest = [r for r in snapshots if r.get("table") != "session"] for row in sessions + rest: store.apply_replica_row(row) print(f"sync pushed={len(pending)} pulled={len(events)} snapshots={len(snapshots)}") diff --git a/tests/test_pending.py b/tests/test_pending.py index 2d3b985..76e5c8c 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -247,36 +247,87 @@ def test_sync_once_applies_subscription_snapshots(tmp_path: Path, monkeypatch: p assert row["_origin_device_id"] == "other-device" -def test_sync_once_dies_on_non_dict_pull_response(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - """Regression test: Hub.request returns None for a 2xx response with an empty - body. _sync_once used to call pulled.get("events") straight on that, raising a - raw AttributeError instead of a catchable HubError/SystemExit - which would - have escaped _knock_scan_cycle's (HubError, StoreError, SystemExit) guard and - killed the whole knock daemon.""" +def _paired_store(tmp_path: Path) -> Store: store = Store(tmp_path) store.set_meta("hub_url", "http://hub.example") store.set_meta("device_token", "tok") + return store + + +def test_sync_once_dies_on_non_dict_pull_response(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Regression test: Hub.request returns None for a 2xx response with an empty + body. _sync_once used to call pulled.get("events") straight on that, raising a + raw AttributeError instead of a catchable HubError - which would have escaped + both _knock_scan_cycle's and cmd_sync --follow's guards and killed whichever + process called it.""" hub = FakeHub() hub.pull_body = None # type: ignore[assignment] monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) - with pytest.raises(SystemExit, match="pull response is not an object"): - _sync_once(store) + with pytest.raises(HubError, match="pull response is not an object"): + _sync_once(_paired_store(tmp_path)) -def test_sync_once_dies_on_event_missing_origin_fields( +def test_sync_once_dies_on_event_missing_required_fields( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """Regression test: a pull event missing origin_device_id/origin_seq used to - raise a raw KeyError from event["origin_device_id"], same uncaught-crash risk - as the non-dict pull response above.""" - store = Store(tmp_path) - store.set_meta("hub_url", "http://hub.example") - store.set_meta("device_token", "tok") + """Regression test: a pull event missing any of the fields + _insert_event_idempotent indexes directly (table, op, row_id, payload, + occurred_at, origin_device_id, origin_seq) used to raise a raw KeyError deep + inside store.apply_remote instead of a catchable HubError raised before that + call - same uncaught-crash risk as the non-dict pull response above.""" hub = FakeHub() hub.pull_body = {"events": [{"table": "activity", "row_id": "x"}]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) - with pytest.raises(SystemExit, match="origin_device_id/origin_seq"): - _sync_once(store) + with pytest.raises(HubError, match="pull event is missing required fields"): + _sync_once(_paired_store(tmp_path)) + + +def test_sync_once_dies_on_non_numeric_origin_seq(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Regression test: int(event["origin_seq"]) used to run unguarded; a + non-numeric origin_seq raised a raw ValueError instead of a catchable + HubError.""" + hub = FakeHub() + hub.pull_body = { + "events": [ + { + "origin_device_id": "other", + "origin_seq": "not-a-number", + "table": "activity", + "op": "insert", + "row_id": "x", + "payload": {}, + "occurred_at": "2026-08-13T12:00:00Z", + } + ] + } + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="non-numeric origin_seq"): + _sync_once(_paired_store(tmp_path)) + + +def test_sync_once_dies_on_non_list_snapshot_field(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Regression test: list(pulled.get("inbox") or []) used to run unguarded; a + truthy non-iterable value (e.g. a malformed hub response sending an object + instead of a list) raised a raw TypeError instead of a catchable HubError.""" + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": {"not": "a list"}} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="inbox is not a list"): + _sync_once(_paired_store(tmp_path)) + + +def test_sync_once_dies_on_snapshot_missing_required_fields( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: apply_replica_row indexes row["origin_device_id"], + row["payload"], row["updated_at"] directly; a snapshot row missing any of + those used to raise a raw KeyError instead of a catchable HubError raised + before that call.""" + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": [{"table": "activity", "row_id": "x"}]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull snapshot is missing required fields"): + _sync_once(_paired_store(tmp_path)) def test_watch_pending_skips_other_executable_types(tmp_path: Path) -> None: From 10b718f1eb2173c60c3d8c5c3597698a9908a5ff Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 21:57:47 -0300 Subject: [PATCH 08/35] Narrow the knock except clause and propagate the coerced origin_seq. _sync_once no longer raises bare SystemExit for any malformed-response check (everything is HubError now), so _knock_scan_cycle's except clause no longer needs to list SystemExit - and the leftover listing left a stale regression test documenting a contract that no longer applies. Narrowed the catch and rewrote the test around HubError. Also: int(event["origin_seq"]) validated the value but never wrote it back into the event dict, so a numeric-string origin_seq from the hub (passes int() cleanly) would still reach _insert_event_idempotent's plain Python != comparison as a string, raising a false-positive 'origin_seq gap' error for a genuinely valid sequence number. The event dict is now rebuilt with the coerced int before apply_remote(). --- src/agent_cli/main.py | 3 ++- tests/test_knock_scan_cycle.py | 16 +++++++------ tests/test_pending.py | 41 +++++++++++++++++++++++++++++----- 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index be35951..c858161 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1755,6 +1755,7 @@ def _sync_once(store: Store) -> None: origin_seq = int(event["origin_seq"]) except (TypeError, ValueError) as exc: raise HubError("pull event has a non-numeric origin_seq") from exc + event = {**event, "origin_seq": origin_seq} store.apply_remote(event) store.mark_origin(event["origin_device_id"], origin_seq) snapshots: list[dict[str, Any]] = [] @@ -2990,7 +2991,7 @@ def _knock_scan_cycle(store: Store, run_argv: Callable[[list[str]], Completed]) if hub_url and hub_token: try: _sync_once(store) - except (HubError, StoreError, SystemExit) as exc: + except (HubError, StoreError) as exc: print(f"sync error: {exc}", file=sys.stderr) diff --git a/tests/test_knock_scan_cycle.py b/tests/test_knock_scan_cycle.py index 3b899d7..ec0a618 100644 --- a/tests/test_knock_scan_cycle.py +++ b/tests/test_knock_scan_cycle.py @@ -6,6 +6,7 @@ import pytest from agent_cli import main as main_mod +from agent_cli.hub import HubError from agent_cli.main import open_store @@ -68,14 +69,15 @@ def test_knock_scan_cycle_skips_sync_when_unpaired( def test_knock_scan_cycle_logs_and_continues_on_malformed_pull_response( tmp_path: Path, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch ) -> None: - """Regression test: _sync_once() calls die() (bare SystemExit, not a HubError/ - StoreError) when the hub returns a malformed pull payload. A narrow except that - only caught (HubError, StoreError) would let that SystemExit propagate and kill - the whole cmd_knock daemon loop instead of logging and moving on like every - other scan in this function.""" + """Regression test: _sync_once() raises HubError when the hub returns a + malformed pull payload. A narrow except that only caught (HubError, StoreError) + still covers this - HubError is exactly what the malformed-response checks + raise - so the daemon logs and moves on instead of dying, without needing to + also catch bare SystemExit (nothing else _sync_once can raise from this call + site is a plain SystemExit).""" def _raise(_store: object) -> None: - raise SystemExit("agent: pull response missing events") + raise HubError("pull response missing events") monkeypatch.setattr(main_mod, "_sync_once", _raise) _stub_scans(monkeypatch) @@ -89,4 +91,4 @@ def _raise(_store: object) -> None: store.close() captured = capsys.readouterr() - assert "sync error: agent: pull response missing events" in captured.err + assert "sync error: pull response missing events" in captured.err diff --git a/tests/test_pending.py b/tests/test_pending.py index 76e5c8c..500dcab 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -254,7 +254,7 @@ def _paired_store(tmp_path: Path) -> Store: return store -def test_sync_once_dies_on_non_dict_pull_response(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: +def test_sync_once_raises_hub_error_on_non_dict_pull_response(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """Regression test: Hub.request returns None for a 2xx response with an empty body. _sync_once used to call pulled.get("events") straight on that, raising a raw AttributeError instead of a catchable HubError - which would have escaped @@ -267,7 +267,7 @@ def test_sync_once_dies_on_non_dict_pull_response(tmp_path: Path, monkeypatch: p _sync_once(_paired_store(tmp_path)) -def test_sync_once_dies_on_event_missing_required_fields( +def test_sync_once_raises_hub_error_on_event_missing_required_fields( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: """Regression test: a pull event missing any of the fields @@ -282,7 +282,7 @@ def test_sync_once_dies_on_event_missing_required_fields( _sync_once(_paired_store(tmp_path)) -def test_sync_once_dies_on_non_numeric_origin_seq(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: +def test_sync_once_raises_hub_error_on_non_numeric_origin_seq(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """Regression test: int(event["origin_seq"]) used to run unguarded; a non-numeric origin_seq raised a raw ValueError instead of a catchable HubError.""" @@ -305,7 +305,38 @@ def test_sync_once_dies_on_non_numeric_origin_seq(tmp_path: Path, monkeypatch: p _sync_once(_paired_store(tmp_path)) -def test_sync_once_dies_on_non_list_snapshot_field(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: +def test_sync_once_accepts_a_numeric_string_origin_seq( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: int(event["origin_seq"]) validated the value but the + original (still-string) event dict was what reached store.apply_remote(). A + numeric string like "1" passes int() cleanly, but _insert_event_idempotent's + `event["origin_seq"] != last_seq + 1` is a plain Python != - "1" != 1 is + always True - so a genuinely valid next sequence number raised a false + "origin_seq gap" StoreError.""" + hub = FakeHub() + hub.pull_body = { + "events": [ + { + "origin_device_id": "other", + "origin_seq": "1", + "table": "task", + "op": "insert", + "row_id": "t1", + "payload": {"id": "t1"}, + "occurred_at": "2026-08-13T12:00:00Z", + } + ] + } + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + store = _paired_store(tmp_path) + _sync_once(store) + assert store.origin_cursor("other") == 1 + row = store.row("task", "t1") + assert row is not None + + +def test_sync_once_raises_hub_error_on_non_list_snapshot_field(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """Regression test: list(pulled.get("inbox") or []) used to run unguarded; a truthy non-iterable value (e.g. a malformed hub response sending an object instead of a list) raised a raw TypeError instead of a catchable HubError.""" @@ -316,7 +347,7 @@ def test_sync_once_dies_on_non_list_snapshot_field(tmp_path: Path, monkeypatch: _sync_once(_paired_store(tmp_path)) -def test_sync_once_dies_on_snapshot_missing_required_fields( +def test_sync_once_raises_hub_error_on_snapshot_missing_required_fields( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: """Regression test: apply_replica_row indexes row["origin_device_id"], From 1fbf40569bbd07af7f3de49d6b2ba5f49f06fa69 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 22:25:06 -0300 Subject: [PATCH 09/35] Close the remaining hub-data-shape gaps structurally instead of one field at a time. int(event["origin_seq"]) didn't catch OverflowError on an out-of-range float (JSON parses 1e309 as inf, and int(inf) overflows) and silently accepted a bool or a fractional float as a valid sequence number. Reject both explicitly and catch OverflowError alongside the existing TypeError/ValueError. Field presence was validated, but not the shape of nested values - a payload whose "type" is unhashable (e.g. a list) makes Store._maybe_wake's frozenset membership check raise a raw TypeError from deep inside apply_remote, a shape no single prior fix anticipated. Rather than keep chasing individual shapes, wrap apply_remote/mark_origin and apply_replica_row in a broad except-Exception-to-HubError safety net: HubError/StoreError are SystemExit subclasses so they pass through unchanged, everything else unexpected now becomes a catchable, logged HubError instead of an uncaught crash. Also parametrized the field-presence regression tests per field (matching the existing tests/test_jobs.py pattern) instead of dropping several fields at once, and added a call-order assertion proving sync runs after every scan, not just alongside them. --- src/agent_cli/main.py | 27 ++++++-- tests/test_knock_scan_cycle.py | 58 +++++++++++++---- tests/test_pending.py | 111 +++++++++++++++++++++++++++------ 3 files changed, 161 insertions(+), 35 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index c858161..3dad569 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1752,12 +1752,26 @@ def _sync_once(store: Store) -> None: if not isinstance(event, dict) or any(field not in event for field in _PULL_EVENT_FIELDS): raise HubError("pull event is missing required fields") try: - origin_seq = int(event["origin_seq"]) - except (TypeError, ValueError) as exc: + raw_seq = event["origin_seq"] + if isinstance(raw_seq, bool): + raise ValueError("origin_seq must not be a boolean") + if isinstance(raw_seq, float) and not raw_seq.is_integer(): + raise ValueError("origin_seq must be a whole number") + origin_seq = int(raw_seq) + except (TypeError, ValueError, OverflowError) as exc: raise HubError("pull event has a non-numeric origin_seq") from exc event = {**event, "origin_seq": origin_seq} - store.apply_remote(event) - store.mark_origin(event["origin_device_id"], origin_seq) + # Field presence is checked above, but not the shape of nested values + # (e.g. payload["type"]) - apply_remote/mark_origin can still hit a + # genuinely unanticipated shape deep inside store.py. Convert any such + # failure to a HubError rather than let it crash whichever loop called + # _sync_once; HubError/StoreError themselves pass through unchanged + # (SystemExit is not an Exception subclass). + try: + store.apply_remote(event) + store.mark_origin(event["origin_device_id"], origin_seq) + except Exception as exc: + raise HubError(f"pull event could not be applied: {exc}") from exc snapshots: list[dict[str, Any]] = [] for key in ("inbox", "pings", "subscriptions"): value = pulled.get(key) @@ -1772,7 +1786,10 @@ def _sync_once(store: Store) -> None: sessions = [r for r in snapshots if r.get("table") == "session"] rest = [r for r in snapshots if r.get("table") != "session"] for row in sessions + rest: - store.apply_replica_row(row) + try: + store.apply_replica_row(row) + except Exception as exc: + raise HubError(f"pull snapshot could not be applied: {exc}") from exc print(f"sync pushed={len(pending)} pulled={len(events)} snapshots={len(snapshots)}") finally: hub.close() diff --git a/tests/test_knock_scan_cycle.py b/tests/test_knock_scan_cycle.py index ec0a618..8e1342f 100644 --- a/tests/test_knock_scan_cycle.py +++ b/tests/test_knock_scan_cycle.py @@ -21,21 +21,48 @@ def _init_paired_store(tmp_path: Path) -> None: store.close() -def _stub_scans(monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setattr(main_mod, "scan_usage", lambda store: None) - monkeypatch.setattr(main_mod, "scan_merged", lambda store, run_argv: ([], 0)) - monkeypatch.setattr("agent_cli.pending.scan_pending", lambda store, hub: []) - monkeypatch.setattr("agent_cli.github_act.scan_github", lambda store, run_argv: []) - monkeypatch.setattr("agent_cli.mail_act.scan_mail", lambda store, run_argv: []) - monkeypatch.setattr("agent_cli.error_fix_act.scan_error_fix", lambda store, run_argv: []) +def _stub_scans(monkeypatch: pytest.MonkeyPatch, order: list[str] | None = None) -> None: + log = order if order is not None else [] + + def scan_usage(store: object) -> None: + log.append("scan_usage") + + def scan_merged(store: object, run_argv: object) -> tuple[list[str], int]: + log.append("scan_merged") + return ([], 0) + + def scan_pending(store: object, hub: object) -> list[str]: + log.append("scan_pending") + return [] + + def scan_github(store: object, run_argv: object) -> list[str]: + log.append("scan_github") + return [] + + def scan_mail(store: object, run_argv: object) -> list[str]: + log.append("scan_mail") + return [] + + def scan_error_fix(store: object, run_argv: object) -> list[str]: + log.append("scan_error_fix") + return [] + + monkeypatch.setattr(main_mod, "scan_usage", scan_usage) + monkeypatch.setattr(main_mod, "scan_merged", scan_merged) + monkeypatch.setattr("agent_cli.pending.scan_pending", scan_pending) + monkeypatch.setattr("agent_cli.github_act.scan_github", scan_github) + monkeypatch.setattr("agent_cli.mail_act.scan_mail", scan_mail) + monkeypatch.setattr("agent_cli.error_fix_act.scan_error_fix", scan_error_fix) def test_knock_scan_cycle_syncs_when_paired( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - calls: list[int] = [] - monkeypatch.setattr(main_mod, "_sync_once", lambda store: calls.append(1)) - _stub_scans(monkeypatch) + """Sync must run once, and after every scan - not just alongside them - since + it is meant to push whatever those scans just created.""" + order: list[str] = [] + monkeypatch.setattr(main_mod, "_sync_once", lambda store: order.append("sync")) + _stub_scans(monkeypatch, order) _init_paired_store(tmp_path) store = open_store() @@ -44,7 +71,16 @@ def test_knock_scan_cycle_syncs_when_paired( finally: store.close() - assert calls == [1] + assert order[-1] == "sync" + assert order.count("sync") == 1 + assert set(order[:-1]) == { + "scan_usage", + "scan_merged", + "scan_pending", + "scan_github", + "scan_mail", + "scan_error_fix", + } def test_knock_scan_cycle_skips_sync_when_unpaired( diff --git a/tests/test_pending.py b/tests/test_pending.py index 500dcab..1eb65f2 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -267,31 +267,60 @@ def test_sync_once_raises_hub_error_on_non_dict_pull_response(tmp_path: Path, mo _sync_once(_paired_store(tmp_path)) -def test_sync_once_raises_hub_error_on_event_missing_required_fields( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch +def _valid_pull_event() -> dict[str, Any]: + return { + "origin_device_id": "other", + "origin_seq": 1, + "table": "activity", + "op": "insert", + "row_id": "x", + "payload": {}, + "occurred_at": "2026-08-13T12:00:00Z", + } + + +@pytest.mark.parametrize( + "field", + ["origin_device_id", "origin_seq", "table", "op", "row_id", "payload", "occurred_at"], +) +def test_sync_once_raises_hub_error_on_event_missing_one_required_field( + field: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """Regression test: a pull event missing any of the fields - _insert_event_idempotent indexes directly (table, op, row_id, payload, - occurred_at, origin_device_id, origin_seq) used to raise a raw KeyError deep + """Regression test: a pull event missing any single one of the fields + _insert_event_idempotent indexes directly used to raise a raw KeyError deep inside store.apply_remote instead of a catchable HubError raised before that - call - same uncaught-crash risk as the non-dict pull response above.""" + call. Parametrized per field so a future accidental narrowing of + _PULL_EVENT_FIELDS to any one of them is still caught.""" + event = _valid_pull_event() + del event[field] hub = FakeHub() - hub.pull_body = {"events": [{"table": "activity", "row_id": "x"}]} + hub.pull_body = {"events": [event]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) with pytest.raises(HubError, match="pull event is missing required fields"): _sync_once(_paired_store(tmp_path)) -def test_sync_once_raises_hub_error_on_non_numeric_origin_seq(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - """Regression test: int(event["origin_seq"]) used to run unguarded; a - non-numeric origin_seq raised a raw ValueError instead of a catchable - HubError.""" +@pytest.mark.parametrize( + "bad_seq", + [ + "not-a-number", + 1e309, # float('inf') once parsed - int() raises OverflowError, not ValueError + True, # bool is an int subclass in Python - int(True) == 1 would pass silently + 1.5, # non-integral float - int(1.5) == 1 would silently truncate + ], +) +def test_sync_once_raises_hub_error_on_non_numeric_origin_seq( + bad_seq: object, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: int(event["origin_seq"]) used to run unguarded (raising a + raw ValueError on garbage, OverflowError on an out-of-range float) and also + silently accepted a bool or a fractional float as a valid sequence number.""" hub = FakeHub() hub.pull_body = { "events": [ { "origin_device_id": "other", - "origin_seq": "not-a-number", + "origin_seq": bad_seq, "table": "activity", "op": "insert", "row_id": "x", @@ -305,6 +334,35 @@ def test_sync_once_raises_hub_error_on_non_numeric_origin_seq(tmp_path: Path, mo _sync_once(_paired_store(tmp_path)) +def test_sync_once_raises_hub_error_when_apply_remote_hits_an_unanticipated_shape( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: field presence is validated, but not the shape of nested + values. A payload whose "type" is an unhashable value (e.g. a list) makes + Store._maybe_wake's `payload.get("type") not in WAKE_ACTIVITY_TYPES` raise a + raw TypeError from deep inside store.apply_remote, well past the field + presence check - a shape neither _PULL_EVENT_FIELDS nor any single previous + fix anticipated. The broad safety net around apply_remote/mark_origin must + catch this too, not just the specific shapes already known about.""" + hub = FakeHub() + hub.pull_body = { + "events": [ + { + "origin_device_id": "other", + "origin_seq": 1, + "table": "activity", + "op": "insert", + "row_id": "x", + "payload": {"type": ["not", "hashable"]}, + "occurred_at": "2026-08-13T12:00:00Z", + } + ] + } + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull event could not be applied"): + _sync_once(_paired_store(tmp_path)) + + def test_sync_once_accepts_a_numeric_string_origin_seq( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -347,15 +405,30 @@ def test_sync_once_raises_hub_error_on_non_list_snapshot_field(tmp_path: Path, m _sync_once(_paired_store(tmp_path)) -def test_sync_once_raises_hub_error_on_snapshot_missing_required_fields( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch +def _valid_pull_row() -> dict[str, Any]: + return { + "table": "activity", + "origin_device_id": "other", + "row_id": "x", + "payload": {}, + "updated_at": "2026-08-13T12:00:00Z", + } + + +@pytest.mark.parametrize("field", ["table", "origin_device_id", "row_id", "payload", "updated_at"]) +def test_sync_once_raises_hub_error_on_snapshot_missing_one_required_field( + field: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """Regression test: apply_replica_row indexes row["origin_device_id"], - row["payload"], row["updated_at"] directly; a snapshot row missing any of - those used to raise a raw KeyError instead of a catchable HubError raised - before that call.""" + """Regression test: apply_replica_row indexes row["table"], + row["origin_device_id"], row["row_id"], row["payload"], row["updated_at"] + directly; a snapshot row missing any single one of those used to raise a raw + KeyError instead of a catchable HubError raised before that call. + Parametrized per field so a future accidental narrowing of _PULL_ROW_FIELDS + to any one of them is still caught.""" + row = _valid_pull_row() + del row[field] hub = FakeHub() - hub.pull_body = {"events": [], "inbox": [{"table": "activity", "row_id": "x"}]} + hub.pull_body = {"events": [], "inbox": [row]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) with pytest.raises(HubError, match="pull snapshot is missing required fields"): _sync_once(_paired_store(tmp_path)) From 1d5d655a9ef20aa31997b36cd7e0082ad7f64408 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 22:46:02 -0300 Subject: [PATCH 10/35] Fix the wake-type frozenset check at its root instead of only catching the crash. The broad exception safety net converted a malformed payload.type into a HubError every cycle, but since the event never actually persists (the exception rolls back apply_remote's transaction before mark_origin runs), the hub keeps re-serving the same event forever - an infinite, silently-logged retry wedge that looks like routine transient-error handling. _maybe_wake/_maybe_work and the wake=False branches in apply_remote/apply_replica_row now guard the frozenset membership check with isinstance(typ, str) first, so a malformed type is simply treated as not-a-wake-type and the event still applies normally. The safety net stays in place for shapes that genuinely can't be fixed at their root. Also added the missing test for the snapshot-row exception safety net (only the event-row one was covered). --- src/agent_cli/store.py | 19 +++++----- tests/test_pending.py | 83 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 85 insertions(+), 17 deletions(-) diff --git a/src/agent_cli/store.py b/src/agent_cli/store.py index 060f005..c3c66e0 100644 --- a/src/agent_cli/store.py +++ b/src/agent_cli/store.py @@ -355,7 +355,8 @@ def apply_remote(self, event: dict[str, Any], *, wake: bool = True) -> None: self._maybe_wake(event) elif inserted: payload = event.get("payload") - if isinstance(payload, dict) and payload.get("type") in WAKE_ACTIVITY_TYPES: + typ = payload.get("type") if isinstance(payload, dict) else None + if isinstance(typ, str) and typ in WAKE_ACTIVITY_TYPES: target = self._inbox_target(payload) if target is not None and self._owns_session(target): self.enqueue_wake(event["row_id"], target) @@ -494,7 +495,8 @@ def apply_replica_row(self, row: dict[str, Any], *, wake: bool = True) -> None: self._maybe_wake(event) else: payload = event.get("payload") - if isinstance(payload, dict) and payload.get("type") in WAKE_ACTIVITY_TYPES: + typ = payload.get("type") if isinstance(payload, dict) else None + if isinstance(typ, str) and typ in WAKE_ACTIVITY_TYPES: target = self._inbox_target(payload) if target is not None and self._owns_session(target): self.enqueue_wake(event["row_id"], target) @@ -714,14 +716,12 @@ def _maybe_wake(self, event: dict[str, Any]) -> None: payload = event.get("payload") if not isinstance(payload, dict): return - if payload.get("type") not in WAKE_ACTIVITY_TYPES: + typ = payload.get("type") + if not isinstance(typ, str) or typ not in WAKE_ACTIVITY_TYPES: return - if event.get("op") == "update" and payload.get("type") == "error.seen": + if event.get("op") == "update" and typ == "error.seen": return - if ( - payload.get("type") in DONE_WAKE_ACTIVITY_TYPES - and payload.get("execution_status") != "done" - ): + if typ in DONE_WAKE_ACTIVITY_TYPES and payload.get("execution_status") != "done": return target = self._inbox_target(payload) if target is None: @@ -740,7 +740,8 @@ def _maybe_work(self, event: dict[str, Any]) -> None: return if payload.get("execution_status") != "pending": return - if payload.get("type") not in EXECUTABLE_ACTIVITY_TYPES: + typ = payload.get("type") + if not isinstance(typ, str) or typ not in EXECUTABLE_ACTIVITY_TYPES: return sid = payload.get("session_id") if not isinstance(sid, str) or sid == "": diff --git a/tests/test_pending.py b/tests/test_pending.py index 1eb65f2..a204350 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -334,16 +334,18 @@ def test_sync_once_raises_hub_error_on_non_numeric_origin_seq( _sync_once(_paired_store(tmp_path)) -def test_sync_once_raises_hub_error_when_apply_remote_hits_an_unanticipated_shape( +def test_sync_once_accepts_an_activity_payload_whose_type_is_not_a_wake_type_shape( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """Regression test: field presence is validated, but not the shape of nested - values. A payload whose "type" is an unhashable value (e.g. a list) makes - Store._maybe_wake's `payload.get("type") not in WAKE_ACTIVITY_TYPES` raise a - raw TypeError from deep inside store.apply_remote, well past the field - presence check - a shape neither _PULL_EVENT_FIELDS nor any single previous - fix anticipated. The broad safety net around apply_remote/mark_origin must - catch this too, not just the specific shapes already known about.""" + """Regression test: a payload whose "type" is an unhashable value (e.g. a + list) used to make Store._maybe_wake's `payload.get("type") not in + WAKE_ACTIVITY_TYPES` raise a raw TypeError from deep inside + store.apply_remote - a shape neither _PULL_EVENT_FIELDS nor the broad + except-Exception safety net's introduction actually fixed, just quietly + converted into a permanent HubError retry loop (the cursor never advances, + so the hub keeps re-serving the same event forever). _maybe_wake now treats + a non-string type as simply "not a wake type" and lets the event apply + normally instead of failing the whole transaction.""" hub = FakeHub() hub.pull_body = { "events": [ @@ -359,6 +361,39 @@ def test_sync_once_raises_hub_error_when_apply_remote_hits_an_unanticipated_shap ] } monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + store = _paired_store(tmp_path) + _sync_once(store) + assert store.origin_cursor("other") == 1 + row = store.row("activity", "x") + assert row is not None + + +def test_sync_once_raises_hub_error_when_apply_remote_hits_an_unanticipated_shape( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test for the general safety net itself: field presence is + validated, but not every possible shape of every nested value can be + anticipated and fixed at its root (unlike the non-string-type case above). + A payload containing a value json.dumps cannot serialize (impossible from a + real JSON hub response, but a stand-in for "something genuinely + unanticipated") still reaches the broad except-Exception net around + apply_remote/mark_origin and becomes a catchable HubError instead of an + uncaught crash.""" + hub = FakeHub() + hub.pull_body = { + "events": [ + { + "origin_device_id": "other", + "origin_seq": 1, + "table": "activity", + "op": "insert", + "row_id": "x", + "payload": {"type": "message", "body": {1, 2, 3}}, + "occurred_at": "2026-08-13T12:00:00Z", + } + ] + } + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) with pytest.raises(HubError, match="pull event could not be applied"): _sync_once(_paired_store(tmp_path)) @@ -434,6 +469,38 @@ def test_sync_once_raises_hub_error_on_snapshot_missing_one_required_field( _sync_once(_paired_store(tmp_path)) +def test_sync_once_accepts_a_snapshot_payload_whose_type_is_not_a_wake_type_shape( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the row-side sibling of + test_sync_once_accepts_an_activity_payload_whose_type_is_not_a_wake_type_shape. + apply_replica_row also calls _maybe_wake, so the same non-string-type fix + must let the row apply normally instead of raising.""" + row = {**_valid_pull_row(), "payload": {"type": ["not", "hashable"]}} + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": [row]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + store = _paired_store(tmp_path) + _sync_once(store) + stored = store.row("activity", "x") + assert stored is not None + + +def test_sync_once_raises_hub_error_when_apply_replica_row_hits_an_unanticipated_shape( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the row-side sibling of + test_sync_once_raises_hub_error_when_apply_remote_hits_an_unanticipated_shape, + proving the broad safety net around store.apply_replica_row catches a + genuinely unanticipated shape too, not just the event-side one.""" + row = {**_valid_pull_row(), "payload": {"type": "message", "body": {1, 2, 3}}} + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": [row]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull snapshot could not be applied"): + _sync_once(_paired_store(tmp_path)) + + def test_watch_pending_skips_other_executable_types(tmp_path: Path) -> None: store = Store(tmp_path) _owned_session(store) From fa6e9b3d9ac7b941a1aca4754e5782cd5b0b80b1 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 23:00:22 -0300 Subject: [PATCH 11/35] Close the last unguarded frozenset check: pending_work() via the restore path. cmd_restore applies hub-returned own_events through apply_remote(wake=False), which materializes them into row_data under this device's own origin_device_id - so own-origin rows are not exclusively written by trusted local code. pending_work()'s EXECUTABLE_ACTIVITY_TYPES membership check had the same unguarded pattern already fixed elsewhere in _maybe_wake/_maybe_work: a malformed type from a corrupted restore response would crash the whole knock daemon on its next scan cycle. Same isinstance(typ, str) guard, same fix. --- src/agent_cli/store.py | 3 ++- tests/test_store.py | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/agent_cli/store.py b/src/agent_cli/store.py index c3c66e0..e63c08d 100644 --- a/src/agent_cli/store.py +++ b/src/agent_cli/store.py @@ -654,7 +654,8 @@ def pending_work(self) -> list[dict[str, Any]]: continue if payload.get("execution_status") != "pending": continue - if payload.get("type") not in EXECUTABLE_ACTIVITY_TYPES: + typ = payload.get("type") + if not isinstance(typ, str) or typ not in EXECUTABLE_ACTIVITY_TYPES: continue payload["_origin_device_id"] = row["origin_device_id"] out.append(payload) diff --git a/tests/test_store.py b/tests/test_store.py index 376767b..74c3cf2 100644 --- a/tests/test_store.py +++ b/tests/test_store.py @@ -52,6 +52,29 @@ def test_mark_pushed_does_not_regress_the_cursor(tmp_path: Path) -> None: assert store.sync_get("pushed_origin_seq", "0") == "10" +def test_pending_work_skips_a_non_string_type_instead_of_raising(tmp_path: Path) -> None: + """Regression test: pending_work's `payload.get("type") not in + EXECUTABLE_ACTIVITY_TYPES` used to run unguarded, same defect as the + hub-pull path this PR otherwise hardened. Own-origin rows aren't only + ever written by this device's own trusted code - cmd_restore applies + hub-returned "own_events" via apply_remote(wake=False), so a malformed + type can still reach here via a corrupted restore response. A non-string + type must be treated as simply not executable, not raise TypeError.""" + store = Store(tmp_path) + store.write( + "activity", + "insert", + "a1", + { + "id": "a1", + "session_id": "s1", + "type": ["not", "hashable"], + "execution_status": "pending", + }, + ) + assert store.pending_work() == [] + + def test_remote_gap_fail_closed(tmp_path: Path) -> None: store = Store(tmp_path) with pytest.raises(StoreError, match="gap"): From 3af887f2abeacc49ffcdc220630a9b488dd754fe Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 23:15:18 -0300 Subject: [PATCH 12/35] Give cmd_restore the same hub-response hardening as _sync_once. cmd_restore consumes the identical hub-data shape as _sync_once (events with origin_device_id/origin_seq/table/op/row_id/payload/ occurred_at, snapshot rows for inbox/pings) but received none of this PR's hardening: no dict-shape check on the restore body, no per-event field validation or origin_seq coercion before apply_remote (a numeric-string origin_seq deterministically raised a false-positive "origin_seq gap" error for the very first restored event, the exact defect class _sync_once was hardened against), no per-row field validation before apply_replica_row. Extracted the shared validation into _coerce_pull_event/_check_pull_row (raising an internal _PullShapeError), used by both _sync_once (converts to HubError) and cmd_restore (converts to die(), the appropriate convention for a one-shot CLI command) instead of duplicating the checks. cmd_restore had no test coverage at all before this; new tests/test_restore.py covers the hardening plus one happy-path test. --- src/agent_cli/main.py | 104 +++++++++++++++++++++++-------- tests/test_restore.py | 138 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 216 insertions(+), 26 deletions(-) create mode 100644 tests/test_restore.py diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 3dad569..38d99b3 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1491,6 +1491,8 @@ def cmd_restore(_: list[str]) -> None: body = hub.restore() finally: hub.close() + if not isinstance(body, dict): + die("restore response is not an object") if body.get("device_id") != store.device_id(): die("restore device_id does not match this device") if "own_events" in body: @@ -1500,13 +1502,32 @@ def cmd_restore(_: list[str]) -> None: if not isinstance(events, list): die("restore response missing own_events") for event in events: - store.apply_remote(event, wake=False) - store.mark_origin(event["origin_device_id"], int(event["origin_seq"])) - snapshots = list(body.get("inbox") or []) + list(body.get("pings") or []) + try: + event = _coerce_pull_event(event) + except _PullShapeError as exc: + die(f"restore {exc}") + try: + store.apply_remote(event, wake=False) + store.mark_origin(event["origin_device_id"], event["origin_seq"]) + except Exception as exc: + die(f"restore event could not be applied: {exc}") + snapshots: list[dict[str, Any]] = [] + for key in ("inbox", "pings"): + value = body.get(key) + if value is None: + continue + if not isinstance(value, list): + die(f"restore response {key} is not a list") + snapshots.extend(value) for row in snapshots: - if not isinstance(row, dict): - die("restore snapshot is not an object") - store.apply_replica_row(row, wake=False) + try: + _check_pull_row(row) + except _PullShapeError as exc: + die(f"restore {exc}") + try: + store.apply_replica_row(row, wake=False) + except Exception as exc: + die(f"restore snapshot could not be applied: {exc}") print(f"restored events={len(events)} snapshots={len(snapshots)}") finally: store.close() @@ -1731,6 +1752,42 @@ def lookup_session() -> dict | None: _PULL_ROW_FIELDS = ("table", "origin_device_id", "row_id", "payload", "updated_at") +class _PullShapeError(ValueError): + """A hub-supplied event/row/response doesn't have the shape callers need. + Internal only: every caller catches this and converts it to whatever error + convention fits that call site (HubError for _sync_once, die() for the + one-shot cmd_restore CLI) - it must never itself propagate out of this + module.""" + + +def _coerce_pull_event(event: object) -> dict[str, Any]: + """Validate a pulled/restored event has every field _insert_event_idempotent + indexes, and normalize origin_seq to an int (rejecting bool, a fractional + float, and anything int() can't convert, including an out-of-range float + that would otherwise raise OverflowError). Returns a new dict; the caller's + own copy of the raw event is left untouched.""" + if not isinstance(event, dict) or any(field not in event for field in _PULL_EVENT_FIELDS): + raise _PullShapeError("event is missing required fields") + raw_seq = event["origin_seq"] + try: + if isinstance(raw_seq, bool): + raise ValueError("origin_seq must not be a boolean") + if isinstance(raw_seq, float) and not raw_seq.is_integer(): + raise ValueError("origin_seq must be a whole number") + origin_seq = int(raw_seq) + except (TypeError, ValueError, OverflowError) as exc: + raise _PullShapeError("event has a non-numeric origin_seq") from exc + return {**event, "origin_seq": origin_seq} + + +def _check_pull_row(row: object) -> dict[str, Any]: + """Validate a pulled/restored snapshot row has every field + apply_replica_row indexes directly.""" + if not isinstance(row, dict) or any(field not in row for field in _PULL_ROW_FIELDS): + raise _PullShapeError("snapshot is missing required fields") + return row + + def _sync_once(store: Store) -> None: # Every malformed-hub-response check below raises HubError (not die()'s bare # SystemExit): both cmd_knock's _knock_scan_cycle and cmd_sync --follow's @@ -1749,27 +1806,20 @@ def _sync_once(store: Store) -> None: if not isinstance(events, list): raise HubError("pull response missing events") for event in events: - if not isinstance(event, dict) or any(field not in event for field in _PULL_EVENT_FIELDS): - raise HubError("pull event is missing required fields") try: - raw_seq = event["origin_seq"] - if isinstance(raw_seq, bool): - raise ValueError("origin_seq must not be a boolean") - if isinstance(raw_seq, float) and not raw_seq.is_integer(): - raise ValueError("origin_seq must be a whole number") - origin_seq = int(raw_seq) - except (TypeError, ValueError, OverflowError) as exc: - raise HubError("pull event has a non-numeric origin_seq") from exc - event = {**event, "origin_seq": origin_seq} - # Field presence is checked above, but not the shape of nested values - # (e.g. payload["type"]) - apply_remote/mark_origin can still hit a - # genuinely unanticipated shape deep inside store.py. Convert any such - # failure to a HubError rather than let it crash whichever loop called - # _sync_once; HubError/StoreError themselves pass through unchanged - # (SystemExit is not an Exception subclass). + event = _coerce_pull_event(event) + except _PullShapeError as exc: + raise HubError(f"pull {exc}") from exc + # Field presence and origin_seq are validated above, but not the + # shape of nested values (e.g. payload["type"]) - apply_remote/ + # mark_origin can still hit a genuinely unanticipated shape deep + # inside store.py. Convert any such failure to a HubError rather + # than let it crash whichever loop called _sync_once; HubError/ + # StoreError themselves pass through unchanged (SystemExit is not + # an Exception subclass). try: store.apply_remote(event) - store.mark_origin(event["origin_device_id"], origin_seq) + store.mark_origin(event["origin_device_id"], event["origin_seq"]) except Exception as exc: raise HubError(f"pull event could not be applied: {exc}") from exc snapshots: list[dict[str, Any]] = [] @@ -1781,8 +1831,10 @@ def _sync_once(store: Store) -> None: raise HubError(f"pull response {key} is not a list") snapshots.extend(value) for row in snapshots: - if not isinstance(row, dict) or any(field not in row for field in _PULL_ROW_FIELDS): - raise HubError("pull snapshot is missing required fields") + try: + _check_pull_row(row) + except _PullShapeError as exc: + raise HubError(f"pull {exc}") from exc sessions = [r for r in snapshots if r.get("table") == "session"] rest = [r for r in snapshots if r.get("table") != "session"] for row in sessions + rest: diff --git a/tests/test_restore.py b/tests/test_restore.py new file mode 100644 index 0000000..aade1d4 --- /dev/null +++ b/tests/test_restore.py @@ -0,0 +1,138 @@ +from __future__ import annotations + +import os +from pathlib import Path +from typing import Any + +import pytest + +from agent_cli import main as main_mod +from agent_cli.main import open_store + + +class _FakeRestoreHub: + def __init__(self, body: Any) -> None: + self.body = body + + def restore(self) -> Any: + return self.body + + def close(self) -> None: + return None + + +def _init_store(tmp_path: Path) -> None: + os.environ["AGENT_HOME"] = str(tmp_path) + main_mod.main(["init"]) + + +def _run_restore(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, body: Any) -> None: + _init_store(tmp_path) + store = open_store() + device_id = store.device_id() + store.close() + if isinstance(body, dict) and "device_id" not in body: + body = {**body, "device_id": device_id} + monkeypatch.setattr(main_mod, "_hub_from_store", lambda _s: _FakeRestoreHub(body)) + main_mod.cmd_restore([]) + + +def test_cmd_restore_dies_on_non_dict_body(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Regression test: Hub.request (and so Hub.restore) returns None for a 2xx + response with an empty body - cmd_restore used to call body.get("device_id") + straight on that, raising a raw AttributeError instead of a clean die().""" + with pytest.raises(SystemExit, match="restore response is not an object"): + _run_restore(tmp_path, monkeypatch, None) + + +def test_cmd_restore_dies_on_event_missing_required_fields( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: a restored event missing any field + _insert_event_idempotent indexes used to raise a raw KeyError deep inside + store.apply_remote instead of a clean die().""" + body = {"own_events": [{"table": "activity", "row_id": "x"}]} + with pytest.raises(SystemExit, match="restore event is missing required fields"): + _run_restore(tmp_path, monkeypatch, body) + + +def test_cmd_restore_dies_on_snapshot_missing_required_fields( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: a restored snapshot row missing any field + apply_replica_row indexes used to raise a raw KeyError instead of a clean + die().""" + body = {"own_events": [], "inbox": [{"table": "activity", "row_id": "x"}]} + with pytest.raises(SystemExit, match="restore snapshot is missing required fields"): + _run_restore(tmp_path, monkeypatch, body) + + +def test_cmd_restore_accepts_a_numeric_string_origin_seq( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: cmd_restore passed event["origin_seq"] to + apply_remote uncoerced, only int()-converting it afterward for + mark_origin. A numeric-string origin_seq (e.g. "1") then hit + _insert_event_idempotent's plain Python `event["origin_seq"] != last_seq + + 1` as a string, deterministically raising a false "origin_seq gap" for the + very first restored event. cmd_restore now shares _sync_once's coercion, + applied before apply_remote sees the event.""" + body = { + "own_events": [ + { + "origin_device_id": "other", + "origin_seq": "1", + "table": "task", + "op": "insert", + "row_id": "t1", + "payload": {"id": "t1"}, + "occurred_at": "2026-08-13T12:00:00Z", + } + ] + } + _run_restore(tmp_path, monkeypatch, body) + store = open_store() + try: + assert store.origin_cursor("other") == 1 + assert store.row("task", "t1") is not None + finally: + store.close() + + +def test_cmd_restore_applies_events_and_snapshots( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """Happy-path coverage: cmd_restore had none before this change. Replays + one own event and one inbox snapshot row.""" + body = { + "own_events": [ + { + "origin_device_id": "other", + "origin_seq": 1, + "table": "task", + "op": "insert", + "row_id": "t1", + "payload": {"id": "t1"}, + "occurred_at": "2026-08-13T12:00:00Z", + } + ], + "inbox": [ + { + "table": "activity", + "origin_device_id": "other", + "row_id": "a1", + "payload": {"id": "a1", "type": "message"}, + "updated_at": "2026-08-13T12:00:00Z", + } + ], + } + capsys.readouterr() + _run_restore(tmp_path, monkeypatch, body) + captured = capsys.readouterr() + assert "restored events=1 snapshots=1" in captured.out + store = open_store() + try: + assert store.row("task", "t1") is not None + assert store.row("activity", "a1") is not None + finally: + store.close() From 24d94ba6be5b293c3b8ef93c3320f7c37c645136 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 23:33:25 -0300 Subject: [PATCH 13/35] Parametrize test_restore.py's field-presence tests and fix a docstring overclaim. Mirror the per-field @pytest.mark.parametrize pattern already used for the equivalent _sync_once tests, instead of dropping several fields at once - a future accidental narrowing of _PULL_EVENT_FIELDS/ _PULL_ROW_FIELDS to any single one of them is now caught here too. Also corrected a regression test's docstring: _sync_once's internal _hub_from_store() call can in principle still raise a bare SystemExit if pairing were revoked between _knock_scan_cycle's cached check and the _sync_once call, but no code path anywhere in this repo clears hub_url/device_token once set - confirmed write-once, only ever set by cmd_pair. Not reachable today; noted as a future concern instead of claimed impossible. --- tests/test_knock_scan_cycle.py | 9 ++++-- tests/test_restore.py | 57 +++++++++++++++++++++++++++------- 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/tests/test_knock_scan_cycle.py b/tests/test_knock_scan_cycle.py index 8e1342f..488e0a2 100644 --- a/tests/test_knock_scan_cycle.py +++ b/tests/test_knock_scan_cycle.py @@ -108,9 +108,12 @@ def test_knock_scan_cycle_logs_and_continues_on_malformed_pull_response( """Regression test: _sync_once() raises HubError when the hub returns a malformed pull payload. A narrow except that only caught (HubError, StoreError) still covers this - HubError is exactly what the malformed-response checks - raise - so the daemon logs and moves on instead of dying, without needing to - also catch bare SystemExit (nothing else _sync_once can raise from this call - site is a plain SystemExit).""" + raise - so the daemon logs and moves on instead of dying. _sync_once's + _hub_from_store() call can still raise a bare SystemExit ("device is not + paired") if pairing is ever revoked concurrently with this call, but no + code path in this repo clears hub_url/device_token once set (verified: + they're write-once, only ever set by cmd_pair) - not reachable today, only + a future concern if an unpair command is ever added.""" def _raise(_store: object) -> None: raise HubError("pull response missing events") diff --git a/tests/test_restore.py b/tests/test_restore.py index aade1d4..0f0359b 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -45,24 +45,59 @@ def test_cmd_restore_dies_on_non_dict_body(tmp_path: Path, monkeypatch: pytest.M _run_restore(tmp_path, monkeypatch, None) -def test_cmd_restore_dies_on_event_missing_required_fields( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch +def _valid_restore_event() -> dict[str, Any]: + return { + "origin_device_id": "other", + "origin_seq": 1, + "table": "activity", + "op": "insert", + "row_id": "x", + "payload": {}, + "occurred_at": "2026-08-13T12:00:00Z", + } + + +@pytest.mark.parametrize( + "field", + ["origin_device_id", "origin_seq", "table", "op", "row_id", "payload", "occurred_at"], +) +def test_cmd_restore_dies_on_event_missing_one_required_field( + field: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """Regression test: a restored event missing any field + """Regression test: a restored event missing any single one of the fields _insert_event_idempotent indexes used to raise a raw KeyError deep inside - store.apply_remote instead of a clean die().""" - body = {"own_events": [{"table": "activity", "row_id": "x"}]} + store.apply_remote instead of a clean die(). Parametrized per field (like + the equivalent tests/test_pending.py _sync_once tests) so a future + accidental narrowing of _PULL_EVENT_FIELDS to any one of them is still + caught, not just the "several fields missing at once" case.""" + event = _valid_restore_event() + del event[field] + body = {"own_events": [event]} with pytest.raises(SystemExit, match="restore event is missing required fields"): _run_restore(tmp_path, monkeypatch, body) -def test_cmd_restore_dies_on_snapshot_missing_required_fields( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch +def _valid_restore_row() -> dict[str, Any]: + return { + "table": "activity", + "origin_device_id": "other", + "row_id": "x", + "payload": {}, + "updated_at": "2026-08-13T12:00:00Z", + } + + +@pytest.mark.parametrize("field", ["table", "origin_device_id", "row_id", "payload", "updated_at"]) +def test_cmd_restore_dies_on_snapshot_missing_one_required_field( + field: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """Regression test: a restored snapshot row missing any field - apply_replica_row indexes used to raise a raw KeyError instead of a clean - die().""" - body = {"own_events": [], "inbox": [{"table": "activity", "row_id": "x"}]} + """Regression test: a restored snapshot row missing any single one of the + fields apply_replica_row indexes used to raise a raw KeyError instead of a + clean die(). Parametrized per field, same reasoning as the event test + above.""" + row = _valid_restore_row() + del row[field] + body = {"own_events": [], "inbox": [row]} with pytest.raises(SystemExit, match="restore snapshot is missing required fields"): _run_restore(tmp_path, monkeypatch, body) From 8f5a5bba443c24bc24d62b62d70dac46c0d54039 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Tue, 1 Sep 2026 23:48:31 -0300 Subject: [PATCH 14/35] Cover cmd_restore's non-list-field and unanticipated-shape safety-net paths. Mirror the _sync_once tests that already exercise these: a non-list inbox/pings value, and a payload containing something json.dumps can't serialize. Both were untested on the restore side even though cmd_restore now shares the same validation and safety-net code. --- tests/test_restore.py | 48 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/test_restore.py b/tests/test_restore.py index 0f0359b..6e854c8 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -134,6 +134,54 @@ def test_cmd_restore_accepts_a_numeric_string_origin_seq( store.close() +def test_cmd_restore_dies_on_non_list_snapshot_field(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_on_non_list_snapshot_field. A truthy + non-list "inbox" (e.g. a malformed hub response sending an object instead + of a list) used to raise a raw TypeError from list(...) instead of a + clean die().""" + body = {"own_events": [], "inbox": {"not": "a list"}} + with pytest.raises(SystemExit, match="restore response inbox is not a list"): + _run_restore(tmp_path, monkeypatch, body) + + +def test_cmd_restore_dies_when_apply_remote_hits_an_unanticipated_shape( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_when_apply_remote_hits_an_unanticipated_shape. + A payload containing a value json.dumps cannot serialize (a stand-in for + "something genuinely unanticipated") reaches the broad except-Exception net + around apply_remote/mark_origin and becomes a clean die() instead of an + uncaught crash.""" + body = { + "own_events": [ + { + "origin_device_id": "other", + "origin_seq": 1, + "table": "activity", + "op": "insert", + "row_id": "x", + "payload": {"type": "message", "body": {1, 2, 3}}, + "occurred_at": "2026-08-13T12:00:00Z", + } + ] + } + with pytest.raises(SystemExit, match="restore event could not be applied"): + _run_restore(tmp_path, monkeypatch, body) + + +def test_cmd_restore_dies_when_apply_replica_row_hits_an_unanticipated_shape( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_when_apply_replica_row_hits_an_unanticipated_shape.""" + row = {**_valid_restore_row(), "payload": {"type": "message", "body": {1, 2, 3}}} + body = {"own_events": [], "inbox": [row]} + with pytest.raises(SystemExit, match="restore snapshot could not be applied"): + _run_restore(tmp_path, monkeypatch, body) + + def test_cmd_restore_applies_events_and_snapshots( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: From 9d1dd0e24d473d954170d0fd0963f5fc75fb8021 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 00:10:36 -0300 Subject: [PATCH 15/35] Widen OTel trace/span redaction to JSON, colon, and quoted-value forms. The regexes only matched logfmt key=value. Three independent reviews across this PR's history (two different vendor families) flagged that a span_id in any other real OTel log shape - JSON (span_id:...), key: value, or a quoted value with = - still fell under _HEX's 20-char floor and leaked into the dedup fingerprint unredacted, the exact class of bug this PR exists to fix. Capture the label/separator/quote prefix and substitute only the value, so the redacted line still reads naturally in whatever format it came in. --- src/agent_cli/errors.py | 14 ++++++++------ tests/test_errors.py | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/agent_cli/errors.py b/src/agent_cli/errors.py index ab8c7a0..4805cba 100644 --- a/src/agent_cli/errors.py +++ b/src/agent_cli/errors.py @@ -28,9 +28,11 @@ r'(?i)("[^"]*(?:password|secret|token|api[_-]?key|access[_-]?token|client[_-]?secret|authorization|passwd|access_key)[^"]*"\s*:\s*")[^"]*(")' ) _HEX = re.compile(r"\b[a-fA-F0-9]{20,}\b") -_OTEL_TRACE_ID = re.compile(r"(?i)\btrace_id=[0-9a-fA-F]{32}\b") -_OTEL_SPAN_ID = re.compile(r"(?i)\bspan_id=[0-9a-fA-F]{16}\b") -_OTEL_TRACEPARENT = re.compile(r"(?i)\btraceparent:\s*[0-9a-fA-F]{2}-[0-9a-fA-F]{32}-[0-9a-fA-F]{16}-[0-9a-fA-F]{2}\b") +_OTEL_TRACE_ID = re.compile(r'(?i)(\btrace_id["\']?\s*[:=]\s*["\']?)[0-9a-fA-F]{32}\b') +_OTEL_SPAN_ID = re.compile(r'(?i)(\bspan_id["\']?\s*[:=]\s*["\']?)[0-9a-fA-F]{16}\b') +_OTEL_TRACEPARENT = re.compile( + r'(?i)(\btraceparent["\']?\s*[:=]\s*["\']?)[0-9a-fA-F]{2}-[0-9a-fA-F]{32}-[0-9a-fA-F]{16}-[0-9a-fA-F]{2}\b' +) _SECRET = re.compile( r"(?i)(? str: out = _AKIA.sub("[redacted]", out) out = _JWT.sub("[redacted]", out) out = _EMAIL.sub("[redacted]", out) - out = _OTEL_TRACE_ID.sub("trace_id=[redacted]", out) - out = _OTEL_SPAN_ID.sub("span_id=[redacted]", out) - out = _OTEL_TRACEPARENT.sub("traceparent: [redacted]", out) + out = _OTEL_TRACE_ID.sub(r"\1[redacted]", out) + out = _OTEL_SPAN_ID.sub(r"\1[redacted]", out) + out = _OTEL_TRACEPARENT.sub(r"\1[redacted]", out) out = _HEX.sub("[redacted]", out) return out diff --git a/tests/test_errors.py b/tests/test_errors.py index 54c61cb..c5cb7ae 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -182,6 +182,21 @@ def test_redact_and_fingerprint() -> None: assert trace_id not in traceparent assert span_id not in traceparent assert "traceparent: [redacted]" in traceparent + traceparent_eq = redact(f"TimeoutError boom traceparent=00-{trace_id}-{span_id}-01") + assert trace_id not in traceparent_eq + assert span_id not in traceparent_eq + otel_colon = redact(f"TimeoutError boom trace_id: {trace_id} span_id: {span_id}") + assert trace_id not in otel_colon + assert span_id not in otel_colon + otel_json = redact(f'TimeoutError boom "trace_id":"{trace_id}","span_id":"{span_id}"') + assert trace_id not in otel_json + assert span_id not in otel_json + otel_json_spaced = redact(f'TimeoutError boom "trace_id": "{trace_id}", "span_id": "{span_id}"') + assert trace_id not in otel_json_spaced + assert span_id not in otel_json_spaced + otel_quoted_value = redact(f'TimeoutError boom trace_id="{trace_id}" span_id="{span_id}"') + assert trace_id not in otel_quoted_value + assert span_id not in otel_quoted_value def test_scan_inserts_once_then_enriches(tmp_path: Path) -> None: From fffce6e96d8526b94be2fa1d73c94bc885273817 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 00:23:02 -0300 Subject: [PATCH 16/35] Strengthen the widened OTel redaction tests to check the preserved prefix. Five of the seven new shape assertions only checked that the raw trace/span id value was gone, not that the label/separator/quote prefix survived the substitution intact - a regression dropping the capture group (subbing the whole match instead of just the value) would still have passed. Added the missing prefix-preserving assertion for each shape. --- tests/test_errors.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/test_errors.py b/tests/test_errors.py index c5cb7ae..10716bb 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -185,18 +185,27 @@ def test_redact_and_fingerprint() -> None: traceparent_eq = redact(f"TimeoutError boom traceparent=00-{trace_id}-{span_id}-01") assert trace_id not in traceparent_eq assert span_id not in traceparent_eq + assert "traceparent=[redacted]" in traceparent_eq otel_colon = redact(f"TimeoutError boom trace_id: {trace_id} span_id: {span_id}") assert trace_id not in otel_colon assert span_id not in otel_colon + assert "trace_id: [redacted]" in otel_colon + assert "span_id: [redacted]" in otel_colon otel_json = redact(f'TimeoutError boom "trace_id":"{trace_id}","span_id":"{span_id}"') assert trace_id not in otel_json assert span_id not in otel_json + assert '"trace_id":"[redacted]"' in otel_json + assert '"span_id":"[redacted]"' in otel_json otel_json_spaced = redact(f'TimeoutError boom "trace_id": "{trace_id}", "span_id": "{span_id}"') assert trace_id not in otel_json_spaced assert span_id not in otel_json_spaced + assert '"trace_id": "[redacted]"' in otel_json_spaced + assert '"span_id": "[redacted]"' in otel_json_spaced otel_quoted_value = redact(f'TimeoutError boom trace_id="{trace_id}" span_id="{span_id}"') assert trace_id not in otel_quoted_value assert span_id not in otel_quoted_value + assert 'trace_id="[redacted]"' in otel_quoted_value + assert 'span_id="[redacted]"' in otel_quoted_value def test_scan_inserts_once_then_enriches(tmp_path: Path) -> None: From 39520973166e78b800866f160e7c9c4558578a76 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 00:49:47 -0300 Subject: [PATCH 17/35] Handle escaped-quote OTel log shapes; fix docs and a misleading test fixture. The optional-quote group in the OTel regexes didn't match a backslash-escaped quote, so a span_id inside an escaped nested-JSON shape (e.g. Docker's json-file log driver wrapping an application's JSON log line) still leaked past the fix, the same defect class this PR closes elsewhere. The quote groups now accept an optional leading backslash. Also documented the new redaction scope in DESIGN.md (it only mentioned the -configured secret/PII redaction, not the always-on OTel id stripping), and fixed cmd_restore's happy-path test to use a genuine own-device origin for its own_events fixture instead of a foreign one, matching what its docstring already claimed and DESIGN.md's own/foreign distinction. --- DESIGN.md | 2 +- src/agent_cli/errors.py | 6 +++--- tests/test_errors.py | 10 ++++++++++ tests/test_restore.py | 11 +++++++++-- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index d726887..4aef3fa 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -593,7 +593,7 @@ The script: 1. Authenticates with credentials that never enter the store or `evidence`. 2. Pulls new lines since the last cursor (persisted next to the config). 3. Filters to incident lines only: HTTP access-log lines (`METHOD path status`) are dropped; lines with a logger level token `ERROR` / `FATAL` / `PANIC` / `CRITICAL` are kept; lines with an `*Error` / `*Exception` class are kept; other lines (including ones that merely mention the word "error") are dropped. Optional config strings `line_must_match` / `line_must_not_match` further filter (non-empty regexes; invalid values are rejected at load). Filtered lines advance the cursor but do not insert `error.seen`. -4. Redacts secrets and obvious personal data **before** any row is written. +4. Redacts secrets and obvious personal data **before** any row is written. Separately from the `$AGENT_HOME`-configured redaction, an OTel `trace_id`/`span_id`/`traceparent` value (logfmt, colon, JSON, or quoted-value form) is always stripped before hashing — those are per-occurrence random ids, not secrets, but leaving them in would make every occurrence of the same recurring error hash to a different stack signature and never dedupe. 5. Computes a fingerprint: service + error class + normalized stack signature + environment. 6. Inserts `error.seen` or **enriches** an existing **open** row with that fingerprint on this session (`count`, `last_seen`, optional extra excerpt, optional `line_fingerprint`). First insert knocks `da ist Post id `. Enrichment never knocks. After skip or a terminal implement task, the next match is a new `error.seen` (new id, knocks). 7. Payload holds a **sanitized** excerpt plus an optional pointer to raw evidence on this disk. It does not hold the full log dump. diff --git a/src/agent_cli/errors.py b/src/agent_cli/errors.py index 4805cba..3a41f4d 100644 --- a/src/agent_cli/errors.py +++ b/src/agent_cli/errors.py @@ -28,10 +28,10 @@ r'(?i)("[^"]*(?:password|secret|token|api[_-]?key|access[_-]?token|client[_-]?secret|authorization|passwd|access_key)[^"]*"\s*:\s*")[^"]*(")' ) _HEX = re.compile(r"\b[a-fA-F0-9]{20,}\b") -_OTEL_TRACE_ID = re.compile(r'(?i)(\btrace_id["\']?\s*[:=]\s*["\']?)[0-9a-fA-F]{32}\b') -_OTEL_SPAN_ID = re.compile(r'(?i)(\bspan_id["\']?\s*[:=]\s*["\']?)[0-9a-fA-F]{16}\b') +_OTEL_TRACE_ID = re.compile(r'(?i)(\btrace_id(?:\\?["\'])?\s*[:=]\s*(?:\\?["\'])?)[0-9a-fA-F]{32}\b') +_OTEL_SPAN_ID = re.compile(r'(?i)(\bspan_id(?:\\?["\'])?\s*[:=]\s*(?:\\?["\'])?)[0-9a-fA-F]{16}\b') _OTEL_TRACEPARENT = re.compile( - r'(?i)(\btraceparent["\']?\s*[:=]\s*["\']?)[0-9a-fA-F]{2}-[0-9a-fA-F]{32}-[0-9a-fA-F]{16}-[0-9a-fA-F]{2}\b' + r'(?i)(\btraceparent(?:\\?["\'])?\s*[:=]\s*(?:\\?["\'])?)[0-9a-fA-F]{2}-[0-9a-fA-F]{32}-[0-9a-fA-F]{16}-[0-9a-fA-F]{2}\b' ) _SECRET = re.compile( r"(?i)(? None: assert span_id not in otel_quoted_value assert 'trace_id="[redacted]"' in otel_quoted_value assert 'span_id="[redacted]"' in otel_quoted_value + # Docker json-file log driver wraps an application's JSON log line inside its + # own "log" field, escaping the inner quotes - a very plausible real shape + # for a containerized service's logs, not a hypothetical one. + otel_escaped_json = redact( + '{"log":"...{\\"trace_id\\":\\"' + trace_id + '\\",\\"span_id\\":\\"' + span_id + '\\"}..."}' + ) + assert trace_id not in otel_escaped_json + assert span_id not in otel_escaped_json + assert '\\"trace_id\\":\\"[redacted]\\"' in otel_escaped_json + assert '\\"span_id\\":\\"[redacted]\\"' in otel_escaped_json def test_scan_inserts_once_then_enriches(tmp_path: Path) -> None: diff --git a/tests/test_restore.py b/tests/test_restore.py index 6e854c8..2610639 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -186,11 +186,18 @@ def test_cmd_restore_applies_events_and_snapshots( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: """Happy-path coverage: cmd_restore had none before this change. Replays - one own event and one inbox snapshot row.""" + one own event (own_events replays this device's own history, so its + origin_device_id must genuinely be this device's own id, not a foreign + one - foreign data only ever arrives via inbox/pings) and one inbox + snapshot row (from another device, correctly foreign).""" + _init_store(tmp_path) + store = open_store() + device_id = store.device_id() + store.close() body = { "own_events": [ { - "origin_device_id": "other", + "origin_device_id": device_id, "origin_seq": 1, "table": "task", "op": "insert", From 4b5d3972e95f25ac5c94988392f9800fb3c8e38b Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 01:05:34 -0300 Subject: [PATCH 18/35] Fix a second foreign-origin fixture and cover the wake=False guard branch. test_cmd_restore_accepts_a_numeric_string_origin_seq still used a foreign origin_device_id for an own_events entry - the prior round's fix only caught the other instance in this file. Also: apply_remote/apply_replica_row's wake=False branches (the only mode cmd_restore ever uses) each have their own inline isinstance(typ, str) guard against a non-string payload type, mirroring _maybe_wake's guard on the wake=True path. The wake=True guard has sync-side regression tests; this one had none - a regression that reintroduced the unguarded check only in the wake=False branch would have stayed green everywhere except real restore runs. --- tests/test_restore.py | 65 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 3 deletions(-) diff --git a/tests/test_restore.py b/tests/test_restore.py index 2610639..f7fc306 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -111,11 +111,18 @@ def test_cmd_restore_accepts_a_numeric_string_origin_seq( _insert_event_idempotent's plain Python `event["origin_seq"] != last_seq + 1` as a string, deterministically raising a false "origin_seq gap" for the very first restored event. cmd_restore now shares _sync_once's coercion, - applied before apply_remote sees the event.""" + applied before apply_remote sees the event. own_events replays this + device's own history, so origin_device_id must genuinely be this + device's own id, not a foreign one - same reasoning as + test_cmd_restore_applies_events_and_snapshots below.""" + _init_store(tmp_path) + store = open_store() + device_id = store.device_id() + store.close() body = { "own_events": [ { - "origin_device_id": "other", + "origin_device_id": device_id, "origin_seq": "1", "table": "task", "op": "insert", @@ -128,7 +135,7 @@ def test_cmd_restore_accepts_a_numeric_string_origin_seq( _run_restore(tmp_path, monkeypatch, body) store = open_store() try: - assert store.origin_cursor("other") == 1 + assert store.origin_cursor(device_id) == 1 assert store.row("task", "t1") is not None finally: store.close() @@ -182,6 +189,58 @@ def test_cmd_restore_dies_when_apply_replica_row_hits_an_unanticipated_shape( _run_restore(tmp_path, monkeypatch, body) +def test_cmd_restore_accepts_an_event_whose_type_is_not_a_wake_type_shape( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: apply_remote's wake=False branch (the only one + cmd_restore ever uses) has its own inline isinstance(typ, str) guard + before the WAKE_ACTIVITY_TYPES membership check (store.py's + apply_remote, elif inserted: branch). Unlike the wake=True path + (_maybe_wake, covered by tests/test_pending.py), nothing exercised this + wake=False guard specifically - a regression that reintroduced the + unguarded check only there would stay green on the sync side while + breaking every restore whose event has a non-string type.""" + _init_store(tmp_path) + store = open_store() + device_id = store.device_id() + store.close() + body = { + "own_events": [ + { + "origin_device_id": device_id, + "origin_seq": 1, + "table": "activity", + "op": "insert", + "row_id": "x", + "payload": {"type": ["not", "hashable"]}, + "occurred_at": "2026-08-13T12:00:00Z", + } + ] + } + _run_restore(tmp_path, monkeypatch, body) + store = open_store() + try: + assert store.row("activity", "x") is not None + finally: + store.close() + + +def test_cmd_restore_accepts_a_snapshot_whose_type_is_not_a_wake_type_shape( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the row-side sibling of + test_cmd_restore_accepts_an_event_whose_type_is_not_a_wake_type_shape. + apply_replica_row's wake=False branch has the same inline guard.""" + row = {**_valid_restore_row(), "payload": {"type": ["not", "hashable"]}} + body = {"own_events": [], "inbox": [row]} + _run_restore(tmp_path, monkeypatch, body) + store = open_store() + try: + assert store.row("activity", "x") is not None + finally: + store.close() + + def test_cmd_restore_applies_events_and_snapshots( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: From e374edfa791881716a1564e094767c2446c095f6 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 01:17:52 -0300 Subject: [PATCH 19/35] Cover the missing rejection-side test for cmd_restore's origin_seq coercion. Same recurring pattern: the sync side had a parametrized regression test proving _coerce_pull_event rejects a bad origin_seq (bool, fractional float, overflow float, garbage string); the restore side only had the acceptance-side sibling for a valid numeric string, never a test proving restore actually rejects a bad one. --- tests/test_restore.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/test_restore.py b/tests/test_restore.py index f7fc306..6f166cc 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -102,6 +102,30 @@ def test_cmd_restore_dies_on_snapshot_missing_one_required_field( _run_restore(tmp_path, monkeypatch, body) +@pytest.mark.parametrize( + "bad_seq", + [ + "not-a-number", + 1e309, # float('inf') once parsed - int() raises OverflowError, not ValueError + True, # bool is an int subclass in Python - int(True) == 1 would pass silently + 1.5, # non-integral float - int(1.5) == 1 would silently truncate + ], +) +def test_cmd_restore_dies_on_non_numeric_origin_seq( + bad_seq: object, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_on_non_numeric_origin_seq. cmd_restore + shares _sync_once's origin_seq coercion (_coerce_pull_event), but had no + test proving the restore side actually rejects a bad origin_seq rather + than just accepting a good one.""" + event = _valid_restore_event() + event["origin_seq"] = bad_seq + body = {"own_events": [event]} + with pytest.raises(SystemExit, match="restore event has a non-numeric origin_seq"): + _run_restore(tmp_path, monkeypatch, body) + + def test_cmd_restore_accepts_a_numeric_string_origin_seq( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: From 0f78e19522f2ba80b30a295b0de129dd7a0feea9 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 01:47:19 -0300 Subject: [PATCH 20/35] Validate payload shape and op value in the shared pull-event/row helpers. Field presence was checked but not payload's type or op's value. A non-dict payload or an unrecognized op used to pass _coerce_pull_event/ _check_pull_row untouched: apply_remote/apply_replica_row would commit it as-is (_maybe_wake/_maybe_work both already return early on a non-dict payload without raising, so the broad except-Exception backstop never fires), and the corruption would only surface later - on every future store.rows()/store.row() call for that whole table, not at write time. Store._write_in_txn already enforces both constraints for this device's own local writes; the hub-pull/restore path must not be laxer. Mirrors that same validation. Also added the missing traceparent JSON/quoted-value/escaped-JSON test coverage - the regex already supported those shapes, only the test coverage was asymmetric with trace_id/span_id. --- src/agent_cli/main.py | 24 +++++++++++++++++++----- tests/test_errors.py | 17 +++++++++++++++++ tests/test_pending.py | 37 +++++++++++++++++++++++++++++++++++++ tests/test_restore.py | 39 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 5 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 38d99b3..98973dd 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1762,12 +1762,21 @@ class _PullShapeError(ValueError): def _coerce_pull_event(event: object) -> dict[str, Any]: """Validate a pulled/restored event has every field _insert_event_idempotent - indexes, and normalize origin_seq to an int (rejecting bool, a fractional - float, and anything int() can't convert, including an out-of-range float - that would otherwise raise OverflowError). Returns a new dict; the caller's - own copy of the raw event is left untouched.""" + indexes, that payload/op are shaped the way a local write already requires + (Store._write_in_txn rejects both the same way for this device's own + writes - the hub-pull/restore path must not be laxer, since a payload + that isn't an object is otherwise stored as-is and only fails later, on + every future read of that whole table, not at write time), and normalize + origin_seq to an int (rejecting bool, a fractional float, and anything + int() can't convert, including an out-of-range float that would + otherwise raise OverflowError). Returns a new dict; the caller's own copy + of the raw event is left untouched.""" if not isinstance(event, dict) or any(field not in event for field in _PULL_EVENT_FIELDS): raise _PullShapeError("event is missing required fields") + if not isinstance(event["payload"], dict): + raise _PullShapeError("event payload is not an object") + if event["op"] not in ("insert", "update", "delete"): + raise _PullShapeError("event has an unknown op") raw_seq = event["origin_seq"] try: if isinstance(raw_seq, bool): @@ -1782,9 +1791,14 @@ def _coerce_pull_event(event: object) -> dict[str, Any]: def _check_pull_row(row: object) -> dict[str, Any]: """Validate a pulled/restored snapshot row has every field - apply_replica_row indexes directly.""" + apply_replica_row indexes directly, and that payload is an object - same + reasoning as _coerce_pull_event: an unvalidated non-object payload would + otherwise be stored as-is and only fail later, on every future read of + that whole table.""" if not isinstance(row, dict) or any(field not in row for field in _PULL_ROW_FIELDS): raise _PullShapeError("snapshot is missing required fields") + if not isinstance(row["payload"], dict): + raise _PullShapeError("snapshot payload is not an object") return row diff --git a/tests/test_errors.py b/tests/test_errors.py index 775e4bb..31550b9 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -216,6 +216,23 @@ def test_redact_and_fingerprint() -> None: assert span_id not in otel_escaped_json assert '\\"trace_id\\":\\"[redacted]\\"' in otel_escaped_json assert '\\"span_id\\":\\"[redacted]\\"' in otel_escaped_json + traceparent_value = f"00-{trace_id}-{span_id}-01" + traceparent_json = redact(f'TimeoutError boom "traceparent":"{traceparent_value}"') + assert trace_id not in traceparent_json + assert span_id not in traceparent_json + assert '"traceparent":"[redacted]"' in traceparent_json + traceparent_json_spaced = redact(f'TimeoutError boom "traceparent": "{traceparent_value}"') + assert trace_id not in traceparent_json_spaced + assert span_id not in traceparent_json_spaced + assert '"traceparent": "[redacted]"' in traceparent_json_spaced + traceparent_quoted_value = redact(f'TimeoutError boom traceparent="{traceparent_value}"') + assert trace_id not in traceparent_quoted_value + assert span_id not in traceparent_quoted_value + assert 'traceparent="[redacted]"' in traceparent_quoted_value + traceparent_escaped_json = redact('{"log":"...{\\"traceparent\\":\\"' + traceparent_value + '\\"}..."}') + assert trace_id not in traceparent_escaped_json + assert span_id not in traceparent_escaped_json + assert '\\"traceparent\\":\\"[redacted]\\"' in traceparent_escaped_json def test_scan_inserts_once_then_enriches(tmp_path: Path) -> None: diff --git a/tests/test_pending.py b/tests/test_pending.py index a204350..12b5704 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -300,6 +300,43 @@ def test_sync_once_raises_hub_error_on_event_missing_one_required_field( _sync_once(_paired_store(tmp_path)) +def test_sync_once_raises_hub_error_on_event_with_non_dict_payload( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: field presence was checked but not payload's type. + A non-dict payload (e.g. a JSON list) used to pass validation untouched, + get committed by apply_remote (Store._maybe_wake/_maybe_work each already + return early on a non-dict payload, so nothing rolls back the write), and + only fail later - on every future store.rows()/store.row() call for that + whole table, not at write time. Store._write_in_txn already rejects this + shape for this device's own local writes; the hub-pull path must not be + laxer.""" + event = {**_valid_pull_event(), "payload": ["not", "an", "object"]} + hub = FakeHub() + hub.pull_body = {"events": [event]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + store = _paired_store(tmp_path) + with pytest.raises(HubError, match="pull event payload is not an object"): + _sync_once(store) + assert store.origin_cursor("other") == 0 + assert store.rows("activity") == [] + + +def test_sync_once_raises_hub_error_on_event_with_unknown_op( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: sibling of the non-dict-payload test above, for op. + Store._write_in_txn rejects any op outside insert/update/delete for a + local write; the hub-pull path must reject it too, not silently accept + and materialize a row under an op nothing else in the codebase expects.""" + event = {**_valid_pull_event(), "op": "bogus"} + hub = FakeHub() + hub.pull_body = {"events": [event]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull event has an unknown op"): + _sync_once(_paired_store(tmp_path)) + + @pytest.mark.parametrize( "bad_seq", [ diff --git a/tests/test_restore.py b/tests/test_restore.py index 6f166cc..e97024a 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -102,6 +102,45 @@ def test_cmd_restore_dies_on_snapshot_missing_one_required_field( _run_restore(tmp_path, monkeypatch, body) +def test_cmd_restore_dies_on_event_with_non_dict_payload( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_on_event_with_non_dict_payload. Field + presence was checked but not payload's type - a non-dict payload used to + pass validation, get committed by apply_remote, and only fail later on + every future read of that whole table.""" + event = {**_valid_restore_event(), "payload": ["not", "an", "object"]} + body = {"own_events": [event]} + with pytest.raises(SystemExit, match="restore event payload is not an object"): + _run_restore(tmp_path, monkeypatch, body) + store = open_store() + try: + assert store.rows("task") == [] + finally: + store.close() + + +def test_cmd_restore_dies_on_event_with_unknown_op(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_on_event_with_unknown_op.""" + event = {**_valid_restore_event(), "op": "bogus"} + body = {"own_events": [event]} + with pytest.raises(SystemExit, match="restore event has an unknown op"): + _run_restore(tmp_path, monkeypatch, body) + + +def test_cmd_restore_dies_on_snapshot_with_non_dict_payload( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the row-side sibling of + test_cmd_restore_dies_on_event_with_non_dict_payload.""" + row = {**_valid_restore_row(), "payload": ["not", "an", "object"]} + body = {"own_events": [], "inbox": [row]} + with pytest.raises(SystemExit, match="restore snapshot payload is not an object"): + _run_restore(tmp_path, monkeypatch, body) + + @pytest.mark.parametrize( "bad_seq", [ From 5ef22a644367b5e2df947112bd57eddfb393f084 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 02:03:18 -0300 Subject: [PATCH 21/35] Validate table against OWNED_TABLES; close a third payload-corruption entry point. _coerce_pull_event/_check_pull_row checked payload/op but not table against OWNED_TABLES, unlike Store._write_in_txn's own validation for local writes. Lower severity than the payload/op gap (no known-table read gets poisoned, since _maybe_wake/_maybe_work/_upsert_row treat table as an opaque string), but a real asymmetry: an unrecognized table value would be durably committed as an orphaned row nothing ever reads back. Fixed in both validators. More importantly: _run_sync_ws_session's incoming "subscription" frame handler applied rows straight to store.apply_replica_row, validated only by isinstance(row, dict) and a truthy "table" - never through _check_pull_row. That's a third live entry point (after _sync_once and cmd_restore) for the exact bug this PR closed elsewhere: a non-dict payload doesn't raise anywhere in apply_replica_row/_upsert_row, so it would be committed as-is and only fail later, on every future read of that whole table. Now routes through the same shared validator. Also: cmd_restore's snapshot loop validated and applied each row in the same iteration, so an earlier valid row could already be committed before a later malformed one triggered die() - unlike _sync_once's snapshot handling, which validates the full batch before applying any of it. Split into two passes to match. Fixed a pre-existing test whose minimal row fixture (missing most required fields) no longer reached the mocked apply_replica_row it was actually testing, now that row validation runs first - the test mocks apply_replica_row directly to simulate a StoreConnectionError, so the row's shape was never meant to matter; given it a fully valid shape. --- src/agent_cli/main.py | 11 ++++-- tests/test_pending.py | 32 +++++++++++++++++ tests/test_restore.py | 29 +++++++++++++++- tests/test_sync_follow_reconnect.py | 54 +++++++++++++++++++++++++++-- 4 files changed, 120 insertions(+), 6 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 98973dd..5c656ca 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -46,7 +46,7 @@ tmux_name, ) from .skills import SKILL_NAMES, has_skill, skill_for_agent_role -from .store import Store, StoreConnectionError, StoreError, utcnow +from .store import OWNED_TABLES, Store, StoreConnectionError, StoreError, utcnow from .usage import AuthStale, scan_usage, usage_poll_due from .watch import ( assigned_session_id, @@ -1464,7 +1464,9 @@ def _run_sync_ws_session( rows = message.get("rows") if isinstance(rows, list): for row in rows: - if not isinstance(row, dict) or not row.get("table"): + try: + row = _check_pull_row(row) + except _PullShapeError: continue try: store.apply_replica_row(row) @@ -1524,6 +1526,7 @@ def cmd_restore(_: list[str]) -> None: _check_pull_row(row) except _PullShapeError as exc: die(f"restore {exc}") + for row in snapshots: try: store.apply_replica_row(row, wake=False) except Exception as exc: @@ -1773,6 +1776,8 @@ def _coerce_pull_event(event: object) -> dict[str, Any]: of the raw event is left untouched.""" if not isinstance(event, dict) or any(field not in event for field in _PULL_EVENT_FIELDS): raise _PullShapeError("event is missing required fields") + if event["table"] not in OWNED_TABLES: + raise _PullShapeError("event has an unknown table") if not isinstance(event["payload"], dict): raise _PullShapeError("event payload is not an object") if event["op"] not in ("insert", "update", "delete"): @@ -1797,6 +1802,8 @@ def _check_pull_row(row: object) -> dict[str, Any]: that whole table.""" if not isinstance(row, dict) or any(field not in row for field in _PULL_ROW_FIELDS): raise _PullShapeError("snapshot is missing required fields") + if row["table"] not in OWNED_TABLES: + raise _PullShapeError("snapshot has an unknown table") if not isinstance(row["payload"], dict): raise _PullShapeError("snapshot payload is not an object") return row diff --git a/tests/test_pending.py b/tests/test_pending.py index 12b5704..b8d5514 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -333,7 +333,26 @@ def test_sync_once_raises_hub_error_on_event_with_unknown_op( hub = FakeHub() hub.pull_body = {"events": [event]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + store = _paired_store(tmp_path) with pytest.raises(HubError, match="pull event has an unknown op"): + _sync_once(store) + assert store.origin_cursor("other") == 0 + assert store.rows("activity") == [] + + +def test_sync_once_raises_hub_error_on_event_with_unknown_table( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: sibling of the unknown-op test above, for table. + Store._write_in_txn rejects any table outside OWNED_TABLES for a local + write; the hub-pull path must reject it too, instead of durably + committing an orphaned row under a table name no application code ever + reads back.""" + event = {**_valid_pull_event(), "table": "not_a_real_table"} + hub = FakeHub() + hub.pull_body = {"events": [event]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull event has an unknown table"): _sync_once(_paired_store(tmp_path)) @@ -506,6 +525,19 @@ def test_sync_once_raises_hub_error_on_snapshot_missing_one_required_field( _sync_once(_paired_store(tmp_path)) +def test_sync_once_raises_hub_error_on_snapshot_with_unknown_table( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the row-side sibling of + test_sync_once_raises_hub_error_on_event_with_unknown_table.""" + row = {**_valid_pull_row(), "table": "not_a_real_table"} + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": [row]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull snapshot has an unknown table"): + _sync_once(_paired_store(tmp_path)) + + def test_sync_once_accepts_a_snapshot_payload_whose_type_is_not_a_wake_type_shape( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_restore.py b/tests/test_restore.py index e97024a..9de8130 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -116,7 +116,7 @@ def test_cmd_restore_dies_on_event_with_non_dict_payload( _run_restore(tmp_path, monkeypatch, body) store = open_store() try: - assert store.rows("task") == [] + assert store.rows("activity") == [] finally: store.close() @@ -128,6 +128,22 @@ def test_cmd_restore_dies_on_event_with_unknown_op(tmp_path: Path, monkeypatch: body = {"own_events": [event]} with pytest.raises(SystemExit, match="restore event has an unknown op"): _run_restore(tmp_path, monkeypatch, body) + store = open_store() + try: + assert store.rows("activity") == [] + finally: + store.close() + + +def test_cmd_restore_dies_on_event_with_unknown_table( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_on_event_with_unknown_table.""" + event = {**_valid_restore_event(), "table": "not_a_real_table"} + body = {"own_events": [event]} + with pytest.raises(SystemExit, match="restore event has an unknown table"): + _run_restore(tmp_path, monkeypatch, body) def test_cmd_restore_dies_on_snapshot_with_non_dict_payload( @@ -141,6 +157,17 @@ def test_cmd_restore_dies_on_snapshot_with_non_dict_payload( _run_restore(tmp_path, monkeypatch, body) +def test_cmd_restore_dies_on_snapshot_with_unknown_table( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the row-side sibling of + test_cmd_restore_dies_on_event_with_unknown_table.""" + row = {**_valid_restore_row(), "table": "not_a_real_table"} + body = {"own_events": [], "inbox": [row]} + with pytest.raises(SystemExit, match="restore snapshot has an unknown table"): + _run_restore(tmp_path, monkeypatch, body) + + @pytest.mark.parametrize( "bad_seq", [ diff --git a/tests/test_sync_follow_reconnect.py b/tests/test_sync_follow_reconnect.py index 4099029..29b61f3 100644 --- a/tests/test_sync_follow_reconnect.py +++ b/tests/test_sync_follow_reconnect.py @@ -378,6 +378,49 @@ def fake_sync_once(store: object) -> None: assert calls == [1], "a genuine data-integrity StoreError must not be retried" +def test_subscription_row_with_non_dict_payload_is_skipped_not_committed( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: incoming websocket subscription rows went straight to + store.apply_replica_row(row) guarded only by isinstance(row, dict) and + row.get("table") - never through _check_pull_row. A non-dict payload + doesn't raise anywhere in apply_replica_row/_upsert_row (dumps([]) is + valid JSON), so it used to get durably committed and only fail later, on + every future store.rows()/row() call for that whole table - the same bug + class this PR closed for _sync_once/cmd_restore, reachable a third way + through the live subscription push path.""" + _init_paired_store(tmp_path) + store = open_store() + try: + row = { + "table": "session", + "origin_device_id": "other", + "row_id": "s1", + "payload": ["not", "an", "object"], + "updated_at": "2026-08-13T12:00:00Z", + } + + class _SubscriptionWs: + def send(self, data: str) -> None: + pass + + def __iter__(self): + return iter([json.dumps({"type": "subscription", "rows": [row]})]) + + def close(self) -> None: + pass + + class _FakeHub: + def connect_sync_ws(self) -> _SubscriptionWs: + return _SubscriptionWs() + + with pytest.raises(HubError, match="websocket closed"): + main_mod._run_sync_ws_session(store, _FakeHub(), _FakeRuntime(), {}, {}, {}) + assert store.rows("session") == [] + finally: + store.close() + + def test_subscription_row_store_connection_error_reaches_the_reconnect_loop( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -402,9 +445,14 @@ def send(self, data: str) -> None: pass def __iter__(self): - return iter( - [json.dumps({"type": "subscription", "rows": [{"table": "session", "id": "s1"}]})] - ) + row = { + "table": "session", + "origin_device_id": "other", + "row_id": "s1", + "payload": {"id": "s1"}, + "updated_at": "2026-08-13T12:00:00Z", + } + return iter([json.dumps({"type": "subscription", "rows": [row]})]) def close(self) -> None: pass From 11a2fead3433d6b1cd1c4185d5b26cb6ffe8e5dc Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 02:19:44 -0300 Subject: [PATCH 22/35] Guard the new table checks against unhashable values; correct two docstrings. event["table"] not in OWNED_TABLES (a frozenset) requires table to be hashable - the exact bug class already fixed five times elsewhere in this PR for payload["type"] (isinstance(typ, str) guards in store.py). A JSON-decoded list/dict for table raised a raw TypeError that no caller's except clause caught, crashing the knock daemon, the sync --follow daemon, or the restore command depending on which entry point received it. Same isinstance(table, str) guard, same fix, now applied everywhere OWNED_TABLES is checked. Also corrected two docstrings/comments that overclaimed Store._write_in_txn already rejects a non-dict payload for local writes - it only validates table and op, not payload's type. Left _write_in_txn itself unchanged: local writes are this codebase's own trusted, code-constructed payloads, not externally-supplied hub data, so the risk this PR's validators guard against doesn't apply there. --- src/agent_cli/main.py | 17 +++++++----- tests/test_pending.py | 36 ++++++++++++++++++++++--- tests/test_restore.py | 22 ++++++++++++++++ tests/test_sync_follow_reconnect.py | 41 +++++++++++++++++++++++++++++ 4 files changed, 106 insertions(+), 10 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 5c656ca..37ac4a7 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1765,18 +1765,21 @@ class _PullShapeError(ValueError): def _coerce_pull_event(event: object) -> dict[str, Any]: """Validate a pulled/restored event has every field _insert_event_idempotent - indexes, that payload/op are shaped the way a local write already requires - (Store._write_in_txn rejects both the same way for this device's own - writes - the hub-pull/restore path must not be laxer, since a payload - that isn't an object is otherwise stored as-is and only fails later, on - every future read of that whole table, not at write time), and normalize + indexes, and that table/op are shaped the way Store._write_in_txn already + requires for this device's own local writes (op/table checked there; the + hub-pull/restore path must not be laxer). payload is additionally + required to be an object here even though _write_in_txn doesn't check + that for local writes either - a payload that isn't one is otherwise + stored as-is and only fails later, on every future read of that whole + table, not at write time - a risk specific to externally-supplied hub + data, not to this codebase's own trusted call sites. Normalize origin_seq to an int (rejecting bool, a fractional float, and anything int() can't convert, including an out-of-range float that would otherwise raise OverflowError). Returns a new dict; the caller's own copy of the raw event is left untouched.""" if not isinstance(event, dict) or any(field not in event for field in _PULL_EVENT_FIELDS): raise _PullShapeError("event is missing required fields") - if event["table"] not in OWNED_TABLES: + if not isinstance(event["table"], str) or event["table"] not in OWNED_TABLES: raise _PullShapeError("event has an unknown table") if not isinstance(event["payload"], dict): raise _PullShapeError("event payload is not an object") @@ -1802,7 +1805,7 @@ def _check_pull_row(row: object) -> dict[str, Any]: that whole table.""" if not isinstance(row, dict) or any(field not in row for field in _PULL_ROW_FIELDS): raise _PullShapeError("snapshot is missing required fields") - if row["table"] not in OWNED_TABLES: + if not isinstance(row["table"], str) or row["table"] not in OWNED_TABLES: raise _PullShapeError("snapshot has an unknown table") if not isinstance(row["payload"], dict): raise _PullShapeError("snapshot payload is not an object") diff --git a/tests/test_pending.py b/tests/test_pending.py index b8d5514..8aa46a1 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -308,9 +308,10 @@ def test_sync_once_raises_hub_error_on_event_with_non_dict_payload( get committed by apply_remote (Store._maybe_wake/_maybe_work each already return early on a non-dict payload, so nothing rolls back the write), and only fail later - on every future store.rows()/store.row() call for that - whole table, not at write time. Store._write_in_txn already rejects this - shape for this device's own local writes; the hub-pull path must not be - laxer.""" + whole table, not at write time. Store._write_in_txn doesn't check this + shape either for local writes, but that's this codebase's own trusted + code constructing payloads, not externally-supplied hub data - the risk + this test guards against is specific to the pull path.""" event = {**_valid_pull_event(), "payload": ["not", "an", "object"]} hub = FakeHub() hub.pull_body = {"events": [event]} @@ -356,6 +357,22 @@ def test_sync_once_raises_hub_error_on_event_with_unknown_table( _sync_once(_paired_store(tmp_path)) +def test_sync_once_raises_hub_error_on_event_with_unhashable_table( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: `table not in OWNED_TABLES` (a frozenset) requires + table to be hashable - the exact bug class already fixed for + payload["type"] elsewhere in this PR (isinstance(typ, str) guards in + store.py). A JSON-decoded list/dict for table used to raise a raw + TypeError instead of a catchable HubError.""" + event = {**_valid_pull_event(), "table": ["activity"]} + hub = FakeHub() + hub.pull_body = {"events": [event]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull event has an unknown table"): + _sync_once(_paired_store(tmp_path)) + + @pytest.mark.parametrize( "bad_seq", [ @@ -538,6 +555,19 @@ def test_sync_once_raises_hub_error_on_snapshot_with_unknown_table( _sync_once(_paired_store(tmp_path)) +def test_sync_once_raises_hub_error_on_snapshot_with_unhashable_table( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the row-side sibling of + test_sync_once_raises_hub_error_on_event_with_unhashable_table.""" + row = {**_valid_pull_row(), "table": ["activity"]} + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": [row]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull snapshot has an unknown table"): + _sync_once(_paired_store(tmp_path)) + + def test_sync_once_accepts_a_snapshot_payload_whose_type_is_not_a_wake_type_shape( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_restore.py b/tests/test_restore.py index 9de8130..5d8b0e8 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -146,6 +146,17 @@ def test_cmd_restore_dies_on_event_with_unknown_table( _run_restore(tmp_path, monkeypatch, body) +def test_cmd_restore_dies_on_event_with_unhashable_table( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_on_event_with_unhashable_table.""" + event = {**_valid_restore_event(), "table": ["activity"]} + body = {"own_events": [event]} + with pytest.raises(SystemExit, match="restore event has an unknown table"): + _run_restore(tmp_path, monkeypatch, body) + + def test_cmd_restore_dies_on_snapshot_with_non_dict_payload( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -168,6 +179,17 @@ def test_cmd_restore_dies_on_snapshot_with_unknown_table( _run_restore(tmp_path, monkeypatch, body) +def test_cmd_restore_dies_on_snapshot_with_unhashable_table( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the row-side sibling of + test_cmd_restore_dies_on_event_with_unhashable_table.""" + row = {**_valid_restore_row(), "table": ["activity"]} + body = {"own_events": [], "inbox": [row]} + with pytest.raises(SystemExit, match="restore snapshot has an unknown table"): + _run_restore(tmp_path, monkeypatch, body) + + @pytest.mark.parametrize( "bad_seq", [ diff --git a/tests/test_sync_follow_reconnect.py b/tests/test_sync_follow_reconnect.py index 29b61f3..1422a9d 100644 --- a/tests/test_sync_follow_reconnect.py +++ b/tests/test_sync_follow_reconnect.py @@ -421,6 +421,47 @@ def connect_sync_ws(self) -> _SubscriptionWs: store.close() +def test_subscription_row_with_unhashable_table_is_skipped_not_crashed( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: `table not in OWNED_TABLES` inside _check_pull_row + requires table to be hashable. A JSON-decoded list/dict for table used to + raise a raw TypeError that neither the _PullShapeError catch around + _check_pull_row nor the (StoreError, KeyError, TypeError) catch around + apply_replica_row (which only wraps the *other* try block) would catch - + crashing the whole agent sync --follow daemon on a malformed WS frame.""" + _init_paired_store(tmp_path) + store = open_store() + try: + row = { + "table": ["session"], + "origin_device_id": "other", + "row_id": "s1", + "payload": {"id": "s1"}, + "updated_at": "2026-08-13T12:00:00Z", + } + + class _SubscriptionWs: + def send(self, data: str) -> None: + pass + + def __iter__(self): + return iter([json.dumps({"type": "subscription", "rows": [row]})]) + + def close(self) -> None: + pass + + class _FakeHub: + def connect_sync_ws(self) -> _SubscriptionWs: + return _SubscriptionWs() + + with pytest.raises(HubError, match="websocket closed"): + main_mod._run_sync_ws_session(store, _FakeHub(), _FakeRuntime(), {}, {}, {}) + assert store.rows("session") == [] + finally: + store.close() + + def test_subscription_row_store_connection_error_reaches_the_reconnect_loop( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: From ee033026c705dd04762a7c6bba4d67c304450254 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 02:58:03 -0300 Subject: [PATCH 23/35] Validate a pulled/restored event's origin_device_id and a snapshot row's updated_at. An event in the events/own_events list is documented (DESIGN.md) as always this device's own history - foreign data only ever arrives as row snapshots - but nothing enforced that: a hub response could assert a foreign origin_device_id and it would be applied unchanged. Also validate updated_at is a non-empty string; an empty value passed the previous presence-only check, stored fine on first insert, and only broke a later legitimate update to that row with a raw timestamp cast error. Adds a regression test for cmd_restore's validate-all-then-apply-all snapshot batching (mirrored for _sync_once) proving a batch with one invalid row among valid ones commits nothing. --- src/agent_cli/main.py | 27 +++++-- tests/test_pending.py | 166 ++++++++++++++++++++++++------------------ tests/test_restore.py | 84 ++++++++++++++++++--- 3 files changed, 190 insertions(+), 87 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 37ac4a7..1b7fad3 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1505,7 +1505,7 @@ def cmd_restore(_: list[str]) -> None: die("restore response missing own_events") for event in events: try: - event = _coerce_pull_event(event) + event = _coerce_pull_event(event, store.device_id()) except _PullShapeError as exc: die(f"restore {exc}") try: @@ -1763,7 +1763,7 @@ class _PullShapeError(ValueError): module.""" -def _coerce_pull_event(event: object) -> dict[str, Any]: +def _coerce_pull_event(event: object, own_device_id: str) -> dict[str, Any]: """Validate a pulled/restored event has every field _insert_event_idempotent indexes, and that table/op are shaped the way Store._write_in_txn already requires for this device's own local writes (op/table checked there; the @@ -1775,10 +1775,18 @@ def _coerce_pull_event(event: object) -> dict[str, Any]: data, not to this codebase's own trusted call sites. Normalize origin_seq to an int (rejecting bool, a fractional float, and anything int() can't convert, including an out-of-range float that would - otherwise raise OverflowError). Returns a new dict; the caller's own copy - of the raw event is left untouched.""" + otherwise raise OverflowError). Also validates origin_device_id equals + own_device_id: DESIGN.md's sync contract is "own events, gapless" - + foreign-origin data arrives as row snapshots, never as an event + (apply_replica_row already enforces this the other way, rejecting a row + that isn't foreign-owned) - so a pulled/restored event claiming a + foreign origin_device_id is a malformed hub response, not a normal case + apply_remote/mark_origin should accept. Returns a new dict; the caller's + own copy of the raw event is left untouched.""" if not isinstance(event, dict) or any(field not in event for field in _PULL_EVENT_FIELDS): raise _PullShapeError("event is missing required fields") + if event["origin_device_id"] != own_device_id: + raise _PullShapeError("event origin_device_id is not this device's own") if not isinstance(event["table"], str) or event["table"] not in OWNED_TABLES: raise _PullShapeError("event has an unknown table") if not isinstance(event["payload"], dict): @@ -1802,13 +1810,20 @@ def _check_pull_row(row: object) -> dict[str, Any]: apply_replica_row indexes directly, and that payload is an object - same reasoning as _coerce_pull_event: an unvalidated non-object payload would otherwise be stored as-is and only fail later, on every future read of - that whole table.""" + that whole table. updated_at additionally must be a non-empty string: + it's stored as-is on first insert (the row_data upsert only compares + updated_at against an existing row on conflict), so an empty value isn't + rejected until some later write to that same row fails its + ::timestamptz cast - by which point the row is already stuck with a + value no legitimate update can pass the "newer than" check against.""" if not isinstance(row, dict) or any(field not in row for field in _PULL_ROW_FIELDS): raise _PullShapeError("snapshot is missing required fields") if not isinstance(row["table"], str) or row["table"] not in OWNED_TABLES: raise _PullShapeError("snapshot has an unknown table") if not isinstance(row["payload"], dict): raise _PullShapeError("snapshot payload is not an object") + if not isinstance(row["updated_at"], str) or not row["updated_at"].strip(): + raise _PullShapeError("snapshot updated_at is not a valid timestamp") return row @@ -1831,7 +1846,7 @@ def _sync_once(store: Store) -> None: raise HubError("pull response missing events") for event in events: try: - event = _coerce_pull_event(event) + event = _coerce_pull_event(event, store.device_id()) except _PullShapeError as exc: raise HubError(f"pull {exc}") from exc # Field presence and origin_seq are validated above, but not the diff --git a/tests/test_pending.py b/tests/test_pending.py index 8aa46a1..7c5a998 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -267,9 +267,9 @@ def test_sync_once_raises_hub_error_on_non_dict_pull_response(tmp_path: Path, mo _sync_once(_paired_store(tmp_path)) -def _valid_pull_event() -> dict[str, Any]: +def _valid_pull_event(device_id: str) -> dict[str, Any]: return { - "origin_device_id": "other", + "origin_device_id": device_id, "origin_seq": 1, "table": "activity", "op": "insert", @@ -291,13 +291,33 @@ def test_sync_once_raises_hub_error_on_event_missing_one_required_field( inside store.apply_remote instead of a catchable HubError raised before that call. Parametrized per field so a future accidental narrowing of _PULL_EVENT_FIELDS to any one of them is still caught.""" - event = _valid_pull_event() + store = _paired_store(tmp_path) + event = _valid_pull_event(store.device_id()) del event[field] hub = FakeHub() hub.pull_body = {"events": [event]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) with pytest.raises(HubError, match="pull event is missing required fields"): - _sync_once(_paired_store(tmp_path)) + _sync_once(store) + + +def test_sync_once_raises_hub_error_on_event_with_foreign_origin_device_id( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: DESIGN.md's sync contract is "own events, gapless" - + foreign-origin data arrives as row snapshots, never as an event. An event + whose origin_device_id doesn't match this device's own used to pass every + check and reach apply_remote/mark_origin unchanged, letting a malformed + hub response poison this device's own ledger under a foreign device's + identity.""" + store = _paired_store(tmp_path) + event = {**_valid_pull_event(store.device_id()), "origin_device_id": "other"} + hub = FakeHub() + hub.pull_body = {"events": [event]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="event origin_device_id is not this device's own"): + _sync_once(store) + assert store.rows("activity") == [] def test_sync_once_raises_hub_error_on_event_with_non_dict_payload( @@ -312,14 +332,14 @@ def test_sync_once_raises_hub_error_on_event_with_non_dict_payload( shape either for local writes, but that's this codebase's own trusted code constructing payloads, not externally-supplied hub data - the risk this test guards against is specific to the pull path.""" - event = {**_valid_pull_event(), "payload": ["not", "an", "object"]} + store = _paired_store(tmp_path) + event = {**_valid_pull_event(store.device_id()), "payload": ["not", "an", "object"]} hub = FakeHub() hub.pull_body = {"events": [event]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) - store = _paired_store(tmp_path) with pytest.raises(HubError, match="pull event payload is not an object"): _sync_once(store) - assert store.origin_cursor("other") == 0 + assert store.origin_cursor(store.device_id()) == 0 assert store.rows("activity") == [] @@ -330,14 +350,14 @@ def test_sync_once_raises_hub_error_on_event_with_unknown_op( Store._write_in_txn rejects any op outside insert/update/delete for a local write; the hub-pull path must reject it too, not silently accept and materialize a row under an op nothing else in the codebase expects.""" - event = {**_valid_pull_event(), "op": "bogus"} + store = _paired_store(tmp_path) + event = {**_valid_pull_event(store.device_id()), "op": "bogus"} hub = FakeHub() hub.pull_body = {"events": [event]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) - store = _paired_store(tmp_path) with pytest.raises(HubError, match="pull event has an unknown op"): _sync_once(store) - assert store.origin_cursor("other") == 0 + assert store.origin_cursor(store.device_id()) == 0 assert store.rows("activity") == [] @@ -349,12 +369,13 @@ def test_sync_once_raises_hub_error_on_event_with_unknown_table( write; the hub-pull path must reject it too, instead of durably committing an orphaned row under a table name no application code ever reads back.""" - event = {**_valid_pull_event(), "table": "not_a_real_table"} + store = _paired_store(tmp_path) + event = {**_valid_pull_event(store.device_id()), "table": "not_a_real_table"} hub = FakeHub() hub.pull_body = {"events": [event]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) with pytest.raises(HubError, match="pull event has an unknown table"): - _sync_once(_paired_store(tmp_path)) + _sync_once(store) def test_sync_once_raises_hub_error_on_event_with_unhashable_table( @@ -365,12 +386,13 @@ def test_sync_once_raises_hub_error_on_event_with_unhashable_table( payload["type"] elsewhere in this PR (isinstance(typ, str) guards in store.py). A JSON-decoded list/dict for table used to raise a raw TypeError instead of a catchable HubError.""" - event = {**_valid_pull_event(), "table": ["activity"]} + store = _paired_store(tmp_path) + event = {**_valid_pull_event(store.device_id()), "table": ["activity"]} hub = FakeHub() hub.pull_body = {"events": [event]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) with pytest.raises(HubError, match="pull event has an unknown table"): - _sync_once(_paired_store(tmp_path)) + _sync_once(store) @pytest.mark.parametrize( @@ -388,23 +410,12 @@ def test_sync_once_raises_hub_error_on_non_numeric_origin_seq( """Regression test: int(event["origin_seq"]) used to run unguarded (raising a raw ValueError on garbage, OverflowError on an out-of-range float) and also silently accepted a bool or a fractional float as a valid sequence number.""" + store = _paired_store(tmp_path) hub = FakeHub() - hub.pull_body = { - "events": [ - { - "origin_device_id": "other", - "origin_seq": bad_seq, - "table": "activity", - "op": "insert", - "row_id": "x", - "payload": {}, - "occurred_at": "2026-08-13T12:00:00Z", - } - ] - } + hub.pull_body = {"events": [{**_valid_pull_event(store.device_id()), "origin_seq": bad_seq}]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) with pytest.raises(HubError, match="non-numeric origin_seq"): - _sync_once(_paired_store(tmp_path)) + _sync_once(store) def test_sync_once_accepts_an_activity_payload_whose_type_is_not_a_wake_type_shape( @@ -419,24 +430,13 @@ def test_sync_once_accepts_an_activity_payload_whose_type_is_not_a_wake_type_sha so the hub keeps re-serving the same event forever). _maybe_wake now treats a non-string type as simply "not a wake type" and lets the event apply normally instead of failing the whole transaction.""" + store = _paired_store(tmp_path) + event = {**_valid_pull_event(store.device_id()), "payload": {"type": ["not", "hashable"]}} hub = FakeHub() - hub.pull_body = { - "events": [ - { - "origin_device_id": "other", - "origin_seq": 1, - "table": "activity", - "op": "insert", - "row_id": "x", - "payload": {"type": ["not", "hashable"]}, - "occurred_at": "2026-08-13T12:00:00Z", - } - ] - } + hub.pull_body = {"events": [event]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) - store = _paired_store(tmp_path) _sync_once(store) - assert store.origin_cursor("other") == 1 + assert store.origin_cursor(store.device_id()) == 1 row = store.row("activity", "x") assert row is not None @@ -452,23 +452,13 @@ def test_sync_once_raises_hub_error_when_apply_remote_hits_an_unanticipated_shap unanticipated") still reaches the broad except-Exception net around apply_remote/mark_origin and becomes a catchable HubError instead of an uncaught crash.""" + store = _paired_store(tmp_path) + event = {**_valid_pull_event(store.device_id()), "payload": {"type": "message", "body": {1, 2, 3}}} hub = FakeHub() - hub.pull_body = { - "events": [ - { - "origin_device_id": "other", - "origin_seq": 1, - "table": "activity", - "op": "insert", - "row_id": "x", - "payload": {"type": "message", "body": {1, 2, 3}}, - "occurred_at": "2026-08-13T12:00:00Z", - } - ] - } + hub.pull_body = {"events": [event]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) with pytest.raises(HubError, match="pull event could not be applied"): - _sync_once(_paired_store(tmp_path)) + _sync_once(store) def test_sync_once_accepts_a_numeric_string_origin_seq( @@ -480,24 +470,19 @@ def test_sync_once_accepts_a_numeric_string_origin_seq( `event["origin_seq"] != last_seq + 1` is a plain Python != - "1" != 1 is always True - so a genuinely valid next sequence number raised a false "origin_seq gap" StoreError.""" - hub = FakeHub() - hub.pull_body = { - "events": [ - { - "origin_device_id": "other", - "origin_seq": "1", - "table": "task", - "op": "insert", - "row_id": "t1", - "payload": {"id": "t1"}, - "occurred_at": "2026-08-13T12:00:00Z", - } - ] + store = _paired_store(tmp_path) + event = { + **_valid_pull_event(store.device_id()), + "origin_seq": "1", + "table": "task", + "row_id": "t1", + "payload": {"id": "t1"}, } + hub = FakeHub() + hub.pull_body = {"events": [event]} monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) - store = _paired_store(tmp_path) _sync_once(store) - assert store.origin_cursor("other") == 1 + assert store.origin_cursor(store.device_id()) == 1 row = store.row("task", "t1") assert row is not None @@ -568,6 +553,43 @@ def test_sync_once_raises_hub_error_on_snapshot_with_unhashable_table( _sync_once(_paired_store(tmp_path)) +def test_sync_once_raises_hub_error_on_snapshot_with_empty_updated_at( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: updated_at was presence-checked but not validated as a + genuine timestamp. An empty string used to pass validation, get stored + as-is on first insert (the row_data upsert only compares updated_at + against an existing row on conflict, not on a fresh insert), and only + fail later - on the next legitimate update to that same row, which would + raise a raw ::timestamptz cast error trying to compare against it.""" + row = {**_valid_pull_row(), "updated_at": ""} + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": [row]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull snapshot updated_at is not a valid timestamp"): + _sync_once(_paired_store(tmp_path)) + + +def test_sync_once_rejects_whole_batch_when_one_of_two_snapshots_is_invalid( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test for the validate-all-then-apply-all split itself: the + row-side sibling of the equivalent tests/test_restore.py test. An earlier + version of this PR validated and applied snapshot rows in the same loop, + so a batch with one valid row before an invalid one would durably commit + the valid row before raising HubError on the invalid one - a partial + apply of an atomically-intended batch.""" + valid_row = {**_valid_pull_row(), "row_id": "valid-1"} + invalid_row = {**_valid_pull_row(), "row_id": "invalid-1", "table": "not_a_real_table"} + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": [valid_row, invalid_row]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + store = _paired_store(tmp_path) + with pytest.raises(HubError, match="pull snapshot has an unknown table"): + _sync_once(store) + assert store.rows("activity") == [] + + def test_sync_once_accepts_a_snapshot_payload_whose_type_is_not_a_wake_type_shape( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_restore.py b/tests/test_restore.py index 5d8b0e8..074ada7 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -45,9 +45,9 @@ def test_cmd_restore_dies_on_non_dict_body(tmp_path: Path, monkeypatch: pytest.M _run_restore(tmp_path, monkeypatch, None) -def _valid_restore_event() -> dict[str, Any]: +def _valid_restore_event(device_id: str) -> dict[str, Any]: return { - "origin_device_id": "other", + "origin_device_id": device_id, "origin_seq": 1, "table": "activity", "op": "insert", @@ -57,6 +57,15 @@ def _valid_restore_event() -> dict[str, Any]: } +def _own_device_id(tmp_path: Path) -> str: + _init_store(tmp_path) + store = open_store() + try: + return store.device_id() + finally: + store.close() + + @pytest.mark.parametrize( "field", ["origin_device_id", "origin_seq", "table", "op", "row_id", "payload", "occurred_at"], @@ -70,13 +79,33 @@ def test_cmd_restore_dies_on_event_missing_one_required_field( the equivalent tests/test_pending.py _sync_once tests) so a future accidental narrowing of _PULL_EVENT_FIELDS to any one of them is still caught, not just the "several fields missing at once" case.""" - event = _valid_restore_event() + event = _valid_restore_event(_own_device_id(tmp_path)) del event[field] body = {"own_events": [event]} with pytest.raises(SystemExit, match="restore event is missing required fields"): _run_restore(tmp_path, monkeypatch, body) +def test_cmd_restore_dies_on_event_with_foreign_origin_device_id( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: own_events replays this device's own history + (DESIGN.md: "own events, gapless" - foreign data only ever arrives as row + snapshots). An event whose origin_device_id doesn't match this device's + own used to pass every check and reach apply_remote/mark_origin + unchanged, letting a malformed restore response poison this device's own + ledger under a foreign device's identity.""" + event = {**_valid_restore_event(_own_device_id(tmp_path)), "origin_device_id": "other"} + body = {"own_events": [event]} + with pytest.raises(SystemExit, match="restore event origin_device_id is not this device's own"): + _run_restore(tmp_path, monkeypatch, body) + store = open_store() + try: + assert store.rows("activity") == [] + finally: + store.close() + + def _valid_restore_row() -> dict[str, Any]: return { "table": "activity", @@ -110,7 +139,7 @@ def test_cmd_restore_dies_on_event_with_non_dict_payload( presence was checked but not payload's type - a non-dict payload used to pass validation, get committed by apply_remote, and only fail later on every future read of that whole table.""" - event = {**_valid_restore_event(), "payload": ["not", "an", "object"]} + event = {**_valid_restore_event(_own_device_id(tmp_path)), "payload": ["not", "an", "object"]} body = {"own_events": [event]} with pytest.raises(SystemExit, match="restore event payload is not an object"): _run_restore(tmp_path, monkeypatch, body) @@ -124,7 +153,7 @@ def test_cmd_restore_dies_on_event_with_non_dict_payload( def test_cmd_restore_dies_on_event_with_unknown_op(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """Regression test: the restore-side sibling of test_sync_once_raises_hub_error_on_event_with_unknown_op.""" - event = {**_valid_restore_event(), "op": "bogus"} + event = {**_valid_restore_event(_own_device_id(tmp_path)), "op": "bogus"} body = {"own_events": [event]} with pytest.raises(SystemExit, match="restore event has an unknown op"): _run_restore(tmp_path, monkeypatch, body) @@ -140,7 +169,7 @@ def test_cmd_restore_dies_on_event_with_unknown_table( ) -> None: """Regression test: the restore-side sibling of test_sync_once_raises_hub_error_on_event_with_unknown_table.""" - event = {**_valid_restore_event(), "table": "not_a_real_table"} + event = {**_valid_restore_event(_own_device_id(tmp_path)), "table": "not_a_real_table"} body = {"own_events": [event]} with pytest.raises(SystemExit, match="restore event has an unknown table"): _run_restore(tmp_path, monkeypatch, body) @@ -151,7 +180,7 @@ def test_cmd_restore_dies_on_event_with_unhashable_table( ) -> None: """Regression test: the restore-side sibling of test_sync_once_raises_hub_error_on_event_with_unhashable_table.""" - event = {**_valid_restore_event(), "table": ["activity"]} + event = {**_valid_restore_event(_own_device_id(tmp_path)), "table": ["activity"]} body = {"own_events": [event]} with pytest.raises(SystemExit, match="restore event has an unknown table"): _run_restore(tmp_path, monkeypatch, body) @@ -190,6 +219,43 @@ def test_cmd_restore_dies_on_snapshot_with_unhashable_table( _run_restore(tmp_path, monkeypatch, body) +def test_cmd_restore_dies_on_snapshot_with_empty_updated_at( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the row-side sibling of the equivalent + tests/test_pending.py _sync_once test. updated_at was presence-checked + but not validated as a genuine timestamp; an empty string used to pass + validation, get stored as-is on first insert, and only fail later on the + next legitimate update to that same row via a raw ::timestamptz cast + error.""" + row = {**_valid_restore_row(), "updated_at": ""} + body = {"own_events": [], "inbox": [row]} + with pytest.raises(SystemExit, match="restore snapshot updated_at is not a valid timestamp"): + _run_restore(tmp_path, monkeypatch, body) + + +def test_cmd_restore_rejects_whole_batch_when_one_of_two_snapshots_is_invalid( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test for the validate-all-then-apply-all split itself: an + earlier version of this PR validated and applied snapshot rows in the + same loop, so a batch with one valid row before an invalid one would + durably commit the valid row before dying on the invalid one - a partial + apply of an atomically-intended batch. Proves the fix holds: with a + valid row after the invalid one in the list, cmd_restore must still die + without committing the valid row at all.""" + valid_row = {**_valid_restore_row(), "row_id": "valid-1"} + invalid_row = {**_valid_restore_row(), "row_id": "invalid-1", "table": "not_a_real_table"} + body = {"own_events": [], "inbox": [valid_row, invalid_row]} + with pytest.raises(SystemExit, match="restore snapshot has an unknown table"): + _run_restore(tmp_path, monkeypatch, body) + store = open_store() + try: + assert store.rows("activity") == [] + finally: + store.close() + + @pytest.mark.parametrize( "bad_seq", [ @@ -207,7 +273,7 @@ def test_cmd_restore_dies_on_non_numeric_origin_seq( shares _sync_once's origin_seq coercion (_coerce_pull_event), but had no test proving the restore side actually rejects a bad origin_seq rather than just accepting a good one.""" - event = _valid_restore_event() + event = _valid_restore_event(_own_device_id(tmp_path)) event["origin_seq"] = bad_seq body = {"own_events": [event]} with pytest.raises(SystemExit, match="restore event has a non-numeric origin_seq"): @@ -276,7 +342,7 @@ def test_cmd_restore_dies_when_apply_remote_hits_an_unanticipated_shape( body = { "own_events": [ { - "origin_device_id": "other", + "origin_device_id": _own_device_id(tmp_path), "origin_seq": 1, "table": "activity", "op": "insert", From 65f1faadad9d1eab2343a97d918bf71a27113b07 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 03:11:14 -0300 Subject: [PATCH 24/35] Actually parse a snapshot row's updated_at instead of just checking it's non-empty. A non-empty but bogus value (e.g. a garbage string) passed the previous check the same way an empty one used to, hitting the identical stuck-row failure mode: stored as-is on first insert, only breaking a later legitimate update via a raw timestamp cast error. Parse it with datetime.fromisoformat, matching the existing precedent in watch.py. Also fixes a docstring in the new mixed-batch restore test that had the row order backwards. --- src/agent_cli/main.py | 15 ++++++++++----- tests/test_pending.py | 16 ++++++++++++++++ tests/test_restore.py | 17 ++++++++++++++++- 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 1b7fad3..ba361a3 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -14,6 +14,7 @@ import uuid import webbrowser from collections.abc import Callable +from datetime import datetime from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path from typing import Any @@ -1810,10 +1811,10 @@ def _check_pull_row(row: object) -> dict[str, Any]: apply_replica_row indexes directly, and that payload is an object - same reasoning as _coerce_pull_event: an unvalidated non-object payload would otherwise be stored as-is and only fail later, on every future read of - that whole table. updated_at additionally must be a non-empty string: - it's stored as-is on first insert (the row_data upsert only compares - updated_at against an existing row on conflict), so an empty value isn't - rejected until some later write to that same row fails its + that whole table. updated_at additionally must actually parse as a + timestamp: it's stored as-is on first insert (the row_data upsert only + compares updated_at against an existing row on conflict), so a bogus + value isn't rejected until some later write to that same row fails its ::timestamptz cast - by which point the row is already stuck with a value no legitimate update can pass the "newer than" check against.""" if not isinstance(row, dict) or any(field not in row for field in _PULL_ROW_FIELDS): @@ -1822,8 +1823,12 @@ def _check_pull_row(row: object) -> dict[str, Any]: raise _PullShapeError("snapshot has an unknown table") if not isinstance(row["payload"], dict): raise _PullShapeError("snapshot payload is not an object") - if not isinstance(row["updated_at"], str) or not row["updated_at"].strip(): + if not isinstance(row["updated_at"], str): raise _PullShapeError("snapshot updated_at is not a valid timestamp") + try: + datetime.fromisoformat(row["updated_at"].replace("Z", "+00:00")) + except ValueError as exc: + raise _PullShapeError("snapshot updated_at is not a valid timestamp") from exc return row diff --git a/tests/test_pending.py b/tests/test_pending.py index 7c5a998..bf5b7e7 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -570,6 +570,22 @@ def test_sync_once_raises_hub_error_on_snapshot_with_empty_updated_at( _sync_once(_paired_store(tmp_path)) +def test_sync_once_raises_hub_error_on_snapshot_with_unparseable_updated_at( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: sibling of the empty-string test above, for a + non-empty but still bogus value. A non-blank string that isn't a real + timestamp (e.g. "not-a-timestamp") used to pass a mere non-empty check, + hitting the identical stuck-row failure mode later on the first + conflicting update.""" + row = {**_valid_pull_row(), "updated_at": "not-a-timestamp"} + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": [row]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull snapshot updated_at is not a valid timestamp"): + _sync_once(_paired_store(tmp_path)) + + def test_sync_once_rejects_whole_batch_when_one_of_two_snapshots_is_invalid( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_restore.py b/tests/test_restore.py index 074ada7..56f6890 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -234,6 +234,21 @@ def test_cmd_restore_dies_on_snapshot_with_empty_updated_at( _run_restore(tmp_path, monkeypatch, body) +def test_cmd_restore_dies_on_snapshot_with_unparseable_updated_at( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the row-side sibling of the equivalent + tests/test_pending.py _sync_once test, for a non-empty but still bogus + value. A non-blank string that isn't a real timestamp (e.g. + "not-a-timestamp") used to pass a mere non-empty check, hitting the + identical stuck-row failure mode later on the first conflicting + update.""" + row = {**_valid_restore_row(), "updated_at": "not-a-timestamp"} + body = {"own_events": [], "inbox": [row]} + with pytest.raises(SystemExit, match="restore snapshot updated_at is not a valid timestamp"): + _run_restore(tmp_path, monkeypatch, body) + + def test_cmd_restore_rejects_whole_batch_when_one_of_two_snapshots_is_invalid( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -242,7 +257,7 @@ def test_cmd_restore_rejects_whole_batch_when_one_of_two_snapshots_is_invalid( same loop, so a batch with one valid row before an invalid one would durably commit the valid row before dying on the invalid one - a partial apply of an atomically-intended batch. Proves the fix holds: with a - valid row after the invalid one in the list, cmd_restore must still die + valid row before the invalid one in the list, cmd_restore must still die without committing the valid row at all.""" valid_row = {**_valid_restore_row(), "row_id": "valid-1"} invalid_row = {**_valid_restore_row(), "row_id": "invalid-1", "table": "not_a_real_table"} From 616bf4836205469320bb40d3a678937df2ec5cd1 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 03:47:50 -0300 Subject: [PATCH 25/35] Accept a lowercase z UTC designator in a snapshot row's updated_at; fix a docstring. str.replace("Z", "+00:00") only normalized the uppercase form, but RFC 3339 permits lowercase z just as validly - a standards-conformant timestamp using it was falsely rejected as invalid. Use a case-insensitive regex instead. Also corrects a docstring that overstated apply_replica_row's same-device guard as rejecting a row, when it actually skips it silently and carves out an exception for ping rows. --- src/agent_cli/main.py | 13 +++++++------ tests/test_pending.py | 16 ++++++++++++++++ tests/test_restore.py | 17 +++++++++++++++++ 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index ba361a3..c47eabd 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1779,11 +1779,12 @@ def _coerce_pull_event(event: object, own_device_id: str) -> dict[str, Any]: otherwise raise OverflowError). Also validates origin_device_id equals own_device_id: DESIGN.md's sync contract is "own events, gapless" - foreign-origin data arrives as row snapshots, never as an event - (apply_replica_row already enforces this the other way, rejecting a row - that isn't foreign-owned) - so a pulled/restored event claiming a - foreign origin_device_id is a malformed hub response, not a normal case - apply_remote/mark_origin should accept. Returns a new dict; the caller's - own copy of the raw event is left untouched.""" + (apply_replica_row already enforces the row-side half of this, ignoring + a same-device non-ping snapshot rather than applying it) - so a + pulled/restored event claiming a foreign origin_device_id is a + malformed hub response, not a normal case apply_remote/mark_origin + should accept. Returns a new dict; the caller's own copy of the raw + event is left untouched.""" if not isinstance(event, dict) or any(field not in event for field in _PULL_EVENT_FIELDS): raise _PullShapeError("event is missing required fields") if event["origin_device_id"] != own_device_id: @@ -1826,7 +1827,7 @@ def _check_pull_row(row: object) -> dict[str, Any]: if not isinstance(row["updated_at"], str): raise _PullShapeError("snapshot updated_at is not a valid timestamp") try: - datetime.fromisoformat(row["updated_at"].replace("Z", "+00:00")) + datetime.fromisoformat(re.sub(r"[Zz]$", "+00:00", row["updated_at"])) except ValueError as exc: raise _PullShapeError("snapshot updated_at is not a valid timestamp") from exc return row diff --git a/tests/test_pending.py b/tests/test_pending.py index bf5b7e7..78f2103 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -586,6 +586,22 @@ def test_sync_once_raises_hub_error_on_snapshot_with_unparseable_updated_at( _sync_once(_paired_store(tmp_path)) +def test_sync_once_accepts_a_lowercase_z_updated_at(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Regression test: str.replace("Z", "+00:00") is case-sensitive, but + RFC 3339 (SS5.6) permits a lowercase "z" as the UTC designator just as + validly as an uppercase one. A genuinely valid, standards-conformant + timestamp ending in a lowercase "z" used to be falsely rejected as an + invalid timestamp instead of being accepted.""" + row = {**_valid_pull_row(), "updated_at": "2026-08-13T12:00:00z"} + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": [row]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + store = _paired_store(tmp_path) + _sync_once(store) + row_stored = store.row("activity", "x") + assert row_stored is not None + + def test_sync_once_rejects_whole_batch_when_one_of_two_snapshots_is_invalid( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_restore.py b/tests/test_restore.py index 56f6890..d160887 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -249,6 +249,23 @@ def test_cmd_restore_dies_on_snapshot_with_unparseable_updated_at( _run_restore(tmp_path, monkeypatch, body) +def test_cmd_restore_accepts_a_lowercase_z_updated_at(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Regression test: the row-side sibling of the equivalent + tests/test_pending.py _sync_once test. str.replace("Z", "+00:00") is + case-sensitive, but RFC 3339 (SS5.6) permits a lowercase "z" as the UTC + designator just as validly as an uppercase one. A genuinely valid, + standards-conformant timestamp ending in a lowercase "z" used to be + falsely rejected as an invalid timestamp instead of being accepted.""" + row = {**_valid_restore_row(), "updated_at": "2026-08-13T12:00:00z"} + body = {"own_events": [], "inbox": [row]} + _run_restore(tmp_path, monkeypatch, body) + store = open_store() + try: + assert store.row("activity", "x") is not None + finally: + store.close() + + def test_cmd_restore_rejects_whole_batch_when_one_of_two_snapshots_is_invalid( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: From a5589cfea8e5a6126da33d722d262b7f95b5ad75 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 04:03:05 -0300 Subject: [PATCH 26/35] Validate a pulled/restored event's occurred_at the same way a snapshot row's updated_at is. occurred_at was presence-checked only. apply_remote writes it into row_data.updated_at too (via _materialize), so a bogus value was the same unvalidated-timestamp gap already closed on the row side - just not yet on the event side. --- src/agent_cli/main.py | 11 ++++++++++- tests/test_pending.py | 20 ++++++++++++++++++++ tests/test_restore.py | 16 ++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index c47eabd..606e11d 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1776,7 +1776,10 @@ def _coerce_pull_event(event: object, own_device_id: str) -> dict[str, Any]: data, not to this codebase's own trusted call sites. Normalize origin_seq to an int (rejecting bool, a fractional float, and anything int() can't convert, including an out-of-range float that would - otherwise raise OverflowError). Also validates origin_device_id equals + otherwise raise OverflowError). occurred_at must actually parse as a + timestamp, same reasoning and check as _check_pull_row's updated_at (a + bogus value would otherwise be stored as-is - apply_remote writes it + into row_data.updated_at too, via _materialize). Also validates origin_device_id equals own_device_id: DESIGN.md's sync contract is "own events, gapless" - foreign-origin data arrives as row snapshots, never as an event (apply_replica_row already enforces the row-side half of this, ignoring @@ -1795,6 +1798,12 @@ def _coerce_pull_event(event: object, own_device_id: str) -> dict[str, Any]: raise _PullShapeError("event payload is not an object") if event["op"] not in ("insert", "update", "delete"): raise _PullShapeError("event has an unknown op") + if not isinstance(event["occurred_at"], str): + raise _PullShapeError("event occurred_at is not a valid timestamp") + try: + datetime.fromisoformat(re.sub(r"[Zz]$", "+00:00", event["occurred_at"])) + except ValueError as exc: + raise _PullShapeError("event occurred_at is not a valid timestamp") from exc raw_seq = event["origin_seq"] try: if isinstance(raw_seq, bool): diff --git a/tests/test_pending.py b/tests/test_pending.py index 78f2103..90f7ce8 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -361,6 +361,26 @@ def test_sync_once_raises_hub_error_on_event_with_unknown_op( assert store.rows("activity") == [] +def test_sync_once_raises_hub_error_on_event_with_unparseable_occurred_at( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the event-side sibling of + test_sync_once_raises_hub_error_on_snapshot_with_unparseable_updated_at. + occurred_at was presence-checked only, like updated_at used to be; a + bogus non-timestamp string used to pass validation and be stored as-is + by apply_remote (which writes it into row_data.updated_at too, via + _materialize).""" + store = _paired_store(tmp_path) + event = {**_valid_pull_event(store.device_id()), "occurred_at": "not-a-timestamp"} + hub = FakeHub() + hub.pull_body = {"events": [event]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull event occurred_at is not a valid timestamp"): + _sync_once(store) + assert store.origin_cursor(store.device_id()) == 0 + assert store.rows("activity") == [] + + def test_sync_once_raises_hub_error_on_event_with_unknown_table( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_restore.py b/tests/test_restore.py index d160887..8376d7c 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -164,6 +164,22 @@ def test_cmd_restore_dies_on_event_with_unknown_op(tmp_path: Path, monkeypatch: store.close() +def test_cmd_restore_dies_on_event_with_unparseable_occurred_at( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_on_event_with_unparseable_occurred_at.""" + event = {**_valid_restore_event(_own_device_id(tmp_path)), "occurred_at": "not-a-timestamp"} + body = {"own_events": [event]} + with pytest.raises(SystemExit, match="restore event occurred_at is not a valid timestamp"): + _run_restore(tmp_path, monkeypatch, body) + store = open_store() + try: + assert store.rows("activity") == [] + finally: + store.close() + + def test_cmd_restore_dies_on_event_with_unknown_table( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: From 24cde814f47b49742565ed9beb951d9a01953682 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 04:54:43 -0300 Subject: [PATCH 27/35] Use a real Completed(...) test double instead of a bare None-returning lambda. _knock_scan_cycle's run_argv parameter is typed Callable[[list[str]], Completed]; three new tests passed a lambda returning None instead, breaking both the declared type contract and the runner-double convention every other test in this repo already follows. --- tests/test_knock_scan_cycle.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_knock_scan_cycle.py b/tests/test_knock_scan_cycle.py index 488e0a2..5ee548d 100644 --- a/tests/test_knock_scan_cycle.py +++ b/tests/test_knock_scan_cycle.py @@ -8,6 +8,7 @@ from agent_cli import main as main_mod from agent_cli.hub import HubError from agent_cli.main import open_store +from agent_cli.runtime import Completed def _init_paired_store(tmp_path: Path) -> None: @@ -67,7 +68,7 @@ def test_knock_scan_cycle_syncs_when_paired( store = open_store() try: - main_mod._knock_scan_cycle(store, lambda _argv: None) + main_mod._knock_scan_cycle(store, lambda _argv: Completed(0, "", "")) finally: store.close() @@ -95,7 +96,7 @@ def test_knock_scan_cycle_skips_sync_when_unpaired( store = open_store() try: - main_mod._knock_scan_cycle(store, lambda _argv: None) + main_mod._knock_scan_cycle(store, lambda _argv: Completed(0, "", "")) finally: store.close() @@ -125,7 +126,7 @@ def _raise(_store: object) -> None: store = open_store() try: - main_mod._knock_scan_cycle(store, lambda _argv: None) + main_mod._knock_scan_cycle(store, lambda _argv: Completed(0, "", "")) finally: store.close() From a4e8bb0b32a52f2f1a466c2bd118265f98c44f8a Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 05:31:34 -0300 Subject: [PATCH 28/35] Catch RecursionError from a deeply nested hub response; split the event loop into validate-all-then-apply-all. A pathologically nested JSON body still overflows CPython's C-accelerated decoder's own recursion guard, raising RecursionError rather than JSONDecodeError - not covered by Hub.request's existing except tuple, letting a malformed hub response crash the caller the same way this method already prevents for other decode failures. Separately, the events loop in cmd_restore/_sync_once validated and applied each event in the same iteration, so a batch with a valid event before an invalid one durably committed the valid one before dying on the invalid one - the identical partial-apply bug the snapshots loop was already split into two passes to fix, just never mirrored on the event side. --- src/agent_cli/hub.py | 9 ++++++++- src/agent_cli/main.py | 8 ++++++-- tests/test_hub.py | 24 ++++++++++++++++++++++++ tests/test_pending.py | 28 ++++++++++++++++++++++++++++ tests/test_restore.py | 30 ++++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 3 deletions(-) diff --git a/src/agent_cli/hub.py b/src/agent_cli/hub.py index acc1e6e..f912dff 100644 --- a/src/agent_cli/hub.py +++ b/src/agent_cli/hub.py @@ -43,7 +43,14 @@ def request(self, method: str, path: str, **kwargs: Any) -> Any: if response.content: try: return response.json() - except (json.JSONDecodeError, UnicodeDecodeError, ValueError) as exc: + except (json.JSONDecodeError, UnicodeDecodeError, ValueError, RecursionError) as exc: + # RecursionError: CPython's C-accelerated json decoder still + # bounds recursion by C stack depth (Py_EnterRecursiveCall), + # not just sys.getrecursionlimit() - a pathologically nested + # body (adversarial or buggy hub) hits it well before running + # out of memory. Confirmed empirically: json.loads('[' * n + + # ']' * n) raises RecursionError around n=1_000_000, not + # JSONDecodeError, so it needs its own arm in this tuple. raise HubError(f"hub {method} {path} → invalid JSON response") from exc return None diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 606e11d..d187a82 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1504,11 +1504,13 @@ def cmd_restore(_: list[str]) -> None: events = body.get("events") if not isinstance(events, list): die("restore response missing own_events") + coerced_events: list[dict[str, Any]] = [] for event in events: try: - event = _coerce_pull_event(event, store.device_id()) + coerced_events.append(_coerce_pull_event(event, store.device_id())) except _PullShapeError as exc: die(f"restore {exc}") + for event in coerced_events: try: store.apply_remote(event, wake=False) store.mark_origin(event["origin_device_id"], event["origin_seq"]) @@ -1859,11 +1861,13 @@ def _sync_once(store: Store) -> None: events = pulled.get("events") if not isinstance(events, list): raise HubError("pull response missing events") + coerced_events: list[dict[str, Any]] = [] for event in events: try: - event = _coerce_pull_event(event, store.device_id()) + coerced_events.append(_coerce_pull_event(event, store.device_id())) except _PullShapeError as exc: raise HubError(f"pull {exc}") from exc + for event in coerced_events: # Field presence and origin_seq are validated above, but not the # shape of nested values (e.g. payload["type"]) - apply_remote/ # mark_origin can still hit a genuinely unanticipated shape deep diff --git a/tests/test_hub.py b/tests/test_hub.py index 0dd78c0..68750dc 100644 --- a/tests/test_hub.py +++ b/tests/test_hub.py @@ -19,3 +19,27 @@ def handler(_request: httpx.Request) -> httpx.Response: hub = Hub("https://hub.example", "tok", client=client) with pytest.raises(HubError, match="invalid JSON"): hub.pull({}) + + +def test_request_recursion_error_response_raises_hub_error_not_recursion_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Regression test: CPython's C-accelerated json decoder still bounds + recursion by C stack depth, not just sys.getrecursionlimit() - a + pathologically deeply-nested response body (adversarial or buggy hub) + can raise RecursionError, a RuntimeError subclass the previous except + tuple (JSONDecodeError/UnicodeDecodeError/ValueError) didn't catch. That + would have escaped every caller's HubError/StoreError guard the same way + a raw json.JSONDecodeError used to before this method existed.""" + + def handler(_request: httpx.Request) -> httpx.Response: + return httpx.Response(200, text="[1]") + + def raise_recursion_error(self: httpx.Response) -> None: + raise RecursionError("Stack overflow while decoding a JSON array") + + monkeypatch.setattr(httpx.Response, "json", raise_recursion_error) + with httpx.Client(transport=httpx.MockTransport(handler)) as client: + hub = Hub("https://hub.example", "tok", client=client) + with pytest.raises(HubError, match="invalid JSON"): + hub.pull({}) diff --git a/tests/test_pending.py b/tests/test_pending.py index 90f7ce8..0b6418f 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -642,6 +642,34 @@ def test_sync_once_rejects_whole_batch_when_one_of_two_snapshots_is_invalid( assert store.rows("activity") == [] +def test_sync_once_rejects_whole_event_batch_when_one_of_two_events_is_invalid( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the event-side sibling of + test_sync_once_rejects_whole_batch_when_one_of_two_snapshots_is_invalid. + The events loop kept validating and applying each event in the same + iteration even after the snapshots loop was split into a + validate-all-then-apply-all pattern to fix exactly this partial-apply + shape - a batch with one valid event before an invalid one used to + durably commit the valid event (and advance its origin cursor) before + raising HubError on the invalid one.""" + store = _paired_store(tmp_path) + valid_event = {**_valid_pull_event(store.device_id()), "row_id": "valid-1"} + invalid_event = { + **_valid_pull_event(store.device_id()), + "origin_seq": 2, + "row_id": "invalid-1", + "table": "not_a_real_table", + } + hub = FakeHub() + hub.pull_body = {"events": [valid_event, invalid_event]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull event has an unknown table"): + _sync_once(store) + assert store.origin_cursor(store.device_id()) == 0 + assert store.rows("activity") == [] + + def test_sync_once_accepts_a_snapshot_payload_whose_type_is_not_a_wake_type_shape( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_restore.py b/tests/test_restore.py index 8376d7c..5f32773 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -304,6 +304,36 @@ def test_cmd_restore_rejects_whole_batch_when_one_of_two_snapshots_is_invalid( store.close() +def test_cmd_restore_rejects_whole_event_batch_when_one_of_two_events_is_invalid( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the event-side sibling of + test_cmd_restore_rejects_whole_batch_when_one_of_two_snapshots_is_invalid. + cmd_restore's events loop kept validating and applying each event in the + same iteration even after its snapshots loop was split into a + validate-all-then-apply-all pattern to fix exactly this partial-apply + shape - a batch with one valid event before an invalid one used to + durably commit the valid event (and advance its origin cursor) before + dying on the invalid one.""" + device_id = _own_device_id(tmp_path) + valid_event = {**_valid_restore_event(device_id), "row_id": "valid-1"} + invalid_event = { + **_valid_restore_event(device_id), + "origin_seq": 2, + "row_id": "invalid-1", + "table": "not_a_real_table", + } + body = {"own_events": [valid_event, invalid_event]} + with pytest.raises(SystemExit, match="restore event has an unknown table"): + _run_restore(tmp_path, monkeypatch, body) + store = open_store() + try: + assert store.origin_cursor(device_id) == 0 + assert store.rows("activity") == [] + finally: + store.close() + + @pytest.mark.parametrize( "bad_seq", [ From c42deb593b2f30ed13a72486e97d03c321ce64fa Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 06:06:40 -0300 Subject: [PATCH 29/35] Close the last three gaps in hub-pull shape validation: two more RecursionError paths, row_id/origin_device_id, and restore's session ordering. Hub._detail() (used to build the error message for a >= 400 response) and the live websocket message loop each had their own separate JSON-decode call that never picked up the RecursionError fix already applied to Hub.request's success path. row_id (both events and snapshot rows) and a snapshot row's origin_device_id were the only fields in the whole pull-shape validation family still presence-checked only, unlike every sibling field - closing that gap. cmd_restore's snapshot-apply loop was missing the sessions-first ordering _sync_once already has; apply_replica_row's wake=False branch depends on the parent session row already being present to correctly attribute ownership of session mail. --- src/agent_cli/hub.py | 2 +- src/agent_cli/main.py | 28 ++++++++++++++--- tests/test_hub.py | 25 +++++++++++++++ tests/test_pending.py | 48 +++++++++++++++++++++++++++++ tests/test_restore.py | 36 ++++++++++++++++++++++ tests/test_sync_follow_reconnect.py | 35 +++++++++++++++++++++ 6 files changed, 168 insertions(+), 6 deletions(-) diff --git a/src/agent_cli/hub.py b/src/agent_cli/hub.py index f912dff..fc4ce38 100644 --- a/src/agent_cli/hub.py +++ b/src/agent_cli/hub.py @@ -116,7 +116,7 @@ def connect_sync_ws(self) -> Any: def _detail(response: httpx.Response) -> str: try: body = response.json() - except ValueError: + except (ValueError, RecursionError): return response.text if isinstance(body, dict) and "detail" in body: return str(body["detail"]) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index d187a82..d5b371c 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1454,7 +1454,7 @@ def _run_sync_ws_session( for raw in ws: try: message = json.loads(raw) - except (TypeError, ValueError): + except (TypeError, ValueError, RecursionError): continue if not isinstance(message, dict): continue @@ -1529,7 +1529,14 @@ def cmd_restore(_: list[str]) -> None: _check_pull_row(row) except _PullShapeError as exc: die(f"restore {exc}") - for row in snapshots: + # session rows first, same as _sync_once: apply_replica_row's + # wake=False branch still calls _owns_session, which depends on the + # parent session row already being present - restoring a session's + # mail before its own session row would silently skip enqueueing + # that message's wake, since ownership can't be confirmed yet. + sessions = [r for r in snapshots if r.get("table") == "session"] + rest = [r for r in snapshots if r.get("table") != "session"] + for row in sessions + rest: try: store.apply_replica_row(row, wake=False) except Exception as exc: @@ -1788,14 +1795,18 @@ def _coerce_pull_event(event: object, own_device_id: str) -> dict[str, Any]: a same-device non-ping snapshot rather than applying it) - so a pulled/restored event claiming a foreign origin_device_id is a malformed hub response, not a normal case apply_remote/mark_origin - should accept. Returns a new dict; the caller's own copy of the raw - event is left untouched.""" + should accept. row_id must be a non-empty string too, same reasoning as + every other field here - an unchecked value (e.g. a list) would only + fail later, as a raw type error from whatever stores it. Returns a new + dict; the caller's own copy of the raw event is left untouched.""" if not isinstance(event, dict) or any(field not in event for field in _PULL_EVENT_FIELDS): raise _PullShapeError("event is missing required fields") if event["origin_device_id"] != own_device_id: raise _PullShapeError("event origin_device_id is not this device's own") if not isinstance(event["table"], str) or event["table"] not in OWNED_TABLES: raise _PullShapeError("event has an unknown table") + if not isinstance(event["row_id"], str) or event["row_id"] == "": + raise _PullShapeError("event row_id is not a valid id") if not isinstance(event["payload"], dict): raise _PullShapeError("event payload is not an object") if event["op"] not in ("insert", "update", "delete"): @@ -1828,11 +1839,18 @@ def _check_pull_row(row: object) -> dict[str, Any]: compares updated_at against an existing row on conflict), so a bogus value isn't rejected until some later write to that same row fails its ::timestamptz cast - by which point the row is already stuck with a - value no legitimate update can pass the "newer than" check against.""" + value no legitimate update can pass the "newer than" check against. + row_id and origin_device_id must be non-empty strings too, same + reasoning - an unchecked value would only fail later, as a raw type + error from whatever stores it.""" if not isinstance(row, dict) or any(field not in row for field in _PULL_ROW_FIELDS): raise _PullShapeError("snapshot is missing required fields") if not isinstance(row["table"], str) or row["table"] not in OWNED_TABLES: raise _PullShapeError("snapshot has an unknown table") + if not isinstance(row["row_id"], str) or row["row_id"] == "": + raise _PullShapeError("snapshot row_id is not a valid id") + if not isinstance(row["origin_device_id"], str) or row["origin_device_id"] == "": + raise _PullShapeError("snapshot origin_device_id is not a valid id") if not isinstance(row["payload"], dict): raise _PullShapeError("snapshot payload is not an object") if not isinstance(row["updated_at"], str): diff --git a/tests/test_hub.py b/tests/test_hub.py index 68750dc..7712e98 100644 --- a/tests/test_hub.py +++ b/tests/test_hub.py @@ -43,3 +43,28 @@ def raise_recursion_error(self: httpx.Response) -> None: hub = Hub("https://hub.example", "tok", client=client) with pytest.raises(HubError, match="invalid JSON"): hub.pull({}) + + +def test_request_recursion_error_on_error_response_body_falls_back_to_text( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Regression test: the row-side sibling of the test above, for _detail() + - called only for an HTTP >= 400 response to build the error message. + _detail() had its own separate response.json() call with its own except + tuple (bare ValueError), never updated when the success-path tuple above + gained RecursionError. A >= 400 response with a pathologically nested + error body used to crash Hub.request with an uncaught RecursionError + instead of falling back to the raw response text like every other + unparseable error body already does.""" + + def handler(_request: httpx.Request) -> httpx.Response: + return httpx.Response(400, text="bad request, raw text") + + def raise_recursion_error(self: httpx.Response) -> None: + raise RecursionError("Stack overflow while decoding a JSON array") + + monkeypatch.setattr(httpx.Response, "json", raise_recursion_error) + with httpx.Client(transport=httpx.MockTransport(handler)) as client: + hub = Hub("https://hub.example", "tok", client=client) + with pytest.raises(HubError, match="bad request, raw text"): + hub.pull({}) diff --git a/tests/test_pending.py b/tests/test_pending.py index 0b6418f..3f9fd60 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -415,6 +415,23 @@ def test_sync_once_raises_hub_error_on_event_with_unhashable_table( _sync_once(store) +@pytest.mark.parametrize("bad_row_id", ["", ["not", "a", "string"]]) +def test_sync_once_raises_hub_error_on_event_with_invalid_row_id( + bad_row_id: object, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: row_id was presence-checked only, like every other + field here before it got its own validation - an empty string or a + non-string value used to pass validation untouched and only fail later, + as a raw type/constraint error from whatever eventually stores it.""" + store = _paired_store(tmp_path) + event = {**_valid_pull_event(store.device_id()), "row_id": bad_row_id} + hub = FakeHub() + hub.pull_body = {"events": [event]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull event row_id is not a valid id"): + _sync_once(store) + + @pytest.mark.parametrize( "bad_seq", [ @@ -573,6 +590,37 @@ def test_sync_once_raises_hub_error_on_snapshot_with_unhashable_table( _sync_once(_paired_store(tmp_path)) +@pytest.mark.parametrize("bad_row_id", ["", ["not", "a", "string"]]) +def test_sync_once_raises_hub_error_on_snapshot_with_invalid_row_id( + bad_row_id: object, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the row-side sibling of + test_sync_once_raises_hub_error_on_event_with_invalid_row_id.""" + row = {**_valid_pull_row(), "row_id": bad_row_id} + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": [row]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull snapshot row_id is not a valid id"): + _sync_once(_paired_store(tmp_path)) + + +@pytest.mark.parametrize("bad_origin_device_id", ["", ["not", "a", "string"]]) +def test_sync_once_raises_hub_error_on_snapshot_with_invalid_origin_device_id( + bad_origin_device_id: object, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: unlike an event's origin_device_id (checked for + ownership), a snapshot row's origin_device_id is legitimately foreign - + but it was still only presence-checked, never type/emptiness-checked, + before apply_replica_row compares it against this device's own id and + stores it as the row's recorded owner.""" + row = {**_valid_pull_row(), "origin_device_id": bad_origin_device_id} + hub = FakeHub() + hub.pull_body = {"events": [], "inbox": [row]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull snapshot origin_device_id is not a valid id"): + _sync_once(_paired_store(tmp_path)) + + def test_sync_once_raises_hub_error_on_snapshot_with_empty_updated_at( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_restore.py b/tests/test_restore.py index 5f32773..0491490 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -202,6 +202,18 @@ def test_cmd_restore_dies_on_event_with_unhashable_table( _run_restore(tmp_path, monkeypatch, body) +@pytest.mark.parametrize("bad_row_id", ["", ["not", "a", "string"]]) +def test_cmd_restore_dies_on_event_with_invalid_row_id( + bad_row_id: object, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_on_event_with_invalid_row_id.""" + event = {**_valid_restore_event(_own_device_id(tmp_path)), "row_id": bad_row_id} + body = {"own_events": [event]} + with pytest.raises(SystemExit, match="restore event row_id is not a valid id"): + _run_restore(tmp_path, monkeypatch, body) + + def test_cmd_restore_dies_on_snapshot_with_non_dict_payload( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -235,6 +247,30 @@ def test_cmd_restore_dies_on_snapshot_with_unhashable_table( _run_restore(tmp_path, monkeypatch, body) +@pytest.mark.parametrize("bad_row_id", ["", ["not", "a", "string"]]) +def test_cmd_restore_dies_on_snapshot_with_invalid_row_id( + bad_row_id: object, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_on_snapshot_with_invalid_row_id.""" + row = {**_valid_restore_row(), "row_id": bad_row_id} + body = {"own_events": [], "inbox": [row]} + with pytest.raises(SystemExit, match="restore snapshot row_id is not a valid id"): + _run_restore(tmp_path, monkeypatch, body) + + +@pytest.mark.parametrize("bad_origin_device_id", ["", ["not", "a", "string"]]) +def test_cmd_restore_dies_on_snapshot_with_invalid_origin_device_id( + bad_origin_device_id: object, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the restore-side sibling of + test_sync_once_raises_hub_error_on_snapshot_with_invalid_origin_device_id.""" + row = {**_valid_restore_row(), "origin_device_id": bad_origin_device_id} + body = {"own_events": [], "inbox": [row]} + with pytest.raises(SystemExit, match="restore snapshot origin_device_id is not a valid id"): + _run_restore(tmp_path, monkeypatch, body) + + def test_cmd_restore_dies_on_snapshot_with_empty_updated_at( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_sync_follow_reconnect.py b/tests/test_sync_follow_reconnect.py index 1422a9d..6b0d637 100644 --- a/tests/test_sync_follow_reconnect.py +++ b/tests/test_sync_follow_reconnect.py @@ -462,6 +462,41 @@ def connect_sync_ws(self) -> _SubscriptionWs: store.close() +def test_recursion_error_from_deeply_nested_frame_is_skipped_not_crashed( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: json.loads(raw) on an incoming websocket frame was + only guarded by except (TypeError, ValueError) - CPython's C-accelerated + json decoder still bounds recursion by C stack depth, not just + sys.getrecursionlimit(), so a pathologically deeply-nested frame + (adversarial or buggy hub) raises RecursionError, a RuntimeError + subclass neither of those catches - the same gap just fixed in + Hub.request, reachable a third way through the live websocket loop.""" + _init_paired_store(tmp_path) + store = open_store() + try: + nested_frame = "[" * 1_000_000 + "]" * 1_000_000 + + class _SubscriptionWs: + def send(self, data: str) -> None: + pass + + def __iter__(self): + return iter([nested_frame]) + + def close(self) -> None: + pass + + class _FakeHub: + def connect_sync_ws(self) -> _SubscriptionWs: + return _SubscriptionWs() + + with pytest.raises(HubError, match="websocket closed"): + main_mod._run_sync_ws_session(store, _FakeHub(), _FakeRuntime(), {}, {}, {}) + finally: + store.close() + + def test_subscription_row_store_connection_error_reaches_the_reconnect_loop( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: From 7dce49fb61504419511d3059fd20909b46b3e4da Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 06:25:37 -0300 Subject: [PATCH 30/35] Revert an unjustified session-ordering change in cmd_restore; make a new RecursionError test deterministic. The sessions-first sort added to cmd_restore's snapshot-apply loop was based on an incorrect premise: the "parent session" row DESIGN.md bundles with inbox mail is the sender's session (the activity's own session_id), not the recipient session apply_replica_row's wake path actually checks ownership of via _owns_session(to_session) - and that recipient session, being this device's own, can only ever arrive via own_events (which always fully precedes snapshot processing), never via the replica-row path at all regardless of ordering. Reverting rather than keeping harmless code with a false justification. Separately, the new websocket RecursionError regression test relied on a real 1,000,000-character nested JSON string actually overflowing CPython's C decoder - both interpreter/platform-dependent and, since the fake websocket only ever yields one frame, unable to distinguish "RecursionError was caught" from any other path reaching the same fallthrough. Replaced with a deterministic json.loads monkeypatch, matching the equivalent test in test_hub.py. --- src/agent_cli/main.py | 9 +-------- tests/test_sync_follow_reconnect.py | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index d5b371c..ecc3424 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1529,14 +1529,7 @@ def cmd_restore(_: list[str]) -> None: _check_pull_row(row) except _PullShapeError as exc: die(f"restore {exc}") - # session rows first, same as _sync_once: apply_replica_row's - # wake=False branch still calls _owns_session, which depends on the - # parent session row already being present - restoring a session's - # mail before its own session row would silently skip enqueueing - # that message's wake, since ownership can't be confirmed yet. - sessions = [r for r in snapshots if r.get("table") == "session"] - rest = [r for r in snapshots if r.get("table") != "session"] - for row in sessions + rest: + for row in snapshots: try: store.apply_replica_row(row, wake=False) except Exception as exc: diff --git a/tests/test_sync_follow_reconnect.py b/tests/test_sync_follow_reconnect.py index 6b0d637..6ab179a 100644 --- a/tests/test_sync_follow_reconnect.py +++ b/tests/test_sync_follow_reconnect.py @@ -471,18 +471,27 @@ def test_recursion_error_from_deeply_nested_frame_is_skipped_not_crashed( sys.getrecursionlimit(), so a pathologically deeply-nested frame (adversarial or buggy hub) raises RecursionError, a RuntimeError subclass neither of those catches - the same gap just fixed in - Hub.request, reachable a third way through the live websocket loop.""" + Hub.request, reachable a third way through the live websocket loop. + Monkeypatches json.loads directly (matching tests/test_hub.py's + equivalent test) rather than constructing a real, actually-deeply-nested + frame string: relying on CPython's C decoder genuinely overflowing at a + specific depth would make this test depend on interpreter/platform + internals rather than deterministically exercising the new except arm.""" _init_paired_store(tmp_path) store = open_store() try: - nested_frame = "[" * 1_000_000 + "]" * 1_000_000 + + def raise_recursion_error(_raw: str) -> None: + raise RecursionError("Stack overflow while decoding a JSON array") + + monkeypatch.setattr(main_mod.json, "loads", raise_recursion_error) class _SubscriptionWs: def send(self, data: str) -> None: pass def __iter__(self): - return iter([nested_frame]) + return iter(["[1]"]) def close(self) -> None: pass From 10746f5f853f1e9831b203c4918fb553ef9dfffd Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 06:36:10 -0300 Subject: [PATCH 31/35] Remove _sync_once's own sessions-first sort for the same reason its cmd_restore mirror was reverted. No comment or history anywhere justifies it, and the same mechanism that disproved the cmd_restore copy applies identically here: apply_replica_row unconditionally skips any row this device itself owns regardless of position, so a device's own session row can only ever arrive via own_events (which fully precedes snapshot processing in both functions), never via this replica-row path - independent of whether wake is True or False, and independent of which of inbox/pings/subscriptions it came from. --- src/agent_cli/main.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index ecc3424..137f20d 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1904,9 +1904,7 @@ def _sync_once(store: Store) -> None: _check_pull_row(row) except _PullShapeError as exc: raise HubError(f"pull {exc}") from exc - sessions = [r for r in snapshots if r.get("table") == "session"] - rest = [r for r in snapshots if r.get("table") != "session"] - for row in sessions + rest: + for row in snapshots: try: store.apply_replica_row(row) except Exception as exc: From 63876f1aa7af178f2e502c99ae2c3f0229128fc6 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 07:17:55 -0300 Subject: [PATCH 32/35] Match a variable-length OTel id instead of requiring the exact standard length. _OTEL_SPAN_ID required exactly 16 hex chars, leaving a malformed or non-conformant span_id of 17-19 chars unredacted - it falls between that exact match and the generic _HEX fallback's 20-char floor. The label match already does the real specificity work here, so matching whatever hex value actually follows it is both simpler and closes the gap; applied the same change to trace_id and traceparent for consistency. --- src/agent_cli/errors.py | 6 +++--- tests/test_errors.py | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/agent_cli/errors.py b/src/agent_cli/errors.py index 3a41f4d..72cf592 100644 --- a/src/agent_cli/errors.py +++ b/src/agent_cli/errors.py @@ -28,10 +28,10 @@ r'(?i)("[^"]*(?:password|secret|token|api[_-]?key|access[_-]?token|client[_-]?secret|authorization|passwd|access_key)[^"]*"\s*:\s*")[^"]*(")' ) _HEX = re.compile(r"\b[a-fA-F0-9]{20,}\b") -_OTEL_TRACE_ID = re.compile(r'(?i)(\btrace_id(?:\\?["\'])?\s*[:=]\s*(?:\\?["\'])?)[0-9a-fA-F]{32}\b') -_OTEL_SPAN_ID = re.compile(r'(?i)(\bspan_id(?:\\?["\'])?\s*[:=]\s*(?:\\?["\'])?)[0-9a-fA-F]{16}\b') +_OTEL_TRACE_ID = re.compile(r'(?i)(\btrace_id(?:\\?["\'])?\s*[:=]\s*(?:\\?["\'])?)[0-9a-fA-F]+\b') +_OTEL_SPAN_ID = re.compile(r'(?i)(\bspan_id(?:\\?["\'])?\s*[:=]\s*(?:\\?["\'])?)[0-9a-fA-F]+\b') _OTEL_TRACEPARENT = re.compile( - r'(?i)(\btraceparent(?:\\?["\'])?\s*[:=]\s*(?:\\?["\'])?)[0-9a-fA-F]{2}-[0-9a-fA-F]{32}-[0-9a-fA-F]{16}-[0-9a-fA-F]{2}\b' + r'(?i)(\btraceparent(?:\\?["\'])?\s*[:=]\s*(?:\\?["\'])?)[0-9a-fA-F]+-[0-9a-fA-F]+-[0-9a-fA-F]+-[0-9a-fA-F]+\b' ) _SECRET = re.compile( r"(?i)(? None: assert '\\"traceparent\\":\\"[redacted]\\"' in traceparent_escaped_json +def test_redact_strips_otel_ids_of_non_standard_length() -> None: + """Regression test: _OTEL_TRACE_ID/_OTEL_SPAN_ID required an exact + 32/16-char hex value. A malformed or non-conformant id (e.g. a span_id + of 17-19 hex chars) falls between that exact match and the generic + _HEX fallback's 20-char floor, so it used to pass through unredacted - + undermining the "always stripped before hashing" dedup guarantee for + exactly the malformed values most likely to vary occurrence to + occurrence. The label match already does the real specificity work, so + matching the value's actual length instead of requiring the standard + one only helps.""" + odd_span_id = "b" * 18 + otel = redact(f"TimeoutError boom trace_id={'a' * 32} span_id={odd_span_id}") + assert odd_span_id not in otel + assert "span_id=[redacted]" in otel + short_trace_id = "a" * 10 + otel_short_trace = redact(f"TimeoutError boom trace_id={short_trace_id} span_id={'b' * 16}") + assert short_trace_id not in otel_short_trace + assert "trace_id=[redacted]" in otel_short_trace + + def test_scan_inserts_once_then_enriches(tmp_path: Path) -> None: store = Store(tmp_path) _runner_session(store) From 706ebebb4ca57178a6904dfcf28b407efa892423 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 07:28:11 -0300 Subject: [PATCH 33/35] Document the per-cycle hub sync in DESIGN.md's CLI-surface entry for agent knock. README.md and DESIGN.md's own daemon-installation paragraph already mention it; the CLI-surface catalog entry was the one place still missing it. --- DESIGN.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DESIGN.md b/DESIGN.md index 4aef3fa..6151928 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -412,7 +412,7 @@ agent sync [--follow] agent restore agent ping send|list|ack agent daemon [--install|--uninstall] # always-on supervisor; init installs the user service -agent knock [--once] # --once drains; without --once is foreground; user service is the supported always-on path +agent knock [--once] # --once drains; without --once is foreground, syncing (push + pull) with the hub after each cycle once paired; user service is the supported always-on path agent watch pr-merged # one scan; device daemon covers the loop agent watch pending # one scan; LISTEN agent_work / execute subscription.set and query.request agent watch grok-usage # one scan; knock child (under the device daemon) polls every 60s From 8bff95302b39ddd9ba8966d2ec2723346900add3 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 08:28:14 -0300 Subject: [PATCH 34/35] Validate every event and snapshot before applying either, in both cmd_restore and _sync_once. Events were fully validated and applied - a complete phase including the store writes and origin-cursor advance - before snapshot validation even began. A malformed snapshot paired with well-formed events in the same response let those events durably commit before dying on the snapshot - the same partial-apply bug already fixed twice (once within the events list, once within the snapshots list), recurring one level up between the two lists themselves. Also drops an unused monkeypatch parameter from two existing websocket subscription tests. --- src/agent_cli/main.py | 46 +++++++++++++++++------------ tests/test_pending.py | 23 +++++++++++++++ tests/test_restore.py | 24 +++++++++++++++ tests/test_sync_follow_reconnect.py | 8 ++--- 4 files changed, 76 insertions(+), 25 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 137f20d..f367814 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1510,12 +1510,6 @@ def cmd_restore(_: list[str]) -> None: coerced_events.append(_coerce_pull_event(event, store.device_id())) except _PullShapeError as exc: die(f"restore {exc}") - for event in coerced_events: - try: - store.apply_remote(event, wake=False) - store.mark_origin(event["origin_device_id"], event["origin_seq"]) - except Exception as exc: - die(f"restore event could not be applied: {exc}") snapshots: list[dict[str, Any]] = [] for key in ("inbox", "pings"): value = body.get(key) @@ -1529,6 +1523,16 @@ def cmd_restore(_: list[str]) -> None: _check_pull_row(row) except _PullShapeError as exc: die(f"restore {exc}") + # Every event and snapshot is validated above before either is + # applied here - a malformed snapshot must not be discovered only + # after well-formed events ahead of it in the response are already + # durably committed and their origin cursor advanced. + for event in coerced_events: + try: + store.apply_remote(event, wake=False) + store.mark_origin(event["origin_device_id"], event["origin_seq"]) + except Exception as exc: + die(f"restore event could not be applied: {exc}") for row in snapshots: try: store.apply_replica_row(row, wake=False) @@ -1878,19 +1882,6 @@ def _sync_once(store: Store) -> None: coerced_events.append(_coerce_pull_event(event, store.device_id())) except _PullShapeError as exc: raise HubError(f"pull {exc}") from exc - for event in coerced_events: - # Field presence and origin_seq are validated above, but not the - # shape of nested values (e.g. payload["type"]) - apply_remote/ - # mark_origin can still hit a genuinely unanticipated shape deep - # inside store.py. Convert any such failure to a HubError rather - # than let it crash whichever loop called _sync_once; HubError/ - # StoreError themselves pass through unchanged (SystemExit is not - # an Exception subclass). - try: - store.apply_remote(event) - store.mark_origin(event["origin_device_id"], event["origin_seq"]) - except Exception as exc: - raise HubError(f"pull event could not be applied: {exc}") from exc snapshots: list[dict[str, Any]] = [] for key in ("inbox", "pings", "subscriptions"): value = pulled.get(key) @@ -1904,6 +1895,23 @@ def _sync_once(store: Store) -> None: _check_pull_row(row) except _PullShapeError as exc: raise HubError(f"pull {exc}") from exc + # Every event and snapshot is validated above before either is + # applied here - a malformed snapshot must not be discovered only + # after well-formed events ahead of it in the response are already + # durably committed and their origin cursor advanced. + for event in coerced_events: + # Field presence and origin_seq are validated above, but not the + # shape of nested values (e.g. payload["type"]) - apply_remote/ + # mark_origin can still hit a genuinely unanticipated shape deep + # inside store.py. Convert any such failure to a HubError rather + # than let it crash whichever loop called _sync_once; HubError/ + # StoreError themselves pass through unchanged (SystemExit is not + # an Exception subclass). + try: + store.apply_remote(event) + store.mark_origin(event["origin_device_id"], event["origin_seq"]) + except Exception as exc: + raise HubError(f"pull event could not be applied: {exc}") from exc for row in snapshots: try: store.apply_replica_row(row) diff --git a/tests/test_pending.py b/tests/test_pending.py index 3f9fd60..755f598 100644 --- a/tests/test_pending.py +++ b/tests/test_pending.py @@ -718,6 +718,29 @@ def test_sync_once_rejects_whole_event_batch_when_one_of_two_events_is_invalid( assert store.rows("activity") == [] +def test_sync_once_rejects_whole_response_when_a_valid_event_is_paired_with_an_invalid_snapshot( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: events were fully validated AND applied (a + complete, separate phase, including the store write and cursor advance) + before snapshot validation even began - so a well-formed event paired + with a malformed snapshot in the same pull response used to durably + commit the event before dying on the snapshot, the identical + partial-apply bug this PR already fixed twice, once within the events + list and once within the snapshots list, just one level up between the + two lists themselves.""" + store = _paired_store(tmp_path) + valid_event = {**_valid_pull_event(store.device_id())} + invalid_row = {**_valid_pull_row(), "table": "not_a_real_table"} + hub = FakeHub() + hub.pull_body = {"events": [valid_event], "inbox": [invalid_row]} + monkeypatch.setattr("agent_cli.main._hub_from_store", lambda _s: hub) + with pytest.raises(HubError, match="pull snapshot has an unknown table"): + _sync_once(store) + assert store.origin_cursor(store.device_id()) == 0 + assert store.rows("activity") == [] + + def test_sync_once_accepts_a_snapshot_payload_whose_type_is_not_a_wake_type_shape( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_restore.py b/tests/test_restore.py index 0491490..ed7f08c 100644 --- a/tests/test_restore.py +++ b/tests/test_restore.py @@ -370,6 +370,30 @@ def test_cmd_restore_rejects_whole_event_batch_when_one_of_two_events_is_invalid store.close() +def test_cmd_restore_rejects_whole_response_when_a_valid_event_is_paired_with_an_invalid_snapshot( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression test: the restore-side sibling of + test_sync_once_rejects_whole_response_when_a_valid_event_is_paired_with_an_invalid_snapshot. + cmd_restore's events were fully validated AND applied (a complete, + separate phase, including the store write and cursor advance) before + snapshot validation even began - so a well-formed event paired with a + malformed snapshot in the same restore response used to durably commit + the event before dying on the snapshot.""" + device_id = _own_device_id(tmp_path) + valid_event = _valid_restore_event(device_id) + invalid_row = {**_valid_restore_row(), "table": "not_a_real_table"} + body = {"own_events": [valid_event], "inbox": [invalid_row]} + with pytest.raises(SystemExit, match="restore snapshot has an unknown table"): + _run_restore(tmp_path, monkeypatch, body) + store = open_store() + try: + assert store.origin_cursor(device_id) == 0 + assert store.rows("activity") == [] + finally: + store.close() + + @pytest.mark.parametrize( "bad_seq", [ diff --git a/tests/test_sync_follow_reconnect.py b/tests/test_sync_follow_reconnect.py index 6ab179a..af85612 100644 --- a/tests/test_sync_follow_reconnect.py +++ b/tests/test_sync_follow_reconnect.py @@ -378,9 +378,7 @@ def fake_sync_once(store: object) -> None: assert calls == [1], "a genuine data-integrity StoreError must not be retried" -def test_subscription_row_with_non_dict_payload_is_skipped_not_committed( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: +def test_subscription_row_with_non_dict_payload_is_skipped_not_committed(tmp_path: Path) -> None: """Regression test: incoming websocket subscription rows went straight to store.apply_replica_row(row) guarded only by isinstance(row, dict) and row.get("table") - never through _check_pull_row. A non-dict payload @@ -421,9 +419,7 @@ def connect_sync_ws(self) -> _SubscriptionWs: store.close() -def test_subscription_row_with_unhashable_table_is_skipped_not_crashed( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: +def test_subscription_row_with_unhashable_table_is_skipped_not_crashed(tmp_path: Path) -> None: """Regression test: `table not in OWNED_TABLES` inside _check_pull_row requires table to be hashable. A JSON-decoded list/dict for table used to raise a raw TypeError that neither the _PullShapeError catch around From 409d3dda7bd4f268d36e4b5788b5445fe6da7864 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Wed, 2 Sep 2026 08:52:43 -0300 Subject: [PATCH 35/35] Correct a comment that overclaimed full validate-before-apply coverage. The round-11 fix's comment said "every event and snapshot is validated above before either is applied" - true only for shape validation. Semantic checks (an origin_seq gap, a foreign row-ownership conflict) can only be evaluated against live store state at apply time, inside each item's own transaction, so a shape-valid batch can still partially commit before a later semantic conflict is discovered. Scoped the comment accurately instead of overclaiming; full atomicity would need one transaction spanning the whole apply loop, tracked separately as a bigger change than a validation fix. --- src/agent_cli/main.py | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index f367814..54c7b2f 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1523,10 +1523,17 @@ def cmd_restore(_: list[str]) -> None: _check_pull_row(row) except _PullShapeError as exc: die(f"restore {exc}") - # Every event and snapshot is validated above before either is - # applied here - a malformed snapshot must not be discovered only + # Every event and snapshot's SHAPE is validated above before either + # is applied here - a malformed snapshot must not be discovered only # after well-formed events ahead of it in the response are already - # durably committed and their origin cursor advanced. + # durably committed and their origin cursor advanced. This does not + # cover semantic conflicts (an origin_seq gap, a foreign row- + # ownership conflict): those can only be checked against live store + # state at apply time, inside each item's own transaction, so a + # batch that's shape-valid throughout can still partially commit + # before a later semantic conflict is discovered. Closing that would + # need one transaction spanning the whole apply loop, a bigger + # change than this fix - tracked separately. for event in coerced_events: try: store.apply_remote(event, wake=False) @@ -1895,10 +1902,17 @@ def _sync_once(store: Store) -> None: _check_pull_row(row) except _PullShapeError as exc: raise HubError(f"pull {exc}") from exc - # Every event and snapshot is validated above before either is - # applied here - a malformed snapshot must not be discovered only + # Every event and snapshot's SHAPE is validated above before either + # is applied here - a malformed snapshot must not be discovered only # after well-formed events ahead of it in the response are already - # durably committed and their origin cursor advanced. + # durably committed and their origin cursor advanced. This does not + # cover semantic conflicts (an origin_seq gap, a foreign row- + # ownership conflict): those can only be checked against live store + # state at apply time, inside each item's own transaction, so a + # batch that's shape-valid throughout can still partially commit + # before a later semantic conflict is discovered. Closing that would + # need one transaction spanning the whole apply loop, a bigger + # change than this fix - tracked separately. for event in coerced_events: # Field presence and origin_seq are validated above, but not the # shape of nested values (e.g. payload["type"]) - apply_remote/