diff --git a/.agents/skills/lib_py/herdr_config.py b/.agents/skills/lib_py/herdr_config.py index e1012bb..ed54918 100644 --- a/.agents/skills/lib_py/herdr_config.py +++ b/.agents/skills/lib_py/herdr_config.py @@ -137,6 +137,18 @@ def dumps(doc: dict) -> str: return "\n".join(lines) + "\n" +def _same_doc(a, b) -> bool: + """Equality of two parsed TOML documents in which nan equals nan (a valid + TOML value that a plain == never matches, review R3).""" + if isinstance(a, float) and isinstance(b, float) and math.isnan(a) and math.isnan(b): + return True + if isinstance(a, dict) and isinstance(b, dict): + return a.keys() == b.keys() and all(_same_doc(a[k], b[k]) for k in a) + if isinstance(a, list) and isinstance(b, list): + return len(a) == len(b) and all(_same_doc(x, y) for x, y in zip(a, b)) + return type(a) is type(b) and a == b + + def server_config_text(user_text: Optional[str]) -> Tuple[str, List[str]]: """(the TOML body for the server, warnings). The body always parses, has session.resume_agents_on_restore = false and otherwise equals the source - @@ -156,7 +168,7 @@ def server_config_text(user_text: Optional[str]) -> Tuple[str, List[str]]: expected = dict(user, session=dict(session, **{KEY: False})) try: text = dumps(expected) - ok = tomllib.loads(text) == expected + ok = _same_doc(tomllib.loads(text), expected) except (TypeError, tomllib.TOMLDecodeError): ok = False if not ok: @@ -233,7 +245,16 @@ def _write_checked(target: str, source: str, body: str, mode: int, herdr: Option try: why = herdr_rejects(target, herdr) except (OSError, subprocess.SubprocessError) as e: - warnings.append(f"cannot run `{herdr} config check` ({e}); the config is unchecked") + # Not a pass (review R1): an unchecked merge with a schema error + # makes herdr drop the whole file - restore back on. The merged + # body is not used unchecked; MINIMAL is a fixed file herdr 0.9.1 + # accepts (test_real_herdr_accepts_every_result), and returning no + # config would leave restore on, so it is used with a warning. + if candidate != MINIMAL: + warnings.append(f"cannot run `{herdr} config check` ({e}); not using an unchecked merge; " + f"{LOSES_SETTINGS}") + continue + warnings.append(f"cannot run `{herdr} config check` ({e}); using the fixed minimal config unchecked") return True if why is None: return True diff --git a/.agents/skills/lib_py/herdr_restore.py b/.agents/skills/lib_py/herdr_restore.py index 07b86e6..c1a5f23 100644 --- a/.agents/skills/lib_py/herdr_restore.py +++ b/.agents/skills/lib_py/herdr_restore.py @@ -43,8 +43,14 @@ def _herdr(herdr: str, session: str, *args: str) -> dict: def _shell_in_foreground(herdr: str, session: str, pane_id: str) -> bool: info = _herdr(herdr, session, "pane", "process-info", "--pane", pane_id).get("process_info") or {} - shell = info.get("shell_pid") - return shell is not None and info.get("foreground_process_group_id") == shell + shell, fg = info.get("shell_pid"), info.get("foreground_process_group_id") + # Only a real pid proves anything (review R2): None, 0, -1, False, "" or a + # string must leave the pane alone, however they compare. + return _is_pid(shell) and _is_pid(fg) and fg == shell + + +def _is_pid(v) -> bool: + return isinstance(v, int) and not isinstance(v, bool) and v > 0 def close_restored_shells(session_id: str, herdr: str = "herdr", diff --git a/tests/test_issue45_herdr_config.py b/tests/test_issue45_herdr_config.py index c5d51f6..904a8cc 100644 --- a/tests/test_issue45_herdr_config.py +++ b/tests/test_issue45_herdr_config.py @@ -380,3 +380,41 @@ def test_a_foreign_file_with_the_output_name_is_kept(tmp_path): other_path, _ = hc.write_server_config({"HERDR_CONFIG_PATH": str(other)}) assert other_path not in (path, str(foreign)) assert hc.user_config_path({"HERDR_CONFIG_PATH": path}) == str(source) + + +# --- review round 2 (codex R1, R3) ------------------------------------------- + +@needs_tomllib +@pytest.mark.parametrize("exc", [subprocess.TimeoutExpired(["herdr", "config", "check"], 30), + OSError("herdr went away")], ids=["timeout", "oserror"]) +def test_an_unrunnable_check_never_hands_over_an_unchecked_merge(tmp_path, monkeypatch, exc): + """R1: a check that cannot run is not a pass. A merge with a schema error + would make herdr drop the whole file (restore back on), so the result is + the fixed minimal config, with a warning - never the unchecked merge.""" + source = tmp_path / "config.toml" + source.write_text('[ui]\nconfirm_close = "invalid-bool"\n') + + def boom(path, herdr): + raise exc + monkeypatch.setattr(hc, "herdr_rejects", boom) + path, warnings = hc.write_server_config({"HERDR_CONFIG_PATH": str(source)}, herdr="/fake/herdr") + assert path is not None + text = Path(path).read_text() + assert "invalid-bool" not in text and "confirm_close" not in text, text + assert tomllib.loads(text) == {"session": {"resume_agents_on_restore": False}} + assert any("not using an unchecked merge" in w for w in warnings), warnings + assert source.read_text() == '[ui]\nconfirm_close = "invalid-bool"\n' + + +@needs_tomllib +@pytest.mark.parametrize("nan_part", ["[custom]\nvalue = nan\n", "[custom]\nvalues = [1.0, nan, -nan]\n", + "[custom.deep]\nt = { a = nan }\n"], ids=["top", "array", "nested"]) +def test_a_nan_value_does_not_drop_the_other_settings(nan_part): + """R3: nan is valid TOML and never == itself; the re-parse check must treat + it as equal, or the user's other settings are thrown away.""" + body, warnings = hc.server_config_text("[ui]\nconfirm_close = false\n" + nan_part) + assert warnings == [], warnings + parsed = tomllib.loads(body) + assert parsed["ui"]["confirm_close"] is False + assert parsed["session"]["resume_agents_on_restore"] is False + assert "custom" in parsed diff --git a/tests/test_issue45_restore_unit.py b/tests/test_issue45_restore_unit.py new file mode 100644 index 0000000..7a7d407 --- /dev/null +++ b/tests/test_issue45_restore_unit.py @@ -0,0 +1,56 @@ +"""Issue #45 (B), review round 2 R2: resume closes a restored pane only when +herdr's answer proves the shell is in the foreground. Odd pid values - missing, +null, 0, negative, bool, string - must leave the pane alone. herdr is a stub +here; the real-server behaviour is tests/test_issue45_restore.py.""" +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / ".agents" / "skills")) +from lib_py import herdr_restore as hr # noqa: E402 + +SID = "probe-id" + + +def _stub(monkeypatch, process_info): + calls = [] + + def fake(herdr, session, *args): + calls.append(args) + if args[:2] == ("pane", "list"): + return {"panes": [{"pane_id": "probe-pane", "agent": None, "agent_session": {"value": SID}}]} + if args[:2] == ("pane", "process-info"): + return {"process_info": process_info} + return {} + monkeypatch.setattr(hr, "_herdr", fake) + return calls + + +@pytest.mark.parametrize("shell,fg", [(0, 0), (False, False), ("", ""), (-1, -1), (None, None), + ("123", "123"), (True, True), (1.0, 1.0)], + ids=["zero", "false", "empty", "negative", "null", "string", "true", "float"]) +def test_an_odd_pid_answer_leaves_the_pane_alone(monkeypatch, shell, fg): + calls = _stub(monkeypatch, {"shell_pid": shell, "foreground_process_group_id": fg}) + assert hr.close_restored_shells(SID, session="m45-review-probe") == [] + assert ("pane", "close", "probe-pane") not in calls + + +def test_a_missing_process_info_leaves_the_pane_alone(monkeypatch): + calls = _stub(monkeypatch, {}) + assert hr.close_restored_shells(SID, session="m45-review-probe") == [] + assert ("pane", "close", "probe-pane") not in calls + + +def test_a_real_shell_in_the_foreground_is_closed(monkeypatch): + calls = _stub(monkeypatch, {"shell_pid": 4242, "foreground_process_group_id": 4242}) + assert hr.close_restored_shells(SID, session="m45-review-probe") == ["probe-pane"] + assert ("pane", "close", "probe-pane") in calls + + +def test_a_busy_pane_is_left_alone(monkeypatch): + calls = _stub(monkeypatch, {"shell_pid": 4242, "foreground_process_group_id": 4300}) + assert hr.close_restored_shells(SID, session="m45-review-probe") == [] + assert ("pane", "close", "probe-pane") not in calls