tests: the deep dive's figure refreshes itself on request - #119
Merged
Merged
Conversation
The guard is right and stays strict: a number on the **Verified this pass** line is re-derived by a test, or it does not belong on the line. What was wrong was who paid for it. Every branch that adds or removes a test moves the count, so this file failed that branch until someone hand-edited a figure in a docs file they had no reason to know existed. PR #115 is what that costs. An outside contributor's first change -- four tests, the fan-out example #51 asked for -- sat red for a month on `assert 2151 == 2155`, and nothing in the failure named a fix they could run. The check did its job; the ergonomics did not. `GRAPHARC_UPDATE_FIGURES=1 pytest tests/test_deep_dive.py` now rewrites the line and skips the check that wrote it, so the next run is the one that verifies. The failure message names that command, and CONTRIBUTING.md has a section for the case, because a contributor reads the failure and the contributing guide, not this file's docstring. Strictness is unchanged where it counts. `_updating()` is false for unset, empty, "0", "false" and "no", so a leftover `=0` in a shell profile cannot disarm the guard, and CI sets nothing -- a stale figure still fails there. That property has its own test, because a self-healing check in CI would assert nothing at all. The rewrite is a pure function over the line, tested without touching the real document: only the capture group's span changes, so the comma grouping and every surrounding word stay byte-identical. The version the paragraph says is on PyPI is deliberately not rewritten -- whether a release is published is not something this tree can re-derive, and a note claiming it should be written by whoever released it. Verified: 2156 selected, 13 deselected, ruff clean. Dogfooded -- the figure in this commit was written by the mechanism it adds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Shashankss1205
added a commit
that referenced
this pull request
Sep 25, 2026
) * mcp: the execute timeout no longer severs an approved mutating run Three defects on one path, all from issue #113. **One budget covered the park and the work.** `approval_timeout + 120s` bounded the whole subprocess, so a human approving near the end of the park left roughly two minutes for the run itself -- and a governed run's agent phases delegate to Claude Code, which reads files, edits them and verifies. The likely outcome was not a wedged process being cleaned up, it was a human-approved, tree-mutating run being SIGKILLed partway through mutating the tree. The non-mutating branch passed `timeout=None` and was not bounded at all. Separate budgets now: `DEFAULT_WORK_TIMEOUT` is 1800s, the number `GRAPHARC_SLACK_WORK_TIMEOUT` already uses for this exact case, overridable via `GRAPHARC_MCP_WORK_TIMEOUT` -- which refuses a non-numeric or non-positive value rather than substituting a ceiling nobody chose. **plan.json could not catch the reissue.** `executed_run_id` is stamped after `loop.run()` returns, so a killed run never reaches it; the record said "never executed" and the #100 guard waved a second `go` through on one human approval. The guard now reads the trace, which is written as the run proceeds and is the only place that evidence survives a kill. Deliberately not a pre-stamp: that would mark a plan executed when it had merely parked and been denied. The phase vocabulary is pinned by a test, because the obvious spelling -- "not one of `observe.metrics`'s loop phases" -- counts `approval_response` as an execution and so refuses a denied plan forever. A torn trace reports no evidence rather than wedging the plan beside it. **The kill left grandchildren.** No new session and `process.kill()` signals the direct child only, so a delegated Claude Code process outlived the run and kept the workspace. `start_new_session=True` plus a process-group SIGKILL, falling back to the single process where `killpg` is unavailable. The docstring promised "a timeout leaves the plan unexecuted and this call safe to reissue", true of an unanswered park and false of everything else. It distinguishes them now and sends the agent to `show_graph` first; the `DriverError` says the same. Both asserted -- that docstring is the agent's whole briefing. Verified: 2171 selected, 13 deselected, ruff clean. Red without the fix (reverting only `grapharc/` fails the reissue test `assert 0 == 2`), while the two over-refusal guards pass on both sides. deep-dive.md's figure updated 2,151 -> 2,171. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * mcp: the phase vocabulary gets one owner, and bare `go` gets a test Two gaps in the fix, found reviewing it rather than running it. **The vocabulary was duplicated.** `cli.plan` carried its own copy of which phases are bookkeeping, alongside the copy `observe.metrics` already had for its own purpose. A phase classified one way in one file and the other way in the other is a bug in whichever is wrong, with nothing in the tree to say which -- and the two are read for different questions, so the copies would have drifted quietly. `LOOP_PHASES`, `SHAPE_PHASES`, `DRIVER_PHASES` and `began_execution()` now live in `observe.trace`, beside the `TraceEvent` they classify; both callers defer to them, and a test asserts identity rather than equality, because two frozensets that happen to match today is the state that assertion exists to rule out. The docstring now states the direction the predicate errs in, which was implicit before: **an unlisted phase reads as an execution.** That is the safe way round. A new bookkeeping phase makes `go` refuse a plan it could have run, recoverable with `--again`; the opposite would re-run a half-finished mutating plan and spend a human approval that was given once. **Bare `go` was covered but not pinned.** `grapharc plan … && grapharc go` is the flow the README teaches, and bare `go` reaches its plan by a different route: `find_unexecuted_plan` passes over any record with an `executed_run_id`, and a killed run never wrote one, so a half-finished plan looks *unexecuted* to the selector and is the newest candidate. The refusal lands after the selection, so it holds -- verified against main, where the same scenario exits 0 and puts a third run in the trace. It holds for a reason a refactor of the selector could undo without touching the guard, so it now has a test of its own. Verified: 2173 selected, 13 deselected, ruff clean. The metrics refactor changes no drawing behaviour -- its local names alias the shared ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * observe: the other two copies of the phase vocabulary, and a guard on the count The previous commit claimed the vocabulary had one owner. It had three: `observe.viewmodel` kept its own pair -- under a comment reading "mirrors `metrics`", which was true and was precisely the problem -- and `slack.live` kept its own `_SHAPE_PHASES`. Both now defer to `observe.trace`, so the four modules that ask which phases are bookkeeping get one answer. The identity assertions only cover names a test knows to look for, which cannot notice a fifth copy appearing under a new name. So there is a second check that reads the source: exactly one file in the package may define these sets. It asserts on *files* rather than file:line, because a line number would fail on any unrelated edit to trace.py, which is noise and not a finding -- the first version of it did exactly that, and it also missed `DRIVER_PHASES`. The predicate's direction moves into a test of its own rather than riding along at the end of another, and that test also asserts the three groups partition the bookkeeping set with nothing dropped between them. No behaviour change in any of the four: every local name aliases the shared object, and the drawing and feed logic reads as it did. Verified: 2175 selected, 13 deselected, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: the figure after the merge, refreshed by the new command 2,180 selected: main's 2,151 plus the five tests #119 added and this branch's own. Written by GRAPHARC_UPDATE_FIGURES rather than by hand, which is the first use of #119 on the kind of conflict it exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Shashankss1205
added a commit
that referenced
this pull request
Sep 26, 2026
2,214: the four tests the parallel research registry brought with it. Pushed straight to main rather than through a pull request because #115's own branch could not carry it. The figure has to equal the count *after* the merge, and their branch was one commit of docs away from being green -- but writing to a fork's branch needs a git push, and fetching that branch here is not something this environment permits. So the choice was a briefly-red main or a contributor waiting a second month on a number in a docs file. Their four tests and the registry were green on every Python version; this line was the only failure. Written by GRAPHARC_UPDATE_FIGURES, which is what #119 added for exactly this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem, stated fairly
The guard is right. A number on the Verified this pass line is re-derived by a test, or it does not belong on the line — that is the rule #105 settled, and this PR does not touch it.
What was wrong is who paid for it. Every branch that adds or removes a test moves the count, so
tests/test_deep_dive.pyfailed that branch until someone hand-edited a figure in a docs file they had no reason to know existed.#115 is what that costs. An outside contributor's first change — four tests, the fan-out example #51 asked for — sat red for a month on
assert 2151 == 2155. The failure named the file but no action they could take. My own #117 hit the same wall.The change
Rewrites the line, skips the check that wrote it (so the next run is the one that verifies), and the ordinary failure message now names that command.
CONTRIBUTING.mdgets an "If you added or removed a test" section, because a contributor reads the failure and the contributing guide — not this file's docstring.Strictness is unchanged where it matters
_updating()is false for unset, empty,"0","false"and"no". CI sets nothing, so a stale figure still fails there. That property has its own test — a self-healing check in CI would assert nothing at all, which would be a worse outcome than the friction this PR removes.The rewrite is a pure function over the line and is tested without touching the real document: only the capture group's span changes, so comma grouping and every surrounding word stay byte-identical. There is a test that it refuses a line it cannot find the figure in, because silently writing nothing would leave a stale figure looking refreshed.
The version the paragraph says is on PyPI is deliberately not rewritten this way. Whether a release is published is not something the tree can re-derive, and a note claiming it should be written by whoever released it. Its failure message says so.
Verification
ruff check .clean.docs/deep-dive.mdis one number.Note on merge order
This branch is cut from
main, so its figure reads 2,156 (2,151 + the 5 tests here). #117 adds tests too. Whichever merges second will need one figure refresh — with this merged, that is the one command above rather than a hand edit.🤖 Generated with Claude Code