From 1a42985ff4c9d2607a2dae4642bb3e05bb17ac2c Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Thu, 13 Aug 2026 23:35:12 +0530 Subject: [PATCH] go refuses a plan that already ran, and the record stops forgetting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bare `grapharc go` has always skipped executed plans — `find_unexecuted_plan` passes over any record carrying an `executed_run_id`. The explicitly-named form made no such check, so a second `go ` re-ran the whole graph, exit 0, and overwrote the stamp. Three executions left a plan.json naming one while the trace — the audit trail, and the one that was right — held all three. Two things were wrong and the second is the one that matters. The record disagreed with the trace. `plan.json` is what `show_graph` / `graph_status` and the MCP driver read to answer "did this plan run, and as what?", and a scalar that the next run clobbers cannot answer it. The record now accumulates `executed_run_ids`, oldest first, alongside an `executed_at` stamp. The scalar stays as the newest, because `find_unexecuted_plan` and `grapharc/mcp/driver.py` read it and a plan.json written before this change must keep working — `_executed_run_ids` falls back to it, so an old record reports its one run rather than reporting none. And one approval could be spent N times. An approval binds to a proposal fingerprint, which does not change between runs of the same saved plan, so re-issuing `go ` on a `mutating: true` plan was an agent editing the tree once per invocation on the strength of a single human yes. A plan carrying an `executed_run_id` is now refused with exit 2 — before anything executes, naming the previous run and when it happened — unless `--again` asks for the re-run in as many words. Explicit re-runs stay possible; silent ones stop. `--again` is `go`-only by design, which is why the flag-parity test in tests/test_cli.py grew a second exemption: it governs re-executing a saved plan, and `plan` never executes one. It is not a planning flag. The eight new tests in tests/test_go_rerun.py cover the refusal, the message, the `--again` escape, the accumulating record, the scalar-only upgrade path, and that bare `go` still skips quietly rather than refusing a directory it was never given. Neutering the guard and the accumulation turns four of them red. Closes #100 Co-Authored-By: Claude Opus 5 (1M context) --- grapharc/cli/main.py | 10 ++ grapharc/cli/plan.py | 56 +++++++++++ tests/test_cli.py | 5 +- tests/test_go_rerun.py | 204 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 tests/test_go_rerun.py diff --git a/grapharc/cli/main.py b/grapharc/cli/main.py index 426aabe..0640b72 100644 --- a/grapharc/cli/main.py +++ b/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 ` is refused rather than silently re-running the graph" + ), + ) _add_planning_flags(go) go.set_defaults(handler=_cmd_go) diff --git a/grapharc/cli/plan.py b/grapharc/cli/plan.py index dc86169..57484fb 100644 --- a/grapharc/cli/plan.py +++ b/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 []` — 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 ` 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) diff --git a/tests/test_cli.py b/tests/test_cli.py index 66df4f4..52205f3 100644 --- a/tests/test_cli.py +++ b/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 ------------------------------------------------------- diff --git a/tests/test_go_rerun.py b/tests/test_go_rerun.py new file mode 100644 index 0000000..7201464 --- /dev/null +++ b/tests/test_go_rerun.py @@ -0,0 +1,204 @@ +"""`grapharc go ` 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 ` 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"]