fix(issue-45): review R1-R3 - herdr checks the config, any TOML spelling kept, source never overwritten

R1: tomllib accepting a file is not herdr accepting it (a schema error makes
herdr drop the whole file, restore included). The shim passes REAL_HERDR and
the written file must pass `herdr config check` ("parse error"/"using defaults"
reject; unknown keys and sound-file notices do not), else the key-only file is
written and checked. A non-table `session` and a source that is not UTF-8 reach
that fallback instead of raising.

R2: the line edit is replaced by parse -> set -> serialize; the result must
parse back to the source plus the key, so dotted keys and inline tables keep
the user's other settings.

R3: the output is never the source (aliases included) nor a file not generated
for the same source; then a per-source name beside it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

tests/test_issue45_herdr_config.py

@@ -30,20 +30,32 @@ CASES = [
     ("missing", None, True, False),
     ("empty", "", True, False),
     ("broken", "[session]\n[session]\n", False, True),
+    ("session_not_a_table", "session = 1\n[ui]\nconfirm_close = false\n", False, True),
     ("no_session", "[ui]\nconfirm_close = false\n", True, False),
     ("no_session_no_newline", "[ui]\nconfirm_close = false", True, False),
     ("session_without_key", "[session]\n# a comment\n\n[ui]\nconfirm_close = false\n", True, False),
     ("key_true", "[ui]\nconfirm_close = false\n[session]\nresume_agents_on_restore = true  # mine\n", True, False),
     ("key_false", "[session]\nresume_agents_on_restore=false\n", True, False),
     ("sub_table_only", "[session.extra]\na = 1\n", True, False),
-    ("dotted_key", "session.resume_agents_on_restore = true\n", False, True),
-    ("inline_table", "session = { resume_agents_on_restore = true }\n", False, True),
-    ("header_inside_string", 'banner = """\n[session]\n"""\n', False, True),
+    # valid spellings a line edit cannot reach: the rest must survive (review R2)
+    ("dotted_key", "session.resume_agents_on_restore = true\n[ui]\nconfirm_close = false\n", True, False),
+    ("inline_table", "session = { resume_agents_on_restore = true }\n[ui]\nconfirm_close = false\n", True, False),
+    ("header_inside_string", 'banner = """\n[session]\n"""\n[ui]\nconfirm_close = false\n', True, False),
+    ("value_kinds", 'top = "a\\"b\\u007f\\u00e9"\n"odd key" = [1, 2.5, inf, true]\n'
+                    'when = 1979-05-27T07:32:00Z\n[ui.sound]\npath = "s/done.wav"\n'
+                    '[[keys.bind]]\nkey = "a"\n[[keys.bind]]\nkey = "b"\n', True, False),
 ]
 
 
 def _without_key(d):
-    return hc._without_key(d)
+    d = dict(d)
+    session = dict(d.get("session") or {})
+    session.pop("resume_agents_on_restore", None)
+    if session:
+        d["session"] = session
+    else:
+        d.pop("session", None)
+    return d
 
 
 @pytest.mark.parametrize("name,user,kept,warned", CASES, ids=[c[0] for c in CASES])
@@ -58,17 +70,10 @@ def test_merge_always_parses_with_restore_off(name, user, kept, warned):
         assert text == hc.MINIMAL
 
 
-def test_existing_false_is_left_byte_for_byte():
-    user = "[session]\nresume_agents_on_restore=false # keep\n"
-    assert hc.server_config_text(user) == (user, [])
-
-
-def test_a_bad_value_type_falls_back_to_minimal():
-    # herdr rejects a non-bool here as a parse error of the whole file.
+def test_a_non_bool_restore_value_is_replaced():
+    # herdr would reject the whole file for this; the key is ours to set anyway.
     text, warnings = hc.server_config_text('[session]\nresume_agents_on_restore = "no"\n')
-    assert tomllib.loads(text)["session"]["resume_agents_on_restore"] is False
-    # tomllib reads the string fine, so the merge replaces the line instead.
-    assert warnings == []
+    assert tomllib.loads(text) == {"session": {"resume_agents_on_restore": False}} and warnings == []
 
 
 def test_without_tomllib_restore_is_still_off(monkeypatch):
@@ -187,14 +192,101 @@ def test_cli_prints_the_path(tmp_path):
 @pytest.mark.skipif(not HERDR, reason="herdr is not installed")
 @pytest.mark.parametrize("name,user,kept,warned", CASES, ids=[c[0] for c in CASES])
 def test_real_herdr_accepts_every_result(tmp_path, name, user, kept, warned):
+    if kept and user:  # a source herdr itself takes must stay one it takes
+        src = tmp_path / "source.toml"
+        src.write_text(user)
+        assert hc.herdr_rejects(str(src), HERDR) is None
     text, _ = hc.server_config_text(user)
     cfg = tmp_path / "server.toml"
     cfg.write_text(hc.MARKER + "\n" + text)
