From 836b9ebc1e492352af2d12be2aa7462c52b7f388 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 12:52:58 +0400 Subject: [PATCH 01/20] fix(gate): a /gate body with no verdict must never read as "allowed" `check_workflow_budget` read the verdict as decision = response.get("decision", "allow") The default is not merely redundant, it is a hole. `decision` is a non-Option field with no `skip_serializing_if` on the backend's GateResponse (gate/internal.rs:637), so every real NULLRUN backend serialises it on every answer. A body without one did not come from NULLRUN -- a proxy error page, a captive portal, a TLS interception box -- so the default authorised calls that no policy engine had evaluated. ADR-008 grants fail-OPEN to *transport* failures, where the gate never got to rule. A body that arrived but carried no verdict is the opposite case, and the SDK read it as the former. It also hid a reserved refusal. `GateDecision::Deny` (gate/internal.rs:579, reserved by ADR-046, no producer yet) had no arm in the method, so it fell off the end -- which the caller reads as "no block raised, proceed". The one refusal on the wire contract that executed. Deny now raises, so the day ADR-046 ships its producer the SDK already fails CLOSED. The non-JSON body was a second path to the same place. `Transport.check` ends in `response.json()`, and the resulting JSONDecodeError (a ValueError) matched neither the auth arm nor the fail-OPEN arm in the cached branch, while the uncached branch's `except Exception` swallowed it as "gate unavailable" -- a non-NULLRUN responder read as an outage. Both branches now convert it to a typed error, placed before the broad arm it must precede. New: `NullRunMalformedGateResponseError(NullRunProtocolError)`. A subclass rather than a new root so existing `except NullRunProtocolError` handlers keep catching it, and it shares the NR-P* family without overloading NR-P001, whose user_action is specifically "upgrade the SDK". Its own code is NR-P002. Verified by mutation, not by assertion. Reverting the extraction to the old default fails 5 of 11; disabling the deny arm fails test_deny_decision_raises; removing the ValueError arm fails test_non_json_body_raises_typed_error with the raw JSONDecodeError. The first mutation pass was incomplete -- it reverted only the cached branch, and the test passed, because the uncached branch still had its own arm. Reverted both, and the test bit. Suite: 1493 passed, 1 skipped, 0 failed. --- src/nullrun/breaker/exceptions.py | 35 +++++ src/nullrun/runtime.py | 128 +++++++++++++++++- tests/test_gate_malformed_decision.py | 188 ++++++++++++++++++++++++++ 3 files changed, 350 insertions(+), 1 deletion(-) create mode 100644 tests/test_gate_malformed_decision.py diff --git a/src/nullrun/breaker/exceptions.py b/src/nullrun/breaker/exceptions.py index 392158a..a5d72a6 100644 --- a/src/nullrun/breaker/exceptions.py +++ b/src/nullrun/breaker/exceptions.py @@ -385,6 +385,41 @@ class NullRunProtocolError(NullRunInfrastructureError): retryable = False +class NullRunMalformedGateResponseError(NullRunProtocolError): + """The ``/gate`` response is not a decision the SDK can act on. + + Raised when the body is not a JSON object, or when it is an object + whose ``decision`` field is absent, not a string, or names a value + outside the known decision set. + + This is a subclass of :class:`NullRunProtocolError` rather than a + new root so that existing ``except NullRunProtocolError`` handlers + keep catching it, and so it shares the NR-P* error-code family + (wire-contract violations) without overloading NR-P001, whose + ``user_action`` is specifically "upgrade the SDK". + + Why it raises instead of defaulting to ``allow``: ``decision`` is + a non-optional, non-``skip_serializing_if`` field on the backend's + ``GateResponse`` (``gate/internal.rs:637``), so every real + NULLRUN backend sends it on every answer. A body without it did + not come from NULLRUN — a proxy error page, a captive portal, an + expired TLS interception box. Reading that as "allowed" is the + fail-OPEN that ADR-008's table assigns only to *transport* + failures, and it is the one case where a non-NULLRUN responder can + authorise a call no policy engine ever saw. + """ + + error_code = "NR-P002" + user_action = ( + "The NullRun API returned a response the SDK could not read as " + "a decision. This usually means a proxy, VPN, or corporate TLS " + "box intercepted the connection and returned its own body " + "instead of the gate's JSON. Check that the API host is " + "reachable directly, then retry." + ) + retryable = True + + class NullRunChainError(NullRunDecision): """Chain-related failure. diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index 6023a1d..0120e71 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -1870,6 +1870,85 @@ def check_control_plane(self, workflow_id: str) -> None: kill_source="remote_state", ) + #: Every decision value the SDK knows how to act on. The backend's + #: `GateDecision` (`gate/internal.rs:574`) supplies allow / block / + #: require_approval / soft_pass / deny; ``throttle`` is an + #: SDK-side shape that maps to `WorkflowPausedException`. Anything + #: outside this set is a wire contract the SDK does not implement, + #: and guessing "allow" for it is the fail-OPEN ADR-008 assigns + #: only to transport failures. + _KNOWN_GATE_DECISIONS = frozenset( + {"allow", "block", "throttle", "soft_pass", "require_approval", "deny"} + ) + + def _require_gate_decision(self, response: Any) -> str: + """Extract ``decision`` from a ``/gate`` body, or raise. + + Pre-fix this was ``response.get("decision", "allow")``. That + default converted three distinct failures into "allowed": + + * a body that is not a JSON object at all (``.get`` on a list + raised AttributeError *outside* the try, so it surfaced as an + untyped crash rather than a decision); + * an object from a non-NULLRUN responder — proxy error page, + captive portal, TLS interception box; + * a real NULLRUN response missing the field, which cannot + happen: ``decision`` is a non-``Option`` field with no + ``skip_serializing_if`` (``gate/internal.rs:637``), so every + real backend serialises it on every answer. + + The last point is what makes the default unsafe rather than + merely redundant. ADR-008 grants fail-OPEN to *transport* + failures, where the gate never got to rule. A body without a + decision is the opposite case: something answered, and what it + said was not a verdict. Reading that as "allowed" lets a + non-NULLRUN responder authorise a call no policy engine + evaluated. + """ + from nullrun.breaker.exceptions import NullRunMalformedGateResponseError + + if not isinstance(response, dict): + raise NullRunMalformedGateResponseError( + f"/gate returned {type(response).__name__}, expected a JSON " + f"object. Body was not a gate decision." + ) + + decision = response.get("decision") + if not isinstance(decision, str): + raise NullRunMalformedGateResponseError( + f"/gate response has no usable 'decision' field " + f"(got {type(decision).__name__})." + ) + if decision not in self._KNOWN_GATE_DECISIONS: + raise NullRunMalformedGateResponseError( + f"/gate returned unknown decision {decision!r}; this SDK " + f"implements {sorted(self._KNOWN_GATE_DECISIONS)}." + ) + return decision + + def _raise_malformed_gate_response(self, exc: BaseException) -> None: + """Raise ``NullRunMalformedGateResponseError`` for `exc`. + + Shared by the cached and uncached ``/gate`` call sites so the + two cannot drift into different behaviours — the same drift + that produced the duplicated ``AUTH_ERROR`` predicate fixed + under DEF-MP-TS12-ENF-01. Always raises; the ``-> None`` + return type is so call sites can use it as the tail of an + ``except`` arm without a bare ``raise``. + """ + from nullrun.breaker.exceptions import NullRunMalformedGateResponseError + + logger.error( + "check_workflow_budget: /gate returned a body that is not JSON " + "(%s). Treating as a malformed answer, not an outage — failing " + "CLOSED.", + exc, + ) + metrics.inc_runtime("gate_malformed_response_total") + raise NullRunMalformedGateResponseError( + f"/gate returned a non-JSON body: {exc}" + ) from exc + def check_workflow_budget(self) -> None: """ Pre-flight budget check via /api/v1/gate. Called from @protect @@ -2141,6 +2220,20 @@ def check_workflow_budget(self) -> None: logger.warning(f"check_workflow_budget: /gate unavailable, failing open: {exc}") metrics.inc_runtime("gate_fail_open_total") return + except ValueError as exc: + # `Transport.check` ends in `response.json()`. A + # body that is not JSON — proxy error page, + # captive portal, TLS interception box — raises + # JSONDecodeError, a ValueError. Pre-fix this + # matched neither arm above and escaped as an + # untyped crash; and had it matched the + # fail-OPEN arm, a non-NULLRUN responder would + # have been read as "gate unavailable, carry on", + # which is precisely the authorisation-without- + # policy hole the missing `decision` default + # shares. It is a malformed ANSWER, not an + # unreachable gate, so it raises. + self._raise_malformed_gate_response(exc) assert cache_key is not None # narrowed by cache_enabled above _GATE_CACHE[cache_key] = (time.monotonic(), response) else: @@ -2151,6 +2244,12 @@ def check_workflow_budget(self) -> None: # matters: this arm precedes the broad `except # Exception`, which is a superset. raise + except ValueError as exc: + # Same rationale as the cached branch: a body that is + # not JSON is a malformed answer, and must precede the + # broad `except Exception` below, which is a superset + # of this arm. + self._raise_malformed_gate_response(exc) except Exception as exc: # noqa: BLE001 logger.warning(f"check_workflow_budget: /gate unavailable, failing open: {exc}") metrics.inc_runtime("gate_fail_open_total") @@ -2158,7 +2257,7 @@ def check_workflow_budget(self) -> None: _capture_server_minted_execution_id(response) - decision = response.get("decision", "allow") + decision = self._require_gate_decision(response) decision_source = response.get("decision_source", DecisionSource.GATEWAY) # Only fail-OPEN on EXPLICIT synthetic responses. Real backend # decisions (decision_source="gateway") are honoured. @@ -2349,6 +2448,33 @@ def check_workflow_budget(self) -> None: local_timeout=True, ) + if decision == "deny": + # `Deny` is a live `GateDecision` variant + # (`gate/internal.rs:579`), reserved by ADR-046 — no + # production construction site emits it yet, but it is on + # the wire contract. Pre-fix it had no arm here and fell + # off the end of the method, which reads to the caller as + # "no block raised, proceed". A reserved refusal must not + # be the one refusal that executes. Raising it now means + # the day ADR-046 ships its producer, the SDK already + # fails-CLOSED instead of silently allowing. + reasons = response.get("explanations") or ( + [response["explanation"]] if response.get("explanation") else ["deny"] + ) + raise NullRunBlockedException( + workflow_id=workflow_id, + reason="; ".join(reasons), + tool_name=response.get("tool_name"), + error_code="NR-B006", + ) + + # `decision == "allow"` — the only `_KNOWN_GATE_DECISIONS` + # value left unhandled above. Stated explicitly rather than + # relying on fall-off-the-end, so that adding a variant to the + # known set without adding an arm is a visible no-op here + # rather than an implicit allow. + return + # ============================================================================= # v3 wire-protocol helpers # ============================================================================= diff --git a/tests/test_gate_malformed_decision.py b/tests/test_gate_malformed_decision.py new file mode 100644 index 0000000..6259a47 --- /dev/null +++ b/tests/test_gate_malformed_decision.py @@ -0,0 +1,188 @@ +"""A ``/gate`` body with no usable ``decision`` must never read as "allowed". + +ADR-008 grants fail-OPEN to *transport* failures — the case where the +gate never got to rule on the call. A body that arrives but carries no +verdict is the opposite case: something answered, and it was not a +policy engine. Pre-fix ``check_workflow_budget`` read it as allowed: + + decision = response.get("decision", "allow") + +That default converted four distinct situations into "the call may +proceed": + +* a JSON object from a non-NULLRUN responder (proxy error page, + captive portal, TLS interception box), +* a body that is not a JSON object at all, +* a ``decision`` the SDK has no arm for, +* ``decision="deny"`` — a live variant of the backend's ``GateDecision`` + (``gate/internal.rs:579``) reserved by ADR-046, which had no arm and + fell off the end of the method. + +The last one matters most: a *reserved refusal* was the one refusal +that executed. Each test below fails against the pre-fix code. +""" + +from __future__ import annotations + +import httpx +import pytest +import respx + +from nullrun.breaker.exceptions import ( + NullRunInfrastructureError, + NullRunMalformedGateResponseError, + NullRunProtocolError, +) + +BASE_URL = "https://api.test.nullrun.io" +GATE_URL = f"{BASE_URL}/api/v1/gate" + + +def _gate_json(**overrides) -> dict: + """A well-formed gate answer, with `overrides` applied.""" + body = { + "decision": "allow", + "decision_source": "gateway", + "explanation": "", + "policy_version": 1, + "explanations": [], + } + body.update(overrides) + return body + + +class TestMalformedGateDecisionRaises: + """Every shape that is not a verdict must raise, not allow.""" + + def test_absent_decision_field_raises(self, make_runtime, mock_api): + """No `decision` key at all. + + The backend's `decision` is a non-`Option` field with no + `skip_serializing_if` (`gate/internal.rs:637`), so a real + NULLRUN response always carries one. Its absence means the + responder was not NULLRUN. + """ + payload = _gate_json() + del payload["decision"] + respx.post(GATE_URL).mock(return_value=httpx.Response(200, json=payload)) + + rt = make_runtime() + with pytest.raises(NullRunMalformedGateResponseError): + rt.check_workflow_budget() + + def test_null_decision_raises(self, make_runtime, mock_api): + """`decision: null` is not a verdict either.""" + respx.post(GATE_URL).mock( + return_value=httpx.Response(200, json=_gate_json(decision=None)) + ) + rt = make_runtime() + with pytest.raises(NullRunMalformedGateResponseError): + rt.check_workflow_budget() + + def test_non_string_decision_raises(self, make_runtime, mock_api): + """A numeric or object `decision` cannot be compared to "block".""" + respx.post(GATE_URL).mock( + return_value=httpx.Response(200, json=_gate_json(decision={"code": 1})) + ) + rt = make_runtime() + with pytest.raises(NullRunMalformedGateResponseError): + rt.check_workflow_budget() + + def test_unknown_decision_string_raises(self, make_runtime, mock_api): + """An unrecognised verdict is a contract this SDK does not implement. + + Reading it as "allow" is how a future backend addition would + silently become a bypass. + """ + respx.post(GATE_URL).mock( + return_value=httpx.Response(200, json=_gate_json(decision="allow_anyway")) + ) + rt = make_runtime() + with pytest.raises(NullRunMalformedGateResponseError): + rt.check_workflow_budget() + + def test_deny_decision_raises(self, make_runtime, mock_api): + """`deny` is a refusal, and refusals do not execute. + + `GateDecision::Deny` exists on the wire (`gate/internal.rs:579`), + reserved by ADR-046 with no production producer yet. Pre-fix it + matched no arm and fell off the end of `check_workflow_budget`, + which the caller reads as "no block raised, proceed". + """ + respx.post(GATE_URL).mock( + return_value=httpx.Response( + 200, + json=_gate_json( + decision="deny", + explanation="policy denies this capability", + ), + ) + ) + rt = make_runtime() + with pytest.raises(Exception) as exc_info: + rt.check_workflow_budget() + # Not the malformed-shape error: the body was well-formed, the + # decision was just a refusal the SDK must honour. + assert not isinstance(exc_info.value, NullRunMalformedGateResponseError) + assert "policy denies this capability" in str(exc_info.value) + + +class TestMalformedGateBodyRaises: + """Bodies that are not gate answers at all.""" + + def test_non_json_body_raises_typed_error(self, make_runtime, mock_api): + """An HTML interception page must not escape as JSONDecodeError. + + `Transport.check` ends in `response.json()`. Pre-fix the + resulting ValueError matched neither the auth arm nor the + fail-OPEN arm in the cached branch, and in the uncached branch + was swallowed by `except Exception` as "gate unavailable" — + i.e. a non-NULLRUN responder was read as an outage and the call + proceeded. + """ + respx.post(GATE_URL).mock( + return_value=httpx.Response( + 200, + text="Corporate proxy block page", + headers={"content-type": "text/html"}, + ) + ) + rt = make_runtime() + with pytest.raises(NullRunMalformedGateResponseError): + rt.check_workflow_budget() + + def test_json_array_body_raises_typed_error(self, make_runtime, mock_api): + """A JSON array is not a gate answer; `.get` on it would AttributeError.""" + respx.post(GATE_URL).mock( + return_value=httpx.Response(200, json=["decision", "allow"]) + ) + rt = make_runtime() + with pytest.raises(NullRunMalformedGateResponseError): + rt.check_workflow_budget() + + +class TestMalformedErrorHierarchy: + """The new error must be catchable by the handlers that already exist.""" + + def test_is_a_protocol_error(self): + """Callers already catching version mismatch catch this too.""" + assert issubclass(NullRunMalformedGateResponseError, NullRunProtocolError) + + def test_is_infrastructure_error(self): + """And it is not a *decision*, so it must not be swallowed as one.""" + assert issubclass(NullRunMalformedGateResponseError, NullRunInfrastructureError) + + def test_has_distinct_error_code(self): + """NR-P002, not the NR-P001 of "upgrade the SDK".""" + assert NullRunMalformedGateResponseError.error_code == "NR-P002" + + +class TestAllowUnchanged: + """The fail-CLOSED work must not have broken the ordinary allow path.""" + + def test_allow_still_returns(self, make_runtime, mock_api): + respx.post(GATE_URL).mock( + return_value=httpx.Response(200, json=_gate_json(decision="allow")) + ) + rt = make_runtime() + rt.check_workflow_budget() # must not raise From cdf94c39be46a440d71a7ccf88933a2999d08fe7 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 12:56:39 +0400 Subject: [PATCH 02/20] fix(gate): refuse NULLRUN_SENSITIVE_FAIL_OPEN against production The opt-out was read straight into the enforcement path with no environment check: fail_open = os.environ.get("NULLRUN_SENSITIVE_FAIL_OPEN", "") == "1" Its sibling NULLRUN_SKIP_BUDGET_CHECK has been production-guarded since it was caught doing the same thing. The asymmetry was an oversight, and this is the more dangerous of the two: the budget opt-out skips a pre-flight, while this one lets the body of a sensitive tool run with no policy evaluation at all -- an unblocked charge_card during an outage, which ADR-008 calls a security regression rather than an availability trade-off. Resolution now lives in `NullRunRuntime.sensitive_fail_open_enabled` and the decorator asks the runtime, so the environment policy stays in one module with `_is_production_environment` and the two opt-outs cannot drift apart the way their predicates did under DEF-MP-TS12-ENF-01. In production the flag alone is refused: ignored, logged at ERROR, counted as `sensitive_fail_open_blocked_in_prod`. Enforcement falls back to its own fail-CLOSED default, so an operator who set the var carelessly keeps a working agent rather than a crash loop, and the attempt is visible instead of silent. An incident-response runbook can still force it with NULLRUN_ALLOW_SENSITIVE_FAIL_OPEN=1, mirroring NULLRUN_ALLOW_SKIP_BUDGET_CHECK -- logged at WARNING and counted as `sensitive_fail_open_allowed_in_prod`. The guard is verified two ways, and the second is the one that matters. Seven predicate tests cover the env matrix. The e2e test covers the caller, because a correct helper with a caller that still reads the raw env var looks identical from the predicate alone: under mutation (decorator reading os.environ again) the seven still pass and only `test_sensitive_body_does_not_run_in_prod` fails, logging "body will run" against the production host. `_StubRuntime` in test_langchain_enforcement.py gained the method. Its "every gate passes" contract means reporting the flag OFF, not mirroring the raw read -- mirroring would have hidden the very regression the new tests exist to catch. Suite: 1502 passed, 1 skipped, 0 failed. --- src/nullrun/decorators.py | 3 +- src/nullrun/runtime.py | 79 +++++++++ tests/test_langchain_enforcement.py | 12 ++ tests/test_sensitive_fail_open_guard.py | 210 ++++++++++++++++++++++++ 4 files changed, 302 insertions(+), 2 deletions(-) create mode 100644 tests/test_sensitive_fail_open_guard.py diff --git a/src/nullrun/decorators.py b/src/nullrun/decorators.py index b9fd555..63e350f 100644 --- a/src/nullrun/decorators.py +++ b/src/nullrun/decorators.py @@ -38,7 +38,6 @@ def researcher(q): import functools import inspect import logging -import os import threading from collections.abc import Callable from contextvars import Token @@ -851,7 +850,7 @@ def _run_tool_policy_gate( TransportErrorSource, ) - fail_open = os.environ.get("NULLRUN_SENSITIVE_FAIL_OPEN", "").strip() == "1" + fail_open = runtime.sensitive_fail_open_enabled() # *display* workflow_id via the runtime's precedence chain # (contextvar → self.workflow_id → None). Sentinel stays as the # last resort for never-bound keys (no workflow context). diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index 0120e71..c732e3f 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -1949,6 +1949,85 @@ def _raise_malformed_gate_response(self, exc: BaseException) -> None: f"/gate returned a non-JSON body: {exc}" ) from exc + def sensitive_fail_open_enabled(self) -> bool: + """Resolve ``NULLRUN_SENSITIVE_FAIL_OPEN``, honouring the prod guard. + + The raw env var is a documented bypass, and before this method + it was read straight into the enforcement path with no + environment check: + + fail_open = os.environ.get("NULLRUN_SENSITIVE_FAIL_OPEN", "") == "1" + + Its sibling ``NULLRUN_SKIP_BUDGET_CHECK`` has been + production-guarded since it was found doing exactly this + (``check_workflow_budget``). The asymmetry was an oversight, + and it is the more dangerous of the two: the budget opt-out + skips a *pre-flight*, while this one lets the body of a + sensitive tool run while the policy engine is unreachable -- + an unblocked ``charge_card`` during an outage, which ADR-008 + calls a security regression rather than an availability + trade-off. + + In production the flag alone is refused: it is ignored, an + ERROR is logged, and a metric is emitted, so the attempt is + visible rather than silent. Enforcement then proceeds + fail-CLOSED, which is the policy the flag was trying to + disable -- refusing the bypass does not break the agent, it + restores the safe default. An operator who genuinely needs it + in prod (an incident-response runbook) acknowledges with + ``NULLRUN_ALLOW_SENSITIVE_FAIL_OPEN=1``, mirroring + ``NULLRUN_ALLOW_SKIP_BUDGET_CHECK``. + + Lives here rather than in ``decorators`` so the environment + policy stays in one module with + :func:`_is_production_environment`, and so the two opt-outs + cannot drift apart again the way their predicates did under + DEF-MP-TS12-ENF-01. + """ + if os.environ.get("NULLRUN_SENSITIVE_FAIL_OPEN", "").strip() != "1": + return False + + if _is_production_environment(self.api_url): + allow_ack = ( + os.environ.get("NULLRUN_ALLOW_SENSITIVE_FAIL_OPEN", "").strip() == "1" + ) + if not allow_ack: + logger.error( + "NULLRUN_SENSITIVE_FAIL_OPEN=1 is set but the SDK is " + "configured for production (api_url=%r). Refusing the " + "bypass: sensitive tools stay fail-CLOSED, so their " + "bodies do NOT run while the policy engine is " + "unreachable. Unset the var, or — only for " + "incident-response scenarios — also set " + "NULLRUN_ALLOW_SENSITIVE_FAIL_OPEN=1 to acknowledge " + "the risk.", + self.api_url, + ) + try: + metrics.inc_runtime("sensitive_fail_open_blocked_in_prod") + except Exception: # noqa: BLE001 — metrics never gate + pass + return False + logger.warning( + "sensitive tool gate: failing OPEN via " + "NULLRUN_SENSITIVE_FAIL_OPEN=1 in production " + "(NULLRUN_ALLOW_SENSITIVE_FAIL_OPEN=1 also set). This is an " + "explicit operator ack — ensure the incident-response runbook " + "drove it, and unset both vars when the incident closes." + ) + try: + metrics.inc_runtime("sensitive_fail_open_allowed_in_prod") + except Exception: # noqa: BLE001 + pass + return True + + logger.debug( + "sensitive tool gate: failing OPEN via NULLRUN_SENSITIVE_FAIL_OPEN=1 " + "(non-production api_url=%r).", + self.api_url, + ) + return True + def check_workflow_budget(self) -> None: """ Pre-flight budget check via /api/v1/gate. Called from @protect diff --git a/tests/test_langchain_enforcement.py b/tests/test_langchain_enforcement.py index 5e852cd..b825493 100644 --- a/tests/test_langchain_enforcement.py +++ b/tests/test_langchain_enforcement.py @@ -262,6 +262,18 @@ def _resolve_workflow_id(self, *a, **k): def _emit_sdk_error(self, *a, **k): return None + def sensitive_fail_open_enabled(self, *a, **k): + """The real runtime refuses this bypass in production. + + The stub keeps the "every gate passes" contract these tests + assert against, so it reports the flag OFF -- the default for + an operator who has set nothing. If the decorator ever reads + the env var directly again, the production-guard tests in + `test_sensitive_fail_open_guard.py` are what catch it; this + stub must not paper over that by mirroring the raw read. + """ + return False + @pytest.fixture def stub_runtime(monkeypatch): diff --git a/tests/test_sensitive_fail_open_guard.py b/tests/test_sensitive_fail_open_guard.py new file mode 100644 index 0000000..775cab7 --- /dev/null +++ b/tests/test_sensitive_fail_open_guard.py @@ -0,0 +1,210 @@ +"""``NULLRUN_SENSITIVE_FAIL_OPEN`` must be refused in production. + +The opt-out lets a sensitive tool's body run while the policy engine is +unreachable. Pre-fix it was read straight into the enforcement path: + + fail_open = os.environ.get("NULLRUN_SENSITIVE_FAIL_OPEN", "") == "1" + +Its sibling ``NULLRUN_SKIP_BUDGET_CHECK`` has been production-guarded +since it was caught doing the same thing. The asymmetry was an +oversight, and it is the more dangerous half: the budget opt-out skips +a *pre-flight*, while this one lets ``charge_card`` run with no +policy evaluation at all. ADR-008 calls that a security regression +rather than an availability trade-off. + +The guard refuses the bypass rather than raising: enforcement falls +back to its own fail-CLOSED default, so an operator who set the var +carelessly keeps a working agent instead of a crash loop, and the +attempt is logged at ERROR with a metric rather than passing +unnoticed. + +Every test below fails against the pre-fix code. +""" + +from __future__ import annotations + +import httpx +import pytest +import respx + +BASE_URL = "https://api.test.nullrun.io" +EXECUTE_URL = f"{BASE_URL}/api/v1/execute" +PROD_URL = "https://api.nullrun.io" +# A non-prod-looking host that is NOT one of the hosts +# `_is_production_environment` treats as a dev/staging escape hatch +# (localhost / 127.0.0.1 / staging / test), so `NULLRUN_ENV=production` +# is the only thing marking it production. +CUSTOM_URL = "https://nullrun.internal.example.com" + +_FLAG = "NULLRUN_SENSITIVE_FAIL_OPEN" +_ACK = "NULLRUN_ALLOW_SENSITIVE_FAIL_OPEN" + + +@pytest.fixture(autouse=True) +def _clean_flag_env(monkeypatch): + """Both vars start unset, so a developer's shell cannot decide + whether these tests pass.""" + monkeypatch.delenv(_FLAG, raising=False) + monkeypatch.delenv(_ACK, raising=False) + monkeypatch.delenv("NULLRUN_ENV", raising=False) + monkeypatch.delenv("NULLRUN_API_URL", raising=False) + + +@pytest.fixture +def mock_prod_api(mock_api): + """Mirror the conftest auth route onto the non-test hosts used here. + + `mock_api` only mocks `BASE_URL`, so a runtime built with any other + `api_url` authenticates against an unmocked host and respx fails the + test before the guard is ever consulted. Depends on `mock_api` so + it registers inside that fixture's `with respx.mock:` context + rather than opening a second one. + + Deliberately registers `/execute` for NO host: each test below + needs `/execute` to fail, and a permissive default here would + shadow the failure they are asserting on. + """ + def _verify(request: httpx.Request) -> httpx.Response: + return httpx.Response( + 200, + json={ + "organization_id": "ws-test", + "workflow_id": "00000000-0000-0000-0000-000000000001", + "plan": "pro", + "features": [], + "limits": {"max_cost_cents": 10000}, + "secret_key": "test-secret-deterministic", + }, + ) + + respx.post(f"{PROD_URL}/api/v1/auth/verify").mock(side_effect=_verify) + respx.post(f"{CUSTOM_URL}/api/v1/auth/verify").mock(side_effect=_verify) + return mock_api + + +class TestProductionGuard: + """The flag alone must not open the gate against production.""" + + def test_flag_ignored_in_prod_without_ack(self, make_runtime, mock_prod_api): + monkey_api = make_runtime(api_url=PROD_URL) + import os + + os.environ[_FLAG] = "1" + assert monkey_api.sensitive_fail_open_enabled() is False + + def test_flag_honoured_in_prod_with_ack(self, make_runtime, mock_prod_api): + rt = make_runtime(api_url=PROD_URL) + import os + + os.environ[_FLAG] = "1" + os.environ[_ACK] = "1" + assert rt.sensitive_fail_open_enabled() is True + + def test_ack_alone_does_nothing(self, make_runtime, mock_prod_api): + """The ack is a second signature, not a substitute.""" + rt = make_runtime(api_url=PROD_URL) + import os + + os.environ[_ACK] = "1" + assert rt.sensitive_fail_open_enabled() is False + + def test_flag_ignored_when_nullrun_env_is_production(self, make_runtime, mock_prod_api): + """`NULLRUN_ENV=production` marks prod even on a custom host.""" + rt = make_runtime(api_url=CUSTOM_URL) + import os + + os.environ["NULLRUN_ENV"] = "production" + os.environ[_FLAG] = "1" + assert rt.sensitive_fail_open_enabled() is False + + def test_flag_honoured_outside_prod(self, make_runtime): + """The documented dev / test use keeps working.""" + rt = make_runtime(api_url=BASE_URL) + import os + + os.environ[_FLAG] = "1" + assert rt.sensitive_fail_open_enabled() is True + + def test_absent_flag_is_false(self, make_runtime): + assert make_runtime(api_url=BASE_URL).sensitive_fail_open_enabled() is False + + def test_blank_flag_is_false(self, make_runtime): + """Whitespace is not a yes. The `.strip()` matters.""" + rt = make_runtime(api_url=BASE_URL) + import os + + os.environ[_FLAG] = " " + assert rt.sensitive_fail_open_enabled() is False + + +class TestEndToEndAgainstProductionUrl: + """The guard must hold on the real enforcement path, not just the + predicate. + + This is the test that would have caught the original defect: the + helper can be right while the caller still reads the raw env var. + """ + + def test_sensitive_body_does_not_run_in_prod(self, make_runtime, mock_prod_api): + """With /execute unreachable, the body must NOT run in prod + even though the operator set the flag.""" + from nullrun.decorators import protect + + rt = make_runtime(api_url=PROD_URL) + import os + + os.environ[_FLAG] = "1" + + respx.post(f"{PROD_URL}/api/v1/execute").mock( + side_effect=httpx.ConnectError("connection refused") + ) + + ran: list[int] = [] + + @protect + def charge_card(amount: int) -> str: + ran.append(amount) + return "charged" + + with pytest.raises(Exception): + charge_card(100) + + assert ran == [], ( + "sensitive body ran while the policy engine was unreachable, " + "with NULLRUN_SENSITIVE_FAIL_OPEN=1 set against production" + ) + + def test_sensitive_body_runs_in_dev_with_flag(self, make_runtime, mock_api): + """The documented bypass still works off production.""" + from nullrun.breaker.exceptions import NullRunBlockedException + from nullrun.decorators import protect + + rt = make_runtime(api_url=BASE_URL) + import os + + os.environ[_FLAG] = "1" + + respx.post(EXECUTE_URL).mock( + side_effect=httpx.ConnectError("connection refused") + ) + + ran: list[int] = [] + + @protect + def charge_card(amount: int) -> str: + ran.append(amount) + return "charged" + + # The bypass is a `return`, not a swallow of a raised error -- + # whichever the transport does, the observable contract is that + # the body ran. Guard against the opposite regression too. + try: + charge_card(100) + except (NullRunBlockedException, Exception): + pass + + assert ran == [100], ( + "the documented dev/test bypass stopped working -- " + "NULLRUN_SENSITIVE_FAIL_OPEN=1 against a non-prod api_url " + "must let the body run" + ) From 700fea77d42119ffb82c32a1b8356b1818abbe98 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 12:59:09 +0400 Subject: [PATCH 03/20] fix(gate): make the non-prod budget bypass visible in logs and metrics NULLRUN_SKIP_BUDGET_CHECK=1 outside production was a `logger.debug` and a bare `return`. DEBUG is below every default handler, and nothing was counted, so a test suite running the whole budget path with the bypass on left no trace: tests went green, the dashboard showed the org spending nothing, and the only evidence the gate was never consulted was the absence of a block. CLAUDE.md's rule is that a test passing only with this flag set is evidence of a broken gate, not of a working one. That rule is not actionable unless setting the flag is loud enough to find. Now: a WARNING naming the flag and stating that no budget, rate-limit or tool-block check ran (so a reader can tell a bypass from an allow), plus a `skip_budget_used_non_prod` counter a CI dashboard can alert on. The production refusal already emitted `skip_budget_blocked_in_prod`; this is its dev/test counterpart. Verified by mutation: reverting to the `logger.debug` line fails all three new tests. The fourth asserts the production refusal is untouched by making the dev path louder. Two test-infrastructure corrections this surfaced: `mock_prod_api` moved to conftest. Both guard test files need a runtime that genuinely looks like production, and the first version duplicated the prod auth response in each module -- the same duplication that let the AUTH_ERROR predicate drift between `check_workflow_budget` and `_run_tool_policy_gate` under DEF-MP-TS12-ENF-01. The new tests set the env vars with `os.environ[...] = ...` instead of `monkeypatch.setenv`, which leaked NULLRUN_SKIP_BUDGET_CHECK into every later test in the session. Symptom was confusing: the files passed alone and the full suite failed 9 tests in test_v3_wire_contract.py, because those tests' budget pre-flight was being skipped by a flag set three modules earlier. Now monkeypatch-scoped, and the full suite is green. Suite: 1506 passed, 1 skipped, 0 failed. --- src/nullrun/runtime.py | 32 ++++++- tests/conftest.py | 55 ++++++++++++ tests/test_sensitive_fail_open_guard.py | 94 +++++--------------- tests/test_skip_budget_check_visible.py | 112 ++++++++++++++++++++++++ 4 files changed, 219 insertions(+), 74 deletions(-) create mode 100644 tests/test_skip_budget_check_visible.py diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index c732e3f..9597589 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -2107,7 +2107,37 @@ def check_workflow_budget(self) -> None: except Exception: # noqa: BLE001 pass return - logger.debug("check_workflow_budget: skipped via NULLRUN_SKIP_BUDGET_CHECK=1") + # Non-production. Pre-fix this was `logger.debug`, which + # means a test suite running the whole budget path with + # the bypass on leaves no trace anywhere: the tests go + # green, the dashboard shows the org as spending nothing, + # and the only evidence the gate was never consulted is + # the absence of a block. CLAUDE.md's rule is explicit + # that a test which only passes with this flag set is + # evidence of a broken gate -- so the flag setting must be + # loud enough to find. + # Non-production. Pre-fix this was `logger.debug`, which + # means a test suite running the whole budget path with + # the bypass on leaves no trace anywhere: the tests go + # green, the dashboard shows the org as spending nothing, + # and the only evidence the gate was never consulted is + # the absence of a block. CLAUDE.md's rule is explicit + # that a test which only passes with this flag set is + # evidence of a broken gate -- so the flag setting must be + # loud enough to find. + logger.warning( + "check_workflow_budget: budget gate BYPASSED via " + "NULLRUN_SKIP_BUDGET_CHECK=1 (non-production api_url=%r). " + "No budget check, no rate-limit check, and no tool-block " + "check ran for this call -- a test passing in this state " + "proves nothing about enforcement. Unset the var unless " + "you are deliberately exercising a non-budget path.", + self.api_url, + ) + try: + metrics.inc_runtime("skip_budget_used_non_prod") + except Exception: # noqa: BLE001 — metrics never gate + pass return # Bump the ``check_calls`` counter so the dashboard can show diff --git a/tests/conftest.py b/tests/conftest.py index 17bc8ad..ac3e60c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -251,3 +251,58 @@ def _isolated_wal(monkeypatch, tmp_path): # and NullRunAuthError propagates back into the test fixture setup. monkeypatch.setenv("NULLRUN_WAL_PATH", str(tmp_path / "sdk.wal")) yield + + +# --------------------------------------------------------------------------- +# Production-host mocks +# +# `mock_api` only mocks BASE_URL, so a runtime built with any other +# `api_url` authenticates against an unmocked host and respx fails the +# test before the environment guard under test is ever consulted. Tests +# covering the production guards (NULLRUN_SKIP_BUDGET_CHECK, +# NULLRUN_SENSITIVE_FAIL_OPEN) need a runtime that genuinely looks like +# production, so they need this. +# +# Lives here rather than in either test module because both need it, and +# two copies of "what does a prod auth response look like" is exactly the +# duplication that let the AUTH_ERROR predicate drift between +# `check_workflow_budget` and `_run_tool_policy_gate` under +# DEF-MP-TS12-ENF-01. +# --------------------------------------------------------------------------- + +PROD_URL = "https://api.nullrun.io" + +# A host that is NOT one of the ones `_is_production_environment` treats as +# a dev/staging escape hatch (localhost / 127.0.0.1 / staging / test), so +# `NULLRUN_ENV=production` is the only thing marking it production. +CUSTOM_NONPROD_URL = "https://nullrun.internal.example.com" + + +@pytest.fixture +def mock_prod_api(mock_api): + """Mirror the auth route onto the production hosts. + + Depends on `mock_api` so it registers inside that fixture's + `with respx.mock:` context rather than opening a second one. + + Deliberately registers `/execute` for NO host: every caller needs + `/execute` to fail, and a permissive default here would shadow the + failure they are asserting on. + """ + + def _verify(request) -> Response: + return Response( + 200, + json={ + "organization_id": "ws-test", + "workflow_id": "00000000-0000-0000-0000-000000000001", + "plan": "pro", + "features": [], + "limits": {"max_cost_cents": 10000}, + "secret_key": "test-secret-deterministic", + }, + ) + + respx.post(f"{PROD_URL}/api/v1/auth/verify").mock(side_effect=_verify) + respx.post(f"{CUSTOM_NONPROD_URL}/api/v1/auth/verify").mock(side_effect=_verify) + return mock_api diff --git a/tests/test_sensitive_fail_open_guard.py b/tests/test_sensitive_fail_open_guard.py index 775cab7..ff055bf 100644 --- a/tests/test_sensitive_fail_open_guard.py +++ b/tests/test_sensitive_fail_open_guard.py @@ -29,12 +29,8 @@ BASE_URL = "https://api.test.nullrun.io" EXECUTE_URL = f"{BASE_URL}/api/v1/execute" -PROD_URL = "https://api.nullrun.io" -# A non-prod-looking host that is NOT one of the hosts -# `_is_production_environment` treats as a dev/staging escape hatch -# (localhost / 127.0.0.1 / staging / test), so `NULLRUN_ENV=production` -# is the only thing marking it production. -CUSTOM_URL = "https://nullrun.internal.example.com" +from tests.conftest import CUSTOM_NONPROD_URL as CUSTOM_URL # noqa: E402 +from tests.conftest import PROD_URL # noqa: E402 _FLAG = "NULLRUN_SENSITIVE_FAIL_OPEN" _ACK = "NULLRUN_ALLOW_SENSITIVE_FAIL_OPEN" @@ -50,90 +46,46 @@ def _clean_flag_env(monkeypatch): monkeypatch.delenv("NULLRUN_API_URL", raising=False) -@pytest.fixture -def mock_prod_api(mock_api): - """Mirror the conftest auth route onto the non-test hosts used here. - - `mock_api` only mocks `BASE_URL`, so a runtime built with any other - `api_url` authenticates against an unmocked host and respx fails the - test before the guard is ever consulted. Depends on `mock_api` so - it registers inside that fixture's `with respx.mock:` context - rather than opening a second one. - - Deliberately registers `/execute` for NO host: each test below - needs `/execute` to fail, and a permissive default here would - shadow the failure they are asserting on. - """ - def _verify(request: httpx.Request) -> httpx.Response: - return httpx.Response( - 200, - json={ - "organization_id": "ws-test", - "workflow_id": "00000000-0000-0000-0000-000000000001", - "plan": "pro", - "features": [], - "limits": {"max_cost_cents": 10000}, - "secret_key": "test-secret-deterministic", - }, - ) - - respx.post(f"{PROD_URL}/api/v1/auth/verify").mock(side_effect=_verify) - respx.post(f"{CUSTOM_URL}/api/v1/auth/verify").mock(side_effect=_verify) - return mock_api - - class TestProductionGuard: """The flag alone must not open the gate against production.""" - def test_flag_ignored_in_prod_without_ack(self, make_runtime, mock_prod_api): + def test_flag_ignored_in_prod_without_ack(self, make_runtime, mock_prod_api, monkeypatch): monkey_api = make_runtime(api_url=PROD_URL) - import os - - os.environ[_FLAG] = "1" + monkeypatch.setenv(_FLAG, "1") assert monkey_api.sensitive_fail_open_enabled() is False - def test_flag_honoured_in_prod_with_ack(self, make_runtime, mock_prod_api): + def test_flag_honoured_in_prod_with_ack(self, make_runtime, mock_prod_api, monkeypatch): rt = make_runtime(api_url=PROD_URL) - import os - - os.environ[_FLAG] = "1" - os.environ[_ACK] = "1" + monkeypatch.setenv(_FLAG, "1") + monkeypatch.setenv(_ACK, "1") assert rt.sensitive_fail_open_enabled() is True - def test_ack_alone_does_nothing(self, make_runtime, mock_prod_api): + def test_ack_alone_does_nothing(self, make_runtime, mock_prod_api, monkeypatch): """The ack is a second signature, not a substitute.""" rt = make_runtime(api_url=PROD_URL) - import os - - os.environ[_ACK] = "1" + monkeypatch.setenv(_ACK, "1") assert rt.sensitive_fail_open_enabled() is False - def test_flag_ignored_when_nullrun_env_is_production(self, make_runtime, mock_prod_api): + def test_flag_ignored_when_nullrun_env_is_production(self, make_runtime, mock_prod_api, monkeypatch): """`NULLRUN_ENV=production` marks prod even on a custom host.""" rt = make_runtime(api_url=CUSTOM_URL) - import os - - os.environ["NULLRUN_ENV"] = "production" - os.environ[_FLAG] = "1" + monkeypatch.setenv("NULLRUN_ENV", "production") + monkeypatch.setenv(_FLAG, "1") assert rt.sensitive_fail_open_enabled() is False - def test_flag_honoured_outside_prod(self, make_runtime): + def test_flag_honoured_outside_prod(self, make_runtime, monkeypatch): """The documented dev / test use keeps working.""" rt = make_runtime(api_url=BASE_URL) - import os - - os.environ[_FLAG] = "1" + monkeypatch.setenv(_FLAG, "1") assert rt.sensitive_fail_open_enabled() is True - def test_absent_flag_is_false(self, make_runtime): + def test_absent_flag_is_false(self, make_runtime, monkeypatch): assert make_runtime(api_url=BASE_URL).sensitive_fail_open_enabled() is False - def test_blank_flag_is_false(self, make_runtime): + def test_blank_flag_is_false(self, make_runtime, monkeypatch): """Whitespace is not a yes. The `.strip()` matters.""" rt = make_runtime(api_url=BASE_URL) - import os - - os.environ[_FLAG] = " " + monkeypatch.setenv(_FLAG, " ") assert rt.sensitive_fail_open_enabled() is False @@ -145,15 +97,13 @@ class TestEndToEndAgainstProductionUrl: helper can be right while the caller still reads the raw env var. """ - def test_sensitive_body_does_not_run_in_prod(self, make_runtime, mock_prod_api): + def test_sensitive_body_does_not_run_in_prod(self, make_runtime, mock_prod_api, monkeypatch): """With /execute unreachable, the body must NOT run in prod even though the operator set the flag.""" from nullrun.decorators import protect rt = make_runtime(api_url=PROD_URL) - import os - - os.environ[_FLAG] = "1" + monkeypatch.setenv(_FLAG, "1") respx.post(f"{PROD_URL}/api/v1/execute").mock( side_effect=httpx.ConnectError("connection refused") @@ -174,15 +124,13 @@ def charge_card(amount: int) -> str: "with NULLRUN_SENSITIVE_FAIL_OPEN=1 set against production" ) - def test_sensitive_body_runs_in_dev_with_flag(self, make_runtime, mock_api): + def test_sensitive_body_runs_in_dev_with_flag(self, make_runtime, mock_api, monkeypatch): """The documented bypass still works off production.""" from nullrun.breaker.exceptions import NullRunBlockedException from nullrun.decorators import protect rt = make_runtime(api_url=BASE_URL) - import os - - os.environ[_FLAG] = "1" + monkeypatch.setenv(_FLAG, "1") respx.post(EXECUTE_URL).mock( side_effect=httpx.ConnectError("connection refused") diff --git a/tests/test_skip_budget_check_visible.py b/tests/test_skip_budget_check_visible.py new file mode 100644 index 0000000..880b77a --- /dev/null +++ b/tests/test_skip_budget_check_visible.py @@ -0,0 +1,112 @@ +"""A budget gate that was never consulted must not be invisible. + +`NULLRUN_SKIP_BUDGET_CHECK=1` disables the pre-flight entirely. In +production the SDK already refuses it without +`NULLRUN_ALLOW_SKIP_BUDGET_CHECK=1`. Outside production it was a +silent `logger.debug` plus a bare `return` -- so a test suite running +the whole budget path with the bypass on left no trace at all: the +tests went green, the dashboard showed the org spending nothing, and +the only evidence the gate had never been consulted was the absence of +a block. + +CLAUDE.md is explicit that a test which only passes with this flag set +is evidence of a broken gate, not evidence of a working one. For that +rule to be actionable the flag setting has to be loud enough to find. + +These tests fail against the pre-fix code, which logged at DEBUG and +emitted nothing. +""" + +from __future__ import annotations + +import logging + +import pytest + +BASE_URL = "https://api.test.nullrun.io" +from tests.conftest import PROD_URL # noqa: E402 + +_FLAG = "NULLRUN_SKIP_BUDGET_CHECK" +_ACK = "NULLRUN_ALLOW_SKIP_BUDGET_CHECK" + + +@pytest.fixture(autouse=True) +def _clean_flag_env(monkeypatch): + monkeypatch.delenv(_FLAG, raising=False) + monkeypatch.delenv(_ACK, raising=False) + monkeypatch.delenv("NULLRUN_ENV", raising=False) + monkeypatch.delenv("NULLRUN_API_URL", raising=False) + + +class TestNonProdSkipIsLoud: + def test_logs_at_warning(self, make_runtime, mock_api, caplog, monkeypatch): + """DEBUG is the level a developer's default handler drops.""" + rt = make_runtime(api_url=BASE_URL) + monkeypatch.setenv(_FLAG, "1") + + with caplog.at_level(logging.DEBUG, logger="nullrun.runtime"): + rt.check_workflow_budget() + + warnings = [ + r for r in caplog.records if r.levelno >= logging.WARNING + ] + assert warnings, ( + "bypassing the budget gate emitted no WARNING -- the skip is " + "invisible at any default log level" + ) + assert any( + "NULLRUN_SKIP_BUDGET_CHECK" in r.getMessage() for r in warnings + ), f"the WARNING does not name the flag: {[r.getMessage() for r in warnings]}" + + def test_warning_says_what_did_not_run(self, make_runtime, mock_api, caplog, monkeypatch): + """The message must say the check was skipped, not just that a + flag was set. 'BYPASSED ... no budget check ... ran' is + actionable; 'skipping' reads like a normal fast path.""" + rt = make_runtime(api_url=BASE_URL) + monkeypatch.setenv(_FLAG, "1") + + with caplog.at_level(logging.DEBUG, logger="nullrun.runtime"): + rt.check_workflow_budget() + + text = "\n".join(r.getMessage() for r in caplog.records) + assert "BYPASSED" in text + assert "No budget check" in text, ( + "the warning does not state that no check ran, so a reader " + "cannot tell a bypass from a normal allow" + ) + + def test_increments_a_counter(self, make_runtime, mock_api, monkeypatch): + """Silent in the logs is not enough; it must be graphable. + + The production refusal already emits + `skip_budget_blocked_in_prod`. The dev/test path needs its own + counter, because "bypass active in non-prod" is a signal a + CI dashboard can alert on, which no log line provides. + """ + from nullrun.observability import metrics + + rt = make_runtime(api_url=BASE_URL) + monkeypatch.setenv(_FLAG, "1") + + before = getattr(metrics.runtime, "skip_budget_used_non_prod", 0) + rt.check_workflow_budget() + after = getattr(metrics.runtime, "skip_budget_used_non_prod", 0) + + assert after == before + 1, ( + "skip_budget_used_non_prod was not incremented -- the " + "non-prod bypass is still unobservable in metrics" + ) + + +class TestProductionRefusalUnchanged: + """The guard from the previous change must not have been softened + by making the dev path louder.""" + + def test_prod_still_raises_without_ack(self, make_runtime, mock_prod_api, monkeypatch): + from nullrun.breaker.exceptions import NullRunInfrastructureError + + rt = make_runtime(api_url=PROD_URL) + monkeypatch.setenv(_FLAG, "1") + + with pytest.raises(NullRunInfrastructureError): + rt.check_workflow_budget() From 3c45138128342e4149174ec2c43bb893ce44507a Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 15:01:27 +0400 Subject: [PATCH 04/20] test(gate): pin that a 503 is an allow and a 403 is a stop (ADR-063 1.3f) The backend now answers a failed workflow-state read with 503 and a tripped breaker with 403. Both are correct on the backend -- the gate blocks either way -- but the SDK does something different with each, and the difference is invisible from the gate's own tests. 403 CIRCUIT_BREAKER_TRIPPED -> real gateway decision, honoured, raises, ONE attempt 503 *_LOOKUP_FAILED -> retried 3x, then a synthetic FALLBACK decision, which check_workflow_budget treats as a transport error and returns WITHOUT raising. An allow. So the current SDK behaves identically to 0.18.5 here. That is not a stale-SDK problem an SDK release fixes -- it is ADR-008's documented fail-OPEN on transport error, unchanged. Pinned so the asymmetry is a visible fact rather than something discovered during an incident, and so a future release that tightens it has to come with an ADR amendment. The 503 test asserts the CURRENT fail-OPEN on purpose. Asserting the desired behaviour would ship a red test; asserting "it raises" would be a lie. The comment says so at the assertion. Also pins that the "do not retry" instruction survives the round trip. The backend ships no agent_message for a halt-category refusal and no SDK version reads that field, so the explanation is the only channel a model sees -- worth a test, since nothing else would notice its loss. 4 passed; 29 with the neighbouring fallback/gate-path suites. --- ...adr063_infra_refusal_is_not_fail_closed.py | 180 ++++++++++++++++++ 1 file changed, 180 insertions(+) create mode 100644 tests/test_adr063_infra_refusal_is_not_fail_closed.py diff --git a/tests/test_adr063_infra_refusal_is_not_fail_closed.py b/tests/test_adr063_infra_refusal_is_not_fail_closed.py new file mode 100644 index 0000000..60f0904 --- /dev/null +++ b/tests/test_adr063_infra_refusal_is_not_fail_closed.py @@ -0,0 +1,180 @@ +"""ADR-063 §1.3(f): what the SDK does with an `infra` refusal. + +The backend now answers a failed workflow-state read with **503** +(`WORKFLOW_INACTIVE_LOOKUP_FAILED` / `CIRCUIT_BREAKER_STATE_LOOKUP_FAILED`, +category `infra`) rather than the 500 it used to ship, and answers a +tripped breaker with **403** (`CIRCUIT_BREAKER_TRIPPED`, category `halt`). + +Backend fail-CLOSED is not the same as user-visible fail-CLOSED. The +gate blocks in both cases, but what the SDK *does* with that block +differs, and the difference is the whole reason this file exists: + +* **403** lands in `transport.py`'s `400 <= status < 500` branch and + becomes a real `decision="block"` with `decision_source=GATEWAY`. + `check_workflow_budget` honours it and raises. The agent stops. +* **503** is `>= 500`, so `_retry_with_backoff` retries it + (`retry_on_5xx=True`, `max_retries=3`), then the transport + synthesises `decision="block"` with `decision_source=FALLBACK`. + `check_workflow_budget` calls `is_fallback_decision_source(...)` on + that and **returns without raising** — a fail-OPEN, per ADR-008's + documented "dead backend must not freeze the agent" rule. + +So on the current SDK a tripped breaker stops the agent and a failed +state read does not. Both are correct *for their category*: the +breaker genuinely stopped the workflow, the read failure did not stop +anything. But it means the pause work must NOT rely on a 5xx to halt +an agent — a `WORKFLOW_PAUSED` shipped as 503 would be an allow on +every SDK released to date. + +These tests pin that asymmetry so it cannot change silently, in either +direction. If a future SDK makes 5xx fail-CLOSED, the 503 tests go +red and the ADR gets amended; if one accidentally lets a 403 through +as a fallback, the 403 test goes red. +""" + +from __future__ import annotations + +import httpx +import pytest +import respx + +from nullrun.breaker.exceptions import NullRunBudgetError + +BASE_URL = "https://api.test.nullrun.io" +GATE_URL = f"{BASE_URL}/api/v1/gate" + + +def _infra_503() -> httpx.Response: + """The exact shape the backend ships for a failed state read.""" + return httpx.Response( + 503, + json={ + "decision": "block", + "decision_source": "gateway", + "error_code": "WORKFLOW_INACTIVE_LOOKUP_FAILED", + "category": "infra", + "user_message": "The gate could not read this workflow's state ...", + "explanation": ( + "The authorization check could not read this workflow's " + "state, so it failed closed. The workflow was not stopped. " + "Wait briefly and try again." + ), + "explanations": [], + "policy_version": 0, + }, + ) + + +def _breaker_trip_403() -> httpx.Response: + return httpx.Response( + 403, + json={ + "decision": "block", + "decision_source": "gateway", + "error_code": "CIRCUIT_BREAKER_TRIPPED", + "category": "halt", + "explanation": ( + "This workflow was stopped by its circuit breaker and will " + "not run again on its own. Do not retry this call and do not " + "try a different tool or approach." + ), + "explanations": [], + "policy_version": 0, + }, + ) + + +class TestInfraRefusalIsNotFailClosed: + """503 must NOT be read as "allowed" — but it currently is. + + The test asserts the *actual* behaviour, fail-OPEN included, and + says so in the failure message. Asserting the desired behaviour + here would ship a red test; asserting "it raises" would be a lie. + The point is that the asymmetry is now a pinned, visible fact + rather than something discovered during an incident. + """ + + def test_503_state_read_failure_fails_open_today(self, make_runtime, mock_api): + """A failed state read currently lets the call through. + + Pinned deliberately. ADR-008's fail-OPEN on transport error is + the documented policy and is not being changed here, but the + consequence — `infra` refusals do not stop an agent — must be + a known quantity before the pause work builds on 5xx. + """ + respx.post(GATE_URL).mock(return_value=_infra_503()) + rt = make_runtime() + + # Must NOT raise. If a future release makes this fail-CLOSED + # this test goes red, and ADR-008 + ADR-063 §1.3(f) get + # amended in the same commit. + rt.check_workflow_budget() # noqa: B018 - the absence of a raise IS the assertion + + def test_503_is_retried_before_failing_open(self, make_runtime, mock_api): + """The 503 is not a first-try fail-open. + + `_retry_with_backoff(retry_on_5xx=True, max_retries=3)` means a + transient 503 gets three more attempts, which is what makes the + fail-OPEN tolerable for a rolling deploy. Pinning the attempt + count stops a future "reduce retries on 5xx" change from + quietly making gate calls flakier under load. + """ + calls: list[httpx.Request] = [] + + def _count(request: httpx.Request) -> httpx.Response: + calls.append(request) + return _infra_503() + + respx.post(GATE_URL).mock(side_effect=_count) + rt = make_runtime() + rt.check_workflow_budget() + + assert len(calls) > 1, ( + "a 503 must be retried, not failed open on the first " + f"response — only {len(calls)} attempt(s) were made" + ) + + def test_403_breaker_trip_stops_the_agent(self, make_runtime, mock_api): + """The inverse case, and the one the whole fix is for. + + A tripped breaker is a real gateway decision, so the runtime + honours it and raises. Before the registry change this shipped + 500, which is also `>= 500` — so it took the same retry-then- + fail-OPEN path and the agent was told to retry a workflow an + operator had just stopped. + """ + respx.post(GATE_URL).mock(return_value=_breaker_trip_403()) + rt = make_runtime() + + with pytest.raises(NullRunBudgetError) as exc_info: + rt.check_workflow_budget() + + assert "stopped by its circuit breaker" in exc_info.value.reason + assert "Do not retry" in exc_info.value.reason, ( + "the agent-facing instruction must survive the round trip — " + "it is the only channel a model sees, since the backend " + "ships no agent_message for a `halt` category " + "(ADR-063 §1.3e)" + ) + + def test_403_is_not_retried(self, make_runtime, mock_api): + """A 4xx is final. One attempt, no backoff. + + This is the property that distinguishes the fixed trip from + the 500 it replaced: the old shape made three attempts against + an already-OPEN breaker. + """ + calls: list[httpx.Request] = [] + + def _count(request: httpx.Request) -> httpx.Response: + calls.append(request) + return _breaker_trip_403() + + respx.post(GATE_URL).mock(side_effect=_count) + rt = make_runtime() + with pytest.raises(NullRunBudgetError): + rt.check_workflow_budget() + + assert len(calls) == 1, ( + f"a 403 must be answered once; {len(calls)} attempts were made" + ) From 838a9059a21364073635bd51b783f8a68e50ab88 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 15:25:05 +0400 Subject: [PATCH 05/20] test: correct the ADR-063 fixtures to the real wire nesting The two response bodies in this file put `error_code` at the top level. It is not there. Captured from a live gate run: top_level_keys=["category", "decision", "decision_source", "details", "explanation", "policy_id", "policy_version", "projected_cost_cents", "remaining_budget_cents", "reservation_id", "staleness_ms", "user_message"] `error_code` lives at `details.error_code` (`internal.rs:716`), which is where the backend's own status mapper reads it (`gate.rs:88-90`). `category` / `user_message` are the top-level fields, set by `attach_refusal_surface` (`gate.rs:163-172`). These fixtures were hand-assembled when ADR-064 was written and the key landed where a reader would expect it. Harmless as a behaviour pin -- the SDK reads `explanation`, so all four assertions are unaffected and still pass. Not harmless as a reference: ADR-064's SDK-side plan reads exactly this body, and a client written against the wrong nesting classifies every refusal as unparseable, which is fail-OPEN. ADR-064 records the correction. 4 passed. --- ...adr063_infra_refusal_is_not_fail_closed.py | 36 +++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/tests/test_adr063_infra_refusal_is_not_fail_closed.py b/tests/test_adr063_infra_refusal_is_not_fail_closed.py index 60f0904..c706a88 100644 --- a/tests/test_adr063_infra_refusal_is_not_fail_closed.py +++ b/tests/test_adr063_infra_refusal_is_not_fail_closed.py @@ -45,22 +45,47 @@ def _infra_503() -> httpx.Response: - """The exact shape the backend ships for a failed state read.""" + """The exact shape the backend ships for a failed state read. + + Captured from a live gate run, not hand-assembled: the top-level + keys are `category` / `decision` / `decision_source` / `details` / + `explanation` / `policy_id` / `policy_version` / + `projected_cost_cents` / `remaining_budget_cents` / `reservation_id` + / `staleness_ms` / `user_message`. + + **`error_code` is NOT top-level.** It lives at `details.error_code` + (`internal.rs:716`), which is where the backend's own status mapper + reads it from (`gate.rs:88-90`). `category` and `user_message` are + top-level fields on `GateResponse`, set by `attach_refusal_surface` + (`gate.rs:163-172`). + + ADR-064 §Correction records an earlier version of these fixtures + that put `error_code` at the top level; they were built from a + hand-assembled sample rather than a captured response. The mistake + is load-bearing, not cosmetic: ADR-064's SDK-side plan reads this + body, and a client written against the wrong nesting classifies + every refusal as unparseable -- which is fail-OPEN. + """ return httpx.Response( 503, json={ "decision": "block", "decision_source": "gateway", - "error_code": "WORKFLOW_INACTIVE_LOOKUP_FAILED", "category": "infra", "user_message": "The gate could not read this workflow's state ...", + "details": {"error_code": "WORKFLOW_INACTIVE_LOOKUP_FAILED"}, "explanation": ( "The authorization check could not read this workflow's " "state, so it failed closed. The workflow was not stopped. " "Wait briefly and try again." ), "explanations": [], + "policy_id": None, "policy_version": 0, + "projected_cost_cents": None, + "remaining_budget_cents": None, + "reservation_id": None, + "staleness_ms": None, }, ) @@ -71,15 +96,20 @@ def _breaker_trip_403() -> httpx.Response: json={ "decision": "block", "decision_source": "gateway", - "error_code": "CIRCUIT_BREAKER_TRIPPED", "category": "halt", + "details": {"error_code": "CIRCUIT_BREAKER_TRIPPED"}, "explanation": ( "This workflow was stopped by its circuit breaker and will " "not run again on its own. Do not retry this call and do not " "try a different tool or approach." ), "explanations": [], + "policy_id": None, "policy_version": 0, + "projected_cost_cents": None, + "remaining_budget_cents": None, + "reservation_id": None, + "staleness_ms": None, }, ) From 83960b480e8ce2a65d957613bdbbb36f9835b464 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 21:37:19 +0400 Subject: [PATCH 06/20] fix(breaker): classify every gate refusal, and raise when it cannot be MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR-062 §2.2. The gate does not send a bare "no" — it sends a `category`, one of denied | budget | halt | infra, and that is what tells the SDK whether the answer is a policy decision, a money wall, a stop, or a backend fault. The SDK had no vocabulary for it, so every refusal became the same thing. This adds `nullrun.breaker.categories` mirroring the backend enum, resolves the category at the wire boundary in `Transport.check`, and puts it on the returned decision. The load-bearing half is what happens when the category is ABSENT or UNRECOGNISED. It raises `NullRunUnclassifiedRefusalError` rather than guessing. Guessing is the dangerous direction specifically: a refusal rounded to something permissive can be rendered as a friendly, model-readable "that's not allowed", and an agent handed that adapts and retries against a stop an operator deliberately placed. That is the bypass ADR-061 closed server-side, re-opened here. Two runtime arms had to change for the raise to mean anything. `check_workflow_budget`'s ADR-008 fail-OPEN arms catch `NullRunError`, and `NullRunUnclassifiedRefusalError` is one, so without an explicit re-raise the unclassifiable refusal was caught by the fail-OPEN arm and returned as `None` — the agent proceeding on a call the backend refused. Verified by mutation: dropping the two re-raise arms turns both branches red with "check_workflow_budget: /gate unavailable, failing open: Gate refused the call (decision="block") but sent no 'category' field". That is DEF-MP-TS12-ENF-01's exact shape with a different trigger. ADR-008 is not narrowed. A real outage still fails open; the counter-test pins it, and the arm order is what makes the two distinguishable — the re-raise is listed before the `NullRunError` and bare `except Exception` arms precisely because both are supersets of it. Also re-files the `DEF-NR-TOOLBLOCKED-PARSER` source-pin comment into `_parse_v3_error_envelope`, where the parser actually is. It was filed against a `check()` branch that does not exist, so the pin pointed readers at the wrong function; the pin now asserts the function it means. --- src/nullrun/breaker/categories.py | 209 +++++++++++++++++ src/nullrun/runtime.py | 19 +- src/nullrun/transport.py | 79 ++++++- tests/test_2026_09_10_check_failopen.py | 51 +++- tests/test_2026_09_10_toolblocked_parser.py | 21 +- tests/test_auth_fail_closed.py | 7 + tests/test_refusal_categories.py | 245 ++++++++++++++++++++ 7 files changed, 613 insertions(+), 18 deletions(-) create mode 100644 src/nullrun/breaker/categories.py create mode 100644 tests/test_refusal_categories.py diff --git a/src/nullrun/breaker/categories.py b/src/nullrun/breaker/categories.py new file mode 100644 index 0000000..72dac21 --- /dev/null +++ b/src/nullrun/breaker/categories.py @@ -0,0 +1,209 @@ +"""ADR-062 §2.2 refusal categories, mirrored from the backend. + +The gate does not send a bare "no". It sends *why* it said no, in +exactly four values (``backend/src/proxy/http/gate/error_codes.rs`` +→ ``DecisionCategory``): + +``denied`` + An operator refused **this specific call** on policy grounds. + Another call, or a different tool, might pass. This is the only + category the model may be told about in prose. + +``budget`` + A money or quota boundary is exhausted. Retrying does not help; + an operator has to raise a limit. Telling a model "your budget is + exhausted" produces tool-shopping followed by retries. + +``halt`` + The run itself is over — kill, pause, or a tripped breaker. + Retrying, adapting, and waiting all fail identically, so the + agent's only correct move is to stop. + +``infra`` + The check could not be completed, or an integrity invariant was + violated. There is no verdict. This is not a policy decision and + not the model's to resolve. + +The backend attaches the category (``category``), the model-safe +text (``agent_message``) and the operator text (``user_message``) +in one place — ``gate.rs::attach_refusal_surface`` — so a refusal +that carries a category is one the server understood. + +The rule this module exists to enforce (ADR-062 §2.2): + + A refusal the SDK cannot classify must RAISE, never be guessed. + +Failing open on an unclassifiable refusal is the exact hole ADR-061 +closed on the server side: if the SDK invents a category, a +``budget`` or ``halt`` refusal can be rendered as a friendly, +model-readable "that's not allowed" — and the agent adapts and +retries against a stop an operator deliberately placed. An absent or +unrecognised category therefore produces +:class:`NullRunUnclassifiedRefusalError`, an *infrastructure* +exception: the SDK could not read the answer, which is a system +fault, never a policy outcome. +""" + +from __future__ import annotations + +from enum import Enum +from typing import Any + +from nullrun.breaker.exceptions import NullRunInfrastructureError + +__all__ = [ + "DecisionCategory", + "NullRunUnclassifiedRefusalError", + "is_gate_refusal", + "resolve_refusal_category", +] + + +class DecisionCategory(str, Enum): + """The four ADR-062 refusal categories, as they appear on the wire. + + Mirrors the backend enum exactly. The wire values are the + lowercase snake_case strings the Rust + ``#[serde(rename_all = "snake_case")]` emits; membership is + deliberately *not* widened by a permissive fallback, because a + value the SDK does not recognise is a version skew the operator + needs to see, not something to round to ``infra``. + """ + + DENIED = "denied" + BUDGET = "budget" + HALT = "halt" + INFRA = "infra" + + def is_model_message_safe(self) -> bool: + """Whether this refusal may be turned into model-readable text. + + Only ``denied``. A rule judged *this call* unacceptable and + another call might pass, so telling the model is useful and + safe. The other three describe a wall the model cannot + climb, and a model told about a wall walks into it. + """ + return self is DecisionCategory.DENIED + + @classmethod + def parse(cls, raw: object) -> DecisionCategory: + """Parse a wire category string. + + Raises: + ValueError: on anything outside the four known values, + including a non-string. There is no default member: + guessing is the failure mode this module exists to + prevent. + """ + if isinstance(raw, DecisionCategory): + return raw + if isinstance(raw, str): + try: + return cls(raw) + except ValueError: + pass + raise ValueError( + f"unknown DecisionCategory {raw!r}; expected one of " + f"{', '.join(m.value for m in cls)}" + ) + + +class NullRunUnclassifiedRefusalError(NullRunInfrastructureError): + """The gate refused, but did not say why in a way the SDK trusts. + + Raised by :func:`resolve_refusal_category` when a response is + unambiguously a gate refusal (``decision == "block"``) and either + omits ``category`` or carries a value outside the four known + ones. + + It is an *infrastructure* error, not a decision, because the + honest description of the situation is "the SDK cannot tell + whether this was a policy outcome or a backend fault". Two + concrete causes, both real: + + * The backend took the NR-005 path — no registered + ``GateErrorCode`` matched the refusal — and deliberately sent no + category rather than shipping a wrong one + (``gate.rs::attach_refusal_surface`` returns early on + ``resolved = None``). + * Wire-version skew: a newer backend introduced a fifth category + and this SDK has not been taught it yet. + + In both cases ``retryable`` is True, because the correct + remediation is the same — get an SDK that understands the + backend — and the block itself is transient from the caller's + point of view. + """ + + error_code = "NR-P003" + user_action = ( + "The NullRun gate refused the call without a refusal category the " + "SDK understands, so the SDK cannot tell a policy decision from a " + "backend fault and will not guess. Upgrade the SDK " + "(pip install -U nullrun). If the backend is already current, this " + "means the refusal's error_code is not registered in " + "GateErrorCode — report the error_code below to NullRun support." + ) + retryable = True + + def __init__(self, message: str, *, wire_category: object = None, error_code_wire: str | None = None) -> None: + self.wire_category = wire_category + self.wire_error_code = error_code_wire + super().__init__(message) + + +def is_gate_refusal(body: Any) -> bool: + """Whether ``body`` is the gate's refusal envelope. + + The discriminator is the ``decision`` field, which + ``GateResponse`` always serialises (it is neither ``Option`` nor + ``skip_serializing_if``). No other NULLRUN error envelope — + protocol mismatch, admin 422, heartbeat 404 — carries it. + + Narrowing on this rather than on "any non-2xx" matters: the + strict absent-category rule below is a *refusal* rule, and + applying it to every error endpoint would turn unrelated wire + errors into infrastructure faults. + """ + return isinstance(body, dict) and body.get("decision") == "block" + + +def resolve_refusal_category(body: Any) -> DecisionCategory | None: + """Classify a refusal body, or return ``None`` if it is not one. + + Returns ``None`` for any body that is not a gate refusal, so + callers can use this unconditionally at the top of their + error handling without changing non-refusal behaviour. + + Raises: + NullRunUnclassifiedRefusalError: the body is a gate refusal + and its ``category`` is absent or unrecognised. Never + guess — see the module docstring for why the permissive + reading is the dangerous one. + """ + if not is_gate_refusal(body): + return None + + raw = body.get("category") + if raw is None: + raise NullRunUnclassifiedRefusalError( + "Gate refused the call (decision=\"block\") but sent no " + "'category' field, so the SDK cannot tell a policy decision " + "from a backend fault. Refusing to guess: surfacing an " + "unclassified refusal as a model-readable message is the " + f"bypass ADR-062 §2.2 exists to close. (error_code=" + f"{body.get('error_code')!r})", + wire_category=None, + error_code_wire=body.get("error_code"), + ) + try: + return DecisionCategory.parse(raw) + except ValueError as exc: + raise NullRunUnclassifiedRefusalError( + f"Gate refused the call with an unrecognised category {raw!r}: " + f"{exc} This SDK does not know this value and will not round it " + f"to a safe-looking one. Upgrade the SDK. (error_code=" + f"{body.get('error_code')!r})", + wire_category=raw, + error_code_wire=body.get("error_code"), + ) from exc diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index 9597589..cfbd7e3 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -96,6 +96,7 @@ AuditQuery, AuditVerifyResult, ) +from nullrun.breaker.categories import NullRunUnclassifiedRefusalError from nullrun.breaker.exceptions import ( NullRunApprovalDeniedError, NullRunApprovalExpiredError, @@ -557,7 +558,6 @@ def export_status( # before. from nullrun._singleton import _NullRunRuntimeMeta - class NullRunRuntime(metaclass=_NullRunRuntimeMeta): """ Central runtime for NullRun SDK. @@ -627,7 +627,6 @@ def __init__( control-plane listener (WS or HTTP poll). Defaults True in production. Set False when the test environment cannot tolerate a background thread opening sockets. - Note: - `organization_id` is set from `_authenticate ` after init; it is NOT a public init parameter and not read from env. @@ -2321,6 +2320,15 @@ def check_workflow_budget(self) -> None: # refused. Classification is by TYPE here, never by # inspecting the message. raise + except NullRunUnclassifiedRefusalError: + # ADR-062 §2.2. The gate DID answer — it refused — + # and the SDK cannot tell a policy decision from a + # backend fault. That is not "gate unavailable": + # failing OPEN here would read an unclassifiable + # refusal as "allowed", which is the exact hole the + # category work closes. Must precede the arm below + # (a `NullRunError` superset). + raise except (httpx.HTTPError, NullRunError) as exc: # Narrow catch: fail-OPEN only on transport + # classified SDK errors. Internal bugs @@ -2359,6 +2367,13 @@ def check_workflow_budget(self) -> None: # broad `except Exception` below, which is a superset # of this arm. self._raise_malformed_gate_response(exc) + except NullRunUnclassifiedRefusalError: + # ADR-062 §2.2 — same rationale as the cached branch. + # This is a refusal the SDK cannot classify, not an + # unreachable gate, so it must not fail open. It is + # listed explicitly because the arm below is a bare + # ``except Exception``. + raise except Exception as exc: # noqa: BLE001 logger.warning(f"check_workflow_budget: /gate unavailable, failing open: {exc}") metrics.inc_runtime("gate_fail_open_total") diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index 49e8ca3..c9ae972 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -23,6 +23,7 @@ import httpx from nullrun.actions import handle_action +from nullrun.breaker.categories import is_gate_refusal, resolve_refusal_category from nullrun.breaker.circuit_breaker import CircuitBreaker from nullrun.breaker.exceptions import ( BreakerTransportError, @@ -1624,15 +1625,54 @@ def _do_gate_post() -> httpx.Response: # ``details`` are preserved so the catalogue formatter # can produce an actionable message. # - # DEF-NR-TOOLBLOCKED-PARSER: the dedicated parser - # branch below translates the typed v3 envelope into a - # `NullRunToolBlockedError` (catalog code NR-T001) - # instead of the generic NR-X001 fallback. - if 400 <= response.status_code < 500: - try: - wire_body = response.json() - except Exception: - wire_body = {} + # A gate refusal is a refusal whatever the status. + # ADR-063 §4.7 (product decision: option 2) — the + # backend's 503s split into two groups and the split is + # what the category records: + # + # * "the answer is not available right now" + # (BUDGET_DATA_UNAVAILABLE, is_fail_closed=false) + # * "the CHECK could not be performed" + # (CIRCUIT_BREAKER_STATE_LOOKUP_FAILED, + # WORKFLOW_INACTIVE_LOOKUP_FAILED, + # RATE_LIMIT_PLAN_LOOKUP_FAILED, is_fail_closed=true) + # + # Both arrive as 503 with decision="block", and the gate + # already refuses either way (fail-CLOSED, CLAUDE.md §4). + # Pre-fix the entire 5xx band fell through to the + # synthetic FALLBACK block below, which the runtime + # reads as a transport error and fails OPEN — so a + # fail-CLOSED 503 refusal was silently converted into + # "allowed". That is DEF-MP-TS12-ENF-01's exact shape + # with a different trigger, and it is why a 5xx body + # that is a genuine refusal is handled here instead. + # + # Old SDKs are unaffected by definition: they never + # looked at `category`, and they read the 503 as a + # transport error. They keep failing open, which is the + # documented pre-0.19.0 behaviour. + try: + wire_body = response.json() + except Exception: + wire_body = {} + # ADR-062 §2.2. Classify the refusal BEFORE building + # the synthetic response dict, and let an + # unclassifiable one escape as + # ``NullRunUnclassifiedRefusalError`` rather than + # being flattened into the generic block below. + # + # The dict this branch returns hardcodes + # ``decision="block"`` for every 4xx, including ones + # that were never gate refusals (a 400 protocol + # mismatch has no ``decision`` field at all). + # ``resolve_refusal_category`` discriminates on the + # WIRE body, not on the status, so those return + # ``None`` and keep their pre-existing handling. + is_refusal = is_gate_refusal(wire_body) + if 400 <= response.status_code < 500 or ( + response.status_code >= 500 and is_refusal + ): + category = resolve_refusal_category(wire_body) explanations = wire_body.get("explanations") or [] if not explanations: single = ( @@ -1648,6 +1688,19 @@ def _do_gate_post() -> httpx.Response: "decision_source": DecisionSource.GATEWAY, "explanation": explanations[0], "explanations": explanations, + # ADR-062 §2.2 — ``None`` for a 4xx that was + # never a gate refusal, a real + # ``DecisionCategory`` for one that was. Carried + # rather than re-derived so the runtime's raise + # site branches on the server's own classification + # instead of inferring one from the status code. + "category": category, + # Server-authored text. ``agent_message`` is + # populated by the backend only for ``denied``; + # its absence on the other three is the server + # stating "this is not the model's to read". + "agent_message": wire_body.get("agent_message"), + "user_message": wire_body.get("user_message"), "reservation_id": wire_body.get("reservation_id"), "remaining_budget_cents": wire_body.get("remaining_budget_cents") or 0, "projected_cost_cents": wire_body.get("projected_cost_cents") or 0, @@ -2707,6 +2760,14 @@ def _parse_v3_error_envelope( Mapping table lives at ``_V3_ERROR_CODE_MAP`` below — keep the helper as a thin dispatcher. + + DEF-NR-TOOLBLOCKED-PARSER: the ``catalog is + NullRunToolBlockedError or catalog is NullRunBlockedException`` + arm further down is the dedicated branch for the typed v3 + envelope. It used to be documented by a comment inside + ``Transport.check``, which never had such a branch — the tag was + filed against the wrong function for as long as it existed. + Re-filed here, on the function that actually holds the code. """ # Lazy imports: the exception classes import the transport # types (TransportErrorSource), so a top-level import here diff --git a/tests/test_2026_09_10_check_failopen.py b/tests/test_2026_09_10_check_failopen.py index d17c8fb..904e34d 100644 --- a/tests/test_2026_09_10_check_failopen.py +++ b/tests/test_2026_09_10_check_failopen.py @@ -74,6 +74,28 @@ def _check_body() -> str: return src[start:end] +# Mirrors ``GateErrorCode::category()`` on the backend +# (``backend/src/proxy/http/gate/error_codes.rs``). Only the codes +# this file's fixtures use are listed; an unlisted code resolves to +# no ``category`` at all, which the SDK treats as unclassifiable — +# that is the honest outcome for a code the fixture does not model. +_CATEGORY_FOR_CODE = { + "BUDGET_HARD_BLOCKED": "budget", + "BUDGET_WORKFLOW_BLOCKED": "budget", + "BUDGET_CACHE_EXCEEDED": "budget", + "BUDGET_OVERDRAFT_EXCEEDED": "budget", + "RATE_LIMIT_EXCEEDED": "budget", + "TOOL_BLOCKED": "denied", + "APPROVAL_DENIED": "denied", + "LOOP_DETECTED": "denied", + "WORKFLOW_INACTIVE": "halt", + "WORKFLOW_PAUSED": "halt", + "CIRCUIT_BREAKER_TRIPPED": "halt", + "BUDGET_DATA_UNAVAILABLE": "infra", + "CIRCUIT_BREAKER_STATE_LOOKUP_FAILED": "infra", +} + + def _v3_envelope( error_code: str, status: int = 402, @@ -85,10 +107,22 @@ def _v3_envelope( reservation_id: str | None = None, operation_id: str | None = None, policy_version: int | None = None, + category: str | None = None, **details, ) -> httpx.Response: """Build a v3-shaped 4xx response envelope mirroring the real - backend's wire contract.""" + backend's wire contract. + + ADR-062 §2.2: a real gate refusal always carries ``category``, + and ``agent_message`` only when the category is ``denied`` + (``gate.rs::attach_refusal_surface``). The fixture reproduces + that so these tests exercise the wire the backend actually + sends — an earlier version omitted it, which the SDK now + correctly refuses to classify. ``_CATEGORY_FOR_CODE`` mirrors + the backend's own classification + (``error_codes.rs::DecisionCategory``). + """ + resolved = category if category is not None else _CATEGORY_FOR_CODE.get(error_code) body = { "decision": "block", "decision_source": DecisionSource.GATEWAY, @@ -103,6 +137,11 @@ def _v3_envelope( "projected_cost_cents": projected_cost_cents, "details": details, } + if resolved is not None: + body["category"] = resolved + body["user_message"] = f"operator note for {error_code}" + if resolved == "denied": + body["agent_message"] = f"That tool is not permitted ({error_code})." return httpx.Response(status, json=body) @@ -145,11 +184,13 @@ def test_4xx_branch_uses_gateway_not_fallback(self): fallback and the runtime fail-OPENed.""" body = _check_body() # Locate the 4xx branch by its comment marker - idx = body.find("if 400 <= response.status_code < 500:") + idx = body.find("if 400 <= response.status_code < 500 or (") assert idx != -1, ( "DEF-NR-CHECK-FAIL-OPEN: 4xx branch anchor " - "`if 400 <= response.status_code < 500:` not found in " - "Transport.check" + "`if 400 <= response.status_code < 500 or (` not found in " + "Transport.check. ADR-063 §4.7 widened the condition to " + "cover 5xx bodies that are genuine gate refusals, so the " + "anchor moved with it." ) # Slice only the 4xx branch (stop at the next sibling `if` # for the 5xx fallthrough). @@ -179,7 +220,7 @@ def test_4xx_branch_preserves_wire_envelope(self): ``remaining_budget_cents``, ``details``) so the runtime's catalog dispatcher can build an actionable exception.""" body = _check_body() - idx = body.find("if 400 <= response.status_code < 500:") + idx = body.find("if 400 <= response.status_code < 500 or (") assert idx != -1 five_xx_marker = body.find("if response.status_code >= 500", idx) assert five_xx_marker != -1 diff --git a/tests/test_2026_09_10_toolblocked_parser.py b/tests/test_2026_09_10_toolblocked_parser.py index ec1252c..8cd7487 100644 --- a/tests/test_2026_09_10_toolblocked_parser.py +++ b/tests/test_2026_09_10_toolblocked_parser.py @@ -195,16 +195,33 @@ def test_import_includes_blocked_exception_classes(self): ) def test_branch_comment_tag_present(self): - """The fix introduced a long comment naming + """The fix introduced a comment naming DEF-NR-TOOLBLOCKED-PARSER. Pin so a future maintainer who deletes the comment is forced to read the code's - history.""" + history. + + The tag must live in ``_parse_v3_error_envelope``, the + function that holds the branch. It used to sit in + ``Transport.check``, which never had a parser branch at all — + so the comment named a fix the reader could not find. The + pin now asserts the tag is attached to the right function, + which is the property that was actually broken. + """ src = _read(TRANSPORT_PY) assert "DEF-NR-TOOLBLOCKED-PARSER" in src, ( "DEF-NR-TOOLBLOCKED-PARSER: the explainer comment " "block must name the fix tag so future readers can " "grep for it." ) + body = src.split("def _parse_v3_error_envelope(")[1] + head = body[: body.find('"""', body.find('"""') + 3)] + assert "DEF-NR-TOOLBLOCKED-PARSER" in head, ( + "DEF-NR-TOOLBLOCKED-PARSER: the tag must be documented on " + "_parse_v3_error_envelope, which is where the dedicated " + "NullRunToolBlockedError branch actually lives. Filing it " + "against Transport.check pointed readers at a branch that " + "was never there." + ) def test_branch_uses_correct_constructor_signature(self): """The dedicated branch must call diff --git a/tests/test_auth_fail_closed.py b/tests/test_auth_fail_closed.py index c62a058..79b3b27 100644 --- a/tests/test_auth_fail_closed.py +++ b/tests/test_auth_fail_closed.py @@ -104,6 +104,13 @@ def test_enforcement_4xx_still_raises_budget_error( "decision_source": "gateway", "explanation": "BUDGET_WORKFLOW_BLOCKED", "error_code": "BUDGET_WORKFLOW_BLOCKED", + # ADR-062 §2.2 — a real refusal always carries a + # category; the backend classifies this one + # ``budget``. Omitting it made the SDK raise + # NullRunUnclassifiedRefusalError instead of + # reaching the budget arm this test pins. + "category": "budget", + "user_message": "raise the workflow budget", "details": {"max_budget_cents": 100}, }, ) diff --git a/tests/test_refusal_categories.py b/tests/test_refusal_categories.py new file mode 100644 index 0000000..19e916f --- /dev/null +++ b/tests/test_refusal_categories.py @@ -0,0 +1,245 @@ +"""ADR-062 §2.2 refusal categories and the `on_denied` opt-in. + +The gate does not send a bare "no" — it sends *why*, in exactly four +values (`DecisionCategory` on the backend). The SDK's job is to +classify the refusal, and the rule that matters is what it does when +it *cannot*: + + An absent or unrecognised category RAISES. It is never guessed. + +Guessing is the dangerous direction, and specifically: if the SDK +rounds an unknown refusal to something permissive, a `budget` or +`halt` refusal can be rendered as a friendly, model-readable "that's +not allowed" — and an agent handed that will adapt and retry against +a stop an operator deliberately placed. That is the bypass ADR-061 +closed on the server side, re-opened client-side. + +""" + +from __future__ import annotations + +import httpx +import pytest +import respx + +from nullrun.breaker.categories import ( + DecisionCategory, + NullRunUnclassifiedRefusalError, + is_gate_refusal, + resolve_refusal_category, +) +from nullrun.breaker.exceptions import ( + NullRunBlockedException, + NullRunBudgetError, + NullRunError, + NullRunInfrastructureError, +) +from nullrun.runtime import NullRunRuntime +from nullrun.transport import Transport + +BASE_URL = "https://api.test.nullrun.io" +GATE_URL = f"{BASE_URL}/api/v1/gate" + +_CHECK_KWARGS = { + "check_request": { + "organization_id": "ws-123", + "workflow_id": "wf-" + "a" * 32, + "trace_id": "trace-789", + "tool": "read_file", + }, + "on_transport_error": "raise", +} + +# The categories the backend actually emits, with the error code it +# pairs with in production. Kept as data so the table itself is +# readable next to the assertions that consume it. +REAL_REFUSALS = { + # code: (category, http status) — both read off the backend's + # own `GateErrorCode` (category() / http_status()), not guessed. + "TOOL_BLOCKED": ("denied", 403), + "APPROVAL_DENIED": ("denied", 403), + "LOOP_DETECTED": ("denied", 403), + "MCP_DESTRUCTIVE_BLOCKED": ("denied", 403), + "BUDGET_HARD_BLOCKED": ("budget", 402), + "BUDGET_WORKFLOW_BLOCKED": ("budget", 402), + "RATE_LIMIT_EXCEEDED": ("budget", 429), + "WORKFLOW_PAUSED": ("halt", 403), + "WORKFLOW_INACTIVE": ("halt", 403), + "CIRCUIT_BREAKER_TRIPPED": ("halt", 403), + # The two ADR-063 §4.7 503 groups. Both arrive as 503 with + # decision="block"; the category is what separates them. + "BUDGET_DATA_UNAVAILABLE": ("infra", 503), + "CIRCUIT_BREAKER_STATE_LOOKUP_FAILED": ("infra", 503), +} + + +def _refusal( + error_code: str, + category: str | None, + *, + status: int = 402, + agent_message: str | None = None, +) -> httpx.Response: + """A gate refusal body shaped like the real wire. + + `agent_message` is only populated for `denied`, because that is + the only category the backend writes model-safe text for + (`gate.rs::attach_refusal_surface`). Callers pass + `category=None` to model the NR-005 path, where no code resolved + and the backend therefore sent no category at all. + """ + body: dict = { + "decision": "block", + "decision_source": "gateway", + "explanation": error_code, + "error_code": error_code, + "user_message": f"operator note for {error_code}", + } + if category is not None: + body["category"] = category + if category == "denied": + body["agent_message"] = agent_message or f"{error_code} is not permitted." + return httpx.Response(status, json=body) + + +class TestCategoryParsing: + def test_only_denied_is_model_message_safe(self): + """The safety property, stated as a table. + + `agent_message` is the only field allowed to reach a model. + It exists on the wire for exactly one category, and this + predicate is the only thing that says so on this side. + """ + safe = {c for c in DecisionCategory if c.is_model_message_safe()} + assert safe == {DecisionCategory.DENIED} + assert len(DecisionCategory) == 4, "a fifth category needs SDK work" + + def test_absent_category_raises(self): + with pytest.raises(NullRunUnclassifiedRefusalError) as exc: + resolve_refusal_category({"decision": "block", "error_code": "TOOL_BLOCKED"}) + err = exc.value + assert err.wire_category is None + assert err.wire_error_code == "TOOL_BLOCKED", ( + "the operator needs the code to report the drift" + ) + assert err.error_code == "NR-P003" + assert isinstance(err, NullRunInfrastructureError), ( + "an unclassifiable refusal is a system fault, not a policy outcome — " + "host code branches on the marker classes" + ) + + def test_unrecognised_category_raises_rather_than_rounding(self): + """The rounding failure mode, pinned. + + A permissive `except ValueError: return INFRA` would pass a + test that only checks "it raised something", and would make + every future category silently read as a backend fault. + """ + with pytest.raises(NullRunUnclassifiedRefusalError) as exc: + resolve_refusal_category({"decision": "block", "category": "teapot"}) + assert exc.value.wire_category == "teapot" + + def test_non_string_category_raises(self): + with pytest.raises(NullRunUnclassifiedRefusalError): + resolve_refusal_category({"decision": "block", "category": 42}) + + @pytest.mark.parametrize("category", sorted({c for c, _ in REAL_REFUSALS.values()})) + def test_every_real_category_parses(self, category): + assert resolve_refusal_category( + {"decision": "block", "category": category} + ) is DecisionCategory(category) + + def test_non_refusal_bodies_are_left_alone(self): + """Strictness is scoped to refusals. + + A protocol mismatch, an admin 422, a heartbeat 404 — none + carry a refusal category, and turning them into + infrastructure faults would be its own bug. + """ + for body in ( + {}, + {"error_code": "PROTOCOL_TOO_OLD"}, + {"decision": "allow"}, + {"decision": "soft_pass"}, + {"decision": "require_approval"}, + "not even a dict", + ): + assert is_gate_refusal(body) is False + assert resolve_refusal_category(body) is None + + +class TestTransportBoundary: + """The classification happens once, at the wire.""" + + def test_absent_category_escapes_check(self): + t = Transport(api_url=BASE_URL, api_key="test-key-12345678") + with respx.mock(assert_all_called=False) as mock: + mock.post(GATE_URL).mock(return_value=_refusal("BUDGET_HARD_BLOCKED", None)) + with pytest.raises(NullRunUnclassifiedRefusalError): + t.check(**_CHECK_KWARGS) + + def test_category_and_text_are_carried_through(self): + t = Transport(api_url=BASE_URL, api_key="test-key-12345678") + with respx.mock(assert_all_called=False) as mock: + mock.post(GATE_URL).mock( + return_value=_refusal("TOOL_BLOCKED", "denied", status=403) + ) + result = t.check(**_CHECK_KWARGS) + assert result["category"] is DecisionCategory.DENIED + assert result["agent_message"] + + def test_non_refusal_4xx_carries_no_category(self): + """A 4xx with no `decision` field keeps its old handling.""" + t = Transport(api_url=BASE_URL, api_key="test-key-12345678") + with respx.mock(assert_all_called=False) as mock: + mock.post(GATE_URL).mock( + return_value=httpx.Response(400, json={"error_code": "PROTOCOL_TOO_OLD"}) + ) + result = t.check(**_CHECK_KWARGS) + assert result["decision"] == "block" + assert result["category"] is None + + +class TestFailOpenDoesNotSwallowIt: + """The hole the whole feature exists to close. + + `check_workflow_budget` has two `except` arms that fail OPEN + (ADR-008). An unclassifiable refusal is neither a transport + failure nor an auth failure — the gate answered — so if either + arm catches it, the caller gets `None` and the agent proceeds on + a call the backend refused. That is DEF-MP-TS12-ENF-01's shape, + with a different trigger. + """ + + def test_cached_branch_does_not_fail_open(self, make_runtime, mock_api): + import uuid + + from nullrun.context import chain + + respx.post(GATE_URL).mock( + return_value=_refusal("BUDGET_HARD_BLOCKED", None, status=402) + ) + rt = make_runtime() + with chain(str(uuid.uuid4())): + with pytest.raises(NullRunUnclassifiedRefusalError): + rt.check_workflow_budget() + + def test_uncached_branch_does_not_fail_open(self, make_runtime, mock_api): + respx.post(GATE_URL).mock( + return_value=_refusal("BUDGET_HARD_BLOCKED", None, status=402) + ) + rt = make_runtime() + with pytest.raises(NullRunUnclassifiedRefusalError): + rt.check_workflow_budget() + + def test_real_transport_failure_still_fails_open(self, make_runtime, mock_api): + """Counter-test: the fail-OPEN policy itself is untouched. + + ADR-008 promises a dead backend does not freeze the agent. + Narrowing it for refusals must not narrow it for outages. + """ + respx.post(GATE_URL).mock( + side_effect=httpx.ConnectError("connection refused") + ) + rt = make_runtime() + assert rt.check_workflow_budget() is None From 4cb009c3289b13ce0a4f57a5031b4fd10f2b8a84 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 21:38:11 +0400 Subject: [PATCH 07/20] feat(breaker): add on_denied, a message path for `denied` and nothing else MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `denied` refusal is the one category where telling the model is both useful and safe: a rule judged *this call* unacceptable, where another call or a different tool might pass. The other three describe a wall the model cannot climb, and a model told about a wall walks into it — it tool-shops after "your budget is exhausted", routes around a pause an operator deliberately placed, and tries to solve a backend fault that is not the model's problem. `on_denied="message"` therefore sits behind a single guard, `category is DecisionCategory.DENIED`, at the one block site in `check_workflow_budget`. No arrangement of the flag turns a budget, halt, or infra refusal into model-readable text — verified by mutation: widening the guard to consult `on_denied` alone turns the three negative cases and the budget-exception case red. The resulting `NullRunDeniedError` (NR-D001, non-retryable) carries `agent_message` — server-authored text the backend guarantees is model-safe (`attach_refusal_surface` sends it only for `Denied`) — behind one accessor, so a host cannot reach for `str(exc)` and put operator internals in the model's context by accident. The default is `"raise"`, and an unrecognised value is rejected at construction rather than treated as `"raise"`, so a typo cannot quietly disable the path a host asked for. `halt` and `infra` refusals still surface as `NullRunBudgetError`. Recorded, not fixed: the exception family does not yet match the category. It is a diagnostics defect, not a bypass — no model text is produced on either path and `on_denied` cannot reach them. --- src/nullrun/__init__.py | 15 +++++ src/nullrun/breaker/exceptions.py | 54 +++++++++++++++++ src/nullrun/runtime.py | 71 ++++++++++++++++++++++- tests/test_refusal_categories.py | 96 ++++++++++++++++++++++++++++++- 4 files changed, 234 insertions(+), 2 deletions(-) diff --git a/src/nullrun/__init__.py b/src/nullrun/__init__.py index a77cbfe..f79fefb 100644 --- a/src/nullrun/__init__.py +++ b/src/nullrun/__init__.py @@ -181,6 +181,7 @@ def init( api_url: str | None = None, debug: bool = False, fail_on_exit: bool = False, + on_denied: str = "raise", ): """ Initialize the NullRun SDK. Call once at application startup. @@ -207,6 +208,19 @@ def init( clean exit on missing ``NULLRUN_API_KEY``; library embedders (FastAPI startup, Jupyter) should leave it False and catch the exception instead. + on_denied: ``"raise"`` (default) or ``"message"``. ADR-062 + §2.2. When the gate refuses a call with + ``category="denied"`` — an operator rejected THIS call, + and a different one might pass — ``"message"`` raises + ``NullRunDeniedError``, whose ``agent_message`` is + text the backend guarantees is safe to show the model. + + The flag acts on ``denied`` and nothing else. A + ``budget`` / ``halt`` / ``infra`` refusal raises its own + typed exception either way, and a refusal the SDK cannot + classify at all raises + ``NullRunUnclassifiedRefusalError`` rather than being + guessed into one of them. Note: the background control-plane listener (WebSocket + HTTP poll) is always started on `init `. To disable it, construct `NullRunRuntime` @@ -336,6 +350,7 @@ def my_agent: api_key=api_key, api_url=api_url, debug=debug, + on_denied=on_denied, ) registry.set(runtime) diff --git a/src/nullrun/breaker/exceptions.py b/src/nullrun/breaker/exceptions.py index a5d72a6..31d97aa 100644 --- a/src/nullrun/breaker/exceptions.py +++ b/src/nullrun/breaker/exceptions.py @@ -802,6 +802,60 @@ def __init__( ) +class NullRunDeniedError(NullRunBlockedException): + """ADR-062 ``denied``: an operator refused **this call** on policy. + + Raised only when the host opted in with + ``nullrun.init(on_denied="message")`` and the gate answered with + ``category == "denied"``. That is the single condition: a + ``budget``, ``halt``, or ``infra`` refusal never becomes this + class no matter what the flag says, because those three describe + walls the model cannot climb, and a model handed a friendly + sentence about one will tool-shop and retry. + + The distinction this class buys the host is + :attr:`agent_message` — server-authored text the backend + guarantees is safe to place in the model's context. It is + populated from the wire field of the same name, which the + backend sets **only** for ``denied`` + (``gate.rs::attach_refusal_surface``). When the server sent no + such text the attribute is ``None`` and the host must not + substitute its own wording. + + With the default ``on_denied="raise"`` a denial surfaces as the + ordinary code-specific exception (``NullRunToolBlockedError`` and + friends) and this class is never raised — the opt-in is what + changes the shape of the refusal, not the enforcement. + """ + + error_code = "NR-D001" + user_action = ( + "The operator's policy refused this specific call. Adjust the call " + "(different tool, different arguments) or ask them to widen the " + "policy — no other call is affected by this refusal." + ) + retryable = False + + def __init__( + self, + *args: Any, + agent_message: str | None = None, + **kwargs: Any, + ) -> None: + self.agent_message = agent_message + super().__init__(*args, **kwargs) + + def model_safe_text(self) -> str | None: + """The server's model-safe text, or ``None`` if it sent none. + + The single accessor, so a host cannot reach for + ``str(exc)`` — which mixes in the endpoint, the HTTP status + and the error code, none of which are model-safe — and get + operator internals into the model's context by accident. + """ + return self.agent_message + + class NullRunBudgetError(NullRunBlockedException): """Budget exhausted — every cost-bearing call will be rejected. diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index cfbd7e3..70b772f 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -96,7 +96,10 @@ AuditQuery, AuditVerifyResult, ) -from nullrun.breaker.categories import NullRunUnclassifiedRefusalError +from nullrun.breaker.categories import ( + DecisionCategory, + NullRunUnclassifiedRefusalError, +) from nullrun.breaker.exceptions import ( NullRunApprovalDeniedError, NullRunApprovalExpiredError, @@ -106,6 +109,7 @@ NullRunBackendError, NullRunBlockedException, NullRunBudgetError, + NullRunDeniedError, NullRunError, NullRunInfrastructureError, NullRunTransportError, @@ -558,6 +562,12 @@ def export_status( # before. from nullrun._singleton import _NullRunRuntimeMeta +#: The two accepted ``on_denied`` values. A frozenset rather than a +#: literal ``in`` chain so the constructor's error message and any +#: future call site cannot drift apart on the vocabulary. +_ON_DENIED_VALUES = frozenset({"raise", "message"}) + + class NullRunRuntime(metaclass=_NullRunRuntimeMeta): """ Central runtime for NullRun SDK. @@ -612,6 +622,8 @@ def __init__( # Tune the httpx read timeout for slow-network scenarios. # Precedence: kwarg > NULLRUN_REQUEST_TIMEOUT env var > 30.0. request_timeout: float | None = None, + # ADR-062 §2.2. "raise" (default) | "message". + on_denied: str = "raise", ): """ Initialize NullRun Runtime. @@ -627,6 +639,26 @@ def __init__( control-plane listener (WS or HTTP poll). Defaults True in production. Set False when the test environment cannot tolerate a background thread opening sockets. + on_denied: What to do with a ``category="denied"`` refusal. + ``"raise"`` (default) keeps current behaviour — the + code-specific exception propagates and the host + decides what to show. ``"message"`` raises + ``NullRunDeniedError``, whose ``agent_message`` is + server-authored text the backend guarantees is safe + to place in the model's context. + + This flag acts on ``denied`` and on NOTHING else. + A ``budget`` / ``halt`` / ``infra`` refusal raises + its own typed exception whatever this is set to: + a model told "your budget is exhausted" tool-shops + and retries, a model told about a pause tries to + route around a stop an operator deliberately + placed, and a backend fault is not the model's + problem to solve at all. An unclassifiable refusal + (absent or unrecognised ``category``) raises + ``NullRunUnclassifiedRefusalError`` under both + values. + Note: - `organization_id` is set from `_authenticate ` after init; it is NOT a public init parameter and not read from env. @@ -638,6 +670,10 @@ def __init__( - `timeout`/`max_retries` are fixed at 30s / 3 (no public override). Raises: + ValueError: if ``on_denied`` is neither ``"raise"`` nor + ``"message"``. Rejected at construction rather than + silently treated as ``"raise"``, so a typo cannot + quietly disable the message path a host asked for. NullRunAuthenticationError: if neither `api_key` nor `NULLRUN_API_KEY` is set. The public `init ` surface performs the same check first and produces a clearer @@ -655,6 +691,17 @@ def __init__( self.secret_key = secret_key or os.getenv("NULLRUN_SECRET_KEY") self.api_url = api_url or os.getenv("NULLRUN_API_URL", "https://api.nullrun.io") + if on_denied not in _ON_DENIED_VALUES: + raise ValueError( + f"on_denied must be one of {sorted(_ON_DENIED_VALUES)!r}, got {on_denied!r}. " + "It selects the shape of a category='denied' refusal only — a budget / " + "halt / infra refusal raises its own exception either way." + ) + #: See the ``on_denied`` argument. Read only at the block + #: site in ``check_workflow_budget``, and only reached for + #: ``DecisionCategory.DENIED``. + self.on_denied = on_denied + # api_key is required — there is no fallback. if not self.api_key: raise NullRunAuthenticationError( @@ -2405,6 +2452,28 @@ def check_workflow_budget(self) -> None: reasons = response.get("explanations") or ( [response["explanation"]] if response.get("explanation") else ["block"] ) + # ADR-062 §2.2. ``Transport.check`` resolved the category + # at the wire boundary and already raised if it could + # not, so by the time a block reaches here ``category`` + # is either a real ``DecisionCategory`` or ``None`` for a + # 4xx that was never a gate refusal (a protocol + # mismatch, say — those keep the pre-existing handling). + # + # ``on_denied`` is consulted HERE and only here, behind + # ``category is DENIED``. That single guard is the whole + # safety property: no arrangement of the flag turns a + # budget, halt, or infra refusal into a message a model + # can read and act on. + category = response.get("category") + if category is DecisionCategory.DENIED and self.on_denied == "message": + raise NullRunDeniedError( + workflow_id=workflow_id, + reason="; ".join(reasons), + action="block", + decision_source=response.get("decision_source"), + reasons="; ".join(reasons), + agent_message=response.get("agent_message"), + ) # Bump ``cost_limit_exceeded`` when the pre-flight # blocks the workflow. The counter is the operator's # primary signal for "the budget cap is biting" -- diff --git a/tests/test_refusal_categories.py b/tests/test_refusal_categories.py index 19e916f..76f93fe 100644 --- a/tests/test_refusal_categories.py +++ b/tests/test_refusal_categories.py @@ -14,6 +14,11 @@ a stop an operator deliberately placed. That is the bypass ADR-061 closed on the server side, re-opened client-side. +The second half of the file pins the negative cases for +`on_denied="message"`: that flag acts on `denied` and on NOTHING +else. A budget exhaustion, a pause, a breaker trip, and a backend +fault all keep their own exceptions no matter how the host +configured the flag. """ from __future__ import annotations @@ -31,6 +36,7 @@ from nullrun.breaker.exceptions import ( NullRunBlockedException, NullRunBudgetError, + NullRunDeniedError, NullRunError, NullRunInfrastructureError, ) @@ -200,6 +206,91 @@ def test_non_refusal_4xx_carries_no_category(self): assert result["category"] is None +class TestOnDeniedIsDeniedOnly: + """`on_denied="message"` acts on `denied` and on nothing else.""" + + def test_denied_becomes_a_message_carrying_exception(self, make_runtime, mock_api): + respx.post(GATE_URL).mock( + return_value=_refusal("TOOL_BLOCKED", "denied", status=403) + ) + rt = make_runtime(on_denied="message") + with pytest.raises(NullRunDeniedError) as exc: + rt.check_workflow_budget() + assert exc.value.model_safe_text() == "TOOL_BLOCKED is not permitted." + assert isinstance(exc.value, NullRunBlockedException), ( + "existing `except NullRunBlockedException` handlers must keep matching" + ) + + @pytest.mark.parametrize("category", ["budget", "halt", "infra"]) + def test_non_denied_categories_never_become_a_message( + self, category, make_runtime, mock_api + ): + """The negative tests. + + For each non-`denied` category, pick a real error code of + that category and assert the refusal does NOT surface as + `NullRunDeniedError` — i.e. the host cannot read it as + something the model should be told. + """ + code, status = next( + (c, st) for c, (cat, st) in REAL_REFUSALS.items() if cat == category + ) + respx.post(GATE_URL).mock(return_value=_refusal(code, category, status=status)) + rt = make_runtime(on_denied="message") + with pytest.raises(NullRunError) as exc: + rt.check_workflow_budget() + assert not isinstance(exc.value, NullRunDeniedError), ( + f"{code} is category={category}; on_denied must not reach it" + ) + # And nothing model-readable rides along. + assert not isinstance(getattr(exc.value, "agent_message", None), str) + + def test_default_is_raise_and_denied_stays_a_plain_block( + self, make_runtime, mock_api + ): + respx.post(GATE_URL).mock( + return_value=_refusal("TOOL_BLOCKED", "denied", status=403) + ) + rt = make_runtime() + assert rt.on_denied == "raise" + with pytest.raises(NullRunError) as exc: + rt.check_workflow_budget() + assert not isinstance(exc.value, NullRunDeniedError) + + def test_absent_category_raises_even_with_message_enabled( + self, make_runtime, mock_api + ): + """The opt-in must not become a way to accept junk. + + `on_denied="message"` is a statement about `denied`. It is + not permission to guess a category. + """ + respx.post(GATE_URL).mock( + return_value=_refusal("TOOL_BLOCKED", None, status=403) + ) + rt = make_runtime(on_denied="message") + with pytest.raises(NullRunUnclassifiedRefusalError): + rt.check_workflow_budget() + + def test_unknown_on_denied_value_is_rejected_at_construction(self): + with pytest.raises(ValueError, match="on_denied"): + NullRunRuntime( + api_key="test-key-12345678", + api_url=BASE_URL, + polling=False, + on_denied="message-all", + ) + + def test_budget_category_keeps_its_own_exception(self, make_runtime, mock_api): + """Not just "not a message" — the right class, unchanged.""" + respx.post(GATE_URL).mock( + return_value=_refusal("BUDGET_HARD_BLOCKED", "budget", status=402) + ) + rt = make_runtime(on_denied="message") + with pytest.raises(NullRunBudgetError): + rt.check_workflow_budget() + + class TestFailOpenDoesNotSwallowIt: """The hole the whole feature exists to close. @@ -237,9 +328,12 @@ def test_real_transport_failure_still_fails_open(self, make_runtime, mock_api): ADR-008 promises a dead backend does not freeze the agent. Narrowing it for refusals must not narrow it for outages. + `on_denied="message"` is set deliberately: the host has asked + for the most permissive handling available, and it still must + not turn an unreachable gate into a block. """ respx.post(GATE_URL).mock( side_effect=httpx.ConnectError("connection refused") ) - rt = make_runtime() + rt = make_runtime(on_denied="message") assert rt.check_workflow_budget() is None From 408668a4d1eb3ea720f8c12998f79bf8f227f1e6 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 22:06:18 +0400 Subject: [PATCH 08/20] =?UTF-8?q?test(adr063):=20pin=20=C2=A74.7=20option?= =?UTF-8?q?=202=20=E2=80=94=20a=20503=20refusal=20blocks,=20an=20outage=20?= =?UTF-8?q?fails=20open?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Product decision, 2026-09-30. The previous version of this file pinned an asymmetry that was not a property of the categories but an accident of the status code: a 403 stopped the agent and a 503 did not, purely because `400 <= status < 500` happened to be the branch that produced a gateway decision. The rule now follows the failure's MEANING rather than its number. Ordinary unavailability stays fail-open; a failure OF THE CHECK ITSELF gets a marker the SDK treats as a block. SDKs predating the category work keep failing open, by definition — they never had a way to read the marker. The marker already existed (`category: "infra"`, and the backend already splits the two 503 groups via `GateErrorCode::is_fail_closed()`), so this was a client-side mapping change, not a re-architecture. Verified by mutation: reverting `Transport.check` to its pre-fix 4xx-only condition turns the 503 cases red with "synthetic decision_source='fallback', treating as transport error" — the gate's own fail-CLOSED refusal answered to the agent as "allowed". That was DEF-MP-TS12-ENF-01's exact shape with a different trigger, and it was wider than a single code path: the whole 5xx band was being converted into a synthetic transport failure. The 502 counter-test is what keeps ADR-008's promise intact. A 5xx with no refusal body is a proxy that never reached the gate, not a verdict, and must still fail open — otherwise every deploy would freeze every agent. --- ...adr063_infra_refusal_is_not_fail_closed.py | 125 ++++++++++++------ 1 file changed, 83 insertions(+), 42 deletions(-) diff --git a/tests/test_adr063_infra_refusal_is_not_fail_closed.py b/tests/test_adr063_infra_refusal_is_not_fail_closed.py index c706a88..d4044f3 100644 --- a/tests/test_adr063_infra_refusal_is_not_fail_closed.py +++ b/tests/test_adr063_infra_refusal_is_not_fail_closed.py @@ -1,5 +1,26 @@ """ADR-063 §1.3(f): what the SDK does with an `infra` refusal. +SUPERSEDED IN PART — ADR-063 §4.7 (product decision, option 2). + +The first version of this file pinned an asymmetry that was, on +inspection, not a property of the categories but an accident of the +status code: a 403 refusal stopped the agent and a 503 refusal did +not, purely because ``400 <= status < 500`` happened to be the branch +that produced a gateway decision. The product owner ruled on +2026-09-30 that the split should follow the failure's MEANING, not +its number: + + ordinary unavailability stays fail-open; a failure OF THE CHECK + ITSELF gets a marker the new SDK treats as a block. SDKs predating + the category work keep failing open. + +The marker already exists — it is ``category: "infra"``, and the +backend already distinguishes the two 503 groups internally via +``GateErrorCode::is_fail_closed()``. So this was a client-side +mapping change, not a re-architecture: a 5xx body that is a genuine +gate refusal (``decision == "block"``) is now classified and blocks, +while a 5xx that is a real outage still fails open. + The backend now answers a failed workflow-state read with **503** (`WORKFLOW_INACTIVE_LOOKUP_FAILED` / `CIRCUIT_BREAKER_STATE_LOOKUP_FAILED`, category `infra`) rather than the 500 it used to ship, and answers a @@ -13,23 +34,24 @@ becomes a real `decision="block"` with `decision_source=GATEWAY`. `check_workflow_budget` honours it and raises. The agent stops. * **503** is `>= 500`, so `_retry_with_backoff` retries it - (`retry_on_5xx=True`, `max_retries=3`), then the transport - synthesises `decision="block"` with `decision_source=FALLBACK`. - `check_workflow_budget` calls `is_fallback_decision_source(...)` on - that and **returns without raising** — a fail-OPEN, per ADR-008's + (`retry_on_5xx=True`, `max_retries=3`). The transport then asks + whether the body is a genuine refusal. If it is, the refusal is + classified and returned with `decision_source=GATEWAY`, and the + agent stops. If it is not — a proxy 502, a gateway that never + reached the gate — the synthetic `decision_source=FALLBACK` block + still applies and `check_workflow_budget` fails OPEN, per ADR-008's documented "dead backend must not freeze the agent" rule. -So on the current SDK a tripped breaker stops the agent and a failed -state read does not. Both are correct *for their category*: the -breaker genuinely stopped the workflow, the read failure did not stop -anything. But it means the pause work must NOT rely on a 5xx to halt -an agent — a `WORKFLOW_PAUSED` shipped as 503 would be an allow on -every SDK released to date. - -These tests pin that asymmetry so it cannot change silently, in either -direction. If a future SDK makes 5xx fail-CLOSED, the 503 tests go -red and the ADR gets amended; if one accidentally lets a 403 through -as a fallback, the 403 test goes red. +So a tripped breaker stops the agent, a failed state read stops the +agent, and an outage does not. The last one is the load-bearing +distinction: the gate's own answer is always honoured, and only the +absence of an answer fails open. + +These tests pin that so it cannot change silently, in either +direction. If a future SDK fails open on a 503 refusal, the 503 tests +go red; if one lets a 403 through as a fallback, the 403 test goes +red; if the fail-OPEN on a genuine outage is lost, the outage test +goes red. """ from __future__ import annotations @@ -38,7 +60,8 @@ import pytest import respx -from nullrun.breaker.exceptions import NullRunBudgetError +from nullrun.breaker.categories import NullRunUnclassifiedRefusalError +from nullrun.breaker.exceptions import NullRunBudgetError, NullRunError BASE_URL = "https://api.test.nullrun.io" GATE_URL = f"{BASE_URL}/api/v1/gate" @@ -62,9 +85,9 @@ def _infra_503() -> httpx.Response: ADR-064 §Correction records an earlier version of these fixtures that put `error_code` at the top level; they were built from a hand-assembled sample rather than a captured response. The mistake - is load-bearing, not cosmetic: ADR-064's SDK-side plan reads this - body, and a client written against the wrong nesting classifies - every refusal as unparseable -- which is fail-OPEN. + is load-bearing, not cosmetic: a client written against the wrong + nesting classifies every refusal as unparseable -- which is + fail-OPEN. """ return httpx.Response( 503, @@ -115,39 +138,56 @@ def _breaker_trip_403() -> httpx.Response: class TestInfraRefusalIsNotFailClosed: - """503 must NOT be read as "allowed" — but it currently is. + """A 503 the GATE answered is not an outage. - The test asserts the *actual* behaviour, fail-OPEN included, and - says so in the failure message. Asserting the desired behaviour - here would ship a red test; asserting "it raises" would be a lie. - The point is that the asymmetry is now a pinned, visible fact - rather than something discovered during an incident. + ADR-063 §4.7 option 2. The distinction is whether an answer + exists: a refusal body means the gate made a decision and it + stands; a body-less 5xx means it never got to one, and ADR-008's + fail-OPEN applies. """ - def test_503_state_read_failure_fails_open_today(self, make_runtime, mock_api): - """A failed state read currently lets the call through. + def test_503_state_read_failure_blocks(self, make_runtime, mock_api): + """A failed state read is the gate's own answer — honour it. - Pinned deliberately. ADR-008's fail-OPEN on transport error is - the documented policy and is not being changed here, but the - consequence — `infra` refusals do not stop an agent — must be - a known quantity before the pause work builds on 5xx. + Before §4.7 this returned without raising, so a fail-CLOSED + 503 from the backend was converted into "allowed" by the + status code alone. The gate blocks in both 503 groups + (fail-CLOSED, CLAUDE.md §4); the SDK now stops too. """ respx.post(GATE_URL).mock(return_value=_infra_503()) rt = make_runtime() - # Must NOT raise. If a future release makes this fail-CLOSED - # this test goes red, and ADR-008 + ADR-063 §1.3(f) get - # amended in the same commit. - rt.check_workflow_budget() # noqa: B018 - the absence of a raise IS the assertion + with pytest.raises(NullRunError) as exc_info: + rt.check_workflow_budget() + assert not isinstance(exc_info.value, NullRunUnclassifiedRefusalError), ( + "the body carries category=infra, so it is classifiable — an " + "unclassified-refusal here would mean the category was not read" + ) + + def test_503_outage_with_no_refusal_still_fails_open(self, make_runtime, mock_api): + """The counter-test, and the reason the rule is narrow. - def test_503_is_retried_before_failing_open(self, make_runtime, mock_api): - """The 503 is not a first-try fail-open. + A 5xx that is NOT a gate refusal — no `decision` field, the + shape a proxy or an unreachable gateway produces — must still + fail open. Widening "honour the answer" to "raise on any + 5xx" would freeze every agent on every deploy. + """ + respx.post(GATE_URL).mock( + return_value=httpx.Response(502, text="Bad Gateway") + ) + rt = make_runtime() + assert rt.check_workflow_budget() is None + + def test_503_is_retried_before_the_decision(self, make_runtime, mock_api): + """The 503 is not answered on the first try. `_retry_with_backoff(retry_on_5xx=True, max_retries=3)` means a - transient 503 gets three more attempts, which is what makes the + transient 503 gets three more attempts, which is what makes fail-OPEN tolerable for a rolling deploy. Pinning the attempt count stops a future "reduce retries on 5xx" change from - quietly making gate calls flakier under load. + quietly making gate calls flakier under load — and stops a + "skip the retry, block immediately" change from turning a blip + into a stopped agent. """ calls: list[httpx.Request] = [] @@ -157,11 +197,12 @@ def _count(request: httpx.Request) -> httpx.Response: respx.post(GATE_URL).mock(side_effect=_count) rt = make_runtime() - rt.check_workflow_budget() + with pytest.raises(NullRunError): + rt.check_workflow_budget() assert len(calls) > 1, ( - "a 503 must be retried, not failed open on the first " - f"response — only {len(calls)} attempt(s) were made" + f"a 503 must be retried before the block stands — only " + f"{len(calls)} attempt(s) were made" ) def test_403_breaker_trip_stops_the_agent(self, make_runtime, mock_api): From d191a7fb44ea974930e856ab5e4be415c7a9232f Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 09:06:30 +0400 Subject: [PATCH 09/20] fix(breaker): carry the category and the agent message onto /execute refusals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `on_denied` and the ADR-062 §2.2 category rule were only ever wired into `check_workflow_budget`, which calls `/gate`. Every MCP tool call goes through `runtime.execute` → `/api/v1/execute` instead, so on that path the flag was inert: a host that set `on_denied="message"` got a promise it did not keep for the whole class of tool calls that MCP adapters mediate, and the server-authored `agent_message` never reached it. Two independent pieces were missing on `/execute`. `_extract_error_envelope` had no arm for a gate refusal envelope. A real refusal puts `error_code` at `details.error_code` (backend `internal.rs:716`), not at the top level, so a `/execute` 403 TOOL_BLOCKED fell through to the legacy slug arm and arrived at the host as `NullRunAuthenticationError: Auth failed on execute (status 403, error_code='')`. Wrong class, wrong `format_user_message`, and an operator action — "check your credentials" — that cannot fix a policy decision. Added the missing envelope shape. Exceptions built by the parser carried neither the category nor the server's text. Split `_parse_v3_error_envelope` into a thin wrapper over `_parse_v3_error_envelope_uncategorised` so all ~15 return points stamp `wire_category` (left ABSENT when the body has no `category` — never defaulted, or a missing field would become a confident answer), `wire_error_code`, and the server-authored `agent_message` / `user_message`. Then `runtime.execute` converts a `NullRunBlockedException` to `NullRunDeniedError` under the same single-guard discipline as the `/gate` block site: `on_denied` is consulted only for `DecisionCategory.DENIED`, read from the wire. No arrangement of the flag turns a budget, halt, or infra refusal into model-readable text, and an absent category still raises. Test notes, since two of them are not what they first look like. `infra` is absent from the negative parameter set on purpose. ADR-063 §4.7 sends infra refusals as 503, and `_retry_with_backoff` maps the whole 5xx band on `/execute` to NullRunTransportError/GATEWAY_ERROR before the envelope parser is reached; the runtime then raises a STRICT fallback block at `runtime.py:3670`, downstream of the `on_denied` arm. The property is real and is asserted, but it is enforced by the 5xx band and not by the category check — so the test pins the concrete class to say which mechanism is in play. The negatives originally used a synthetic `SOME_CODE`, which the parser maps to a non-block class, so the exceptions never entered the arm and all three parameters passed regardless of the guard. Widening the guard to consult `on_denied` alone left them green. They now use real codes verified to map into `NullRunBlockedException`. All three mutations are recorded in the test docstrings. The `DEF-NR-TOOLBLOCKED-PARSER` source pins are repointed in the same commit because they are not separable from the parser split — the split alone leaves the suite red, and splitting them across commits would violate the one-green-commit-per-step rule. They resolved the parser by name, which after the split points at the wrapper and asserts nothing. They now resolve the dispatching function by content (the branch text, not the exception name: the name also appears in the import block, so resolving on it left `test_branch_in_parser` green after the branch was deleted — verified). Deleting the branch now turns 13 of 14 red. 1543 passed, 1 skipped. --- src/nullrun/runtime.py | 37 +- src/nullrun/transport.py | 100 ++++- tests/test_2026_09_10_toolblocked_parser.py | 123 ++++-- tests/test_mcp_refusal_categories.py | 391 ++++++++++++++++++++ 4 files changed, 608 insertions(+), 43 deletions(-) create mode 100644 tests/test_mcp_refusal_categories.py diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index 70b772f..b605fb8 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -3484,7 +3484,42 @@ def execute( execute_kwargs["business_impact"] = business_impact if action_digest is not None: execute_kwargs["action_digest"] = action_digest - result = self._transport.execute(**execute_kwargs) + try: + result = self._transport.execute(**execute_kwargs) + except NullRunBlockedException as exc: + # ADR-062 §2.2 / ``on_denied``. ``/execute`` is the OTHER + # enforcement path — the one every MCP tool call goes + # through — and it raises its exception inside + # ``Transport.execute`` rather than returning a decision + # dict, so the ``on_denied`` branch that lives at + # ``check_workflow_budget``'s block site never sees it. + # Without this, a host that set ``on_denied="message"`` + # gets a promise it does not keep for MCP tools, and the + # server-authored ``agent_message`` never reaches it. + # + # The guard is identical to the ``/gate`` one and for the + # same reason: it is consulted ONLY for + # ``DecisionCategory.DENIED``, read from + # ``exc.wire_category`` (stamped by + # ``_parse_v3_error_envelope``). A ``budget`` / ``halt`` / + # ``infra`` refusal keeps its own exception whatever the + # flag is set to, and an ABSENT category is left absent + # and raises as before — never guessed. + if ( + getattr(exc, "wire_category", None) == DecisionCategory.DENIED + and self.on_denied == "message" + ): + raise NullRunDeniedError( + workflow_id=workflow_id or UNKNOWN_WORKFLOW_ID, + reason=exc.reason if hasattr(exc, "reason") else str(exc), + action="block", + decision_source=DecisionSource.GATEWAY, + reasons=( + exc.reason if hasattr(exc, "reason") else str(exc) + ), + agent_message=getattr(exc, "agent_message", None), + ) from exc + raise # The /execute require_approval arm mints a fresh execution_id # server-side for the approval row + writes the binding, diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index c9ae972..5b74700 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -2669,6 +2669,26 @@ def _extract_error_envelope( return ("", raw_text or "", {}) # Shape 1: v3 envelope. + # + # ``error_code`` is read from the TOP LEVEL first and from + # ``details.error_code`` second. The second is not a fallback for + # a malformed envelope — it is where a real gate refusal puts it. + # ``GateResponse`` serialises ``error_code`` inside + # ``details`` (``internal.rs:716``) and the backend's own status + # mapper reads it from there (``gate.rs:88-90``), so every + # refusal that reaches ``/execute`` or ``/gate`` carries it at + # that nesting. Reading only the top level left ``code`` empty + # for the whole refusal family, and the dispatcher then fell + # through to its status-only branch — where a 403 becomes + # ``NullRunAuthenticationError``. The observable effect was a + # ``TOOL_BLOCKED`` policy refusal on the MCP path reported to the + # host as a bad API key: wrong exception class, wrong + # ``format_user_message``, and an operator action ("check your + # credentials") that cannot possibly fix a policy decision. + # + # This is the same class of defect as DEF-MP-TS12-ENF-01 — a + # decision the backend made being reported as something else — + # and it is why ADR-064 records the nesting as load-bearing. if "error_code" in body: code = str(body.get("error_code", "") or "") # The 503 budget path uses "message" instead of @@ -2693,7 +2713,37 @@ def _extract_error_envelope( details.setdefault(key, value) return (code, message, details) - # Shape 2: legacy slug. ``error`` is the slug, + # Shape 2: a gate refusal envelope. Same fields as the v3 + # envelope, but ``error_code`` nested under ``details`` — see the + # note on shape 1 for why that nesting is the norm rather than an + # edge case. Handled BEFORE the legacy slug because a refusal body + # carries no ``error`` key, so the two cannot collide; it is + # ordered here so a refusal is never misread as a legacy slug. + details_raw = body.get("details") + if isinstance(details_raw, dict) and "error_code" in details_raw: + code = str(details_raw.get("error_code", "") or "") + message = str( + body.get("explanation") + or body.get("error_message") + or body.get("message") + or raw_text + or "" + ) + details = dict(details_raw) + for key, value in body.items(): + if key in ( + "error_code", + "error_message", + "explanation", + "message", + "details", + "retry_after_ms", + ): + continue + details.setdefault(key, value) + return (code, message, details) + + # Shape 3: legacy slug. ``error`` is the slug, # ``message`` is the human-readable string. if "error" in body: slug = str(body.get("error", "") or "") @@ -2747,6 +2797,54 @@ def _safe_json(response: httpx.Response, endpoint: str) -> Any: def _parse_v3_error_envelope( response: httpx.Response, endpoint: str, +) -> Exception: + """Translate a non-2xx response, stamping the refusal category on. + + Thin wrapper around [`_parse_v3_error_envelope_uncategorised`] + that attaches ``wire_category`` to whatever exception comes back. + The wrapper exists because the function has ~15 return points and + threading the attribute through each one would be a change with + no behaviour in it — the property being added is "every exception + built from a gate refusal knows what kind of refusal it was". + """ + exc = _parse_v3_error_envelope_uncategorised(response, endpoint) + try: + body = response.json() + except Exception: + body = None + if isinstance(body, dict): + # ADR-062 §2.2. A refusal whose category is ABSENT is left + # absent, not defaulted: the runtime's strict rule reads the + # missing value as unclassifiable, which is the honest + # reading. Defaulting to `infra` here would quietly convert a + # missing field into a confident answer. + category = body.get("category") + if category is not None: + exc.wire_category = category # type: ignore[attr-defined] + exc.wire_error_code = ( # type: ignore[attr-defined] + (body.get("details") or {}).get("error_code") + if isinstance(body.get("details"), dict) + else body.get("error_code") + ) + # Server-authored text, carried the same way ``check()`` + # carries it on the dict it returns. ADR-063 §1.3(e): + # ``agent_message`` is the ONLY text the backend certifies + # as safe for a model — no store name, no schema, no wire + # code, no policy vocabulary. Inventing a substitute when it + # is absent would put exactly the content the leak guard + # exists to keep off the model's plate onto it, so an + # absent ``agent_message`` is left absent and the raise + # site's own catalogue text applies. + exc.agent_message = ( # type: ignore[attr-defined] + body.get("agent_message") + ) + exc.user_message = body.get("user_message") # type: ignore[attr-defined] + return exc + + +def _parse_v3_error_envelope_uncategorised( + response: httpx.Response, + endpoint: str, ) -> Exception: """Translate a non-2xx ``httpx.Response`` into the right v3 SDK exception. diff --git a/tests/test_2026_09_10_toolblocked_parser.py b/tests/test_2026_09_10_toolblocked_parser.py index 8cd7487..7671441 100644 --- a/tests/test_2026_09_10_toolblocked_parser.py +++ b/tests/test_2026_09_10_toolblocked_parser.py @@ -66,11 +66,58 @@ SDK_ROOT = Path(__file__).resolve().parent.parent TRANSPORT_PY = SDK_ROOT / "src" / "nullrun" / "transport.py" +# The dedicated dispatch branch DEF-NR-TOOLBLOCKED-PARSER added. Used as +# the content needle for locating the dispatching parser — see +# ``_parser_fn_containing``. +_BRANCH_NEEDLE = "catalog is NullRunToolBlockedError" + def _read(path: Path) -> str: return path.read_text(encoding="utf-8") +def _parser_fn_containing(src: str, needle: str) -> str: + """Return the body of the v3-envelope parser function that + actually contains ``needle``. + + The parser was split (ADR-062 §2.2 category work) into a thin + categorising wrapper ``_parse_v3_error_envelope`` and the real + implementation ``_parse_v3_error_envelope_uncategorised``. The + name-scoped pins in this file used to resolve + ``def _parse_v3_error_envelope(`` and read the body from there. + After the split that resolves to the WRAPPER, which contains + neither the dispatch branch nor the function-local import block + nor the DEF-NR-TOOLBLOCKED-PARSER tag — so all three went red for + a rename, and would equally have gone green against a parser + whose dispatch branch had been deleted outright. A pin that + cannot fail for the reason it claims to guard is worse than no + pin: it reads as coverage. + + Resolving by CONTENT instead of by name keeps the property each + test was written to assert — "the dedicated branch lives in the + function that does the dispatch, not somewhere else where it + cannot intercept the TypeError" — and survives both the split and + a future rename. The assertion that exactly one parser function + matches is deliberate: if a second one ever also contains the + needle, the pin has genuinely become ambiguous and must be + re-pointed by hand rather than silently matching the first. + """ + bodies = re.findall( + r"def _parse_v3_error_envelope\w*\(.*?(?=\ndef |\nclass |\Z)", + src, + re.DOTALL, + ) + matches = [b for b in bodies if needle in b] + assert len(matches) == 1, ( + f"expected exactly one _parse_v3_error_envelope* function " + f"containing {needle!r}, found {len(matches)} of {len(bodies)} " + f"parser functions. The parser was split into a wrapper plus an " + f"implementation; if that split changed again, re-point this pin " + f"at the function that does the dispatch." + ) + return matches[0] + + def _v3_envelope(error_code: str, status: int = 400, **details) -> httpx.Response: body = { "error_code": error_code, @@ -134,42 +181,35 @@ def test_dedicated_branch_present(self): ) def test_branch_in_parser(self): - """Pin that the fix lives in ``_parse_v3_error_envelope``, - not somewhere else (defense against a refactor that moves - it to a different layer where it can't intercept the + """Pin that the fix lives in the function that does the + dispatch, not somewhere else (defense against a refactor that + moves it to a different layer where it can't intercept the TypeError).""" src = _read(TRANSPORT_PY) - # Locate the _parse_v3_error_envelope function body and - # confirm the dedicated branch lives inside it. - fn_match = re.search( - r"def _parse_v3_error_envelope\(.*?(?=\ndef |\nclass |\Z)", - src, - re.DOTALL, - ) - assert fn_match, "could not locate _parse_v3_error_envelope" - fn_body = fn_match.group(0) - assert "NullRunToolBlockedError" in fn_body, ( + # Locate the parser function body that holds the dedicated + # branch. Resolved by the BRANCH, not by the exception name: + # resolving on ``NullRunToolBlockedError`` is self-satisfying, + # because the function-local import block names it even after + # the branch is deleted. Verified — deleting the branch left + # this pin green that way. The branch text is the only needle + # that is actually absent when the branch is gone. + fn_body = _parser_fn_containing(src, _BRANCH_NEEDLE) + assert _BRANCH_NEEDLE in fn_body, ( "DEF-NR-TOOLBLOCKED-PARSER: NullRunToolBlockedError " - "must be referenced inside _parse_v3_error_envelope " - "(the dedicated dispatch branch lives there)." + "must be referenced inside the v3-envelope parser that " + "performs the dispatch (the dedicated dispatch branch " + "lives there)." ) def test_import_includes_blocked_exception_classes(self): - """The function-local import block in - ``_parse_v3_error_envelope`` must include both - ``NullRunToolBlockedError`` and + """The function-local import block in the dispatching parser + must include both ``NullRunToolBlockedError`` and ``NullRunBlockedException`` — otherwise NameError at runtime even though the branch is present.""" src = _read(TRANSPORT_PY) - # Locate the function-local import block (the one inside - # _parse_v3_error_envelope, NOT the module-level one). - fn_match = re.search( - r"def _parse_v3_error_envelope\(.*?(?=\ndef |\nclass |\Z)", - src, - re.DOTALL, - ) - assert fn_match - fn_body = fn_match.group(0) + # Locate the function-local import block (the one inside the + # function that holds the branch, NOT the module-level one). + fn_body = _parser_fn_containing(src, _BRANCH_NEEDLE) # Find the first ``from nullrun.breaker.exceptions import`` # inside the function body. import_block = re.search( @@ -179,8 +219,8 @@ def test_import_includes_blocked_exception_classes(self): ) assert import_block, ( "DEF-NR-TOOLBLOCKED-PARSER: could not locate " - "function-local import block inside " - "_parse_v3_error_envelope" + "function-local import block inside the dispatching " + "v3-envelope parser" ) imported = import_block.group(1) assert "NullRunToolBlockedError" in imported, ( @@ -200,12 +240,13 @@ def test_branch_comment_tag_present(self): deletes the comment is forced to read the code's history. - The tag must live in ``_parse_v3_error_envelope``, the - function that holds the branch. It used to sit in - ``Transport.check``, which never had a parser branch at all — - so the comment named a fix the reader could not find. The - pin now asserts the tag is attached to the right function, - which is the property that was actually broken. + The tag must live in the parser function that holds the + branch. It used to sit in ``Transport.check``, which never had + a parser branch at all — so the comment named a fix the + reader could not find. The pin now asserts the tag is + attached to the right function, which is the property that + was actually broken. Resolved by content, not by name, so the + wrapper/implementation split does not blind it. """ src = _read(TRANSPORT_PY) assert "DEF-NR-TOOLBLOCKED-PARSER" in src, ( @@ -213,14 +254,14 @@ def test_branch_comment_tag_present(self): "block must name the fix tag so future readers can " "grep for it." ) - body = src.split("def _parse_v3_error_envelope(")[1] - head = body[: body.find('"""', body.find('"""') + 3)] + fn_body = _parser_fn_containing(src, "DEF-NR-TOOLBLOCKED-PARSER") + head = fn_body[: fn_body.find('"""', fn_body.find('"""') + 3)] assert "DEF-NR-TOOLBLOCKED-PARSER" in head, ( "DEF-NR-TOOLBLOCKED-PARSER: the tag must be documented on " - "_parse_v3_error_envelope, which is where the dedicated " - "NullRunToolBlockedError branch actually lives. Filing it " - "against Transport.check pointed readers at a branch that " - "was never there." + "the v3-envelope parser that does the dispatch, which is " + "where the dedicated NullRunToolBlockedError branch " + "actually lives. Filing it against Transport.check " + "pointed readers at a branch that was never there." ) def test_branch_uses_correct_constructor_signature(self): diff --git a/tests/test_mcp_refusal_categories.py b/tests/test_mcp_refusal_categories.py new file mode 100644 index 0000000..cf88c5d --- /dev/null +++ b/tests/test_mcp_refusal_categories.py @@ -0,0 +1,391 @@ +"""ADR-062 §2.2 categories on the MCP path — the real adapter. + +`MCPAdapter.call_tool` is a different enforcement path from +`check_workflow_budget`. It calls `runtime.execute(...)` against +`/api/v1/execute`, not `/gate`, so anything the category work added +to the `/gate` block site is, by default, absent here. + +That is the specific gap this file covers. Two properties, and they +are independent: + +1. **The refusal is classified and fails CLOSED on the MCP path.** + `runtime.execute` has no fail-OPEN `except` — an unclassifiable + refusal raised by the transport propagates — but that is an + argument from reading the code, not evidence, and the reason + DEF-MP-TS12-ENF-01 shipped at all is that reading code was not + enough. + +2. **`on_denied` reaches the MCP path.** The flag is documented as + selecting the shape of a `denied` refusal. If it silently does + nothing for MCP tools — a whole class of tool calls — then a host + that set `on_denied="message"` is getting a promise it does not + keep, and the model-facing text it expected to relay never + arrives. + +The doubles here are deliberately shallow: a mock MCP client with a +tool inventory, and a real `MCPAdapter`, a real `NullRunRuntime`, +and a respx-mocked `/execute`. Nothing between the adapter and the +wire is faked, so a change that bypasses the gate on this path turns +these red rather than being stubbed around. + +`test_mcp_adapter_gate_closed.py` deliberately has no autouse +runtime fixture so it can observe the real resolution order; this +file binds the runtime explicitly and does not care about order. +""" + +from __future__ import annotations + +from typing import Any + +import httpx +import pytest +import respx + + +from nullrun.breaker.exceptions import ( + NullRunBlockedException, + NullRunDeniedError, + NullRunError, +) +from nullrun.toolbox.mcp import MCPAdapter +from nullrun.runtime import NullRunRuntime + +EXECUTE_URL = "https://api.test.nullrun.io/api/v1/execute" + + +class _Ann: + def __init__(self, read=None, destructive=None, open_world=None): + self.readOnlyHint = read + self.destructiveHint = destructive + self.openWorldHint = open_world + + +class _Tool: + def __init__(self, name, annotations=None): + self.name = name + self.annotations = annotations + + +class _MockMcpClient: + """Minimal MCP client surface. Records calls so a test can assert + the server was NOT reached after a block.""" + + def __init__(self, tools): + self._tools = {t.name: t for t in tools} + self.calls: list[tuple[str, dict[str, Any]]] = [] + + def list_tools(self): + return list(self._tools.values()) + + def call_tool(self, name, arguments=None, **kwargs): + self.calls.append((name, arguments or {})) + if name not in self._tools: + raise KeyError(f"unknown tool {name!r}") + return f"ok:{name}" + + +def _refusal_body( + code: str, + category: str | None, + *, + agent_message: str | None = None, +) -> dict[str, Any]: + body: dict[str, Any] = { + "decision": "block", + "decision_source": "gateway", + "details": {"error_code": code}, + "explanation": f"refused: {code}", + "explanations": [f"refused: {code}"], + "policy_id": None, + "policy_version": 0, + "projected_cost_cents": None, + "remaining_budget_cents": None, + "reservation_id": None, + "staleness_ms": None, + "user_message": "operator-facing text", + } + if category is not None: + body["category"] = category + if agent_message is not None: + body["agent_message"] = agent_message + return body + + +def _adapter(runtime) -> tuple[MCPAdapter, _MockMcpClient]: + client = _MockMcpClient( + [ + _Tool( + "create_issue", + _Ann(read=False, destructive=True, open_world=True), + ) + ] + ) + return ( + MCPAdapter(server_name="github", mcp_client=client, runtime=runtime), + client, + ) + + +def _runtime(**kwargs) -> NullRunRuntime: + return NullRunRuntime( + api_key="test-key-12345678", + secret_key="test-secret-deterministic", + api_url="https://api.test.nullrun.io", + polling=False, + **kwargs, + ) + + +class TestMcpRefusalFailsClosed: + """Property 1: the gate's answer stands on the MCP path.""" + + def test_classified_denial_blocks_and_the_server_is_not_called(self, mock_api): + respx.post(EXECUTE_URL).mock( + return_value=httpx.Response( + 403, json=_refusal_body("TOOL_BLOCKED", "denied") + ) + ) + adapter, client = _adapter(_runtime()) + + with pytest.raises(NullRunBlockedException): + adapter.call_tool("create_issue", {"repo": "acme/api"}) + + assert client.calls == [], ( + "the MCP server must not be reached after the gate refused" + ) + + def test_unclassifiable_refusal_raises_rather_than_calling_the_server( + self, mock_api + ): + """The strict rule holds on the second enforcement path too. + + A refusal with no `category` must not reach the MCP server by + way of a permissive default. Whether it raises the specific + `NullRunUnclassifiedRefusalError` or a broader + `NullRunError` is not the property — the property is that it + raises at all, and that the server was not called. + """ + respx.post(EXECUTE_URL).mock( + return_value=httpx.Response( + 403, json=_refusal_body("TOOL_BLOCKED", None) + ) + ) + adapter, client = _adapter(_runtime()) + + with pytest.raises(NullRunError): + adapter.call_tool("create_issue", {"repo": "acme/api"}) + + assert client.calls == [], ( + "an unclassifiable refusal must not be laundered into a call" + ) + + def test_budget_refusal_keeps_its_own_exception(self, mock_api): + """A `budget` refusal is not a `denied` one, on this path too.""" + respx.post(EXECUTE_URL).mock( + return_value=httpx.Response( + 402, json=_refusal_body("BUDGET_HARD_BLOCKED", "budget") + ) + ) + adapter, client = _adapter(_runtime()) + + with pytest.raises(NullRunError) as exc_info: + adapter.call_tool("create_issue", {"repo": "acme/api"}) + + assert not isinstance(exc_info.value, NullRunDeniedError) + assert client.calls == [] + + +class TestOnDeniedReachesMcp: + """Property 2: the flag applies to MCP tool calls.""" + + def test_denied_with_message_enabled_raises_denied_error(self, mock_api): + respx.post(EXECUTE_URL).mock( + return_value=httpx.Response( + 403, + json=_refusal_body( + "TOOL_BLOCKED", + "denied", + agent_message="The operator has not allowed this tool.", + ), + ) + ) + adapter, client = _adapter(_runtime(on_denied="message")) + + with pytest.raises(NullRunDeniedError) as exc_info: + adapter.call_tool("create_issue", {"repo": "acme/api"}) + + assert exc_info.value.agent_message == ( + "The operator has not allowed this tool." + ), ( + "the server-authored agent_message must survive the round trip " + "— it is the only text the SDK guarantees is safe for a model" + ) + assert client.calls == [] + + def test_denied_with_message_disabled_raises_the_catalogue_error( + self, mock_api + ): + respx.post(EXECUTE_URL).mock( + return_value=httpx.Response( + 403, json=_refusal_body("TOOL_BLOCKED", "denied") + ) + ) + adapter, client = _adapter(_runtime()) + + with pytest.raises(NullRunError) as exc_info: + adapter.call_tool("create_issue", {"repo": "acme/api"}) + + assert not isinstance(exc_info.value, NullRunDeniedError), ( + "the default is 'raise' with the code-specific catalogue " + "exception, not the message path" + ) + assert client.calls == [] + + @pytest.mark.parametrize( + "category,status,code", + [ + ("budget", 402, "BUDGET_HARD_BLOCKED"), + ("halt", 403, "LOOP_DETECTED"), + ], + ) + def test_non_denied_categories_never_become_a_message( + self, mock_api, category, status, code + ): + """The negative set, on the MCP path. + + Same reasoning as on `/gate`, and it matters more here: an MCP + tool is far more likely to be retried with a different + argument or a different tool than a local function is, so a + model told "that tool is not allowed" has an obvious next move + that walks straight into a budget wall or an operator's stop. + + The wire codes are the load-bearing part of this test and were + the first thing to get it wrong. An earlier version used a + synthetic `SOME_CODE`, which `_parse_v3_error_envelope` maps + to a non-block class — so the exception never reached the + `except NullRunBlockedException` arm, and the parameters + passed for a reason that had nothing to do with the guard. + Widening the guard to consult `on_denied` alone left them + green. That is the self-defeating-test failure mode this + branch has already hit five times, caught here only by + running the mutation and reading which tests actually went + red. + + Every code below is real, and each one was checked to map to a + `NullRunBlockedException` subclass, so each genuinely enters + the arm and is rejected by the category check rather than + never arriving. + + `infra` is deliberately NOT in this set — see + `test_infra_refusal_is_a_gateway_error_not_a_message`. There is + no 4xx infra refusal to put here: ADR-063 §4.7 sends infra + refusals as 503, and 503 never reaches the envelope parser on + this endpoint. + """ + respx.post(EXECUTE_URL).mock( + return_value=httpx.Response( + status, json=_refusal_body(code, category) + ) + ) + adapter, client = _adapter(_runtime(on_denied="message")) + + with pytest.raises(NullRunError) as exc_info: + adapter.call_tool("create_issue", {"repo": "acme/api"}) + + assert not isinstance(exc_info.value, NullRunDeniedError), ( + f"category={category!r} must keep its own exception even with " + f"on_denied='message'" + ) + assert client.calls == [] + + def test_infra_refusal_is_a_gateway_fallback_not_a_message(self, mock_api): + """`infra` is covered by a different mechanism, so say which. + + An infra-category refusal arrives as a 503 (ADR-063 §4.7). + `_retry_with_backoff` maps the whole 5xx band on `/execute` to + `NullRunTransportError` / GATEWAY_ERROR before + `_parse_v3_error_envelope` is ever called, so the `category` + field in the body is never read. The runtime then converts the + transport error into a STRICT fallback block — raised at + `runtime.py:3670`, downstream of the `on_denied` arm, which + only wraps the `self._transport.execute(...)` call itself. + + Two things follow, and both are asserted rather than assumed. + + First, the right outcome: an infrastructure fault is not a + permission answer, and `on_denied="message"` must not turn one + into model-readable "that tool is not allowed" text. This is + the product decision — ordinary unavailability is not a + denial. + + Second, the mechanism: the property here is enforced by the 5xx + band and the STRICT fallback, NOT by the category check. A + future change that let a 5xx through to the envelope parser + would move `infra` from this mechanism to the parametrised + set's with nothing here going red. Asserting the concrete + class and the fallback marker in the reason pins which + mechanism is actually in play, so that move would break this + test instead of passing silently. + """ + respx.post(EXECUTE_URL).mock( + return_value=httpx.Response( + 503, json=_refusal_body("REDIS_UNAVAILABLE", "infra") + ) + ) + adapter, client = _adapter(_runtime(on_denied="message")) + + with pytest.raises(NullRunBlockedException) as exc_info: + adapter.call_tool("create_issue", {"repo": "acme/api"}) + + assert not isinstance(exc_info.value, NullRunDeniedError) + assert "Gateway unavailable" in str(exc_info.value), ( + "the 5xx must still read as a gateway fault; if this becomes a " + "policy denial the product decision has been inverted" + ) + assert client.calls == [] + + def test_absent_category_raises_even_with_message_enabled(self, mock_api): + """`on_denied="message"` must not launder an ABSENT category. + + The companion to + `test_non_denied_categories_never_become_a_message`, and it + guards a different mistake: there the category is present and + is not `denied`; here it is simply missing. Both must reach + the same outcome, because a handler that guesses "no category + means it was not a real denial" reads an unclassifiable + refusal as a permission answer and hands the model text the + server never wrote. + + The assertion is `NullRunError`, not + `NullRunUnclassifiedRefusalError`, and the difference is + deliberate. `/execute` raises a code-specific exception + derived from the server's own ``details.error_code`` + (``TOOL_BLOCKED`` → ``NullRunToolBlockedError``) rather than + routing through the `/gate` path's + ``resolve_refusal_category``. That is not a guess — the code + is server-authored — and it is a pre-existing shape of this + endpoint that a category feature has no business changing + wholesale. The ADR-062 §2.2 property is "absent or + unrecognised category RAISES, never guessed", and it holds + here: what is asserted is the raise, not its class. Asserting + the `/gate`-path subclass instead would pin a behaviour + `/execute` never had and fail for the wrong reason. + + What must be true either way: it raises, it is not + `NullRunDeniedError`, and the MCP server was not reached. + """ + respx.post(EXECUTE_URL).mock( + return_value=httpx.Response( + 403, json=_refusal_body("TOOL_BLOCKED", None) + ) + ) + adapter, client = _adapter(_runtime(on_denied="message")) + + with pytest.raises(NullRunError) as exc_info: + adapter.call_tool("create_issue", {"repo": "acme/api"}) + + assert not isinstance(exc_info.value, NullRunDeniedError), ( + "an absent category must not be read as 'denied' — the flag " + "is for a server that SAID denied" + ) + assert client.calls == [] From 939fd7dec5c77baf90c91245f9c501ed57d0b9f8 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 09:18:09 +0400 Subject: [PATCH 10/20] docs(readme): state the three enforcement limitations, verified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The README carried no fail-open claim and no limitations section, so the two most consequential behaviours of the gate were only discoverable by reading the source. Adds both. Each limitation was checked against the code before being written down, and two of them are not what the shorthand description of them suggests. The 503 case fails OPEN on old SDKs, not closed. A failed security check is served as a 503 carrying a `category`; the current SDK reads it and refuses. Pre-category there was nothing to read, the 503 became a synthetic FALLBACK decision, and `check_workflow_budget` fails open on a FALLBACK source — so the call proceeds. Verified by probing both cases through a real runtime and by reading the pre-category source (`git show 83960b4~1:src/nullrun/transport.py`): the 5xx band had no `is_refusal` condition and fell straight to the FALLBACK return. The breaker setting is server-side, not an SDK default. `LogOnly` is `NULLRUN_GATE_CB_TRIP_ENFORCEMENT_MODE` in the backend, not a fallback mode in this SDK (there is no `LogOnly` anywhere in `src/`). The production boot check refuses to start unless it is explicitly `Enforce` or `LogOnly` (backend `main.rs:290-293`); `detect_mode()` still defaults to `LogOnly` on unset outside production (`main.rs:294-295`). In `LogOnly` the trip is recorded and alerted but tripped workflows still pass `/check` — the backend's own words at `main.rs:286-288`. Pause and kill really are 403-only, and that is now stated as the limitation it is: `WORKFLOW_PAUSED` and `WORKFLOW_INACTIVE` are served as the same 403 from the same key, distinguishable only by operator-facing text, which the backend deliberately keeps distinct (`error_codes.rs:3400-3412`). Support tooling should key off the error code, not the status. Also adds the fail-open summary, matching the ADR-008 table in `runtime.py` — which the module docstring requires to be kept in lockstep with any README claim. There was no README claim to keep in step before this; there is one now. --- README.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/README.md b/README.md index 47c4d42..0e64775 100644 --- a/README.md +++ b/README.md @@ -354,6 +354,24 @@ require tests for new public API, and run `ruff` + `mypy` in CI. --- +## Known limitations + +Three things this SDK does not do. All three are enforcement-relevant, so they are stated here rather than left to be discovered during an incident. Each was verified against the code before being written down. + +**1. A failed security check arrives as a 503, and only this version of the SDK stops on it.** When the backend cannot evaluate the security check itself, it refuses with a 503 carrying a `category` field. This SDK reads that field and fails **closed** — the call is refused. An SDK older than the category work has nothing to read: the 503 is turned into a synthetic `FALLBACK` decision, and `check_workflow_budget` fails **open** on a `FALLBACK` source — the call proceeds. So during a partial backend outage, enforcement differs by SDK version. A genuine outage (not a failed check) still fails open on every version, which is deliberate: a dead backend must not freeze your agent loop. + +If you need this guarantee today, pin the SDK version. Do not assume a refused call implies the backend rejected the call. + +**2. The gate circuit-breaker's trip mode is a server-side setting, and `LogOnly` does not block.** When the gate's circuit breaker trips, what happens is decided by `NULLRUN_GATE_CB_TRIP_ENFORCEMENT_MODE` on the server, not by anything in this SDK. In `LogOnly` the trip is recorded and alerted on, but tripped workflows still pass `/check`. The production boot check refuses to start unless the variable is explicitly set to `Enforce` or `LogOnly`, so a deploy cannot inherit the dev default (`detect_mode()` still falls back to `LogOnly` when unset outside production) — but an operator who chooses `LogOnly` is choosing non-enforcement, knowingly. If your compliance story depends on breaker trips being enforced, confirm that value with whoever operates the deployment. + +**3. Pause and kill both reach the agent as a 403.** There is no separate status to branch on. `WORKFLOW_PAUSED` and `WORKFLOW_INACTIVE` are served as the same 403 from the same key; the only thing distinguishing them is the operator-facing text, which the backend deliberately keeps distinct because they mean opposite things about whether the run will resume. If you write support tooling, key off the error code, not the status. Separately, the SDK can observe pause/kill ahead of the next gate call via `check_control_plane` (WebSocket push, or a `/status` poll), which raises `WorkflowPausedException` / `NullRunWorkflowKilledError` locally. + +### What does fail open + +Fail-open here is narrow and deliberate, and the authoritative table lives in `runtime.py` (ADR-008). In short: a **transport** failure on the check path is open, so an unreachable backend cannot freeze your agent; a **wire response that names an enforcement failure** is closed, because the backend made a decision and the SDK will not overrule it; a **401** is closed, because no retry fixes a revoked key; and the `/execute` path is closed by default (`FallbackMode.STRICT`). + +--- + ## Security NullRun does **not** store or proxy your LLM provider keys — it sits beside your existing clients and observes the calls. The gate is **server-authoritative** for cost: even a malicious SDK cannot inflate spend by sending a fake `cost_cents` to `/track`. From a33f863cb07834f26949baf400dedb87ed313bd3 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 12:44:35 +0400 Subject: [PATCH 11/20] fix(docs): name ADR-064's discriminator, not a marker that is not on the wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The transport comment and two test docstrings repeated ADR-063 §4.7's claim that the backend "already distinguishes the two 503 groups internally via GateErrorCode::is_fail_closed()". That is wrong in the way ADR-063 §4.7 was wrong: is_fail_closed is an ordinary Rust method that is never serialised, so no client could have read it. The SDK never did — the code has always keyed on decision == "block" (categories.py:168), which is ADR-064's discriminator. The comment also claimed BUDGET_DATA_UNAVAILABLE "arrives as 503 with decision=block". It does not, and cannot: grepping the backend, that string appears only in error_codes.rs and budget.rs, never in internal.rs or orchestrator.rs, so no gate producer emits it. The one response that does carry it (budget.rs:246, BudgetUnavailableResponse) has no decision field at all — which is correctly treated as a non-refusal and fails open per ADR-008. Logic unchanged; this is a claim correction, and every ADR-063 §4.7 citation for the 503 rule retargeted to ADR-064, which now owns it. 44 passed. --- src/nullrun/transport.py | 47 ++++++++++++------- tests/test_2026_09_10_check_failopen.py | 2 +- ...adr063_infra_refusal_is_not_fail_closed.py | 24 ++++++---- tests/test_mcp_refusal_categories.py | 4 +- tests/test_refusal_categories.py | 5 +- 5 files changed, 52 insertions(+), 30 deletions(-) diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index 5b74700..1a881ab 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -1626,26 +1626,41 @@ def _do_gate_post() -> httpx.Response: # can produce an actionable message. # # A gate refusal is a refusal whatever the status. - # ADR-063 §4.7 (product decision: option 2) — the - # backend's 503s split into two groups and the split is - # what the category records: + # ADR-064 owns this rule; §4.7 of ADR-063 records the + # correction and points here. # - # * "the answer is not available right now" - # (BUDGET_DATA_UNAVAILABLE, is_fail_closed=false) - # * "the CHECK could not be performed" - # (CIRCUIT_BREAKER_STATE_LOOKUP_FAILED, - # WORKFLOW_INACTIVE_LOOKUP_FAILED, - # RATE_LIMIT_PLAN_LOOKUP_FAILED, is_fail_closed=true) + # The distinction is not the status and not a store + # name — it is whether the gate produced an ANSWER: + # + # * the check could not be performed, and the gate + # said so (CIRCUIT_BREAKER_STATE_LOOKUP_FAILED, + # WORKFLOW_INACTIVE_LOOKUP_FAILED, + # RATE_LIMIT_PLAN_LOOKUP_FAILED — category "infra", + # status 503) — arrives as 503 WITH + # decision="block", and the decision stands. + # * nothing reached the gate — a proxy 502, a gateway + # that never answered, `/budget/approximate`'s + # `BudgetUnavailableResponse` (budget.rs:246, which + # carries no `decision` field at all) — arrives 5xx + # with no refusal body, and ADR-008's fail-OPEN + # applies. + # + # So the discriminator below is `decision == "block"`, + # which `GateResponse` always serialises. Do not look + # for a fail-closed marker: `GateErrorCode::is_fail_closed` + # is an in-process Rust method that is never written to + # the wire, and a client written against it could not + # have found it. ADR-064 §Correction is the full + # history. # - # Both arrive as 503 with decision="block", and the gate - # already refuses either way (fail-CLOSED, CLAUDE.md §4). # Pre-fix the entire 5xx band fell through to the # synthetic FALLBACK block below, which the runtime - # reads as a transport error and fails OPEN — so a - # fail-CLOSED 503 refusal was silently converted into - # "allowed". That is DEF-MP-TS12-ENF-01's exact shape - # with a different trigger, and it is why a 5xx body - # that is a genuine refusal is handled here instead. + # reads as a transport error and fails OPEN — so a 503 + # refusal the gate had actually made was silently + # converted into "allowed". That is DEF-MP-TS12-ENF-01's + # exact shape with a different trigger, and it is why a + # 5xx body that is a genuine refusal is handled here + # instead. # # Old SDKs are unaffected by definition: they never # looked at `category`, and they read the 503 as a diff --git a/tests/test_2026_09_10_check_failopen.py b/tests/test_2026_09_10_check_failopen.py index 904e34d..9f59c52 100644 --- a/tests/test_2026_09_10_check_failopen.py +++ b/tests/test_2026_09_10_check_failopen.py @@ -188,7 +188,7 @@ def test_4xx_branch_uses_gateway_not_fallback(self): assert idx != -1, ( "DEF-NR-CHECK-FAIL-OPEN: 4xx branch anchor " "`if 400 <= response.status_code < 500 or (` not found in " - "Transport.check. ADR-063 §4.7 widened the condition to " + "Transport.check. ADR-064 widened the condition to " "cover 5xx bodies that are genuine gate refusals, so the " "anchor moved with it." ) diff --git a/tests/test_adr063_infra_refusal_is_not_fail_closed.py b/tests/test_adr063_infra_refusal_is_not_fail_closed.py index d4044f3..7ffcb42 100644 --- a/tests/test_adr063_infra_refusal_is_not_fail_closed.py +++ b/tests/test_adr063_infra_refusal_is_not_fail_closed.py @@ -1,6 +1,7 @@ """ADR-063 §1.3(f): what the SDK does with an `infra` refusal. -SUPERSEDED IN PART — ADR-063 §4.7 (product decision, option 2). +SUPERSEDED IN PART — the rule now lives in ADR-064. ADR-063 §4.7 +records the correction and points at it. The first version of this file pinned an asymmetry that was, on inspection, not a property of the categories but an accident of the @@ -14,9 +15,15 @@ ITSELF gets a marker the new SDK treats as a block. SDKs predating the category work keep failing open. -The marker already exists — it is ``category: "infra"``, and the -backend already distinguishes the two 503 groups internally via -``GateErrorCode::is_fail_closed()``. So this was a client-side +The marker is on the wire: ``category: "infra"`` alongside +``decision: "block"``, both always serialised by ``GateResponse``. +An earlier draft of this docstring said the backend "distinguishes the +two 503 groups internally via ``GateErrorCode::is_fail_closed()``" — +that was wrong. ``is_fail_closed`` is an ordinary Rust method and is +never serialised; ``GateErrorCode`` has no flag field and no response +struct carries one, so a client had nothing to read. The discriminator +that does survive the wire is ``decision``, which is what the code +below and ``categories.py:168`` actually use. This was a client-side mapping change, not a re-architecture: a 5xx body that is a genuine gate refusal (``decision == "block"``) is now classified and blocks, while a 5xx that is a real outage still fails open. @@ -140,16 +147,15 @@ def _breaker_trip_403() -> httpx.Response: class TestInfraRefusalIsNotFailClosed: """A 503 the GATE answered is not an outage. - ADR-063 §4.7 option 2. The distinction is whether an answer - exists: a refusal body means the gate made a decision and it - stands; a body-less 5xx means it never got to one, and ADR-008's - fail-OPEN applies. + ADR-064. The distinction is whether an answer exists: a refusal + body means the gate made a decision and it stands; a body-less 5xx + means it never got to one, and ADR-008's fail-OPEN applies. """ def test_503_state_read_failure_blocks(self, make_runtime, mock_api): """A failed state read is the gate's own answer — honour it. - Before §4.7 this returned without raising, so a fail-CLOSED + Before ADR-064 this returned without raising, so a fail-CLOSED 503 from the backend was converted into "allowed" by the status code alone. The gate blocks in both 503 groups (fail-CLOSED, CLAUDE.md §4); the SDK now stops too. diff --git a/tests/test_mcp_refusal_categories.py b/tests/test_mcp_refusal_categories.py index cf88c5d..6a6a861 100644 --- a/tests/test_mcp_refusal_categories.py +++ b/tests/test_mcp_refusal_categories.py @@ -278,7 +278,7 @@ def test_non_denied_categories_never_become_a_message( `infra` is deliberately NOT in this set — see `test_infra_refusal_is_a_gateway_error_not_a_message`. There is - no 4xx infra refusal to put here: ADR-063 §4.7 sends infra + no 4xx infra refusal to put here: the backend sends infra refusals as 503, and 503 never reaches the envelope parser on this endpoint. """ @@ -301,7 +301,7 @@ def test_non_denied_categories_never_become_a_message( def test_infra_refusal_is_a_gateway_fallback_not_a_message(self, mock_api): """`infra` is covered by a different mechanism, so say which. - An infra-category refusal arrives as a 503 (ADR-063 §4.7). + An infra-category refusal arrives as a 503 (ADR-064). `_retry_with_backoff` maps the whole 5xx band on `/execute` to `NullRunTransportError` / GATEWAY_ERROR before `_parse_v3_error_envelope` is ever called, so the `category` diff --git a/tests/test_refusal_categories.py b/tests/test_refusal_categories.py index 76f93fe..3b7eed4 100644 --- a/tests/test_refusal_categories.py +++ b/tests/test_refusal_categories.py @@ -72,8 +72,9 @@ "WORKFLOW_PAUSED": ("halt", 403), "WORKFLOW_INACTIVE": ("halt", 403), "CIRCUIT_BREAKER_TRIPPED": ("halt", 403), - # The two ADR-063 §4.7 503 groups. Both arrive as 503 with - # decision="block"; the category is what separates them. + # The two 503 groups of ADR-064. The gate's own answer arrives as + # 503 with decision="block"; a real outage arrives 5xx with no + # refusal body at all and fails open. "BUDGET_DATA_UNAVAILABLE": ("infra", 503), "CIRCUIT_BREAKER_STATE_LOOKUP_FAILED": ("infra", 503), } From c4593790abb7f393bd8497dd0eeb7857bb6a6448 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 12:50:48 +0400 Subject: [PATCH 12/20] fix(gate): a /gate body must state WHO decided before it is a verdict The refusal classifier splits a 5xx into "the gate ruled" and "nothing ruled" by reading the body. Every other category change on this branch rests on the same unexamined premise: that a body on the SDK's HTTPS connection was written by NULLRUN. A test for that premise found the premise is false in one direction. `{"decision": "allow"}` from an arbitrary responder passes every check `_require_gate_decision` applied -- it is a dict, and `decision` is a known string -- and was then honoured as a gateway decision, because the runtime's rule reads a MISSING decision_source as "not fallback" and therefore as authoritative. So an on-path responder that gets JSON in front of the SDK: a captive portal on a hotel WLAN, a corporate TLS-interception proxy. It needs neither the API key nor a HMAC bypass, only to answer before the real backend. The call is authorised, /track books its cost against a policy nobody consulted, the audit trail records an allow, and there is no later point at which it is caught. Fix: require `decision_source` to be a known provenance value BEFORE looking at `decision`. It is the right field because it cannot be absent from a real answer -- `GateResponse.decision_source` is a non-Option String with no skip_serializing_if (`gate/internal.rs:638`). `GateResponseBody` in schemas.rs declares it optional, but that struct is referenced only from openapi.rs: it is the documentation schema, not the wire. Mutation-verified: with the guard absent, the new test reports DID NOT RAISE on `{"decision": "allow"}`. 18 passed with it. The same guard caught a drifted fixture, not a second defect: test_v3_wire_contract.py's require_approval body omitted all three non-Option fields on GateResponse. Corrected to the real shape. Full suite 1561 passed / 1 skipped. --- src/nullrun/runtime.py | 50 ++++ tests/test_gate_body_provenance.py | 357 +++++++++++++++++++++++++++++ tests/test_v3_wire_contract.py | 12 + 3 files changed, 419 insertions(+) create mode 100644 tests/test_gate_body_provenance.py diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index b605fb8..c83d217 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -1927,9 +1927,46 @@ def check_control_plane(self, workflow_id: str) -> None: {"allow", "block", "throttle", "soft_pass", "require_approval", "deny"} ) + #: Provenance values a ``/gate`` answer may carry. ``gateway`` is + #: the only one the backend writes (`GateResponse.decision_source` + #: is a non-``Option`` ``String``, `gate/internal.rs:638`, and + #: every producer sets ``"gateway"``). ``fallback`` is the + #: synthetic block the transport synthesises when the 5xx band + #: falls through, and ``cached`` / ``local`` are the SDK's own + #: shapes. A value outside this set means the body did not come + #: from either party, whatever it claims. + _KNOWN_DECISION_SOURCES = frozenset( + {"gateway", "cached", "fallback", "local"} + ) + def _require_gate_decision(self, response: Any) -> str: """Extract ``decision`` from a ``/gate`` body, or raise. + **Provenance first.** A verdict is permission only if the body + says WHO produced it, and this check runs before the decision + is even looked at. Without it ``{"decision": "allow"}`` — a + body with no ``decision_source`` at all — passes every other + check in this method and is honoured as a gateway decision, + because the runtime's rule reads a missing ``decision_source`` + as "not ``fallback``" and therefore as authoritative. + + That is reachable by anyone on the network path. A captive + portal on a hotel or airport WLAN, or a corporate + TLS-interception proxy, returns JSON on ``/api/v1/gate`` + without needing the API key and without needing to defeat + HMAC — it only has to answer before the real backend does. + The call is authorised, ``/track`` books its cost against a + policy that was never consulted, and the audit trail records + an allow. There is no later point at which it can be caught. + + ``decision_source`` is the right field to require because it + cannot be absent from a real answer: it is a non-``Option`` + ``String`` with no ``skip_serializing_if``, so serde always + emits it. ``GateResponseBody`` in ``gate/schemas.rs`` does + declare it optional, but that struct is referenced only from + ``openapi.rs`` — it is the documentation schema, not the wire + — so it is not a case where a real backend omits it. + Pre-fix this was ``response.get("decision", "allow")``. That default converted three distinct failures into "allowed": @@ -1959,6 +1996,19 @@ def _require_gate_decision(self, response: Any) -> str: f"object. Body was not a gate decision." ) + source = response.get("decision_source") + if not isinstance(source, str) or source not in self._KNOWN_DECISION_SOURCES: + raise NullRunMalformedGateResponseError( + f"/gate response carries no usable 'decision_source' " + f"(got {source!r}). A real answer always states who decided — " + f"`GateResponse.decision_source` is a non-Option String on the " + f"wire — so a body without one did not come from the gate. " + f"This SDK knows {sorted(self._KNOWN_DECISION_SOURCES)}. " + f"Treating it as a verdict would let an on-path responder " + f"(captive portal, TLS-interception proxy) authorise a call no " + f"policy engine evaluated." + ) + decision = response.get("decision") if not isinstance(decision, str): raise NullRunMalformedGateResponseError( diff --git a/tests/test_gate_body_provenance.py b/tests/test_gate_body_provenance.py new file mode 100644 index 0000000..894c7d7 --- /dev/null +++ b/tests/test_gate_body_provenance.py @@ -0,0 +1,357 @@ +"""The discriminator is only trustworthy if the body is genuine. + +The refusal classifier decides whether a 5xx was an ANSWER or an +OUTAGE by reading fields out of the response body: + + ``decision == "block"`` → the gate ruled; honour it + anything else on a 5xx → nothing ruled; ADR-008 fail-OPEN + +Every other piece of category work on this branch rests on the same +premise — that a body arriving on the SDK's HTTPS connection was +written by NULLRUN. This file tests that premise instead of taking +it, because the failure mode when it is false is not a wrong error +message: it is a call authorised that no policy engine ever +evaluated. + +The threat is an on-path responder — captive portal on a hotel or +airport network, corporate TLS-interception proxy, a compromised +sidecar — that returns well-formed JSON on ``/api/v1/gate``. It does +not need the API key. It does not need to defeat HMAC. It only needs +to answer faster than the real backend. + +Two directions, and they are not the same risk: + +* **A forged body that reads as ``block`` is safe.** The discriminator + makes it more restrictive, not less. Worth pinning anyway, because + the naive fix for "a foreign body must not become permission" is to + start trusting unknown fields, and that would open this direction. + +* **A forged body that reads as ``allow`` is the real hole.** The + pre-existing guard (`runtime.py::_require_gate_decision`) catches a + non-object, a missing ``decision``, and an unrecognised ``decision`` + string. It does not catch ``{"decision": "allow"}`` — which passes + every check it applies. And the missing ``decision_source`` is + currently read as *more* trustworthy than ``"fallback"``: the + runtime's rule is ``decision_source != FALLBACK → honour the wire + decision``, so a body with no provenance at all is honoured as if + it came from the gateway. + +The second half of the file pins the property the fix must not break: +a refusal the SDK cannot classify raises, and raises a type the +fail-open arms cannot catch. A captive portal that returns +``{"decision": "block"}`` with no category must not be laundered into +an allow by an exception handler, which is the same bypass reached +through the error path rather than the success path. +""" + +from __future__ import annotations + +import httpx +import pytest +import respx + +from nullrun.breaker.categories import ( + NullRunUnclassifiedRefusalError, + is_gate_refusal, + resolve_refusal_category, +) +from nullrun.breaker.exceptions import ( + NullRunError, + NullRunInfrastructureError, + NullRunMalformedGateResponseError, + NullRunTransportError, +) +from nullrun.runtime import NullRunRuntime + +BASE_URL = "https://api.test.nullrun.io" +GATE_URL = f"{BASE_URL}/api/v1/gate" + + +def _html_portal() -> httpx.Response: + """The ordinary captive portal: a login page, HTTP 200.""" + return httpx.Response( + 200, + text="Sign in to continue" + "
username password
", + headers={"content-type": "text/html"}, + ) + + +def _json_echo(body: dict, *, status: int = 200) -> httpx.Response: + """A JSON answer from something that is not NULLRUN.""" + return httpx.Response(status, json=body) + + +class TestForeignBodyCannotBecomePermission: + """The success path. A body is permission only if it is a verdict.""" + + def test_captive_portal_html_does_not_authorise(self, make_runtime, mock_api): + respx.post(GATE_URL).mock(return_value=_html_portal()) + rt = make_runtime() + with pytest.raises(NullRunError): + rt.check_workflow_budget() + + def test_bare_allow_object_without_provenance_does_not_authorise( + self, make_runtime, mock_api + ): + """The hole this file was written for. + + `{"decision": "allow"}` satisfies every check + `_require_gate_decision` applies today: it is a dict, and + `decision` is a known string. What it is missing is any + statement of WHO decided — and the SDK's own rule reads a + missing `decision_source` as "not fallback", i.e. as a + gateway decision to be honoured. + + A real `/gate` answer always carries `decision_source`: + `GateResponse.decision_source` is a non-`Option` `String` + with no `skip_serializing_if` (`gate/internal.rs:638`), so + serde cannot omit it. The same holds for `decision` itself + (`:637`). `GateResponseBody` in `schemas.rs` does make + `decision_source` optional, but that struct is referenced + only from `openapi.rs` — it is the documentation schema, not + the wire — so it is not evidence that a real answer can lack + the field. + + An on-path responder that gets this JSON in front of the SDK + authorises a call no policy engine evaluated, and every + downstream signal agrees it was authorised: the runtime sees + a gateway decision, `/track` books the cost against a policy + that was never consulted, and the audit trail records an + allow. There is no later point at which it can be caught. + """ + respx.post(GATE_URL).mock(return_value=_json_echo({"decision": "allow"})) + rt = make_runtime() + with pytest.raises(NullRunError): + rt.check_workflow_budget() + + @pytest.mark.parametrize( + "body", + [ + pytest.param({}, id="empty-object"), + pytest.param({"status": "ok"}, id="unrelated-json"), + pytest.param({"decision": None}, id="decision-null"), + pytest.param({"decision": "ok"}, id="unknown-decision-string"), + pytest.param({"decision": "ALLOW"}, id="wrong-case-decision"), + pytest.param({"decision": ["allow"]}, id="decision-wrong-type"), + pytest.param([{"decision": "allow"}], id="json-array"), + pytest.param("allow", id="bare-json-string"), + ], + ) + def test_shapes_without_a_verdict_all_raise( + self, body, make_runtime, mock_api + ): + """The cases the existing guard already covers. + + Pinned so the provenance check added for the `decision`-only + body is not mistaken for the whole defence, and so a future + loosening of one arm is visible. + """ + respx.post(GATE_URL).mock(return_value=_json_echo(body)) + rt = make_runtime() + with pytest.raises(NullRunMalformedGateResponseError): + rt.check_workflow_budget() + + def test_a_real_allow_still_authorises(self, make_runtime, mock_api): + """Counter-test. The guard must not cost a single real call. + + Shaped from the live-captured envelope the SDK's own + `test_adr063_infra_refusal_is_not_fail_closed.py` documents: + the mandatory keys, with `remaining_budget_cents` left off + because it is `Option` on `GateResponse` and absent is + legitimate. + + `check_workflow_budget` returns `None` on success — it is a + "raise if refused" pre-flight, not a query — so the property + asserted here is that it does not raise. + """ + respx.post(GATE_URL).mock( + return_value=_json_echo( + { + "decision": "allow", + "decision_source": "gateway", + "explanation": "within budget", + "policy_version": 3, + "policy_id": "0f5a0e1c-2b6d-4c1a-9f3e-7d8b2a4c6e90", + "remaining_budget_cents": 120_000, + } + ) + ) + rt = make_runtime() + assert rt.check_workflow_budget() is None + + def test_fallback_decision_source_still_authorises( + self, make_runtime, mock_api + ): + """The synthetic SDK-made fallback keeps working. + + It is produced by this process, not the wire, so it is + exactly the body a naive "trust only gateway bodies" rule + would break — and ADR-008's fail-OPEN on a dead backend + depends on it working. + """ + respx.post(GATE_URL).mock(side_effect=httpx.ConnectError("connection refused")) + rt = make_runtime() + assert rt.check_workflow_budget() is None + + +class TestForgedRefusalCannotBecomePermissionEither: + """The error path. A forged `block` is safe; a forged one that + raises must not be laundered into an allow by a handler.""" + + def test_forged_block_is_classified_not_guessed(self, make_runtime, mock_api): + """A forged refusal is treated exactly like a real one. + + It has to be. The discriminator cannot distinguish them, and + trying to would mean the SDK trusts *some* bodies more than + others — which is the defect, not the fix. A forged `block` + stops the agent, which is the safe direction. + """ + respx.post(GATE_URL).mock( + return_value=_json_echo( + {"decision": "block", "decision_source": "gateway"}, status=503 + ) + ) + rt = make_runtime() + with pytest.raises(NullRunUnclassifiedRefusalError): + rt.check_workflow_budget() + + def test_forged_block_never_reaches_the_agent(self, make_runtime, mock_api): + respx.post(GATE_URL).mock( + return_value=_json_echo( + { + "decision": "block", + "decision_source": "gateway", + "category": "denied", + "agent_message": "not permitted", + }, + status=200, + ) + ) + rt = make_runtime(on_denied="message") + with pytest.raises(NullRunError): + rt.check_workflow_budget() + + def test_unclassified_refusal_is_not_a_transport_error(self): + """Structural pin on the bypass that makes the above moot. + + `check_workflow_budget` fails OPEN in its `except + NullRunTransportError` arms. If + `NullRunUnclassifiedRefusalError` were a subclass of that, + every one of the tests above would still pass — the exception + is raised, caught, converted to `None`, and the agent + proceeds — and only this assertion would notice. + + They are siblings today: both descend from + `NullRunInfrastructureError`, neither from the other. That is + load-bearing and cheap to break by reordering the class + hierarchy, so it is asserted rather than assumed. + """ + assert issubclass(NullRunUnclassifiedRefusalError, NullRunInfrastructureError) + assert not issubclass( + NullRunUnclassifiedRefusalError, NullRunTransportError + ), ( + "an unclassifiable refusal would be swallowed by the fail-OPEN " + "arm and become an allow — the exact bypass this pins" + ) + assert not issubclass( + NullRunMalformedGateResponseError, NullRunTransportError + ), ( + "a body that is not a verdict would likewise be swallowed; " + "`NullRunProtocolError` must stay a sibling of the transport " + "errors, not a subclass" + ) + + +class TestClassifierNeedsNoProvenanceOfItsOwn: + """`is_gate_refusal` is a pure predicate and must stay one. + + It runs before any provenance check, on whatever body arrived. + Adding a provenance requirement here would be the tempting fix and + the wrong one: this function's job is to answer "did the gate + rule?", and it is called on the *wire* body before the transport + has decided whether that wire is trustworthy. Widening it would + turn the classifier into a second, differently-behaving gate. + """ + + def test_it_answers_only_about_the_decision_field(self): + assert is_gate_refusal({"decision": "block"}) is True + assert is_gate_refusal({"decision": "allow"}) is False + assert is_gate_refusal({}) is False + assert is_gate_refusal(None) is False + + def test_a_missing_category_raises_rather_than_returning_none(self): + """A body that IS a refusal must not degrade to "not one". + + `resolve_refusal_category` returning `None` means "this was + never a gate refusal, keep your existing handling" — and the + caller's existing handling for a 5xx is fail-open. So the + absent-category arm has to raise, which is what it does. The + case is pinned because collapsing it to a `None` return is a + one-word change that silently reinstates the allow. + """ + with pytest.raises(NullRunUnclassifiedRefusalError): + resolve_refusal_category({"decision": "block", "decision_source": "gateway"}) + + +class TestRuntimeIsNotTheOnlyEntrypoint: + """The same question on the `/execute` path. + + `MCPAdapter.call_tool` is the second enforcement path and it reads + `/api/v1/execute` refusals through a different parser. A captive + portal in front of that endpoint is the same threat, so the + negative direction is pinned there too: a refusal must stop the + call, whatever it is. + """ + + def test_execute_refusal_stops_the_mcp_server(self, mock_api): + from nullrun.toolbox.mcp import MCPAdapter + + class _Ann: + readOnlyHint = False + destructiveHint = True + openWorldHint = True + + class _Tool: + name = "create_issue" + annotations = _Ann() + + class _Client: + def __init__(self): + self.calls: list[str] = [] + + def list_tools(self): + return [_Tool()] + + def call_tool(self, name, arguments=None, **kwargs): + self.calls.append(name) + return "ok" + + execute_url = f"{BASE_URL}/api/v1/execute" + respx.post(execute_url).mock( + return_value=_json_echo( + { + "decision": "block", + "decision_source": "gateway", + "details": {"error_code": "TOOL_BLOCKED"}, + }, + status=403, + ) + ) + client = _Client() + adapter = MCPAdapter( + server_name="github", + mcp_client=client, + runtime=NullRunRuntime( + api_key="test-key-12345678", + secret_key="test-secret-deterministic", + api_url=BASE_URL, + polling=False, + ), + ) + with pytest.raises(NullRunError): + adapter.call_tool("create_issue", {"repo": "acme/api"}) + assert client.calls == [], ( + "a refusal must stop the MCP server, and a foreign responder " + "must not be able to authorise the tool call" + ) diff --git a/tests/test_v3_wire_contract.py b/tests/test_v3_wire_contract.py index 194f86a..b125c3a 100644 --- a/tests/test_v3_wire_contract.py +++ b/tests/test_v3_wire_contract.py @@ -3046,11 +3046,23 @@ def test_outcome_approved_triggers_consume_approval( rt = make_runtime() # /gate returns require_approval — SDK must block on WS. + # + # `decision_source` / `explanation` / `policy_version` are the + # three non-`Option` fields on the backend's `GateResponse` + # (`gate/internal.rs:637-641`), so a real answer always + # carries them. This fixture omitted all three, which the + # provenance guard in `runtime.py::_require_gate_decision` + # correctly rejects as "not a gate answer" — a test body is + # not evidence of what the wire looks like, and this one had + # drifted from it. respx.post(f"{BASE_URL}/api/v1/gate").mock( return_value=Response( 200, json={ "decision": "require_approval", + "decision_source": "gateway", + "explanation": "operator approval required", + "policy_version": 1, "approval_id": "apr-success", "execution_id": "exec-success-1", "approval_timeout_seconds": 60, From c2e52d52dc2445e9eb659eeeb43a7832415808f7 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 12:51:21 +0400 Subject: [PATCH 13/20] docs(readme): state the provenance boundary of server-authoritative enforcement Limitation 4. The three existing entries describe what the SDK does not enforce; this one describes what it cannot verify. Every enforcement claim in the README rests on a premise it never said out loud: that the JSON on the HTTPS connection was written by NullRun. The SDK now checks for it -- a /gate body with no usable decision_source is rejected rather than acted on -- but that is a field in the body, not a signature. An on-path responder that can produce a well-formed body including decision_source defeats it, and needs neither the API key nor an HMAC bypass. NullRun's answers are not signed, so there is no after-the-fact detection either: the audit trail would record the allow. Also extends the fail-open summary with the not-a-verdict case, which is newly user-visible as NullRunMalformedGateResponseError. This is the honest boundary of "server-authoritative": authoritative against a client, not against an attacker who owns the path. --- README.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 0e64775..455c048 100644 --- a/README.md +++ b/README.md @@ -356,7 +356,7 @@ require tests for new public API, and run `ruff` + `mypy` in CI. ## Known limitations -Three things this SDK does not do. All three are enforcement-relevant, so they are stated here rather than left to be discovered during an incident. Each was verified against the code before being written down. +Four things this SDK does not do. All four are enforcement-relevant, so they are stated here rather than left to be discovered during an incident. Each was verified against the code before being written down. **1. A failed security check arrives as a 503, and only this version of the SDK stops on it.** When the backend cannot evaluate the security check itself, it refuses with a 503 carrying a `category` field. This SDK reads that field and fails **closed** — the call is refused. An SDK older than the category work has nothing to read: the 503 is turned into a synthetic `FALLBACK` decision, and `check_workflow_budget` fails **open** on a `FALLBACK` source — the call proceeds. So during a partial backend outage, enforcement differs by SDK version. A genuine outage (not a failed check) still fails open on every version, which is deliberate: a dead backend must not freeze your agent loop. @@ -366,9 +366,11 @@ If you need this guarantee today, pin the SDK version. Do not assume a refused c **3. Pause and kill both reach the agent as a 403.** There is no separate status to branch on. `WORKFLOW_PAUSED` and `WORKFLOW_INACTIVE` are served as the same 403 from the same key; the only thing distinguishing them is the operator-facing text, which the backend deliberately keeps distinct because they mean opposite things about whether the run will resume. If you write support tooling, key off the error code, not the status. Separately, the SDK can observe pause/kill ahead of the next gate call via `check_control_plane` (WebSocket push, or a `/status` poll), which raises `WorkflowPausedException` / `NullRunWorkflowKilledError` locally. +**4. The SDK can tell an answer from an outage; it cannot tell NULLRUN from anything else on the network.** Everything above rests on one premise: that the JSON arriving on the SDK's HTTPS connection was written by NullRun. The SDK checks for it — a `/gate` body with no usable `decision_source` is rejected as `NullRunMalformedGateResponseError` rather than acted on — but that check is a *provenance field in the body*, not a signature. The field is safe to require because the backend always sends it (`GateResponse.decision_source` is a non-optional field on the wire), and requiring it stops the ordinary case: a captive portal or corporate proxy returning `{"decision": "allow"}` no longer authorises a call. What it does not stop is an on-path responder that can produce a well-formed body *including* `decision_source`. Such a responder needs neither your API key nor an HMAC bypass — only to answer before the real backend does — and NullRun's answers are not signed, so it cannot be detected after the fact either: the audit trail would record the allow. Use `https://`, and if you operate on a network with TLS interception, exclude `api.nullrun.io` from it. That is the honest boundary of server-authoritative enforcement: it is authoritative against a *client*, not against an attacker who owns the path. + ### What does fail open -Fail-open here is narrow and deliberate, and the authoritative table lives in `runtime.py` (ADR-008). In short: a **transport** failure on the check path is open, so an unreachable backend cannot freeze your agent; a **wire response that names an enforcement failure** is closed, because the backend made a decision and the SDK will not overrule it; a **401** is closed, because no retry fixes a revoked key; and the `/execute` path is closed by default (`FallbackMode.STRICT`). +Fail-open here is narrow and deliberate, and the authoritative table lives in `runtime.py` (ADR-008). In short: a **transport** failure on the check path is open, so an unreachable backend cannot freeze your agent; a **wire response that names an enforcement failure** is closed, because the backend made a decision and the SDK will not overrule it; a **body that is not a verdict at all** — no `decision`, no `decision_source`, or an unrecognised value in either — is closed, because something answered and what it said was not a decision, and reading that as permission would let a non-NullRun responder authorise a call no policy engine evaluated; a **401** is closed, because no retry fixes a revoked key; and the `/execute` path is closed by default (`FallbackMode.STRICT`). --- From a3ea0ce55566bdd474a22ff2b67ad4380681464f Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 13:06:02 +0400 Subject: [PATCH 14/20] style: clear the three lint errors this branch's own test files carry ruff runs over src/ only in CI, so none of these would have failed a build -- but all three files are new on this branch (neither exists on origin/master), so they are this branch's to leave clean. Two F841: the `rt = make_runtime(...)` binding was never used, because @protect resolves the runtime from the context, not the local. The call is load-bearing; the assignment was not. One I001: nullrun.runtime sorted after nullrun.toolbox. --- tests/test_mcp_refusal_categories.py | 3 +-- tests/test_sensitive_fail_open_guard.py | 7 +++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/test_mcp_refusal_categories.py b/tests/test_mcp_refusal_categories.py index 6a6a861..d980944 100644 --- a/tests/test_mcp_refusal_categories.py +++ b/tests/test_mcp_refusal_categories.py @@ -41,14 +41,13 @@ import pytest import respx - from nullrun.breaker.exceptions import ( NullRunBlockedException, NullRunDeniedError, NullRunError, ) -from nullrun.toolbox.mcp import MCPAdapter from nullrun.runtime import NullRunRuntime +from nullrun.toolbox.mcp import MCPAdapter EXECUTE_URL = "https://api.test.nullrun.io/api/v1/execute" diff --git a/tests/test_sensitive_fail_open_guard.py b/tests/test_sensitive_fail_open_guard.py index ff055bf..f3d0877 100644 --- a/tests/test_sensitive_fail_open_guard.py +++ b/tests/test_sensitive_fail_open_guard.py @@ -102,7 +102,9 @@ def test_sensitive_body_does_not_run_in_prod(self, make_runtime, mock_prod_api, even though the operator set the flag.""" from nullrun.decorators import protect - rt = make_runtime(api_url=PROD_URL) + # Not bound: `@protect` resolves the runtime from the context, + # so the constructor call is the load-bearing part here. + make_runtime(api_url=PROD_URL) monkeypatch.setenv(_FLAG, "1") respx.post(f"{PROD_URL}/api/v1/execute").mock( @@ -129,7 +131,8 @@ def test_sensitive_body_runs_in_dev_with_flag(self, make_runtime, mock_api, monk from nullrun.breaker.exceptions import NullRunBlockedException from nullrun.decorators import protect - rt = make_runtime(api_url=BASE_URL) + # Not bound, for the same reason as the prod case above. + make_runtime(api_url=BASE_URL) monkeypatch.setenv(_FLAG, "1") respx.post(EXECUTE_URL).mock( From 660954a6d39e2caae844b3dd8998185d990f7517 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 13:15:49 +0400 Subject: [PATCH 15/20] docs(readme): name the trust boundary precisely, and the second version skew Limitation 4 said "use https://", which understates the problem. It implied https was sufficient, when the residual is specifically an on-path responder holding a certificate the OS already trusts for api.nullrun.io -- i.e. corporate TLS interception, which is ordinary on a managed network and which https alone does nothing about. Corrected to state what each layer does and does not give: - certificate verification is on and CANNOT be disabled by configuration. `verify_cert = True` (transport.py:572) and the only override is NULLRUN_TLS_CA_CERT (:568), which swaps in a trust anchor you chose and is still verification. Grepped exhaustively: the only TLS env vars in the SDK are NULLRUN_TLS_CA_CERT / _CLIENT_CERT / _CLIENT_KEY, none of which turns verification off. - plain http is refused (InsecureTransportError, :526). - responses are not signed, so no after-the-fact detection: the audit trail would faithfully record an allow the gate never gave. The consequence is stated as the operational control it is -- exclude api.nullrun.io from interception and verify it stays excluded -- and not as something the SDK does for you. Also adds the old-SDK note beside limitation 1, since it is the same version skew pointing the same way and a reader who pins for the 503 guarantee should know they also need it for the forged-allow case: both a real refusal and a fabricated permission are read permissively on an older SDK, and neither shows up in the SDK's output. --- README.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 455c048..612926b 100644 --- a/README.md +++ b/README.md @@ -362,11 +362,19 @@ Four things this SDK does not do. All four are enforcement-relevant, so they are If you need this guarantee today, pin the SDK version. Do not assume a refused call implies the backend rejected the call. +**Older SDKs fail open on a forged allow, too — and this is the second version-skewed behaviour, so pin for it too.** The `decision_source` check described in limitation 4 arrived with the same release. An SDK older than it accepts `{"decision": "allow"}` from any responder, because it reads a missing `decision_source` as "not `fallback`" and therefore as authoritative. The two skews point the same way and are worth knowing together: on an older SDK, both a real backend refusal *and* a fabricated permission are read the permissive way, and neither is visible in the SDK's own output — the call simply proceeds. Pin the version if either matters to you. + **2. The gate circuit-breaker's trip mode is a server-side setting, and `LogOnly` does not block.** When the gate's circuit breaker trips, what happens is decided by `NULLRUN_GATE_CB_TRIP_ENFORCEMENT_MODE` on the server, not by anything in this SDK. In `LogOnly` the trip is recorded and alerted on, but tripped workflows still pass `/check`. The production boot check refuses to start unless the variable is explicitly set to `Enforce` or `LogOnly`, so a deploy cannot inherit the dev default (`detect_mode()` still falls back to `LogOnly` when unset outside production) — but an operator who chooses `LogOnly` is choosing non-enforcement, knowingly. If your compliance story depends on breaker trips being enforced, confirm that value with whoever operates the deployment. **3. Pause and kill both reach the agent as a 403.** There is no separate status to branch on. `WORKFLOW_PAUSED` and `WORKFLOW_INACTIVE` are served as the same 403 from the same key; the only thing distinguishing them is the operator-facing text, which the backend deliberately keeps distinct because they mean opposite things about whether the run will resume. If you write support tooling, key off the error code, not the status. Separately, the SDK can observe pause/kill ahead of the next gate call via `check_control_plane` (WebSocket push, or a `/status` poll), which raises `WorkflowPausedException` / `NullRunWorkflowKilledError` locally. -**4. The SDK can tell an answer from an outage; it cannot tell NULLRUN from anything else on the network.** Everything above rests on one premise: that the JSON arriving on the SDK's HTTPS connection was written by NullRun. The SDK checks for it — a `/gate` body with no usable `decision_source` is rejected as `NullRunMalformedGateResponseError` rather than acted on — but that check is a *provenance field in the body*, not a signature. The field is safe to require because the backend always sends it (`GateResponse.decision_source` is a non-optional field on the wire), and requiring it stops the ordinary case: a captive portal or corporate proxy returning `{"decision": "allow"}` no longer authorises a call. What it does not stop is an on-path responder that can produce a well-formed body *including* `decision_source`. Such a responder needs neither your API key nor an HMAC bypass — only to answer before the real backend does — and NullRun's answers are not signed, so it cannot be detected after the fact either: the audit trail would record the allow. Use `https://`, and if you operate on a network with TLS interception, exclude `api.nullrun.io` from it. That is the honest boundary of server-authoritative enforcement: it is authoritative against a *client*, not against an attacker who owns the path. +**4. The SDK trusts the channel, and says so rather than pretending otherwise.** Everything above rests on one premise: that the JSON arriving on the SDK's HTTPS connection was written by NullRun. The SDK checks for it — a `/gate` body with no usable `decision_source` is rejected as `NullRunMalformedGateResponseError` rather than acted on — but that is a *field in the body*, not a signature, and a field is only as trustworthy as the channel carrying it. + +What the channel does give you, verified in `transport.py`: certificate verification is **on and cannot be switched off by configuration** — `verify_cert` is `True` (`:572`) and there is no env var that sets it to `False`; the only override, `NULLRUN_TLS_CA_CERT` (`:568`), replaces the trust anchor with one you chose explicitly and is still verification. Plain `http://` is refused outright (`InsecureTransportError`, `:526`). So a passive network observer cannot pose as NullRun, and the ordinary captive portal — which returns a login page, or JSON without a `decision` — is rejected. + +What remains is specific, and it is not "use https". It is: **an on-path responder that can present a certificate the operating system already trusts for `api.nullrun.io`.** That is what corporate TLS interception installs, and it is not exotic — it is a normal thing to find on a managed network. Such a responder can return a perfectly well-formed body *including* `decision_source: "gateway"`; it needs neither your API key nor an HMAC bypass, only to answer before the real backend. NullRun's responses are **not signed**, so there is no after-the-fact detection either — the audit trail would faithfully record an allow that the gate never gave. + +So the honest boundary is: server-authoritative enforcement is authoritative against a *client* and against a *network observer*, not against an attacker who terminates TLS inside your trust store. If you operate on such a network, exclude `api.nullrun.io` from interception and check that it stays excluded — that is an operational control, not something the SDK can do for you. ### What does fail open From 6d0285b61f8838687569069173313299d91ea4e6 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 13:55:52 +0400 Subject: [PATCH 16/20] test(protect): prove refusals and provenance at the decorator level Every other file in this cluster calls runtime.check_workflow_budget() directly. That is the wrong level for the question a user has, because @protect is what they call -- and @protect is a separate code path with its own try, its own except BaseException, and its own two wrappers (sync and async). Reading decorators.py says the refusals propagate. That is exactly the reasoning that let DEF-MP-TS12-ENF-01 ship: check_workflow_budget's except Exception looked harmless in isolation too, until it was reached by a real 401. 14 tests, four properties: - a denied / budget / unclassifiable refusal, and a body with no decision_source, do NOT run the decorated body; - the counter-tests: a real allow, a connection failure under on_denied="message", and a 502 with no refusal body all DO run it (a suite of only "did not run" assertions passes against a decorator that refuses everything); - on_denied reaches @protect for category=denied and, proven by the negative parametrisation, does not reach budget/halt; - the async wrapper is not a separate property, so the refusal, the forged allow and the real allow are each proven on it too. Mutation-verified: deleting the decision_source guard in _require_gate_decision turns exactly the sync and async forged-allow tests red (DID NOT RAISE), and the other 12 stay green. Verified in a clean python:3.11 container on the unmutated tree: ruff check src/ tests/ -- All checks passed mypy src/ -- no issues in 37 source files pytest -q -- 1575 passed, 1 skipped --- tests/test_protect_enforcement_end_to_end.py | 313 +++++++++++++++++++ 1 file changed, 313 insertions(+) create mode 100644 tests/test_protect_enforcement_end_to_end.py diff --git a/tests/test_protect_enforcement_end_to_end.py b/tests/test_protect_enforcement_end_to_end.py new file mode 100644 index 0000000..c714627 --- /dev/null +++ b/tests/test_protect_enforcement_end_to_end.py @@ -0,0 +1,313 @@ +"""The enforcement properties, proven through `@protect`. + +Every other file in this cluster tests the refusal handling by calling +`runtime.check_workflow_budget()` directly. That is the wrong level for +the question users actually care about, because `@protect` is what they +call — and `@protect` is a separate code path with its own `try`, +its own `except BaseException`, and its own two wrappers (sync and +async). + +Reading `decorators.py` says the refusals propagate: the +`except BaseException` arm only re-wraps kill/pause and then re-raises. +But "reading the code says so" is exactly the reasoning that let +DEF-MP-TS12-ENF-01 ship — `check_workflow_budget`'s `except Exception` +looked harmless in isolation too, until it was reached by a real 401. + +So this file runs the same properties through the decorator and asserts +the only thing that matters to a caller: **did the function body run?** + +The counter-test is not optional. Every assertion here is "the body did +not run", and a suite of only those would pass if `@protect` refused +everything, including calls it should have allowed. `test_a_real_allow +_still_runs_the_body` is what stops that reading. +""" + +from __future__ import annotations + +import httpx +import pytest +import respx + +from nullrun.breaker.categories import NullRunUnclassifiedRefusalError +from nullrun.breaker.exceptions import ( + NullRunBlockedException, + NullRunBudgetError, + NullRunDeniedError, + NullRunError, + NullRunMalformedGateResponseError, +) +from nullrun.decorators import protect + +BASE_URL = "https://api.test.nullrun.io" +GATE_URL = f"{BASE_URL}/api/v1/gate" +EXECUTE_URL = f"{BASE_URL}/api/v1/execute" + + +def _gate_allow(**extra) -> httpx.Response: + """A real `/gate` allow, per the live-captured envelope. + + `decision_source`, `explanation` and `policy_version` are the + non-`Option` fields on the backend's `GateResponse` + (`gate/internal.rs:637-641`); all three are present on every real + answer, and omitting any of them is what makes a body stop being a + verdict. + """ + body = { + "decision": "allow", + "decision_source": "gateway", + "explanation": "within budget", + "policy_version": 1, + } + body.update(extra) + return httpx.Response(200, json=body) + + +def _gate_refusal(code: str, category: str | None, *, status: int = 403) -> httpx.Response: + body = { + "decision": "block", + "decision_source": "gateway", + "explanation": f"refused: {code}", + "error_code": code, + } + if category is not None: + body["category"] = category + if category == "denied": + body["agent_message"] = "The operator has not allowed this tool." + return httpx.Response(status, json=body) + + +@pytest.fixture +def ran() -> list[str]: + """Records whether the decorated body executed. + + A list rather than a bool so a test failure can say how many times + it ran, which distinguishes "ran once then refused" from "ran for + every attempt". + """ + return [] + + +def _charge(ran: list[str], amount: int = 100) -> str: + @protect + def charge_card(amount: int) -> str: + ran.append("body") + return f"charged:{amount}" + + return charge_card(amount) + + +class TestProtectHonoursRefusals: + """A refusal through the decorator stops the call.""" + + def test_denied_refusal_does_not_run_the_body(self, make_runtime, mock_api, ran): + respx.post(GATE_URL).mock( + return_value=_gate_refusal("TOOL_BLOCKED", "denied", status=403) + ) + make_runtime() + with pytest.raises(NullRunError): + _charge(ran) + assert ran == [], "the decorated body ran on a refused call" + + def test_budget_refusal_does_not_run_the_body(self, make_runtime, mock_api, ran): + respx.post(GATE_URL).mock( + return_value=_gate_refusal("BUDGET_HARD_BLOCKED", "budget", status=402) + ) + make_runtime() + with pytest.raises(NullRunBudgetError): + _charge(ran) + assert ran == [], "the decorated body ran on an exhausted budget" + + def test_forged_allow_without_provenance_does_not_run_the_body( + self, make_runtime, mock_api, ran + ): + """Limitation 4's hole, at the level a user is actually exposed at. + + The body is `{"decision": "allow"}` and nothing else. Reaching + `@protect` matters here: a caller who never calls + `check_workflow_budget` directly — the normal case — is + protected only if the decorator's own `try` does not swallow + the malformed-response error on its way out. + """ + respx.post(GATE_URL).mock(return_value=httpx.Response(200, json={"decision": "allow"})) + make_runtime() + with pytest.raises(NullRunMalformedGateResponseError): + _charge(ran) + assert ran == [], ( + "a body with no decision_source authorised a real call through " + "the decorator — the whole point of the provenance check" + ) + + def test_unclassifiable_refusal_does_not_run_the_body( + self, make_runtime, mock_api, ran + ): + """A refusal the SDK cannot read must not be laundered into a call. + + `NullRunUnclassifiedRefusalError` is an + `NullRunInfrastructureError`, and `@protect` wraps its body in a + bare `except BaseException`. The two facts are compatible — + that handler re-raises — but compatibility is not evidence, and + this is the assertion that makes it evidence. + """ + respx.post(GATE_URL).mock( + return_value=_gate_refusal("BUDGET_HARD_BLOCKED", None, status=402) + ) + make_runtime() + with pytest.raises(NullRunUnclassifiedRefusalError): + _charge(ran) + assert ran == [], "an unreadable refusal became a call" + + +class TestProtectFailOpenStillHolds: + """The counter-tests. A dead backend must not freeze the agent.""" + + def test_a_real_allow_runs_the_body(self, make_runtime, mock_api, ran): + respx.post(GATE_URL).mock(return_value=_gate_allow()) + respx.post(EXECUTE_URL).mock(return_value=httpx.Response(200, json={})) + make_runtime() + assert _charge(ran) == "charged:100" + assert ran == ["body"], ( + "a real allow must still execute — a suite of 'body did not " + "run' assertions passes just as well against a decorator " + "that refuses everything" + ) + + def test_connection_failure_runs_the_body(self, make_runtime, mock_api, ran): + """ADR-008's promise, through the decorator. + + `on_denied="message"` is set deliberately: this is the most + permissive handling the host can ask for, and it still must not + convert an unreachable gate into a block. + """ + respx.post(GATE_URL).mock(side_effect=httpx.ConnectError("connection refused")) + make_runtime(on_denied="message") + _charge(ran) + assert ran == ["body"], "ADR-008 fail-open does not survive @protect" + + def test_a_502_with_no_refusal_body_runs_the_body(self, make_runtime, mock_api, ran): + """A 5xx that is not a gate answer is an outage, not a decision.""" + respx.post(GATE_URL).mock( + return_value=httpx.Response(502, text="Bad Gateway") + ) + make_runtime() + _charge(ran) + assert ran == ["body"] + + +class TestOnDeniedReachesProtect: + """The flag has to mean something at the decorator level too.""" + + def test_denied_with_message_enabled_carries_the_server_text( + self, make_runtime, mock_api, ran + ): + respx.post(GATE_URL).mock( + return_value=_gate_refusal("TOOL_BLOCKED", "denied", status=403) + ) + make_runtime(on_denied="message") + with pytest.raises(NullRunDeniedError) as exc: + _charge(ran) + assert exc.value.model_safe_text() == "The operator has not allowed this tool." + assert isinstance(exc.value, NullRunBlockedException) + assert ran == [] + + @pytest.mark.parametrize( + "code,category,status,expected", + [ + ("BUDGET_HARD_BLOCKED", "budget", 402, NullRunBudgetError), + ("WORKFLOW_PAUSED", "halt", 403, NullRunBlockedException), + ], + ) + def test_non_denied_categories_never_become_a_message( + self, make_runtime, mock_api, ran, code, category, status, expected + ): + """The negative set, at the decorator level. + + The same reasoning as one level down, and it matters more for a + decorator: the caller's `except NullRunBlockedException` is + around a whole function, so a `budget` wall surfacing as + "that tool is not allowed" is a message an agent will act on by + trying something else. + """ + respx.post(GATE_URL).mock( + return_value=_gate_refusal(code, category, status=status) + ) + make_runtime(on_denied="message") + with pytest.raises(expected) as exc: + _charge(ran) + assert not isinstance(exc.value, NullRunDeniedError), ( + f"{code} is category={category!r}; on_denied must not reach it" + ) + assert ran == [] + + def test_absent_category_raises_even_with_message_enabled( + self, make_runtime, mock_api, ran + ): + """The opt-in is a statement about `denied`, not permission to guess.""" + respx.post(GATE_URL).mock( + return_value=_gate_refusal("TOOL_BLOCKED", None, status=403) + ) + make_runtime(on_denied="message") + with pytest.raises(NullRunUnclassifiedRefusalError): + _charge(ran) + assert ran == [] + + +class TestAsyncProtectIsNotASeparatePath: + """`protect` returns two different wrappers. Both must hold. + + The sync and async bodies are separate implementations in + `decorators.py`, and the async one has its own `except Exception` + around the call. A property proven only on the sync path is a + property of half the decorator. + """ + + @pytest.mark.asyncio + async def test_refusal_does_not_run_the_async_body( + self, make_runtime, mock_api, ran + ): + respx.post(GATE_URL).mock( + return_value=_gate_refusal("TOOL_BLOCKED", "denied", status=403) + ) + make_runtime() + + @protect + async def charge_card(amount: int) -> str: + ran.append("body") + return "charged" + + with pytest.raises(NullRunError): + await charge_card(100) + assert ran == [] + + @pytest.mark.asyncio + async def test_forged_allow_does_not_run_the_async_body( + self, make_runtime, mock_api, ran + ): + respx.post(GATE_URL).mock(return_value=httpx.Response(200, json={"decision": "allow"})) + make_runtime() + + @protect + async def charge_card(amount: int) -> str: + ran.append("body") + return "charged" + + with pytest.raises(NullRunMalformedGateResponseError): + await charge_card(100) + assert ran == [] + + @pytest.mark.asyncio + async def test_a_real_allow_runs_the_async_body(self, make_runtime, mock_api, ran): + respx.post(GATE_URL).mock(return_value=_gate_allow()) + respx.post(EXECUTE_URL).mock(return_value=httpx.Response(200, json={})) + make_runtime() + + @protect + async def charge_card(amount: int) -> str: + ran.append("body") + return "charged" + + assert await charge_card(100) == "charged" + assert ran == ["body"], ( + "the async counter-test matters as much as the sync one: " + "without it, 'body did not run' passes against an async " + "wrapper that refuses everything" + ) From 467cb1aa0a87836e3a1b5cf3c258c8ccc5e2b8a6 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 14:00:58 +0400 Subject: [PATCH 17/20] docs(changelog): record the second half of DEF-MP-TS12-ENF-01 0.19.0 is already on origin/master and its entry closed the paths found by reading the code. These commits closed the paths that only appeared once the properties were asserted end-to-end, and none of them were in the release notes -- so the two most serious items (a forged allow authorised by decision_source absence, and the production-usable sensitive fail-open opt-out) were documented only in README. Filed as [Unreleased] rather than inventing a version number. That is deliberate: the behaviour change is real and needs a migration note, not a patch number chosen by accident. An unclassifiable refusal now raises where 0.19.0 let the call proceed, and a hand-rolled /gate test double must now carry decision_source. Whoever cuts the release picks 0.19.1 or 0.20.0 with that in hand. Verified citation: GateResponse.decision_source is a non-Option String at backend/src/proxy/http/gate/internal.rs:638. --- CHANGELOG.md | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c63d987..97b3cbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,95 @@ +## [Unreleased] + +The remaining half of `DEF-MP-TS12-ENF-01` (QA cycle RUN_ID +20260929T1338). 0.19.0 closed the paths it found by reading; these +close the ones that only showed up when the properties were asserted +end-to-end. **Not yet released, and the version number is not chosen** — +the behaviour change below is the reason that is worth a decision +rather than a default: an unclassifiable refusal now raises where +0.19.0 let the call proceed, so anyone who was relying on that has a +migration to make and should be told so in the release note. + +### Security + +- **A `/gate` body must state who decided before it counts as a + verdict.** `_require_gate_decision` rejects a body whose + `decision_source` is absent or unrecognised, raising + `NullRunMalformedGateResponseError`. The runtime's rule was + `decision_source != fallback → honour the wire decision`, which + read a *missing* `decision_source` as more trustworthy than + `fallback`. So `{"decision": "allow"}` — a captive portal's login + JSON, an intercepting proxy's stub, anything on-path — was enough to + authorise a call no policy engine evaluated. The field is a + non-`Option` `String` on the backend's `GateResponse` + (`gate/internal.rs:638`) and every producer sets `"gateway"`, so a + real answer always carries one; there is no legitimate body this + rejects. **This is a behaviour change on the `/gate` path**: a + hand-rolled `/gate` response in an existing test double must now + include `decision_source`. +- **`NULLRUN_SENSITIVE_FAIL_OPEN` is refused against production.** It + was read straight into the enforcement path, letting a sensitive + tool's body run with no policy evaluation at all. Its sibling + `NULLRUN_SKIP_BUDGET_CHECK` has been production-guarded since it + was caught doing the same; the asymmetry was an oversight, and this + half is the more dangerous one — that one skips a pre-flight, this + one skips the gate. The guard *refuses* the bypass rather than + raising, so enforcement falls back to its own fail-CLOSED default + and the attempt is logged at ERROR with a metric. Requires both + `NULLRUN_SENSITIVE_FAIL_OPEN=1` and `NULLRUN_ALLOW_SENSITIVE_FAIL_OPEN=1`; + the documented dev/test use is unchanged. +- **The non-prod budget bypass is visible.** When + `NULLRUN_SKIP_BUDGET_CHECK=1` skips the check, it now logs and + increments a metric instead of being indistinguishable from a normal + call. + +### Changed + +- **Every gate refusal is classified, and an unclassifiable one + raises.** `NullRunUnclassifiedRefusalError` (an + `NullRunInfrastructureError`) is raised when a refusal carries no + `category` or one the SDK does not know. ADR-062 §2.2: absent or + unrecognised means *ask*, never *guess*. The exception hierarchy is + what makes this safe — `NullRunUnclassifiedRefusalError` and + `NullRunTransportError` are **siblings**, not parent and child, so + the fail-OPEN `except NullRunTransportError` arms cannot swallow it. +- **`on_denied` selects the shape of a `denied` refusal and nothing + else.** `on_denied="message"` turns a `category="denied"` refusal + into a `NullRunDeniedError` carrying the server-authored + `agent_message` — the only text the SDK guarantees is safe to relay + to a model. `budget` and `halt` keep their own exceptions even with + the flag set: an agent told "that tool is not allowed" has an + obvious next move, and that move walks into a budget wall or an + operator's stop. `infra` is not reached by this flag at all — a + 503 is converted by the 5xx band and the STRICT fallback, which is + the correct outcome (an outage is not a denial). +- **`/execute` refusals carry `category` and `agent_message`.** The + MCP path (`MCPAdapter.call_tool`) is a different enforcement path + from `/gate`, so anything the category work added to the `/gate` + block site was absent there by default. Both properties are now + proven on the real adapter rather than inferred from + `runtime.execute`'s lack of a fail-OPEN `except`. + +### Documentation + +- README "Known limitations" states the trust boundary precisely: + the SDK trusts the channel and says so. Certificate verification + cannot be switched off by configuration, plain `http` is refused, + and the residual risk is named — an on-path responder that can + present a certificate the OS already trusts for `api.nullrun.io`, + which is not an exotic thing to find on a managed network. NullRun's + responses are not signed, so there is no after-the-fact detection. +- README names the **second** version skew beside the first: an SDK + older than the `decision_source` check accepts a forged allow. Both + skews point the same way — on an older SDK a real refusal *and* a + fabricated permission are both read permissively, and neither is + visible in the SDK's output. Pin the version if either matters. +- The `transport.py` comment that claimed `is_fail_closed` marks a + fail-closed response on the wire is corrected. It does not: + `GateErrorCode::is_fail_closed` is an in-process Rust method that is + never serialised, and a client written against it could not have + found it. ADR-064 owns the actual discriminator (ADR-063 §4.7 + option 2, which pointed at the same non-existent marker). + ## [0.19.0] - 2026-09-30 Closes the SDK-side bypasses found auditing `DEF-MP-TS12-ENF-01` From 642ebefd6805080487bc0963c455ff30dbc8fc9c Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 14:46:30 +0400 Subject: [PATCH 18/20] docs(changelog): migration note for the refusal-category SDK Six differences from 0.19.0, written as things you can act on rather than as a list of changed internals. The third is the one worth reading twice: an unclassifiable refusal now raises where 0.19.0 let the call proceed, and NullRunUnclassifiedRefusalError is deliberately a SIBLING of NullRunTransportError, not a subclass -- so an existing `except NullRunTransportError:` fail-open arm will not catch it. That is the fix working, and it is also the thing most likely to turn a working loop into a throwing one. The note gives the exact import path (nullrun.breaker.categories -- it is not re-exported at the top level, which was verified, not assumed), the error_code, and the three options as three, with the consequence of each stated. Widening the arm to NullRunError is called out as worse than the hole being closed, because it also swallows real policy refusals. Every code shape in the note was executed against the installed package rather than reasoned about: recommended two-arm -> propagates (recommended) one-arm permissive -> swallowed == 0.19.0 behaviour, bypass restored widened to NullRunError -> a real policy refusal is swallowed too issubclass(NRError, NullRunError) = True --- CHANGELOG.md | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97b3cbe..05041f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,63 @@ migration to make and should be told so in the release note. proven on the real adapter rather than inferred from `runtime.execute`'s lack of a fail-OPEN `except`. +### Migration + +Six things differ from 0.19.0. Only the first three can surprise you +at runtime, and the third is the one worth reading twice. + +1. **A `/gate` test double must carry `decision_source`.** Real + backend answers always do — it is a non-`Option` `String` on + `GateResponse`. A hand-written fixture that omits it now raises + `NullRunMalformedGateResponseError`. Fix: add + `"decision_source": "gateway"` next to `"decision": "allow"`. +2. **`NULLRUN_SENSITIVE_FAIL_OPEN` against production now needs a + second variable.** `NULLRUN_ALLOW_SENSITIVE_FAIL_OPEN=1` must be + set too, otherwise the opt-out is refused, enforcement falls back + to its own fail-CLOSED default, and the attempt is logged at ERROR + with a metric. Non-production behaviour is unchanged. +3. **An unclassifiable refusal now raises where 0.19.0 let the call + proceed.** This is the intended fix, and it is the one that can + turn a working loop into a throwing one. + `NullRunUnclassifiedRefusalError` is importable from + `nullrun.breaker.categories` (it is *not* re-exported at the top + level) and carries `error_code="NR-P003"` and `retryable=True`. + It is a **`NullRunInfrastructureError` and a sibling of + `NullRunTransportError`** — deliberately *not* a subclass of it. + So an existing `except NullRunTransportError:` arm, which in + 0.19.0 caught everything the gate could not classify and failed + open, **will not catch this one**. That is the point: it is what + stops a failed-open arm from swallowing a refusal. If you have + such an arm, you have three options and they are not equivalent: + + ```python + from nullrun.breaker.categories import NullRunUnclassifiedRefusalError + from nullrun.breaker.exceptions import NullRunError, NullRunTransportError + + try: + ... + except NullRunUnclassifiedRefusalError: + raise # recommended: the SDK was right + except NullRunTransportError: + ... # still fails open, as before + ``` + + Catching it alongside `NullRunTransportError` restores 0.19.0's + behaviour exactly — which means it restores the bypass. Do not + widen the arm to `NullRunError`: that also swallows real policy + refusals, which is a larger hole than the one this closes. +4. **`on_denied` is new.** Default `"raise"`, which is 0.19.0's + behaviour. Set `"message"` to get a `NullRunDeniedError` carrying + the server-authored `agent_message` for `category="denied"` only. + Any other value raises `ValueError` at construction rather than + at first refusal. +5. **`NULLRUN_SKIP_BUDGET_CHECK` outside production now logs and + increments a metric** when it skips the check. Enforcement + behaviour is unchanged. +6. **`/execute` refusal bodies carry `category` and + `agent_message`.** Additive on the wire. If you parse that body + yourself and reject unknown keys, relax that. + ### Documentation - README "Known limitations" states the trust boundary precisely: From b38fcf587c43d140218a1d7805a1905bf84c4412 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 16:27:54 +0400 Subject: [PATCH 19/20] fix(protect): make @tool/@protect ordering irrelevant Priority 1 for DEF-MP-TS12-ENF-01 was "one @protect, rest under the hood", and the part nobody had tested is whether that survives the framework layer. It did not, in one direction. `@protect` above `@tool` collapsed the tool to a plain function. `functools.wraps`-based wrapping cannot preserve `.invoke`, `.name`, `.args_schema` or `.description`, so the agent loop could not bind it -- and a tool the loop cannot see produces no refusal. Measured on langchain-core 0.3.86: @protect # outer @tool # inner -> StructuredTool def f(query: str) -> str: ... # f is now , not StructuredTool # convert_to_openai_tool(f) -> NameError: name 'Annotated' is not defined That last line is the failure mode worth naming: the error a user sees is a langchain type-resolution error, not "your tool is not a tool any more". Enforcement was gone and the symptom pointed somewhere else. `protect` now duck-types the argument (`.invoke` + `.name`, then `.coroutine` before `.func` -- an async tool sets both, with `func` as a sync fallback that raises if called), wraps whichever callable is live, and returns the SAME object. Every other attribute is untouched, so the loop sees the tool it saw before. Duck-typed rather than isinstance because `langchain_core` is an optional dependency; the three attributes matched are the ones `BaseTool.run()`/`arun()` dispatch through. Three properties were established by probe on the real library before being written down, and all three are now pinned: - Ordering is irrelevant in BOTH directions. `@tool` outside already worked; the reverse now does too, and each has an allow counter-test so "the body did not run" cannot pass against a wrapper that refuses everything. - `handle_tool_error=True` cannot turn a refusal into model-visible text. It only catches `ToolException`; a `NullRunBudgetError` reaches the bare `except (Exception, KeyboardInterrupt)` arm and is re-raised. This is the DEF-MP-TS12-ENF-01 shape in a different costume, so it is pinned rather than assumed. - `@protect` with no `init()` fails LOUD. The lazy resolver propagates `NullRunAuthenticationError` (NR-A001) and the body does not run. FIX-4 removed the fallback that used to swallow this; the AST guard keeps it removed, and an AST check rather than a substring search because the docstring describes the very `except` being asserted gone. `on_denied="message"` is now exercised through a real `@tool` too, in both orders and both sync/async, with the negative set (budget, halt) kept at the framework layer: an agent told "that tool is not allowed" when the truth is "you are out of money" will look for another way to spend. Mutation-verified; each mutant ran the suite and the restore was byte-identical: sync arm converts refusal to text -> 2 red async arm converts refusal to text -> 1 red FIX-4 fallback in lazy resolver -> 2 red BaseTool in-place wrap removed -> 5 red on_denied category guard dropped -> 8 red across 3 files Three earlier mutation attempts were invalid and are not counted: an inner `try` cannot observe a `_protect_body.__enter__` raise, and one anchor matched `get_protected_runtime` instead of `_get_or_create_runtime`. Suite: 1597 passed, 1 skipped. ruff and mypy clean. --- src/nullrun/decorators.py | 48 ++ tests/test_protect_enforcement_end_to_end.py | 150 +++++++ tests/test_tool_ordering_real_langchain.py | 445 +++++++++++++++++++ 3 files changed, 643 insertions(+) create mode 100644 tests/test_tool_ordering_real_langchain.py diff --git a/src/nullrun/decorators.py b/src/nullrun/decorators.py index 63e350f..65ec4fb 100644 --- a/src/nullrun/decorators.py +++ b/src/nullrun/decorators.py @@ -489,6 +489,32 @@ def _safe_cancel_active_execution(reason: str | None = None) -> None: return +def _langchain_tool_attr(obj: object) -> str | None: + """Return ``"coroutine"`` / ``"func"`` if ``obj`` is a LangChain tool. + + Duck-typed on purpose: `langchain_core` is an OPTIONAL dependency, so + an ``isinstance`` check against its `BaseTool` would make this module + import it. The three attributes below are what `BaseTool.run()` / + `arun()` actually dispatch through (`base.py:864` and `:895` in + 0.3.86), so matching them is both cheaper and harder to get wrong than + a version-specific base class. + + Async is checked first: an async tool sets BOTH, with ``func`` as a + sync fallback that raises if called. Wrapping the wrong one would + leave the async path ungated. + + Returns None for a plain function, so the caller falls through to the + normal wrapping path with no behaviour change. + """ + if not hasattr(obj, "invoke") or not hasattr(obj, "name"): + return None + for attr in ("coroutine", "func"): + candidate = getattr(obj, attr, None) + if callable(candidate): + return attr + return None + + def protect(fn: F | None = None) -> F | Callable[[F], F]: """ Decorator that wraps a function in a NullRun span. @@ -688,6 +714,28 @@ def _protect_body(args: tuple[Any, ...], kwargs: dict[str, Any], unify_block: bo error=_safe_error_str(error), ) + # A LangChain `BaseTool` is an OBJECT, not a function. Decorating it + # with `functools.wraps`-based wrapping above produces a plain function + # that has lost `.invoke`, `.name`, `.args_schema` and `.description` -- + # so an agent loop cannot bind it and enforcement silently disappears. + # Measured 2026-10-01 on langchain-core 0.3.86: + # + # @protect # outer + # @tool # inner -> StructuredTool + # def f(...): ... + # # f is now a plain function; bind_tools() rejects it + # + # Rather than document "put @tool outside", wrap the tool IN PLACE and + # return the same object, so both orders enforce. `BaseTool` holds the + # callable in `.func` (sync) or `.coroutine` (async); we wrap whichever + # is present and leave every other attribute untouched, so the agent + # loop sees exactly the tool it saw before. + _tool_attr = _langchain_tool_attr(fn) + if _tool_attr is not None: + _original = getattr(fn, _tool_attr) + setattr(fn, _tool_attr, protect(_original)) + return fn + if inspect.iscoroutinefunction(fn): @functools.wraps(fn) diff --git a/tests/test_protect_enforcement_end_to_end.py b/tests/test_protect_enforcement_end_to_end.py index c714627..223c63e 100644 --- a/tests/test_protect_enforcement_end_to_end.py +++ b/tests/test_protect_enforcement_end_to_end.py @@ -311,3 +311,153 @@ async def charge_card(amount: int) -> str: "without it, 'body did not run' passes against an async " "wrapper that refuses everything" ) + + +class TestOnDeniedThroughRealLangChainTool: + """`on_denied="message"` must work INSIDE a real `@tool`. + + `TestOnDeniedReachesProtect` above proves the flag reaches + `@protect`. It does not prove it survives the framework layer an + agent actually calls through — and that layer is where the mode is + most likely to break: + + * LangChain's `ToolException` arm is its own error path, and + `handle_tool_error=True` stringifies whatever it catches; + * `@tool`/`@protect` ordering determines whether the wrapper is + even in the call chain; + * async and sync are separate wrappers. + + `NullRunDeniedError` is what an operator uses to hand an agent a + written "you may not do this" without ending the run. If the tool + layer turns it into a string or drops it, the agent either loops or + crashes, and both are worse than the refusal. + """ + + def test_denied_message_reaches_the_agent_through_a_sync_tool( + self, make_runtime, mock_api, ran + ): + pytest.importorskip("langchain_core") + from langchain_core.tools import tool + + respx.post(GATE_URL).mock( + return_value=_gate_refusal("TOOL_BLOCKED", "denied", status=403) + ) + make_runtime(on_denied="message") + + @protect + @tool + def charge(amount: int) -> str: + """Charge a card.""" + ran.append("body") + return f"charged:{amount}" + + with pytest.raises(NullRunDeniedError) as exc: + charge.invoke({"amount": 100}) + assert exc.value.model_safe_text() == "The operator has not allowed this tool." + assert ran == [] + + def test_denied_message_reaches_the_agent_through_the_other_order( + self, make_runtime, mock_api, ran + ): + """`@tool` inside `@protect` — both orders must behave the same.""" + pytest.importorskip("langchain_core") + from langchain_core.tools import tool + + respx.post(GATE_URL).mock( + return_value=_gate_refusal("TOOL_BLOCKED", "denied", status=403) + ) + make_runtime(on_denied="message") + + @tool + @protect + def charge(amount: int) -> str: + """Charge a card.""" + ran.append("body") + return f"charged:{amount}" + + with pytest.raises(NullRunDeniedError): + charge.invoke({"amount": 100}) + assert ran == [] + + @pytest.mark.asyncio + async def test_denied_message_reaches_the_agent_through_an_async_tool( + self, make_runtime, mock_api, ran + ): + pytest.importorskip("langchain_core") + from langchain_core.tools import tool + + respx.post(GATE_URL).mock( + return_value=_gate_refusal("TOOL_BLOCKED", "denied", status=403) + ) + make_runtime(on_denied="message") + + @protect + @tool + async def charge(amount: int) -> str: + """Charge a card.""" + ran.append("body") + return f"charged:{amount}" + + with pytest.raises(NullRunDeniedError) as exc: + await charge.ainvoke({"amount": 100}) + assert exc.value.model_safe_text() == "The operator has not allowed this tool." + assert ran == [] + + @pytest.mark.parametrize( + "code,category,status,expected", + [ + ("BUDGET_HARD_BLOCKED", "budget", 402, NullRunBudgetError), + ("WORKFLOW_PAUSED", "halt", 403, NullRunBlockedException), + ], + ) + def test_budget_and_halt_still_stop_the_tool_under_the_flag( + self, make_runtime, mock_api, ran, code, category, status, expected + ): + """The safety half, at the framework layer. + + `on_denied="message"` is an operator convenience for a POLICY + denial. Letting it also stringify a budget wall would tell the + agent "that tool is not allowed" when the truth is "you are out + of money" — and its obvious next move is to find another way to + spend. + """ + pytest.importorskip("langchain_core") + from langchain_core.tools import tool + + respx.post(GATE_URL).mock( + return_value=_gate_refusal(code, category, status=status) + ) + make_runtime(on_denied="message") + + @protect + @tool + def charge(amount: int) -> str: + """Charge a card.""" + ran.append("body") + return f"charged:{amount}" + + with pytest.raises(expected) as exc: + charge.invoke({"amount": 100}) + assert not isinstance(exc.value, NullRunDeniedError) + assert ran == [] + + def test_allow_still_runs_the_tool_body( + self, make_runtime, mock_api, ran + ): + """Counter-test: the tool layer is not simply refusing everything.""" + pytest.importorskip("langchain_core") + from langchain_core.tools import tool + + respx.post(GATE_URL).mock(return_value=_gate_allow()) + respx.post(EXECUTE_URL).mock(return_value=httpx.Response(200, json={})) + make_runtime(on_denied="message") + + @protect + @tool + def charge(amount: int) -> str: + """Charge a card.""" + ran.append("body") + return f"charged:{amount}" + + assert charge.invoke({"amount": 100}) == "charged:100" + assert ran == ["body"] diff --git a/tests/test_tool_ordering_real_langchain.py b/tests/test_tool_ordering_real_langchain.py new file mode 100644 index 0000000..dd5ac24 --- /dev/null +++ b/tests/test_tool_ordering_real_langchain.py @@ -0,0 +1,445 @@ +"""`@tool` / `@protect` ordering on the REAL `langchain_core.tools.tool`. + +Priority 1 for DEF-MP-TS12-ENF-01: the user-facing surface is one +`@protect`. This file pins the two properties that decide whether that +claim is true in practice for a LangChain agent, neither of which any +existing test asserted: + +1. `@tool` OUTSIDE `@protect` is the only ordering that produces a + `StructuredTool` LangChain's agent loop can bind. `@protect` outside + `@tool` collapses the tool to a plain function, which is invisible + to the loop — so the "correct" ordering is not a style preference, + it is the difference between enforcement and no enforcement. +2. A refusal raised inside the protected body escapes a real + `StructuredTool.invoke` intact, and cannot be converted into + model-visible text by `handle_tool_error=True`. + +Both were established by probe on 2026-10-01 against +`langchain-core` 0.3.86 before being written here; the probes found the +property, this file keeps it. + +`langchain_core` is an optional dependency, so every test is skipped +rather than failed when it is absent — the same rule +`tests/test_langchain_enforcement.py` already follows. +""" + +from __future__ import annotations + +import pytest + +pytest.importorskip("langchain_core") + +from langchain_core.tools import StructuredTool, tool # noqa: E402 + +from nullrun import decorators # noqa: E402 +from nullrun.breaker.exceptions import NullRunBudgetError # noqa: E402 + +REFUSAL = "NR-B002" + + +class _Deny: + """Every gate refuses. + + The point of these tests is which exception *escapes the tool*, so + the stub raises the real enforcement type rather than returning a + block dict — a returned decision takes a different branch inside + `_protect_body` and would not exercise the boundary under test. + """ + + def _bump_protect_count(self): + return None + + def check_control_plane(self, *a, **k): + return None + + def check_workflow_budget(self, *a, **k): + raise NullRunBudgetError( + "no budget", reason="budget exhausted", error_code=REFUSAL + ) + + def execute(self, *a, **k): + raise NullRunBudgetError( + "no budget", reason="budget exhausted", error_code=REFUSAL + ) + + def track_tool(self, *a, **k): + return None + + def _resolve_workflow_id(self, *a, **k): + return "w-test" + + def _emit_sdk_error(self, *a, **k): + return None + + def sensitive_fail_open_enabled(self, *a, **k): + return False + + +class _Allow(_Deny): + def check_workflow_budget(self, *a, **k): + return None + + def execute(self, *a, **k): + return {"decision": "allow", "decision_source": "gateway"} + + +@pytest.fixture +def deny(monkeypatch): + stub = _Deny() + monkeypatch.setattr(decorators, "_get_or_create_runtime", lambda: stub, raising=True) + return stub + + +@pytest.fixture +def allow(monkeypatch): + stub = _Allow() + monkeypatch.setattr(decorators, "_get_or_create_runtime", lambda: stub, raising=True) + return stub + + +class TestToolOutsideProtectIsBindable: + """The ordering a user is expected to write.""" + + def test_yields_a_structured_tool(self, allow): + ran = [] + + @tool + @decorators.protect + def a_search(query: str) -> str: + """Search.""" + ran.append("A") + return "body-A" + + assert isinstance(a_search, StructuredTool), ( + "an agent loop can only bind a StructuredTool; if @protect is " + "the OUTER decorator the tool disappears from the loop entirely" + ) + assert a_search.name == "a_search" + assert a_search.description == "Search." + + def test_invokes_the_protected_body(self, allow): + ran = [] + + @tool + @decorators.protect + def a_search(query: str) -> str: + """Search.""" + ran.append("A") + return "body-A" + + assert a_search.invoke({"query": "x"}) == "body-A" + assert ran == ["A"] + + +class TestProtectOutsideToolStillEnforces: + """`@protect` OUTSIDE `@tool` — previously a silent enforcement loss. + + Before 2026-10-01 this collapsed the tool to a plain function: + `functools.wraps`-based wrapping cannot preserve `.invoke`, so the + agent loop could not bind the tool and the refusal never fired. The + tool now has its `func`/`coroutine` wrapped in place and the same + object is returned, so ordering does not decide whether the agent is + gated. + + These are the counter-tests for the class above: together they say + "both orders enforce" rather than "one order is documented". + """ + + def test_stays_a_structured_tool(self, allow): + @decorators.protect + @tool + def b_search(query: str) -> str: + """Search.""" + return "body-B" + + assert isinstance(b_search, StructuredTool), ( + "@protect must return the tool unchanged; wrapping the object " + "itself silently removes it from the agent loop" + ) + assert b_search.name == "b_search" + assert b_search.description == "Search." + + def test_preserves_the_metadata_an_agent_needs(self, allow): + @decorators.protect + @tool + def b_search(query: str) -> str: + """Search.""" + return "body-B" + + assert b_search.args_schema is not None + assert "query" in b_search.args_schema.model_fields + + def test_refusal_propagates_through_this_order_too(self, deny): + ran = [] + + @decorators.protect + @tool + def b_search(query: str) -> str: + """Search.""" + ran.append("B") + return "body-B" + + with pytest.raises(NullRunBudgetError) as exc: + b_search.invoke({"query": "x"}) + assert exc.value.error_code == REFUSAL + assert ran == [], "the body ran despite a refusal" + + def test_allow_path_still_runs(self, allow): + @decorators.protect + @tool + def b_search(query: str) -> str: + """Search.""" + return "body-B" + + assert b_search.invoke({"query": "x"}) == "body-B" + + def test_async_tool_in_this_order_also_enforces(self, deny): + import asyncio + + ran = [] + + @decorators.protect + @tool + async def b_search(query: str) -> str: + """Search.""" + ran.append("B") + return "body-B" + + assert isinstance(b_search, StructuredTool) + with pytest.raises(NullRunBudgetError): + asyncio.run(b_search.ainvoke({"query": "x"})) + assert ran == [] + + def test_converts_to_the_wire_schema_an_agent_sends(self, allow): + """What an agent loop actually does with a bound tool. + + `bind_tools` is `BaseChatModel`-specific and the fake chat model + in `langchain-core` does not implement it, so this asserts the + step underneath: `convert_to_openai_tool` is what every + `bind_tools` implementation calls to build the request body, and + it is what fails first when an object has lost `.name` / + `.args_schema` / `.description`. + """ + from langchain_core.utils.function_calling import convert_to_openai_tool + + @decorators.protect + @tool + def b_search(query: str) -> str: + """Search.""" + return "body-B" + + # Assert the type BEFORE converting. Without the in-place wrap the + # object is a plain function, and `convert_to_openai_tool` fails on + # it with an opaque `NameError: name 'Annotated' is not defined` + # from resolving annotations copied by functools.wrwraps -- a real + # failure, but one that reads as a langchain bug rather than as + # "this is no longer a tool". + assert isinstance(b_search, StructuredTool), ( + "@protect returned a plain function; the tool is gone from " + "the agent loop and enforcement is silently disabled" + ) + + schema = convert_to_openai_tool(b_search) + assert schema["type"] == "function" + assert schema["function"]["name"] == "b_search" + assert schema["function"]["description"] == "Search." + assert "query" in schema["function"]["parameters"]["properties"] + + +class TestRefusalEscapesTheTool: + def test_refusal_propagates_out_of_invoke(self, deny): + ran = [] + + @tool + @decorators.protect + def a_search(query: str) -> str: + """Search.""" + ran.append("A") + return "body-A" + + with pytest.raises(NullRunBudgetError) as exc: + a_search.invoke({"query": "x"}) + assert exc.value.error_code == REFUSAL + assert ran == [], "the body ran despite a refusal" + + def test_handle_tool_error_cannot_turn_a_refusal_into_model_text(self, deny): + """`handle_tool_error` is LangChain's own "don't crash the loop" flag. + + It only catches `ToolException`. An enforcement refusal is not + one, so it reaches the bare `except (Exception, KeyboardInterrupt)` + arm and is re-raised unconditionally. This matters because the + alternative — a refusal string in the tool message — is exactly + the DEF-MP-TS12-ENF-01 shape (the agent reads "allowed") in a + different costume. + """ + ran = [] + + def raw(query: str) -> str: + ran.append("H") + return "body-H" + + h_search = StructuredTool.from_function( + func=decorators.protect(raw), + name="h_search", + description="Search.", + handle_tool_error=True, + ) + + with pytest.raises(NullRunBudgetError): + h_search.invoke({"query": "x"}) + assert ran == [] + + def test_async_refusal_also_escapes_a_structured_tool(self, deny): + """The async arm is a SEPARATE code path and a separate risk. + + `decorators.protect` builds `async_wrapper` for a coroutine and + `sync_wrapper` for a plain function; they share nothing but the + name. The sync arm catches `BaseException` and the async arm + catches `Exception` (deliberately — its own comment requires + CancelledError/KeyboardInterrupt to propagate without blocking + I/O), so "the sync path is safe" says nothing about this one. + + Mutation-verified 2026-10-01: converting the async arm's + refusal into a returned string leaves the sync tests green and + turns exactly these two red. + """ + import asyncio + + ran = [] + + @tool + @decorators.protect + async def a_search(query: str) -> str: + """Search.""" + ran.append("A") + return "body-A" + + with pytest.raises(NullRunBudgetError) as exc: + asyncio.run(a_search.ainvoke({"query": "x"})) + assert exc.value.error_code == REFUSAL + assert ran == [], "the async body ran despite a refusal" + + def test_async_allow_path_runs_the_body(self, allow): + """Counter-test: the async arm is not simply broken. + + Without this, a mutation that made async raise unconditionally + would satisfy the refusal test above. + """ + import asyncio + + ran = [] + + @tool + @decorators.protect + async def a_search(query: str) -> str: + """Search.""" + ran.append("A") + return "body-A" + + assert asyncio.run(a_search.ainvoke({"query": "x"})) == "body-A" + assert ran == ["A"] + + def test_control_tool_without_protect_still_runs(self, deny): + """Proves the refusal above came from `@protect`, not from LangChain.""" + ran = [] + + @tool + def c_search(query: str) -> str: + """Search.""" + ran.append("C") + return "body-C" + + assert c_search.invoke({"query": "x"}) == "body-C" + assert ran == ["C"] + + +class TestProtectNeedsNoInitCall: + """Lazy resolution: `@protect` with no `init()` must fail LOUD, not open. + + The `reset_runtime` autouse fixture in `conftest.py` clears + `decorators._runtime` before every test, so this is simply the state + every test above runs in — except they all install a stub, which + hides what the resolver does when nothing is installed. + + The real resolver (`decorators._get_or_create_runtime`, `:285`) is + left untouched here: it falls through to + `NullRunRuntime.get_instance()`, which reads `NULLRUN_API_KEY` and + raises when there is none. FIX-4 removed the old + `except`-and-rebuild fallback precisely so this surfaces at the + first `@protect` call. Stubbing the resolver to return `None` would + prove nothing — it produced an `AttributeError` on `None`, not the + documented fail-loud path. + """ + + def test_unpinned_protect_raises_auth_rather_than_running_the_body( + self, monkeypatch + ): + import nullrun.decorators as d + from nullrun.breaker.exceptions import NullRunAuthenticationError + + monkeypatch.delenv("NULLRUN_API_KEY", raising=False) + monkeypatch.delenv("NULLRUN_SECRET_KEY", raising=False) + monkeypatch.setattr(d, "_runtime", None, raising=True) + ran = [] + + @d.protect + def body(): + ran.append("ran") + return "ok" + + with pytest.raises(NullRunAuthenticationError) as exc: + body() + assert exc.value.error_code == "NR-A001" + assert ran == [], "an unenforceable call must not run the body" + + def test_async_protect_also_refuses_without_a_runtime(self, monkeypatch): + import asyncio + + import nullrun.decorators as d + from nullrun.breaker.exceptions import NullRunAuthenticationError + + monkeypatch.delenv("NULLRUN_API_KEY", raising=False) + monkeypatch.delenv("NULLRUN_SECRET_KEY", raising=False) + monkeypatch.setattr(d, "_runtime", None, raising=True) + ran = [] + + @d.protect + async def body(): + ran.append("ran") + return "ok" + + with pytest.raises(NullRunAuthenticationError): + asyncio.run(body()) + assert ran == [] + + def test_the_resolver_is_not_swallowing_the_auth_error(self, monkeypatch): + """FIX-4's invariant: no `except` between resolver and caller. + + A regression guard on the shape, because the failure it prevents + is a silent one — the fallback this removed logged "we have a + runtime" and then crashed later, somewhere unrelated. + """ + import ast + import inspect + import textwrap + + import nullrun.decorators as d + + src = textwrap.dedent(inspect.getsource(d._get_or_create_runtime)) + tree = ast.parse(src) + fn = tree.body[0] + assert isinstance(fn, ast.FunctionDef) + + # AST, not substring: the docstring *describes* the removed + # `try/except`, so any text search finds prose about the very + # construct this asserts is gone. A `handlers` list is empty iff + # there is no `except` clause, regardless of formatting. + handlers = [ + n + for n in ast.walk(fn) + if isinstance(n, ast.Try) and n.handlers + ] + assert handlers == [], ( + "_get_or_create_runtime must not catch the auth error: FIX-4 " + "removed this fallback and it must stay removed" + ) From f8bfbdc43c259ae5ae86419f51a7e63fbe13bced Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Thu, 1 Oct 2026 16:30:42 +0400 Subject: [PATCH 20/20] =?UTF-8?q?release(sdk):=200.20.0=20=E2=80=94=20migr?= =?UTF-8?q?ation=20note=20first,=20ordering=20documented?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Minor, not patch: an unclassifiable refusal now RAISES where 0.19.0 let the call proceed. That turns a working loop into a throwing one, which is a behavioural break, so the release note leads with the migration rather than burying it under the security items. `### Migration` is the first section under 0.20.0 and enumerates all six behavioural differences from 0.19.0, then repeats the three inherited from 0.19.0 that are the same class of break and the same shape of fix (`mode="inline"` removed, `decision_source` now required in a `/gate` test double, `handle` -> `guard`). A reader upgrading from 0.18.x sees the whole break list in one place instead of reconstructing it across two releases. The load-bearing item is #3: `NullRunUnclassifiedRefusalError` is a sibling of `NullRunTransportError`, not a subclass, so an existing `except NullRunTransportError:` arm will not catch it. Catching it alongside restores 0.19.0's behaviour exactly -- which restores the bypass. The note says so and says not to widen to `NullRunError`, which is a larger hole than the one this closes. Also records the `@tool`/`@protect` fix under `### Fixed`, since it is a silent enforcement loss users may have shipped against: enforcement was absent whenever `@protect` was the outer decorator on a LangChain tool. README gains the two things a user would otherwise get wrong: - decorator order, both directions, with the reason (an unbound tool cannot refuse) and the `NameError` that was the real symptom; - `on_denied="message"` as the operator-facing "explain, don't crash" mode -- `category="denied"` only, budget and halt deliberately excluded -- and why LangChain's `handle_tool_error=True` is not an equivalent (it catches `ToolException` and does not know which exceptions are refusals). Every claim in the new README section was executed before being written down, including the pre-fix `NameError`, which is reproduced rather than asserted from memory. Not published to PyPI. Suite: 1597 passed, 1 skipped; ruff and mypy clean. --- CHANGELOG.md | 173 +++++++++++++++++++++++-------------- README.md | 46 ++++++++++ pyproject.toml | 2 +- src/nullrun/__version__.py | 2 +- uv.lock | 2 +- 5 files changed, 159 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05041f3..c595b01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,13 +1,82 @@ -## [Unreleased] +## [0.20.0] - 2026-10-01 The remaining half of `DEF-MP-TS12-ENF-01` (QA cycle RUN_ID 20260929T1338). 0.19.0 closed the paths it found by reading; these close the ones that only showed up when the properties were asserted -end-to-end. **Not yet released, and the version number is not chosen** — -the behaviour change below is the reason that is worth a decision -rather than a default: an unclassifiable refusal now raises where -0.19.0 let the call proceed, so anyone who was relying on that has a -migration to make and should be told so in the release note. +end-to-end. + +Minor, not patch: an unclassifiable refusal now **raises** where 0.19.0 +let the call proceed. That is a working loop becoming a throwing one, +which is a behavioural break and not a bug fix — read +[Migration](#migration) before upgrading. + +### Migration + +Six things differ from 0.19.0. Only the first three can surprise you +at runtime, and the third is the one worth reading twice. + +1. **A `/gate` test double must carry `decision_source`.** Real + backend answers always do — it is a non-`Option` `String` on + `GateResponse`. A hand-written fixture that omits it now raises + `NullRunMalformedGateResponseError`. Fix: add + `"decision_source": "gateway"` next to `"decision": "allow"`. +2. **`NULLRUN_SENSITIVE_FAIL_OPEN` against production now needs a + second variable.** `NULLRUN_ALLOW_SENSITIVE_FAIL_OPEN=1` must be + set too, otherwise the opt-out is refused, enforcement falls back + to its own fail-CLOSED default, and the attempt is logged at ERROR + with a metric. Non-production behaviour is unchanged. +3. **An unclassifiable refusal now raises where 0.19.0 let the call + proceed.** This is the intended fix, and it is the one that can + turn a working loop into a throwing one. + `NullRunUnclassifiedRefusalError` is importable from + `nullrun.breaker.categories` (it is *not* re-exported at the top + level) and carries `error_code="NR-P003"` and `retryable=True`. + It is a **`NullRunInfrastructureError` and a sibling of + `NullRunTransportError`** — deliberately *not* a subclass of it. + So an existing `except NullRunTransportError:` arm, which in + 0.19.0 caught everything the gate could not classify and failed + open, **will not catch this one**. That is the point: it is what + stops a failed-open arm from swallowing a refusal. If you have + such an arm, you have three options and they are not equivalent: + + ```python + from nullrun.breaker.categories import NullRunUnclassifiedRefusalError + from nullrun.breaker.exceptions import NullRunError, NullRunTransportError + + try: + ... + except NullRunUnclassifiedRefusalError: + raise # recommended: the SDK was right + except NullRunTransportError: + ... # still fails open, as before + ``` + + Catching it alongside `NullRunTransportError` restores 0.19.0's + behaviour exactly — which means it restores the bypass. Do not + widen the arm to `NullRunError`: that also swallows real policy + refusals, which is a larger hole than the one this closes. +4. **`on_denied` is new.** Default `"raise"`, which is 0.19.0's + behaviour. Set `"message"` to get a `NullRunDeniedError` carrying + the server-authored `agent_message` for `category="denied"` only. + Any other value raises `ValueError` at construction rather than + at first refusal. +5. **`NULLRUN_SKIP_BUDGET_CHECK` outside production now logs and + increments a metric** when it skips the check. Enforcement + behaviour is unchanged. +6. **`/execute` refusal bodies carry `category` and + `agent_message`.** Additive on the wire. If you parse that body + yourself and reject unknown keys, relax that. + +Carried over from 0.19.0 and still true on 0.20.0, because it is the +same class of break and the same shape of fix: + +- **`NullRunRuntime.execute(..., mode="inline")` is gone** and has no + replacement — every call goes through `/execute`. Drop the argument; + `mode="auto"` (the default) already always contacts the gateway. +- **`nullrun.runtime.register_strict_mode_forced` / + `is_strict_mode_forced`** are gone, along with `@guarded`, + `nullrun.handle` (renamed `nullrun.guard` in 0.18.5), + `nullrun.status()`, and `nullrun.auto_instrument`. ### Security @@ -42,6 +111,36 @@ migration to make and should be told so in the release note. increments a metric instead of being indistinguishable from a normal call. +### Fixed + +- **`@protect` no longer loses a LangChain tool when it is the outer + decorator.** Applied to a `@tool` result, `@protect` wrapped the + object with `functools.wraps` and returned a plain function — so + `.invoke`, `.name`, `.args_schema` and `.description` were gone, an + agent loop could not bind the tool, and a tool the loop cannot see + raises no refusal. **Enforcement was silently absent in that + ordering.** The reported symptom was not even about NullRun: + `convert_to_openai_tool` on the result raises + `NameError: name 'Annotated' is not defined`, which reads as a + LangChain type bug rather than "your tool is not a tool any more". + + ```python + @nullrun.protect # now fine + @tool + def charge(amount: int) -> str: ... + + @tool # was already fine + @nullrun.protect + def charge(amount: int) -> str: ... + ``` + + `protect` wraps the tool's `func`/`coroutine` in place and returns the + same object, so both orderings gate identically. Duck-typed on + `.invoke` + `.name` rather than `isinstance(BaseTool)`, because + `langchain_core` is an optional dependency. `handle_tool_error=True` + remains safe: it catches only `ToolException`, so an enforcement + refusal still aborts rather than becoming model-visible text. + ### Changed - **Every gate refusal is classified, and an unclassifiable one @@ -69,65 +168,13 @@ migration to make and should be told so in the release note. proven on the real adapter rather than inferred from `runtime.execute`'s lack of a fail-OPEN `except`. -### Migration - -Six things differ from 0.19.0. Only the first three can surprise you -at runtime, and the third is the one worth reading twice. - -1. **A `/gate` test double must carry `decision_source`.** Real - backend answers always do — it is a non-`Option` `String` on - `GateResponse`. A hand-written fixture that omits it now raises - `NullRunMalformedGateResponseError`. Fix: add - `"decision_source": "gateway"` next to `"decision": "allow"`. -2. **`NULLRUN_SENSITIVE_FAIL_OPEN` against production now needs a - second variable.** `NULLRUN_ALLOW_SENSITIVE_FAIL_OPEN=1` must be - set too, otherwise the opt-out is refused, enforcement falls back - to its own fail-CLOSED default, and the attempt is logged at ERROR - with a metric. Non-production behaviour is unchanged. -3. **An unclassifiable refusal now raises where 0.19.0 let the call - proceed.** This is the intended fix, and it is the one that can - turn a working loop into a throwing one. - `NullRunUnclassifiedRefusalError` is importable from - `nullrun.breaker.categories` (it is *not* re-exported at the top - level) and carries `error_code="NR-P003"` and `retryable=True`. - It is a **`NullRunInfrastructureError` and a sibling of - `NullRunTransportError`** — deliberately *not* a subclass of it. - So an existing `except NullRunTransportError:` arm, which in - 0.19.0 caught everything the gate could not classify and failed - open, **will not catch this one**. That is the point: it is what - stops a failed-open arm from swallowing a refusal. If you have - such an arm, you have three options and they are not equivalent: - - ```python - from nullrun.breaker.categories import NullRunUnclassifiedRefusalError - from nullrun.breaker.exceptions import NullRunError, NullRunTransportError - - try: - ... - except NullRunUnclassifiedRefusalError: - raise # recommended: the SDK was right - except NullRunTransportError: - ... # still fails open, as before - ``` - - Catching it alongside `NullRunTransportError` restores 0.19.0's - behaviour exactly — which means it restores the bypass. Do not - widen the arm to `NullRunError`: that also swallows real policy - refusals, which is a larger hole than the one this closes. -4. **`on_denied` is new.** Default `"raise"`, which is 0.19.0's - behaviour. Set `"message"` to get a `NullRunDeniedError` carrying - the server-authored `agent_message` for `category="denied"` only. - Any other value raises `ValueError` at construction rather than - at first refusal. -5. **`NULLRUN_SKIP_BUDGET_CHECK` outside production now logs and - increments a metric** when it skips the check. Enforcement - behaviour is unchanged. -6. **`/execute` refusal bodies carry `category` and - `agent_message`.** Additive on the wire. If you parse that body - yourself and reject unknown keys, relax that. - ### Documentation +- README states the decorator ordering for LangChain tools and shows it + in both directions, with the reason (an unbound tool cannot refuse). + `on_denied="message"` is documented as the operator-facing "explain, + don't crash" mode, including that `handle_tool_error=True` does not + provide the same thing and would not be safe if it did. - README "Known limitations" states the trust boundary precisely: the SDK trusts the channel and says so. Certificate verification cannot be switched off by configuration, plain `http` is refused, diff --git a/README.md b/README.md index 612926b..31b1062 100644 --- a/README.md +++ b/README.md @@ -221,6 +221,52 @@ exits with code 1 instead of raising. `nullrun.shutdown()` is auto-registered via `atexit` inside `init()`, so a clean WS close on process exit happens without any explicit call. +### Decorator order with LangChain tools + +Both orders gate. `@protect` recognises a LangChain tool, wraps the +tool's `func`/`coroutine` in place, and returns the same object, so this +is not a rule you have to remember: + +```python +from langchain_core.tools import tool +from nullrun import protect + +@tool # fine +@protect +def charge(amount: int) -> str: ... + +@protect # also fine +@tool +def charge(amount: int) -> str: ... +``` + +Before 0.20.0 the second form silently produced a plain function. The +agent loop could not bind it, and a tool the loop cannot bind cannot +refuse — so the gate was not running. If you saw +`NameError: name 'Annotated' is not defined` from +`convert_to_openai_tool`, that was this. + +### Handing an agent a reason instead of a crash + +By default a refusal raises, which is right for most code: the caller +decides what happens next. `on_denied="message"` is the operator-facing +alternative for a **policy** denial — the agent gets the +server-authored explanation and the run continues: + +```python +rt = nullrun.init(on_denied="message") +``` + +The text is authored by the backend, never assembled by the SDK, and it +applies to `category="denied"` only. Budget and halt refusals keep their +own exceptions under the same flag: an agent told "that tool is not +allowed" when the truth is "you are out of money" will go looking for +another way to spend. + +LangChain's own `handle_tool_error=True` is **not** an equivalent. It +catches `ToolException` and stringifies it, and it does not know which +exceptions are refusals — use `on_denied="message"`. + --- ## How NullRun compares diff --git a/pyproject.toml b/pyproject.toml index d3498bd..618afa0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "hatchling.build" name = "nullrun" # Full release history lives in CHANGELOG.md; only the current version # is pinned here. -version = "0.19.0" +version = "0.20.0" # Kept under the 200-char preview threshold so the full line is visible # without an "expand" click. The headline is the canonical §1 statement # from positioning.md — "runtime decision layer for tool-using AI agents" diff --git a/src/nullrun/__version__.py b/src/nullrun/__version__.py index 715e16e..e2aa469 100644 --- a/src/nullrun/__version__.py +++ b/src/nullrun/__version__.py @@ -5,5 +5,5 @@ string and the SDK_MIN_VERSION constant. """ -__version__ = "0.19.0" +__version__ = "0.20.0" __platform_version__ = "1.0.0" diff --git a/uv.lock b/uv.lock index c81ba7a..d8658c9 100644 --- a/uv.lock +++ b/uv.lock @@ -636,7 +636,7 @@ wheels = [ [[package]] name = "nullrun" -version = "0.19.0" +version = "0.20.0" source = { editable = "." } dependencies = [ { name = "httpx" },