go <dir> refuses a plan that already ran, and the record stops forgetting

grapharc/cli/main.py

@@ -405,6 +405,7 @@ def _cmd_go(args: argparse.Namespace) -> int:
             config_path=args.config,
             approve=args.approve,
             approval_timeout=args.approval_timeout,
+            again=args.again,
             as_json=args.json,
         )
     candidate = Path(target)
@@ -423,6 +424,7 @@ def _cmd_go(args: argparse.Namespace) -> int:
             config_path=args.config,
             approve=args.approve,
             approval_timeout=args.approval_timeout,
+            again=args.again,
             as_json=args.json,
         )
     return plan(
@@ -996,6 +998,14 @@ def build_parser() -> argparse.ArgumentParser:
         metavar="MODULE:ATTR",
         help="the node kinds a planner may propose (default: grapharc.stdlib:build_registry)",
     )
+    go.add_argument(
+        "--again",
+        action="store_true",
+        help=(
+            "execute a saved plan that has already run; without this, a second "
+            "`go <run-dir>` is refused rather than silently re-running the graph"
+        ),
+    )
     _add_planning_flags(go)
     go.set_defaults(handler=_cmd_go)
 

grapharc/cli/plan.py

@@ -218,6 +218,21 @@ def _write_plan_file(
     )
 
 
+def _executed_run_ids(record: dict[str, Any]) -> list[str]:
+    """Every run that has executed this plan, oldest first.
+
+    Reads the list when there is one and falls back to the scalar, so a
+    `plan.json` written before the list existed reports its single run rather
+    than reporting none. A malformed list is treated the same way — this is a
+    record for a human to read, not a place to raise.
+    """
+    history = record.get("executed_run_ids")
+    if isinstance(history, list):
+        return [str(item) for item in history]
+    scalar = record.get("executed_run_id")
+    return [str(scalar)] if scalar else []
+
+
 def find_unexecuted_plan(runs_root: Path | None = None) -> Path | None:
     """The newest saved plan `go` has not executed yet, or None."""
     import json
@@ -293,6 +308,7 @@ def execute_plan(
     config_path: Path | None = None,
     approve: bool = False,
     approval_timeout: float | None = None,
+    again: bool = False,
     as_json: bool = False,
 ) -> int:
     """`grapharc go [<run-dir>]` — execute a plan `grapharc plan` saved.
@@ -305,8 +321,22 @@ def execute_plan(
     only an answered yes — the gate an external driver relies on when the
     plan can change things. These flags used to be accepted here and
     silently dropped, which was worse than refusing them.
+
+    **An executed plan is not re-executed by accident.** Bare `go` has always
+    skipped executed plans — `find_unexecuted_plan` passes over them — but the
+    explicitly-named-directory form did not make the same check, so a second
+    `go <dir>` ran the whole graph again and silently overwrote the record of
+    the first. That matters twice over: the plan record disagreed with the
+    trace, which is the audit trail and was right; and because an approval
+    binds to a proposal fingerprint that does not change between runs, one
+    human yes could be spent on N executions of a `mutating` plan. So a plan
+    that already carries an `executed_run_id` is refused here unless `again`
+    asks for the re-run in as many words, and the record accumulates
+    `executed_run_ids` rather than clobbering a scalar — the scalar stays as
+    the newest, which is what `find_unexecuted_plan` and the MCP driver read.
     """
     import json
+    from datetime import UTC, datetime
 
     from grapharc.planner import LoopLimits
     from grapharc.runtime.budget import Budget