-    env = {k: v for k, v in os.environ.items() if not k.startswith("HERDR_")}
-    env.update(HOME=str(tmp_path), HERDR_CONFIG_PATH=str(cfg))
-    res = subprocess.run([HERDR, "config", "check"], capture_output=True, text=True, env=env)
-    out = res.stdout + res.stderr
-    assert "parse error" not in out, out
-    # the fixtures use only keys herdr knows, apart from the made-up ones below
-    issues = [l for l in out.splitlines()[1:] if l.strip()]
-    assert all(l.startswith("unknown config key") and ("extra" in l or "banner" in l) for l in issues), out
+    assert hc.herdr_rejects(str(cfg), HERDR) is None
+
+
+@pytest.mark.skipif(not HERDR, reason="herdr is not installed")
+def test_herdr_rejects_tells_schema_errors_from_harmless_issues(tmp_path):
+    bad = tmp_path / "bad.toml"
+    bad.write_text('[ui]\nconfirm_close = "no"\n')
+    assert "using defaults" in hc.herdr_rejects(str(bad), HERDR)
+    soft = tmp_path / "soft.toml"
+    soft.write_text('made_up = 1\n[ui.sound]\npath = "x.wav"\n')
+    assert hc.herdr_rejects(str(soft), HERDR) is None
+
+
+def _cli(tmp_path, source: Path, *extra) -> subprocess.CompletedProcess:
+    env = {k: v for k, v in os.environ.items() if not k.startswith("HERDR_") and k != "XDG_CONFIG_HOME"}
+    env.update(HOME=str(tmp_path), HERDR_CONFIG_PATH=str(source),
+               PYTHONPATH=str(REPO_ROOT / ".agents" / "skills"))
+    return subprocess.run([sys.executable, "-m", "lib_py.herdr_config", "server-config", *extra],
+                          capture_output=True, text=True, env=env)
+
+
+@pytest.mark.skipif(not HERDR, reason="herdr is not installed")
+@pytest.mark.parametrize("body,kept", [
+    (b"[ui]\nconfirm_close = false\n", True),             # control: valid, kept
+    (b'[ui]\nconfirm_close = "no"\n', False),             # tomllib yes, herdr's schema no (review R1)
+    (b"session = 1\n[ui]\nconfirm_close = false\n", False),  # non-table session (R1)
+    (b"[ui]\nconfirm_close = false\n# \xff\xfe\n", False),   # not UTF-8 (R1)
+], ids=["valid", "schema-error", "session-scalar", "invalid-utf8"])
+def test_cli_always_hands_herdr_a_restore_off_config_it_accepts(tmp_path, body, kept):
+    source = tmp_path / "custom.toml"
+    source.write_bytes(body)
+    res = _cli(tmp_path, source, "--herdr", HERDR)
+    assert res.returncode == 0, res.stderr
+    out = Path(res.stdout.strip())
+    assert out.parent == tmp_path and out != source
+    assert hc.herdr_rejects(str(out), HERDR) is None
+    parsed = tomllib.loads(out.read_text())
+    assert parsed["session"]["resume_agents_on_restore"] is False
+    if kept:
+        assert parsed["ui"]["confirm_close"] is False and res.stderr == ""
+    else:
+        assert parsed == {"session": {"resume_agents_on_restore": False}} and "WARNING" in res.stderr
+    assert source.read_bytes() == body
+
+
+@pytest.mark.skipif(not HERDR, reason="herdr is not installed")
+def test_a_config_herdr_rejects_is_never_left_in_place(tmp_path):
+    # the merge itself is fine for tomllib; only herdr's schema says no
+    source = tmp_path / "config.toml"
+    source.write_text('[ui]\nconfirm_close = "no"\n[session]\nresume_agents_on_restore = true\n')
+    path, warnings = hc.write_server_config({"HERDR_CONFIG_PATH": str(source)}, herdr=HERDR)
+    assert tomllib.loads(Path(path).read_text()) == {"session": {"resume_agents_on_restore": False}}
+    assert any("herdr rejects" in w for w in warnings), warnings
+
+
+# --- the user's file is never the output (review R3) -----------------------
+
+def test_source_named_like_the_output_is_not_overwritten(tmp_path):
+    source = tmp_path / hc.OUTPUT_NAME
+    source.write_bytes(b"[ui]\nconfirm_close = false\n")
+    path, _ = hc.write_server_config({"HERDR_CONFIG_PATH": str(source)})
+    assert source.read_bytes() == b"[ui]\nconfirm_close = false\n"
+    assert Path(path).parent == tmp_path and Path(path) != source
+    assert tomllib.loads(Path(path).read_text())["ui"]["confirm_close"] is False
+    # and the next start finds the same file again
+    assert hc.write_server_config({"HERDR_CONFIG_PATH": str(source)})[0] == path
+
+
+def test_an_alias_of_the_source_is_not_overwritten(tmp_path):
+    source = tmp_path / "config.toml"
+    source.write_bytes(b"[ui]\nconfirm_close = false\n")
+    (tmp_path / hc.OUTPUT_NAME).symlink_to(source)
+    path, _ = hc.write_server_config({"HERDR_CONFIG_PATH": str(source)})
+    assert source.read_bytes() == b"[ui]\nconfirm_close = false\n"
+    assert os.path.islink(tmp_path / hc.OUTPUT_NAME)
+    assert not os.path.samefile(path, source)
+
+
+def test_a_foreign_file_with_the_output_name_is_kept(tmp_path):
+    source = tmp_path / "config.toml"
+    source.write_text("[ui]\nconfirm_close = false\n")
+    foreign = tmp_path / hc.OUTPUT_NAME
+    foreign.write_bytes(b"# someone else's\n")
+    path, _ = hc.write_server_config({"HERDR_CONFIG_PATH": str(source)})
+    assert foreign.read_bytes() == b"# someone else's\n" and Path(path) != foreign
+    # a file generated for another source is someone else's too
+    other = tmp_path / "other.toml"
+    other.write_text("")
+    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)