test(issue-25): plan reuse is explicit only and empty plan reports are not silent (P1-P11)
P1 is the reproduction: a running planner plus a promoted plan in its report directory must not reach the Creator. P2 pins that a named plan is used, that the newer unrelated one is not, and that the plan rides only the first implementation prompt - the correction round carries feedback alone, as the implicit load did too. P3-P5 keep the option's checks in front of any job. P6 keeps --plan mode and its promotion working. P9-P11 cover the loads the review widened: an empty initial plan stops the run, an empty discussion refine keeps the previous plan, and an empty escalation refine leaves the "follow the updated plan below" prompt carrying the real one. The existing _drive helpers hand every job the same non-empty PASS report and so cannot reach these paths at all, so this module drives the loop with its own stub that scripts a report per job. P7-P8 pin wrapper/runner parity, the legacy shell refusal, and that neither implementation globs for a plan any more. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
GodopuandClaude Opus 5 committed
1 parent
e551e3baf1
commit
a334083760
1 file changed
+332
@@ -0,0 +1,332 @@
|
||||
"""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_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
|
||||
Reference in new issue
Block a user