fix(issue-45): review round 2 - an unrunnable config check is not a pass, odd pids never close a pane, nan does not drop settings

codex round 2 (PR #48).

- R1: when `herdr config check` could not run (timeout, OSError), the
  merged config was handed to the server as checked. A merge with a schema
  error makes herdr drop the whole file, restore back on - the #45 failure.
  The merged body is no longer used unchecked: _write_checked falls to the
  fixed MINIMAL config with a warning. When the check cannot run on MINIMAL
  either, MINIMAL is still used (with its own warning) rather than no
  config: returning none would leave restore on, and MINIMAL is a fixed
  two-line file the real-herdr test accepts. Unknown non-zero exits stay as
  they were - herdr exits non-zero for harmless warnings (an unknown
  section) that do not drop the file.
- R2: _shell_in_foreground compared shell_pid and the foreground group with
  ==, so 0/0, False/False, ""/"" or -1/-1 closed the pane. Both must now be
  positive ints (bool excluded) before they are compared.
- R3: the re-parse check used ==, and nan != nan, so a valid TOML nan threw
  away every other user setting even on Python 3.11. _same_doc compares
  documents recursively with nan equal to nan.

Tests: R1 (timeout, OSError) and R3 (top-level, array, nested nan) in
test_issue45_herdr_config.py; R2 in the new tests/test_issue45_restore_unit.py
(a herdr stub: zero, false, empty, negative, null, string, true, float;
missing info; a real shell closed; a busy pane kept). On the previous code
12 of them fail. Python 3.11: 67 passed; Python 3.9: 33 passed / 34 skipped
(tomllib-only). Real-herdr restore tests and related files on 3.9: 197
passed / 1 skipped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff

.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

.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",

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

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