From 2dbdbdb539bbebcc24c82924dee3cb0efdb0e709 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 5 Oct 2026 23:37:01 -0700 Subject: [PATCH] fix: one lookup reads a run's archived receipt, with one error contract The run's own archive was read five ways: _read_terminal_receipt, _running_record and amend_terminal_followup (each globbing by update_id with different error handling), plus the _persist_running stale-snapshot guard and finalize_interrupted_update_receipt (each re-reading the _run_file path inline). update_receipt.read_run_record is now the only reader: unreadable, non-object and other-run files all mean "no record", it never raises, and each caller keeps its outcome on top of that. The divergence was a bug in one place: _read_terminal_receipt and _running_record called .get() on whatever json.loads returned, so a run archive holding valid JSON that is not an object (null, a list) raised AttributeError out of settle_lost_completion -- a post-commit path that must never fail the update. It now settles like a missing archive. --- hermes_cli/update_completion.py | 28 ++++------ hermes_cli/update_receipt.py | 53 ++++++++++++------- .../test_update_completion_process.py | 3 ++ .../test_update_receipt_run_record.py | 28 ++++++++++ 4 files changed, 73 insertions(+), 39 deletions(-) create mode 100644 tests/hermes_cli/test_update_receipt_run_record.py diff --git a/hermes_cli/update_completion.py b/hermes_cli/update_completion.py index b4631bdad0..44af0eb469 100644 --- a/hermes_cli/update_completion.py +++ b/hermes_cli/update_completion.py @@ -135,33 +135,23 @@ def _resume_receipt(data: dict) -> None: def _read_terminal_receipt(request: dict) -> dict | None: from hermes_cli import update_receipt - # The ROOT home's receipts (the run's own file), never latest.json: another - # profile/context may have finalized more recently. update_id = request["receipt"]["update_id"] - try: - for path in update_receipt._receipt_dir().glob(f"update_*_{update_id}.json"): - data = json.loads(path.read_text(encoding="utf-8-sig")) - if data.get("update_id") == update_id and data.get("finished_at"): - return data - except (OSError, ValueError): - pass + found = update_receipt.read_run_record(update_id) + if found is not None and found[1].get("finished_at"): + return found[1] # The store refused the terminal write after the commit point (loudly): this process still # holds the correlated terminal record it finalized, so the exit status never turns into 1. return update_receipt.finalized_receipt(update_id) def _running_record(request: dict) -> dict | None: - from hermes_cli.update_receipt import _receipt_dir + from hermes_cli.update_receipt import read_run_record - for path in _receipt_dir().glob(f"update_*_{request['receipt']['update_id']}.json"): - try: - data = json.loads(path.read_text(encoding="utf-8-sig")) - except (OSError, ValueError): - continue - if data.get("update_id") == request["receipt"]["update_id"] and data.get("outcome") == "running": - data.pop("writer_pid", None) - return data - return None + found = read_run_record(request["receipt"]["update_id"]) + if found is None or found[1].get("outcome") != "running": + return None + found[1].pop("writer_pid", None) + return found[1] def _owed_user_action(request: dict) -> str | None: diff --git a/hermes_cli/update_receipt.py b/hermes_cli/update_receipt.py index f107a8ff8a..0a70ea23d7 100644 --- a/hermes_cli/update_receipt.py +++ b/hermes_cli/update_receipt.py @@ -261,6 +261,23 @@ def _run_file(directory: Path, data: dict[str, Any]) -> Path: return directory / f"update_{stamp[:8]}_{stamp[8:]}_{data.get('pid') or os.getpid()}_{data.get('update_id')}.json" +def read_run_record(update_id: str) -> Optional[tuple[Path, dict[str, Any]]]: + """The run's own archive ``(path, record)`` in the root store, or None. Never raises. + + The ONE lookup of a run's archive (named at begin, so its running and terminal records share + one file); never ``latest.json``, which another profile or run may have replaced since. A file + that cannot be read, is not a JSON object, or names another run is not this run's record: + callers treat all of those exactly like a missing archive. + """ + with suppress(OSError): + for path in _receipt_dir().glob(f"update_*_{update_id}.json"): + with suppress(OSError, ValueError): + record = json.loads(path.read_text(encoding="utf-8-sig")) + if isinstance(record, dict) and record.get("update_id") == update_id: + return path, record + return None + + def _persist_running(data: dict[str, Any]) -> None: """Write the open run to disk: a killed update still leaves its own record. Never raises.""" with suppress(Exception): @@ -271,10 +288,9 @@ def _persist_running(data: dict[str, Any]) -> None: path = _run_file(directory, data) # A completion child can finalize while its parent still holds the pre-child # context. That stale running snapshot must never undo the terminal receipt. - with suppress(OSError, ValueError): - stored = json.loads(path.read_text(encoding="utf-8-sig")) - if stored.get("update_id") == data.get("update_id") and stored.get("finished_at"): - return + stored = read_run_record(str(data.get("update_id"))) + if stored is not None and stored[1].get("finished_at"): + return from hermes_cli.process_identity import _process_create_time payload = (json.dumps({**data, "writer_pid": os.getpid(), @@ -428,19 +444,18 @@ def record_user_action(step: str, reason: str) -> None: def amend_terminal_followup(update_id: str, step: str, reason: str) -> None: """A follow-up that failed after the run's receipt was finalized lands on that receipt. Never raises.""" - with suppress(Exception): + found = read_run_record(update_id) + if found is None: + return + path, record = found + record.setdefault("followups", []).append({"step": step, "reason": reason, "at": _utc_now_iso()}) + payload = (json.dumps(record, indent=2, default=str) + "\n").encode("utf-8") + with suppress(Exception): # an unwritable store: the printed follow-up line still names it from hermes_cli.runtime_state import _atomic_bytes - directory = _receipt_dir() - for path in directory.glob(f"update_*_{update_id}.json"): - record = json.loads(path.read_text(encoding="utf-8-sig")) - if record.get("update_id") != update_id: - continue - record.setdefault("followups", []).append({"step": step, "reason": reason, "at": _utc_now_iso()}) - payload = (json.dumps(record, indent=2, default=str) + "\n").encode("utf-8") - _atomic_bytes(path, payload) - if (read_latest_receipt() or {}).get("update_id") == update_id: - _write_latest(payload) + _atomic_bytes(path, payload) + if (read_latest_receipt() or {}).get("update_id") == update_id: + _write_latest(payload) def owe_followup(update_id: Optional[str], step: str, reason: str, **retry: str) -> None: @@ -710,11 +725,9 @@ def finalize_interrupted_update_receipt(stop_reason: str, *, exit_code: int = 13 with suppress(Exception): clone = copy.copy(current) clone.data = copy.deepcopy(current.data) - on_disk: dict = {} - with suppress(Exception): - path = _run_file(_receipt_dir(), current.data) - on_disk = json.loads(path.read_text(encoding="utf-8-sig")) - if on_disk.get("update_id") == current.data.get("update_id"): + found = read_run_record(str(current.data.get("update_id"))) + if found is not None: + path, on_disk = found if on_disk.get("finished_at"): # the completion child already closed the run _current.reset(current.current_token) return path diff --git a/tests/hermes_cli/test_update_completion_process.py b/tests/hermes_cli/test_update_completion_process.py index 9b3fb3dd0f..7648225b80 100644 --- a/tests/hermes_cli/test_update_completion_process.py +++ b/tests/hermes_cli/test_update_completion_process.py @@ -168,6 +168,9 @@ def transition(tmp_path): "def record_user_action(*args): pass\n" "def _receipt_dir():\n" " return pathlib.Path(os.environ['HERMES_HOME']) / 'logs/update_receipts'\n" + "def read_run_record(update_id):\n" + " for path in _receipt_dir().glob(f'update_*_{update_id}.json'):\n" + " return path, json.loads(path.read_text(encoding='utf-8-sig'))\n" "def finalize_pending_update_receipt(code, reason):\n" " r = _current.get()\n" " if r is None: return\n" diff --git a/tests/hermes_cli/test_update_receipt_run_record.py b/tests/hermes_cli/test_update_receipt_run_record.py new file mode 100644 index 0000000000..0db8d58532 --- /dev/null +++ b/tests/hermes_cli/test_update_receipt_run_record.py @@ -0,0 +1,28 @@ +"""A run's own archive is read through one lookup: an unreadable record is a missing record.""" +from __future__ import annotations + +import pytest + +from hermes_cli import update_completion, update_receipt + + +def _lost_run(tmp_path): + update_receipt.begin_update_receipt() + run = dict(update_receipt._current.get().data) + request = {"source": str(tmp_path / "checkout"), "receipt": run, "desktop": False, "gateway_mode": False} + return run, update_receipt._run_file(update_receipt._receipt_dir(), run), request + + +@pytest.mark.parametrize("content", ["null", "[]", "{torn"]) +def test_lost_completion_over_a_malformed_run_archive_still_closes_the_run(_isolate_hermes_home, tmp_path, content): + # The code is committed: an archive that is valid JSON but not a record (hand edit, foreign + # writer) must settle exactly like a missing one, never raise out of the post-commit path. + run, archive, request = _lost_run(tmp_path) + archive.write_text(content, encoding="utf-8") + + result = update_completion.settle_lost_completion(request, "the completion process exited -9") + + assert result["exit_code"] == 0 + assert result["receipt"]["update_id"] == run["update_id"] + assert result["receipt"]["outcome"] == "success" + assert [row["step"] for row in result["receipt"]["followups"]] == ["completion"]