@@ -343,6 +373,25 @@ def execute_plan(
     except (OSError, ValueError, KeyError) as exc:
         return fail(f"unreadable plan file {plan_file}: {exc}", as_json=as_json, command="go")
 
+    previous_run_id = record.get("executed_run_id")
+    if previous_run_id and not again:
+        # Exit 2 rather than EXIT_FAILED: nothing went wrong at run time, the
+        # command was refused before anything executed — the same shape every
+        # other "this is not what you meant" refusal in the CLI takes.
+        when = record.get("executed_at")
+        return fail(
+            f"{plan_file} has already been executed as run {previous_run_id}"
+            + (f" at {when}" if when else "")
+            + " — pass --again to run it a second time. An approval binds to "
+            "the plan's fingerprint, which does not change between runs, so a "
+            "re-run of a mutating plan spends an earlier yes.",
+            as_json=as_json,
+            command="go",
+            plan=str(plan_file),
+            executed_run_id=str(previous_run_id),
+            executed_run_ids=[str(r) for r in _executed_run_ids(record)],
+        )
+
     run_dir = plan_file.parent
     trace_path = run_dir / "trace.jsonl"
 
@@ -418,7 +467,14 @@ def execute_plan(
 
     executed = any(r.executed for r in result.rounds)
     if executed:
+        # The scalar stays the newest run — `find_unexecuted_plan` and the MCP
+        # driver read it, and an older reader must keep working. The list is
+        # what stops the record from forgetting: three executions used to leave
+        # a plan.json naming one, while the trace correctly held all three.
+        history = [*_executed_run_ids(record), result.run_id]
         record["executed_run_id"] = result.run_id
+        record["executed_run_ids"] = history
+        record["executed_at"] = datetime.now(UTC).isoformat()
         plan_file.write_text(json.dumps(record, indent=2) + "\n", encoding="utf-8")
 
     url = watch_url(trace_path, run_id=result.run_id)

tests/test_cli.py

@@ -2249,7 +2249,10 @@ def test_go_and_plan_share_every_planning_flag():
     # `--scripted` and `--go` are plan-only by design: go means do (no
     # scripted doing), and go needs no flag to do what its name says.
     assert plan_actions - go_actions == {"--scripted", "--go"}
-    assert go_actions - plan_actions == set()
+    # `--again` is go-only for the same kind of reason, in the other
+    # direction: it governs re-executing a plan that has already run, and
+    # `plan` never executes a saved one. It is not a planning flag.
+    assert go_actions - plan_actions == {"--again"}
 
 
 # -- init: the scaffold -------------------------------------------------------

tests/test_go_rerun.py

@@ -0,0 +1,204 @@
+"""`grapharc go <dir>` on a plan that has already run.
+
+Bare `go` has always skipped executed plans — `find_unexecuted_plan` passes
+over any record carrying an `executed_run_id`. The explicitly-named-directory
+form did not make the same check, so a second `go <dir>` re-ran the whole graph
+and overwrote the stamp, leaving a `plan.json` that named one run while the
+trace correctly held three.
+
+Two separate claims are under test here, and the second is the load-bearing
+one. The *record* must be able to name every run that executed the plan. And
+the *decision* must not be spent twice: an approval binds to a proposal
+fingerprint, which does not change between runs, so a silent re-run of a
+`mutating` plan executes on the strength of an earlier human yes.
+
+The planner is scripted throughout — no model backend, no network.
+"""
+
+from __future__ import annotations
+
+import json
+from pathlib import Path
+
+from grapharc.cli.main import main
+from grapharc.cli.plan import _executed_run_ids
+
+
+def _saved_plan(tmp_path, capsys) -> Path:
+    """A run directory holding an admitted, unexecuted `plan.json`."""
+    trace = tmp_path / "run" / "trace.jsonl"
+    assert main(["plan", "investigate", "--scripted", "--trace", str(trace), "--json"]) == 0
+    capsys.readouterr()  # drop the plan document
+    return trace.parent
+
+
+def _last_document(text: str) -> dict:
+    """The final JSON document in a stream that may carry several."""
+    decoder = json.JSONDecoder()
+    documents, index = [], 0
+    while index < len(text):
+        if text[index] != "{":
+            index += 1
+            continue
+        try:
+            document, index = decoder.raw_decode(text, index)
+        except json.JSONDecodeError:
+            index += 1
+            continue
+        documents.append(document)
+    assert documents, f"no JSON document in: {text[:200]!r}"
+    return documents[-1]
+
+
+def _record(run_dir: Path) -> dict:
+    return json.loads((run_dir / "plan.json").read_text(encoding="utf-8"))
+
+
+def _runs_in_trace(run_dir: Path) -> list[str]:
+    seen: list[str] = []
+    for line in (run_dir / "trace.jsonl").read_text(encoding="utf-8").splitlines():
+        if not line.strip():
+            continue
+        run_id = json.loads(line).get("run_id")
+        if run_id and run_id not in seen:
+            seen.append(run_id)
+    return seen
+
+
+# -- the refusal ------------------------------------------------------------
+
+
+def test_a_second_go_on_an_executed_plan_is_refused(tmp_path, capsys):
+    """The bug: this used to exit 0 having silently run the whole graph again."""
+    run_dir = _saved_plan(tmp_path, capsys)
+    assert main(["go", str(run_dir), "--json"]) == 0
+    first = _record(run_dir)["executed_run_id"]
+    before = _runs_in_trace(run_dir)
+    capsys.readouterr()
+
+    code = main(["go", str(run_dir), "--json"])
+
+    assert code == 2
+    payload = _last_document(capsys.readouterr().out)
+    assert payload["ok"] is False
+    assert payload["executed_run_id"] == first
+    assert "already been executed" in payload["error"]
+    assert "--again" in payload["error"]
+    # Refused before anything ran: the trace gained no run from the refusal.
+    # (It holds two — the planning run, then the one execution.)
+    assert _runs_in_trace(run_dir) == before
+
+
+def test_the_refusal_names_the_run_and_when_it_happened(tmp_path, capsys):
+    """A refusal a reader cannot act on is an obstacle, not a gate."""
+    run_dir = _saved_plan(tmp_path, capsys)
+    assert main(["go", str(run_dir), "--json"]) == 0
+    capsys.readouterr()
+
+    assert main(["go", str(run_dir)]) == 2
+
+    message = capsys.readouterr().err
+    assert _record(run_dir)["executed_run_id"] in message
+    assert _record(run_dir)["executed_at"] in message
+
+
+def test_again_executes_it_a_second_time(tmp_path, capsys):
+    """Explicit re-runs stay possible; only the silent ones stop."""
+    run_dir = _saved_plan(tmp_path, capsys)
+    assert main(["go", str(run_dir), "--json"]) == 0
+    first = _record(run_dir)["executed_run_id"]
+    capsys.readouterr()
+
+    code = main(["go", str(run_dir), "--again", "--json"])
+
+    assert code == 0
+    payload = _last_document(capsys.readouterr().out)
+    assert payload["executed"] is True
+    assert payload["run_id"] != first
+    # The planning run, then both executions.
+    assert _runs_in_trace(run_dir)[-2:] == [first, payload["run_id"]]
+
+
+# -- the record -------------------------------------------------------------
+
+
+def test_the_record_names_every_run_that_executed_the_plan(tmp_path, capsys):
+    """Three executions used to leave a plan.json naming one, while the trace
+    — the audit trail, and the one that was right — held all three."""
+    run_dir = _saved_plan(tmp_path, capsys)
+    assert main(["go", str(run_dir), "--json"]) == 0
+    assert main(["go", str(run_dir), "--again", "--json"]) == 0
+    assert main(["go", str(run_dir), "--again", "--json"]) == 0
+    capsys.readouterr()
+
+    record = _record(run_dir)
+    assert len(record["executed_run_ids"]) == 3
+    # The record now agrees with the trace, which was always right. The trace
+    # also carries the planning run that produced the plan, hence the slice.
+    assert record["executed_run_ids"] == _runs_in_trace(run_dir)[-3:]
+    # The scalar stays the newest: `find_unexecuted_plan` and the MCP driver
+    # read it, and an older reader must keep working.
+    assert record["executed_run_id"] == record["executed_run_ids"][-1]
+
+
+def test_a_plan_that_never_executed_carries_no_history(tmp_path, capsys):
+    """Absent rather than empty: a reader that tests for the key must not see
+    one appear merely because the plan was saved."""
+    run_dir = _saved_plan(tmp_path, capsys)
+
+    record = _record(run_dir)
+    assert "executed_run_id" not in record
+    assert "executed_run_ids" not in record
+    assert _executed_run_ids(record) == []
+
+
+# -- compatibility with records written before the list existed -------------
+
+
+def test_an_old_record_with_only_the_scalar_reports_its_one_run():
+    """A `plan.json` written before `executed_run_ids` existed must report the
+    run it does know about, not report none."""
+    assert _executed_run_ids({"executed_run_id": "abc123"}) == ["abc123"]
+    assert _executed_run_ids({}) == []
+    # A malformed list is a record for a human to read, not a place to raise.
+    assert _executed_run_ids({"executed_run_ids": "not-a-list"}) == []
+
+
+def test_an_old_record_is_refused_and_then_accumulates_from_its_scalar(tmp_path, capsys):
+    """The upgrade path: a pre-existing scalar-only record still refuses a
+    silent re-run, and `--again` grows the list from it rather than losing it."""
+    run_dir = _saved_plan(tmp_path, capsys)
+    record = _record(run_dir)
+    record["executed_run_id"] = "old-run-id"
+    (run_dir / "plan.json").write_text(json.dumps(record, indent=2) + "\n", encoding="utf-8")
+    capsys.readouterr()
+
+    assert main(["go", str(run_dir), "--json"]) == 2
+    capsys.readouterr()
+    assert main(["go", str(run_dir), "--again", "--json"]) == 0
+    capsys.readouterr()
+
+    grown = _record(run_dir)
+    assert grown["executed_run_ids"][0] == "old-run-id"
+    assert len(grown["executed_run_ids"]) == 2
+
+
+# -- bare `go` is unchanged -------------------------------------------------
+
+
+def test_bare_go_still_skips_an_executed_plan(tmp_path, capsys, monkeypatch):
+    """`find_unexecuted_plan` already passed over executed plans; the new guard
+    must not turn that quiet skip into a refusal."""
+    monkeypatch.chdir(tmp_path)
+    trace = tmp_path / ".grapharc" / "runs" / "r1" / "trace.jsonl"
+    assert main(["plan", "investigate", "--scripted", "--trace", str(trace), "--json"]) == 0
+    capsys.readouterr()
+
+    assert main(["go", "--json"]) == 0
+    capsys.readouterr()
+
+    # Nothing left unexecuted: the bare form reports that, rather than refusing
+    # a directory it was never given.
+    assert main(["go", "--json"]) == 1
+    payload = _last_document(capsys.readouterr().out)
+    assert "no unexecuted plan" in payload["error"]