diff --git a/CHANGELOG.md b/CHANGELOG.md index c63d987..c595b01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,199 @@ +## [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. + +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 + +- **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. + +### 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 + 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 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, + 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` diff --git a/README.md b/README.md index 47c4d42..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 @@ -354,6 +400,34 @@ require tests for new public API, and run `ruff` + `mypy` in CI. --- +## Known limitations + +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. + +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 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 + +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`). + +--- + ## 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`. 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/__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/__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/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/breaker/exceptions.py b/src/nullrun/breaker/exceptions.py index 392158a..31d97aa 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. @@ -767,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/decorators.py b/src/nullrun/decorators.py index b9fd555..65ec4fb 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 @@ -490,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. @@ -689,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) @@ -851,7 +898,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 6023a1d..c83d217 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -96,6 +96,10 @@ AuditQuery, AuditVerifyResult, ) +from nullrun.breaker.categories import ( + DecisionCategory, + NullRunUnclassifiedRefusalError, +) from nullrun.breaker.exceptions import ( NullRunApprovalDeniedError, NullRunApprovalExpiredError, @@ -105,6 +109,7 @@ NullRunBackendError, NullRunBlockedException, NullRunBudgetError, + NullRunDeniedError, NullRunError, NullRunInfrastructureError, NullRunTransportError, @@ -557,6 +562,11 @@ 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): """ @@ -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,25 @@ 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 @@ -639,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 @@ -656,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( @@ -1870,6 +1916,214 @@ 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"} + ) + + #: 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": + + * 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." + ) + + 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( + 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 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 @@ -1949,7 +2203,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 @@ -2133,6 +2417,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 @@ -2141,6 +2434,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 +2458,19 @@ 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 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") @@ -2158,7 +2478,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. @@ -2182,6 +2502,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" -- @@ -2349,6 +2691,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 # ============================================================================= @@ -3165,7 +3534,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 49e8ca3..1a881ab 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,69 @@ 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-064 owns this rule; §4.7 of ADR-063 records the + # correction and points here. + # + # 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. + # + # 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 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 + # 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 +1703,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, @@ -2616,6 +2684,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 @@ -2640,7 +2728,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 "") @@ -2694,6 +2812,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. @@ -2707,6 +2873,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/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_2026_09_10_check_failopen.py b/tests/test_2026_09_10_check_failopen.py index d17c8fb..9f59c52 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-064 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..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, ( @@ -195,16 +235,34 @@ 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 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, ( "DEF-NR-TOOLBLOCKED-PARSER: the explainer comment " "block must name the fix tag so future readers can " "grep for it." ) + 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 " + "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): """The dedicated branch must call 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..7ffcb42 --- /dev/null +++ b/tests/test_adr063_infra_refusal_is_not_fail_closed.py @@ -0,0 +1,257 @@ +"""ADR-063 §1.3(f): what the SDK does with an `infra` refusal. + +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 +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 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. + +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`). 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 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 + +import httpx +import pytest +import respx + +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" + + +def _infra_503() -> httpx.Response: + """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: 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", + "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, + }, + ) + + +def _breaker_trip_403() -> httpx.Response: + return httpx.Response( + 403, + json={ + "decision": "block", + "decision_source": "gateway", + "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, + }, + ) + + +class TestInfraRefusalIsNotFailClosed: + """A 503 the GATE answered is not an outage. + + 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 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. + """ + respx.post(GATE_URL).mock(return_value=_infra_503()) + rt = make_runtime() + + 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. + + 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 + 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 — and stops a + "skip the retry, block immediately" change from turning a blip + into a stopped agent. + """ + 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() + with pytest.raises(NullRunError): + rt.check_workflow_budget() + + assert len(calls) > 1, ( + 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): + """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" + ) 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_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_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 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_mcp_refusal_categories.py b/tests/test_mcp_refusal_categories.py new file mode 100644 index 0000000..d980944 --- /dev/null +++ b/tests/test_mcp_refusal_categories.py @@ -0,0 +1,390 @@ +"""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.runtime import NullRunRuntime +from nullrun.toolbox.mcp import MCPAdapter + +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: the backend 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-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` + 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 == [] 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..223c63e --- /dev/null +++ b/tests/test_protect_enforcement_end_to_end.py @@ -0,0 +1,463 @@ +"""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" + ) + + +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_refusal_categories.py b/tests/test_refusal_categories.py new file mode 100644 index 0000000..3b7eed4 --- /dev/null +++ b/tests/test_refusal_categories.py @@ -0,0 +1,340 @@ +"""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. + +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 + +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, + NullRunDeniedError, + 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 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), +} + + +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 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. + + `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. + `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(on_denied="message") + assert rt.check_workflow_budget() is None diff --git a/tests/test_sensitive_fail_open_guard.py b/tests/test_sensitive_fail_open_guard.py new file mode 100644 index 0000000..f3d0877 --- /dev/null +++ b/tests/test_sensitive_fail_open_guard.py @@ -0,0 +1,161 @@ +"""``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" +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" + + +@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) + + +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, monkeypatch): + monkey_api = make_runtime(api_url=PROD_URL) + 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, monkeypatch): + rt = make_runtime(api_url=PROD_URL) + 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, monkeypatch): + """The ack is a second signature, not a substitute.""" + rt = make_runtime(api_url=PROD_URL) + 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, monkeypatch): + """`NULLRUN_ENV=production` marks prod even on a custom host.""" + rt = make_runtime(api_url=CUSTOM_URL) + 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, monkeypatch): + """The documented dev / test use keeps working.""" + rt = make_runtime(api_url=BASE_URL) + monkeypatch.setenv(_FLAG, "1") + assert rt.sensitive_fail_open_enabled() is True + + 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, monkeypatch): + """Whitespace is not a yes. The `.strip()` matters.""" + rt = make_runtime(api_url=BASE_URL) + monkeypatch.setenv(_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, monkeypatch): + """With /execute unreachable, the body must NOT run in prod + even though the operator set the flag.""" + from nullrun.decorators import protect + + # 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( + 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, monkeypatch): + """The documented bypass still works off production.""" + from nullrun.breaker.exceptions import NullRunBlockedException + from nullrun.decorators import protect + + # 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( + 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" + ) 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() 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" + ) 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, 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" },