GodopuandClaude Opus 5
1e826515cc
Review R1/N1 (reviewer-grok-01, planner-reviewer-codex-01), reproduced before fixing. The usage line and SKILL.md both promise "relative to the control root", but parse_args did Path(...).expanduser().resolve(), which is relative to the caller's cwd. The runner never chdirs to workspace_root, so the two are routinely different - the documented recipe runs the loop from the control root while the source root is a worktree. Two ways it went wrong: a plan that exists only under the control root was not found at all (exit 1), and a same-named file in the caller's cwd was loaded instead, quietly putting an unrelated plan into the first implementation prompt. That second one is the very failure #25 is about, reintroduced through the option meant to replace it. Resolve against workspace_root the way jobs_dir already does (Issue #21 R2); pathlib's `/` leaves an absolute or expanded-home path untouched. Every earlier test passed an absolute tmp_path, which is why none of them caught this: P3c covers both failure modes with the control root away from the caller's cwd, and P3d pins that absolute and ~ paths are not rewritten. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
375 lines
16 KiB
Python
375 lines
16 KiB
Python
"""Issue #25: a promoted plan is reused only when it is named, and an empty
|
|
plan report is never silent.
|
|
|
|
Without --plan the runner used to glob the auto-resolved planner's report
|
|
directory for plan-*.md and feed the newest one to the Creator as "the plan
|
|
already agreed on". Both keys are wrong: the directory belongs to a session,
|
|
not to this task, and mtime is checkout order. 47 unrelated plans are committed
|
|
in this repository (P1).
|
|
|
|
The same review also found that the three --plan-mode reads had no content
|
|
check at all, so an empty planner or refine report silently blanked the plan
|
|
and switched the Creator's prompt (P9-P11).
|
|
|
|
Every test writes only under tmp_path (Issue #20).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import json
|
|
import os
|
|
import subprocess
|
|
import sys
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
REPO_ROOT = Path(__file__).resolve().parent.parent
|
|
RUNNER_PY = REPO_ROOT / ".agents" / "skills" / "lib_py" / "loop" / "runner.py"
|
|
RUN_LOOP_SH = REPO_ROOT / ".agents" / "skills" / "multi-agent-mux-loop" / "scripts" / "run_loop.sh"
|
|
|
|
GIT_ID = {"GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@example.invalid",
|
|
"GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@example.invalid"}
|
|
|
|
PLANNER = "planner-claude-01"
|
|
CREATOR = "creator-claude-01"
|
|
REVIEWER = "reviewer-claude-01"
|
|
|
|
OLD_PLAN = "# plan for a completely different issue\n\nOLD-PLAN-MARKER\n"
|
|
NEW_PLAN = "# the plan I actually mean\n\nNAMED-PLAN-MARKER\n"
|
|
PASS_REPORT = "looks fine\n\n[VERDICT: PASS]\n"
|
|
FAIL_REPORT = "line 3 is wrong\n\n[VERDICT: NOT PASS]\n"
|
|
FAIL_ESCALATE = "needs a redesign\n\n[ESCALATE: PLANNER]\n\n[VERDICT: NOT PASS]\n"
|
|
|
|
SELF_PLAN_PROMPT = "계획서가 존재하지 않으므로"
|
|
EXISTING_PLAN_PROMPT = "이미 수립된"
|
|
CORRECTION_WITH_PLAN = "갱신된 다음 계획서에 입각하여"
|
|
|
|
|
|
def _git(cwd: Path, *args: str) -> str:
|
|
return subprocess.run(["git", "-C", str(cwd), *args], capture_output=True, text=True,
|
|
check=True, env={**os.environ, **GIT_ID}).stdout.strip()
|
|
|
|
|
|
@pytest.fixture
|
|
def ctl(tmp_path, monkeypatch):
|
|
"""A control root whose planner report directory already holds an unrelated
|
|
promoted plan - the condition that used to inject it (F1/F3)."""
|
|
main = tmp_path / "main"
|
|
main.mkdir()
|
|
_git(main, "init", "-q")
|
|
(main / "a.txt").write_text("a\n")
|
|
_git(main, "add", "a.txt")
|
|
_git(main, "commit", "-q", "-m", "init")
|
|
|
|
stale = main / ".agents" / "reports" / PLANNER / "plan-OLD.md"
|
|
stale.parent.mkdir(parents=True)
|
|
stale.write_text(OLD_PLAN, encoding="utf-8")
|
|
os.utime(stale, (2_000_000_000, 2_000_000_000)) # newest by mtime, by a mile
|
|
|
|
for k, v in GIT_ID.items():
|
|
monkeypatch.setenv(k, v)
|
|
monkeypatch.setenv("WORKSPACE_ROOT", str(main))
|
|
monkeypatch.delenv("MAM_REAL_ROOT", raising=False)
|
|
monkeypatch.setenv("MAM_JOBS_DIR", str(main / ".mam" / "jobs"))
|
|
monkeypatch.delenv("MAM_LOOP_WORKTREE", raising=False)
|
|
monkeypatch.delenv("MAM_LOOP_BASE", raising=False)
|
|
# the planner row is what makes the old glob fire at all (F7)
|
|
monkeypatch.setenv("MAM_STATE_JSON", json.dumps({"herdr_sessions": [
|
|
{"name": CREATOR, "agent": "claude", "role": "creator", "status": "running"},
|
|
{"name": REVIEWER, "agent": "claude", "role": "reviewer", "status": "running"},
|
|
{"name": PLANNER, "agent": "claude", "role": "planner", "status": "running"},
|
|
]}))
|
|
return main
|
|
|
|
|
|
def _runner(*extra):
|
|
from lib_py.loop import runner as lr
|
|
return lr.LoopRunner(["--creator", CREATOR, "--task", "t25", *extra])
|
|
|
|
|
|
def _drive(monkeypatch, tmp_path, argv, reports=None, on_worker=None, max_loop="1"):
|
|
"""run() end to end with the delegation stubbed.
|
|
|
|
`reports` maps (role, nth-call-of-that-role) to the report body; a body of
|
|
None means the report file is missing. Everything else gets a PASS for a
|
|
Reviewer and a one-line report otherwise. The existing _drive helpers hand
|
|
every job the same non-empty PASS report, which cannot reach the empty-plan
|
|
paths at all (F13).
|
|
|
|
Returns (rc, [(role, prompt)], runner).
|
|
"""
|
|
from lib_py.loop import runner as lr
|
|
monkeypatch.setattr(lr.signal, "signal", lambda *a, **k: None)
|
|
r = lr.LoopRunner(["--creator", CREATOR, "--task", "t25",
|
|
"--reviewer", REVIEWER, "--max-loop", max_loop, *argv])
|
|
reports = reports or {}
|
|
calls, files, nth = [], {}, {}
|
|
stub_dir = tmp_path / "stub-reports"
|
|
stub_dir.mkdir(exist_ok=True)
|
|
|
|
def fake_delegate(session, agent_type, role, prompt):
|
|
calls.append((role, prompt))
|
|
jid = f"j{len(calls)}"
|
|
n = nth[role] = nth.get(role, 0) + 1
|
|
body = reports.get((role, n), PASS_REPORT if role == "Reviewer" else "done\n")
|
|
if body is None:
|
|
files[jid] = None
|
|
else:
|
|
f = stub_dir / f"{jid}.md"
|
|
f.write_text(body, encoding="utf-8")
|
|
files[jid] = f
|
|
if role == "Worker" and on_worker:
|
|
on_worker(r)
|
|
return jid
|
|
|
|
monkeypatch.setattr(r, "delegate_job", fake_delegate)
|
|
monkeypatch.setattr(r, "wait_for_job", lambda *a, **k: True)
|
|
monkeypatch.setattr(r, "find_report", lambda jid: files.get(jid))
|
|
return r.run(), calls, r
|
|
|
|
|
|
def _roles(calls, role):
|
|
return [p for (ro, p) in calls if ro == role]
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# P1-P2 — reuse is explicit
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_p1_no_options_does_not_inject_an_unrelated_plan(ctl, monkeypatch, tmp_path, capsys):
|
|
"""The #25 reproduction: a running planner plus a promoted plan in its
|
|
report directory must not reach the Creator. Fails before the fix."""
|
|
rc, calls, _ = _drive(monkeypatch, tmp_path, [])
|
|
assert rc == 0
|
|
worker = _roles(calls, "Worker")[0]
|
|
assert worker.startswith(SELF_PLAN_PROMPT)
|
|
assert "OLD-PLAN-MARKER" not in worker
|
|
assert EXISTING_PLAN_PROMPT not in worker
|
|
out = capsys.readouterr().out
|
|
assert "Found existing promoted plan" not in out
|
|
assert "No plan loaded" in out
|
|
|
|
|
|
def test_p2_plan_file_is_the_only_plan_and_lives_one_round(ctl, monkeypatch, tmp_path):
|
|
"""The named plan is used, the newer unrelated one is not, and the plan is
|
|
not repeated in the correction round (N1/D16 - the behaviour the implicit
|
|
load had too)."""
|
|
named = tmp_path / "my-plan.md"
|
|
named.write_text(NEW_PLAN, encoding="utf-8")
|
|
rc, calls, _ = _drive(monkeypatch, tmp_path, ["--plan-file", str(named)],
|
|
reports={("Reviewer", 1): FAIL_REPORT}, max_loop="2")
|
|
assert rc == 0
|
|
workers = _roles(calls, "Worker")
|
|
assert workers[0].startswith(EXISTING_PLAN_PROMPT)
|
|
assert "NAMED-PLAN-MARKER" in workers[0]
|
|
assert "OLD-PLAN-MARKER" not in workers[0]
|
|
|
|
assert len(workers) == 2, "a correction round ran"
|
|
assert "NAMED-PLAN-MARKER" not in workers[1]
|
|
assert workers[1].startswith("리뷰어들이 지적한 다음 피드백")
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# P3-P5 — the option is checked before any job exists
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_p3_unusable_plan_files_are_refused_before_any_job(ctl, tmp_path, monkeypatch):
|
|
empty, ws, sub = tmp_path / "empty.md", tmp_path / "ws.md", tmp_path / "sub"
|
|
empty.write_text("")
|
|
ws.write_text(" \n\t\n")
|
|
sub.mkdir()
|
|
cases = [tmp_path / "nope.md", sub, empty, ws]
|
|
|
|
if os.getuid() != 0:
|
|
locked = tmp_path / "locked.md"
|
|
locked.write_text(NEW_PLAN, encoding="utf-8")
|
|
locked.chmod(0o000)
|
|
cases.append(locked)
|
|
|
|
from lib_py.loop import runner as lr
|
|
for path in cases:
|
|
r = lr.LoopRunner(["--creator", CREATOR, "--task", "t25", "--plan-file", str(path)])
|
|
monkeypatch.setattr(r, "delegate_job", lambda *a, **k: pytest.fail("a job was delegated"))
|
|
with pytest.raises(SystemExit) as e:
|
|
r.parse_args()
|
|
assert e.value.code == 1, path
|
|
|
|
|
|
def test_p3b_the_refusal_names_the_resolved_path(ctl, tmp_path, capsys):
|
|
missing = tmp_path / "nope.md"
|
|
with pytest.raises(SystemExit):
|
|
_runner("--plan-file", str(missing)).parse_args()
|
|
assert str(missing.resolve()) in capsys.readouterr().err
|
|
|
|
|
|
def test_p3c_a_relative_plan_file_is_taken_against_the_control_root(ctl, tmp_path, monkeypatch):
|
|
"""R1/N1: the usage line promises control-root-relative, and the runner
|
|
never chdirs to the control root. Resolving against the caller's cwd looked
|
|
somewhere else - and a same-named file there won silently, which is the
|
|
very failure #25 is about."""
|
|
(ctl / "plans").mkdir()
|
|
(ctl / "plans" / "named.md").write_text(NEW_PLAN, encoding="utf-8")
|
|
caller = tmp_path / "caller"
|
|
(caller / "plans").mkdir(parents=True)
|
|
monkeypatch.chdir(caller)
|
|
|
|
r = _runner("--plan-file", "plans/named.md")
|
|
r.parse_args()
|
|
assert Path(r.plan_file) == (ctl / "plans" / "named.md").resolve()
|
|
assert "NAMED-PLAN-MARKER" in r.current_plan
|
|
|
|
# the same name in the caller's cwd must not win
|
|
(caller / "plans" / "named.md").write_text(OLD_PLAN, encoding="utf-8")
|
|
r = _runner("--plan-file", "plans/named.md")
|
|
r.parse_args()
|
|
assert Path(r.plan_file) == (ctl / "plans" / "named.md").resolve()
|
|
assert "NAMED-PLAN-MARKER" in r.current_plan
|
|
assert "OLD-PLAN-MARKER" not in r.current_plan
|
|
|
|
|
|
def test_p3d_an_absolute_plan_file_is_left_alone(ctl, tmp_path, monkeypatch):
|
|
"""Anchoring the relative case must not rewrite absolute or ~ paths."""
|
|
named = tmp_path / "abs-plan.md"
|
|
named.write_text(NEW_PLAN, encoding="utf-8")
|
|
monkeypatch.chdir(tmp_path)
|
|
r = _runner("--plan-file", str(named))
|
|
r.parse_args()
|
|
assert Path(r.plan_file) == named.resolve()
|
|
|
|
monkeypatch.setenv("HOME", str(tmp_path))
|
|
home_plan = tmp_path / "home-plan.md"
|
|
home_plan.write_text(NEW_PLAN, encoding="utf-8")
|
|
r = _runner("--plan-file", "~/home-plan.md")
|
|
r.parse_args()
|
|
assert Path(r.plan_file) == home_plan.resolve()
|
|
|
|
|
|
def test_p4_plan_file_conflicts_with_plan_and_needs_a_value(ctl, tmp_path, monkeypatch):
|
|
named = tmp_path / "my-plan.md"
|
|
named.write_text(NEW_PLAN, encoding="utf-8")
|
|
from lib_py.loop import runner as lr
|
|
for argv in (["--plan-file", str(named), "--plan"], ["--plan", "--plan-file", str(named)],
|
|
["--plan-file"]):
|
|
r = lr.LoopRunner(["--creator", CREATOR, "--task", "t25", *argv])
|
|
monkeypatch.setattr(r, "delegate_job", lambda *a, **k: pytest.fail("a job was delegated"))
|
|
with pytest.raises(SystemExit) as e:
|
|
r.parse_args()
|
|
assert e.value.code == 1, argv
|
|
|
|
|
|
def _reviewable_worktree(ctl, tmp_path):
|
|
"""A linked worktree with one commit to review. The source root has to be
|
|
separate from the control root here: --base turns review identity on, and
|
|
the loop's own checkpoint under the control root's .mam would otherwise
|
|
count as the source changing mid-review."""
|
|
wt = tmp_path / "wt"
|
|
_git(ctl, "worktree", "add", "-q", "-b", "feat", str(wt))
|
|
base = _git(wt, "rev-parse", "HEAD")
|
|
(wt / "a.txt").write_text("a\nimplemented\n")
|
|
_git(wt, "commit", "-qam", "impl")
|
|
return ["--worktree", str(wt), "--base", base, "--review-only"]
|
|
|
|
|
|
def test_p5_plan_file_is_warned_about_and_ignored_with_review_only(ctl, monkeypatch, tmp_path, capsys):
|
|
named = tmp_path / "my-plan.md"
|
|
named.write_text(NEW_PLAN, encoding="utf-8")
|
|
rc, calls, r = _drive(monkeypatch, tmp_path,
|
|
["--plan-file", str(named), *_reviewable_worktree(ctl, tmp_path)])
|
|
assert rc == 0
|
|
assert _roles(calls, "Worker") == [], "Phase 2 is skipped"
|
|
assert "NAMED-PLAN-MARKER" not in _roles(calls, "Reviewer")[0]
|
|
assert r.current_plan == ""
|
|
assert "--plan-file is ignored with --review-only" in capsys.readouterr().out
|
|
|
|
|
|
def test_p5b_the_ignore_warning_needs_the_option(ctl, monkeypatch, tmp_path, capsys):
|
|
"""The warning is keyed on the option being given, not on a default value -
|
|
unlike the --plan-talk warning it sits next to (N2, runner.py:246)."""
|
|
rc, _, _ = _drive(monkeypatch, tmp_path, _reviewable_worktree(ctl, tmp_path))
|
|
assert rc == 0
|
|
assert "is ignored with --review-only" not in capsys.readouterr().out
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# P6, P9-P11 — --plan mode: compatible, and never silently planless
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_p6_plan_mode_still_plans_and_promotes(ctl, monkeypatch, tmp_path):
|
|
rc, calls, r = _drive(monkeypatch, tmp_path, ["--plan", "--plan-talk", "0"],
|
|
reports={("Planner", 1): NEW_PLAN})
|
|
assert rc == 0
|
|
worker = _roles(calls, "Worker")[0]
|
|
assert worker.startswith(EXISTING_PLAN_PROMPT)
|
|
assert "NAMED-PLAN-MARKER" in worker
|
|
promoted = ctl / ".agents" / "reports" / PLANNER / f"plan-{r.plan_job_id}.md"
|
|
assert promoted.is_file() and "NAMED-PLAN-MARKER" in promoted.read_text()
|
|
|
|
|
|
@pytest.mark.parametrize("body", ["", " \n\t\n"], ids=["empty", "whitespace"])
|
|
def test_p9_an_empty_initial_plan_stops_the_run(ctl, monkeypatch, tmp_path, capsys, body):
|
|
"""D13: paying for a planning phase and getting nothing back must not be a
|
|
quiet demotion to self-planning."""
|
|
rc, calls, _ = _drive(monkeypatch, tmp_path, ["--plan", "--plan-talk", "0"],
|
|
reports={("Planner", 1): body})
|
|
assert rc == 1
|
|
assert _roles(calls, "Worker") == [], "no implementation was delegated"
|
|
assert "has no plan text" in capsys.readouterr().err
|
|
|
|
|
|
def test_p10_an_empty_refine_keeps_the_previous_plan(ctl, monkeypatch, tmp_path, capsys):
|
|
"""D14: the discussion turn must not blank a plan that is still good."""
|
|
rc, calls, _ = _drive(monkeypatch, tmp_path, ["--plan", "--plan-talk", "1"],
|
|
reports={("Planner", 1): NEW_PLAN, ("Planner", 2): ""})
|
|
assert rc == 0
|
|
worker = _roles(calls, "Worker")[1] # [0] is the Creator's challenge turn
|
|
assert worker.startswith(EXISTING_PLAN_PROMPT)
|
|
assert "NAMED-PLAN-MARKER" in worker
|
|
assert "keeping the previous plan" in capsys.readouterr().out
|
|
|
|
|
|
def test_p11_an_empty_escalation_refine_never_claims_an_empty_plan(ctl, monkeypatch, tmp_path, capsys):
|
|
"""D14/D15: the correction prompt asserts "follow the updated plan below",
|
|
so the plan it carries must never be blank."""
|
|
rc, calls, _ = _drive(monkeypatch, tmp_path, ["--plan", "--plan-talk", "0"],
|
|
reports={("Planner", 1): NEW_PLAN, ("Planner", 2): "",
|
|
("Reviewer", 1): FAIL_ESCALATE},
|
|
max_loop="2")
|
|
assert rc == 0
|
|
correction = _roles(calls, "Worker")[1]
|
|
assert CORRECTION_WITH_PLAN in correction
|
|
assert "NAMED-PLAN-MARKER" in correction, "the prompt claims a plan; it must carry one"
|
|
assert "keeping the previous plan" in capsys.readouterr().out
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# P7-P8 — wrapper parity and the legacy shell path
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_p7_wrapper_and_runner_agree_on_the_option(tmp_path):
|
|
line = " --plan-file <path> Reuse an existing plan file, relative to the control root (not with --plan)"
|
|
sh = subprocess.run(["bash", str(RUN_LOOP_SH), "--help"], capture_output=True, text=True)
|
|
assert line in sh.stdout.splitlines()
|
|
|
|
code = ("import sys; sys.path.insert(0, %r)\n"
|
|
"from lib_py.loop import runner as lr\n"
|
|
"lr.LoopRunner([]).print_usage()\n" % str(REPO_ROOT / ".agents" / "skills"))
|
|
py = subprocess.run([sys.executable, "-c", code], capture_output=True, text=True)
|
|
assert line in py.stdout.splitlines()
|
|
|
|
|
|
def test_p7b_the_legacy_shell_loop_refuses_the_option(tmp_path):
|
|
marker = tmp_path / "loop.marker"
|
|
res = subprocess.run(["bash", str(RUN_LOOP_SH), "--creator", CREATOR, "--task", "t25",
|
|
"--plan-file", str(tmp_path / "x.md")],
|
|
capture_output=True, text=True, cwd=str(tmp_path),
|
|
env={**os.environ, "MAM_LOOP_PY": "0", "MAM_LOOP_MARKER": str(marker)})
|
|
assert res.returncode == 1
|
|
assert "--plan-file" in res.stderr
|
|
assert not marker.exists(), "refused before the loop lock"
|
|
|
|
|
|
def test_p8_neither_loop_implementation_globs_for_a_plan_any_more():
|
|
for path in (RUN_LOOP_SH, RUNNER_PY):
|
|
assert "plan-*.md" not in path.read_text(encoding="utf-8"), path
|