fix(reset): refuse id-less claude/grok targets, gate submit during a reset, validate the spec (#78)

Review round 1 of PR #82:
- A running claude/grok row without a recorded id is refused by the
  precheck (stop would fill it with a folder-wide guess the purge then
  deletes); record-id never records a pre-assigned guess, and a target is
  skipped (id_unverified) when the row's id after the normal stop is not
  the one recorded before it.
- reset takes the update admission lock exclusively while it publishes
  .mam/loop-guard-active and .mam/reset-in-progress; delegate submit
  refuses on a live reset marker (mam_reset_admit). Before each target's
  stop its open jobs are re-read (skipped as open_job, never cancelled),
  and a row whose agent/server no longer matches the spec is skipped.
- read_spec validates every stored field (herdr_server required, never
  defaulted) and rejects duplicate names.
- Signal tests wait 60 s for the kill-session gate (this Mac needs ~18 s).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
GodopuandClaude Opus 5.5 committed 2026-10-30 00:15:31 +09:00
1 parent f90a705316
commit 5c3c2eb861
6 files changed
+529 -62

No files matched your search

+372 -2
View File
@@ -458,6 +458,27 @@ def test_select_targets_and_plan_cli(tmp_path: Path):
assert "bad-role-claude" in res_pre.stdout
assert "mystery-row" in res_pre.stdout
# Review B2: claude/grok get their id at spawn, so a running row without one
# is refused (stop would fill it with a folder-wide guess); a post-discovered
# agent without one stays a target (its id is captured by its own pane).
no_id_state = {
"herdr_sessions": [
_row("reviewer-claude-noid", "claude", ws, own_id=None),
_row("reviewer-grok-noid", "grok", ws, own_id=None),
_row("reviewer-agy-noid", "agy", ws, own_id=None),
]
}
res_noid = _run_reset_py(["plan", "--workspace", str(ws)], state=no_id_state)
assert res_noid.returncode == 1
noid_plan = json.loads(res_noid.stdout)
assert [t["name"] for t in noid_plan["targets"]] == ["reviewer-agy-noid"]
assert len(noid_plan["errors"]) == 2
for name, agent in (("reviewer-claude-noid", "claude"), ("reviewer-grok-noid", "grok")):
assert any(
"running session %s (%s) has no recorded conversation id" % (name, agent) in e
for e in noid_plan["errors"]
)
def test_spec_file_write_read_and_validation(tmp_path: Path):
"""D5: write-spec atomically writes .mam/reset-spec.json; corrupt, wrong-version,
@@ -526,6 +547,34 @@ def test_spec_file_write_read_and_validation(tmp_path: Path):
with pytest.raises(SpecError, match="unknown agent"):
read_spec(str(ws))
# Review B2 (codex): every stored field is validated; a lost herdr_server is
# never replaced by "default", and a name listed twice is refused
good = dict(targets[0])
for mutate in (
lambda e: e.pop("herdr_server"),
lambda e: e.update(herdr_server=""),
lambda e: e.update(herdr_server=None),
lambda e: e.pop("herdr_workspace"),
lambda e: e.update(herdr_workspace=None),
lambda e: e.pop("old_id"),
lambda e: e.update(old_id=""),
lambda e: e.update(old_id=7),
lambda e: e.pop("old_created_at"),
lambda e: e.update(old_created_at=None),
):
bad = dict(good)
mutate(bad)
spath.write_text(json.dumps({"version": 1, "workspace": str(ws), "agents": [bad]}), encoding="utf-8")
with pytest.raises(SpecError, match="incomplete agent entry"):
read_spec(str(ws))
spath.write_text(json.dumps({"version": 1, "workspace": str(ws), "agents": [good, dict(good)]}), encoding="utf-8")
with pytest.raises(SpecError, match="lists reviewer-claude-01 twice"):
read_spec(str(ws))
# Control: legal empty values (label, created_at) and an explicit null old_id pass
ok = dict(good, herdr_workspace="", old_created_at="", old_id=None)
spath.write_text(json.dumps({"version": 1, "workspace": str(ws), "agents": [ok]}), encoding="utf-8")
assert read_spec(str(ws))["agents"] == [ok]
# Corrupt JSON
spath.write_text("{corrupt", encoding="utf-8")
with pytest.raises(SpecError, match="check it, then remove it by hand"):
@@ -1335,7 +1384,7 @@ with open(spy_log, "a", encoding="utf-8") as lf:
if verb == "kill-session" and not os.path.exists(entered):
with open(entered, "w", encoding="utf-8") as fh:
fh.write("1")
deadline = time.time() + 15.0
deadline = time.time() + 60.0
while not os.path.exists(release) and time.time() < deadline:
time.sleep(0.05)
@@ -1366,7 +1415,7 @@ os.execv(real, [real, *raw_args])
text=True,
)
try:
deadline = time.time() + 15.0
deadline = time.time() + 60.0
while not entered_gate.exists() and time.time() < deadline:
assert proc.poll() is None, "reset.sh exited before reaching kill-session"
time.sleep(0.05)
@@ -1414,3 +1463,324 @@ os.execv(real, [real, *raw_args])
assert not any(
l.startswith("reset [") and "--purge-conversation" in l for l in stdout.splitlines()
)
# --------------------------------------------------------------------------- Review round 1 (PR #82)
def _write_spec_file(sandbox: Path, agents: List[dict]) -> Path:
spec = sandbox / ".mam" / "reset-spec.json"
spec.write_text(
json.dumps({"version": 1, "workspace": os.path.realpath(str(sandbox)), "written_at": "x", "agents": agents}),
encoding="utf-8",
)
return spec
def _spec_entry(name: str, agent: str, old_id: Optional[str], server: str = "default") -> dict:
return {
"name": name,
"agent": agent,
"role": "reviewer",
"herdr_server": server,
"herdr_workspace": "ws",
"old_id": old_id,
"old_created_at": "2026-10-01T00:00:00Z",
}
def _result_of(stdout: str, name: str) -> str:
line = [l for l in stdout.splitlines() if l.startswith(name + " ")][-1]
return line.split(None, 6)[6]
def _live_marker(path: Path, holder: subprocess.Popen) -> None:
ls = " ".join(
subprocess.run(["ps", "-p", str(holder.pid), "-o", "lstart="], capture_output=True, text=True).stdout.split()
)
path.write_text("pid=%d\nlstart=%s\nstarted=x\n" % (holder.pid, ls), encoding="utf-8")
def test_record_id_never_records_a_pre_assigned_guess(tmp_path: Path):
"""Review B2 (claude): a claude/grok entry with old_id=None never takes the id
a normal stop guessed onto its row; a post-discovered one still does."""
from lib_py.reset import read_spec, write_spec
ws = tmp_path / "ws"
ws.mkdir()
write_spec(str(ws), [_spec_entry("c-noid", "claude", None), _spec_entry("a-noid", "agy", None)])
state = {
"herdr_sessions": [
_row("c-noid", "claude", ws, status="stopped", own_id=UA),
_row("a-noid", "agy", ws, status="stopped", own_id=UB),
]
}
for name in ("c-noid", "a-noid"):
res = _run_reset_py(["record-id", "--workspace", str(ws), "--session", name], state=state)
assert res.returncode == 0, res.stderr
old = {e["name"]: e["old_id"] for e in read_spec(str(ws))["agents"]}
assert old == {"c-noid": None, "a-noid": UB}
def test_claude_without_recorded_id_is_refused_and_unregistered_conversation_kept(
mam_sandbox: Path, mock_herdr: Path, mock_agents
):
"""Review B2 (claude) (c): a running claude row with no id and an unregistered
claude conversation in the same folder: the reset refuses before any change,
so the conversation that is not provably this pane's is never purged."""
name = "reviewer-claude-01"
tf = _claude_transcript(mam_sandbox, mam_sandbox, UC)
_mock_herdr_agent(mock_herdr, name, mam_sandbox, agent="claude")
_write_rows(mam_sandbox, [_row(name, "claude", mam_sandbox, own_id=None)])
spy_log = _install_herdr_spy(mam_sandbox)
yaml_before = (mam_sandbox / ".mam" / "agent-sessions.yaml").read_bytes()
res = _run_reset(mam_sandbox, "--yes")
assert res.returncode == 1, res.stdout + res.stderr
assert "running session %s (claude) has no recorded conversation id" % name in res.stderr
assert "nothing was changed" in res.stderr
assert tf.is_file()
assert (mam_sandbox / ".mam" / "agent-sessions.yaml").read_bytes() == yaml_before
assert not (mam_sandbox / ".mam" / "reset-spec.json").exists()
assert not any(c["verb"] in ("kill-session", "pane close", "agent start") for c in _read_spy_log(spy_log))
res_dry = _run_reset(mam_sandbox, "--dry-run")
assert res_dry.returncode == 1
assert "has no recorded conversation id" in res_dry.stdout
def test_rerun_skips_a_target_whose_row_id_is_not_the_recorded_one(
mam_sandbox: Path, mock_herdr: Path, mock_agents
):
"""Review B2 (claude) (b): after the normal stop the row's id must equal the
spec's old_id. A: the row's id changed after the spec was written (UC, spec
UA). B: an older spec holds a claude entry with old_id null, and the stop
guesses an unregistered conversation onto its row. Neither is purged."""
name_a = "reviewer-claude-01"
name_b = "reviewer-claude-02"
files = [_claude_transcript(mam_sandbox, mam_sandbox, u) for u in (UA, UC, UD)]
_mock_herdr_agent(mock_herdr, name_a, mam_sandbox, agent="claude", pane_id="w1:p_a")
_mock_herdr_agent(mock_herdr, name_b, mam_sandbox, agent="claude", pane_id="w1:p_b")
_write_rows(
mam_sandbox,
[_row(name_a, "claude", mam_sandbox, own_id=UC), _row(name_b, "claude", mam_sandbox, own_id=None)],
)
_write_spec_file(mam_sandbox, [_spec_entry(name_a, "claude", UA), _spec_entry(name_b, "claude", None)])
res = _run_reset(mam_sandbox, "--yes")
_wait_reconcile_idle(mam_sandbox)
assert res.returncode == 4, res.stdout + res.stderr
assert _result_of(res.stdout, name_a) == "skipped: id not verified"
assert _result_of(res.stdout, name_b) == "skipped: id not verified"
assert not any(l.startswith("reset [") and "--purge-conversation" in l for l in res.stdout.splitlines())
for f in files:
assert f.is_file(), f
rows = _read_rows(mam_sandbox)
assert rows[name_a]["claude_session_id_own"] == UC
assert (mam_sandbox / ".mam" / "reset-spec.json").is_file()
def test_spec_server_is_validated_and_a_changed_row_is_not_stopped(
mam_sandbox: Path, mock_herdr: Path, mock_agents
):
"""Review B2 (codex): a spec entry that lost herdr_server is refused before any
stop (never `--herdr-session default`); a valid spec whose server no longer
matches the row skips that target untouched."""
name = "reviewer-claude-01"
tf = _claude_transcript(mam_sandbox, mam_sandbox, UA)
_mock_herdr_agent(mock_herdr, name, mam_sandbox, agent="claude")
_write_rows(mam_sandbox, [_row(name, "claude", mam_sandbox, own_id=UA)])
spy_log = _install_herdr_spy(mam_sandbox)
entry = _spec_entry(name, "claude", UA)
entry.pop("herdr_server")
spec = _write_spec_file(mam_sandbox, [entry])
spec_text = spec.read_text(encoding="utf-8")
res = _run_reset(mam_sandbox, "--yes")
assert res.returncode == 1, res.stdout + res.stderr
assert "incomplete agent entry" in res.stderr
assert "--herdr-session" not in res.stdout
assert spec.read_text(encoding="utf-8") == spec_text
assert not any(c["verb"] in ("kill-session", "pane close") for c in _read_spy_log(spy_log))
_write_spec_file(mam_sandbox, [_spec_entry(name, "claude", UA, server="isolated-server")])
res_changed = _run_reset(mam_sandbox, "--yes")
assert res_changed.returncode == 4, res_changed.stdout + res_changed.stderr
assert _result_of(res_changed.stdout, name) == "skipped: row changed"
assert "reset [%s]:" % name not in res_changed.stdout
assert not any(c["verb"] in ("kill-session", "pane close") for c in _read_spy_log(spy_log))
assert _read_rows(mam_sandbox)[name]["status"] == "running"
assert tf.is_file()
def test_job_after_precheck_skips_its_target_and_submit_is_refused_during_reset(
mam_sandbox: Path, mock_herdr: Path, mock_agents
):
"""Review B1 (codex), "reset first": while the reset runs (A's stop), a
delegate submit to B is refused by .mam/reset-in-progress and registers
nothing; a record for B written by other means is seen by B's re-check, so B
is skipped with its job left open and its conversation kept."""
name_a = "reviewer-claude-01"
name_b = "reviewer-claude-02"
tf_a = _claude_transcript(mam_sandbox, mam_sandbox, UA)
tf_b = _claude_transcript(mam_sandbox, mam_sandbox, UB)
_mock_herdr_agent(mock_herdr, name_a, mam_sandbox, agent="claude", pane_id="w1:p_a")
_mock_herdr_agent(mock_herdr, name_b, mam_sandbox, agent="claude", pane_id="w1:p_b")
_write_rows(
mam_sandbox,
[_row(name_a, "claude", mam_sandbox, own_id=UA), _row(name_b, "claude", mam_sandbox, own_id=UB)],
)
bin_dir = mam_sandbox / "bin"
herdr_bin = bin_dir / "herdr"
herdr_real = bin_dir / "herdr.real"
herdr_bin.rename(herdr_real)
probe = mam_sandbox / "submit_probe.json"
jobs_dir = mam_sandbox / ".mam" / "jobs"
job_file = jobs_dir / "job78late.json"
submit = mam_sandbox / ".agents" / "skills" / "multi-agent-mux-delegate-job" / "multi-agent-mux-delegate-job"
herdr_bin.write_text(
f"""#!{sys.executable}
import json, os, subprocess, sys
real = {repr(str(herdr_real))}
probe = {repr(str(probe))}
args = list(sys.argv[1:])
while args and args[0] in ("-L", "--server", "-s", "--session"):
args = args[2:]
if args[:2] == ["pane", "close"] or args[:1] == ["kill-session"]:
if not os.path.exists(probe):
marker = {repr(str(mam_sandbox / ".mam" / "reset-in-progress"))}
res = subprocess.run(
["bash", {repr(str(submit))}, "submit", "--agent", "claude-code",
"--agent-session", "herdr:{name_b}", "--prompt", "hi"],
cwd={repr(str(mam_sandbox))}, stdin=subprocess.DEVNULL, capture_output=True, text=True, timeout=120,
)
jobs = sorted(os.listdir({repr(str(jobs_dir))})) if os.path.isdir({repr(str(jobs_dir))}) else []
with open(probe, "w", encoding="utf-8") as fh:
json.dump({{"rc": res.returncode, "stdout": res.stdout, "stderr": res.stderr,
"marker": os.path.isfile(marker), "jobs": jobs}}, fh)
os.makedirs({repr(str(jobs_dir))}, exist_ok=True)
with open({repr(str(job_file))}, "w", encoding="utf-8") as fh:
json.dump({{"job_id": "job78late", "agent_session": "herdr:{name_b}", "status": "pending"}}, fh)
os.execv(real, [real, *sys.argv[1:]])
""",
encoding="utf-8",
)
herdr_bin.chmod(0o755)
res = _run_reset(mam_sandbox, "--yes")
_wait_reconcile_idle(mam_sandbox)
assert res.returncode == 4, res.stdout + res.stderr
p = json.loads(probe.read_text(encoding="utf-8"))
assert p["marker"] is True
assert p["rc"] != 0, p
assert "a reset of %s is in progress" % mam_sandbox in p["stderr"], p
assert "registered job" not in p["stdout"]
assert p["jobs"] == []
assert _result_of(res.stdout, name_a) == "done"
assert _result_of(res.stdout, name_b) == "skipped: job open"
assert "job78late" in res.stdout
assert "reset [%s]:" % name_b not in res.stdout
assert not tf_a.exists()
assert tf_b.is_file()
assert json.loads(job_file.read_text(encoding="utf-8"))["status"] == "pending"
rows = _read_rows(mam_sandbox)
assert rows[name_b]["status"] == "running" and rows[name_b]["claude_session_id_own"] == UB
assert not (mam_sandbox / ".mam" / "reset-in-progress").exists()
assert not (mam_sandbox / ".mam" / "loop-guard-active").exists()
def _descendant_args(root_pid: int) -> List[str]:
res = subprocess.run(["ps", "-eo", "pid=,ppid=,args="], capture_output=True, text=True, timeout=5)
kids: Dict[int, List[int]] = {}
args: Dict[int, str] = {}
for line in res.stdout.splitlines():
parts = line.split(None, 2)
if len(parts) == 3 and parts[0].isdigit() and parts[1].isdigit():
kids.setdefault(int(parts[1]), []).append(int(parts[0]))
args[int(parts[0])] = parts[2]
out, todo = [], [root_pid]
while todo:
for c in kids.get(todo.pop(), []):
out.append(args.get(c, ""))
todo.append(c)
return out
def test_reset_waits_for_an_admitted_submit_and_sees_its_job(mam_sandbox: Path, mock_herdr: Path, mock_agents):
"""Review B1 (codex), "submit first": a submit that holds the admission lock
shared until its record is written makes the reset wait before it publishes
its markers; the precheck then sees the job and refuses, nothing changed."""
import fcntl
name = "reviewer-claude-01"
tf = _claude_transcript(mam_sandbox, mam_sandbox, UA)
_mock_herdr_agent(mock_herdr, name, mam_sandbox, agent="claude")
_write_rows(mam_sandbox, [_row(name, "claude", mam_sandbox, own_id=UA)])
spy_log = _install_herdr_spy(mam_sandbox)
lock = open(mam_sandbox / ".mam" / "update-admission.lock", "a")
fcntl.flock(lock, fcntl.LOCK_SH)
script = mam_sandbox / ".agents" / "skills" / "multi-agent-mux-reset" / "scripts" / "reset.sh"
proc = subprocess.Popen(
["bash", str(script), "--yes"],
cwd=str(mam_sandbox),
env=dict(os.environ, MAM_UPDATE_ADMISSION_WAIT_S="60", PYTHONDONTWRITEBYTECODE="1"),
stdin=subprocess.DEVNULL,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
)
try:
deadline = time.time() + 60
while time.time() < deadline:
assert proc.poll() is None, proc.communicate()
if any("lib_py.pane_lock acquire --fd 7" in a for a in _descendant_args(proc.pid)):
break
time.sleep(0.1)
else:
pytest.fail("reset.sh never waited for the admission lock")
assert not (mam_sandbox / ".mam" / "reset-in-progress").exists()
assert not (mam_sandbox / ".mam" / "loop-guard-active").exists()
jobs_dir = mam_sandbox / ".mam" / "jobs"
jobs_dir.mkdir(parents=True, exist_ok=True)
(jobs_dir / "job78first.json").write_text(
json.dumps({"job_id": "job78first", "agent_session": "herdr:%s" % name, "status": "pending"}),
encoding="utf-8",
)
finally:
fcntl.flock(lock, fcntl.LOCK_UN)
lock.close()
stdout, stderr = proc.communicate(timeout=120)
assert proc.returncode == 1, stdout + stderr
assert "job78first" in stderr and "nothing was changed" in stderr
assert tf.is_file()
assert not any(c["verb"] in ("kill-session", "pane close") for c in _read_spy_log(spy_log))
assert not (mam_sandbox / ".mam" / "reset-in-progress").exists()
assert not (mam_sandbox / ".mam" / "loop-guard-active").exists()
def test_mam_reset_admit_refuses_only_a_live_reset(mam_sandbox: Path):
lib = mam_sandbox / ".agents" / "skills" / "lib.sh"
marker = mam_sandbox / ".mam" / "reset-in-progress"
def admit():
return subprocess.run(
["bash", "-c", 'source "$1" && mam_reset_admit "$2"', "_", str(lib), str(mam_sandbox)],
cwd=str(mam_sandbox), capture_output=True, text=True, timeout=60,
)
assert admit().returncode == 0
holder = subprocess.Popen(["sleep", "120"])
try:
_live_marker(marker, holder)
res = admit()
assert res.returncode == 1
assert "a reset of %s is in progress" % mam_sandbox in res.stderr
finally:
holder.kill()
holder.wait()
res_stale = admit()
assert res_stale.returncode == 0, res_stale.stderr