Skip to content

Commit 21bc764

Browse files
Make the shipped plan trace complete and stop counting planner spend twice
The shipped registry withheld the trace recorder from its PlannerNode and Materializer, so `grapharc plan` wrote a file holding only admission/round/ stop: no plan event saying what was proposed and what it cost, and — because the built subgraph inherits the materializer's recorder — no start/end pair for any node the loop executed. Three nodes ran and none of them appeared. README's "the trace holds ... the executed nodes' own start/end pairs" was true of a hand-wired loop and false of the one the command drives. Both collaborators get the recorder now, and the phase counts are asserted. With the plan event present, a second defect became live: the round event also carried the planner's tokens, and metrics, cost and replay all add events they cannot place inside a node on top of node totals. The planner's spend was therefore counted once as `plan` and again as `round` — meter 1979, metrics 2558. A round's duration_ms was worse, since it encloses the plan plus every node the round ran. Neither is on the event now; both are on its state_delta as round_tokens / round_iterations / round_duration_ms, where no reader sums them, so what a round spent stays answerable from the file without being added to the totals a second time. RoundRecord.iterations was declared and never assigned, so every round reported 0 while the run's meter counted the same work. _charge_back already read the figure and discarded it; it is carried through _Execution now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 82900ed commit 21bc764

4 files changed

Lines changed: 146 additions & 11 deletions

File tree

