From 933163cf854714b8b307709ba76af880a2c2caaa Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Tue, 4 Aug 2026 00:51:45 +0530 Subject: [PATCH] A policy's `resource = "node"` rules were compiled by nobody, so a denied kind ran MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PolicyEngine.edge_policy()` compiled the edge half of a document and nothing compiled the other one. `AdmissionChecker` gated node kinds on `NodeRegistry` membership alone, and `check_node` — correct, tested, and advertised in the engine's own module docstring as the answer to "may this node run?" — had no runtime caller anywhere in `grapharc/`. A written, valid, non-refused `deny` rule over a node kind therefore meant nothing at all: $ grapharc plan "fix the outage" --policy nodepolicy.toml policy : nodepolicy.toml (tenant 'default', 1 edge rule(s)) round 1: admitted nodes=2 executed=True state : notes=['triage ran', 'deploy ran', ...] Fail-open, silent, and on the documented path: `grapharc/policy/example.toml` ships `no-shell-nodes` as the canonical example of governing what may run, so an operator who copied the shipped example got a policy that denied nothing. The only hint that half the file had been discarded was `1 edge rule(s)` in a line that reads as a summary rather than a warning. **Enforced, not merely refused at load.** `NodePolicy`/`NodeRule` sit beside `EdgePolicy`/`EdgeRule` with the same tiered semantics — every deny before every ask before every allow, first match within a tier, unmatched takes the default — and `PolicyEngine.node_policy(tenant=…)` compiles the node half exactly as `edge_policy()` compiles the edge half. A test pins the compiled object to `check_node` across a kind x tenant matrix, as the edge one already was. `AdmissionChecker(node_policy=…)` consults it for every proposed node, in every scope, keyed on the registry `kind` like every other node decision — so renaming a denied instance launders nothing and naming an instance after a permitted kind borrows nothing. A refusal is `policy/node_denied` (or `node_needs_approval`, which reports `NEEDS_APPROVAL` exactly as the edge half does), carrying the rule's own `reason`, under the POLICY check the planner already replans against. **What a document that says nothing about nodes means.** `node_policy()` is faithful to `check_node`, which means a document with no node rules and `default = "deny"` compiles to a policy that denies every kind. That is the right answer for the API and the wrong reading of an operator's intent, so `grapharc plan --policy` compiles the node half only when the document declares at least one `node` rule: saying nothing about nodes is not the same statement as denying all of them, and the registry — an allowlist with no wildcard — is still the gate in that case. `node_policy=None` on the checker means exactly that, and is what every existing caller keeps. Both halves now travel together as `GatePolicy` (`resolve_edge_policy` becomes `resolve_policy`; `compile_policy` is the single place a document becomes admission's objects, so `--policy`, a cached generated policy and a freshly generated one cannot be read three different ways). The banner counts both: `(tenant 'default', 1 edge rule(s), 2 node rule(s))`. The repro above now refuses `deploy` with the operator's reason and replans around it; `example.toml`'s node rules are enforced, and a test drives the shipped document through the gate. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 + README.md | 2 +- ROADMAP.md | 5 +- docs/cookbook/05-governance.md | 104 +++++++++++++++++--- grapharc/cli/generate.py | 35 +++++-- grapharc/cli/graphrun.py | 9 +- grapharc/cli/plan.py | 93 +++++++++++++++--- grapharc/examples/plan_incident.py | 5 + grapharc/planner/__init__.py | 4 + grapharc/planner/admission.py | 116 ++++++++++++++++++++++- grapharc/policy/__init__.py | 6 ++ grapharc/policy/engine.py | 41 ++++++++ grapharc/policy/example.toml | 4 + grapharc/stdlib.py | 4 + tests/test_admission.py | 147 +++++++++++++++++++++++++++++ tests/test_cli.py | 93 +++++++++++++++++- tests/test_cookbook_governance.py | 32 +++++++ tests/test_generate.py | 37 +++++++- tests/test_policy_engine.py | 103 ++++++++++++++++++++ 19 files changed, 793 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3cad53..c7e2e1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,3 +16,4 @@ Entries are newest-last within a release, matching the order they were written. - 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, and length alone turned out not to be a safe rank — a citation list like `[101, 205, 309, …]` *longer* than the verdict still won — so object spans are tried before array spans, each longest-first; junk still returns `None`, so fail-closed is unchanged. - a **bare backend name was read as a model name**, because `split_spec` only consulted the backend list when the spec contained a slash. `--model claude-cli` — the backend `models --check` reports as `usable` — shelled out to `claude -p --model claude-cli` and was refused by the CLI on *every* call, and `--model mock` named the paid subscription backend and spawned the real binary, so the double documented as "never reaches a provider" reached for one. A bare backend name now resolves to that backend (`claude-cli` to its own default model, `mock` to the scripted double, which ignores the model segment anyway); `openrouter`, `openai` and `ollama` front catalogues rather than a model, so those are refused with an example spelling instead of a guess about what to bill you for. The slash forms and bare *model* names are unchanged. - a failing `claude -p` **reported no reason at all**. The CLI exits non-zero with an empty stderr and puts its explanation in the JSON envelope on stdout; the gateway read only stderr, so the error was `claude -p exited 1: ` — a sentence that stops at the colon. Since the wrong-model bug above presented itself exactly that way, the two hid each other. stdout is read first now, and the recovered text also feeds the transient-vs-deterministic classifier, which was previously deciding from `""`. +- a policy document's `resource = "node"` rules were **silently discarded**. `edge_policy()` compiled the edge half and nothing compiled the other one, `AdmissionChecker` gated node kinds on registry membership alone, and `check_node` — correct, documented, advertised in the engine's own docstring — had no runtime caller anywhere. So a document denying the kind `deploy` admitted it and ran it, and the only hint that half the file had been dropped was an oblique `1 edge rule(s)` in a line that reads as a summary. The shipped `example.toml` led with exactly that shape: an operator who copied `no-shell-nodes` got a policy that denied nothing. `PolicyEngine.node_policy()` now compiles the node half as `edge_policy()` does the edge half, `AdmissionChecker(node_policy=...)` consults it for every proposed node, and a refusal comes back as `policy/node_denied` quoting the rule's own `reason` — a code the planner replans against, exactly like `edge_denied`. A document that declares *no* node rules still leaves kinds to the registry: saying nothing about nodes is not the same statement as denying all of them, and the banner now counts both halves so a reader can tell which was said. diff --git a/README.md b/README.md index 8e50429..eabceac 100644 --- a/README.md +++ b/README.md @@ -381,7 +381,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, 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`. +**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 the tool plane: **the planner half is wired and the tool half is not.** `PolicyEngine.edge_policy()` and `PolicyEngine.node_policy()` compile the document into the `EdgePolicy` and `NodePolicy` the admission checker consults, and `grapharc plan --policy` is a real caller — so what may run, and what may connect to what, *is* governed by a document you can read. (A `resource = "node"` rule used to be dropped by the compiler and enforced by nothing; [issue #66](https://github.com/CodeGraphContext/GraphARC/issues/66).) 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`. ## Independent verification diff --git a/ROADMAP.md b/ROADMAP.md index 620b425..9a91a7e 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -341,7 +341,10 @@ Everything here works and nothing calls it. to an unknown tenant a load error and a request naming one a recorded denial. - [x] **7.5 — The document reaches the gate.** `edge_policy(tenant=…)` - compiles `edge` rules into the `EdgePolicy` `AdmissionChecker` consults, + compiles `edge` rules into the `EdgePolicy` `AdmissionChecker` consults + and `node_policy(tenant=…)` compiles `node` rules into the `NodePolicy` + beside it — the node half reached nothing at all until issue #66, so a + `deny` rule over a kind was text and the kind still ran — and `grapharc plan --policy` is a shipped caller, so this package is no longer imported by nothing. What the compiled object still cannot carry is what `permission_policy()` cannot either: the approver role and the audit diff --git a/docs/cookbook/05-governance.md b/docs/cookbook/05-governance.md index b7daf8f..93f2eab 100644 --- a/docs/cookbook/05-governance.md +++ b/docs/cookbook/05-governance.md @@ -1320,6 +1320,7 @@ id = "no-shell-nodes" resource = "node" match = "shell_*" effect = "deny" +reason = "a shell node is an unbounded tool" [[rule]] id = "other-nodes-run" @@ -1404,7 +1405,7 @@ edge triage->patch default allow rule=other-edges-are-fine spend * default allow rule=small-spend-is-fine spend * default ask rule=over-a-dollar-asks-finance ask:finance -policy version: 2026-07-01 digest: b1d593faa1d41fcf +policy version: 2026-07-01 digest: 028e3486e70a1161 audit records: 11 ``` @@ -1602,9 +1603,9 @@ print("same digest: ", parse_document(edited).digest == engine.digest) ``` ``` -tool write_file allow rule=acme-may-write v=2026-07-01 digest=b1d593fa ctx={'run_id': 'run-42', 'node': 'patch'} -tool delete_bucket deny rule=no-deletes v=2026-07-01 digest=b1d593fa ctx={'run_id': 'run-42', 'node': 'patch'} -edge triage->deploy deny rule=nothing-routes-into-deploy v=2026-07-01 digest=b1d593fa ctx={'run_id': 'run-42', 'node': 'patch'} +tool write_file allow rule=acme-may-write v=2026-07-01 digest=028e3486 ctx={'run_id': 'run-42', 'node': 'patch'} +tool delete_bucket deny rule=no-deletes v=2026-07-01 digest=028e3486 ctx={'run_id': 'run-42', 'node': 'patch'} +edge triage->deploy deny rule=nothing-routes-into-deploy v=2026-07-01 digest=028e3486 ctx={'run_id': 'run-42', 'node': 'patch'} same version: True same digest: False @@ -1627,10 +1628,12 @@ really does lose records and why it defaults to off. ## How do I make my TOML document govern admission? -It does not, by default. `AdmissionChecker` takes an `EdgePolicy` built in code; -`PolicyEngine.check_edge` answers over a document. **There is no shipped -compiler between them** — `permission_policy()` exists for tools and has no edge -equivalent. Here is the bridge, which is about fifteen lines: +`AdmissionChecker` takes an `EdgePolicy` and a `NodePolicy` built in code; +`PolicyEngine.check_edge` and `check_node` answer over a document. +`PolicyEngine.edge_policy()` and `PolicyEngine.node_policy()` are the shipped +compilers between them, and the next recipe uses both. Here is what +`edge_policy()` does, written out, because the semantics are worth seeing once — +it is about fifteen lines: ```python from grapharc.harness.permissions import Decision @@ -1717,6 +1720,75 @@ cannot drift silently. --- +## How do I stop a node *kind* from running, from the document? + +A `resource = "node"` rule is compiled by `PolicyEngine.node_policy()` and +handed to the checker as `node_policy=`. It decides on the registry kind, like +everything else here, and a refusal quotes the `reason` the rule carried. + +```python +from grapharc.planner import ( + AdmissionChecker, + NodeRegistry, + NodeSpec, + ProposedEdge, + ProposedNode, + Subgraph, +) +from grapharc.policy import PolicyEngine +from grapharc.runtime.graph import START + +engine = PolicyEngine.from_file("policy.toml") +gate = AdmissionChecker( + registry=NodeRegistry([NodeSpec(name="shell_exec"), NodeSpec(name="summarise")]), + edge_policy=engine.edge_policy(), + node_policy=engine.node_policy(), +) + +# `helper` is a registered kind wired along a permitted edge. The document +# still refuses it, because of what it *is*. +result = gate.check( + Subgraph( + nodes=( + ProposedNode(name="helper", kind="shell_exec"), + ProposedNode(name="summarise"), + ), + edges=( + ProposedEdge(source=START, target="helper"), + ProposedEdge(source="helper", target="summarise"), + ), + ) +) +print("status:", result.status.value) +for rejection in result.rejections: + print(rejection.render()) +print("engine agrees:", engine.check_node("shell_exec").effect.value) +print("and about the other kind:", engine.check_node("summarise").effect.value) +``` + +``` +status: rejected +[policy/node_denied] helper: the node policy denies this kind: kind 'shell_exec' (proposed as 'helper'): a shell node is an unbounded tool the decision is made on the registry kind, not the name you chose: renaming the node will not change it — propose a permitted kind +engine agrees: deny +and about the other kind: allow +``` + +**Why it works this way.** The registry and the node policy are two different +questions and a kind has to pass both: the registry says a kind exists and what +it costs — operator code, fixed at start-up — while the document says whether it +may run here, and can be edited without touching that code. `node_policy=` is +`None` by default, and that is not a wildcard: with no document the registry is +the only node gate, and it is an allowlist with no wildcard either. + +**The sharp edge.** `node_policy()` is faithful to `check_node`, so a document +with *no* node rules and `default = "deny"` compiles to a policy that denies +every kind. That is the same answer `check_node` gives, and it is why +`grapharc plan --policy` compiles the node half only when the document declares +at least one `node` rule — saying nothing about nodes is not the same statement +as denying all of them. Compiling by hand, you decide which you meant. + +--- + ## What this section does not give you Stated plainly, because a governance layer that overstates itself is worse than @@ -1727,8 +1799,9 @@ none: unchecked, and that is your gate to build. 2. **`parent_depth` is on your honour.** The checker cannot observe how deep the run really is. -3. **Edge approvals are not routed.** `NEEDS_APPROVAL` tells you an edge needs a - human; nothing carries it to one. The `ApprovalRouter` handles tools. +3. **Admission approvals are not routed.** `NEEDS_APPROVAL` tells you an edge or + a node kind needs a human; nothing carries it to one. The `ApprovalRouter` + handles tools. 4. **Cycles across the boundary are invisible.** The acyclicity check sees only the topology inside the proposal. 5. **`known_nodes` and `Materializer` do not compose.** A proposal wired to a @@ -1744,14 +1817,17 @@ none: reassignment, but `args` is an ordinary dict whose contents can be mutated in place. `fingerprint()` is what detects that, by hashing content rather than trusting the reference — and `Materializer` checks it for you. -9. **No shipped edge-policy compiler.** The TOML document's `edge` rules do not - reach `AdmissionChecker` on their own; the bridge above is fifteen lines you - write and this section's tests pin. +9. **A document reaches admission only when something hands it over.** The + compilers are shipped (`edge_policy()`, `node_policy()`) and `grapharc plan + --policy` calls both, but an `AdmissionChecker` you build yourself is subject + to a document only if you pass the compiled objects to it. Its `tool` and + `spend` rules reach neither gate: those are the harness's plane. 10. **The spend ledger is in-process.** It does not survive a restart and is not shared between processes. The parts that *are* enforced, and that every snippet above demonstrates: a -proposal cannot execute itself, an unregistered kind cannot run, a denied +proposal cannot execute itself, an unregistered kind cannot run, a kind the +document denies cannot run either, a denied transition cannot be renamed into an allowed one, an over-budget plan is refused before its first node exists, and every decision — yes and no alike — is a recorded event carrying the reason. diff --git a/grapharc/cli/generate.py b/grapharc/cli/generate.py index aacb9d4..ffcb301 100644 --- a/grapharc/cli/generate.py +++ b/grapharc/cli/generate.py @@ -149,7 +149,15 @@ def resolve_or_generate_policy( fallback: Any = None, fallback_label: str = "", ) -> tuple[Any, str, str]: - """Return `(edge_policy, description, source)`. + """Return `(gate_policy, description, source)`. + + The policy is a `grapharc.cli.plan.GatePolicy` carrying both halves a + document compiles to — the edge rules and the node rules — because a run + gated by only one half of a document is the bug this pair exists to prevent. + Every document goes through `compile_policy`, generated ones included, so + what a freshly generated policy means and what the same file means when it + is read back off disk next run cannot differ. A fallback has no node half at + all: it is an `EdgePolicy` written in Python, not a document. `source` is one of `flag-or-config`, `registry-default`, `generated-cached`, `generated`, `builtin-default`. Callers put it in the payload verbatim: it is @@ -168,23 +176,33 @@ def resolve_or_generate_policy( exist and permit ones that do. """ from grapharc import stdlib - from grapharc.cli.plan import resolve_edge_policy + from grapharc.cli.plan import GatePolicy, compile_policy, resolve_policy if policy_path is not None: - policy, description = resolve_edge_policy(policy_path, tenant=tenant) + policy, description = resolve_policy(policy_path, tenant=tenant) return policy, description, "flag-or-config" cached = generated_policy_path(workdir) if cached.is_file(): - policy, description = resolve_edge_policy(cached, tenant=tenant) + # Read back as an ordinary document, node rules included: a generated + # file the operator has since edited is theirs, not the generator's. + policy, description = resolve_policy(cached, tenant=tenant) return policy, f"{description} [previously generated]", "generated-cached" def _settled() -> tuple[Any, str, str]: if fallback is not None: label = fallback_label or "registry default" - return fallback, f"{label} ({describe_policy(fallback)})", "registry-default" + return ( + GatePolicy(edge=fallback), + f"{label} ({describe_policy(fallback)})", + "registry-default", + ) builtin = stdlib.default_edge_policy() - return builtin, f"built-in default ({describe_policy(builtin)})", "builtin-default" + return ( + GatePolicy(edge=builtin), + f"built-in default ({describe_policy(builtin)})", + "builtin-default", + ) if model is None: return _settled() @@ -199,7 +217,10 @@ def _settled() -> tuple[Any, str, str]: from grapharc.policy import PolicyEngine engine = PolicyEngine.from_toml(toml_text) - policy = engine.edge_policy(tenant=tenant) + # Compiled exactly as the file will be on the next run — the text below + # is written to disk and read back as an ordinary document, so the two + # readings must not differ. + policy = compile_policy(engine, tenant=tenant) except Exception: # noqa: BLE001 — any failure falls back rather than breaking the run policy, description, source = _settled() return policy, f"{description} [generation failed]", source diff --git a/grapharc/cli/graphrun.py b/grapharc/cli/graphrun.py index f878942..f31310d 100644 --- a/grapharc/cli/graphrun.py +++ b/grapharc/cli/graphrun.py @@ -142,7 +142,7 @@ def run_graph( proposal = build_proposal(document) bundle = resolve_registry(registry_target) registry, state_schema, writes = bundle.registry, bundle.state_schema, bundle.writes - edge_policy, policy_description, policy_source = resolve_or_generate_policy( + gate_policy, policy_description, policy_source = resolve_or_generate_policy( policy_path, tenant=tenant, fallback=bundle.default_policy, @@ -157,7 +157,12 @@ def run_graph( schema = state_schema or IncidentState trace_path = trace_path or Path(tempfile.mkdtemp(prefix="grapharc-run-")) / "trace.jsonl" trace = TraceRecorder(trace_path) - checker = AdmissionChecker(registry=registry, edge_policy=edge_policy, trace=trace) + checker = AdmissionChecker( + registry=registry, + edge_policy=gate_policy.edge, + node_policy=gate_policy.node, + trace=trace, + ) # `Budget()` is genuinely unlimited on every dimension, so with no # ceilings the budget check passes anything. The meter is real only when diff --git a/grapharc/cli/plan.py b/grapharc/cli/plan.py index 9b22335..4c55eb7 100644 --- a/grapharc/cli/plan.py +++ b/grapharc/cli/plan.py @@ -15,10 +15,11 @@ - `--registry module:attr` — the kinds a planner may propose. Absence is refusal; there is no wildcard. Defaults to the shipped incident demo. -- `--policy PATH [--tenant NAME]` — a TOML document whose `edge` rules are - compiled to the `EdgePolicy` admission consults, via - `PolicyEngine.edge_policy()`. This is the path that makes declarative - governance constrain a run rather than answer questions about one. +- `--policy PATH [--tenant NAME]` — a TOML document whose `edge` and `node` + rules are compiled to the `EdgePolicy` and `NodePolicy` admission consults, + via `PolicyEngine.edge_policy()` and `PolicyEngine.node_policy()`. This is the + path that makes declarative governance constrain a run rather than answer + questions about one. Exit codes follow the CLI's convention: `0` the goal was met, `1` the run stopped short for a recorded reason (refused, out of budget, out of rounds), @@ -133,24 +134,79 @@ def _accepts_an_argument(factory: Any) -> bool: ) -def resolve_edge_policy(policy_path: Path | None, *, tenant: str) -> tuple[Any, str]: - """Compile a policy document's edge rules, or fall back to the demo's. +@dataclass(frozen=True) +class GatePolicy: + """The gate objects one policy document compiles to, travelling together. - Returns `(edge_policy, description)`. The description is printed, because - which policy a run was subject to is the first thing anyone asks afterwards. + Two halves of the same file: `edge` says what may be wired, `node` says + which kinds may run at all. They travel as one object because a run gated by + one half of a document is exactly the failure this exists to fix — the node + half used to be compiled by nobody, so a `deny` rule over a node kind was + text (issue #66). + + `node` is `None` when the document declares no `node` rules. That is not the + same as an empty `NodePolicy`, which denies every kind: a document that says + nothing about nodes leaves them to the registry, which is itself an + allowlist with no wildcard, while a document that mentions nodes at all is + taken at its word, default included. + """ + + edge: Any + node: Any = None + + +def resolve_policy(policy_path: Path | None, *, tenant: str) -> tuple[GatePolicy, str]: + """Compile a policy document's edge and node rules, or fall back to the demo's. + + Returns `(gate_policy, description)`. The description is printed, because + which policy a run was subject to is the first thing anyone asks afterwards — + and it counts both kinds of rule, so a document whose node rules do not + constrain this run says so in the count rather than by omission. """ if policy_path is None: from grapharc.examples.plan_incident import default_edge_policy - return default_edge_policy(), "built-in demo (deny -> deploy, allow otherwise)" + return ( + GatePolicy(edge=default_edge_policy()), + "built-in demo (deny -> deploy, allow otherwise)", + ) from grapharc.policy import PolicyEngine, PolicyError try: engine = PolicyEngine.from_file(policy_path) except (OSError, PolicyError) as exc: raise PlanSetupError(f"--policy {str(policy_path)!r}: {exc}") from exc - policy = engine.edge_policy(tenant=tenant) - return policy, f"{policy_path} (tenant {tenant!r}, {len(policy.rules)} edge rule(s))" + policy = compile_policy(engine, tenant=tenant) + # Both counts are of the rules that survived tenant scoping, which is what + # this run is actually subject to. + description = ( + f"{policy_path} (tenant {tenant!r}, {len(policy.edge.rules)} edge rule(s), " + f"{0 if policy.node is None else len(policy.node.rules)} node rule(s))" + ) + return policy, description + + +def compile_policy(engine: Any, *, tenant: str) -> GatePolicy: + """Compile a loaded `PolicyEngine`'s planner-side rules into both gates. + + The one place a document becomes admission's objects, so a run started from + a file and a run started from a document generated in memory are subject to + the same reading of the same rules. + + The node half is compiled **only when the document declares node rules**. + `PolicyEngine.node_policy()` is faithful to `check_node`, which means a + document with no node rules and a `deny` default denies every kind — a true + reading of the engine, and not what an operator who wrote only edge rules + meant to say. Saying nothing about nodes leaves them where they were: the + registry, an allowlist with no wildcard. + """ + from grapharc.policy import ResourceKind + + node_rules = engine.document.rules_for(ResourceKind.NODE) + return GatePolicy( + edge=engine.edge_policy(tenant=tenant), + node=engine.node_policy(tenant=tenant) if node_rules else None, + ) def _model_for(spec: str | None, registry_target: str = DEFAULT_REGISTRY) -> tuple[Any, str]: @@ -219,7 +275,7 @@ def plan( model, model_description = _model_for(model_spec, registry_target) bundle = resolve_registry(registry_target, model) registry, state_schema, writes = bundle.registry, bundle.state_schema, bundle.writes - edge_policy, policy_description, policy_source = resolve_or_generate_policy( + gate_policy, policy_description, policy_source = resolve_or_generate_policy( policy_path, tenant=tenant, # Only a *real* backend generates. The scripted planner has no @@ -263,7 +319,8 @@ def _announce(message: str) -> None: build_loop = bundle.build_loop or incident_build_loop loop = build_loop( model, - edge_policy=edge_policy, + edge_policy=gate_policy.edge, + node_policy=gate_policy.node, trace=trace, budget=Budget(max_tokens=max_tokens), limits=LoopLimits(max_rounds=max_rounds), @@ -355,4 +412,12 @@ def _announce(message: str) -> None: return EXIT_OK if result.succeeded else EXIT_FAILED -__all__ = ["DEFAULT_REGISTRY", "PlanSetupError", "plan", "resolve_edge_policy", "resolve_registry"] +__all__ = [ + "DEFAULT_REGISTRY", + "GatePolicy", + "PlanSetupError", + "compile_policy", + "plan", + "resolve_policy", + "resolve_registry", +] diff --git a/grapharc/examples/plan_incident.py b/grapharc/examples/plan_incident.py index 72c3e35..17070b9 100644 --- a/grapharc/examples/plan_incident.py +++ b/grapharc/examples/plan_incident.py @@ -146,6 +146,7 @@ def build_loop( model: Any, *, edge_policy: EdgePolicy | None = None, + node_policy: Any = None, trace: TraceRecorder | None = None, budget: Budget | None = None, limits: LoopLimits | None = None, @@ -178,6 +179,10 @@ def build_loop( checker=AdmissionChecker( registry=registry, edge_policy=edge_policy or default_edge_policy(), + # There is no default node policy: this demo's registry *is* its + # node allowlist. One arrives only when a policy document declares + # node rules, and then it gates every kind the planner proposes. + node_policy=node_policy, trace=trace, # This loop materializes each admitted round as a standalone # graph, so structural runnability is admission's business: diff --git a/grapharc/planner/__init__.py b/grapharc/planner/__init__.py index fbd16e2..4431f4c 100644 --- a/grapharc/planner/__init__.py +++ b/grapharc/planner/__init__.py @@ -50,7 +50,9 @@ CostEstimate, EdgePolicy, EdgeRule, + NodePolicy, NodeRegistry, + NodeRule, NodeSpec, Rejection, RemainingBudget, @@ -98,7 +100,9 @@ "MaterializationError", "Materializer", "NodeBuild", + "NodePolicy", "NodeRegistry", + "NodeRule", "NodeSpec", "NotAdmitted", "Planner", diff --git a/grapharc/planner/admission.py b/grapharc/planner/admission.py index e337908..c122471 100644 --- a/grapharc/planner/admission.py +++ b/grapharc/planner/admission.py @@ -12,11 +12,14 @@ | Check | Question | Authority | |---|---|---| | REGISTRY | is every node's `kind` allowed, and does every edge endpoint exist? | `NodeRegistry` | -| POLICY | is every edge permitted between the kinds it joins? | `EdgePolicy`, deny → ask → allow | +| POLICY | may each node's `kind` run, and each edge be taken? | `NodePolicy`, `EdgePolicy` | | BUDGET | does the worst case fit what is *left*? | `RemainingBudget` | | DEPTH | is the nesting within the limit? | `AdmissionLimits.max_depth` | | ACYCLICITY | is the topology acyclic where that is required? | `AdmissionLimits` | +Both policy objects tier deny → ask → allow, and a checker given no +`NodePolicy` decides node kinds on registry membership alone, as it always did. + Four properties worth stating precisely, because each is a place where a gate usually leaks: @@ -305,6 +308,60 @@ def decide(self, source_kind: str, target_kind: str) -> Decision: return self.default +class NodeRule(BaseModel): + """One deny/ask/allow rule over a node kind, matched by fnmatch. + + `match` is a pattern over a **registry kind**, never over the instance name + a planner chose — the same rule the rest of this module follows, so a denied + kind cannot be renamed out of its denial. + + `reason` is the operator's own words, carried from the policy document that + compiled to this rule so a refusal can quote why rather than only what. + """ + + model_config = ConfigDict(frozen=True) + + action: Decision + match: str = "*" + reason: str = "" + + +class NodePolicy(BaseModel): + """Which node kinds may be proposed at all: deny → ask → allow, tiered. + + The counterpart of `EdgePolicy` for nodes, and deliberately a *second* gate + rather than a replacement for `NodeRegistry`: the registry is the operator's + allowlist of kinds that exist and what each costs, while this answers "may + this kind run here" from a document that can be edited without touching the + code that builds the registry. A kind has to pass both. + + Same semantics as `EdgePolicy` and `PermissionPolicy` — every `deny` is + tried before every `ask` and every `ask` before every `allow`, first match + within a tier wins, and an unmatched kind gets `default`. `default` is + `deny`, so an empty `NodePolicy()` admits nothing; that is the same shape + `EdgePolicy` has and the same reason: a policy object that governs nothing + should have to be written down on purpose. + """ + + model_config = ConfigDict(frozen=True) + + rules: tuple[NodeRule, ...] = () + default: Decision = Decision.DENY + + def rule_for(self, kind: str) -> NodeRule | None: + """The rule that decides `kind`, or None when the default applies.""" + for tier in (Decision.DENY, Decision.ASK, Decision.ALLOW): + for rule in self.rules: + if rule.action == tier and fnmatch(kind, rule.match): + return rule + return None + + def decide(self, kind: str) -> Decision: + """Decide one node kind. The argument is a kind, never an instance name.""" + rule = self.rule_for(kind) + return self.default if rule is None else rule.action + + class AdmissionLimits(BaseModel): """Structural limits, set by the operator and not by the proposal.""" @@ -391,6 +448,7 @@ def __init__( *, registry: NodeRegistry, edge_policy: EdgePolicy, + node_policy: NodePolicy | None = None, limits: AdmissionLimits | None = None, known_nodes: Iterable[str] | Mapping[str, str] = (), trace: TraceRecorder | None = None, @@ -398,6 +456,12 @@ def __init__( ) -> None: self.registry = registry self.edge_policy = edge_policy + # `None` is not "allow everything": it means no *document* governs node + # kinds here, and the registry — an allowlist with no wildcard — remains + # the only node gate, which is where it was before this check existed. + # A `NodePolicy` supplied on purpose is consulted for every proposed + # node, and an empty one denies every kind. + self.node_policy = node_policy self.limits = limits or AdmissionLimits() # Nodes already live in the graph this proposal attaches to. Edges may # reference them; proposed nodes may not reuse their names. @@ -439,6 +503,7 @@ def check( rejections: list[Rejection] = [] rejections.extend(self._check_registry(proposal)) + rejections.extend(self._check_node_policy(proposal)) rejections.extend(self._check_policy(proposal)) worst_case, complete = self._worst_case(proposal) rejections.extend(self._check_budget(worst_case, complete, remaining)) @@ -538,6 +603,51 @@ def _check_endpoints( ) return out + def _check_node_policy(self, proposal: Subgraph) -> list[Rejection]: + """Decide every proposed node on its *kind*, when a node policy exists. + + Part of the POLICY check rather than a gate of its own: REGISTRY answers + "does this kind exist and what does it cost", POLICY answers "is it + permitted", and a planner replanning from feedback should read one + answer to the second question whether the refusal was about a node or an + edge. With no node policy configured this decides nothing — the registry + is then the only node gate, as it was before. + """ + if self.node_policy is None: + return [] + out: list[Rejection] = [] + for path, _depth, sub in proposal.scopes(): + for node in sub.nodes: + rule = self.node_policy.rule_for(node.kind) + decision = self.node_policy.default if rule is None else rule.action + if decision is Decision.ALLOW: + continue + denied = decision is Decision.DENY + verb = "denies" if denied else "requires approval for" + # The operator's own words, when the rule carried any. A rule + # without a `reason` still refuses; the detail then names the + # kind and nothing more, which is the whole of what was decided. + because = f": {rule.reason}" if rule is not None and rule.reason else "" + out.append( + Rejection( + check=Check.POLICY, + code="node_denied" if denied else "node_needs_approval", + subject=_scoped(path, node.name), + detail=( + f"the node policy {verb} this kind: " + f"{_describe(node.name, node.kind)}{because}" + ), + remedy=( + "the decision is made on the registry kind, not the name you " + "chose: renaming the node will not change it — propose a " + "permitted kind" + if denied + else "obtain approval and re-submit" + ), + ) + ) + return out + def _check_policy(self, proposal: Subgraph) -> list[Rejection]: """Decide every edge on the *kinds* of its endpoints. @@ -806,7 +916,7 @@ def _emit( def _status_for(rejections: tuple[Rejection, ...]) -> AdmissionStatus: if not rejections: return AdmissionStatus.ADMITTED - if all(r.code == "edge_needs_approval" for r in rejections): + if all(r.code in ("edge_needs_approval", "node_needs_approval") for r in rejections): return AdmissionStatus.NEEDS_APPROVAL return AdmissionStatus.REJECTED @@ -891,7 +1001,9 @@ def _find_cycle(names: frozenset[str], edges: tuple[ProposedEdge, ...]) -> list[ "CostEstimate", "EdgePolicy", "EdgeRule", + "NodePolicy", "NodeRegistry", + "NodeRule", "NodeSpec", "Rejection", "RemainingBudget", diff --git a/grapharc/policy/__init__.py b/grapharc/policy/__init__.py index 58c0b2e..9b1ed2e 100644 --- a/grapharc/policy/__init__.py +++ b/grapharc/policy/__init__.py @@ -17,6 +17,12 @@ `Harness` already obeys, and `engine.approval_router(handlers, tenant=...)` produces the approval callback that goes with it. +For the planner plane the pair is `engine.node_policy(tenant=...)` and +`engine.edge_policy(tenant=...)`, which produce the two objects +`grapharc.planner.admission.AdmissionChecker` consults — so the document +decides which kinds may run and which transitions may be wired, rather than +only answering questions about them. + A shipped, commented example document lives at `grapharc/policy/example.toml`. """ diff --git a/grapharc/policy/engine.py b/grapharc/policy/engine.py index f9ec3fb..8a9ffe4 100644 --- a/grapharc/policy/engine.py +++ b/grapharc/policy/engine.py @@ -22,6 +22,12 @@ That compiled object is also the one unrecorded path, deliberately: a `Harness` holding it decides without consulting the engine, and so writes no audit record. Pair it with `approval_router()`, which comes back through here. + +`node_policy()` and `edge_policy()` do the same job for the *planner* plane, +compiling this document's `node` and `edge` rules into the two objects +`grapharc.planner.admission.AdmissionChecker` consults. Both halves are +compiled: a document's node rules used to reach nothing, so a `deny` rule over +a node kind was text rather than a rule (issue #66). """ from __future__ import annotations @@ -207,6 +213,41 @@ def permission_policy(self, *, tenant: str = DEFAULT_TENANT) -> PermissionPolicy ] return PermissionPolicy(rules=rules, default=self._document.default) + def node_policy(self, *, tenant: str = DEFAULT_TENANT) -> Any: + """Compile this document's `node` rules for `tenant` into a `NodePolicy`. + + The other half of what `edge_policy()` started. A document's `node` + rules used to reach nothing at all: `AdmissionChecker` gated kinds on + registry membership alone, so `no-shell-nodes` in a policy file was + text, not a rule (issue #66). This is the object the checker consults, + and it answers exactly as `check_node` does for the same tenant — a test + pins the two together across a kind × tenant matrix. + + Same two losses as the other compiled objects: no approver role and no + audit record, because `NodePolicy.decide` returns a bare `Decision`. + Admission treats `ASK` as not-yet-permitted. + + **A document with no `node` rules compiles to a policy that denies every + kind**, because an unmatched subject gets the document default and that + default is usually `deny` — the same answer `check_node` gives. That is + faithful rather than convenient, which is why the CLI applies this only + to a document that declares at least one `node` rule: saying nothing + about nodes is not the same statement as denying all of them, and a + caller compiling this by hand should decide which it meant. + """ + from grapharc.planner.admission import NodePolicy, NodeRule + + if not self._document.declares_tenant(tenant): + # `check_node` denies an undeclared tenant outright; the compiled + # policy has to agree, or the two answers diverge. + return NodePolicy(rules=(), default=Decision.DENY) + rules = [ + NodeRule(action=rule.effect, match=rule.match, reason=rule.reason) + for rule in self._by_resource[ResourceKind.NODE] + if rule.matches_tenant(tenant) + ] + return NodePolicy(rules=tuple(rules), default=self._document.default) + def edge_policy(self, *, tenant: str = DEFAULT_TENANT) -> Any: """Compile this document's `edge` rules for `tenant` into an `EdgePolicy`. diff --git a/grapharc/policy/example.toml b/grapharc/policy/example.toml index 6c6eae7..0b5750b 100644 --- a/grapharc/policy/example.toml +++ b/grapharc/policy/example.toml @@ -51,6 +51,10 @@ reason = "acme's contract covers writes" # ---- nodes ------------------------------------------------------------------ +# A node match is an fnmatch over a registry *kind*, never over the instance +# name a planner chose. These rules compile to the `NodePolicy` the admission +# checker consults, so a denied kind is refused before it can be proposed into a +# plan — renaming the instance does not change the answer. [[rule]] id = "no-shell-nodes" diff --git a/grapharc/stdlib.py b/grapharc/stdlib.py index 414bf7e..ef4967e 100644 --- a/grapharc/stdlib.py +++ b/grapharc/stdlib.py @@ -382,6 +382,7 @@ def build_loop( model: Any, *, edge_policy: Any = None, + node_policy: Any = None, trace: Any = None, budget: Any = None, limits: Any = None, @@ -413,6 +414,9 @@ def build_loop( checker=AdmissionChecker( registry=registry, edge_policy=edge_policy or default_edge_policy(), + # None unless a policy document declared node rules; the registry is + # otherwise the only thing deciding which kinds may run. + node_policy=node_policy, trace=trace, # Rounds are materialized standalone, so "can this actually run" # is part of admission here: a plan with no entry, or with nodes diff --git a/tests/test_admission.py b/tests/test_admission.py index 7826f0a..0cf6be6 100644 --- a/tests/test_admission.py +++ b/tests/test_admission.py @@ -25,7 +25,9 @@ CostEstimate, EdgePolicy, EdgeRule, + NodePolicy, NodeRegistry, + NodeRule, NodeSpec, PlannerNode, ProposedEdge, @@ -253,6 +255,151 @@ def test_a_denial_outranks_a_pending_approval(): assert result.status is AdmissionStatus.REJECTED +# -- POLICY over node kinds ---------------------------------------------------- +# The registry says a kind exists and what it costs. A `NodePolicy` — compiled +# from a document an operator can edit without touching the code that builds the +# registry — says whether it may run here. A kind has to pass both, and a +# proposal is refused with a code the planner can replan against. + + +def test_a_policy_forbidden_node_kind_is_rejected_naming_the_policy_check(): + policy = NodePolicy( + rules=(NodeRule(action="deny", match="deploy"), NodeRule(action="allow")) + ) + result = checker(registry("build", "deploy"), node_policy=policy).check( + linear("build", "deploy") + ) + + assert not result.admitted + assert result.failed_checks() == (Check.POLICY,) + (reason,) = result.reasons(Check.POLICY) + assert reason.code == "node_denied" + assert reason.subject == "deploy" + + +def test_a_denied_node_kind_cannot_be_renamed_out_of_its_denial(): + """The same rule the edge half follows: a name grants and borrows nothing.""" + policy = NodePolicy( + rules=(NodeRule(action="deny", match="deploy"), NodeRule(action="allow")) + ) + gate = checker(registry("build", "deploy"), node_policy=policy) + + renamed = gate.check( + Subgraph(nodes=(ProposedNode(name="totally_fine", kind="deploy"),)) + ) + borrowed = gate.check( + Subgraph(nodes=(ProposedNode(name="deploy", kind="build"),)) + ) + + assert [r.code for r in renamed.rejections] == ["node_denied"] + assert "kind 'deploy'" in renamed.rejections[0].detail + assert "totally_fine" in renamed.rejections[0].detail + assert borrowed.admitted + + +def test_a_node_denial_quotes_the_reason_the_operator_wrote(): + policy = NodePolicy( + rules=( + NodeRule(action="deny", match="shell_*", reason="a shell node is unbounded"), + NodeRule(action="allow"), + ) + ) + result = checker(registry("shell_worker"), node_policy=policy).check( + Subgraph(nodes=(ProposedNode(name="shell_worker"),)) + ) + + assert result.rejections[0].detail.endswith("a shell node is unbounded") + + +def test_a_node_kind_needing_approval_is_not_admitted(): + policy = NodePolicy( + rules=(NodeRule(action="ask", match="deploy"), NodeRule(action="allow")) + ) + result = checker(registry("build", "deploy"), node_policy=policy).check( + linear("build", "deploy") + ) + + assert not result.admitted + assert result.needs_approval + assert result.status is AdmissionStatus.NEEDS_APPROVAL + assert [r.code for r in result.reasons(Check.POLICY)] == ["node_needs_approval"] + + +def test_an_unmatched_node_kind_defaults_to_deny(): + """`NodePolicy()` is empty, not permissive — the same shape `EdgePolicy` has.""" + result = checker(registry("fetch"), node_policy=NodePolicy()).check( + Subgraph(nodes=(ProposedNode(name="fetch"),)) + ) + + assert [r.code for r in result.rejections] == ["node_denied"] + + +def test_a_broad_node_deny_beats_a_narrower_allow(): + policy = NodePolicy( + rules=( + NodeRule(action="allow", match="deploy"), + NodeRule(action="deny", match="dep*"), + ) + ) + assert policy.decide("deploy") is Decision.DENY + + +def test_no_node_policy_leaves_the_registry_as_the_only_node_gate(): + """The behaviour every existing caller has: absence is not a wildcard. + + A checker built without a node policy decides node kinds exactly as it did + before one existed — registered kinds pass, unregistered ones are refused by + REGISTRY. + """ + gate = checker(registry("fetch")) + + assert gate.check(Subgraph(nodes=(ProposedNode(name="fetch"),))).admitted + refused = gate.check(Subgraph(nodes=(ProposedNode(name="shell"),))) + assert [r.code for r in refused.rejections] == ["unregistered_node"] + + +def test_a_denied_node_kind_hidden_in_a_nested_scope_is_refused_too(): + policy = NodePolicy( + rules=(NodeRule(action="deny", match="deploy"), NodeRule(action="allow")) + ) + nested = Subgraph( + nodes=( + ProposedNode( + name="outer", + kind="build", + subgraph=Subgraph(nodes=(ProposedNode(name="inner", kind="deploy"),)), + ), + ) + ) + result = checker( + registry("build", "deploy"), + node_policy=policy, + limits=AdmissionLimits(max_depth=2), + ).check(nested) + + assert [r.code for r in result.rejections] == ["node_denied"] + assert result.rejections[0].subject == "outer/inner" + + +def test_a_node_denial_and_an_edge_denial_are_both_reported(): + """One POLICY check, both halves — a planner gets the whole list.""" + result = checker( + registry("build", "deploy"), + edge_policy=EdgePolicy(rules=(EdgeRule(action="deny", target="deploy"),)), + node_policy=NodePolicy( + rules=(NodeRule(action="deny", match="deploy"), NodeRule(action="allow")) + ), + ).check( + Subgraph( + nodes=(ProposedNode(name="build"), ProposedNode(name="deploy")), + edges=(ProposedEdge(source="build", target="deploy"),), + ) + ) + + assert {r.code for r in result.rejections} == {"node_denied", "edge_denied"} + assert result.failed_checks() == (Check.POLICY,) + + # -- POLICY decides on the KIND, never on the name the planner chose ----------- # # The gate is only a gate if what it matches is a fact about the node. `name` is diff --git a/tests/test_cli.py b/tests/test_cli.py index 7da7f97..587634f 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1086,6 +1086,32 @@ def test_the_durable_store_survives_a_real_process_boundary(tmp_path): _PERMISSIVE = 'version = "1"\ndefault = "allow"\n' +# Issue #66's document, plus the catch-all its author's `default = "deny"` +# needs. Its `node` rules used to be compiled by nobody: the run admitted the +# denied kind and executed it, and the only hint was a rule count in the banner. +_DENY_DEPLOY_NODE = """version = "1" +default = "deny" + +[[rule]] +id = "no-deploy-node" +resource = "node" +match = "deploy" +effect = "deny" +reason = "deploying from a plan is never permitted" + +[[rule]] +id = "other-nodes-run" +resource = "node" +match = "*" +effect = "allow" + +[[rule]] +id = "ordinary-work-flows" +resource = "edge" +match = "*->*" +effect = "allow" +""" + def test_plan_runs_the_governed_loop_and_reports_every_round(tmp_path, capsys): code, out, _ = call( @@ -1170,6 +1196,48 @@ def test_a_policy_document_is_what_refuses_the_transition(tmp_path, capsys): assert str(doc) in payload["policy"] +def test_a_document_that_denies_a_node_kind_stops_it_running(tmp_path, capsys): + """Issue #66, end to end: a `resource = "node"` deny rule is enforced. + + The shipped script proposes `deploy` in round 1. Before the fix the whole + node half of the document was discarded and `deploy ran` landed in the + state; now the kind is refused with the operator's own reason and the + planner replans around it. + """ + doc = tmp_path / "nodepolicy.toml" + doc.write_text(_DENY_DEPLOY_NODE, encoding="utf-8") + + code, payload, _ = call_json( + ["plan", "fix the outage", "--policy", str(doc), + "--trace", str(tmp_path / "t.jsonl")], + capsys, + ) + + assert code == 0 + assert payload["rejections"] == ["node_denied"] + assert payload["rounds"][0]["status"] == "rejected" + assert payload["rounds"][0]["executed"] is False + assert "deploy ran" not in payload["state"]["notes"] + assert payload["state"]["notes"] == ["triage ran", "patch ran", "verify ran"] + # The banner counts both halves of the document, so a reader can see that + # the node rules were read rather than skimmed past. + assert "1 edge rule(s), 2 node rule(s)" in payload["policy"] + + +def test_a_node_denial_is_traced_with_the_reason_the_document_gave(tmp_path, capsys): + from grapharc.observe.trace import TraceRecorder + + doc = tmp_path / "nodepolicy.toml" + doc.write_text(_DENY_DEPLOY_NODE, encoding="utf-8") + path = tmp_path / "t.jsonl" + + call(["plan", "fix the outage", "--policy", str(doc), "--trace", str(path)], capsys) + admissions = [e for e in TraceRecorder(path).read_events() if e.phase == "admission"] + + assert admissions, "the gate's decision has to be on the record" + assert "policy/node_denied" in (admissions[0].error or "") + + def test_a_permissive_document_admits_what_the_strict_one_refused(tmp_path, capsys): """The control for the test above: without the rule, round 1 runs.""" doc = tmp_path / "permissive.toml" @@ -1253,10 +1321,31 @@ def test_the_shipped_command_is_what_finally_imports_the_policy_package(tmp_path doc = tmp_path / "policy.toml" doc.write_text(_DENY_DEPLOY, encoding="utf-8") - policy, description = plan_module.resolve_edge_policy(doc, tenant="default") + policy, description = plan_module.resolve_policy(doc, tenant="default") - assert policy.rules, "the document's edge rules must reach the admission gate" + assert policy.edge.rules, "the document's edge rules must reach the admission gate" assert "tenant 'default'" in description + # This document declares no node rules, so nothing governs kinds but the + # registry. Compiling one anyway would read "said nothing about nodes" as + # "denied every node" and refuse the whole run. + assert policy.node is None + + +def test_a_documents_node_rules_are_compiled_alongside_its_edge_rules(tmp_path, capsys): + """The half of the compile that did not exist before issue #66.""" + import grapharc.cli.plan as plan_module + + doc = tmp_path / "policy.toml" + doc.write_text(_DENY_DEPLOY_NODE, encoding="utf-8") + + from grapharc.harness.permissions import Decision + + policy, description = plan_module.resolve_policy(doc, tenant="default") + + assert policy.node is not None + assert policy.node.decide("deploy") is Decision.DENY + assert policy.node.decide("triage") is Decision.ALLOW + assert "2 node rule(s)" in description # -------------------------------------------------------------------------- diff --git a/tests/test_cookbook_governance.py b/tests/test_cookbook_governance.py index ff61f5f..7b89b09 100644 --- a/tests/test_cookbook_governance.py +++ b/tests/test_cookbook_governance.py @@ -322,6 +322,38 @@ def test_the_compiled_edge_policy_answers_as_the_engine_does(tenant, workdir): ).effect, f"{source}->{target} for tenant {tenant!r}" +NODE_MATRIX = ["shell_exec", "shell_", "summarise", "deploy", "unheard_of"] + + +@pytest.mark.parametrize("tenant", ["default", "acme", "stranger"]) +def test_the_shipped_node_compiler_answers_as_the_engine_does(tenant, workdir): + """The section's node recipe uses `node_policy()`; it must not diverge.""" + engine = PolicyEngine.from_file("policy.toml") + compiled = engine.node_policy(tenant=tenant) + for kind in NODE_MATRIX: + assert compiled.decide(kind) is engine.check_node(kind, tenant=tenant).effect, ( + f"{kind!r} for tenant {tenant!r}" + ) + + +def test_the_documents_node_rule_is_what_refuses_the_kind(workdir): + """The claim the node recipe makes, pinned apart from its printed output.""" + engine = PolicyEngine.from_file("policy.toml") + gate = AdmissionChecker( + registry=NodeRegistry([NodeSpec(name="shell_exec"), NodeSpec(name="summarise")]), + edge_policy=engine.edge_policy(), + node_policy=engine.node_policy(), + ) + + refused = gate.check(Subgraph(nodes=(ProposedNode(name="helper", kind="shell_exec"),))) + admitted = gate.check(Subgraph(nodes=(ProposedNode(name="summarise"),))) + + assert [r.code for r in refused.rejections] == ["node_denied"] + # The rule's own reason travels into the rejection, not just its effect. + assert "a shell node is an unbounded tool" in refused.rejections[0].detail + assert admitted.admitted + + def test_an_undeclared_tenant_compiles_to_a_policy_that_denies_everything(workdir): namespace = _bridge_namespace(workdir) engine = PolicyEngine.from_file("policy.toml") diff --git a/tests/test_generate.py b/tests/test_generate.py index 99e8a08..b4f2610 100644 --- a/tests/test_generate.py +++ b/tests/test_generate.py @@ -67,7 +67,7 @@ def test_a_first_run_with_nothing_specified_generates_a_policy(tmp_path): assert source == "generated" assert "review it" in description - assert policy.rules, "a generated policy with no rules would gate nothing" + assert policy.edge.rules, "a generated policy with no rules would gate nothing" def test_the_generated_policy_actually_governs(tmp_path): @@ -76,8 +76,8 @@ def test_the_generated_policy_actually_governs(tmp_path): policy, _, _ = _generate(tmp_path) - assert policy.decide("investigate", "apply_change") is Decision.DENY - assert policy.decide("investigate", "verify") is Decision.ALLOW + assert policy.edge.decide("investigate", "apply_change") is Decision.DENY + assert policy.edge.decide("investigate", "verify") is Decision.ALLOW # -- disclosure -------------------------------------------------------------- @@ -149,6 +149,33 @@ def test_the_second_run_reads_it_off_disk_rather_than_regenerating(tmp_path): assert "previously generated" in description +def test_node_rules_the_operator_added_to_the_cached_file_are_honoured(tmp_path): + """The cached file is an ordinary document, and editing it is the point. + + Reading it back through the same compiler as `--policy` is what stops the + generated path being a place where `resource = "node"` rules go to die + (issue #66). + """ + from grapharc.harness.permissions import Decision + + _generate(tmp_path) + path = generated_policy_path(tmp_path) + path.write_text( + path.read_text(encoding="utf-8") + + '\n[[rule]]\nid = "no-shell-nodes"\nresource = "node"\n' + 'match = "shell_*"\neffect = "deny"\n' + '\n[[rule]]\nid = "others"\nresource = "node"\nmatch = "*"\neffect = "allow"\n', + encoding="utf-8", + ) + + policy, _description, source = _generate(tmp_path) + + assert source == "generated-cached" + assert policy.node is not None + assert policy.node.decide("shell_exec") is Decision.DENY + assert policy.node.decide("investigate") is Decision.ALLOW + + def test_a_cached_policy_beats_generating_a_new_one_even_with_a_model(tmp_path): """Otherwise the policy changes under the operator on every run.""" _generate(tmp_path) @@ -191,7 +218,7 @@ def test_with_no_model_nothing_is_generated(tmp_path): assert source == "builtin-default" assert not generated_policy_path(tmp_path).exists() - assert policy.rules + assert policy.edge.rules # The description says what the policy *does*, read back from its rules, so # it cannot drift from them. assert "deny -> apply_change" in description @@ -214,7 +241,7 @@ def test_a_registrys_own_default_beats_the_builtin(tmp_path): assert source == "registry-default" assert description.startswith("myco:build default") assert "deny -> apply_change" in description - assert policy is sentinel + assert policy.edge is sentinel # -- what goes into the prompt ----------------------------------------------- diff --git a/tests/test_policy_engine.py b/tests/test_policy_engine.py index 6a51f46..3b6a3ea 100644 --- a/tests/test_policy_engine.py +++ b/tests/test_policy_engine.py @@ -210,6 +210,30 @@ def test_shipped_example_document_loads_and_decides(): assert engine.check_node("shell_worker", tenant="acme").denied +def test_the_shipped_examples_node_rules_reach_the_admission_gate(): + """`no-shell-nodes` is the document's canonical "what may run" rule. + + It used to be inert — the compiler dropped every `node` rule (issue #66), so + an operator who copied the shipped example got a policy that denied nothing. + """ + from grapharc.planner import AdmissionChecker, NodeRegistry, NodeSpec, Subgraph + from grapharc.planner.proposal import ProposedNode + + engine = PolicyEngine.from_file(EXAMPLE_DOCUMENT) + gate = AdmissionChecker( + registry=NodeRegistry([NodeSpec(name="shell_exec"), NodeSpec(name="summarise")]), + edge_policy=engine.edge_policy(tenant="acme"), + node_policy=engine.node_policy(tenant="acme"), + ) + + refused = gate.check(Subgraph(nodes=(ProposedNode(name="helper", kind="shell_exec"),))) + admitted = gate.check(Subgraph(nodes=(ProposedNode(name="summarise"),))) + + assert [r.code for r in refused.rejections] == ["node_denied"] + assert "a shell node is an unbounded tool" in refused.rejections[0].detail + assert admitted.admitted + + def test_a_loaded_document_cannot_be_edited_underneath_a_decision(): """The digest describes the document; the document must not move after it.""" doc = parse_document(POLICY_TOML) @@ -865,6 +889,85 @@ def test_tool_rules_do_not_leak_into_the_edge_policy(): assert policy.decide("a", "b") is Decision.DENY +# -------------------------------------------------------------------------- +# Issue #66 — `node_policy()`. `check_node` was correct and had no runtime +# caller: a document's `node` rules were compiled by nobody, so `no-shell-nodes` +# in a policy file governed nothing and a plan proposing a denied kind ran it. +# Same shape as the `edge_policy()` tests above: the compiled object has to +# agree with the engine, and the document has to be what refuses the node. +# -------------------------------------------------------------------------- + +_NODE_MATRIX = ["shell_worker", "shell_", "summarise", "deploy_prod", "unheard_of"] + + +@pytest.mark.parametrize("tenant", ["default", "acme", "globex", "not-a-customer"]) +def test_the_compiled_node_policy_agrees_with_the_engine(engine, tenant): + """Same tenant, same answer, for every kind in the matrix.""" + policy = engine.node_policy(tenant=tenant) + for kind in _NODE_MATRIX: + assert policy.decide(kind) is engine.check_node(kind, tenant=tenant).effect, ( + f"{kind!r} for {tenant!r}" + ) + + +def test_an_undeclared_tenant_compiles_to_a_node_policy_that_permits_nothing(engine): + policy = engine.node_policy(tenant="not-a-customer") + + assert policy.rules == () + assert policy.default is Decision.DENY + assert policy.decide("anything") is Decision.DENY + + +def test_the_compiled_node_policy_carries_the_documents_reason(engine): + """The words an operator wrote have to survive the compile, or a refusal + can only say what happened and never why.""" + rule = engine.node_policy().rule_for("shell_worker") + + assert rule is not None + assert rule.action is Decision.DENY + assert engine.check_node("shell_worker").rule_id == "no-shell" + + +def test_a_document_with_no_node_rules_compiles_to_its_own_default(): + """Faithful to `check_node`, which is the whole point of the compile.""" + engine = PolicyEngine.from_toml( + 'version = "1"\ndefault = "deny"\n' + '[[rule]]\nid = "e"\nresource = "edge"\nmatch = "*->*"\neffect = "allow"\n' + ) + policy = engine.node_policy() + + assert policy.rules == () + assert policy.decide("triage") is engine.check_node("triage").effect is Decision.DENY + + +def test_the_document_stops_a_planner_running_a_denied_node_kind(): + """End to end, and the exact shape of issue #66: the TOML file refuses it.""" + from grapharc.planner import AdmissionChecker, NodeRegistry, NodeSpec, Subgraph + from grapharc.planner.proposal import ProposedNode + + engine = PolicyEngine.from_toml( + 'version = "1"\ndefault = "deny"\n' + '[[rule]]\nid = "no-deploy-node"\nresource = "node"\nmatch = "deploy"\n' + 'effect = "deny"\nreason = "deploying from a plan is never permitted"\n' + '[[rule]]\nid = "others"\nresource = "node"\nmatch = "*"\neffect = "allow"\n' + '[[rule]]\nid = "edges"\nresource = "edge"\nmatch = "*->*"\neffect = "allow"\n' + ) + gate = AdmissionChecker( + registry=NodeRegistry([NodeSpec(name="triage"), NodeSpec(name="deploy")]), + edge_policy=engine.edge_policy(), + node_policy=engine.node_policy(), + ) + + # Renamed instance, denied kind — the rule is about what a node *is*. + refused = gate.check(Subgraph(nodes=(ProposedNode(name="ship", kind="deploy"),))) + admitted = gate.check(Subgraph(nodes=(ProposedNode(name="triage"),))) + + assert not refused.admitted + assert [r.code for r in refused.rejections] == ["node_denied"] + assert "deploying from a plan is never permitted" in refused.rejections[0].detail + assert admitted.admitted + + def test_the_document_stops_a_planner_wiring_a_denied_transition(): """End to end: the TOML file, not Python, is what refuses the edge.""" from grapharc.planner import (