From 35935f9e8a052ca9e9d9b806a62d17eb72afea78 Mon Sep 17 00:00:00 2001 From: Godopu Date: Sun, 4 Oct 2026 11:49:08 +0000 Subject: [PATCH] fix(issue-45): review round 2 R3 - the .mam fallback obeys the same no-overwrite rules The fallback wrote <.mam>/herdr-server-config.toml unchecked, so it could replace a foreign file there, the source itself, or a link to it. It now goes through output_path like the normal path (source identity incl. aliases, generated-for-this-source ownership, then a per-source name) and writes nothing when no name is safe. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/lib_py/herdr_config.py | 23 +++++---- VERSIONS.md | 2 +- tests/test_issue45_herdr_config.py | 68 +++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 11 deletions(-) diff --git a/.agents/skills/lib_py/herdr_config.py b/.agents/skills/lib_py/herdr_config.py index dc5469c..e1012bb 100644 --- a/.agents/skills/lib_py/herdr_config.py +++ b/.agents/skills/lib_py/herdr_config.py @@ -14,9 +14,10 @@ to false. The user's file is never modified. from the server) is followed back to the source recorded in that file. * Output: `mam-server-config.toml` next to the source, so relative paths in it (herdr resolves them against the config file's directory) keep their - meaning. Never the source itself and never a file this module did not - write for the same source: then a name with a hash of the source path. - `<.mam>/herdr-server-config.toml` only when the directory is not writable. + meaning. `<.mam>/herdr-server-config.toml` only when that directory is not + writable. Either way never the source itself and never a file this module + did not write for the same source: then a name with a hash of the source + path, and no write at all when that is taken too. * The source is parsed, the key set, and the result serialized; the result must parse back to exactly the source plus the key (any TOML spelling of the key - header, dotted, inline - works). Then herdr's own `config check` @@ -188,19 +189,20 @@ def _same_file(a: str, b: str) -> bool: return os.path.realpath(a) == os.path.realpath(b) -def output_path(source: str) -> str: - """Beside the source: OUTPUT_NAME, unless that is the source itself or a - file this module did not write for this source; then a per-source name.""" - d = os.path.dirname(source) +def output_path(source: str, directory: Optional[str] = None, name: str = OUTPUT_NAME) -> str: + """`name` in `directory` (default: beside the source), unless that is the + source itself (aliases included) or a file this module did not write for + this source; then a per-source name. Every write goes through here.""" + d = directory or os.path.dirname(source) tag = hashlib.sha256(os.path.realpath(source).encode()).hexdigest()[:12] - for name in (OUTPUT_NAME, f"mam-server-config-{tag}.toml"): - target = os.path.join(d, name) + for n in (name, f"{name[:-len('.toml')]}-{tag}.toml"): + target = os.path.join(d, n) if _same_file(target, source): continue if os.path.lexists(target) and _generated_source(target) not in ("", source): continue return target - raise OSError(f"no free output name beside {source}") + raise OSError(f"no free output name in {d}") def _write_atomic(path: str, text: str, mode: int) -> None: @@ -268,6 +270,7 @@ def write_server_config(env: Mapping[str, str], fallback_dir: Optional[str] = No if fallback_dir: target = os.path.join(fallback_dir, FALLBACK_NAME) try: + target = output_path(source, fallback_dir, FALLBACK_NAME) if _write_checked(target, source, body, mode, herdr, warnings): warnings.append(f"wrote {target} instead; relative paths in the herdr config may not resolve") return target, warnings diff --git a/VERSIONS.md b/VERSIONS.md index 863e1f8..aedaf49 100644 --- a/VERSIONS.md +++ b/VERSIONS.md @@ -55,7 +55,7 @@ - **실서버 테스트 (`tests/test_issue45_restore.py`, herdr 0.9.1)**: 짧은 일회용 HOME·세션과 가짜 claude 로, MAM bootstrap 경로에서 복원이 꺼짐(사용자 설정에 `[session]` 표가 있을 때 포함)과 같은 레이아웃을 일반 서버로 띄우면 `claude --resume` 이 뜨는 양성 대조, 닫기 조건, create → crash → resume 뒤 pane 하나만 남음을 확인한다. 수정을 끄면 A·B 테스트가 각각 실패함을 확인했다. - **단위 (`tests/test_issue45_herdr_config.py`)**: 합치기 12 가지 경우와 실제 `herdr config check`, 원본 경로 규칙, 쓰기·폴백. - **관련 스위트**: 새 두 파일 40 passed. issue9·shim contract·issue42·issue24 resume·uuid_target·issue21 prereqs·issue23_24 199 passed / 1 skipped(S11 기준 이동 후). tier1~4·issue21_phase2_loop_a 148 passed. 전체 스위트는 돌리지 않았다. -- **리뷰**: 1회차 codex NOT PASS(R1 herdr 스키마 검증 없음, R2 점 표기·인라인 표에서 설정 유실, R3 원본과 출력 이름 충돌 시 덮어씀). 811b080 에서 수정. +- **리뷰**: 1회차 codex NOT PASS(R1 herdr 스키마 검증 없음, R2 점 표기·인라인 표에서 설정 유실, R3 원본과 출력 이름 충돌 시 덮어씀). 811b080 에서 수정. 2회차 codex NOT PASS(R3 의 폴백 경로는 같은 검사를 거치지 않음), 폴백도 같은 이름 선택을 거치도록 수정. ### 🚀 `v4.8.4` — `.claude/skills` 링크 검사가 두 로컬 배치를 받고 깨진 링크를 잡는다 (2026-10-04, PR #47) diff --git a/tests/test_issue45_herdr_config.py b/tests/test_issue45_herdr_config.py index bb8a066..4e95973 100644 --- a/tests/test_issue45_herdr_config.py +++ b/tests/test_issue45_herdr_config.py @@ -170,11 +170,79 @@ def test_unwritable_config_dir_falls_back_to_mam_dir(tmp_path): assert path == str(mam / hc.FALLBACK_NAME) assert any("relative paths" in w for w in warnings), warnings assert tomllib.loads(Path(path).read_text())["ui"]["confirm_close"] is False + if HERDR: + assert hc.herdr_rejects(path, HERDR) is None assert hc.write_server_config(_env(tmp_path))[0] is None # no fallback given finally: cfg_dir.chmod(0o755) +# --- the fallback obeys the same rules (review round 2, R3) ---------------- + +def _tag(source: Path) -> str: + import hashlib + return hashlib.sha256(os.path.realpath(source).encode()).hexdigest()[:12] + + +@pytest.mark.skipif(os.geteuid() == 0, reason="root ignores directory permissions") +def test_a_foreign_fallback_file_is_kept(tmp_path): + cfg_dir = tmp_path / "cfg" + cfg_dir.mkdir() + source = cfg_dir / "config.toml" + source.write_text("[ui]\nconfirm_close = false\n") + cfg_dir.chmod(0o555) + mam = tmp_path / ".mam" + mam.mkdir() + foreign = mam / hc.FALLBACK_NAME + foreign.write_bytes(b"# user-owned\n[ui]\nconfirm_close = true\n") + try: + path, warnings = hc.write_server_config({"HERDR_CONFIG_PATH": str(source)}, str(mam), HERDR) + finally: + cfg_dir.chmod(0o755) + assert foreign.read_bytes() == b"# user-owned\n[ui]\nconfirm_close = true\n" + assert path == str(mam / f"herdr-server-config-{_tag(source)}.toml"), warnings + assert tomllib.loads(Path(path).read_text())["ui"]["confirm_close"] is False + + +@pytest.mark.parametrize("alias", [False, True], ids=["same-path", "symlink-alias"]) +def test_the_fallback_is_never_the_source(tmp_path, alias): + mam = tmp_path / ".mam" + mam.mkdir() + body = b"[ui]\nconfirm_close = false\n" + if alias: + source = tmp_path / "real.toml" + source.write_bytes(body) + (mam / hc.FALLBACK_NAME).symlink_to(source) + source_dir = tmp_path + else: + source = mam / hc.FALLBACK_NAME + source.write_bytes(body) + source_dir = mam + # both names beside the source are someone else's, so the fallback is used + for name in (hc.OUTPUT_NAME, f"mam-server-config-{_tag(source)}.toml"): + (source_dir / name).write_bytes(b"# foreign\n") + path, _ = hc.write_server_config({"HERDR_CONFIG_PATH": str(source)}, str(mam), HERDR) + assert source.read_bytes() == body + if alias: # os.replace would swap out the user's link itself + assert (mam / hc.FALLBACK_NAME).is_symlink() + assert path and not os.path.samefile(path, source) + assert tomllib.loads(Path(path).read_text())["ui"]["confirm_close"] is False + + +def test_no_write_when_every_name_is_taken(tmp_path): + mam = tmp_path / ".mam" + mam.mkdir() + source = tmp_path / "config.toml" + source.write_text("[ui]\nconfirm_close = false\n") + taken = [tmp_path / hc.OUTPUT_NAME, tmp_path / f"mam-server-config-{_tag(source)}.toml", + mam / hc.FALLBACK_NAME, mam / f"herdr-server-config-{_tag(source)}.toml"] + for f in taken: + f.write_bytes(b"# foreign\n") + path, warnings = hc.write_server_config({"HERDR_CONFIG_PATH": str(source)}, str(mam)) + assert path is None and any("no free output name" in w for w in warnings), warnings + assert all(f.read_bytes() == b"# foreign\n" for f in taken) + + def test_cli_prints_the_path(tmp_path): env = dict(os.environ, HOME=str(tmp_path), PYTHONPATH=str(REPO_ROOT / ".agents" / "skills")) env.pop("HERDR_CONFIG_PATH", None)