fix(update): one receipt entry per repair outcome; reason no longer printed twice
Follow-up to the two salvaged commits (#111419 @Ckarey007, #111521 @wangtaotaotao95), which both edit `_stage_candidate_venv` and were merged keep-both: - `_record_runtime_repair` (new topical helper next to `_run_runtime_repair`): a `failed`/`safe`/`repaired` repair is one `sqlite_runtime_repair` step; a `skipped`/`not-applicable` repair is one skip WITH its reason instead of a red step plus a skip. Every pip/non-venv install runs the hook and would otherwise carry a failed-looking step in every receipt. - The sync rejection's reason now travels into `RuntimeRepairResult.detail`, which `_report_runtime_repair_failure` prints and the receipt records, so the extra console print in `_stage_candidate_venv` is dropped (it duplicated the ℹ line). - Tests trimmed to invariants: the child-reason test asserts the reason on the rejection the repair returns (the exception carries it now); the receipt test covers failed step / repaired step / skipped skip and the no-receipt no-op. Dropped: the 4-case stage-rejection walk (the same chain is covered by the existing `_repair_under_lock` tests whose rejection now carries the reason).
This commit is contained in:
teknium1
+26
-19
@@ -197,21 +197,32 @@ def _uv_version(uv_bin: str) -> str:
|
||||
).stdout.strip()
|
||||
|
||||
|
||||
def _record_runtime_repair(repair: RuntimeRepairResult) -> None:
|
||||
"""Put the repair outcome into the update receipt (no-op outside ``hermes update``).
|
||||
|
||||
Receipts are built only from explicit ``record_step``/``record_skip`` calls, so without this
|
||||
a failed repair left ``outcome: partial`` with no step naming the reason or the SQLite
|
||||
versions. A deferred or not-applicable repair is a skip WITH its reason, not a failed step:
|
||||
every pip/non-venv install would otherwise carry a red step in every receipt.
|
||||
"""
|
||||
from hermes_cli.update_receipt import record_skip, record_step
|
||||
|
||||
detail = (
|
||||
f"{repair.status}: {repair.detail}" if repair.detail else repair.status
|
||||
) + f" (sqlite {repair.sqlite_before or 'unknown'} → {repair.sqlite_after or 'unknown'})"
|
||||
if repair.status in {"skipped", "not-applicable"}:
|
||||
record_skip("sqlite_runtime_repair", detail)
|
||||
else:
|
||||
record_step("sqlite_runtime_repair", repair.status in {"safe", "repaired"}, detail)
|
||||
|
||||
|
||||
def _run_runtime_repair(
|
||||
uv_bin: str, repair_observer: Callable[[RuntimeRepairResult], None] | None,
|
||||
*, print_skip: bool = False) -> None:
|
||||
"""Run the vulnerable-runtime repair hook; never raises (repair is non-fatal)."""
|
||||
try:
|
||||
repair = repair_vulnerable_runtime(uv_bin)
|
||||
from hermes_cli.update_receipt import record_skip, record_step
|
||||
|
||||
detail = (
|
||||
f"{repair.status}: {repair.detail} "
|
||||
f"(sqlite {repair.sqlite_before or 'unknown'} → {repair.sqlite_after or 'unknown'})"
|
||||
)
|
||||
record_step("sqlite_runtime_repair", repair.status in {"safe", "repaired"}, detail)
|
||||
if repair.status in {"skipped", "not-applicable"}:
|
||||
record_skip("sqlite_runtime_repair", detail)
|
||||
_record_runtime_repair(repair)
|
||||
if repair_observer is not None:
|
||||
repair_observer(repair)
|
||||
if repair.status == "failed":
|
||||
@@ -573,7 +584,7 @@ def _smoke_candidate_venv(venv_dir: Path) -> tuple[bool, str, SQLiteRuntimeInfo
|
||||
|
||||
|
||||
# A failed ``uv sync`` prints its diagnosis last, so the tail is the actionable part. Kept
|
||||
# short: the reason travels into a one-line log entry and a one-line console warning.
|
||||
# short: the reason travels into a one-line log entry, the failure report and the receipt step.
|
||||
_SYNC_TAIL_LINES = 6
|
||||
_SYNC_REASON_CHARS = 600
|
||||
|
||||
@@ -601,11 +612,9 @@ def _stream_sync(argv: list[str], *, cwd: Path, env: dict[str, str]) -> tuple[in
|
||||
child's stdout while it runs, so a full stderr pipe blocks uv forever — stderr is merged
|
||||
into stdout and forwarded line by line instead of being captured and reprinted at the end.
|
||||
|
||||
The tail is kept anyway. Inherited stdout lands the child's diagnosis in console scrollback
|
||||
ONLY: the rejection line the logger records carried the bare exit code, and the generic
|
||||
"did not pass dependency and import smoke tests" detail the repair returns carries no reason
|
||||
either (receipts quote explicitly recorded steps — none records this repair). Seen in the
|
||||
field, that reads as "hermes update says the SQLite repair failed and never says why".
|
||||
The tail is kept anyway: with inherited stdout the child's diagnosis survived in console
|
||||
scrollback only, and the rejection carried a bare exit code — "hermes update says the SQLite
|
||||
repair failed and never says why".
|
||||
"""
|
||||
proc = subprocess.Popen(
|
||||
list(argv), cwd=cwd, env=env, stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
|
||||
@@ -666,10 +675,8 @@ def _stage_candidate_venv(
|
||||
[uv_bin, "sync", "--extra", "all", "--locked", "--python", str(_venv_python(candidate))],
|
||||
cwd=project_root, env=sync_env)
|
||||
if status != 0:
|
||||
# `_repair_under_lock`'s failure detail is generic ("did not pass dependency and import
|
||||
# smoke tests"), so the reason is announced here; without it only console scrollback has
|
||||
# the text — the rejection line the log records carries the bare exit code.
|
||||
print(f" ⚠ candidate dependency sync failed (rc={status}): {reason}")
|
||||
# The reason travels with the rejection into RuntimeRepairResult.detail, which the
|
||||
# failure report prints and the update receipt records.
|
||||
return reject("candidate dependency sync failed (rc=%d): %s", status, reason)
|
||||
healthy, detail, _ = _smoke_candidate_venv(candidate)
|
||||
if not healthy:
|
||||
|
||||
@@ -667,8 +667,11 @@ class TestStageCandidateVenvCrossPlatform:
|
||||
assert "UV_NO_CONFIG" not in sync_kwargs["env"]
|
||||
assert sync_kwargs["stderr"] == subprocess.STDOUT
|
||||
|
||||
def test_sync_failure_reports_the_child_reason(self, tmp_path, capsys, caplog):
|
||||
"""A rejected candidate must say WHY — the child's own diagnosis, not a bare rc."""
|
||||
def test_sync_failure_reports_the_child_reason(self, tmp_path, caplog):
|
||||
"""A rejected candidate must say WHY — the child's own diagnosis, not a bare rc.
|
||||
|
||||
The reason rides the rejection into ``RuntimeRepairResult.detail`` (#111417, #111497).
|
||||
"""
|
||||
import logging
|
||||
|
||||
from hermes_cli.managed_uv import _stage_candidate_venv
|
||||
@@ -690,25 +693,25 @@ class TestStageCandidateVenvCrossPlatform:
|
||||
patch(
|
||||
"hermes_cli.managed_uv._smoke_candidate_venv",
|
||||
return_value=(True, "", None),
|
||||
):
|
||||
candidate = _stage_candidate_venv(
|
||||
), \
|
||||
pytest.raises(_CandidateStageError) as rejected:
|
||||
_stage_candidate_venv(
|
||||
"uv",
|
||||
project_root=root,
|
||||
generation=generation,
|
||||
python=python,
|
||||
)
|
||||
|
||||
assert candidate is None
|
||||
console = capsys.readouterr().out
|
||||
# The reason is the child's own diagnosis: the error + hint, without the progress noise
|
||||
# that precedes them. It reaches the console AND the rejection the updater logs.
|
||||
reason_line = next(
|
||||
line for line in console.splitlines() if "dependency sync failed" in line)
|
||||
assert "error: The lockfile at `uv.lock` needs to be updated" in reason_line
|
||||
assert "hint: To update the lockfile, run `uv lock`." in reason_line
|
||||
assert "Resolving despite existing lockfile" not in reason_line
|
||||
# that precedes them. It reaches the rejection the repair returns AND the log.
|
||||
reason = str(rejected.value)
|
||||
assert reason.startswith("candidate dependency sync failed (rc=1): ")
|
||||
assert "error: The lockfile at `uv.lock` needs to be updated" in reason
|
||||
assert "hint: To update the lockfile, run `uv lock`." in reason
|
||||
assert "Resolving despite existing lockfile" not in reason
|
||||
assert "candidate dependency sync failed (rc=1)" in caplog.text
|
||||
assert "needs to be updated" in caplog.text
|
||||
assert not list((root / ".hermes-runtime").glob("venv-candidate-*"))
|
||||
|
||||
|
||||
class TestRuntimeCutover:
|
||||
|
||||
@@ -1,7 +1,10 @@
|
||||
"""Runtime repair diagnostics must reach persisted update receipts (#111497)."""
|
||||
"""The SQLite runtime repair outcome must reach the persisted update receipt (#111497).
|
||||
|
||||
``record_step``/``record_skip`` are the only way into a receipt; before this a failed repair left
|
||||
``outcome: partial`` with no step naming the reason or the SQLite versions involved.
|
||||
"""
|
||||
|
||||
import json
|
||||
from types import SimpleNamespace
|
||||
|
||||
import pytest
|
||||
|
||||
@@ -9,76 +12,37 @@ from hermes_cli import managed_uv as uv
|
||||
from hermes_cli import update_receipt as receipts
|
||||
|
||||
|
||||
@pytest.mark.parametrize("status", ["safe", "repaired", "failed", "skipped", "not-applicable"])
|
||||
@pytest.mark.parametrize("entry", ["update", "bootstrap"])
|
||||
def test_runtime_result_reaches_persisted_receipt(tmp_path, monkeypatch, status, entry):
|
||||
@pytest.mark.parametrize("status, ok, bucket", [
|
||||
("failed", False, "steps"),
|
||||
("repaired", True, "steps"),
|
||||
("skipped", None, "skips"),
|
||||
])
|
||||
def test_runtime_repair_outcome_reaches_persisted_receipt(tmp_path, monkeypatch, status, ok, bucket):
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
monkeypatch.setattr(receipts, "_current", None)
|
||||
result = uv.RuntimeRepairResult(status, "repair diagnostic", "3.50.4", "3.53.1")
|
||||
result = uv.RuntimeRepairResult(
|
||||
status, "candidate dependency sync failed (rc=1): error: lockfile stale", "3.50.4", "3.53.1")
|
||||
monkeypatch.setattr(uv, "repair_vulnerable_runtime", lambda _: result)
|
||||
if entry == "update":
|
||||
monkeypatch.setattr(uv, "resolve_uv", lambda: "uv")
|
||||
monkeypatch.setattr(uv, "_uv_self_update_is_fresh", lambda: True)
|
||||
invoke = uv.update_managed_uv
|
||||
else:
|
||||
paths = iter([None, "uv"])
|
||||
monkeypatch.setattr(uv, "resolve_uv", lambda: next(paths))
|
||||
monkeypatch.setattr(uv, "_install_uv", lambda _: None)
|
||||
monkeypatch.setattr(uv, "_uv_version", lambda _: "test")
|
||||
invoke = uv.ensure_uv
|
||||
monkeypatch.setattr(uv, "resolve_uv", lambda: "uv")
|
||||
monkeypatch.setattr(uv, "_uv_self_update_is_fresh", lambda: True)
|
||||
observed = []
|
||||
|
||||
receipts.begin_update_receipt()
|
||||
invoke(repair_observer=observed.append)
|
||||
path = receipts.finalize_update_receipt("partial")
|
||||
data = json.loads(path.read_text())
|
||||
step, = data["steps"]
|
||||
assert step["name"] == "sqlite_runtime_repair"
|
||||
assert step["ok"] == (status in {"safe", "repaired"})
|
||||
assert all(value in step["detail"] for value in (
|
||||
result.status, result.detail, result.sqlite_before, result.sqlite_after))
|
||||
assert bool(data["skips"]) == (status in {"skipped", "not-applicable"})
|
||||
uv.update_managed_uv(repair_observer=observed.append)
|
||||
data = json.loads(receipts.finalize_update_receipt("partial").read_text(encoding="utf-8"))
|
||||
|
||||
assert observed == [result]
|
||||
# The same repair hook is also used outside an update, without an active receipt.
|
||||
entry, = data[bucket]
|
||||
other = "skips" if bucket == "steps" else "steps"
|
||||
assert data[other] == []
|
||||
assert entry["name"] == "sqlite_runtime_repair"
|
||||
text = entry["detail"] if bucket == "steps" else entry["reason"]
|
||||
if bucket == "steps":
|
||||
assert entry["ok"] is ok
|
||||
assert text.startswith(f"{status}: ")
|
||||
assert result.detail in text
|
||||
assert "sqlite 3.50.4 → 3.53.1" in text
|
||||
# Outside `hermes update` (setup/bootstrap) there is no receipt; the hook must stay a no-op.
|
||||
uv._run_runtime_repair("uv", observed.append)
|
||||
assert receipts._current is None
|
||||
assert observed == [result, result]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("stage, reason", [
|
||||
("create", "candidate venv creation failed (rc=1): permission denied"),
|
||||
("lock", "candidate dependency sync refused: uv.lock is missing"),
|
||||
("sync", "candidate dependency sync failed (rc=1)"),
|
||||
("smoke", "candidate venv smoke failed: missing module"),
|
||||
])
|
||||
def test_stage_rejection_preserves_reason_and_live_environment(tmp_path, monkeypatch, stage, reason):
|
||||
root = tmp_path / "checkout"
|
||||
live = root / "venv"
|
||||
live.mkdir(parents=True)
|
||||
sentinel = live / "sentinel"
|
||||
sentinel.write_text("unchanged")
|
||||
generation = root / ".hermes-runtime" / "python" / "generation"
|
||||
generation.mkdir(parents=True)
|
||||
if stage != "lock":
|
||||
(root / "uv.lock").write_text("lock")
|
||||
current = SimpleNamespace(wal_reset_vulnerable=True, sqlite_version_string="3.50.4")
|
||||
fixed = SimpleNamespace(sqlite_version_string="3.53.1")
|
||||
monkeypatch.setattr(uv, "probe_sqlite_runtime", lambda _: current)
|
||||
monkeypatch.setattr(uv, "_install_safe_python_generation", lambda *a, **kw: (
|
||||
generation, generation / "python", fixed))
|
||||
|
||||
def run(argv, **kwargs):
|
||||
if argv[1] == "venv":
|
||||
from pathlib import Path
|
||||
Path(argv[2]).mkdir(parents=True)
|
||||
failed = (argv[1] == "venv" and stage == "create") or (argv[1] == "sync" and stage == "sync")
|
||||
return SimpleNamespace(returncode=int(failed), stderr="permission denied", stdout="")
|
||||
|
||||
monkeypatch.setattr(uv.subprocess, "run", run)
|
||||
monkeypatch.setattr(uv, "_smoke_candidate_venv", lambda _: (False, "missing module", None))
|
||||
result = uv._repair_under_lock("uv", root=root, live=live, live_python=live / "python",
|
||||
runtime_root=root / ".hermes-runtime")
|
||||
assert result.status == "failed"
|
||||
assert result.detail == reason
|
||||
assert sentinel.read_text() == "unchanged"
|
||||
assert not generation.exists()
|
||||
assert not list((root / ".hermes-runtime").glob("venv-candidate-*"))
|
||||
|
||||
Reference in New Issue
Block a user