From 82900ed6d3f94a9726bfe6be8caa11056d4ebc80 Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Thu, 30 Jul 2026 04:42:08 +0530 Subject: [PATCH 1/6] Fix three runtime defects that silently corrupted metering and parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unreachable max_seconds poisoned the deadline guard process-wide. setitimer raises OverflowError past the platform's time_t, and it raises *after* the SIGALRM handler is installed and the process-wide slot taken — both were left that way, so every later guard found the slot held and fell back to the async-exception mechanism, which cannot unwind a blocking syscall. Measured: a 0.3s ceiling honoured at 0.30s, then at 5.00s after a single 1e10 run. Arming is undone on failure now, and the armed delay is clamped to what both mechanisms accept. Every async def node double-charged its token re-reports. The re-report ledger was keyed by threading.get_ident(), but on_llm_end is sync, so under ainvoke LangChain dispatches it to a worker thread while the body stays on the event loop. The automatic charge found no ledger, never recorded the call, and the node's named re-report — the documented free path — was charged again. Any node using charge_usage, AgentNode._charge_tokens or planner.proposal._charge reported double its real spend and hit max_tokens at half its declared allowance. The ledger is a contextvars scope now, verified to propagate across that hop; nesting also stops discarding the enclosing node's ledger. A bracket in the model's prose hijacked JSON extraction, because only the first { or [ was ever tried. "Based on the context [lines 3-5]: {...}" was rejected as unparseable, and — worse — 'Analysis (note [1]): {"supported": false}' returned a valid [1], substituting a fabricated value for the verifier's actual answer. Every opener is tried now and the longest parse wins, which also prefers a complete structure over a nested fragment. Junk still returns None, so the caller's fail-closed path is unchanged. A node's end event also reported the movement of the run's *shared* meter rather than its own spend, so overlapping fan-out workers each absorbed their siblings' concurrent charges: three workers costing 8 tokens each traced as 24/16/8, and metrics and cost both reported 48 for 24 tokens of real work, doubling the estimated bill purely because the work ran in parallel. Attribution now comes from a per-node scope on the meter. Each fix ships a test that fails without it, confirmed by stashing the source and watching them go red. Co-Authored-By: Claude Opus 5 (1M context) --- grapharc/runtime/budget.py | 153 +++++++++++++++++++++++++------ grapharc/runtime/graph.py | 11 ++- grapharc/runtime/parsing.py | 37 ++++++-- grapharc/runtime/usage.py | 14 ++- tests/test_budget_enforcement.py | 96 +++++++++++++++++++ tests/test_parsing.py | 27 ++++++ tests/test_replay.py | 94 +++++++++++++++++++ 7 files changed, 395 insertions(+), 37 deletions(-) diff --git a/grapharc/runtime/budget.py b/grapharc/runtime/budget.py index 528d151..8ee4761 100644 --- a/grapharc/runtime/budget.py +++ b/grapharc/runtime/budget.py @@ -13,11 +13,12 @@ from __future__ import annotations +import contextvars import ctypes import signal import threading import time -from collections.abc import Iterator +from collections.abc import Callable, Iterator from contextlib import contextmanager from typing import Any @@ -71,7 +72,7 @@ def _call_key(source: Any) -> Any: class _MeteredCalls: - """The model calls the usage callback charged on one thread, by identity. + """The model calls the usage callback charged in one node scope, by identity. Not a count of tokens: a re-report is recognised because it names the same call, never because it happens to be the same number of tokens. @@ -101,6 +102,30 @@ def claim(self, source: Any) -> bool: return True +class _NodeScope: + """One node execution's re-report ledger and its own share of the spend. + + `tokens` exists because the run's meter is shared: subtracting a + before/after reading off it attributed every *sibling's* concurrent spend to + whichever fan-out worker happened to be running at the time. This counts only + the charges made inside this scope, so the same work costs the same whether + it runs in parallel or one at a time. + """ + + __slots__ = ("ledger", "tokens") + + def __init__(self) -> None: + self.ledger = _MeteredCalls() + self.tokens = 0 + + +# The node scope a re-report may claim a metered call from, as (meter, scope). +# Context-scoped rather than thread-scoped: see `BudgetMeter.automatic_scope`. +_METERED_SCOPE: contextvars.ContextVar[tuple[Any, _NodeScope] | None] = ( + contextvars.ContextVar("grapharc_metered_scope", default=None) +) + + class BudgetMeter: """Thread-safe per-run usage accountant. @@ -116,7 +141,6 @@ def __init__(self, budget: Budget) -> None: self._iterations = 0 self._tokens = 0 self._started_at = time.monotonic() - self._metered: dict[int, _MeteredCalls] = {} def charge_iteration(self, n: int = 1) -> None: with self._lock: @@ -142,39 +166,80 @@ def charge_tokens(self, n: int, *, automatic: bool = False, source: Any = None) run early and is visible in `snapshot()`; under-reporting is invisible and arrives on the bill. Pass `source=` to be exact. - Two shipped callers still charge an unnamed integer for a call the - callback already metered — `grapharc.testing.charge_usage` and - `AgentNode._charge_tokens` — so a node using either pays twice. Both - hold the message: `charge_tokens(total, source=message)` fixes them, and - keeps working when they run outside a graph, where nothing metered the - call and the charge must land. + All three shipped re-reporters now name their source — `testing. + charge_usage`, `AgentNode._charge_tokens` and `planner.proposal._charge` + each pass `source=message` — so none of them pays twice, and each still + works outside a graph, where nothing metered the call and the charge must + land. + + What the source is matched against is a *node scope*, not a thread; see + `automatic_scope` for why that distinction was load-bearing. """ with self._lock: - metered = self._metered.get(threading.get_ident()) + scope = self._current_scope() if automatic: self._tokens += n - if metered is not None and source is not None: - metered.record(source) + self._attribute(scope, n) + if scope is not None and source is not None: + scope.ledger.record(source) return - if source is not None and metered is not None and metered.claim(source): + if source is not None and scope is not None and scope.ledger.claim(source): return self._tokens += n + self._attribute(scope, n) + + @staticmethod + def _attribute(scope: _NodeScope | None, n: int) -> None: + """Credit `n` to the node scope that spent it. Only charges that actually + landed on the run total get here, so a dropped re-report is not counted.""" + if scope is not None: + scope.tokens += n + + def _current_scope(self) -> _NodeScope | None: + """This node execution's scope, or None outside any. Caller holds the lock.""" + entry = _METERED_SCOPE.get() + # The meter is part of the key so a scope opened by one meter — the + # planner runs a sub-meter inside the run's — cannot lend its scope to + # a charge made against another. + return entry[1] if entry is not None and entry[0] is self else None + + def scope_tokens(self) -> int | None: + """Tokens charged inside the current node scope, or None outside one. + + What a single node execution actually spent, as opposed to how the run's + shared total moved while it happened to be running. + """ + with self._lock: + scope = self._current_scope() + return None if scope is None else scope.tokens @contextmanager def automatic_scope(self) -> Iterator[None]: """Bound the window in which a metered call may be re-reported for free. - Scoped per thread and opened once per node, so a re-report in one node - can never claim a call metered by another. + Scoped to the calling *context* and opened once per node, so a re-report + in one node can never claim a call metered by another. + + A `contextvars` scope rather than a thread-keyed one, because the runtime's + usage callback does not always run on the thread the node body runs on. + `on_llm_end` is sync, so under `ainvoke`/`astream` LangChain dispatches it + to a worker thread while the body stays on the event loop. Keyed by thread + ident, the automatic charge then found no ledger, never recorded the call, + and the node's *named* re-report — the documented free path — was charged a + second time: every `async def` node using `charge_usage`, `AgentNode. + _charge_tokens` or `planner.proposal._charge` reported double its real + spend and hit `max_tokens` at half its declared allowance. LangChain copies + the context across that hop, so the callback and the body share one ledger. + + Contexts also nest properly. `reset(token)` restores an enclosing scope's + ledger, where popping a thread-keyed entry discarded it — so an inner scope + used to make the outer node's remaining re-reports pay twice. """ - ident = threading.get_ident() - with self._lock: - self._metered[ident] = _MeteredCalls() + token = _METERED_SCOPE.set((self, _NodeScope())) try: yield finally: - with self._lock: - self._metered.pop(ident, None) + _METERED_SCOPE.reset(token) @property def iterations(self) -> int: @@ -255,6 +320,15 @@ def snapshot(self) -> dict[str, float | int]: # node alive; the cost while a node is being torn down is one timer per 50ms. _REARM_SECONDS = 0.05 +# The longest delay both mechanisms can actually be armed with. `setitimer` +# raises `OverflowError` past the platform's `time_t` (~2**31 seconds), and +# `threading.Timer` accepts a larger value but crashes its own thread once the +# underlying `wait` exceeds `threading.TIMEOUT_MAX`. A `max_seconds` beyond this +# is ~68 years, which no process reaches, so clamping the *armed delay* costs no +# enforcement: the deadline is still computed from the meter, and the guard's +# exit-time check still refuses a node that overran. +_MAX_ARMABLE_SECONDS = min(2.0**31 - 1, threading.TIMEOUT_MAX) + def _async_raise(thread_id: int, exc: type[BaseException] | None) -> None: """Queue `exc` in another thread, or clear a queued one when `exc` is None.""" @@ -329,18 +403,29 @@ def detail() -> str: state: dict[str, Any] = {"armed": True, "fired": False, "timer": None} lock = threading.Lock() thread_id = threading.get_ident() - use_signal = _signal_slot_available() and _SIGNAL_SLOT.acquire(blocking=False) - - if use_signal: + # What the timers are armed with, as opposed to what the deadline *is*. + armable = min(remaining, _MAX_ARMABLE_SECONDS) + + def arm_signal() -> Callable[[], None] | None: + """Arm SIGALRM and return its disarm, or return None to use mechanism 2. + + Arming is undone on failure rather than left half-done. `setitimer` + rejects a `remaining` beyond the platform's `time_t` — `float("inf")`, + or a plausible "effectively unlimited" like `1e10` — and it raises + *after* the handler is installed and the slot is taken. Letting that + propagate leaked both for the life of the process: every later guard + found the slot held and silently degraded to mechanism 2, which cannot + unwind a blocking syscall, and a stray SIGALRM anywhere in the program + would raise `NodeDeadlineExceeded` citing a finished run's meter. + """ + if not (_signal_slot_available() and _SIGNAL_SLOT.acquire(blocking=False)): + return None def on_alarm(signum: int, frame: object) -> None: state["fired"] = True raise NodeDeadlineExceeded(detail()) previous_handler = signal.signal(signal.SIGALRM, on_alarm) - # The third argument is the repeat interval: the kernel re-arms the - # alarm for us, so a swallowed SIGALRM is followed by another one. - signal.setitimer(signal.ITIMER_REAL, remaining, _REARM_SECONDS) def disarm() -> None: # An alarm landing mid-disarm raises out of `setitimer`, so the @@ -350,6 +435,20 @@ def disarm() -> None: finally: signal.signal(signal.SIGALRM, previous_handler) _SIGNAL_SLOT.release() + + try: + # The third argument is the repeat interval: the kernel re-arms the + # alarm for us, so a swallowed SIGALRM is followed by another one. + signal.setitimer(signal.ITIMER_REAL, armable, _REARM_SECONDS) + except (OverflowError, OSError, ValueError): + disarm() + return None + return disarm + + disarm_signal = arm_signal() + + if disarm_signal is not None: + disarm = disarm_signal else: def fire() -> None: @@ -371,7 +470,7 @@ def rearm(delay: float) -> None: timer.start() with lock: - rearm(remaining) + rearm(armable) def disarm() -> None: with lock: diff --git a/grapharc/runtime/graph.py b/grapharc/runtime/graph.py index 2635bf5..b72b884 100644 --- a/grapharc/runtime/graph.py +++ b/grapharc/runtime/graph.py @@ -657,11 +657,20 @@ def _leave( # one, so `observe.cost` can keep a measured figure apart from a # rate-card estimate instead of recording a guess as a fact. models = getattr(usage, "models", ()) + # This node's own spend, from the meter's per-node scope, not the movement + # of the run's shared total while the node ran. Under fan-out those differ: + # the workers' windows overlap, so every worker used to be credited with + # each sibling's concurrent spend — three workers costing 8 tokens each + # traced as 24/16/8, and `metrics` and `cost` both reported 48 for 24 + # tokens of real work, doubling the estimated bill purely because the work + # ran in parallel. Falls back to the difference when no scope was open, so + # a caller driving `_leave` outside `charging()` still gets a figure. + spent = getattr(usage, "tokens", None) emit( "end", state_delta=delta, duration_ms=duration_ms, - tokens=ctx.meter.tokens - tokens_before, + tokens=ctx.meter.tokens - tokens_before if spent is None else spent, cost_usd=getattr(usage, "cost_usd", None), model=models[0] if len(models) == 1 else None, ) diff --git a/grapharc/runtime/parsing.py b/grapharc/runtime/parsing.py index bf8f25c..216cb02 100644 --- a/grapharc/runtime/parsing.py +++ b/grapharc/runtime/parsing.py @@ -21,11 +21,8 @@ _FENCE = re.compile(r"```(?:json|JSON)?\s*(.*?)```", re.DOTALL) -def _balanced_span(text: str) -> str | None: - """The first balanced {...} or [...] region, ignoring braces inside strings.""" - start = next((i for i, ch in enumerate(text) if ch in "{["), None) - if start is None: - return None +def _span_from(text: str, start: int) -> str | None: + """The balanced {...} or [...] region opening at `start`, or None if unclosed.""" opener = text[start] closer = "}" if opener == "{" else "]" depth = 0 @@ -52,6 +49,29 @@ def _balanced_span(text: str) -> str | None: return None +def _balanced_spans(text: str) -> list[str]: + """Every balanced {...} or [...] region, longest first. + + Every opener is tried, not just the first one in the text. Taking only the + first meant any bracket in the model's *prose* hijacked the span and the real + JSON was never reached: `Based on the context [lines 3-5]: {...}` yielded the + unparseable `[lines 3-5]` and the reply was rejected, and — worse — + `Analysis (note [1]): {"supported": false}` yielded a perfectly valid `[1]`, + substituting a fabricated value for the model's actual answer. + + Longest first is what makes the ranking safe. It prefers a complete structure + over both a prose fragment that happens to parse and a nested piece of the + answer itself, so `{"claims": [{...}]}` returns the whole object rather than + the inner list. + """ + spans = [ + span + for i, ch in enumerate(text) + if ch in "{[" and (span := _span_from(text, i)) is not None + ] + return sorted(spans, key=len, reverse=True) + + def extract_json(content: Any) -> Any | None: """Best-effort JSON from a model reply. None when nothing valid is found.""" text = content if isinstance(content, str) else str(content) @@ -59,13 +79,14 @@ def extract_json(content: Any) -> Any | None: if not text: return None + # Whole reply first, then the fence, then balanced spans: the earlier a + # candidate is, the more of the model's reply it accounts for, so a fenced + # top-level array still wins over any span found inside it. candidates = [text] fenced = _FENCE.search(text) if fenced: candidates.append(fenced.group(1).strip()) - span = _balanced_span(text) - if span: - candidates.append(span) + candidates.extend(_balanced_spans(text)) for candidate in candidates: try: diff --git a/grapharc/runtime/usage.py b/grapharc/runtime/usage.py index 9ed0c52..191c6f4 100644 --- a/grapharc/runtime/usage.py +++ b/grapharc/runtime/usage.py @@ -76,6 +76,11 @@ def __init__(self, meter: BudgetMeter) -> None: # `observe.cost` keeps recorded and estimated figures apart. self.cost_usd: float | None = None self.models: list[str] = [] + # This node execution's own token spend, stamped by `charging` when the + # scope closes. Read from the meter's node scope rather than differenced + # off the run total, which credited a fan-out worker with whatever its + # siblings spent while it was running. + self.tokens = 0 def _record_price(self, response: LLMResult) -> None: """Accumulate the price a backend reported through `llm_output`. @@ -129,12 +134,19 @@ def charging(meter: BudgetMeter) -> Iterator[MeterCallbackHandler]: Also opens the meter's automatic scope, so a call metered here can be re-reported by hand inside this block — and only inside it. + + On the way out it stamps the scope's token tally onto the handler. Callers + read `handler.tokens` after the block has closed — the node wrapper reports + it on the node's `end` event — and by then the scope itself is gone. """ handler = MeterCallbackHandler(meter) token = _ACTIVE_HANDLER.set(handler) try: with meter.automatic_scope(): - yield handler + try: + yield handler + finally: + handler.tokens = meter.scope_tokens() or 0 finally: _ACTIVE_HANDLER.reset(token) diff --git a/tests/test_budget_enforcement.py b/tests/test_budget_enforcement.py index 27d5089..00c6166 100644 --- a/tests/test_budget_enforcement.py +++ b/tests/test_budget_enforcement.py @@ -12,7 +12,9 @@ money. """ +import asyncio import operator +import signal import threading import time from typing import Annotated @@ -157,6 +159,73 @@ def auto_plus_manual(state, ctx: RunContext): assert legacy.last_run.meter.tokens == plain.last_run.meter.tokens +async def _auto_plus_manual(state, ctx: RunContext): + message = await ScriptedChatModel(responses=["a scripted reply of some length"]).ainvoke( + "hello" + ) + charge_usage(ctx, message) + return {"ok": True} + + +def test_an_async_node_re_reporting_a_metered_call_does_not_pay_twice(): + """The same gate as the sync case above, on the async path. + + `on_llm_end` is sync, so under `ainvoke` LangChain dispatches it to a worker + thread while the body stays on the event loop. A thread-keyed ledger found + nothing there, never recorded the call, and charged the node's named + re-report a second time — so every `async def` node using the shipped + `charge_usage` reported double its real spend. + """ + + async def auto_only(state): + await ScriptedChatModel(responses=["a scripted reply of some length"]).ainvoke("hello") + return {"ok": True} + + plain = _single_node_graph(auto_only, writes={"ok"}) + asyncio.run(plain.ainvoke({})) + legacy = _single_node_graph(_auto_plus_manual, writes={"ok"}) + asyncio.run(legacy.ainvoke({})) + + assert plain.last_run.meter.tokens > 0 + assert legacy.last_run.meter.tokens == plain.last_run.meter.tokens + + +def test_an_async_node_stays_within_a_token_ceiling_its_real_spend_fits(): + """Double-charging did not just misreport — it halved the usable allowance. + + The ceiling is one token above the call's real cost, so it accommodates the + real spend and nothing like twice it. + """ + + async def auto_only(state): + await ScriptedChatModel(responses=["a scripted reply of some length"]).ainvoke("hello") + return {"ok": True} + + measure = _single_node_graph(auto_only, writes={"ok"}) + asyncio.run(measure.ainvoke({})) + real_spend = measure.last_run.meter.tokens + assert real_spend > 0 + + compiled = _single_node_graph( + _auto_plus_manual, writes={"ok"}, budget=Budget(max_tokens=real_spend + 1) + ) + asyncio.run(compiled.ainvoke({})) + assert compiled.last_run.meter.tokens == real_spend + + +def test_a_nested_automatic_scope_restores_the_enclosing_one(): + """Popping a thread-keyed entry discarded the outer node's ledger, so its + remaining re-reports paid twice. A context scope restores it.""" + meter = BudgetMeter(Budget()) + call = object() + with meter.automatic_scope(): + meter.charge_tokens(100, automatic=True, source=call) + with meter.automatic_scope(): + meter.charge_tokens(50, automatic=True, source=object()) + meter.charge_tokens(100, source=call) # a re-report, still free + assert meter.tokens == 150 + + def test_a_manual_charge_the_callback_never_saw_still_counts(): """Spend the runtime cannot see — a provider billed outside LangChain, say — must still land.""" @@ -520,6 +589,33 @@ def test_deadline_guard_refuses_to_start_work_past_the_deadline(): pytest.fail("the guard let a node start after the budget was spent") +@pytest.mark.parametrize("unreachable", [float("inf"), 1e10]) +def test_an_unreachable_max_seconds_does_not_disable_the_next_run(unreachable): + """A `max_seconds` past the platform's `time_t` used to poison the process. + + `setitimer` raises `OverflowError` *after* the SIGALRM handler is installed + and the process-wide slot is taken, and both were left that way — so every + later guard found the slot held and silently fell back to the async-exception + mechanism, which cannot unwind a blocking syscall. The 0.2s deadline below + then took the full 2s sleep to be noticed. + """ + + def slow(state): + time.sleep(2.0) + return {"ok": True} + + # An unreachable ceiling must not fire, and must not leave a trap behind. + _single_node_graph(slow, writes={"ok"}, budget=Budget(max_seconds=unreachable)).invoke({}) + + started = time.monotonic() + with pytest.raises(NodeDeadlineExceeded, match="max_seconds"): + _single_node_graph(slow, writes={"ok"}, budget=Budget(max_seconds=0.2)).invoke({}) + assert time.monotonic() - started < 1.0, "SIGALRM was still poisoned" + + if hasattr(signal, "SIGALRM"): + assert signal.getsignal(signal.SIGALRM) in (signal.SIG_DFL, signal.SIG_IGN) + + def test_a_deadline_exceeded_is_a_budget_exceeded(): """Callers that already catch BudgetExceeded must keep catching timeouts.""" assert issubclass(NodeDeadlineExceeded, BudgetExceeded) diff --git a/tests/test_parsing.py b/tests/test_parsing.py index f92b447..7d120b3 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -45,6 +45,33 @@ def test_unrecoverable_replies_return_none(reply): assert extract_json(reply) is None +@pytest.mark.parametrize( + "reply", + [ + 'Based on the context [lines 3-5]: {"supported": true, "reason": "ok"}', + 'The quote "{" is fine. {"supported": true, "reason": "ok"}', + 'Analysis (note [1]): {"supported": true, "reason": "ok"}', + 'See table[0] and figure{2}: {"supported": true, "reason": "ok"}', + ], +) +def test_a_bracket_in_the_prose_does_not_hijack_the_answer(reply): + """Only the *first* opener used to be tried, so any bracket in the preamble + captured the span and the real JSON was never reached.""" + assert extract_json(reply) == {"supported": True, "reason": "ok"} + + +def test_a_prose_bracket_is_never_substituted_for_the_answer(): + """The worst shape of the bug above: `[1]` is valid JSON, so it was returned + as the model's verdict. A wrong answer is worse than no answer.""" + reply = 'Analysis (note [1]): {"supported": false, "reason": "negated"}' + assert extract_json(reply) == {"supported": False, "reason": "negated"} + + +def test_the_outermost_structure_wins_over_a_nested_piece_of_it(): + reply = 'Verdict: {"claims": [{"text": "a"}], "n": 1}' + assert extract_json(reply) == {"claims": [{"text": "a"}], "n": 1} + + def test_verifier_still_fails_closed_on_junk(): from grapharc.runtime.verify import verify_claim from grapharc.testing import ScriptedChatModel diff --git a/tests/test_replay.py b/tests/test_replay.py index b9e672e..b1a2458 100644 --- a/tests/test_replay.py +++ b/tests/test_replay.py @@ -258,6 +258,100 @@ class F(GraphARCState): ] +def _fanout_token_run(trace, run_id, *, serial): + """Three workers, one model call each, overlapping in time.""" + import time + + from grapharc.runtime.budget import Budget + from grapharc.testing import ScriptedChatModel + + class F(GraphARCState): + seeds: list[str] = [] + said: Annotated[list[str], operator.add] = [] + + class Shard(GraphARCState): + seed: str = "" + + def worker(shard): + ScriptedChatModel(responses=["a scripted reply of some length"]).invoke( + "hello " + shard.seed + ) + time.sleep(0.05) # hold the window open so the siblings overlap + return {"said": [shard.seed]} + + g = GraphARC(F, name="fan", trace=trace, budget=Budget()) + g.add_node("fan", lambda s: {"seeds": s.seeds}, writes={"seeds"}) + g.add_node("w", worker, writes={"said"}, input_schema=Shard) + g.add_edge(START, "fan") + g.add_fanout_edge("fan", lambda s: [("w", Shard(seed=x)) for x in s.seeds]) + g.add_edge("w", END) + compiled = g.compile() + compiled.invoke( + {"seeds": ["a", "b", "c"]}, + run_id=run_id, + budget=Budget(max_concurrency=1) if serial else Budget(), + ) + per_worker = [ + e.tokens for e in trace.read_events(run_id) if e.phase == "end" and e.node == "w" + ] + return compiled.last_run.meter.tokens, per_worker + + +def test_a_fanout_worker_is_charged_its_own_tokens_not_its_siblings(trace): + """A node's `end` event used to report the movement of the run's *shared* + total while it ran, so overlapping workers each absorbed the others' spend. + + Three workers costing the same each traced as 24/16/8, and `metrics` and + `cost` both reported three times one worker's spend for the whole run — + doubling the estimated bill purely because the work ran in parallel. + """ + real, parallel = _fanout_token_run(trace, "par", serial=False) + + assert len(parallel) == 3 + assert len(set(parallel)) == 1, f"workers absorbed each other's spend: {parallel}" + assert sum(parallel) == real + + +def test_fanout_token_attribution_does_not_depend_on_concurrency(trace): + """The same work must cost the same whether it runs in parallel or serially — + including the dollar figure `cost` estimates from it.""" + serial_real, serial_per = _fanout_token_run(trace, "ser", serial=True) + par_real, par_per = _fanout_token_run(trace, "par", serial=False) + + assert serial_real == par_real + assert sorted(serial_per) == sorted(par_per) + + rates = RateCard(default=3.0) + assert summarize(trace, "par").tokens == summarize(trace, "ser").tokens == serial_real + assert ( + attribute(trace, "par", rates=rates).estimated_cost_usd + == attribute(trace, "ser", rates=rates).estimated_cost_usd + ) + + +def test_a_hand_charged_token_still_lands_on_the_nodes_end_event(trace): + """Attribution moved to the meter's per-node scope, which must still capture a + charge the node makes itself — not only the ones the usage callback saw.""" + from grapharc.runtime.budget import Budget + from grapharc.runtime.graph import RunContext + + class H(GraphARCState): + ok: bool = False + + def node(state, ctx: RunContext): + ctx.meter.charge_tokens(777) + return {"ok": True} + + g = GraphARC(H, name="hand", trace=trace, budget=Budget()) + g.add_node("n", node, writes={"ok"}) + g.add_edge(START, "n") + g.add_edge("n", END) + g.compile().invoke({}, run_id="r1") + + ends = [e for e in trace.read_events("r1") if e.phase == "end"] + assert [e.tokens for e in ends] == [777] + + def test_a_named_sub_step_finds_its_worker_among_parallel_nodes(trace): trace.event(run_id="r1", graph="demo", node="w1", phase="start", step=1) trace.event(run_id="r1", graph="demo", node="w2", phase="start", step=2) From 21bc7642e77b347bd394ec49678d3806674d69db Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Thu, 30 Jul 2026 04:42:30 +0530 Subject: [PATCH 2/6] Make the shipped plan trace complete and stop counting planner spend twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- grapharc/examples/plan_incident.py | 12 ++++++- grapharc/planner/loop.py | 47 ++++++++++++++++++------ tests/test_cli.py | 40 +++++++++++++++++++++ tests/test_planner_loop.py | 58 ++++++++++++++++++++++++++++++ 4 files changed, 146 insertions(+), 11 deletions(-) diff --git a/grapharc/examples/plan_incident.py b/grapharc/examples/plan_incident.py index 7ae4ac1..2adbae4 100644 --- a/grapharc/examples/plan_incident.py +++ b/grapharc/examples/plan_incident.py @@ -163,7 +163,16 @@ def build_loop( # the same object, and a node body could otherwise widen it between rounds. registry.freeze() return GovernedLoop( - planner=PlannerNode(model, name="incident", catalog=registry.catalog()), + # The planner and the materializer get the recorder too. Without it the + # run's own trace held 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 actually executed. README's "the executed nodes' + # own start/end pairs" was true of a hand-wired loop and false of the + # shipped one, which is the one `grapharc plan` drives. + planner=PlannerNode( + model, name="incident", catalog=registry.catalog(), trace=trace + ), checker=AdmissionChecker( registry=registry, edge_policy=edge_policy or default_edge_policy(), @@ -173,6 +182,7 @@ def build_loop( registry=registry, state_schema=state_schema or IncidentState, writes=writes if writes is not None else WRITES, + trace=trace, ), budget=budget, limits=limits, diff --git a/grapharc/planner/loop.py b/grapharc/planner/loop.py index 134eb9b..6a1ed82 100644 --- a/grapharc/planner/loop.py +++ b/grapharc/planner/loop.py @@ -248,6 +248,10 @@ class _Execution(NamedTuple): materialization_error: str = "" execution_error: str = "" hard_stop: LoopStop | None = None + # The sub-run's iteration count, so the round can report what it spent. + # `_charge_back` folds this into the run's meter either way; carrying it here + # is what lets `RoundRecord.iterations` hold a figure instead of always 0. + iterations: int = 0 @property def failure(self) -> str: @@ -482,6 +486,7 @@ def close(**fields: Any) -> None: execution_error=attempt.execution_error, executed=attempt.executed, progressed=progressed, + iterations=attempt.iterations, ) if stop is None: @@ -593,15 +598,22 @@ def _execute( budget=budget, ) except BudgetExceeded as exc: - self._charge_back(compiled, meter) + iterations = self._charge_back(compiled, meter) return _Execution( - state=state, execution_error=exc.reason, hard_stop=LoopStop.BUDGET_EXHAUSTED + state=state, + execution_error=exc.reason, + hard_stop=LoopStop.BUDGET_EXHAUSTED, + iterations=iterations, ) except Exception as exc: # noqa: BLE001 - a failed subgraph is a replanning input - self._charge_back(compiled, meter) - return _Execution(state=state, execution_error=f"raised {exc!r}") - self._charge_back(compiled, meter) - return _Execution(executed=True, state=self._initial_state(raw)) + iterations = self._charge_back(compiled, meter) + return _Execution( + state=state, execution_error=f"raised {exc!r}", iterations=iterations + ) + iterations = self._charge_back(compiled, meter) + return _Execution( + executed=True, state=self._initial_state(raw), iterations=iterations + ) def _round_budget(self, meter: BudgetMeter) -> Budget: """This round's ceiling: exactly what the run has left, per dimension. @@ -618,18 +630,21 @@ def _round_budget(self, meter: BudgetMeter) -> Budget: ) @staticmethod - def _charge_back(compiled: Any, meter: BudgetMeter) -> None: + def _charge_back(compiled: Any, meter: BudgetMeter) -> int: """Fold the round's spend into the run's meter, whether or not it finished. The sub-run's meter is a separate accountant with its own ceiling; the loop's meter never saw those calls, so this charge is new spend rather than a re-report and is counted unnamed on purpose. + + Returns the sub-run's iteration count, for the round to record. """ sub = getattr(compiled, "last_run", None) if sub is None: - return + return 0 meter.charge_tokens(sub.meter.tokens) meter.charge_iteration(sub.meter.iterations) + return sub.meter.iterations # -- recording ------------------------------------------------------------ @@ -648,8 +663,14 @@ def _record( ctx, node=f"{self.name}:round{record.round}", phase="round", - duration_ms=record.duration_ms, - tokens=record.tokens, + # No `tokens=` and no `duration_ms=`: a round is an *envelope*, not a + # measurement. Its tokens are the planner's, already reported by the + # `plan` event, and its duration encloses that plan plus every node + # the round executed. `metrics`, `cost` and `replay` sum node totals + # *plus* every event they cannot place inside a node — and `round` is + # one of those — so reporting either here counted the same spend + # twice. `RoundRecord` still carries both for callers reading the + # returned `LoopResult`; what changes is only what lands on the trace. state_delta={ "round": record.round, "proposal_id": record.proposal.proposal_id if record.proposal else "", @@ -660,6 +681,12 @@ def _record( "executed": record.executed, "progressed": record.progressed, "stop": stop.value if stop is not None else "", + # The envelope's own figures, under names no reader sums. What a + # round spent stays answerable from the file; what it spent is + # just no longer added to the totals a second time. + "round_tokens": record.tokens, + "round_iterations": record.iterations, + "round_duration_ms": round(record.duration_ms, 2), }, error=problem or None, ) diff --git a/tests/test_cli.py b/tests/test_cli.py index 8065f19..c4684bd 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1011,6 +1011,46 @@ def test_plan_runs_the_governed_loop_and_reports_every_round(tmp_path, capsys): assert "round 2: admitted" in out +def test_plan_traces_the_plan_and_every_node_it_executed(tmp_path, capsys): + """README's "the trace holds an `admission` event per round, a `round` event + per round, the executed nodes' own `start`/`end` pairs and one `stop` event". + + That was true of a hand-wired loop and false of the shipped registry, which is + the one this command drives: it withheld the recorder from the `PlannerNode` + and the `Materializer`, so the file held no `plan` event and — because the + built subgraph inherits the materializer's recorder — no `start`/`end` pair + for any node the loop ran. Three nodes executed and none of them appeared. + """ + from grapharc.observe import cost, metrics + from grapharc.observe.trace import TraceRecorder + + path = tmp_path / "t.jsonl" + code, payload, _ = call_json(["plan", "look into the outage", "--trace", str(path)], capsys) + assert code == 0 + + recorder = TraceRecorder(path) + events = recorder.read_events() + run_id = events[0].run_id + phases = {phase: 0 for phase in ("plan", "admission", "round", "start", "end", "stop")} + for event in events: + if event.phase in phases: + phases[event.phase] += 1 + + assert phases["plan"] == 2, "the planner's own event is missing" + assert phases["admission"] == 2 + assert phases["round"] == 2 + assert phases["stop"] == 1 + # The admitted round ran three nodes; each owes a start/end pair. + assert phases["start"] == 3 + assert phases["end"] == 3 + + summary = metrics.summarize(recorder, run_id) + assert summary.nodes_executed == 3 + assert set(summary.per_node) == {"triage", "patch", "verify"} + # And the two readers of that one file still agree with each other. + assert cost.attribute(recorder, run_id).tokens == summary.tokens + + def test_plan_json_carries_the_rounds_and_the_stop_reason(tmp_path, capsys): code, payload, _ = call_json( ["plan", "look into the outage", "--trace", str(tmp_path / "t.jsonl")], capsys diff --git a/tests/test_planner_loop.py b/tests/test_planner_loop.py index 5554731..b648b6f 100644 --- a/tests/test_planner_loop.py +++ b/tests/test_planner_loop.py @@ -821,6 +821,64 @@ def test_a_run_plans_admits_executes_and_reaches_the_goal(trace): assert result.usage["tokens"] > 0 +def test_a_round_records_the_iterations_it_spent(trace): + """`RoundRecord.iterations` was declared and never assigned, so every round + reported 0 while the run's meter counted the same work.""" + loop, _model, _bodies = build_loop( + [plan(("read", "fetch"), ("write", "summarise"))], + trace=trace, + goal_reached=goal_is_done, + ) + + result = loop.run("summarise the incident", LoopState()) + + assert [r.iterations for r in result.rounds] == [2] + assert sum(r.iterations for r in result.rounds) == result.usage["iterations"] + + +def test_the_planners_tokens_are_counted_once_not_once_per_round(trace): + """A `round` event is an envelope, not a measurement. + + Its tokens are the planner's, already reported by the `plan` event, and both + land in the set `metrics`/`cost` add on top of node totals — so the planner's + spend was charged to the report twice. + """ + from grapharc.observe import cost, metrics + + loop, _model, _bodies = build_loop( + [plan(("read", "fetch"), ("write", "summarise"))], + trace=trace, + goal_reached=goal_is_done, + ) + result = loop.run("summarise the incident", LoopState(), run_id="r1") + + real = result.usage["tokens"] + assert real > 0 + assert metrics.summarize(trace, "r1").tokens == real + assert cost.attribute(trace, "r1").tokens == real + + +def test_a_round_event_still_reports_what_the_round_itself_spent(trace): + """Removing the double count must not remove the information.""" + loop, _model, _bodies = build_loop( + [plan(("read", "fetch"), ("write", "summarise"))], + trace=trace, + goal_reached=goal_is_done, + ) + loop.run("summarise the incident", LoopState(), run_id="r1") + + rounds = [e for e in trace.read_events("r1") if e.phase == "round"] + assert rounds, "the round event went missing" + for event in rounds: + # Not in the typed fields any reader sums... + assert event.tokens is None + assert event.duration_ms is None + # ...but still answerable from the file. + assert event.state_delta["round_tokens"] > 0 + assert event.state_delta["round_iterations"] == 2 + assert event.state_delta["round_duration_ms"] >= 0 + + def test_a_rejection_reaches_the_planner_and_the_next_proposal_completes_the_run(): """The replanning edge of ARCHITECTURE §2: rejected + reason -> propose again.""" loop, model, bodies = build_loop( From 58cb5db536e6749d70396be61d4f9b937710aedb Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Thu, 30 Jul 2026 04:42:46 +0530 Subject: [PATCH 3/6] Add colour to the CLI on a terminal, and nothing at all when piped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Human-mode output gains a Claude-Code-style colour hierarchy: dim labels, accented names and paths, semantic status colour (goal_met and admitted green, rejected and REFUSED red), and aligned key/value blocks. The whole design rests on one constraint: styling is decided at print time from the stream's own isatty(), so piped, redirected and captured output is byte-identical to before. That is what lets this land without touching a single expected block — tests/test_readme.py and tests/test_cookbook_models.py byte-compare CLI output against README.md and docs/cookbook/02-models.md, and both harnesses present a non-tty stdout. Padding is emitted outside the escape sequences, so a column is the same character count either way. Verified: 50/36/32/18 escapes on a pty for plan/models/models --check/demo, zero escapes on every piped stream, and ANSI-stripped tty output identical to piped output. NO_COLOR, TERM=dumb, --no-color and --json each yield zero escapes. The contracts that matter are unchanged — --json is one parseable document on stdout with zero-byte stderr, a text-mode failure leaves stdout empty with "error: ..." on stderr, viz stays raw pasteable Mermaid, and replay/diff still emit only the engine formatter's text. New grapharc/cli/style.py is stdlib only. No dependency was added: rich is not in the lock, and the README markets a four-package runtime dep list. Deliberately no glyphs, boxes or rules, even on a terminal. README and two cookbook pages print these blocks verbatim, so tty-only decoration would make what a user sees diverge from what the docs show — drift in the one direction the byte-comparison tests cannot catch. Co-Authored-By: Claude Opus 5 (1M context) --- grapharc/cli/agent.py | 66 ++++++-- grapharc/cli/graphrun.py | 57 ++++--- grapharc/cli/live.py | 39 +++-- grapharc/cli/main.py | 72 ++++++-- grapharc/cli/output.py | 10 +- grapharc/cli/plan.py | 50 ++++-- grapharc/cli/probe.py | 29 +++- grapharc/cli/serve.py | 27 ++- grapharc/cli/style.py | 344 +++++++++++++++++++++++++++++++++++++++ 9 files changed, 610 insertions(+), 84 deletions(-) create mode 100644 grapharc/cli/style.py diff --git a/grapharc/cli/agent.py b/grapharc/cli/agent.py index 20ae880..f4b89dd 100644 --- a/grapharc/cli/agent.py +++ b/grapharc/cli/agent.py @@ -20,7 +20,7 @@ from pathlib import Path from typing import Any -from grapharc.cli import optional +from grapharc.cli import optional, style from grapharc.cli.output import EXIT_FAILED, EXIT_OK, emit, fail # Entry points accepted from `grapharc.tools`, in preference order: a registrar @@ -241,24 +241,62 @@ def run_agent( "refused": len(result.refused), } + width = style.LABEL_WIDTH + note = f" {style.dim(f'({result.note})')}" if result.note else "" + + def count(number: int) -> str: + """A count, red once it is not zero. + + Zero refusals is not news; one is the reason to read the tool-call rows + underneath it. The digits are the same either way when colour is off. + """ + return style.err(str(number)) if number else str(number) + lines = [ - f"task : {task}", - f"model : {model_spec}", - f"workspace : {workspace}", - f"tools : {', '.join(visible) or '(none visible under this policy)'}", - f"policy : allow={allow} ask={ask} deny={deny}", + style.kv("task", task, width=width), + style.kv("model", model_spec, width=width, tint=style.accent), + style.kv("workspace", str(workspace), width=width, tint=style.accent), + style.kv( + "tools", + ", ".join(visible) or "(none visible under this policy)", + width=width, + ), + style.kv( + "policy", + f"{style.dim('allow=')}{allow} {style.dim('ask=')}{ask} {style.dim('deny=')}{deny}", + width=width, + ), "", - f"stopped : {reason}{f' ({result.note})' if result.note else ''}", - f"turns : {result.iterations} tool calls: {len(result.tool_calls)} " - f"denied: {len(result.denied)} refused: {len(result.refused)}", - f"tokens : {meter.tokens:,}", + style.kv( + "stopped", + f"{(style.ok if met else style.warn)(reason)}{note}", + width=width, + ), + style.kv( + "turns", + f"{result.iterations} {style.dim('tool calls:')} {len(result.tool_calls)} " + f"{style.dim('denied:')} {count(len(result.denied))} " + f"{style.dim('refused:')} {count(len(result.refused))}", + width=width, + ), + style.kv("tokens", f"{meter.tokens:,}", width=width), ] for call in result.tool_calls: - suffix = f" [{call.refused_by}]" if call.refused_by else "" - lines.append(f" {call.status.value:<8} {call.tool}{suffix}") + suffix = f" {style.dim(f'[{call.refused_by}]')}" if call.refused_by else "" + # `ToolCallStatus` is ok / denied / error; anything a later version adds + # lands on amber rather than being quietly called a success. + verdict = {"ok": True, "denied": False, "error": False}.get(call.status.value) + lines.append( + f" {style.cell(call.status.value, 8, tint=style.tint_for(verdict))} " + f"{style.accent(call.tool)}{suffix}" + ) lines.append("") - lines.append(f"answer : {result.output}" if met else f"partial : {result.partial_output}") - lines.append(f"trace : {trace_path}") + lines.append( + style.kv("answer", str(result.output), width=width) + if met + else style.kv("partial", str(result.partial_output), width=width, tint=style.dim) + ) + lines.append(style.kv("trace", str(trace_path), width=width, tint=style.accent)) emit(payload, lines, as_json=as_json) return EXIT_OK if met else EXIT_FAILED diff --git a/grapharc/cli/graphrun.py b/grapharc/cli/graphrun.py index 48db789..d104ebe 100644 --- a/grapharc/cli/graphrun.py +++ b/grapharc/cli/graphrun.py @@ -38,6 +38,7 @@ from pathlib import Path from typing import Any +from grapharc.cli import style from grapharc.cli.config import ConfigError from grapharc.cli.config import load as load_settings from grapharc.cli.generate import resolve_or_generate_policy @@ -183,15 +184,32 @@ def run_graph( **settings.provenance(policy_source=policy_source), } + # `REFUSED` and `ADMITTED` are the whole answer, so on a terminal they carry + # the only two colours that matter here. The words, the widths and the order + # are untouched: `--check-only` is what CI runs, and CI reads text. + width = style.LABEL_WIDTH + header = [ + style.kv("graph", graph_path, width=width, tint=style.accent), + style.kv("policy", policy_description, width=width), + ] + trace_line = style.kv("trace", str(trace_path), width=width, tint=style.accent) + if not verdict.admitted: lines = [ - f"graph : {graph_path}", - f"policy : {policy_description}", + *header, "", - f"REFUSED : {len(verdict.rejections)} objection(s)", + style.kv( + "REFUSED", + f"{len(verdict.rejections)} objection(s)", + width=width, + key_tint=style.err, + tint=style.err, + ), + ] + lines += [ + f" {style.cell(r.code, 18, tint=style.err)} {r.detail}" for r in verdict.rejections ] - lines += [f" {r.code:<18} {r.detail}" for r in verdict.rejections] - lines += ["", f"trace : {trace_path}"] + lines += ["", trace_line] emit({"ok": False, **common}, lines, as_json=as_json) return EXIT_FAILED @@ -210,13 +228,12 @@ def run_graph( compiled = materializer.materialize(verdict, proposal) except MaterializationError as exc: lines = [ - f"graph : {graph_path}", - f"policy : {policy_description}", + *header, "", - "ADMITTED, BUT CANNOT BE BUILT", + style.err("ADMITTED, BUT CANNOT BE BUILT"), f" {exc}", "", - f"trace : {trace_path}", + trace_line, ] emit( {"ok": False, "buildable": False, "error": str(exc), **common}, @@ -227,12 +244,13 @@ def run_graph( if check_only: lines = [ - f"graph : {graph_path}", - f"policy : {policy_description}", - f"nodes : {proposal.node_count()}", + *header, + style.kv("nodes", str(proposal.node_count()), width=width), "", - "ADMITTED and buildable. Nothing was run.", - f"fingerprint: {verdict.fingerprint}", + style.ok("ADMITTED") + style.dim(" and buildable. Nothing was run."), + # Wider than the label column on purpose, and always has been: the + # fingerprint is what a later run is compared against. + style.kv("fingerprint", verdict.fingerprint, width=width, tint=style.accent), ] emit( {"ok": True, "checked_only": True, "buildable": True, **common}, @@ -245,13 +263,12 @@ def run_graph( payload = {"ok": True, "checked_only": False, **common, "state": state} lines = [ - f"graph : {graph_path}", - f"policy : {policy_description}", - f"nodes : {proposal.node_count()}", + *header, + style.kv("nodes", str(proposal.node_count()), width=width), "", - "ADMITTED and executed.", - f"state : {state}", - f"trace : {trace_path}", + style.ok("ADMITTED") + style.dim(" and executed."), + style.kv("state", str(state), width=width), + trace_line, ] emit(payload, lines, as_json=as_json) return EXIT_OK diff --git a/grapharc/cli/live.py b/grapharc/cli/live.py index 6af14a8..4ff41fd 100644 --- a/grapharc/cli/live.py +++ b/grapharc/cli/live.py @@ -15,6 +15,7 @@ from pathlib import Path +from grapharc.cli import style from grapharc.cli.output import EXIT_FAILED, EXIT_OK, emit from grapharc.gateway import different_providers, get_model from grapharc.observe.metrics import summarize @@ -23,6 +24,10 @@ DEFAULT_REVIEWER = "openrouter/openai/gpt-4o-mini" +#: The header labels line up at nine characters, which is what `model`, +#: `reviewer` and `budget` have always printed at. +LABEL_WIDTH = 9 + # Real models are open-ended, so a live run always carries a ceiling. LIVE_BUDGET = Budget(max_iterations=40, max_tokens=200_000, max_seconds=600) @@ -55,22 +60,32 @@ def say(line: str) -> None: if not as_json: print(line) - say(f"model : {model_spec}") + say(style.kv("model", model_spec, width=LABEL_WIDTH, tint=style.accent)) model = get_model(model_spec, temperature=0) reviewer = None correlated = None if _needs_reviewer(example): reviewer_spec = reviewer_spec or DEFAULT_REVIEWER - say(f"reviewer : {reviewer_spec}") + say(style.kv("reviewer", reviewer_spec, width=LABEL_WIDTH, tint=style.accent)) correlated = not different_providers(model_spec, reviewer_spec) if correlated: + # Amber, not red: the run is still valid, the evidence is just weaker. say( - " warning: author and reviewer share a provider — correlated " - "agreement makes this weaker evidence than a cross-vendor pair" + style.warn( + " warning: author and reviewer share a provider — correlated " + "agreement makes this weaker evidence than a cross-vendor pair" + ) ) reviewer = get_model(reviewer_spec, temperature=0) - say(f"budget : {LIVE_BUDGET.max_tokens:,} tokens / {LIVE_BUDGET.max_seconds:.0f}s") + say( + style.kv( + "budget", + f"{LIVE_BUDGET.max_tokens:,}{style.dim(' tokens / ')}" + f"{LIVE_BUDGET.max_seconds:.0f}{style.dim('s')}", + width=LABEL_WIDTH, + ) + ) say("") header = { @@ -90,7 +105,7 @@ def say(line: str) -> None: if result is None: emit( {"ok": False, **header, "error": f"'{example}' has no live wiring yet"}, - [f"'{example}' has no live wiring yet"], + [style.err(f"'{example}' has no live wiring yet")], as_json=as_json, ) return EXIT_FAILED @@ -99,14 +114,18 @@ def say(line: str) -> None: lines = [] for key, value in result.items(): rendered = str(value) - lines.append(f"{key}: {rendered[:400]}{'…' if len(rendered) > 400 else ''}") + clipped = f"{rendered[:400]}{'…' if len(rendered) > 400 else ''}" + lines.append(style.kv(str(key), clipped)) if metrics: lines.append("") lines.append( - f"spent: {metrics.tokens:,} tokens across {metrics.nodes_executed} nodes " - f"in {metrics.duration_ms / 1000:.1f}s" + style.kv( + "spent", + f"{metrics.tokens:,}{style.dim(' tokens across ')}{metrics.nodes_executed}" + f"{style.dim(' nodes in ')}{metrics.duration_ms / 1000:.1f}{style.dim('s')}", + ) ) - lines.append(f"trace: {trace_path}") + lines.append(style.kv("trace", str(trace_path), tint=style.accent)) emit( {"ok": True, **header, "result": result, "metrics": metrics}, diff --git a/grapharc/cli/main.py b/grapharc/cli/main.py index e7363ef..567c95c 100644 --- a/grapharc/cli/main.py +++ b/grapharc/cli/main.py @@ -13,6 +13,12 @@ Reading commands (`trace`, `metrics`, `viz`, `diff`) all read the same JSONL the runtime writes, so the metrics and the audit trail cannot disagree: there is only one record. + +Human output is coloured only when stdout is an interactive terminal, and the +colour is the *only* difference: same lines, same words, same column widths. So +`grapharc … | grep`, a captured log and the transcripts printed in `README.md` +all keep the bytes they had before `grapharc.cli.style` existed. `--no-color`, +`NO_COLOR` and `--json` each turn it off. """ from __future__ import annotations @@ -30,6 +36,7 @@ # `grapharc.cli.agent` is safe to import here: it pulls in nothing but the # stdlib and this package. Every component it *drives* — the toolset, the # gateway, the harness — it imports inside the command. +from grapharc.cli import style from grapharc.cli.agent import ( DEFAULT_MAX_SECONDS, DEFAULT_MAX_TOKENS, @@ -296,8 +303,8 @@ def _cmd_demo(args: argparse.Namespace) -> int: "trace": str(trace_path), "result": result, } - lines = [f"{key}: {value}" for key, value in result.items()] - lines += ["", f"trace: {trace_path}"] + lines = style.kv_block((str(key), str(value)) for key, value in result.items()) + lines += ["", style.kv("trace", str(trace_path), tint=style.accent)] emit(payload, lines, as_json=args.json) return EXIT_OK @@ -354,6 +361,7 @@ def _cmd_models(args: argparse.Namespace) -> int: usable = any_provider_usable(results) emit( {"ok": usable, "command": "models", "check": True, "backends": results}, + # Styled by `probe.render`, which owns the three-column layout. render(results), as_json=args.json, ) @@ -373,7 +381,7 @@ def _cmd_models(args: argparse.Namespace) -> int: return fail(str(exc), as_json=args.json, command="models", spec=args.spec) emit( {"ok": True, "command": "models", **resolved}, - [f"{key}: {value}" for key, value in resolved.items()], + style.kv_block((str(key), str(value)) for key, value in resolved.items()), as_json=args.json, ) return EXIT_OK @@ -400,16 +408,22 @@ def _cmd_models(args: argparse.Namespace) -> int: "ollama_base_url": ollama_base_url(), "examples": examples, } + # A redacted key is left the default colour on purpose: highlighting the one + # field on the page that is a fingerprint of a secret invites reading it out. lines = [ - f"backends: {', '.join(BACKENDS)}", - f"openrouter key: {redact(openrouter_api_key())}", - f"openai key: {redact(openai_api_key())}", - f"ollama url: {ollama_base_url()}", + style.kv("backends", ", ".join(BACKENDS)), + style.kv("openrouter key", redact(openrouter_api_key())), + style.kv("openai key", redact(openai_api_key())), + style.kv("ollama url", ollama_base_url(), tint=style.accent), "", - "examples:", - *[f" {spec:<38} {note}" for spec, note in examples.items()], + style.heading("examples:"), + *[ + f" {style.cell(spec, 38, tint=style.accent)} {style.dim(note)}" + for spec, note in examples.items() + ], "", - "grapharc models --check probes which of these this machine can use", + style.accent("grapharc models --check") + + style.dim(" probes which of these this machine can use"), ] emit(payload, lines, as_json=args.json) return EXIT_OK @@ -472,11 +486,19 @@ def _cmd_trace(args: argparse.Namespace) -> int: "count": len(events), "events": [e.model_dump(exclude_none=True) for e in events], } + # `[ 3] act end Δ{'candidate': 1}` — the step counter and + # the phase are scaffolding, the node name and the state delta are the reason + # anyone reads a trace, and `!` is the only thing that ever needs finding in a + # thousand lines. Columns are unchanged; the colour is the index. lines = [] for event in events: - delta = f" Δ{event.state_delta}" if event.state_delta else "" - err = f" !{event.error}" if event.error else "" - lines.append(f"[{event.step:>3}] {event.node:<20} {event.phase:<6}{delta}{err}") + delta = f" {style.accent('Δ')}{event.state_delta}" if event.state_delta else "" + failed = f" {style.err(f'!{event.error}')}" if event.error else "" + lines.append( + f"{style.dim(f'[{event.step:>3}]')} " + f"{style.cell(event.node, 20, tint=style.accent)} " + f"{style.cell(event.phase, 6, tint=style.dim)}{delta}{failed}" + ) emit(payload, lines, as_json=args.json) return EXIT_OK @@ -496,7 +518,7 @@ def _cmd_metrics(args: argparse.Namespace) -> int: data = metrics.model_dump() emit( {"ok": True, "command": "metrics", **data}, - [f"{key}: {value}" for key, value in data.items()], + style.kv_block((str(key), str(value)) for key, value in data.items()), as_json=args.json, ) return EXIT_OK @@ -521,6 +543,9 @@ def _cmd_viz(args: argparse.Namespace) -> int: "run_id": args.run_id, "mermaid": mermaid, }, + # Deliberately unstyled, on a terminal too. This output exists to be + # pasted into a Mermaid renderer; a box around it or an escape sequence + # inside it would make the one thing the command is for stop working. [mermaid], as_json=args.json, ) @@ -541,6 +566,20 @@ def build_parser() -> argparse.ArgumentParser: common.add_argument( "--json", action="store_true", help="print one JSON document instead of text" ) + # On `common` rather than the top level, for the same reason `--json` is: the + # position a shell user reaches for is after the arguments. Declaring it in + # both places would not work anyway — the subparser's default would overwrite + # a value given before the subcommand, in the one namespace they share. + # + # Colour is off whenever stdout is not a terminal, so this is only ever needed + # to *keep* it off somewhere `isatty()` says yes and the reader disagrees: a + # tty being recorded, a pager that shows escapes, a screen reader. `NO_COLOR` + # in the environment does the same thing for every run at once. + common.add_argument( + "--no-color", + action="store_true", + help="never colour the output, even when stdout is a terminal (see also NO_COLOR)", + ) # `--config` belongs only to the commands that resolve settings. It used to # live on `common`, so every command accepted it and nine silently ignored # it — including rejecting a missing file on two commands and not the rest. @@ -802,6 +841,11 @@ def build_parser() -> argparse.ArgumentParser: def main(argv: list[str] | None = None) -> int: args = build_parser().parse_args(argv) + # `--json` disables colour too, belt and braces: a JSON run has no human + # reader, and `tests/test_cli.py` requires that exactly one document reaches + # stdout and nothing at all reaches stderr. Turning styling off at the source + # means no future call site can leak an escape sequence into a payload. + style.configure(no_color=args.no_color or args.json) if args.command == "models" and args.check and args.spec: return fail( "`models --check` probes the configured backends and `models ` " diff --git a/grapharc/cli/output.py b/grapharc/cli/output.py index 749264d..36c77a6 100644 --- a/grapharc/cli/output.py +++ b/grapharc/cli/output.py @@ -16,6 +16,8 @@ from pydantic import BaseModel +from grapharc.cli import style + EXIT_OK = 0 # The command ran and the answer is negative: an agent that stopped short of its # target, a run id with no events, a probe that found no usable backend. @@ -79,12 +81,18 @@ def fail( Text errors go to stderr so `grapharc … > out` still shows the failure; JSON errors go to stdout so the one document a script parses is the one that says what went wrong. + + The message body is never wrapped, dyed or re-flowed — only the `error: ` + prefix is, and only on a terminal. Callers grep stderr for phrases the + message contains (`tests/test_cli.py` asserts on `"not a valid topology"`, + `"no such trace file"`, `"grapharc demo stage0"`), and a hard break inserted + at column 80 would cut one of them in half. """ payload = {"ok": False, "command": command, "error": message, **extra} if as_json: print(dumps(payload)) else: - print(f"error: {message}", file=sys.stderr) + print(f"{style.error_prefix()}{message}", file=sys.stderr) return code diff --git a/grapharc/cli/plan.py b/grapharc/cli/plan.py index 425e2b4..d70edb7 100644 --- a/grapharc/cli/plan.py +++ b/grapharc/cli/plan.py @@ -33,6 +33,7 @@ from pathlib import Path from typing import Any +from grapharc.cli import style from grapharc.cli.config import ConfigError, Settings from grapharc.cli.config import load as load_settings from grapharc.cli.generate import resolve_or_generate_policy @@ -258,27 +259,50 @@ def plan( "state": result.state.model_dump() if hasattr(result.state, "model_dump") else result.state, } + # The plain text of every line below is exactly what it was before colour + # existed — the labels are still ten characters wide and the round rows still + # pad the status to nine — because this block is the transcript printed in + # `README.md`. On a terminal the colour carries the verdict: green admitted, + # red rejected, and the stop reason in the same green only if the goal was met. + stop_tint = style.ok if result.succeeded else style.warn lines = [ - f"goal : {goal}", - f"model : {model_description}", - f"registry : {registry_target}", - f"kinds : {', '.join(sorted(registry.names())) or '(none)'}", - f"policy : {policy_description} [{policy_source}]", - f"config : {settings.describe()}", + style.kv("goal", goal, width=style.LABEL_WIDTH), + style.kv("model", model_description, width=style.LABEL_WIDTH, tint=style.accent), + style.kv("registry", registry_target, width=style.LABEL_WIDTH, tint=style.accent), + style.kv( + "kinds", + ", ".join(sorted(registry.names())) or "(none)", + width=style.LABEL_WIDTH, + ), + style.kv( + "policy", + f"{policy_description} {style.dim(f'[{policy_source}]')}", + width=style.LABEL_WIDTH, + ), + style.kv("config", settings.describe(), width=style.LABEL_WIDTH, tint=style.dim), "", - f"stopped : {result.stop.value} ({result.detail})", - f"rounds : {len(rounds)} of max {max_rounds}", + style.kv( + "stopped", + f"{stop_tint(result.stop.value)} {style.dim(f'({result.detail})')}", + width=style.LABEL_WIDTH, + ), + style.kv("rounds", f"{len(rounds)} of max {max_rounds}", width=style.LABEL_WIDTH), ] for record in rounds: - note = f" rejected: {', '.join(record['rejections'])}" if record["rejections"] else "" + codes = ", ".join(record["rejections"]) + note = f" {style.dim('rejected:')} {style.err(codes)}" if codes else "" + # A round nobody proposed is neither: amber, not a green it did not earn. + verdict = {"admitted": True, "rejected": False}.get(str(record["status"])) lines.append( - f" round {record['round']}: {record['status']:<9} " - f"nodes={record['nodes']} executed={record['executed']}{note}" + f" round {record['round']}: " + f"{style.cell(str(record['status']), 9, tint=style.tint_for(verdict))} " + f"{style.dim('nodes=')}{record['nodes']} " + f"{style.dim('executed=')}{record['executed']}{note}" ) lines += [ "", - f"state : {result.state}", - f"trace : {trace_path}", + style.kv("state", str(result.state), width=style.LABEL_WIDTH), + style.kv("trace", str(trace_path), width=style.LABEL_WIDTH, tint=style.accent), ] emit(payload, lines, as_json=as_json) diff --git a/grapharc/cli/probe.py b/grapharc/cli/probe.py index bd35af9..fa0936d 100644 --- a/grapharc/cli/probe.py +++ b/grapharc/cli/probe.py @@ -18,6 +18,14 @@ import shutil from typing import Any +from grapharc.cli import style + +#: The three columns every row prints, in characters. Constants, not measurements +#: of the terminal: `tests/test_cli.py` asserts on `"bedrock unprobed"`, and +#: the page in `docs/cookbook/02-models.md` shows this table verbatim. +BACKEND_COLUMN = 12 +MARK_COLUMN = 9 + # A backend that is a test double rather than a provider. `--check` exits # non-zero when nothing real is usable, and `mock` being always-usable must not # make an unconfigured machine look ready. @@ -172,15 +180,28 @@ def any_provider_usable(results: list[dict[str, Any]]) -> bool: def render(results: list[dict[str, Any]]) -> list[str]: + """The table `grapharc models --check` prints. + + On a terminal the mark column carries the verdict — green usable, red + unusable, amber for a backend nobody wrote a probe for — which is the one + thing a reader is scanning this table for. The columns are the same width + either way, and the credential line stays indented under its own row. + """ lines = [] for r in results: mark = {True: "usable", False: "unusable", None: "unprobed"}[r["usable"]] - lines.append(f"{r['backend']:<12} {mark:<9} {r['detail']}") + lines.append( + f"{style.cell(r['backend'], BACKEND_COLUMN, tint=style.accent)} " + f"{style.cell(mark, MARK_COLUMN, tint=style.tint_for(r['usable']))} " + f"{r['detail']}" + ) if r["credential"]: - lines.append(f"{'':<12} {'':<9} credential: {r['credential']}") + indent = " " * (BACKEND_COLUMN + 1 + MARK_COLUMN + 1) + lines.append(f"{indent}{style.dim('credential:')} {r['credential']}") lines.append("") - lines.append("local probe only — no provider was contacted, so a configured key") - lines.append("is not a validated one.") + # The caveat, not the answer: dim, so the table above it reads first. + lines.append(style.dim("local probe only — no provider was contacted, so a configured key")) + lines.append(style.dim("is not a validated one.")) return lines diff --git a/grapharc/cli/serve.py b/grapharc/cli/serve.py index dbe53f8..357ea4e 100644 --- a/grapharc/cli/serve.py +++ b/grapharc/cli/serve.py @@ -16,12 +16,17 @@ import sys from typing import Any -from grapharc.cli import optional +from grapharc.cli import optional, style from grapharc.cli.output import EXIT_OK, emit, fail SERVER_HINT = "Install the server extra with: uv sync --extra server" UVICORN_HINT = "Install uvicorn with: uv sync --extra server" +#: `serve` lines its one label up at nine characters, not the ten the report +#: commands use. All three of its lines are printed verbatim in +#: `docs/cookbook/06-serving-and-ops.md`, so the number is not up for revision. +LABEL_WIDTH = 9 + def resolve_registry(target: str) -> Any: """Import `module:attr` and return the graph registry it names. @@ -93,16 +98,22 @@ def serve( "registry": registry_target, "graphs": names, } - lines = [f"serving grapharc.server on http://{host}:{port}"] - lines.append( - f"graphs : {', '.join(names)}" + graphs = ( + ", ".join(names) if names - else "graphs : none registered — pass --registry module:attr, or every " + else "none registered — pass --registry module:attr, or every " "create-session request will 404" ) - lines.append("ctrl-c to stop") - # Printed before the server blocks: a caller watching stdout for the URL - # would otherwise wait for the process to exit to learn it. + lines = [ + f"serving grapharc.server on {style.accent(f'http://{host}:{port}')}", + # An empty registry is a legitimate way to check the process comes up and + # a useless way to run anything, so on a terminal it is amber. + style.kv("graphs", graphs, width=LABEL_WIDTH, tint=None if names else style.warn), + style.dim("ctrl-c to stop"), + ] + # Printed *and flushed* before the server blocks: a caller watching stdout for + # the URL would otherwise wait for the process to exit to learn it. Nothing + # here is buffered, deferred, or drawn on a timer for the same reason. emit(payload, lines, as_json=as_json) sys.stdout.flush() diff --git a/grapharc/cli/style.py b/grapharc/cli/style.py new file mode 100644 index 0000000..1591c87 --- /dev/null +++ b/grapharc/cli/style.py @@ -0,0 +1,344 @@ +"""Colour for the only reader who can see it: a person at a terminal. + +Every helper here is the identity function unless the stream it is writing to is +an interactive terminal. That is not politeness, it is the design: this CLI's +human output is *pinned*. `tests/test_readme.py` byte-compares `grapharc plan` +against a fenced block in `README.md`, and `tests/test_cookbook_models.py` runs +`grapharc models` in a subprocess and byte-compares it against +`docs/cookbook/02-models.md`. Both read a pipe. So a pipe gets exactly the bytes +it got before this module existed, a terminal gets colour, and the two cannot +drift apart — because they are the same string with escape sequences added. + +Four rules the helpers keep, so a call site cannot break that by accident: + +- **Colour off is identity.** `ok("admitted") == "admitted"`, `kv("goal", g, + width=10) == f"{'goal':<10}: {g}"`. A styled line and the line it replaced are + the same object to `==`, to `grep`, and to a doc test. +- **Padding is never dyed.** `cell()` and `kv()` wrap the *text* in escapes and + leave the spaces outside them, so a column is the same number of characters + wide whether or not it is coloured, and a dyed cell cannot swallow its own + padding into a colour a terminal renders differently. +- **Only text is coloured, never structure.** Every call site in `grapharc.cli` + keeps the lines, words, glyphs and column widths it printed before — a + terminal and a pipe differ in colour and in nothing else, so `README.md` is + still a transcript of what you actually see. `rule()` and `bullet()` are the + two helpers that would *draw* something; nothing emits them today, and a call + site that starts to should emit them only when `enabled()`. +- **The terminal is measured only when colour is on.** A width read from + `shutil.get_terminal_size()` in piped output would make a pinned test depend + on the window that ran it. + +Stdlib only, and free to import: `grapharc.cli.main` imports this at module +scope, and `tests/test_cli.py::test_building_the_parser_imports_no_optional_package` +pins that building the parser pulls in no optional package. +""" + +from __future__ import annotations + +import os +import shutil +import sys +from collections.abc import Callable, Iterable +from typing import TextIO + +#: A tint: `(256-colour index, basic SGR parameter)`. Both are carried because a +#: 256-colour escape on a 16-colour terminal renders as nothing at all on some +#: emulators, and guessing wrong there costs the reader the text, not the colour. +Tone = tuple[int, str] + +_RESET = "\x1b[0m" + +# Mid-tone on purpose. A palette tuned for a dark background disappears on a +# light one, and plenty of terminals are light; these five stay legible on both. +# The warm orange is the accent the rest of the project is branded in. +ACCENT: Tone = (173, "33") # warm orange — names, paths, the thing you asked for +OK: Tone = (71, "32") # medium green — it worked, it was admitted, it is usable +WARN: Tone = (172, "33") # amber — it ran and the answer is unwelcome +ERROR: Tone = (167, "31") # soft red — it was refused, it failed, it is unusable +MUTED: Tone = (245, "2") # mid grey — labels, units, prose about the output + +#: The label column `plan`, `agent` and `run` share: they all print +#: `f"{key:<10}: value"`, and lining their blocks up with each other is the whole +#: reason the number is the same in three files. It is a constant and not a +#: measurement of the terminal because `README.md` and the cookbook print these +#: blocks verbatim and `tests/test_readme.py` byte-compares them. +LABEL_WIDTH = 10 + +#: Used by `rule()` when colour is off, instead of the terminal's width. +RULE_WIDTH = 60 +#: The widest rule drawn on a terminal, however wide the window is: a line that +#: runs the full width of a 200-column window reads as a mistake. +RULE_MAX = 72 + +_RULE_GLYPH = "─" +_BULLET_GLYPH = "·" + +#: `None` means "decide per stream"; `False` is `--no-color` (or `--json`, which +#: has no human reader). Set by `configure()`, read by `enabled()`. +_override: bool | None = None + + +# -- the decision ------------------------------------------------------------- + + +def configure(*, no_color: bool = False) -> None: + """Apply the colour decision the parsed flags made. + + Assigned on every call rather than latched, so one `main()` cannot decide the + colour of the next one in the same interpreter — the test suite calls `main()` + hundreds of times in one process, and a sticky flag set by whichever test ran + first is a failure that only reproduces in a full run. + """ + global _override + _override = False if no_color else None + + +def enabled(stream: TextIO | None = None) -> bool: + """Whether `stream` (default stdout) should be written with colour. + + Asked at print time, not at import time: under `pytest`'s `capsys` and under + `contextlib.redirect_stdout` the answer changes after this module is loaded, + and it has to be the current stream's answer or the pinned tests see escapes. + + `TERM=dumb` and `NO_COLOR` beat `FORCE_COLOR`, because refusing to colour is + never the destructive mistake. + """ + if _override is not None: + return _override + if os.environ.get("TERM") == "dumb" or "NO_COLOR" in os.environ: + return False + forced = os.environ.get("FORCE_COLOR") + if forced is not None and forced != "0": + return True + try: + return bool((sys.stdout if stream is None else stream).isatty()) + except (AttributeError, ValueError): + # A stream that is closed, or is not a stream at all. Nobody is reading + # it interactively; plain text is the safe answer. + return False + + +def _depth() -> int: + """256 when the terminal advertises it, else 16.""" + if os.environ.get("COLORTERM") in ("truecolor", "24bit"): + return 256 + return 256 if "256" in os.environ.get("TERM", "") else 16 + + +def _paint(text: str, tone: Tone, stream: TextIO | None = None) -> str: + """`text` in `tone`, or `text`. Empty strings are never wrapped in escapes. + + That last part is load-bearing: `cell("", 12)` is how a continuation line + keeps a column empty, and escapes around nothing would be escapes printed + for nothing. + """ + if not text or not enabled(stream): + return text + code = f"38;5;{tone[0]}" if _depth() == 256 else tone[1] + return f"\x1b[{code}m{text}{_RESET}" + + +def _drawable(text: str, fallback: str, stream: TextIO | None = None) -> str: + """`text` if the stream can encode it, else `fallback`. + + A box-drawing character is worth nothing if printing it raises + `UnicodeEncodeError` and takes the whole command's output with it. + """ + encoding = getattr(stream or sys.stdout, "encoding", None) or "utf-8" + try: + text.encode(encoding) + except (UnicodeEncodeError, LookupError): + return fallback + return text + + +# -- semantic helpers --------------------------------------------------------- +# +# Named for what the text *means*, not for a colour: the call sites say `ok(…)` +# and `err(…)`, so the palette can be re-tuned here and nowhere else. Every one +# of them returns its argument unchanged when colour is off. + + +def accent(text: str, *, stream: TextIO | None = None) -> str: + """A name the reader came for: a model spec, a path, a tool, a URL.""" + return _paint(text, ACCENT, stream) + + +def ok(text: str, *, stream: TextIO | None = None) -> str: + """An affirmative outcome: admitted, usable, executed, the goal was met.""" + return _paint(text, OK, stream) + + +def warn(text: str, *, stream: TextIO | None = None) -> str: + """It ran, and the answer is one the reader will not like.""" + return _paint(text, WARN, stream) + + +def err(text: str, *, stream: TextIO | None = None) -> str: + """A refusal or a failure. Also the `error:` prefix on stderr.""" + return _paint(text, ERROR, stream) + + +def dim(text: str, *, stream: TextIO | None = None) -> str: + """Secondary text: units, provenance, the sentence explaining the output.""" + return _paint(text, MUTED, stream) + + +def label(text: str, *, stream: TextIO | None = None) -> str: + """The key of a `key: value` line — structure, not content.""" + return _paint(text, MUTED, stream) + + +def value(text: str, *, stream: TextIO | None = None) -> str: + """The payload of a `key: value` line, deliberately left alone. + + It exists so a call site can say which half is the answer, and it returns the + text unchanged in *both* modes on purpose: the payload is what the reader is + actually here to read, and every colour available for it is a colour chosen + against somebody's terminal theme. The default foreground is the one colour + guaranteed to be legible. + + It takes (and ignores) `stream` so that it has the same signature as every + other tint and can be handed to `kv(tint=…)` like one — which is exactly what + `kv` does with it by default. + """ + return text + + +def bold(text: str, *, stream: TextIO | None = None) -> str: + """Weight rather than colour, for the rare line that has to win outright.""" + if not text or not enabled(stream): + return text + return f"\x1b[1m{text}{_RESET}" + + +def heading(text: str, *, stream: TextIO | None = None) -> str: + """A section title inside an otherwise flat block, e.g. `examples:`.""" + if not text or not enabled(stream): + return text + code = f"1;38;5;{ACCENT[0]}" if _depth() == 256 else f"1;{ACCENT[1]}" + return f"\x1b[{code}m{text}{_RESET}" + + +def tint_for(good: bool | None) -> Callable[..., str]: + """The tint for a yes / no / nobody-checked outcome. + + One place decides that "usable" and "admitted" are the same green and that an + unprobed backend is amber rather than red — three call sites each choosing + for themselves is how a CLI ends up with two greens. + """ + return {True: ok, False: err, None: warn}[good] + + +def rule(width: int | None = None, *, stream: TextIO | None = None) -> str: + """A horizontal rule. + + The width comes from the terminal only when colour is on. Piped output gets + `RULE_WIDTH`, a constant, because a rule whose length depends on the window + that produced it is a diff waiting to happen in anything that captures it. + """ + if width is None: + if enabled(stream): + width = min(shutil.get_terminal_size((RULE_WIDTH, 24)).columns, RULE_MAX) + else: + width = RULE_WIDTH + return dim(_drawable(_RULE_GLYPH, "-", stream) * max(0, width), stream=stream) + + +def bullet(text: str, *, marker: str = _BULLET_GLYPH, stream: TextIO | None = None) -> str: + """A list item: a dimmed marker, a space, then the text.""" + return f"{dim(_drawable(marker, '-', stream), stream=stream)} {text}" + + +# -- fixed-width blocks ------------------------------------------------------- + + +def cell( + text: str, + width: int, + *, + tint: Callable[..., str] | None = None, + stream: TextIO | None = None, +) -> str: + """`text` padded to `width`, exactly as `f"{text: str: + """One `key: value` line whose plain form is `f"{key: list[str]: + """`kv()` over a mapping's items, in order.""" + return [kv(k, v, width=width, sep=sep, stream=stream) for k, v in pairs] + + +def error_prefix(stream: TextIO | None = None) -> str: + """The literal `error: ` every text-mode failure starts with, dyed on a tty. + + Gated on **stderr**, which is where it goes: `grapharc … 2> log` on a + terminal must not put escape sequences in the log, and the substring + assertions in `tests/test_cli.py` read a captured (non-tty) stderr. + """ + return f"{err('error:', stream=stream or sys.stderr)} " + + +__all__ = [ + "ACCENT", + "ERROR", + "LABEL_WIDTH", + "MUTED", + "OK", + "RULE_MAX", + "RULE_WIDTH", + "WARN", + "Tone", + "accent", + "bold", + "bullet", + "cell", + "configure", + "dim", + "enabled", + "err", + "error_prefix", + "heading", + "kv", + "kv_block", + "label", + "ok", + "rule", + "tint_for", + "value", + "warn", +] From 7abcd53d0ac4e81ffe153839566601d4d7e62a0b Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Thu, 30 Jul 2026 04:42:58 +0530 Subject: [PATCH 4/6] Record the fixed defects in README, and pin the issue format The status section now states each defect that was closed and what it cost while open, in the same register as the rest of the page: the poisoned deadline guard, the async double-charge, the prose-bracket parser, the fan-out attribution, and the round event that was a measurement when it should have been an envelope. .github/ISSUE_TEMPLATE/task.md fixes the seven-section shape used by the ten issues opened against this repo, so every future issue arrives with a summary, why it matters, where in the code with a command to confirm it, what to change, how to verify, acceptance criteria, and an explicit skill level. It also states the house rule out loud: a change arrives with a test that fails without it, checked by reverting the source and watching the test go red. uv.lock is a stale-lock refresh, not a dependency change: pyproject already said 0.1.1 while the lock still said 0.1.0a0. Co-Authored-By: Claude Opus 5 (1M context) --- .github/ISSUE_TEMPLATE/task.md | 72 ++++++++++++++++++++++++++++++++++ README.md | 6 +++ uv.lock | 48 +++++++++++++++++++++-- 3 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 .github/ISSUE_TEMPLATE/task.md diff --git a/.github/ISSUE_TEMPLATE/task.md b/.github/ISSUE_TEMPLATE/task.md new file mode 100644 index 0000000..7603be2 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/task.md @@ -0,0 +1,72 @@ +--- +name: Task or defect +about: The standard format for every issue in this repository +title: "area: what is wrong or missing, stated plainly" +labels: '' +assignees: '' +--- + + + +## Summary + +What is wrong or missing, in one short paragraph. Quote the offending code or the +real command output rather than paraphrasing it. + +## Why this matters + +What breaks, or what a user cannot do, as a consequence. Prefer a concrete +failure over an adjective. + +## Where in the code + +- `path/to/file.py:LINE` — what is there and why it is relevant + +Include a command a reader can run to confirm the problem for themselves: + +```bash +``` + +## What to change + +1. … +2. … + +State anything deliberately out of scope, so a pull request does not grow past +what was agreed here. + +## How to verify + +```bash +uv run pytest -q +uv run ruff check . +``` + +Note which new test proves the fix. This repo's convention is that a change +arrives with a test that **fails without it** — check that by reverting your +source edit and watching the new test go red. + +## Acceptance criteria + +- [ ] … +- [ ] `uv run pytest` stays green and `uv run ruff check .` is clean +- [ ] Any README or cookbook sentence this changes is updated in the same pull + request (several are byte-compared against real output by the test suite) + +## Skill level + +Pick one and delete the other. + +**good first issue** — say why it is well bounded, point at a sibling file that +shows the pattern to copy, and invite questions on the issue. + +**experience required** — say which subsystems the change spans, what could +silently break, and ask for a design comment on the issue before any code is +written. diff --git a/README.md b/README.md index 9df644a..e835034 100644 --- a/README.md +++ b/README.md @@ -480,15 +480,21 @@ Re-derived on 2026-07-28 by running each item, not by reading the commit log. - **The HTTP API does not use the durable session layer.** It has its own `InProcessRuntime`, whose sessions die with the process and whose approvals are recorded without being delivered. [ROADMAP.md](ROADMAP.md) §12.3. - *Closed:* `grapharc plan` drives the governed loop; `PolicyEngine.edge_policy()` compiles the TOML document into the gate `AdmissionChecker` consults, and `grapharc plan --policy` is the caller; `grapharc demo --memory PATH` hands the shipped graphs the durable SQLite store. +- *Closed:* the shipped registry withheld the trace recorder from its `PlannerNode` and `Materializer`, so `grapharc plan` wrote a file with no `plan` event and **no `start`/`end` pair for any node it executed** — the paragraph above claiming otherwise was true of a hand-wired loop and false of the one the command drives. Both now get the recorder, and a test asserts the phase counts. **Real limits of things that do work** - **Admission authorises a kind, not its arguments.** A proposal carrying `args={"path": "/etc/passwd"}` is admitted on the strength of its kind alone. - **The audit-hook sandbox is in-process confinement, not a kernel boundary.** `os.stat` outside the workspace is not blocked, because CPython raises no event for it. `ContainerExecutor` is the boundary where one is needed. - **`run_command` is not confined.** Argv-only and never a shell, but the child is an ordinary process with your privileges. +- *Closed:* a `max_seconds` past the platform's `time_t` — `float("inf")`, or a plausible "effectively unlimited" like `1e10` — used to **disable the deadline guard for the rest of the process**. `setitimer` raised *after* the SIGALRM handler was installed and the process-wide slot taken, leaking both, so every later run silently fell back to the mechanism that cannot unwind a blocking syscall: a 0.3s ceiling then took a 5s sleep to notice. Arming is undone on failure now, and the armed delay is clamped to what both mechanisms accept. +- *Closed:* every `async def` node **double-charged** its token re-reports. The re-report ledger was keyed by thread ident, but `on_llm_end` is sync — under `ainvoke` LangChain dispatches it to a worker thread while the body stays on the event loop — so the automatic charge found no ledger and the node's named re-report was charged again. Any node using the shipped `charge_usage`, `AgentNode._charge_tokens` or `planner.proposal._charge` reported double its real spend and hit `max_tokens` at half its declared allowance. The ledger is a `contextvars` scope now, which also fixes an inner scope discarding the enclosing node's. +- *Closed:* a bracket anywhere in a model's **prose** hijacked JSON extraction, because only the first `{`/`[` was ever tried. `Based on the context [lines 3-5]: {…}` was rejected as unparseable, and — worse — `Analysis (note [1]): {"supported": false}` returned a perfectly valid `[1]`, substituting a fabricated value for the verifier's actual answer. Every opener is tried now, longest parse wins; junk still returns `None`, so fail-closed is unchanged. - **`interrupt()` suspends but cannot be resumed.** LangGraph's native interrupt stops the graph and shows on `get_state`, and there is no supported resume path — resuming means passing a `Command` as *input*, which is closed by design. Use the session layer's approval gate for human-in-the-loop. - **Still unwrapped from LangGraph:** `retry_policy`, `cache_policy`, `durability`, subgraphs. `.inner` reaches them, but execution entry points there fail closed, so `.inner` is an inspection escape hatch and not a way to run the graph. - **Cost is recorded when a backend reports one, estimated when it does not.** Both gateways publish the provider's `cost_usd` through the same `llm_output` envelope, the runtime's usage callback writes it onto the node's `end` event, and an agent's `model` events carry the per-call breakdown. A backend that reports no price still falls back to a `RateCard` estimate, and the two figures stay apart — `recorded_cost_usd` is never a guess. Still missing: no tenant on a trace event, so per-tenant attribution is not offered. +- **A node's tokens are its own, not the run's movement while it ran.** Worth stating because it was the other way round: an `end` event carried the difference between two readings of the run's *shared* meter, so under fan-out the workers' windows overlapped and each was credited with its siblings' concurrent spend. Three workers costing 8 tokens each traced as 24/16/8, and `metrics` and `cost` agreed on 48 for 24 tokens of real work — doubling the estimated bill purely because the work ran in parallel. Attribution now comes from a per-node scope on the meter, so the same work costs the same serially and in parallel; a hand charge the usage callback never saw still lands on the node that made it. +- **A planning round is an envelope, not a measurement.** A `round` event used to carry the planner's `tokens` and the round's `duration_ms`, both of which `metrics`, `cost` and `replay` add on top of node totals — and the planner's spend was already reported by its own `plan` event, so it was counted twice, and a round's duration encloses the plan plus every node it 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. `RoundRecord.iterations` also holds a figure now rather than always `0`. - **The Claude CLI backend is completion-only.** Tool calling and structured output need one of the OpenAI-wire backends: `openrouter`, `openai`, or a local `ollama`. - **A session turn is synchronous**, and a runner claim is a claim rather than a lease — nothing reclaims a session whose runner died holding it. - **A bare model spec resolves to the paid `claude-cli` backend.** `--model mock` does not reach the scripted double; it becomes the model name `mock` on the subscription backend. Only the slash form (`mock/anything`) reaches the double. A mistyped backend *with* a slash is rejected properly, exit 2. diff --git a/uv.lock b/uv.lock index 3d1d785..080d67e 100644 --- a/uv.lock +++ b/uv.lock @@ -350,7 +350,7 @@ wheels = [ [[package]] name = "grapharc" -version = "0.1.0a0" +version = "0.1.1" source = { editable = "." } dependencies = [ { name = "langchain-core" }, @@ -369,17 +369,27 @@ all = [ { name = "opentelemetry-api" }, { name = "opentelemetry-exporter-otlp-proto-http" }, { name = "opentelemetry-sdk" }, + { name = "real-ladybug" }, { name = "uvicorn" }, ] api = [ { name = "anthropic" }, ] +ladybug = [ + { name = "real-ladybug" }, +] mcp = [ { name = "mcp" }, ] memory = [ { name = "neo4j" }, ] +ollama = [ + { name = "langchain-openai" }, +] +openai = [ + { name = "langchain-openai" }, +] openrouter = [ { name = "langchain-openai" }, ] @@ -406,8 +416,10 @@ dev = [ requires-dist = [ { name = "anthropic", marker = "extra == 'api'", specifier = ">=0.40" }, { name = "fastapi", marker = "extra == 'server'", specifier = ">=0.115" }, - { name = "grapharc", extras = ["api", "mcp", "memory", "openrouter", "otel", "server"], marker = "extra == 'all'" }, + { name = "grapharc", extras = ["api", "ladybug", "mcp", "memory", "ollama", "openai", "openrouter", "otel", "server"], marker = "extra == 'all'" }, { name = "langchain-core", specifier = ">=0.3" }, + { name = "langchain-openai", marker = "extra == 'ollama'", specifier = ">=0.2" }, + { name = "langchain-openai", marker = "extra == 'openai'", specifier = ">=0.2" }, { name = "langchain-openai", marker = "extra == 'openrouter'", specifier = ">=0.2" }, { name = "langgraph", specifier = ">=0.4" }, { name = "langgraph-checkpoint-sqlite", specifier = ">=2.0" }, @@ -417,9 +429,10 @@ requires-dist = [ { name = "opentelemetry-exporter-otlp-proto-http", marker = "extra == 'otel'", specifier = ">=1.27" }, { name = "opentelemetry-sdk", marker = "extra == 'otel'", specifier = ">=1.27" }, { name = "pydantic", specifier = ">=2.7" }, + { name = "real-ladybug", marker = "extra == 'ladybug'", specifier = ">=0.15.3" }, { name = "uvicorn", marker = "extra == 'server'", specifier = ">=0.32" }, ] -provides-extras = ["openrouter", "server", "mcp", "memory", "otel", "api", "all"] +provides-extras = ["openrouter", "openai", "ollama", "server", "mcp", "memory", "ladybug", "otel", "api", "all"] [package.metadata.requires-dev] dev = [ @@ -1285,6 +1298,35 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/f1/12/de94a39c2ef588c7e6455cfbe7343d3b2dc9d6b6b2f40c4c6565744c873d/pyyaml-6.0.3-cp314-cp314t-win_arm64.whl", hash = "sha256:ebc55a14a21cb14062aa4162f906cd962b28e2e9ea38f9b4391244cd8de4ae0b", size = 149341, upload-time = "2025-09-25T21:32:56.828Z" }, ] +[[package]] +name = "real-ladybug" +version = "0.15.3" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/07/bc/a9d27215dcbe3fe30262468010c1924b231708cda4f346ecb22c5b05736d/real_ladybug-0.15.3.tar.gz", hash = "sha256:2556e5274e5d0ea8d9ef2cf989b81d91684511696afd89af1fcecfaedf649849", size = 9941787, upload-time = "2026-04-01T07:58:29.101Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/26/a2/a16eddbd05a2cc382ccf77565eb39e34a7ef3470f9825f7cdf7367f7ccd4/real_ladybug-0.15.3-cp312-cp312-macosx_13_0_arm64.whl", hash = "sha256:34fc2d140460fcb334003b7403b842d5d0528ca75d514acec26d8b7b68a7dbb6", size = 4017997, upload-time = "2026-04-01T07:57:46.158Z" }, + { url = "https://files.pythonhosted.org/packages/cb/e3/17020d3579f6f8b564c123b46ee1db27c438379ebc9ad23eef35474bf704/real_ladybug-0.15.3-cp312-cp312-macosx_13_0_x86_64.whl", hash = "sha256:13f83af6450215617d013a0f35439d8b161841fdd7e8d3e7c35161dfd9004659", size = 4484739, upload-time = "2026-04-01T07:57:47.847Z" }, + { url = "https://files.pythonhosted.org/packages/83/8d/37e663a851fb5929f4fd791768883dc7f483bad6295d51224ffbdeb405ed/real_ladybug-0.15.3-cp312-cp312-manylinux_2_26_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:e32e4a4c88f1efd894d2df15ad5697a60a07aeccd40e3536776bcb93e1246ef1", size = 6869231, upload-time = "2026-04-01T07:57:49.695Z" }, + { url = "https://files.pythonhosted.org/packages/94/b4/0cc5898ce664a3522c77ce95d714c04dfb1eaea42d623f04e96fea837ed1/real_ladybug-0.15.3-cp312-cp312-manylinux_2_27_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:6344a2a53264ca52c2686d9125f09ef28fc2423743e123119c082a0ce0e5bc6c", size = 7772857, upload-time = "2026-04-01T07:57:51.645Z" }, + { url = "https://files.pythonhosted.org/packages/64/1c/9380c2921d8e53cf19b4dd9752ac2d411257daf147e2ccca8a932581af22/real_ladybug-0.15.3-cp312-cp312-musllinux_1_2_aarch64.whl", hash = "sha256:32d63ced92cde4ec4efdb7ff9b00e780485feabd10afb318584fd820a6a80fb5", size = 8037522, upload-time = "2026-04-01T07:57:53.457Z" }, + { url = "https://files.pythonhosted.org/packages/f6/5b/fba8899f31bbb1a480af5cdd191605b3bbd3d17bcd140c7c8f9e2e7c1ddf/real_ladybug-0.15.3-cp312-cp312-musllinux_1_2_x86_64.whl", hash = "sha256:7cbf81c9587b9aba15cf2fd976a61a4503e82187d947abe028288ea4eb87662f", size = 8788363, upload-time = "2026-04-01T07:57:55.34Z" }, + { url = "https://files.pythonhosted.org/packages/27/30/57a22ccce2c582dca5b27e14396dc6b2164c2eda1594e314e88c24779546/real_ladybug-0.15.3-cp312-cp312-win_amd64.whl", hash = "sha256:346dfa1051cd9a0aa1b99ddc6db8490d7f203a1ddd6204176b80a80e8cbd10c3", size = 5079466, upload-time = "2026-04-01T07:57:57.569Z" }, + { url = "https://files.pythonhosted.org/packages/bf/76/0e584ead4e91c7ee9d055a036e9d17d73ee594f37905d83d2d15d7300688/real_ladybug-0.15.3-cp313-cp313-macosx_13_0_arm64.whl", hash = "sha256:096e847f6bd69d913021f7ad883b02e4640bf9f47741111caf1c40caed510379", size = 4018193, upload-time = "2026-04-01T07:57:59.446Z" }, + { url = "https://files.pythonhosted.org/packages/f2/f9/c89ebe40642a4c8a7484db003f717f1021cdd3418ea9c752249db83dc8b2/real_ladybug-0.15.3-cp313-cp313-macosx_13_0_x86_64.whl", hash = "sha256:9d0b00e57ef1ef0509e06132250360f8807184f76359c3580989d9713faa491c", size = 4484856, upload-time = "2026-04-01T07:58:01.404Z" }, + { url = "https://files.pythonhosted.org/packages/fa/05/3cf897e75514d67347009870f8127ffa2b5326072eb777bc7c03836e7112/real_ladybug-0.15.3-cp313-cp313-manylinux_2_26_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:42cc920e6c9d68dd324c9d487626eaeb16641f9dc8bb0e712efd0bce24c702a0", size = 6869612, upload-time = "2026-04-01T07:58:03.234Z" }, + { url = "https://files.pythonhosted.org/packages/69/37/447856d8c8167bf356e0214faa0b99e4f9cb7e87a26ba66ca1fad8bba320/real_ladybug-0.15.3-cp313-cp313-manylinux_2_27_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:a56c84e75be098fd472fe036e3a4ab69646add34a1c2e8b99a421951ef9ddb43", size = 7772858, upload-time = "2026-04-01T07:58:05.348Z" }, + { url = "https://files.pythonhosted.org/packages/97/d9/b7079492bcfe23152eb0f50317ea0900de1eafffa3d3272fd2db84c48ded/real_ladybug-0.15.3-cp313-cp313-musllinux_1_2_aarch64.whl", hash = "sha256:5619bd6aa22446d2f2105f86122b5ed403f8e44bfa0d38ea2cc2569814cce59a", size = 8037607, upload-time = "2026-04-01T07:58:07.254Z" }, + { url = "https://files.pythonhosted.org/packages/9d/0d/ff47fa7e54d43de05ba6eaf5b6f2947a6a6fe35392e2a493897215fa99f7/real_ladybug-0.15.3-cp313-cp313-musllinux_1_2_x86_64.whl", hash = "sha256:a12cba80c387e576a7a66298fa0458571a38332af83f27e6725e1dfe6ce79841", size = 8787964, upload-time = "2026-04-01T07:58:09.313Z" }, + { url = "https://files.pythonhosted.org/packages/15/b9/33cf7444eb231a331a05aab3589dcdf6a19256e89d34b6b2d0dd3c70d15b/real_ladybug-0.15.3-cp313-cp313-win_amd64.whl", hash = "sha256:6e38f3299dd0be07cf018a457d43773c6e97035b7762a56863f9676acf0ed0de", size = 5079685, upload-time = "2026-04-01T07:58:11.769Z" }, + { url = "https://files.pythonhosted.org/packages/4d/cf/e08dae74c2029793f676b3ed66fcbfcbb0f91c5be4451d634d2be8ca8a97/real_ladybug-0.15.3-cp314-cp314-macosx_13_0_arm64.whl", hash = "sha256:ba0bbc42febc099c068a4bdc6b5aacbff3057561aab9595f31083e687bcd8f99", size = 4019196, upload-time = "2026-04-01T07:58:13.646Z" }, + { url = "https://files.pythonhosted.org/packages/cd/a2/5f5ebee452f4fc29244ccb80c7c7b48d324995600b71c3c5d69cceeef0de/real_ladybug-0.15.3-cp314-cp314-macosx_13_0_x86_64.whl", hash = "sha256:20bf040361126484a699147a2d523f03ed45b59e6e895a218d3bb0290fe249b7", size = 4485134, upload-time = "2026-04-01T07:58:15.529Z" }, + { url = "https://files.pythonhosted.org/packages/4c/b4/286ca609ce45eb5c22b1d19a3c2c04d87783db9bf8844a52428939580cc0/real_ladybug-0.15.3-cp314-cp314-manylinux_2_26_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:5dfe2c3e0de213c6a1fbc327dca1d584533fec88a9fb58c8b6fa4f9b1f40d772", size = 6870677, upload-time = "2026-04-01T07:58:17.091Z" }, + { url = "https://files.pythonhosted.org/packages/88/fc/a4ac31116c01f4ac7f8781b1280078d7fe54cb679eae9297411de8acaa84/real_ladybug-0.15.3-cp314-cp314-manylinux_2_27_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:cc30aca624d65dd65034277b8bae575c6b3523d3fdc034a81865e5ee5767cd7f", size = 7772549, upload-time = "2026-04-01T07:58:19.166Z" }, + { url = "https://files.pythonhosted.org/packages/ff/27/fddd0f5d0d544a0e9748e02510598c49e570b23f14e1ae6e98f10c17edb7/real_ladybug-0.15.3-cp314-cp314-musllinux_1_2_aarch64.whl", hash = "sha256:cd12bb017befab2e0c6c31ac3723f37088c440cdc41bdacd9514e1b248d6827a", size = 8037579, upload-time = "2026-04-01T07:58:22.748Z" }, + { url = "https://files.pythonhosted.org/packages/73/5b/9ad06eb517c4901f956cdd1e67a9cbc105afffc232bbd7f0357ba4568925/real_ladybug-0.15.3-cp314-cp314-musllinux_1_2_x86_64.whl", hash = "sha256:a1d9875644fd4d8408ce2e34030db12e94e45f3c3c8cf0e848b9c06ef3a3238d", size = 8788117, upload-time = "2026-04-01T07:58:24.74Z" }, + { url = "https://files.pythonhosted.org/packages/27/52/49dd500671f84f252285f9dc4fcc2728c2d9bd7e199bd56a784a883c0529/real_ladybug-0.15.3-cp314-cp314-win_amd64.whl", hash = "sha256:c4119fe790624914f36355682a5d3d533036f8bb918f5d7120a7dcd232e0c6f9", size = 5236217, upload-time = "2026-04-01T07:58:26.773Z" }, +] + [[package]] name = "referencing" version = "0.37.0" From b599b819204c1e26e4b1134e8549ace51e45b8f1 Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Thu, 30 Jul 2026 04:54:59 +0530 Subject: [PATCH 5/6] Gate the CLI styling contract, and narrow the stale policy claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tests/test_cli_style.py asserts the property the byte-compared doc pages quietly depend on: strip the escapes from what a terminal receives and it must equal what a pipe receives, exactly. Both directions are covered, and both were checked by mutation rather than assumed. Making enabled() ignore isatty() and always paint leaks colour into pipes; 8 of these tests catch it. The three doc-comparison tests catch that too, for the three commands they pin — but they cannot see `models --check`, `demo`, `viz` or the error path, and they compare only piped output, so a tty-only change to spacing or line count would pass them while making README describe output no user sees. The complementary mutation is the one nothing else caught. Making enabled() always return False — styling silently never happening at all — leaves 124 tests in test_readme, test_cookbook_models and test_cli passing, because every one of them observes a non-tty stream. Only the "a terminal received no styling at all" assertion fails. Without it the feature could rot to a no-op invisibly. The pty is read with both stdout and stderr on one descriptor, which is the shape a person sees and also catches an escape written to stderr while stdout happens to be a terminal. The parent's NO_COLOR and FORCE_COLOR are stripped so a developer's own environment cannot decide what the test proves. README's policy paragraph claimed nothing in the package imports grapharc.policy. That has not been true since `grapharc plan --policy` landed: cli/plan.py and cli/generate.py both call PolicyEngine.edge_policy(), so the edge half is governed by a document you can read. The tool half is not — permission_policy(), check_tool() and approval_router() have no caller outside grapharc/policy/, so `grapharc agent` still builds its gating from --allow/--deny/--ask globs. The paragraph now says which half, and points at the issue tracking the rest. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- tests/test_cli_style.py | 207 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 1 deletion(-) create mode 100644 tests/test_cli_style.py diff --git a/README.md b/README.md index e835034..7ab5bdf 100644 --- a/README.md +++ b/README.md @@ -423,7 +423,7 @@ Three things that phrase over-promises if left alone. **An interrupt does not st **The HTTP API is FastAPI plus SSE** — create a session, list, get, post an event, stream the trace, fetch it as NDJSON, healthz. A request may name a registered graph and supply input and a budget; it may not *describe* a graph, because topology comes from a registry the operator fills in Python. But note the seam: **it does not use the session layer above.** It ships its own in-process runtime whose sessions die with the process, never evict, and record `message` and `approval` events without delivering them into a running graph. Two session layers that have not been joined ([ROADMAP.md](ROADMAP.md) §12.3). -**Policy is a TOML document** over nodes, edges, tools and spend, with tiered evaluation — every `deny` before every `ask` before every `allow`, so a broad deny beats a narrow allow including one scoped to a single tenant. Every decision lands in an audit record naming the rule id, the reason, the policy version and a digest of the document, so a decision can be tied to the exact text that made it. And the seam again, because it is the biggest one in the repo: **nothing in the package imports `grapharc.policy`.** There is no bridge from a policy document to the `EdgePolicy` the admission checker consults, and no call from the agent or the CLI to the `PermissionPolicy` it can produce. The governance a run is actually subject to today is what an operator wrote in Python. +**Policy is a TOML document** over nodes, edges, tools and spend, with tiered evaluation — every `deny` before every `ask` before every `allow`, so a broad deny beats a narrow allow including one scoped to a single tenant. Every decision lands in an audit record naming the rule id, the reason, the policy version and a digest of the document, so a decision can be tied to the exact text that made it. And the seam, now narrowed to exactly half: **the edge half is wired and the tool half is not.** `PolicyEngine.edge_policy()` compiles the document into the `EdgePolicy` the admission checker consults, and `grapharc plan --policy` is a real caller — so what may connect to what *is* governed by a document you can read. But `permission_policy()`, `check_tool()` and `approval_router()` have no caller outside `grapharc/policy/`, so `grapharc agent` still assembles its tool gating from `--allow` / `--deny` / `--ask` globs. The most dangerous surface in the package is the one the document cannot reach yet; [issue #6](https://github.com/CodeGraphContext/GraphARC/issues/6) is that work, and the precedence question it has to settle is what happens when a flag `allow` meets a document `deny`. ## Reading a run afterwards diff --git a/tests/test_cli_style.py b/tests/test_cli_style.py new file mode 100644 index 0000000..3aba527 --- /dev/null +++ b/tests/test_cli_style.py @@ -0,0 +1,207 @@ +"""The CLI is styled on a terminal and byte-identical when piped. + +That second half is load-bearing rather than cosmetic. `tests/test_readme.py` and +`tests/test_cookbook_models.py` byte-compare command output against fenced blocks +in `README.md` and `docs/cookbook/02-models.md`, and both harnesses present a +non-tty stdout — so those pages only stay true while piped output carries no +styling at all. Nothing else asserts that, which means a single call site that +painted unconditionally, or a helper that stopped consulting `isatty()`, would +leak escapes into the docs' own evidence and no test would notice. + +The gate here is the strong form of the property: strip the escapes from what a +terminal receives and it must equal what a pipe receives, exactly. That catches a +leak in either direction — an escape reaching a pipe, and a tty-only change to +*layout* rather than colour, which would make the pages describe output no user +sees. +""" + +from __future__ import annotations + +import os +import re +import selectors +import subprocess +import sys + +import pytest + +pytestmark = pytest.mark.skipif( + not hasattr(os, "openpty"), reason="needs a pty, which Windows has no equivalent for" +) + +# Commands whose human-mode output is styled and deterministic enough to compare. +# `plan` and `demo` are here because they are the two the docs print verbatim. +STYLED = [ + pytest.param(["plan", "investigate the checkout outage"], id="plan"), + pytest.param(["models"], id="models"), + pytest.param(["models", "--check"], id="models-check"), + pytest.param(["demo", "stage0"], id="demo-stage0"), +] + +_ANSI = re.compile(r"\x1b\[[0-9;]*m") +# Each run mints its own scratch directory, so the path is not a property of the +# output. Normalised rather than excluded, so a *missing* path still fails. +_TMPDIR = re.compile(r"/tmp/grapharc-[A-Za-z0-9_.-]+") + + +def _env(**extra: str) -> dict[str, str]: + """A parent-independent environment, so a developer's own TERM or NO_COLOR + cannot decide what this test proves.""" + env = {k: v for k, v in os.environ.items() if k not in {"NO_COLOR", "FORCE_COLOR"}} + env["TERM"] = "xterm-256color" + env["COLUMNS"] = "100" + env["PYTHONIOENCODING"] = "utf-8" + env.update(extra) + return env + + +def _argv(args: list[str]) -> list[str]: + # `-m` rather than the console script: the entry point is not guaranteed to be + # on PATH in a bare checkout, and the module path is what the other CLI tests use. + return [sys.executable, "-m", "grapharc.cli.main", *args] + + +def _piped(args: list[str], **extra: str) -> tuple[str, str, int]: + proc = subprocess.run( # noqa: S603 — argv array, no shell + _argv(args), + capture_output=True, + text=True, + env=_env(**extra), + stdin=subprocess.DEVNULL, + timeout=180, + ) + return proc.stdout, proc.stderr, proc.returncode + + +def _on_pty(args: list[str], **extra: str) -> tuple[str, int]: + """Run with stdout *and* stderr on one pty and return everything written. + + Both on the same pty because that is the shape a person sees, and because it + catches an escape written to stderr while stdout happens to be a terminal. + """ + primary, secondary = os.openpty() + proc = subprocess.Popen( # noqa: S603 — argv array, no shell + _argv(args), + stdout=secondary, + stderr=secondary, + stdin=subprocess.DEVNULL, + env=_env(**extra), + ) + os.close(secondary) + chunks: list[bytes] = [] + selector = selectors.DefaultSelector() + selector.register(primary, selectors.EVENT_READ) + try: + while selector.select(timeout=180): + try: + data = os.read(primary, 65536) + except OSError: # the child closed its end + break + if not data: + break + chunks.append(data) + finally: + selector.close() + proc.wait(timeout=180) + os.close(primary) + # A pty turns "\n" into "\r\n"; that is the terminal's doing, not the CLI's. + text = b"".join(chunks).decode("utf-8", "replace").replace("\r\n", "\n") + return text, proc.returncode + + +def _normalise(text: str) -> str: + return _TMPDIR.sub("/tmp/grapharc-NORMALISED", text) + + +@pytest.mark.parametrize("args", STYLED) +def test_a_terminal_gets_escapes_and_a_pipe_gets_none(args): + """Both halves in one test: styling must be real, and confined to a terminal.""" + on_pty, pty_code = _on_pty(args) + out, err, piped_code = _piped(args) + + assert pty_code == 0 + assert piped_code == 0 + assert _ANSI.search(on_pty), "a terminal received no styling at all" + assert "\x1b" not in out, "an escape reached piped stdout" + assert "\x1b" not in err, "an escape reached piped stderr" + + +@pytest.mark.parametrize("args", STYLED) +def test_stripping_the_escapes_reproduces_the_piped_output_exactly(args): + """The property the byte-compared doc pages depend on. + + Colour must be the *only* difference between what a terminal shows and what a + pipe carries. A tty-only change to spacing, alignment or line count would pass + the leak check above and still make README describe output nobody sees. + """ + on_pty, _ = _on_pty(args) + out, err, _ = _piped(args) + + stripped = _normalise(_ANSI.sub("", on_pty)) + assert stripped == _normalise(out + err) + + +@pytest.mark.parametrize( + ("label", "args", "extra"), + [ + ("NO_COLOR", ["plan", "x"], {"NO_COLOR": "1"}), + ("TERM=dumb", ["plan", "x"], {"TERM": "dumb"}), + ("--no-color", ["plan", "x", "--no-color"], {}), + ("--json", ["plan", "x", "--json"], {}), + ], +) +def test_every_opt_out_silences_styling_even_on_a_terminal(label, args, extra): + on_pty, code = _on_pty(args, **extra) + + assert code == 0 + assert "\x1b" not in on_pty, f"{label} did not silence styling" + + +def test_json_on_a_terminal_is_still_one_clean_document(): + """`--json` promises a parseable document on stdout and nothing on stderr. + + Asserted on a *pty* specifically: that is the one condition under which a + renderer might decide it is allowed to decorate. + """ + import json + + out, err, _ = _piped(["plan", "x", "--json"]) + assert err == "" + assert json.loads(out)["ok"] is True + + on_pty, _ = _on_pty(["plan", "x", "--json"]) + assert "\x1b" not in on_pty + assert json.loads(on_pty)["ok"] is True + + +def test_viz_on_a_terminal_stays_pasteable_mermaid(): + """Being pasteable into a Mermaid renderer is the only reason `viz` exists, so + it is the one command that must not be styled even on a terminal.""" + out, _, _ = _piped(["demo", "stage0"]) + trace = next( + line.split(": ", 1)[1].strip() for line in out.splitlines() if line.startswith("trace: ") + ) + with open(trace, encoding="utf-8") as handle: + run_id = __import__("json").loads(handle.readline())["run_id"] + + on_pty, code = _on_pty(["viz", trace, run_id]) + + assert code == 0 + assert "\x1b" not in on_pty + assert on_pty.lstrip().startswith("flowchart TD") + + +def test_a_text_mode_failure_keeps_stdout_empty_and_says_error_on_stderr(): + """The failure contract, checked with styling live on stderr's own terminal.""" + out, err, code = _piped(["metrics", "/nope/none.jsonl", "r1"]) + + assert code == 2 + assert out == "" + assert err.startswith("error: ") + assert "no such trace file" in err + + on_pty, pty_code = _on_pty(["metrics", "/nope/none.jsonl", "r1"]) + assert pty_code == 2 + # Styled here, but the message body must survive intact for the substring + # assertions elsewhere in the suite. + assert "no such trace file" in _ANSI.sub("", on_pty) From 8d83eda58e38bf08018323f2e46a97b62ebfa770 Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Thu, 30 Jul 2026 13:20:05 +0530 Subject: [PATCH 6/6] Stop pinning exit codes the host owns in the pty styling tests `models --check` exits 1 on a machine that can reach no real provider, which is what CI is. The styling contract is that a terminal does not change the answer, so assert pty and piped exit codes agree instead of pinning 0, and byte-compare only the commands whose output is reproducible across two invocations. Co-Authored-By: Claude Fable 5 --- tests/test_cli_style.py | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/tests/test_cli_style.py b/tests/test_cli_style.py index 3aba527..bfc15e1 100644 --- a/tests/test_cli_style.py +++ b/tests/test_cli_style.py @@ -22,6 +22,7 @@ import selectors import subprocess import sys +import tempfile import pytest @@ -29,8 +30,9 @@ not hasattr(os, "openpty"), reason="needs a pty, which Windows has no equivalent for" ) -# Commands whose human-mode output is styled and deterministic enough to compare. -# `plan` and `demo` are here because they are the two the docs print verbatim. +# Commands with styled human-mode output. Exit codes are deliberately not pinned +# here: `models --check` exits 1 when the host can reach no real provider, which +# is correct and is what a machine with no credentials does. STYLED = [ pytest.param(["plan", "investigate the checkout outage"], id="plan"), pytest.param(["models"], id="models"), @@ -38,10 +40,21 @@ pytest.param(["demo", "stage0"], id="demo-stage0"), ] +# The subset whose output is reproducible enough to compare byte-for-byte across +# two separate invocations. `models --check` is excluded on purpose: it probes the +# host — a `claude` binary on PATH, a key in the environment, a socket to a local +# ollama — so two runs are not guaranteed to agree, and comparing them would buy a +# flake rather than a guarantee. `docs/cookbook/02-models.md` marks that command as +# varying for the same reason; this follows that judgement rather than contradicting it. +COMPARABLE = [param for param in STYLED if param.id != "models-check"] + _ANSI = re.compile(r"\x1b\[[0-9;]*m") # Each run mints its own scratch directory, so the path is not a property of the # output. Normalised rather than excluded, so a *missing* path still fails. -_TMPDIR = re.compile(r"/tmp/grapharc-[A-Za-z0-9_.-]+") +# Built from `gettempdir()` rather than a literal "/tmp", because a runner that +# sets TMPDIR elsewhere would otherwise leave the paths unnormalised and the +# comparison below would fail for a reason that has nothing to do with styling. +_TMPDIR = re.compile(re.escape(tempfile.gettempdir()) + r"/grapharc-[A-Za-z0-9_.-]+") def _env(**extra: str) -> dict[str, str]: @@ -119,14 +132,16 @@ def test_a_terminal_gets_escapes_and_a_pipe_gets_none(args): on_pty, pty_code = _on_pty(args) out, err, piped_code = _piped(args) - assert pty_code == 0 - assert piped_code == 0 + # Not a fixed value — whether the command *succeeds* is the host's business + # (`models --check` exits 1 where no real provider is reachable). What must + # hold is that being watched by a terminal does not change the answer. + assert pty_code == piped_code, "styling changed the exit code" assert _ANSI.search(on_pty), "a terminal received no styling at all" assert "\x1b" not in out, "an escape reached piped stdout" assert "\x1b" not in err, "an escape reached piped stderr" -@pytest.mark.parametrize("args", STYLED) +@pytest.mark.parametrize("args", COMPARABLE) def test_stripping_the_escapes_reproduces_the_piped_output_exactly(args): """The property the byte-compared doc pages depend on.