grapharc/examples/plan_incident.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,16 @@ def build_loop(
163163
# the same object, and a node body could otherwise widen it between rounds.
164164
registry.freeze()
165165
return GovernedLoop(
166-
planner=PlannerNode(model, name="incident", catalog=registry.catalog()),
166+
# The planner and the materializer get the recorder too. Without it the
167+
# run's own trace held only `admission`/`round`/`stop`: no `plan` event
168+
# saying what was proposed and what it cost, and — because the built
169+
# subgraph inherits the materializer's recorder — no `start`/`end` pair
170+
# for any node the loop actually executed. README's "the executed nodes'
171+
# own start/end pairs" was true of a hand-wired loop and false of the
172+
# shipped one, which is the one `grapharc plan` drives.
173+
planner=PlannerNode(
174+
model, name="incident", catalog=registry.catalog(), trace=trace
175+
),
167176
checker=AdmissionChecker(
168177
registry=registry,
169178
edge_policy=edge_policy or default_edge_policy(),
@@ -173,6 +182,7 @@ def build_loop(
173182
registry=registry,
174183
state_schema=state_schema or IncidentState,
175184
writes=writes if writes is not None else WRITES,
185+
trace=trace,
176186
),
177187
budget=budget,
178188
limits=limits,

grapharc/planner/loop.py

Lines changed: 37 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,10 @@ class _Execution(NamedTuple):
248248
materialization_error: str = ""
249249
execution_error: str = ""
250250
hard_stop: LoopStop | None = None
251+
# The sub-run's iteration count, so the round can report what it spent.
252+
# `_charge_back` folds this into the run's meter either way; carrying it here
253+
# is what lets `RoundRecord.iterations` hold a figure instead of always 0.
254+
iterations: int = 0
251255

252256
@property
253257
def failure(self) -> str:
@@ -482,6 +486,7 @@ def close(**fields: Any) -> None:
482486
execution_error=attempt.execution_error,
483487
executed=attempt.executed,
484488
progressed=progressed,
489+
iterations=attempt.iterations,
485490
)
486491

487492
if stop is None:
@@ -593,15 +598,22 @@ def _execute(
593598
budget=budget,
594599
)
595600
except BudgetExceeded as exc:
596-
self._charge_back(compiled, meter)
601+
iterations = self._charge_back(compiled, meter)
597602
return _Execution(
598-
state=state, execution_error=exc.reason, hard_stop=LoopStop.BUDGET_EXHAUSTED
603+
state=state,
604+
execution_error=exc.reason,
605+
hard_stop=LoopStop.BUDGET_EXHAUSTED,
606+
iterations=iterations,
599607
)
600608
except Exception as exc: # noqa: BLE001 - a failed subgraph is a replanning input
601-
self._charge_back(compiled, meter)
602-
return _Execution(state=state, execution_error=f"raised {exc!r}")
603-
self._charge_back(compiled, meter)
604-
return _Execution(executed=True, state=self._initial_state(raw))
609+
iterations = self._charge_back(compiled, meter)
610+
return _Execution(
611+
state=state, execution_error=f"raised {exc!r}", iterations=iterations
612+
)
613+
iterations = self._charge_back(compiled, meter)
614+
return _Execution(
615+
executed=True, state=self._initial_state(raw), iterations=iterations
616+
)
605617

606618
def _round_budget(self, meter: BudgetMeter) -> Budget:
607619
"""This round's ceiling: exactly what the run has left, per dimension.
@@ -618,18 +630,21 @@ def _round_budget(self, meter: BudgetMeter) -> Budget:
618630
)
619631

620632
@staticmethod
621-
def _charge_back(compiled: Any, meter: BudgetMeter) -> None:
633+
def _charge_back(compiled: Any, meter: BudgetMeter) -> int:
622634
"""Fold the round's spend into the run's meter, whether or not it finished.
623635
624636
The sub-run's meter is a separate accountant with its own ceiling; the
625637
loop's meter never saw those calls, so this charge is new spend rather
626638
than a re-report and is counted unnamed on purpose.
639+
640+
Returns the sub-run's iteration count, for the round to record.
627641
"""
628642
sub = getattr(compiled, "last_run", None)
629643
if sub is None:
630-
return
644+
return 0
631645
meter.charge_tokens(sub.meter.tokens)
632646
meter.charge_iteration(sub.meter.iterations)
647+
return sub.meter.iterations
633648

634649
# -- recording ------------------------------------------------------------
635650

@@ -648,8 +663,14 @@ def _record(
648663
ctx,
649664
node=f"{self.name}:round{record.round}",
650665
phase="round",
651-
duration_ms=record.duration_ms,
652-
tokens=record.tokens,
666+
# No `tokens=` and no `duration_ms=`: a round is an *envelope*, not a
667+
# measurement. Its tokens are the planner's, already reported by the
668+
# `plan` event, and its duration encloses that plan plus every node
669+
# the round executed. `metrics`, `cost` and `replay` sum node totals
670+
# *plus* every event they cannot place inside a node — and `round` is
671+
# one of those — so reporting either here counted the same spend
672+
# twice. `RoundRecord` still carries both for callers reading the
673+
# returned `LoopResult`; what changes is only what lands on the trace.
653674
state_delta={
654675
"round": record.round,
655676
"proposal_id": record.proposal.proposal_id if record.proposal else "",
@@ -660,6 +681,12 @@ def _record(
660681
"executed": record.executed,
661682
"progressed": record.progressed,
662683
"stop": stop.value if stop is not None else "",
684+
# The envelope's own figures, under names no reader sums. What a
685+
# round spent stays answerable from the file; what it spent is
686+
# just no longer added to the totals a second time.
687+
"round_tokens": record.tokens,
688+
"round_iterations": record.iterations,
689+
"round_duration_ms": round(record.duration_ms, 2),
663690
},
664691
error=problem or None,
665692
)

tests/test_cli.py

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1011,6 +1011,46 @@ def test_plan_runs_the_governed_loop_and_reports_every_round(tmp_path, capsys):
10111011
assert "round 2: admitted" in out
10121012

10131013

1014+
def test_plan_traces_the_plan_and_every_node_it_executed(tmp_path, capsys):
1015+
"""README's "the trace holds an `admission` event per round, a `round` event
1016+
per round, the executed nodes' own `start`/`end` pairs and one `stop` event".
1017+
1018+
That was true of a hand-wired loop and false of the shipped registry, which is
1019+
the one this command drives: it withheld the recorder from the `PlannerNode`
1020+
and the `Materializer`, so the file held no `plan` event and — because the
1021+
built subgraph inherits the materializer's recorder — no `start`/`end` pair
1022+
for any node the loop ran. Three nodes executed and none of them appeared.
1023+
"""
1024+
from grapharc.observe import cost, metrics
1025+
from grapharc.observe.trace import TraceRecorder
1026+
1027+
path = tmp_path / "t.jsonl"
1028+
code, payload, _ = call_json(["plan", "look into the outage", "--trace", str(path)], capsys)
1029+
assert code == 0
1030+
1031+
recorder = TraceRecorder(path)
1032+
events = recorder.read_events()
1033+
run_id = events[0].run_id
1034+
phases = {phase: 0 for phase in ("plan", "admission", "round", "start", "end", "stop")}
1035+
for event in events:
1036+
if event.phase in phases:
1037+
phases[event.phase] += 1
1038+
1039+
assert phases["plan"] == 2, "the planner's own event is missing"
1040+
assert phases["admission"] == 2
1041+
assert phases["round"] == 2
1042+
assert phases["stop"] == 1
1043+
# The admitted round ran three nodes; each owes a start/end pair.
1044+
assert phases["start"] == 3
1045+
assert phases["end"] == 3
1046+
1047+
summary = metrics.summarize(recorder, run_id)
1048+
assert summary.nodes_executed == 3
1049+
assert set(summary.per_node) == {"triage", "patch", "verify"}
1050+
# And the two readers of that one file still agree with each other.
1051+
assert cost.attribute(recorder, run_id).tokens == summary.tokens
1052+
1053+
10141054
def test_plan_json_carries_the_rounds_and_the_stop_reason(tmp_path, capsys):
10151055
code, payload, _ = call_json(
10161056
["plan", "look into the outage", "--trace", str(tmp_path / "t.jsonl")], capsys

tests/test_planner_loop.py

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -821,6 +821,64 @@ def test_a_run_plans_admits_executes_and_reaches_the_goal(trace):
821821
assert result.usage["tokens"] > 0
822822

823823

824+
def test_a_round_records_the_iterations_it_spent(trace):
825+
"""`RoundRecord.iterations` was declared and never assigned, so every round
826+
reported 0 while the run's meter counted the same work."""
827+
loop, _model, _bodies = build_loop(
828+
[plan(("read", "fetch"), ("write", "summarise"))],
829+
trace=trace,
830+
goal_reached=goal_is_done,
831+
)
832+
833+
result = loop.run("summarise the incident", LoopState())
834+
835+
assert [r.iterations for r in result.rounds] == [2]
836+
assert sum(r.iterations for r in result.rounds) == result.usage["iterations"]
837+
838+
839+
def test_the_planners_tokens_are_counted_once_not_once_per_round(trace):
840+
"""A `round` event is an envelope, not a measurement.
841+
842+
Its tokens are the planner's, already reported by the `plan` event, and both
843+
land in the set `metrics`/`cost` add on top of node totals — so the planner's
844+
spend was charged to the report twice.
845+
"""
846+
from grapharc.observe import cost, metrics
847+
848+
loop, _model, _bodies = build_loop(
849+
[plan(("read", "fetch"), ("write", "summarise"))],
850+
trace=trace,
851+
goal_reached=goal_is_done,
852+
)
853+
result = loop.run("summarise the incident", LoopState(), run_id="r1")
854+
855+
real = result.usage["tokens"]
856+
assert real > 0
857+
assert metrics.summarize(trace, "r1").tokens == real
858+
assert cost.attribute(trace, "r1").tokens == real
859+
860+
861+
def test_a_round_event_still_reports_what_the_round_itself_spent(trace):
862+
"""Removing the double count must not remove the information."""
863+
loop, _model, _bodies = build_loop(
864+
[plan(("read", "fetch"), ("write", "summarise"))],
865+
trace=trace,
866+
goal_reached=goal_is_done,
867+
)
868+
loop.run("summarise the incident", LoopState(), run_id="r1")
869+
870+
rounds = [e for e in trace.read_events("r1") if e.phase == "round"]
871+
assert rounds, "the round event went missing"
872+
for event in rounds:
873+
# Not in the typed fields any reader sums...
874+
assert event.tokens is None
875+
assert event.duration_ms is None
876+
# ...but still answerable from the file.
877+
assert event.state_delta["round_tokens"] > 0
878+
assert event.state_delta["round_iterations"] == 2
879+
assert event.state_delta["round_duration_ms"] >= 0
880+
881+
824882
def test_a_rejection_reaches_the_planner_and_the_next_proposal_completes_the_run():
825883
"""The replanning edge of ARCHITECTURE §2: rejected + reason -> propose again."""
826884
loop, model, bodies = build_loop(

0 commit comments

Comments
 (0)