diff --git a/.gitignore b/.gitignore index c9bbffe..0d27667 100644 --- a/.gitignore +++ b/.gitignore @@ -232,8 +232,9 @@ __marimo__/ # A policy generated because no `--policy`, no `grapharc.toml` and no existing # one was found. Generated rather than authored: the design is that you *promote* # it — read it, edit it, move it, pass `--policy` — instead of committing it from -# where a tool dropped it. `grapharc.toml` itself is deliberately NOT ignored; -# that one is yours and belongs in version control. +# where a tool dropped it. In a project of your own, the scaffolded +# `grapharc.toml` is authored config and belongs in version control; in *this* +# repo the root one is dogfooding residue, ignored with the rest of it below. .grapharc/ # Durable claim stores from `grapharc demo --memory PATH`, and the two sidecars @@ -247,7 +248,26 @@ __marimo__/ /workspace/ /scratch/ +# Dogfooding residue at the repo root: the `grapharc init` scaffold pair, the +# registries that drive the demo films, and the `app/` directory a bench +# shakeout wrote its success condition into. None of it is project source, and +# none of it — the bench residue especially — should ride along on a push. +# Root-anchored, so `bench/fixture/app/` (a committed part of the bench) is +# untouched. +/registry.py +/grapharc.toml +/demo_registry.py +/sample_incident.py +/app/ + # Claude Code's per-machine settings. Already covered by many people's global # ignore; named here so a fresh clone does not depend on that being true. .claude/settings.local.json HANDOFF.md + +# Generated by `grapharc init --claude-code` when this repo dogfoods its own +# supervision. Generated rather than authored, like the policy above: promote +# a copy into a project you are governing instead of committing the one a +# smoke test dropped here. +/.mcp.json +/.claude/skills/ diff --git a/README.md b/README.md index 1dab2d8..faef417 100644 --- a/README.md +++ b/README.md @@ -179,5 +179,6 @@ The edges are documented, not denied — the full list with mechanisms is in the - The HTTP API does not yet use the durable session layer. - On the Claude CLI backend an agent node is *delegated*, not governed. - Policy documents govern planning; the tool plane still reads CLI flags. +- The MCP gate binds the MCP surface, not the host: an agent with its own file tools in the run directory could forge the approval decision. The trust boundary is the working directory, as it is for the Slack workspace. Version `0.1.5` · [changelog](CHANGELOG.md) · [roadmap](ROADMAP.md) · [website](https://codegraphcontext.github.io/GraphARC/) · MIT diff --git a/ROADMAP.md b/ROADMAP.md index 9a91a7e..9cd69b5 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -407,6 +407,17 @@ Everything here works and nothing calls it. and the answer was negative, `2` could not run at all. - [x] **9.6 — Streaming output to clients** via SSE, with a `last-event-id` cursor so a reconnect skips what it already saw. +- [x] **9.7 — MCP supervision server** (`grapharc mcp`, behind the `mcp` + extra that had waited unimported since it was declared). Three stdio + tools — plan / show_graph / execute — and deliberately no approval + verb: a mutating plan parks on the file handshake for an out-of-band + human, a read-only one runs on the host's own prompt, and the verdict + fails closed (an undeclared registry and an old plan.json both read as + mutating). `grapharc init --claude-code` writes the `.mcp.json` and + the skill that adopt it. Distinct from §3.5, which is the MCP *client* + — GraphARC calling third-party tools — and remains open. The trust + boundary is stated in the deep dive: the gate binds the MCP surface, + not a co-located agent's own hands. - [ ] **9.3 — Cron schedules** and **9.4 — webhook triggers.** - [ ] **9.5 — Chat channels** (Slack / Discord). diff --git a/docs/cookbook/09-supervised-agents.md b/docs/cookbook/09-supervised-agents.md new file mode 100644 index 0000000..93972d2 --- /dev/null +++ b/docs/cookbook/09-supervised-agents.md @@ -0,0 +1,65 @@ +# 09 — Supervised agents: GraphARC as an MCP server + +The agents people already use — Claude Code first among them — act alone: one +loop, its own judgment, edits landing as fast as it can type them. This +chapter turns that around without replacing the agent. The agent keeps its +intelligence; GraphARC supplies the gate. Work is proposed as a graph, the +human sees the graph, and execution happens under budgets, onto the trace, +with the mutating case parked until a human answers out of band. + +## The shape of it + +`grapharc mcp` is a stdio MCP server exposing exactly three tools: + +| tool | what it does | what it cannot do | +|---|---|---| +| `plan(goal, scripted?, max_rounds?)` | run the governed planning loop for the goal; return the admitted shape — nodes, edges, rationale, fingerprint — plus a `mutating` verdict | choose a registry, policy or model: those resolve from the operator's `grapharc.toml` in the server's root, never from the call | +| `show_graph(run_dir)` | read a run directory back: the proposal, whether a human is being asked right now, and — after execution — metrics and the Mermaid of what ran | see raw state: like the live view, it serves rendered summaries, never `state_delta` | +| `execute(run_dir, approval_timeout?)` | re-admit the saved plan through the gate and run it | approve itself: a mutating plan parks on the file handshake until a human answers `grapharc approve ` | + +There is no `approve` tool, and there never will be. The gate is between the +agent and the operator; a client that could call `approve()` would be +approving its own proposal, which is not approval. The supervised agent may +*request* (execute parks) and may *check* (`show_graph` says +`awaiting_approval`); the decision belongs to a human with a terminal or the +live view, outside the connection entirely. + +The tiering is deliberate: a plan whose admitted kinds are all read-only +executes on the host agent's own permission prompt — the human already said +yes to the tool call — while any plan containing a mutating kind waits for +the out-of-band answer. The verdict is computed at plan time against the +registry module's own `MUTATING_KINDS`, is stored in `plan.json` beside the +fingerprint, and fails closed twice over: a registry that declared nothing +reads as mutating, and so does a plan file without the field. + +## Adopting it in a Claude Code project + +```bash +cd your-project +grapharc init # once, if there is no registry.py/grapharc.toml yet +grapharc init --claude-code # writes .mcp.json and .claude/skills/grapharc/SKILL.md +``` + +`.mcp.json` registers `grapharc mcp` as a project server, so Claude Code +starts it on demand. The skill is the behavioural half: it routes multi-step +and state-changing work through plan → show → execute, tells the agent to +render the proposal and the `watch_url` to the user, and states the boundary +the server cannot enforce on the host's *other* hands — never run +`grapharc approve`, never touch the request or decision files, and a timeout +means ask, not retry. Neither file is ever overwritten; an existing one is +yours, and the command refuses by name. + +Watch a supervised run the same way as any other: `grapharc serve +--live-root .grapharc/runs`, and the parked proposal is drawn on the live +page with the approve command beside it. + +## The trust boundary, stated plainly + +The MCP gate binds the MCP surface, not the machine. A host agent holds its +own Write and Bash, and a process in the working directory can forge +`approval-decision.json` — the same posture as the Slack gate's workspace: +the trust boundary is the directory, and the skill's never-clauses are the +contract for hands the server cannot see. The park also lives inside one MCP +call: a host that times the tool out kills the wait, the plan stays +unexecuted, and the call is safe to reissue. And approval records the +decision, never the decider — the trace has no actor field. diff --git a/docs/deep-dive.md b/docs/deep-dive.md index ccb36cb..e139548 100644 --- a/docs/deep-dive.md +++ b/docs/deep-dive.md @@ -228,6 +228,12 @@ A stable system is not one that claims to have no edges — it is one whose edge - **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. +**The MCP supervision surface** + +- **The MCP gate binds the MCP surface, not the host.** The server exposes no approval verb, so a supervised agent cannot decide over its own connection — but the host agent holds its own Write and Bash, and a process in the run directory can forge `approval-decision.json`. The trust boundary is the working directory, the same posture the Slack gate documents for its workspace; the shipped skill states the never-clauses for the hands the server cannot see. +- **The host's tool-permission prompt is UX, not enforcement.** Allowlists and skip-permissions modes erase it, and GraphARC cannot observe it. The park on the file handshake is the gate; the prompt is a courtesy in front of it. +- **A parked `execute` lives inside one MCP call.** A host that times the tool out kills the wait; the plan stays unexecuted and the call is safe to reissue. Approval records the decision, never the decider — the trace has no actor field. + **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. diff --git a/grapharc/cli/adopt.py b/grapharc/cli/adopt.py new file mode 100644 index 0000000..cca4e57 --- /dev/null +++ b/grapharc/cli/adopt.py @@ -0,0 +1,146 @@ +"""`grapharc init --claude-code` — adopt GraphARC as a supervision layer. + +Writes the two files that turn a Claude Code checkout into a supervised one: +`.mcp.json`, registering `grapharc mcp` as a project MCP server, and +`.claude/skills/grapharc/SKILL.md`, the contract that routes multi-step and +state-changing work through plan → show → execute instead of a stream of +unsupervised edits. + +Templates are string constants, the `init` convention: nothing here reads +packaged data files, and what a test asserts about the skill is asserted +against the exact bytes a user gets. Neither file is ever overwritten — +an existing one is the operator's, and the refusal names it. + +The skill's load-bearing clause is the last one: the agent must never answer +the approval itself. The MCP surface enforces that on its own connection (no +approve tool exists), but the host agent also holds Write and Bash — the +skill states the boundary for the hands the server cannot see, and the trust +boundary remains the working directory, as it is for the Slack workspace. +""" + +from __future__ import annotations + +from pathlib import Path + +from grapharc.cli import style +from grapharc.cli.output import EXIT_OK, EXIT_UNAVAILABLE, emit + +MCP_CONFIG_FILENAME = ".mcp.json" +SKILL_PATH = Path(".claude/skills/grapharc/SKILL.md") + +MCP_CONFIG_TEMPLATE = """\ +{ + "mcpServers": { + "grapharc": { + "command": "grapharc", + "args": ["mcp"] + } + } +} +""" + +SKILL_TEMPLATE = """\ +--- +name: grapharc +description: >- + Route multi-step or state-changing repository work through GraphARC + supervision: propose the work as a governed graph, show the user the graph, + execute only what the gate admits — and what a human approved, when the plan + can change files. Use for repo-wide fixes, refactors, migrations, or any + task that would otherwise be a long stream of unsupervised edits. +--- + +# GraphARC supervision + +This project routes substantial work through GraphARC's admission gate. The +`grapharc` MCP server exposes three tools — `plan`, `show_graph`, `execute` — +and deliberately nothing that decides an approval. + +## The flow + +1. **Propose, don't act.** For multi-step or state-changing work, call + `plan` with the user's goal instead of editing files directly. The + registry, policy and model come from this project's `grapharc.toml` — if + `plan` reports no registry or model, tell the user to run `grapharc init` + and set `model` in `grapharc.toml`; do not work around it. +2. **Show the user the graph.** Render the returned `proposal` — its nodes, + edges and rationale — in your reply, and give them `watch_url` when it is + set: the live page is where the graph draws itself while it runs. +3. **Execute under the gate.** Call `execute` with the returned `run_dir`. + A plan that can change files **parks**: tell the user it is waiting and + quote `approve_command` — they answer with `grapharc approve ` + (or `--deny`) in a terminal, or from the live view. A read-only plan runs + immediately. +4. **A timeout means ask, not retry.** If `execute` comes back + `approval_timeout`, the plan is unexecuted and the call is safe to + reissue — after the user says so. Never loop on `execute` waiting for a + yes that has not been given. +5. **Report from the record.** After execution, call `show_graph` for the + metrics and the Mermaid rendering of what actually ran, and cite those + rather than your recollection. + +## What you must never do + +- Never run `grapharc approve`, in any form, for any reason. +- Never create, edit or delete `approval-request.json` or + `approval-decision.json` — those files are the human's channel, not yours. +- Never edit `plan.json` to change what was admitted or whether it counts as + mutating; an edited plan is a new proposal, and the gate treats it as one. + +The decision belongs to the user. Your job is to make the question easy to +answer: show the graph, quote the command, and wait. +""" + + +def adopt_claude_code(*, as_json: bool = False) -> int: + """Write `.mcp.json` and the skill, or refuse naming what already exists.""" + mcp_config = Path(MCP_CONFIG_FILENAME) + skill = SKILL_PATH + + existing = [str(p) for p in (mcp_config, skill) if p.exists()] + if existing: + message = ( + f"refusing to overwrite: {', '.join(existing)} — these are yours " + "once written; move one aside if you want it regenerated" + ) + if as_json: + emit({"ok": False, "command": "init", "error": message}, [], as_json=True) + else: + import sys + + print(f"error: {message}", file=sys.stderr) + return EXIT_UNAVAILABLE + + skill.parent.mkdir(parents=True, exist_ok=True) + mcp_config.write_text(MCP_CONFIG_TEMPLATE, encoding="utf-8") + skill.write_text(SKILL_TEMPLATE, encoding="utf-8") + + payload = { + "ok": True, + "command": "init", + "mcp_config": str(mcp_config), + "skill": str(skill), + } + width = style.LABEL_WIDTH + lines = [ + style.kv("wrote", str(mcp_config), width=width, tint=style.accent), + style.kv("wrote", str(skill), width=width, tint=style.accent), + "", + style.kv( + "next", + "open Claude Code here; multi-step work now routes through " + "plan -> show -> execute, and approvals stay yours", + width=width, + ), + ] + emit(payload, lines, as_json=as_json) + return EXIT_OK + + +__all__ = [ + "MCP_CONFIG_FILENAME", + "MCP_CONFIG_TEMPLATE", + "SKILL_PATH", + "SKILL_TEMPLATE", + "adopt_claude_code", +] diff --git a/grapharc/cli/generate.py b/grapharc/cli/generate.py index 68d1cbf..ab312ed 100644 --- a/grapharc/cli/generate.py +++ b/grapharc/cli/generate.py @@ -158,7 +158,7 @@ def resolve_or_generate_policy( workdir: Path | None = None, write: bool = True, catalog: dict[str, str] | None = None, - mutating: tuple[str, ...] = (), + mutating: tuple[str, ...] | None = (), fallback: Any = None, fallback_label: str = "", registry_target: str = "", diff --git a/grapharc/cli/main.py b/grapharc/cli/main.py index 3aed159..7f6a9f6 100644 --- a/grapharc/cli/main.py +++ b/grapharc/cli/main.py @@ -371,6 +371,10 @@ def _cmd_plan(args: argparse.Namespace) -> int: def _cmd_init(args: argparse.Namespace) -> int: + if getattr(args, "claude_code", False): + from grapharc.cli.adopt import adopt_claude_code + + return adopt_claude_code(as_json=args.json) from grapharc.cli.init_cmd import init return init(as_json=args.json) @@ -399,6 +403,8 @@ def _cmd_go(args: argparse.Namespace) -> int: run_id=args.run_id, max_tokens=args.max_tokens, config_path=args.config, + approve=args.approve, + approval_timeout=args.approval_timeout, as_json=args.json, ) candidate = Path(target) @@ -415,6 +421,8 @@ def _cmd_go(args: argparse.Namespace) -> int: run_id=args.run_id, max_tokens=args.max_tokens, config_path=args.config, + approve=args.approve, + approval_timeout=args.approval_timeout, as_json=args.json, ) return plan( @@ -551,6 +559,23 @@ def _cmd_agent(args: argparse.Namespace) -> int: ) +def _cmd_mcp(args: argparse.Namespace) -> int: + from grapharc.cli import optional + + try: + module = optional.load( + "grapharc.mcp", + needed_for="grapharc mcp", + hint="pip install 'grapharc[mcp]'", + ) + except optional.Unavailable as exc: + return fail(str(exc), as_json=args.json, command="mcp") + root = Path(args.root).resolve() if args.root else None + if root is not None and not root.is_dir(): + return fail(f"--root: not a directory: {root}", as_json=args.json, command="mcp") + return int(module.serve_stdio(root)) + + def _cmd_serve(args: argparse.Namespace) -> int: from grapharc.cli.serve import serve @@ -969,6 +994,15 @@ def build_parser() -> argparse.ArgumentParser: parents=[common], help="scaffold a registry, a config and a runs directory in this directory", ) + ini.add_argument( + "--claude-code", + action="store_true", + help=( + "instead of the scaffold, write .mcp.json and the Claude Code " + "skill that route this project's multi-step work through " + "grapharc mcp supervision" + ), + ) ini.set_defaults(handler=_cmd_init) st = sub.add_parser( @@ -1061,6 +1095,26 @@ def build_parser() -> argparse.ArgumentParser: agent.add_argument("--system-prompt", default=None) agent.set_defaults(handler=_cmd_agent) + mcp = sub.add_parser( + "mcp", + parents=[common], + help=( + "run the MCP supervision server on stdio (plan / show_graph / " + "execute; approval stays out of band)" + ), + ) + mcp.add_argument( + "--root", + default=None, + metavar="PATH", + help=( + "directory whose grapharc.toml and registry govern every plan, and " + "which confines every run_dir a client names (default: the working " + "directory)" + ), + ) + mcp.set_defaults(handler=_cmd_mcp) + serve = sub.add_parser("serve", parents=[common], help="run the HTTP API") serve.add_argument("--host", default="127.0.0.1") serve.add_argument("--port", type=int, default=8000) diff --git a/grapharc/cli/plan.py b/grapharc/cli/plan.py index a70ead4..dc86169 100644 --- a/grapharc/cli/plan.py +++ b/grapharc/cli/plan.py @@ -178,12 +178,17 @@ class PlanSetupError(Exception): """Raised before anything runs, so a bad flag never half-executes a plan.""" -def _write_plan_file(run_dir: Path, *, goal, registry_target, model_spec, result) -> None: +def _write_plan_file( + run_dir: Path, *, goal, registry_target, model_spec, result, mutating: bool = True +) -> None: """Persist the admitted-but-unexecuted plan next to its trace. What `grapharc go` reads. The proposal is stored whole and re-judged by admission at execution time — a hand-edited plan.json is a new proposal, - not a pre-approved one. + not a pre-approved one. `mutating` records the verdict the payload + carries, so a later driver deciding whether execution needs an approval + park does not have to re-import the registry; a plan file without the + field reads as mutating, never as safe. """ import json from datetime import UTC, datetime @@ -203,6 +208,7 @@ def _write_plan_file(run_dir: Path, *, goal, registry_target, model_spec, result "model": model_spec, "fingerprint": admitted.proposal.fingerprint(), "proposal": admitted.proposal.model_dump(mode="json"), + "mutating": mutating, "planned_at": datetime.now(UTC).isoformat(), }, indent=2, @@ -228,6 +234,52 @@ def find_unexecuted_plan(runs_root: Path | None = None) -> Path | None: return None +def _approval_gate( + trace_path: Path, + *, + run_id: str | None, + approval_timeout: float | None, + as_json: bool, +) -> Any: + """The file-handshake gate `--approve` configures, announce included. + + Shared by `plan` and `go` so the two commands cannot drift on how a + parked run asks its question. The announce is printed and flushed before + the run parks — a terminal user (or a log tailer) must learn how to + answer without waiting for the exit — and silent in JSON mode, where + stdout carries exactly one document; a JSON caller detects the park by + watching for `approval-request.json` in the run directory. + """ + import sys + + from grapharc.planner.approval_file import DEFAULT_TIMEOUT_SECONDS, file_approval + + watch_shown = False + + def _announce(message: str) -> None: + if as_json: + return + # The live link first, once: the page is where the parked proposal + # is drawn, and it should be open while the human decides. + nonlocal watch_shown + if not watch_shown: + watch_shown = True + url = watch_url(trace_path, run_id=run_id) + if url: + print( + style.kv("watch", url, width=style.LABEL_WIDTH, tint=style.accent), + flush=True, + file=sys.stdout, + ) + print(message, flush=True, file=sys.stdout) + + return file_approval( + trace_path.parent, + timeout_seconds=approval_timeout or DEFAULT_TIMEOUT_SECONDS, + announce=_announce, + ) + + def execute_plan( target: str | None, *, @@ -239,6 +291,8 @@ def execute_plan( run_id: str | None = None, max_tokens: int | None = None, config_path: Path | None = None, + approve: bool = False, + approval_timeout: float | None = None, as_json: bool = False, ) -> int: """`grapharc go []` — execute a plan `grapharc plan` saved. @@ -246,8 +300,11 @@ def execute_plan( The stored proposal is replayed through the full governed loop — a scripted planner whose one reply *is* the plan — so admission judges it again on the way in: what runs is what the gate admits now, not what a - file claims was admitted before. Executing is the human approval; there - is no second gate. + file claims was admitted before. Bare, executing is the human approval; + with `--approve` the run parks on the file handshake first and executes + only an answered yes — the gate an external driver relies on when the + plan can change things. These flags used to be accepted here and + silently dropped, which was worse than refusing them. """ import json @@ -292,6 +349,7 @@ def execute_plan( try: settings = load_settings(config_path) model_spec = settings.resolve("model", model_spec, record.get("model")) + policy_path = settings.resolve_path("policy", policy_path) tenant = settings.resolve("tenant", tenant, "default") max_tokens = settings.resolve("max_tokens", max_tokens, 100_000) model_args = _parse_model_args(model_arg_pairs) @@ -344,7 +402,16 @@ def execute_plan( registry=bundle.registry, state_schema=schema, writes=bundle.writes, - approval=None, + approval=( + _approval_gate( + trace_path, + run_id=run_id, + approval_timeout=approval_timeout, + as_json=as_json, + ) + if approve + else None + ), ) initial = schema(goal=goal) if "goal" in schema.model_fields else schema() result = loop.run(goal, initial, run_id=run_id) @@ -471,9 +538,11 @@ class RegistryBundle: #: that do not exist and permit ones that do. default_policy: Any = None #: Kinds the module considers dangerous, from its `MUTATING_KINDS`. Handed to - #: the policy generator so it knows what to deny; empty means it denies - #: nothing, which is why a module that can change things should say so. - mutating: tuple[str, ...] = () + #: the policy generator so it knows what to deny. `None` means the module + #: said nothing at all — which readers must treat as "assume mutating", + #: never as "declared safe" — while an explicit empty tuple is a + #: declaration that nothing here can change anything. + mutating: tuple[str, ...] | None = None #: The module's own `build_loop`, when it ships one. This is how a registry #: owns its goal check and observer instead of inheriting the incident #: demo's (`len(notes) >= 3`) — a registry whose state never accumulates @@ -526,12 +595,13 @@ def resolve_registry( registry(model, **kwargs) if _accepts_an_argument(registry) else registry(**kwargs) ) default_policy = getattr(module, "default_edge_policy", None) + declared_mutating = getattr(module, "MUTATING_KINDS", None) return RegistryBundle( registry=registry, state_schema=getattr(module, "STATE_SCHEMA", None), writes=getattr(module, "WRITES", None), default_policy=default_policy() if callable(default_policy) else default_policy, - mutating=tuple(getattr(module, "MUTATING_KINDS", ())), + mutating=None if declared_mutating is None else tuple(declared_mutating), build_loop=getattr(module, "build_loop", None), ) @@ -810,40 +880,13 @@ def plan( schema = state_schema or IncidentState trace = TraceRecorder(trace_path) - approval = None - if approve: - import sys - - from grapharc.planner.approval_file import DEFAULT_TIMEOUT_SECONDS, file_approval - - watch_shown = False - - def _announce(message: str) -> None: - # Printed *and flushed* before the run parks: a terminal user (or a - # log tailer) must learn how to answer without waiting for the exit. - # Silent in JSON mode: stdout there carries exactly one document, and - # a notice printed ahead of it makes the whole output unparseable. - if as_json: - return - # The live link first, once: the page is where the parked proposal - # is drawn, and it should be open while the human decides. - nonlocal watch_shown - if not watch_shown: - watch_shown = True - url = watch_url(trace_path, run_id=run_id) - if url: - print( - style.kv("watch", url, width=style.LABEL_WIDTH, tint=style.accent), - flush=True, - file=sys.stdout, - ) - print(message, flush=True, file=sys.stdout) - - approval = file_approval( - trace_path.parent, - timeout_seconds=approval_timeout or DEFAULT_TIMEOUT_SECONDS, - announce=_announce, + approval = ( + _approval_gate( + trace_path, run_id=run_id, approval_timeout=approval_timeout, as_json=as_json ) + if approve + else None + ) # The registry module's own loop builder wins; the incident demo's is the # fallback that keeps the default path byte-identical. build_loop = bundle.build_loop or incident_build_loop @@ -870,6 +913,33 @@ def _announce(message: str) -> None: loop.plan_only = command == "plan" and not go_after result = loop.run(goal, initial, run_id=run_id) + # The admitted shape as data, computed before the plan file so the file + # can carry the same `mutating` verdict the payload does. An external + # driver — the MCP server first among them — must not have to re-read + # plan.json for the shape, nor re-import the registry for the verdict. + admitted_record = next( + ( + record + for record in reversed(result.rounds) + if record.proposal is not None + and record.admission is not None + and record.admission.admitted + ), + None, + ) + if bundle.mutating is None: + # The registry module never said which kinds mutate. A reader must + # treat that as "assume mutating", so the payload says so rather than + # implying a safety nobody declared. + is_mutating = True + else: + admitted_kinds = ( + {node.kind for node in admitted_record.proposal.nodes} + if admitted_record + else set() + ) + is_mutating = bool(admitted_kinds & set(bundle.mutating)) + if result.stop is LoopStop.PLANNED: _write_plan_file( trace_path.parent, @@ -877,6 +947,7 @@ def _announce(message: str) -> None: registry_target=registry_target, model_spec=model_spec, result=result, + mutating=is_mutating, ) rounds = [ @@ -899,13 +970,25 @@ def _announce(message: str) -> None: "policy": policy_description, "policy_source": policy_source, "trace": str(trace_path), + "run_dir": str(trace_path.parent), **settings.provenance(policy_source=policy_source), "stop": result.stop.value, "detail": result.detail, "rounds": rounds, "rejections": [r.code for r in result.rejections()], + "mutating": is_mutating, "state": result.state.model_dump() if hasattr(result.state, "model_dump") else result.state, } + if admitted_record is not None: + payload["fingerprint"] = admitted_record.admission.fingerprint + payload["proposal"] = { + "nodes": [ + node.model_dump(mode="json", exclude={"subgraph"}) + for node in admitted_record.proposal.nodes + ], + "edges": [edge.model_dump(mode="json") for edge in admitted_record.proposal.edges], + "rationale": admitted_record.proposal.rationale, + } # 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 diff --git a/grapharc/mcp/__init__.py b/grapharc/mcp/__init__.py new file mode 100644 index 0000000..036995f --- /dev/null +++ b/grapharc/mcp/__init__.py @@ -0,0 +1,15 @@ +"""GraphARC as an MCP server — supervision for agents that already exist. + +Ships behind the `mcp` extra (`pip install 'grapharc[mcp]'`). The package +imports nothing from `grapharc.server`, which needs the `server` extra: the +read-side primitives here are the fastapi-free ones (`observe`, the approval +file handshake, `plan.json`). + +`grapharc mcp` is the entry point; `build_server` is the library surface a +test drives directly. +""" + +from grapharc.mcp.driver import DriverError +from grapharc.mcp.server import FORBIDDEN_TOOL_WORDS, build_server, serve_stdio + +__all__ = ["FORBIDDEN_TOOL_WORDS", "DriverError", "build_server", "serve_stdio"] diff --git a/grapharc/mcp/driver.py b/grapharc/mcp/driver.py new file mode 100644 index 0000000..6cab45c --- /dev/null +++ b/grapharc/mcp/driver.py @@ -0,0 +1,200 @@ +"""The MCP server's hands: build argv, spawn the CLI, read the record back. + +Everything here drives `grapharc` **as a subprocess**, never in process, for +three reasons the server module repeats: the stdio transport owns stdout and +`emit()` prints there; the `--json` payloads and exit codes are the tested +interface, so this stays a thin shim over a contract that already has a +suite; and an async subprocess keeps the event loop free, so `show_graph` +answers while an `execute` is parked. + +Argv is **built, never parsed**: no tool accepts a registry, policy or model +argument, because those resolve from the operator's `grapharc.toml` in the +root directory — the requester's call must not be able to widen what the +operator configured. Run directories are confined to the root the server was +started in, the same `is_relative_to` posture as the Slack gate's paths. + +Only fastapi-free modules are imported here (`observe.trace`, +`observe.metrics`, `planner.approval_file`): this package ships behind the +`mcp` extra and must not drag the `server` extra in with it. +""" + +from __future__ import annotations + +import asyncio +import json +import sys +from pathlib import Path +from typing import Any + +PLAN_FILENAME = "plan.json" + +#: A parked `execute` lives inside one MCP call, and hosts time tool calls +#: out. The default stays under typical host limits; a timeout leaves the +#: plan unexecuted and the call safe to reissue. +DEFAULT_APPROVAL_TIMEOUT = 240.0 + + +class DriverError(Exception): + """A tool call that cannot proceed, with the reason as the message.""" + + +def confine_run_dir(root: Path, run_dir: str) -> Path: + """Resolve `run_dir` and refuse anything outside the server's root. + + The server reads plan.json and trace.jsonl from whatever directory a + client names; without this, a client could point it at any readable + path on the machine. + """ + resolved = (root / run_dir).resolve() if not Path(run_dir).is_absolute() else Path( + run_dir + ).resolve() + if not resolved.is_relative_to(root.resolve()): + raise DriverError( + f"run_dir {run_dir!r} is outside the directory this server was " + f"started in ({root}); a run directory is always under it" + ) + return resolved + + +async def run_cli( + argv: list[str], *, cwd: Path, timeout: float | None = None +) -> tuple[int, str, str]: + """One `grapharc` subprocess, the Slack runner's spawn pattern made async.""" + process = await asyncio.create_subprocess_exec( + sys.executable, + "-m", + "grapharc.cli.main", + *argv, + cwd=cwd, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + try: + out, err = await asyncio.wait_for(process.communicate(), timeout=timeout) + except TimeoutError: + process.kill() + await process.wait() + raise DriverError( + f"grapharc {argv[0]} did not finish within {timeout}s and was stopped" + ) from None + return process.returncode or 0, out.decode(errors="replace"), err.decode(errors="replace") + + +def parse_document(stdout: str, *, command: str) -> dict[str, Any]: + """The one JSON document a `--json` command prints, or a named refusal.""" + try: + document = json.loads(stdout) + except (json.JSONDecodeError, ValueError) as exc: + raise DriverError( + f"grapharc {command} --json did not print a readable document: " + f"{stdout.strip()[-300:] or '(empty)'}" + ) from exc + if not isinstance(document, dict): + raise DriverError(f"grapharc {command} --json printed {type(document).__name__}") + return document + + +def read_plan_record(run_dir: Path) -> dict[str, Any]: + plan_file = run_dir / PLAN_FILENAME + if not plan_file.is_file(): + raise DriverError( + f"no {PLAN_FILENAME} in {run_dir} — call plan first; execute and " + "show_graph work on a directory plan created" + ) + try: + return json.loads(plan_file.read_text(encoding="utf-8")) + except (OSError, ValueError) as exc: + raise DriverError(f"unreadable {plan_file}: {exc}") from exc + + +def plan_is_mutating(record: dict[str, Any]) -> bool: + """The plan's own verdict — absent reads as mutating, never as safe.""" + value = record.get("mutating") + return True if not isinstance(value, bool) else value + + +def graph_status(run_dir: Path) -> dict[str, Any]: + """Compose the read-only view of one run directory. + + plan.json for the admitted shape, the trace for what has happened since, + the approval-request file for whether a human is being asked right now. + Rendered summaries only — the raw `state_delta` a node wrote is served by + nothing here, the same posture as the live view. + """ + from grapharc.observe.metrics import summarize, to_mermaid + from grapharc.observe.trace import TraceRecorder + from grapharc.planner.approval_file import read_request + + record = read_plan_record(run_dir) + view: dict[str, Any] = { + "run_dir": str(run_dir), + "goal": record.get("goal", ""), + "fingerprint": record.get("fingerprint", ""), + "mutating": plan_is_mutating(record), + "proposal": { + "nodes": (record.get("proposal") or {}).get("nodes", []), + "edges": (record.get("proposal") or {}).get("edges", []), + "rationale": (record.get("proposal") or {}).get("rationale", ""), + }, + "executed_run_id": record.get("executed_run_id"), + } + + request = read_request(run_dir) + view["awaiting_approval"] = request is not None + view["approve_command"] = ( + f"grapharc approve {run_dir}" if request is not None else None + ) + + trace_path = run_dir / "trace.jsonl" + view["status"] = "planned" + if request is not None: + view["status"] = "awaiting_approval" + if record.get("executed_run_id") and trace_path.is_file(): + recorder = TraceRecorder(trace_path) + run_id = str(record["executed_run_id"]) + metrics = summarize(recorder, run_id) + if metrics is not None: + view["status"] = "done" + view["metrics"] = metrics.model_dump(mode="json") + view["mermaid"] = to_mermaid(recorder, run_id) + return view + + +def build_plan_argv(goal: str, *, scripted: bool, max_rounds: int | None) -> list[str]: + argv = ["plan", goal, "--json"] + if scripted: + argv.append("--scripted") + if max_rounds is not None: + argv += ["--max-rounds", str(int(max_rounds))] + return argv + + +def build_execute_argv( + run_dir: Path, *, mutating: bool, approval_timeout: float +) -> list[str]: + """`go `, parked on the file handshake exactly when the plan mutates. + + The tiering is the maintainer's decision made mechanical: an all-read-only + plan executes on the host's own prompt; anything that can change files + parks for an out-of-band human. The verdict comes from the plan record, + where a missing field already read as mutating. + """ + argv = ["go", str(run_dir), "--json"] + if mutating: + argv += ["--approve", "--approval-timeout", str(float(approval_timeout))] + return argv + + +__all__ = [ + "DEFAULT_APPROVAL_TIMEOUT", + "PLAN_FILENAME", + "DriverError", + "build_execute_argv", + "build_plan_argv", + "confine_run_dir", + "graph_status", + "parse_document", + "plan_is_mutating", + "read_plan_record", + "run_cli", +] diff --git a/grapharc/mcp/server.py b/grapharc/mcp/server.py new file mode 100644 index 0000000..acb9bab --- /dev/null +++ b/grapharc/mcp/server.py @@ -0,0 +1,152 @@ +"""`grapharc mcp` — the supervision surface an external agent plugs into. + +Three tools, and deliberately not a fourth: + +- `plan(goal, ...)` proposes a graph for the goal through the governed loop + and returns the admitted shape as data — nodes, edges, rationale, + fingerprint, and whether executing it can change anything. +- `show_graph(run_dir)` is the read-only view: the proposal, whether a human + is being asked right now, and — after execution — the metrics and the + Mermaid rendering of what actually ran. +- `execute(run_dir, ...)` re-admits and runs the saved plan. A plan + containing a mutating kind parks on the file handshake until a human + answers `grapharc approve ` out of band; an all-read-only plan + runs on the host's own prompt. + +**There is no approve, deny, or decide tool, and there never will be.** The +gate is between the agent and the operator; a client that could call +`approve()` would be approving its own proposal, which is not approval. The +supervised agent may request (execute parks) and may check (show_graph says +`awaiting_approval`); the decision belongs to a human with a terminal or the +live view, outside this connection. + +The server drives the `grapharc` CLI as subprocesses and never prints to +stdout — the stdio transport owns it. Diagnostics go to stderr. +""" + +from __future__ import annotations + +import sys +from pathlib import Path +from typing import Any + +from mcp.server.fastmcp import FastMCP + +from grapharc.mcp import driver +from grapharc.mcp.driver import DEFAULT_APPROVAL_TIMEOUT, DriverError + +#: Verbs this surface refuses to grow. Checked by a gate test against the +#: registered tool names, so the refusal is a property, not a comment. +FORBIDDEN_TOOL_WORDS = ("approve", "deny", "decide") + + +def build_server(root: Path | None = None) -> FastMCP: + """The FastMCP server, rooted where it was started. + + `root` confines every `run_dir` a client names and is the working + directory of every spawned CLI — which is what makes the operator's + `grapharc.toml` there, and nothing the client says, decide the registry, + the policy and the model. + """ + base = (root or Path.cwd()).resolve() + server = FastMCP( + "grapharc", + instructions=( + "GraphARC supervision: plan proposes a governed graph for a goal " + "and returns the admitted shape; show_graph reads a run " + "directory back; execute runs a saved plan, parking for " + "out-of-band human approval when the plan can change files. " + "There is no approve tool: the decision belongs to the human, " + "via `grapharc approve ` or the live view." + ), + ) + + @server.tool() + async def plan( + goal: str, scripted: bool = False, max_rounds: int | None = None + ) -> dict[str, Any]: + """Propose a governed graph for `goal` and return the admitted shape. + + The registry, policy and model come from the operator's grapharc.toml + in the server's directory — they are not parameters, on purpose. + `scripted=True` rehearses with the registry's canned planner: free, + deterministic, no model call. + """ + code, out, err = await driver.run_cli( + driver.build_plan_argv(goal, scripted=scripted, max_rounds=max_rounds), + cwd=base, + ) + document = driver.parse_document(out, command="plan") + if code != 0 and not document.get("ok", False): + # The CLI's failure document is the answer — setup problems arrive + # with the CLI's own instructive text rather than a bare error. + return document + return { + "run_dir": document.get("run_dir"), + "goal": document.get("goal"), + "stop": document.get("stop"), + "proposal": document.get("proposal"), + "fingerprint": document.get("fingerprint"), + "mutating": document.get("mutating", True), + "kinds": document.get("kinds", []), + "policy": document.get("policy"), + "rejections": document.get("rejections", []), + "rounds": document.get("rounds", []), + "watch_url": document.get("watch_url"), + "plan_file": document.get("plan_file"), + "approve_command": ( + f"grapharc approve {document.get('run_dir')}" + if document.get("mutating", True) and document.get("run_dir") + else None + ), + } + + @server.tool() + async def show_graph(run_dir: str) -> dict[str, Any]: + """Read one run directory back: the admitted proposal, whether a human + is being asked right now, and the record of what ran. Read-only.""" + resolved = driver.confine_run_dir(base, run_dir) + return driver.graph_status(resolved) + + @server.tool() + async def execute( + run_dir: str, approval_timeout: float = DEFAULT_APPROVAL_TIMEOUT + ) -> dict[str, Any]: + """Execute the plan saved in `run_dir`, re-admitted through the gate. + + A plan with a mutating kind parks until a human answers + `grapharc approve ` out of band; tell the user, do not + answer it yourself. A timeout leaves the plan unexecuted and this + call safe to reissue. + """ + resolved = driver.confine_run_dir(base, run_dir) + record = driver.read_plan_record(resolved) + mutating = driver.plan_is_mutating(record) + code, out, err = await driver.run_cli( + driver.build_execute_argv( + resolved, mutating=mutating, approval_timeout=approval_timeout + ), + cwd=base, + # The subprocess bounds its own park via --approval-timeout; this + # outer bound only catches a wedged process, generously. + timeout=approval_timeout + 120.0 if mutating else None, + ) + document = driver.parse_document(out, command="go") + document["mutating"] = mutating + if mutating and not document.get("executed", False): + document["approve_command"] = f"grapharc approve {resolved}" + return document + + return server + + +def serve_stdio(root: Path | None = None) -> int: + """Run the server on stdio until the client hangs up. The CLI entry.""" + try: + build_server(root).run(transport="stdio") + except KeyboardInterrupt: # a Ctrl-C is a clean goodbye, not a stack trace + print("grapharc mcp: stopped", file=sys.stderr) + return 0 + + +__all__ = ["FORBIDDEN_TOOL_WORDS", "DriverError", "build_server", "serve_stdio"] diff --git a/grapharc/observe/cost.py b/grapharc/observe/cost.py index 214f8c9..96c98d7 100644 --- a/grapharc/observe/cost.py +++ b/grapharc/observe/cost.py @@ -15,20 +15,20 @@ never added into `recorded_cost_usd`, so nobody can mistake an estimate for an invoice. -Tokens are counted from the `end` events of node executions, which is exactly -the rule `metrics.summarize` uses. That is not a coincidence and not a +Tokens are counted from the terminal events of node executions — `end` and +`error` alike, since the kernel stamps both with the node's spend — which is +exactly the rule `metrics.summarize` uses. That is not a coincidence and not a duplicate implementation: `RunCost.tokens` is asserted equal to `RunMetrics.tokens` by the test suite, because a cost report and an audit trail that disagree are worse than either alone. Two limits, stated rather than smoothed over: -- A node that raises never emits an `end` event, and the kernel's `error` event - carries no token count — so tokens spent inside a node that then failed are - invisible at node level. Where the node was an `AgentNode`, its per-call - `"model"` sub-events still hold them, and they are reported as - `tokens_before_error` rather than folded into the total that must match - `metrics`. +- A trace whose `error` events carry no token count (an older or hand-built + producer) leaves tokens spent inside a failed node invisible at node level. + Where the node was an `AgentNode`, its per-call `"model"` sub-events still + hold them, and they are reported as `tokens_before_error` rather than folded + into the total that must match `metrics`. - There is no tenant on a trace event, so tenant attribution is not offered here. Run, thread (session) and node are what the format supports today. """ @@ -142,10 +142,10 @@ class RunCost(BaseModel): unpriced_tokens: int = 0 per_node: list[NodeCost] = Field(default_factory=list) model_calls: list[ModelCallCost] = Field(default_factory=list) - # Tokens reported by sub-steps of nodes that ended in `error`. Held apart - # from `tokens` because the node-level total must keep matching - # `metrics.summarize`, and because "spend that bought nothing" is the - # number an incident review actually wants. + # Tokens reported by sub-steps of failed nodes whose `error` event carried + # no token count of its own. Held apart from `tokens` because the + # node-level total must keep matching `metrics.summarize`, and because + # "spend that bought nothing" is the number an incident review wants. tokens_before_error: int = 0 @property @@ -291,11 +291,34 @@ def _price_run(run: ReplayedRun, rates: RateCard | None) -> RunCost: ) if not execution.ok: - # No `end` event was written, so the node-level token count for this - # execution does not exist; whatever sub-steps it emitted before - # failing are the only record of what it spent. + # The kernel stamps a failed execution's terminal `error` event + # with what the node spent, exactly as it stamps `end`, and + # `metrics.summarize` counts it. Skipping it here made the cost + # report disagree with the audit trail by precisely the spend a + # budget stop was about. entry.errors += 1 - tokens_before_error += execution.sub_tokens + entry.tokens += execution.tokens + entry.duration_ms += execution.duration_ms or 0.0 + if execution.tokens: + estimate, unpriced_here, models = _estimate(execution, card) + entry.models = list(dict.fromkeys([*entry.models, *models])) + if execution.cost_usd is not None: + entry.recorded_cost_usd = _combine( + entry.recorded_cost_usd, execution.cost_usd + ) + recorded_total.append(execution.cost_usd) + else: + if estimate is not None: + entry.estimated_cost_usd = _combine( + entry.estimated_cost_usd, estimate + ) + estimated_total.append(estimate) + entry.unpriced_tokens += unpriced_here + unpriced += unpriced_here + # A trace whose `error` event carries no token count (an older or + # hand-built producer) still holds the sub-steps' record; that + # spend is not in the total and is reported apart. + tokens_before_error += max(0, execution.sub_tokens - execution.tokens) continue entry.executions += 1 diff --git a/grapharc/observe/viewmodel.py b/grapharc/observe/viewmodel.py index 83bf951..bba6aa5 100644 --- a/grapharc/observe/viewmodel.py +++ b/grapharc/observe/viewmodel.py @@ -194,6 +194,7 @@ def _node_measures(node: NodeView, graph_events: list[TraceEvent], name: str) -> """ costs: list[float] = [] prefix = f"{name}:" + live = 0 for event in graph_events: if event.node == name and event.phase in ("end", "error"): node.tokens += event.tokens or 0 @@ -202,11 +203,16 @@ def _node_measures(node: NodeView, graph_events: list[TraceEvent], name: str) -> if event.cost_usd is not None: costs.append(event.cost_usd) node.executions += 1 + # Sub-steps before this terminal are a breakdown of the total just + # counted; carrying them forward would show a closed execution's + # spend twice on a node that runs again. + live = 0 elif node.status == "running" and ( event.node == name or event.node.startswith(prefix) ): if event.phase not in ("start",) and event.tokens: - node.live_tokens += event.tokens + live += event.tokens + node.live_tokens += live if costs: node.cost_usd = round(sum(costs), 6) if node.duration_ms is not None: diff --git a/grapharc/planner/loop.py b/grapharc/planner/loop.py index c7a8516..247b309 100644 --- a/grapharc/planner/loop.py +++ b/grapharc/planner/loop.py @@ -718,21 +718,11 @@ def _execute( }, ) - if self.plan_only: - # The graph is admitted, materialisable, and on the trace — which - # is exactly what "planned" means. Executing it is `grapharc go`'s - # job, in its own process, whenever the operator says. The - # approval gate is skipped on purpose: a plan that executes - # nothing has nothing to approve; the act of running `go` *is* - # the approval. - return _Execution( - state=state, - executed=False, - hard_stop=LoopStop.PLANNED, - execution_error="awaiting `grapharc go`", - ) - if self.approval is not None: + # Before the plan_only return, deliberately: `plan --approve` used + # to return PLANNED first, which made the flag inert — a gate that + # is configured has been asked for, and a parked plan is a real + # question whether or not this process will also execute it. parked = time.monotonic() decision = self._request_approval( proposal, verdict, ctx, round_number, goal=goal @@ -763,6 +753,19 @@ def _execute( hard_stop=stop, ) + if self.plan_only: + # The graph is admitted, materialisable, on the trace — and, when + # a gate was configured, approved above before being called a + # plan. Executing it is `grapharc go`'s job, in its own process, + # whenever the operator says; with no gate configured, the act of + # running `go` is the approval. + return _Execution( + state=state, + executed=False, + hard_stop=LoopStop.PLANNED, + execution_error="awaiting `grapharc go`", + ) + budget = self._round_budget(meter) try: raw = compiled.invoke( diff --git a/pyproject.toml b/pyproject.toml index 3aebe11..5bdfafc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -116,6 +116,9 @@ all = [ [dependency-groups] dev = [ "httpx>=0.27", + # The MCP server ships behind the `mcp` extra; it is a dev dependency too + # so its gate tests always run in CI rather than silently skipping. + "mcp>=1.2", "pytest>=8.2", "pytest-asyncio>=0.24", "pytest-timeout>=2.3", diff --git a/tests/test_adopt.py b/tests/test_adopt.py new file mode 100644 index 0000000..da6deb6 --- /dev/null +++ b/tests/test_adopt.py @@ -0,0 +1,69 @@ +"""`grapharc init --claude-code` — the adoption files, and their contract. + +Two properties: the command never overwrites what an operator already owns, +and the skill it writes carries the never-self-approve clause in so many +words — the MCP surface refuses the approval verb on its own connection, and +the skill states the same boundary for the host agent's other hands. +""" + +from __future__ import annotations + +import json + +import pytest + +from grapharc.cli.adopt import MCP_CONFIG_FILENAME, SKILL_PATH, SKILL_TEMPLATE +from grapharc.cli.main import main + + +@pytest.fixture +def in_tmp(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + return tmp_path + + +def test_adopt_writes_the_server_config_and_the_skill(in_tmp, capsys): + assert main(["init", "--claude-code", "--json"]) == 0 + + payload = json.loads(capsys.readouterr().out) + assert payload["ok"] is True + config = json.loads((in_tmp / MCP_CONFIG_FILENAME).read_text()) + assert config["mcpServers"]["grapharc"]["command"] == "grapharc" + assert config["mcpServers"]["grapharc"]["args"] == ["mcp"] + assert (in_tmp / SKILL_PATH).is_file() + + +def test_adopt_refuses_to_overwrite_and_names_the_files(in_tmp, capsys): + (in_tmp / MCP_CONFIG_FILENAME).write_text("{}") + + assert main(["init", "--claude-code", "--json"]) == 2 + payload = json.loads(capsys.readouterr().out) + assert payload["ok"] is False + assert MCP_CONFIG_FILENAME in payload["error"] + # The refusal changed nothing: the operator's file is intact and the + # skill was not half-written beside it. + assert (in_tmp / MCP_CONFIG_FILENAME).read_text() == "{}" + assert not (in_tmp / SKILL_PATH).exists() + + +def test_the_skill_carries_the_never_self_approve_clause(in_tmp): + """The one sentence the packaging must not lose, asserted against the + exact bytes a user receives.""" + assert main(["init", "--claude-code"]) == 0 + text = (in_tmp / SKILL_PATH).read_text() + + assert text == SKILL_TEMPLATE + assert "Never run `grapharc approve`" in text + assert "approval-decision.json" in text + assert "A timeout means ask, not retry" in text + assert "The decision belongs to the user" in text + + +def test_plain_init_is_untouched_by_the_new_flag(in_tmp): + """`init` without the flag still scaffolds the registry pair, and the + adoption files are not part of that scaffold.""" + assert main(["init"]) == 0 + assert (in_tmp / "registry.py").is_file() + assert (in_tmp / "grapharc.toml").is_file() + assert not (in_tmp / MCP_CONFIG_FILENAME).exists() + assert not (in_tmp / SKILL_PATH).exists() diff --git a/tests/test_approval.py b/tests/test_approval.py index 59df539..a019ec0 100644 --- a/tests/test_approval.py +++ b/tests/test_approval.py @@ -340,3 +340,143 @@ def test_plan_approve_in_text_mode_still_announces_how_to_answer(tmp_path, capsy ]) assert "grapharc approve" in capsys.readouterr().out + + +# -- the gate reaches `plan` without `--go`, and `go` itself (PR B1) ------------ + + +def _answer(trace_dir: Path, argv_extra: list[str] | None = None) -> threading.Thread: + """Answer the file handshake from a background thread, via the real CLI. + + `--json` so the answer shares captured stdout with the parked command's + own document as two clean JSON values rather than interleaved prose. + """ + + def worker(): + deadline = time.monotonic() + 8 + while time.monotonic() < deadline: + if (trace_dir / REQUEST_FILENAME).exists(): + assert main(["approve", str(trace_dir), "--json", *(argv_extra or [])]) == 0 + return + time.sleep(0.02) + + thread = threading.Thread(target=worker) + thread.start() + return thread + + +def _last_document(text: str) -> dict: + """The final JSON document in a stream that may carry several.""" + decoder = json.JSONDecoder() + documents, index = [], 0 + while index < len(text): + if text[index] != "{": + index += 1 + continue + try: + document, index = decoder.raw_decode(text, index) + except json.JSONDecodeError: + index += 1 + continue + documents.append(document) + assert documents, f"no JSON document in: {text[:200]!r}" + return documents[-1] + + +def test_plan_approve_without_go_parks_and_a_denial_leaves_no_plan_file(tmp_path, capsys): + """`plan --approve` used to return PLANNED before the gate could fire — an + inert flag. Now the parked plan is a real question, and a denial is + fail-closed: nothing is saved for a later `go` to pick up.""" + trace = tmp_path / "run" / "trace.jsonl" + thread = _answer(trace.parent, ["--deny"]) + code = main( + ["plan", "investigate", "--scripted", "--approve", "--approval-timeout", "8", + "--trace", str(trace), "--json"] + ) + thread.join() + + assert code == 1 + payload = _last_document(capsys.readouterr().out) + assert payload["stop"] == "approval_denied" + assert not (trace.parent / "plan.json").exists() + + +def test_plan_approve_approved_still_stops_planned_and_saves_the_plan(tmp_path, capsys): + trace = tmp_path / "run" / "trace.jsonl" + thread = _answer(trace.parent) + code = main( + ["plan", "investigate", "--scripted", "--approve", "--approval-timeout", "8", + "--trace", str(trace), "--json"] + ) + thread.join() + + assert code == 0 + payload = _last_document(capsys.readouterr().out) + assert payload["stop"] == "planned" + assert (trace.parent / "plan.json").exists() + + +def _saved_plan(tmp_path, capsys) -> Path: + trace = tmp_path / "run" / "trace.jsonl" + assert main(["plan", "investigate", "--scripted", "--trace", str(trace), "--json"]) == 0 + capsys.readouterr() # drop the plan document; the tests below have their own + return trace.parent + + +def test_go_approve_denied_executes_nothing(tmp_path, capsys): + """`go --approve` was accepted and silently ignored — the flag now + parks the replayed plan, and a denial leaves it unexecuted.""" + run_dir = _saved_plan(tmp_path, capsys) + thread = _answer(run_dir, ["--deny"]) + code = main(["go", str(run_dir), "--approve", "--approval-timeout", "8", "--json"]) + thread.join() + + assert code == 1 + payload = _last_document(capsys.readouterr().out) + assert payload["stop"] == "approval_denied" + assert payload["executed"] is False + record = json.loads((run_dir / "plan.json").read_text()) + assert "executed_run_id" not in record + + +def test_go_approve_unanswered_times_out_with_the_plan_unexecuted(tmp_path, capsys): + run_dir = _saved_plan(tmp_path, capsys) + code = main(["go", str(run_dir), "--approve", "--approval-timeout", "0.6", "--json"]) + + assert code == 1 + payload = _last_document(capsys.readouterr().out) + assert payload["stop"] == "approval_timeout" + record = json.loads((run_dir / "plan.json").read_text()) + assert "executed_run_id" not in record + + +def test_go_approve_approved_executes_and_stamps_the_plan(tmp_path, capsys): + run_dir = _saved_plan(tmp_path, capsys) + thread = _answer(run_dir) + code = main(["go", str(run_dir), "--approve", "--approval-timeout", "8", "--json"]) + thread.join() + + assert code == 0 + payload = _last_document(capsys.readouterr().out) + assert payload["executed"] is True + record = json.loads((run_dir / "plan.json").read_text()) + assert record["executed_run_id"] + + +def test_the_plan_payload_carries_the_admitted_shape(tmp_path, capsys): + """An external driver must not have to re-read plan.json to learn what was + admitted, under what fingerprint, and whether it can change anything.""" + trace = tmp_path / "run" / "trace.jsonl" + assert main(["plan", "investigate", "--scripted", "--trace", str(trace), "--json"]) == 0 + + payload = _last_document(capsys.readouterr().out) + assert payload["run_dir"] == str(trace.parent) + assert payload["fingerprint"] + names = {node["name"] for node in payload["proposal"]["nodes"]} + assert names # the admitted round, as data + # The incident registry declares deploy as its mutating kind, and the + # admitted replan does not contain it. + assert payload["mutating"] is False + record = json.loads((trace.parent / "plan.json").read_text()) + assert payload["fingerprint"] == record["fingerprint"] + assert names == {node["name"] for node in record["proposal"]["nodes"]} diff --git a/tests/test_config.py b/tests/test_config.py index 079ccac..4d10768 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -372,6 +372,23 @@ def test_an_explicit_policy_still_reports_its_own_layer(project, capsys): assert payload["sources"]["policy"] == "flag" +def test_go_is_governed_by_the_config_files_policy(project, capsys): + """`go` re-admits the saved plan through the gate, so the policy the file + names must govern there too. It resolved `model`, `tenant` and + `max_tokens` from the config but never `policy`: the plan was admitted + under the operator's document and then executed under the registry + default.""" + _, planned = _plan_payload(capsys) + run_dir = str(Path(planned["trace"]).parent) + + code = main(["go", run_dir, "--json"]) + payload = json.loads(capsys.readouterr().out) + + assert code == 0 + assert payload["policy_source"] == "flag-or-config" + assert "deny.toml" in payload["policy"] + + def test_the_demo_command_reads_the_config_too(tmp_path, monkeypatch, capsys): """`memory` and `reviewer_model` were declared in KEYS and read by nothing, so the same file made `plan` fail on a bad model and left `demo` scripted.""" diff --git a/tests/test_graph_viewmodel.py b/tests/test_graph_viewmodel.py index 6ddc552..7660335 100644 --- a/tests/test_graph_viewmodel.py +++ b/tests/test_graph_viewmodel.py @@ -160,6 +160,28 @@ def test_an_open_agent_node_reports_its_live_sub_step_tokens(tmp_path): assert node.live_tokens == 340 +def test_a_rerunning_node_does_not_recount_its_closed_executions(tmp_path): + """`live_tokens` is the still-open execution's spend. A node that ran + before has its earlier sub-steps already inside `tokens` via their `end` + event; scanning every sub-event again displayed that spend twice.""" + trace = TraceRecorder(tmp_path / "t.jsonl") + trace.event( + run_id="r1", graph="agent", node="topology", phase="topology", step=0, + state_delta={"nodes": ["worker"], "edges": [["__start__", "worker", "static"]]}, + ) + trace.event(run_id="r1", graph="agent", node="worker", phase="start", step=1) + trace.event(run_id="r1", graph="agent", node="worker:model", phase="model", step=2, tokens=100) + trace.event(run_id="r1", graph="agent", node="worker", phase="end", step=1, + duration_ms=3.0, tokens=100) + trace.event(run_id="r1", graph="agent", node="worker", phase="start", step=3) + trace.event(run_id="r1", graph="agent", node="worker:model", phase="model", step=4, tokens=30) + view = build_graph_view(replay(trace, "r1")) + node = next(n for n in view.nodes if n.label == "worker") + assert node.status == "running" + assert node.tokens == 100 + assert node.live_tokens == 30 + + def test_planless_planner_run_is_an_honest_empty(tmp_path): trace = TraceRecorder(tmp_path / "t.jsonl") trace.event(run_id="r1", graph="loop", node="planner", phase="plan", step=1) diff --git a/tests/test_mcp_gate.py b/tests/test_mcp_gate.py new file mode 100644 index 0000000..accffa7 --- /dev/null +++ b/tests/test_mcp_gate.py @@ -0,0 +1,170 @@ +"""The MCP supervision surface — `grapharc.mcp`. + +The property under test is the one the module refuses to compromise: a +supervised agent may request and may check, and can never decide. Everything +else is the thin-shim contract — the server drives the tested CLI, confines +what a client may name, honours the plan's own mutating verdict fail-closed, +and keeps stdout for the protocol. +""" + +from __future__ import annotations + +import json +import threading +import time +from pathlib import Path + +import pytest + +from grapharc.cli.main import main +from grapharc.mcp import FORBIDDEN_TOOL_WORDS, build_server +from grapharc.mcp.driver import ( + DriverError, + confine_run_dir, + plan_is_mutating, + read_plan_record, +) +from grapharc.planner.approval_file import REQUEST_FILENAME + + +def _unwrap(raw) -> dict: + """FastMCP's call_tool result as the tool's own dict, across SDK shapes.""" + if isinstance(raw, tuple) and len(raw) == 2 and isinstance(raw[1], dict): + structured = raw[1] + # Structured output arrives either as the dict itself or under "result". + return structured.get("result", structured) + blocks = raw[0] if isinstance(raw, tuple) else raw + text = "".join(getattr(block, "text", "") for block in blocks) + return json.loads(text) + + +@pytest.mark.asyncio +async def test_the_surface_is_three_tools_and_no_approval_verb(tmp_path): + """A client that could call approve() would be approving its own + proposal, which is not approval. The refusal is a property, not prose.""" + server = build_server(tmp_path) + tools = await server.list_tools() + + names = {tool.name for tool in tools} + assert names == {"plan", "show_graph", "execute"} + for name in names: + for word in FORBIDDEN_TOOL_WORDS: + assert word not in name.lower() + + +@pytest.mark.asyncio +async def test_the_plan_tool_offers_no_registry_policy_or_model_parameter(tmp_path): + """Those resolve from the operator's grapharc.toml in the server's root; + the requester's call must not be able to widen what the operator set.""" + server = build_server(tmp_path) + plan_tool = next(t for t in await server.list_tools() if t.name == "plan") + + parameters = set((plan_tool.inputSchema or {}).get("properties", {})) + assert parameters <= {"goal", "scripted", "max_rounds"} + + +@pytest.mark.asyncio +async def test_plan_show_execute_scripted_end_to_end(tmp_path, capsys): + """The whole supervised flow, spend-free: propose, read the shape back, + execute the read-only plan on the spot, and read the record of the run. + The server itself writes nothing to stdout — the protocol owns it.""" + server = build_server(tmp_path) + + planned = _unwrap(await server.call_tool("plan", {"goal": "investigate", "scripted": True})) + assert planned["stop"] == "planned" + assert planned["proposal"]["nodes"], "the admitted shape travels as data" + assert planned["fingerprint"] + assert planned["mutating"] is False # the incident replan admits no deploy + run_dir = planned["run_dir"] + + shown = _unwrap(await server.call_tool("show_graph", {"run_dir": run_dir})) + assert shown["status"] == "planned" + assert shown["awaiting_approval"] is False + + done = _unwrap(await server.call_tool("execute", {"run_dir": run_dir})) + assert done["executed"] is True # read-only: no park, the host prompt sufficed + + after = _unwrap(await server.call_tool("show_graph", {"run_dir": run_dir})) + assert after["status"] == "done" + assert after["executed_run_id"] + assert "mermaid" in after and after["metrics"]["nodes_executed"] + + assert capsys.readouterr().out == "" + + +def _mark_mutating(root: Path, run_dir: str) -> Path: + """Flip the record's verdict, resolving the CLI's root-relative run_dir.""" + resolved = Path(run_dir) if Path(run_dir).is_absolute() else root / run_dir + plan_file = resolved / "plan.json" + record = json.loads(plan_file.read_text()) + record["mutating"] = True + plan_file.write_text(json.dumps(record, indent=2) + "\n") + return resolved + + +@pytest.mark.asyncio +async def test_a_mutating_plan_parks_and_a_timeout_leaves_it_unexecuted(tmp_path): + server = build_server(tmp_path) + planned = _unwrap(await server.call_tool("plan", {"goal": "fix", "scripted": True})) + run_dir = _mark_mutating(tmp_path, planned["run_dir"]) + + outcome = _unwrap( + await server.call_tool("execute", {"run_dir": str(run_dir), "approval_timeout": 0.8}) + ) + + assert outcome["executed"] is False + assert outcome["stop"] == "approval_timeout" + assert "grapharc approve" in outcome["approve_command"] + assert "executed_run_id" not in json.loads((run_dir / "plan.json").read_text()) + + +@pytest.mark.asyncio +async def test_a_mutating_plan_executes_after_an_out_of_band_approval(tmp_path): + """The park is the request and the human's CLI is the decision — the MCP + connection never carries a yes.""" + server = build_server(tmp_path) + planned = _unwrap(await server.call_tool("plan", {"goal": "fix", "scripted": True})) + run_dir = _mark_mutating(tmp_path, planned["run_dir"]) + + def answer(): + deadline = time.monotonic() + 20 + while time.monotonic() < deadline: + if (run_dir / REQUEST_FILENAME).exists(): + assert main(["approve", str(run_dir), "--json"]) == 0 + return + time.sleep(0.05) + + thread = threading.Thread(target=answer) + thread.start() + outcome = _unwrap( + await server.call_tool("execute", {"run_dir": str(run_dir), "approval_timeout": 20}) + ) + thread.join() + + assert outcome["executed"] is True + + +def test_a_plan_record_without_the_verdict_reads_as_mutating(tmp_path): + """Absent is never safe: an old plan.json predates the field, and the + driver must park it rather than assume it read-only.""" + assert plan_is_mutating({}) is True + assert plan_is_mutating({"mutating": "false"}) is True # a string is not a verdict + assert plan_is_mutating({"mutating": False}) is False + + +def test_a_run_dir_outside_the_root_is_refused(tmp_path): + (tmp_path / "inside").mkdir() + assert confine_run_dir(tmp_path, "inside") == (tmp_path / "inside").resolve() + + with pytest.raises(DriverError) as refusal: + confine_run_dir(tmp_path, "/etc") + assert "outside" in str(refusal.value) + + with pytest.raises(DriverError): + confine_run_dir(tmp_path, "../elsewhere") + + +def test_a_directory_without_a_plan_names_the_missing_step(tmp_path): + with pytest.raises(DriverError) as refusal: + read_plan_record(tmp_path) + assert "call plan first" in str(refusal.value) diff --git a/tests/test_replay.py b/tests/test_replay.py index 69b2c21..71d935d 100644 --- a/tests/test_replay.py +++ b/tests/test_replay.py @@ -993,8 +993,34 @@ def test_rate_card_matches_the_longest_prefix(trace): assert RateCard().rate_for("anything") is None +def test_a_stamped_error_events_tokens_are_counted_and_priced(trace): + """The kernel stamps `error` terminals with the node's spend and + `metrics.summarize` counts them; the cost report skipped the whole + execution — a run stopped *for overspending* was billed as nearly free + while claiming to be complete.""" + trace.event(run_id="r1", graph="demo", node="a", phase="start", step=1) + trace.event(run_id="r1", graph="demo", node="a:model", phase="model", step=2, + tokens=100) + trace.event(run_id="r1", graph="demo", node="a", phase="error", step=1, + duration_ms=5.0, error="budget: tokens", tokens=150) + trace.event(run_id="r1", graph="demo", node="b", phase="start", step=3) + trace.event(run_id="r1", graph="demo", node="b", phase="end", step=3, + duration_ms=2.0, tokens=50) + + cost = attribute(trace, "r1", rates=RateCard(default=1.0)) + + assert cost.tokens == summarize(trace, "r1").tokens == 200 + assert cost.node("a").tokens == 150 + assert cost.errors == 1 + assert cost.estimated_cost_usd == pytest.approx(0.2) + assert cost.unpriced_tokens == 0 and cost.complete + assert cost.tokens_before_error == 0, "the stamp already includes the sub-steps" + + def test_tokens_spent_inside_a_failed_node_are_reported_separately(trace): - """The kernel's error event carries no tokens; the spend must not vanish.""" + """An error event with no token count (an older or hand-built producer): + the sub-steps' spend must not vanish, and must not be folded into the + total that matches `metrics`.""" trace.event(run_id="r1", graph="demo", node="agent", phase="start", step=1) trace.event(run_id="r1", graph="demo", node="agent:model", phase="model", step=2, tokens=700) diff --git a/uv.lock b/uv.lock index 7ffa97c..d7db3bf 100644 --- a/uv.lock +++ b/uv.lock @@ -406,6 +406,7 @@ slack = [ [package.dev-dependencies] dev = [ { name = "httpx" }, + { name = "mcp" }, { name = "pytest" }, { name = "pytest-asyncio" }, { name = "pytest-timeout" }, @@ -437,6 +438,7 @@ provides-extras = ["openrouter", "openai", "ollama", "server", "mcp", "ladybug", [package.metadata.requires-dev] dev = [ { name = "httpx", specifier = ">=0.27" }, + { name = "mcp", specifier = ">=1.2" }, { name = "pytest", specifier = ">=8.2" }, { name = "pytest-asyncio", specifier = ">=0.24" }, { name = "pytest-timeout", specifier = ">=2.3" },