From a6fb39ba9f59b3c09bd6ff1641aa03f8a484f773 Mon Sep 17 00:00:00 2001 From: Godopu Date: Sun, 25 Oct 2026 17:35:58 +0000 Subject: [PATCH] test: review round 1 of Issue #71 (names, row-less stop server, migration retry) - create with a non-ASCII name: the attach target is the name herdr has; bash/Python parity for non-ASCII and option-like names; no python3 is an error for a non-ASCII name. - row-less stop without --herdr-session uses HERDR_SESSION_NAME; an unreadable row is not taken for no row. - a retry (entry point, or any write) repairs a YAML a failed migration left in format 1. - tier3: the "cannot infer agent" case registers its row, as tier1 does. - S11: base re-pinned to 5cd72d2, which changes the shim's sanitize body. Refs #71 Co-Authored-By: Claude Opus 5.5 --- tests/test_issue23_24_startup_path.py | 2 +- tests/test_issue71_herdr_names.py | 97 +++++++++++++++++++++++++-- tests/test_tier3_integration.py | 4 ++ 3 files changed, 97 insertions(+), 6 deletions(-) diff --git a/tests/test_issue23_24_startup_path.py b/tests/test_issue23_24_startup_path.py index 4435e13..ce6ecff 100644 --- a/tests/test_issue23_24_startup_path.py +++ b/tests/test_issue23_24_startup_path.py @@ -303,7 +303,7 @@ def test_s11_this_branch_does_not_change_the_generated_shim(mam_sandbox): too (one sanitize definition written in with `declare -f`, agent attach resolved, no silent wrong name), so the base is that commit.""" repo = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) - base = os.environ.get("MAM_BASE_SHA", "4243da5") + base = os.environ.get("MAM_BASE_SHA", "5cd72d2") show = subprocess.run(["git", "show", "%s:.agents/skills/lib.sh" % base], capture_output=True, text=True, cwd=repo) if show.returncode != 0: diff --git a/tests/test_issue71_herdr_names.py b/tests/test_issue71_herdr_names.py index a534fee..c4eb511 100644 --- a/tests/test_issue71_herdr_names.py +++ b/tests/test_issue71_herdr_names.py @@ -29,6 +29,7 @@ from lib_py.agents.sanitize import sanitize_herdr_agent_name # noqa: E402 LONG = "lab-issue71-a-very-long-branch-name-creator-claude" # > 32 characters UPPER = "Lab-Upper-creator-claude" PLAIN = "lab-plain-creator-claude" # needs no conversion +UNI = "\u0130stanbul-creator-claude" # str.lower() differs from tr here STOP = "bash .agents/skills/multi-agent-mux-stop/scripts/stop_session.sh" @@ -77,7 +78,7 @@ def _assert_new_form(row, server): assert "kill_command" not in row and "start_command" not in row -@pytest.mark.parametrize("name", [LONG, UPPER, PLAIN]) +@pytest.mark.parametrize("name", [LONG, UPPER, PLAIN, UNI]) @pytest.mark.parametrize("server", ["default", "mam-iso71"]) def test_create_writes_the_attach_command(mam_sandbox, mock_herdr, mock_agents, name, server): _set_mock(mock_herdr, strict_agent_names=True) @@ -160,7 +161,9 @@ def test_wrapper_attach_calls_herdr_with_the_herdr_name(mam_sandbox, mock_herdr, # --- one sanitize rule ---------------------------------------------------------- NAMES = ["", "plain", "Upper-Case", "9lead", "-dash", "has space/and.dot", "x" * 32, "y" * 33, - LONG, LONG.upper(), "creator-claude-fix-issue-99-long-branch"] + LONG, LONG.upper(), "creator-claude-fix-issue-99-long-branch", "-n", "-e", "a\\nb"] +UNI_NAMES = [UNI, "caf\u00e9-creator-claude", "\u00fc" * 40, "kelvin-\u212a", "\u65e5\u672c-creator-claude", + "\u0130" * 20 + "-creator-claude"] def test_lib_sh_has_one_definition(): @@ -216,6 +219,26 @@ def test_each_hasher_alone_matches_python(tmp_path, hashers): assert res.stdout == sanitize_herdr_agent_name(name) + "\n", name +@pytest.mark.parametrize("hashers", [["python3"], ["shasum", "python3"]]) +def test_a_name_that_is_not_ascii_matches_python(tmp_path, hashers): + if not shutil.which(hashers[0]): + pytest.skip(hashers[0] + " not installed") + defn = _definition(tmp_path) + d = _tools_dir(tmp_path, ["tr", "sed", "awk", "cut"] + hashers) + for name in UNI_NAMES: + res = _sanitize_with_path(defn, d, name) + assert res.returncode == 0, (name, res.stderr) + assert res.stdout == sanitize_herdr_agent_name(name) + "\n", name + + +def test_a_name_that_is_not_ascii_needs_python3(tmp_path): + defn = _definition(tmp_path) + d = _tools_dir(tmp_path, ["tr", "sed", "awk", "cut"]) + for name in UNI_NAMES: + res = _sanitize_with_path(defn, d, name) + assert res.returncode != 0 and res.stdout == "" and "not ASCII" in res.stderr, (name, res.stderr) + + def test_no_hasher_at_all_is_an_error_not_a_name(tmp_path): defn = _definition(tmp_path) d = _tools_dir(tmp_path, ["tr", "sed", "awk", "cut"]) @@ -351,6 +374,41 @@ def test_any_write_migrates_and_rewrites_the_yaml(own_workspace_root, tmp_path): _assert_new_form(s, s["herdr_session"]) +@pytest.mark.parametrize("retry", ["entry-point", "any-write"]) +def test_a_retry_repairs_the_yaml_a_failed_migration_left_behind(own_workspace_root, tmp_path, monkeypatch, retry): + """The DB is committed before the YAML is replaced; if that fails, the next run rewrites the YAML.""" + from lib_py import registry_format + ws = tmp_path / "ws" + yaml_path = _write_old_registry(ws, with_db=True) + real_replace, failed = os.replace, [] + + def replace_once(src, dst, *a, **kw): + if os.path.abspath(str(dst)) == str(yaml_path) and not failed: + failed.append(dst) + raise OSError("injected: YAML not replaced") + return real_replace(src, dst, *a, **kw) + + monkeypatch.setenv("YAML_PATH", "") + monkeypatch.setenv("AGENT_SESSIONS_MUTATION", "") + monkeypatch.setattr(os, "replace", replace_once) + assert registry_format.main(["--yaml", str(yaml_path)]) == 1 and failed + monkeypatch.setattr(os, "replace", real_replace) + assert _state(ws)["registry_format"] == 2 + assert registry_format.stored_version(yaml.safe_load(yaml_path.read_text())) == 1 + + if retry == "entry-point": + res = _entry(yaml_path) + else: + res = subprocess.run(["bash", "-c", 'source "$1" && atomic_dump_yaml "$2"', "_", str(SKILLS / "lib.sh"), str(yaml_path)], + input="pass\n", capture_output=True, text=True, timeout=60) + assert res.returncode == 0, res.stderr + y = yaml.safe_load(yaml_path.read_text()) + assert y["registry_format"] == 2 + assert {s["name"]: s for s in y["herdr_sessions"]} == _db_rows(ws), "YAML and DB agree" + for s in y["herdr_sessions"]: + _assert_new_form(s, s["herdr_session"]) + + def test_a_new_registry_is_written_current(own_workspace_root, tmp_path): yaml_path = tmp_path / "fresh" / ".mam" / "agent-sessions.yaml" yaml_path.parent.mkdir(parents=True) @@ -375,10 +433,12 @@ def _registry_bytes(sandbox): return out -def _stop(sandbox, name, *extra): +def _stop(sandbox, name, *extra, server="default", env=None): script = sandbox / ".agents" / "skills" / "multi-agent-mux-stop" / "scripts" / "stop_session.sh" - return subprocess.run(["bash", str(script), "--session", name, "--herdr-session", "default"] + list(extra), - capture_output=True, text=True, cwd=str(sandbox), timeout=120) + flag = ["--herdr-session", server] if server else [] + return subprocess.run(["bash", str(script), "--session", name] + flag + list(extra), + capture_output=True, text=True, cwd=str(sandbox), timeout=120, + env=dict(os.environ, **(env or {}))) def _agent(cwd, pane): @@ -422,6 +482,33 @@ def test_stop_reports_a_rowless_session_that_is_not_there(mam_sandbox, mock_herd assert _registry_bytes(mam_sandbox) == registry_with_a_row +def test_stop_without_a_row_uses_the_environments_server(mam_sandbox, mock_herdr, registry_with_a_row): + """No --herdr-session: the server is HERDR_SESSION_NAME, as create chose it, not one derived from the workspace.""" + _set_mock(mock_herdr, agents={"orphan73": _agent(mam_sandbox, "w1:p74")}, calls=[]) + res = _stop(mam_sandbox, "orphan73", server=None, env={"HERDR_SESSION_NAME": "wanted-env-server"}) + assert res.returncode == 0, res.stdout + res.stderr + assert "wanted-env-server" in res.stdout + calls = json.loads(Path(mock_herdr).read_text())["calls"] + # every call on an agent or pane names that server (`session list` and `config check` name none) + targeted = [c for c in calls if "agent" in c[:3] or "pane" in c[:3]] + assert targeted and all(c[:2] == ["--session", "wanted-env-server"] for c in targeted), calls + assert ["--session", "wanted-env-server", "agent", "get", "orphan73"] in calls + assert _registry_bytes(mam_sandbox) == registry_with_a_row + + +def test_stop_does_not_take_an_unreadable_row_for_no_row(mam_sandbox, mock_herdr): + """Only "no such row" leads to the row-less path; a lookup that fails otherwise stops.""" + db = mam_sandbox / ".mam" / "agent-sessions.db" + if db.exists(): + db.unlink() + (mam_sandbox / ".mam" / "agent-sessions.yaml").write_text("herdr_sessions:\n- 42\n") + _set_mock(mock_herdr, agents={"orphan74": _agent(mam_sandbox, "w1:p75")}) + res = _stop(mam_sandbox, "orphan74") + assert res.returncode == 1, res.stdout + res.stderr + assert "cannot read the row" in res.stderr + assert "orphan74" in json.loads(Path(mock_herdr).read_text())["agents"] + + def test_stop_without_a_row_purges_nothing(mam_sandbox, mock_herdr, registry_with_a_row): _set_mock(mock_herdr, agents={"orphan72": _agent(mam_sandbox, "w1:p73")}) res = _stop(mam_sandbox, "orphan72", "--purge-conversation", "--yes") diff --git a/tests/test_tier3_integration.py b/tests/test_tier3_integration.py index 1551098..0de5527 100644 --- a/tests/test_tier3_integration.py +++ b/tests/test_tier3_integration.py @@ -393,6 +393,10 @@ def test_integration_invalid_arguments_exit_statuses(mam_sandbox, mock_herdr, mo assert "ERROR: --session required" in res.stderr # 3. stop_session.sh cannot infer agent from weird session name + # (Issue #71: a session with no row needs no agent kind, so the row is + # registered here, with no agent, suffix or pane.cmd) + (tmp_path / ".mam" / "agent-sessions.yaml").write_text( + "herdr_sessions:\n- name: bad-name\n status: running\n pane: {}\n") res = subprocess.run(["bash", str(stop_script), "--session", "bad-name"], capture_output=True, text=True, cwd=str(tmp_path)) assert res.returncode == 2 assert "ERROR: cannot infer agent" in res.stderr