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 <noreply@anthropic.com>
This commit is contained in:
GodopuandClaude Opus 5.5 committed 2026-10-04 11:49:08 +00:00
1 parent 7c544c06ab
commit 35935f9e8a
3 files changed
+82 -11

No files matched your search

+13 -10
View File
@@ -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. 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 * 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 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 meaning. `<.mam>/herdr-server-config.toml` only when that directory is not
write for the same source: then a name with a hash of the source path. writable. Either way never the source itself and never a file this module
`<.mam>/herdr-server-config.toml` only when the directory is not writable. 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 * 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 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` 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) return os.path.realpath(a) == os.path.realpath(b)
def output_path(source: str) -> str: def output_path(source: str, directory: Optional[str] = None, name: str = OUTPUT_NAME) -> str:
"""Beside the source: OUTPUT_NAME, unless that is the source itself or a """`name` in `directory` (default: beside the source), unless that is the
file this module did not write for this source; then a per-source name.""" source itself (aliases included) or a file this module did not write for
d = os.path.dirname(source) 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] tag = hashlib.sha256(os.path.realpath(source).encode()).hexdigest()[:12]
for name in (OUTPUT_NAME, f"mam-server-config-{tag}.toml"): for n in (name, f"{name[:-len('.toml')]}-{tag}.toml"):
target = os.path.join(d, name) target = os.path.join(d, n)
if _same_file(target, source): if _same_file(target, source):
continue continue
if os.path.lexists(target) and _generated_source(target) not in ("", source): if os.path.lexists(target) and _generated_source(target) not in ("", source):
continue continue
return target 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: 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: if fallback_dir:
target = os.path.join(fallback_dir, FALLBACK_NAME) target = os.path.join(fallback_dir, FALLBACK_NAME)
try: try:
target = output_path(source, fallback_dir, FALLBACK_NAME)
if _write_checked(target, source, body, mode, herdr, warnings): if _write_checked(target, source, body, mode, herdr, warnings):
warnings.append(f"wrote {target} instead; relative paths in the herdr config may not resolve") warnings.append(f"wrote {target} instead; relative paths in the herdr config may not resolve")
return target, warnings return target, warnings
+1 -1
View File
@@ -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_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`, 원본 경로 규칙, 쓰기·폴백. - **단위 (`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. 전체 스위트는 돌리지 않았다. - **관련 스위트**: 새 두 파일 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) ### 🚀 `v4.8.4` — `.claude/skills` 링크 검사가 두 로컬 배치를 받고 깨진 링크를 잡는다 (2026-10-04, PR #47)
+68
View File
@@ -170,11 +170,79 @@ def test_unwritable_config_dir_falls_back_to_mam_dir(tmp_path):
assert path == str(mam / hc.FALLBACK_NAME) assert path == str(mam / hc.FALLBACK_NAME)
assert any("relative paths" in w for w in warnings), warnings assert any("relative paths" in w for w in warnings), warnings
assert tomllib.loads(Path(path).read_text())["ui"]["confirm_close"] is False 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 assert hc.write_server_config(_env(tmp_path))[0] is None # no fallback given
finally: finally:
cfg_dir.chmod(0o755) 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): def test_cli_prints_the_path(tmp_path):
env = dict(os.environ, HOME=str(tmp_path), PYTHONPATH=str(REPO_ROOT / ".agents" / "skills")) env = dict(os.environ, HOME=str(tmp_path), PYTHONPATH=str(REPO_ROOT / ".agents" / "skills"))
env.pop("HERDR_CONFIG_PATH", None) env.pop("HERDR_CONFIG_PATH", None)