fix(issue-25): a relative --plan-file is taken against the control root, as its usage line says
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>
This commit is contained in:
1 parent
5fa592347c
commit
1e826515cc
2 files changed
+48
-1
No files matched your search
@@ -256,7 +256,12 @@ class LoopRunner:
|
|||||||
print("Use --plan to have the Planner write a new plan, or --plan-file to reuse one:", file=sys.stderr)
|
print("Use --plan to have the Planner write a new plan, or --plan-file to reuse one:", file=sys.stderr)
|
||||||
print(' run_loop.sh --creator <creator> --plan-file <path> --task "..."', file=sys.stderr)
|
print(' run_loop.sh --creator <creator> --plan-file <path> --task "..."', file=sys.stderr)
|
||||||
sys.exit(1)
|
sys.exit(1)
|
||||||
resolved = Path(self.plan_file).expanduser().resolve()
|
# A relative path is taken against the control root, as the usage
|
||||||
|
# line says and as jobs_dir already does (Issue #21 R2): the runner
|
||||||
|
# never chdirs to workspace_root, so resolving against the caller's
|
||||||
|
# cwd would look somewhere else entirely - and could find a
|
||||||
|
# same-named file there. `/` keeps an absolute path as it is.
|
||||||
|
resolved = (self.workspace_root / Path(self.plan_file).expanduser()).resolve()
|
||||||
text = self._plan_text(resolved)
|
text = self._plan_text(resolved)
|
||||||
if text is None:
|
if text is None:
|
||||||
log_error(f"--plan-file {resolved}: not a readable file with plan text in it.")
|
log_error(f"--plan-file {resolved}: not a readable file with plan text in it.")
|
||||||
|
|||||||
@@ -203,6 +203,48 @@ def test_p3b_the_refusal_names_the_resolved_path(ctl, tmp_path, capsys):
|
|||||||
assert str(missing.resolve()) in capsys.readouterr().err
|
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):
|
def test_p4_plan_file_conflicts_with_plan_and_needs_a_value(ctl, tmp_path, monkeypatch):
|
||||||
named = tmp_path / "my-plan.md"
|
named = tmp_path / "my-plan.md"
|
||||||
named.write_text(NEW_PLAN, encoding="utf-8")
|
named.write_text(NEW_PLAN, encoding="utf-8")
|
||||||
|
|||||||
Reference in new issue
Block a user