Skip to content

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

Merged
Shashankss1205 merged 1 commit into
mainfrom
fix/issue-100-go-rerun-guard
Aug 13, 2026
Merged

go <dir> refuses a plan that already ran, and the record stops forgetting#104
Shashankss1205 merged 1 commit into
mainfrom
fix/issue-100-go-rerun-guard

Conversation

@Shashankss1205

Copy link
Copy Markdown
Collaborator

Closes #100.

The bug

Bare grapharc go has always skipped executed plans — find_unexecuted_plan passes over any record carrying an executed_run_id. The explicitly-named-directory form made no such check:

grapharc plan "look into the outage" --scripted --trace r5/p.jsonl
grapharc go r5   # exit 0
grapharc go r5   # exit 0, runs the whole graph again
grapharc go r5   # exit 0, and again
plan.json executed_run_id : f81e7318e9dc
runs in the trace         : ['b563daf1c7db', '1f792472a215', 'f81e7318e9dc']
3 executions happened; plan.json names 1

Two fixes, and the second is the load-bearing one

The record accumulates. executed_run_ids (oldest first) plus an executed_at stamp. The scalar stays as the newest — 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 the scalar, so an old record reports its one run rather than reporting none; a malformed list degrades to the same fallback rather than raising, because this is a record for a human to read.

One approval can no longer be spent N times. An approval binds to a proposal fingerprint, which does not change between runs of the same saved plan. Re-issuing go <dir> 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 its timestamp — unless --again asks for the re-run explicitly.

Bare grapharc go behaves exactly as before: it skips executed plans quietly rather than refusing a directory it was never given.

The one test that had to change

test_go_and_plan_share_every_planning_flag asserts go_actions - plan_actions == set(). --again is go-only by design — it governs re-executing a saved plan, and plan never executes one, so it is not a planning flag. The exemption is recorded in the test with its reason, mirroring how --scripted / --go are already recorded as plan-only.

Verification

  • uv run pytest — full suite green (exit 0).
  • uv run ruff check grapharc tests — clean.
  • The new tests can go red: neutering the guard and the accumulation while leaving the helper importable turns 4 of the 8 red (test_a_second_go_on_an_executed_plan_is_refused, test_the_refusal_names_the_run_and_when_it_happened, test_the_record_names_every_run_that_executed_the_plan, test_an_old_record_is_refused_and_then_accumulates_from_its_scalar).

Out of scope, as the issue asks: the fingerprint protocol, and whether a re-run of a mutating plan should re-park even without --approve.

🤖 Generated with Claude Code

…ting

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 <run-dir>` 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 <dir>` 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) <noreply@anthropic.com>
@Shashankss1205
Shashankss1205 merged commit 7969a85 into main Aug 13, 2026
6 checks passed
@Shashankss1205
Shashankss1205 deleted the fix/issue-100-go-rerun-guard branch August 13, 2026 18:19
Shashankss1205 added a commit that referenced this pull request Aug 13, 2026
…ht tests

Which is the mechanism working as intended — merging a PR that adds tests
made the deep dive's figure stale, and the check caught it rather than
letting it sit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Shashankss1205 added a commit that referenced this pull request Aug 13, 2026
…e not on the line (#105)

* The deep dive's verified figures are re-derived by a test, or they are not on the line

The **Verified this pass** paragraph closes the deep dive with concrete
numbers, and nothing checked them, so they drifted twice: it read "1,533
passed … 103 submodules" against a tree with 1,754 tests and 116 submodules,
and it read "1,985 passed, 12 deselected" against a tree with 2,137 selected
and 13 live. The paragraph's whole value is that its numbers are real. A
reader who spots one stale figure discounts every other verified claim on the
page, including the ones the suite genuinely enforces.

The figures are now quoted as what one command re-derives — how many tests
`pytest` selects, and how many it holds back as `live` — rather than as a pass
count. That reword is the point, not cosmetics: a pass count cannot be
re-derived without running the suite from inside itself, which is exactly how
"1,985 passed" came to be a number no test owned. The suite being green is
asserted by the suite being green.

tests/test_deep_dive.py re-derives both in one collection pass, in a
subprocess — this module is collected by the session doing the asking, so
re-entering the collector in-process is not on. `-m ""` clears the addopts
`-m 'not live'` and the marker is read off each item, so one pass yields both
figures instead of two passes yielding one each; it costs about two seconds.

Two guards sit behind the two comparisons, both closing ways the check could
pass while saying nothing. One asserts the figures are still quoted at all, so
deleting a number makes the test red rather than vacuous — the trap
`tests/test_readme.py` already closes for its fenced blocks. The other asserts
no *unowned* figure has appeared on the line: any bare count with a unit that
this file does not re-derive fails, with wording that says to add a check or
take the number off. That is the rule the issue settled on.

The version the paragraph says is on PyPI is held against pyproject's, for the
same reason `ci.yml` already refuses a `grapharc.__version__` that disagrees
with it: a release note naming a third number is that failure with no check.

Restoring the historical drift turns both the comparison and the unowned-figure
guard red.

Closes #42

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Rebase on main: the count the test re-derives now includes #104's eight tests

Which is the mechanism working as intended — merging a PR that adds tests
made the deep dive's figure stale, and the check caught it rather than
letting it sit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Shashankss1205 Shashankss1205 mentioned this pull request Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

go <dir> re-executes an already-executed plan silently, and plan.json forgets the earlier runs

1 participant