diff --git a/tests/test_issue25_plan_reuse.py b/tests/test_issue25_plan_reuse.py new file mode 100644 index 0000000..175876f --- /dev/null +++ b/tests/test_issue25_plan_reuse.py @@ -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 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