diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a3f24b..c63d987 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,81 @@ +## [0.19.0] - 2026-09-30 + +Closes the SDK-side bypasses found auditing `DEF-MP-TS12-ENF-01` +(QA cycle RUN_ID 20260929T1338): the gate answering the agent +"allowed" on calls it had actually blocked or never checked. + +### Surface (breaking) + +- `NullRunRuntime.execute(..., mode="inline")` removed. It + returned a synthesised local `allow` **without contacting the + gateway**, so budget, rate limit and tool-block policies were + all skipped — the SDK's own `explanation` string said as much. + The only guard was a sensitivity check, which meant the safety + of a tool call depended on whether someone had remembered to + mark it sensitive. It now raises `NullRunConfigError` + (`error_code="NR-S001"`) at the top of `execute`, before any + context resolution. **There is no replacement**: every call + goes through `/execute`. If you were using `mode="inline"` to + avoid a round-trip, drop the argument — `mode="auto"` (the + default) already always contacts the gateway. +- `nullrun.runtime.register_strict_mode_forced`, + `nullrun.runtime.is_strict_mode_forced` and the module-level + `_STRICT_MODE_FORCED` set removed. They existed only to force + strict mode past the inline fast path. `register_strict_mode_forced` + already had zero callers (the `@sensitive` decorator its own + docstring referenced no longer exists in the SDK); dead security + machinery reads as a live mechanism and invites a bypass being + wired back up. +- `MCPAdapter(runtime=None)` no longer means "do not gate". + `call_tool` was conditional on `self._runtime is not None`, and + `runtime` defaulted to `None` — so a default-constructed adapter + (which is what the module's own documented example builds) called + the MCP server with no `/execute` round-trip at all. The operator + got a contextvar that a *later* `@protect` wrapper might read on + its *next* `/check`: post-hoc annotation, not enforcement. The + umbrella `mcp_destructive_policy` / `mcp_readonly_policy` + therefore applied to a locally-declared function but not to a + remote MCP call, on the same agent, in the same loop. + `runtime=None` now means "resolve the global runtime", on the + same terms `@protect` resolves it. Resolution is lazy — at + `call_tool`, not at construction — so the adapter stays + constructible in fixtures and doc snippets without + `nullrun.init()`. A missing API key raises rather than degrading + to an ungated call, matching `@protect`'s fail-loud invariant. + Passing `runtime=` explicitly still works and still wins. + +`mode` itself is unchanged on the wire — it is still sent, and the +backend still ignores it (`transport.py`: "Wire-present but unused +by backend"). Its only real function was deciding whether to skip +enforcement. + +The per-runtime sensitivity registry (`add_sensitive_tool`, +`register_sensitive_tools`, `remove_sensitive_tool`, +`is_sensitive_tool`, `get_sensitive_tools`) is **unchanged** — it +is a separate documented surface, and no longer has a consumer +inside `execute`. See ADR-061. + +### Fixed + +- A real gate decision is no longer treated as a transport + fail-open. The LangChain/LangGraph callback swallowed + `NullRunBlockedException` alongside transport errors; enforcement + exceptions are now recorded and re-raised through a + thread-local deferred handoff to the `@protect` boundary, which + is the only place the framework lets an exception abort. +- One definition of a synthetic decision. `decorators.py` and + `runtime.py` each had their own test for "is this source + synthetic", and the two disagreed on `AUTH_ERROR`; both now call + `is_fallback_decision_source`. +- A skipped pre-flight gate is countable. `check_control_plane` + and `check_workflow_budget` still no-op when no workflow can be + resolved (correct — a never-bound key has no control-plane state + and no per-workflow budget), but the no-op now increments + `control_plane_no_workflow_total` / + `budget_preflight_no_workflow_total` at DEBUG instead of being + indistinguishable from normal operation. Behaviour is unchanged: + it still never raises. + ## [0.18.5] - 2026-09-26 Consolidated release that bundles three rounds of work landed on diff --git a/pyproject.toml b/pyproject.toml index 6869659..d3498bd 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.18.5" +version = "0.19.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" @@ -103,15 +103,30 @@ dev = [ # their data. Wrapping ``pytest -n auto`` in ``coverage run`` only # traces the coordinator process and produces a false 0% report. "pytest-cov>=5.0", - # The SDK eagerly imports `nullrun.instrumentation.langgraph` - # (from `nullrun.decorators`, imported by `nullrun.__init__` at - # collection time), which itself does `from langchain_core.callbacks - # import BaseCallbackHandler`. Without this dep, *every* test in - # the suite errors at pytest collection, not at a specific test. - # CI installs `[dev]` only, so the test extras need to cover the - # import chain. `langchain-core` is the smallest dep that makes - # the import succeed; the `langgraph` and `langchain` extras pull - # in heavier stacks that the unit tests don't need. + # `langchain-core` is a dev dep so CI exercises the REAL LangChain + # path, not the SDK's `object` fallback. + # + # It was originally here for a different reason, and that reason + # stopped being true: the SDK used to import + # `nullrun.instrumentation.langgraph` eagerly (reached from + # `nullrun.__init__` at collection time), which did an unguarded + # `from langchain_core.callbacks import BaseCallbackHandler`. With + # no such dep, *every* test in the suite errored at pytest + # collection. That import is now guarded — DEF-MP-TS12-SDK-05 + # (RUN_ID 20260929T1338) made it degrade to an `object` base + # instead, and collection without langchain-core now succeeds + # (verified: 1449 tests collected, exit 0, with `langchain_core`, + # `langchain` and `langgraph` all blocked at import). + # + # The dep is still required, because the guarded fallback is a + # DEGRADED mode: with langchain-core absent, the tests that pin + # `BaseCallbackHandler is the real BaseCallbackHandler` + # (`test_langgraph_optional`) and that pin LangChain's + # swallow-exceptions contract (`test_langchain_enforcement`) would + # SKIP rather than run, and the real integration would go untested. + # `langchain-core` is the smallest dep that covers both; the + # `langgraph` and `langchain` extras pull in heavier stacks the + # unit tests don't need. "langchain-core>=0.3,<1.0", # `tests/test_integrations_fastapi.py` does `from fastapi import ...` # at module top-level, so pytest collection aborts the entire suite diff --git a/src/nullrun/__version__.py b/src/nullrun/__version__.py index d5a20cf..715e16e 100644 --- a/src/nullrun/__version__.py +++ b/src/nullrun/__version__.py @@ -5,5 +5,5 @@ string and the SDK_MIN_VERSION constant. """ -__version__ = "0.18.5" +__version__ = "0.19.0" __platform_version__ = "1.0.0" diff --git a/src/nullrun/decorators.py b/src/nullrun/decorators.py index 0cffee6..b9fd555 100644 --- a/src/nullrun/decorators.py +++ b/src/nullrun/decorators.py @@ -62,6 +62,7 @@ def researcher(q): set_trace_id, ) from nullrun.runtime import NullRunRuntime, get_runtime +from nullrun.transport import is_fallback_decision_source # Sentinel used when a gate fires outside a workflow context. UNKNOWN_WORKFLOW_ID = "__nullrun_unknown__" @@ -634,6 +635,16 @@ def _protect_body(args: tuple[Any, ...], kwargs: dict[str, Any], unify_block: bo # backend decides allow/block/require-approval. _run_tool_policy_gate(runtime, fn, args, kwargs) + # 5. Drain any enforcement decision that a framework + # callback could not enforce. LangChain swallows + # exceptions raised from a callback handler, so a real + # block discovered in `on_llm_start` (budget exhausted, + # workflow KILL/PAUSE) cannot abort the LLM call from + # inside the callback. The callback stashes it instead; + # this boundary — which CAN abort — raises it. Runs after + # the gates so the primary decision always wins. + _raise_deferred_enforcement(fn.__name__) + yield runtime except BaseException as exc: # noqa: BLE001 error = exc @@ -737,6 +748,48 @@ def sync_wrapper(*args: Any, **kwargs: Any) -> Any: return sync_wrapper # type: ignore[return-value] +def _raise_deferred_enforcement(tool_name: str) -> None: + """Raise a gate decision a framework callback could not enforce. + + LangChain discards exceptions raised from a callback handler — it + logs ``Error in callback`` and continues. So when + ``NullRunCallback.on_llm_start`` calls ``check_workflow_budget`` + and gets a real block (budget exhausted, workflow KILL/PAUSE), it + cannot abort the LLM call from inside the callback. + + The callback stashes the decision instead; this runs at the + ``@protect`` boundary, which can abort, and re-raises the oldest + unraised one. + + The import is deliberately LAZY and failure-tolerant. The stashing + side lives in ``instrumentation.langgraph``, which is only imported + when LangChain instrumentation is in play; importing it here would + couple the core decorator path to the LangChain adapter. A missing + module simply means nothing was stashed. + + Never invoked on the transport-fail-OPEN path: only REAL gate + decisions are stashed (see ``record_deferred_enforcement``). + """ + try: + from nullrun.instrumentation.langgraph import ( + drain_deferred_enforcement, + ) + except Exception: # noqa: BLE001 — optional adapter, never a hard dep + return + deferred = drain_deferred_enforcement() + if deferred is None: + return + logger.error( + "@protect for %r: raising enforcement decision deferred from a " + "framework callback (%s: %s) — the callback itself could not " + "abort the call.", + tool_name, + type(deferred).__name__, + deferred, + ) + raise deferred + + def _run_tool_policy_gate( runtime: Any, fn: Callable[..., Any], @@ -908,16 +961,17 @@ def _run_tool_policy_gate( # typed transport-error arms above are the canonical path. if isinstance(result, dict): decision_source = result.get("decision_source", "") - if isinstance(decision_source, str) and ( - decision_source.startswith("FALLBACK_") - or decision_source - in { - TransportErrorSource.NETWORK_ERROR, - TransportErrorSource.GATEWAY_ERROR, - TransportErrorSource.BREAKER_OPEN, - TransportErrorSource.AUTH_ERROR, - } - ): + # DEF-MP-TS12-ENF-01: this arm used to test + # `startswith("FALLBACK_")` — an UPPERCASE prefix that no code + # path in the transport produces, since the real value is + # `DecisionSource.FALLBACK == "fallback"`. The clause could + # therefore never fire, and it still carried + # `TransportErrorSource.AUTH_ERROR`, which the same predicate + # in `runtime.check_workflow_budget` had already had removed + # (Fix D). One shared definition now, in + # `transport.is_fallback_decision_source`, so the two copies + # cannot drift apart again. + if is_fallback_decision_source(decision_source): if fail_open: logger.warning( f"tool policy gate for {fn.__name__!r} returned " diff --git a/src/nullrun/instrumentation/langgraph.py b/src/nullrun/instrumentation/langgraph.py index cbeca53..d24d3cd 100644 --- a/src/nullrun/instrumentation/langgraph.py +++ b/src/nullrun/instrumentation/langgraph.py @@ -29,8 +29,6 @@ import threading from typing import Any -from langchain_core.callbacks import BaseCallbackHandler - from nullrun.runtime import get_runtime from nullrun.tracing import ( SpanContext, @@ -39,6 +37,132 @@ get_current_span, ) +# Exceptions that represent a REAL enforcement decision from the gate, +# as opposed to a failure to reach the gate. `NullRunBudgetError` is +# the budget-exhausted block, `WorkflowKilledInterrupt` / +# `WorkflowPausedException` are the dashboard KILL/PAUSE switches. All +# three derive from `Exception`, so an ordering-sensitive `except` +# clause is required: this tuple must be caught BEFORE the broad +# transport arm, or the block is silently downgraded to a fail-OPEN +# transport error. +# +# ADR-008's fail-OPEN policy covers "the policy engine could not be +# reached". It explicitly does NOT cover "the policy engine returned +# `block`" — conflating the two is what let a real budget block +# through as a debug log line. +try: # pragma: no cover - import shape, not behaviour + from nullrun.breaker.exceptions import ( + NullRunBudgetError, + WorkflowKilledInterrupt, + WorkflowPausedException, + ) + + _ENFORCEMENT_EXCEPTIONS: tuple[type[BaseException], ...] = ( + NullRunBudgetError, + WorkflowKilledInterrupt, + WorkflowPausedException, + ) +except ImportError: # pragma: no cover + # The SDK is unusable without its own exception types, but never + # let their absence break `import nullrun` for a consumer who + # does not use LangChain. Empty tuple => every raise is treated + # as a transport error, i.e. the pre-a626bbd behaviour. + _ENFORCEMENT_EXCEPTIONS = () # type: ignore[assignment] + + +# Deferred-enforcement handoff. LangChain swallows exceptions raised +# from a callback, so a block discovered in `on_llm_start` cannot abort +# the LLM call from here. The decision is stashed per-thread; the +# `@protect` boundary (which CAN abort) drains it and raises. +# +# Thread-local, not global: LangGraph runs concurrent chains in a +# thread pool, and one chain's budget block must not abort another's. +import threading as _threading + +_deferred_enforcement: "_threading.local" = _threading.local() + + +def record_deferred_enforcement(exc: BaseException) -> None: + """Stash a real gate decision for the ``@protect`` boundary. + + Called from the LangChain callback path, where the framework + discards exceptions raised by handlers. Returns nothing and never + raises — a stashing failure must not make enforcement worse. + """ + try: + pending = getattr(_deferred_enforcement, "pending", None) + if pending is None: + pending = [] + _deferred_enforcement.pending = pending + pending.append(exc) + except Exception: # noqa: BLE001 — best-effort by construction + pass + + +def drain_deferred_enforcement() -> BaseException | None: + """Return and clear the first stashed decision, if any. + + Called at the ``@protect`` boundary on the same thread, after the + gates have run. Returns the oldest unraised decision so the caller + can fail CLOSED on it. + """ + try: + pending = getattr(_deferred_enforcement, "pending", None) + if not pending: + return None + first = pending[0] + del pending[0] + return first + except Exception: # noqa: BLE001 — best-effort by construction + return None + +# DEF-MP-TS12-SDK-05 (2026-09-29): `langchain-core` is a `dev` extra, +# NOT a core dependency (`pyproject.toml` core deps are httpx only). +# This module used to import it unconditionally at module scope, and +# the import chain is: +# +# NullRunRuntime.__init__ -> instrumentation.auto (make_dedup_state) +# -> instrumentation.langgraph -> langchain_core.callbacks +# +# so a clean `pip install nullrun` + `init()` died with +# `ModuleNotFoundError: No module named 'langchain_core'` — the SDK was +# unusable for every consumer who does not use LangChain, which is the +# majority. +# +# Fix: import the base class defensively and fall back to a bare +# object base. `NullRunCallback` does not call `super().__init__()` and +# defines every method it needs, so it remains fully functional when +# langchain-core IS installed; when it is absent the class still +# imports, and the LangGraph instrumentation that actually requires +# LangChain raises its own clear error at the point of use rather than +# at `import nullrun`. +# +# `langchain-core` is deliberately NOT added to core dependencies — that +# would impose a heavy framework dependency on every consumer. +# +# The guard catches `ImportError`, NOT the narrower +# `ModuleNotFoundError` — this is the only site in the SDK that used +# the narrow form, against 9 siblings that all catch `ImportError` +# (`transport.py`, `transport_websocket.py`, `auto.py`, `autogen.py`, +# `crewai.py`, `llama_index.py`, `auto_requests.py`). +# +# The distinction matters: `ModuleNotFoundError` covers only "this +# module does not exist". A plain `ImportError` is what a PRESENT-BUT- +# BROKEN langchain-core raises from its own dependency chain — the +# pydantic v1/v2 case being the common one. Under the narrow guard +# that propagates out of module scope and `import nullrun` dies, which +# is the exact DEF-MP-TS12-SDK-05 crash, just via a narrower trigger: +# the guard would only have protected users with langchain-core absent +# and left users with it broken still crashing at `init()`. +# +# `object` is the correct fallback for every failure mode here — +# `NullRunCallback` calls no `super()` and defines every method it +# needs — so widening the catch is behaviour-preserving. +try: # pragma: no cover - exercised by the absence test + from langchain_core.callbacks import BaseCallbackHandler +except ImportError: # pragma: no cover + BaseCallbackHandler = object # type: ignore[assignment, misc] + logger = logging.getLogger(__name__) # S-9: FIFO cap on NullRunCallback._active_runs. @@ -538,7 +662,41 @@ def on_llm_start(self, serialized: Any, prompts: Any, **kwargs: Any) -> None: # scope, so the wire-call cost is amortised across the chain. try: self.runtime.check_workflow_budget() - except BaseException as exc: # noqa: BLE001 — never raise out of callback + except _ENFORCEMENT_EXCEPTIONS as exc: + # A REAL gate decision — budget exhausted, workflow killed, + # workflow paused. NOT a transport failure, so the ADR-008 + # fail-OPEN policy does not apply: that policy covers + # "the gate could not be reached", never "the gate said no". + # + # The pre-fix `except BaseException` lumped these together + # with transport errors and dropped them to `logger.debug`, + # so a genuine budget block became an invisible debug line + # and the LLM call proceeded anyway. + # + # Re-raising here would NOT work: LangChain swallows + # exceptions raised from a callback handler (verified + # against langchain-core 1.5.6 — `on_llm_start` logs + # "Error in callback" and continues). So the + # honest options are (a) record the decision so the + # enforcement surface can act on it, and (b) make it + # loud. We do both: the decision is stashed on the + # runtime for `@protect` to raise at a point that CAN + # abort, and it is logged at ERROR rather than debug. + logger.error( + "NullRunCallback.on_llm_start: gate returned a real " + "decision (%s: %s). This LLM call has no reservation; " + "its cost event will be dropped by runtime._route_track. " + "Enforcement of this decision happens at the @protect " + "boundary, which CAN abort.", + type(exc).__name__, + exc, + ) + record_deferred_enforcement(exc) + except Exception as exc: # noqa: BLE001 — never raise out of callback + # Genuine transport failure (backend unreachable, timeout, + # 5xx). Fail-OPEN is the DOCUMENTED ADR-008 policy for + # this class: a dead backend must not freeze the agent, + # and `/track` reconciles the cost afterwards. Unchanged. logger.debug( "NullRunCallback.on_llm_start: check_workflow_budget " "raised %s — proceeding without reservation (llm_call " diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index 42773db..6023a1d 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -17,7 +17,7 @@ | Gate | Transport-error behavior | Recovery behavior | Opt-out | |---|---|---|---| -| `check_workflow_budget` | OPEN (skip check, log warning) | silent post-hoc correction in `/track` events via `cost_correction_applied=true` | `NULLRUN_SKIP_BUDGET_CHECK=1` -- **full billing bypass**, not just check bypass (see docstring WARNING) | +| `check_workflow_budget` | OPEN (skip check, log warning) for TRANSPORT errors; **CLOSED for authentication errors** (401 raises `NullRunAuthError` — see below) | silent post-hoc correction in `/track` events via `cost_correction_applied=true` | `NULLRUN_SKIP_BUDGET_CHECK=1` -- **full billing bypass**, not just check bypass (see docstring WARNING) | | `check_control_plane` | OPEN (treat state as `Normal`) | deferred enforcement -- next WS-push or `/status` poll sees the true state | none | | `_enforce_sensitive_tool` (default `_fallback_mode=strict` since v3.53) | CLOSED -- transport returns `decision=block, decision_source=FALLBACK_*` | n/a | none for the strict path; `NULLRUN_SENSITIVE_FAIL_OPEN=1` opts into the legacy permissive override | | `_enforce_sensitive_tool` (`_fallback_mode=permissive`, opt-in) | CLOSED -- body MUST NOT run when `decision_source` is any `FALLBACK_*` | n/a (body did not run) | `NULLRUN_SENSITIVE_FAIL_OPEN=1` -- explicitly documented as "OPEN-when-engine-unavailable" | @@ -42,6 +42,14 @@ * **SDK-side transport failure** (network timeout, 5xx, breaker open) → fail-OPEN on the *check* path so a dead backend doesn't freeze the user's agent loop (this is what the README describes). +* **Authentication failure (401)** → fail-CLOSED. + ``NullRunAuthError`` propagates. A 401 is a CREDENTIAL/CONFIG + failure, not a transient transport condition: no retry can fix a + revoked key, and there is no post-hoc correction path in ``/track`` + that can retroactively authorise a call the backend refused. Failing + OPEN here means the agent proceeds on a request the backend never + approved. Added 2026-09-29 (DEF-MP-TS12-ENF-01) — this NARROWS the + fail-OPEN set, it does not widen enforcement. * **Backend-side budget-enforcement failure** (the /gate or /track handler actually returned a wire response, just one indicating a Redis outage or aggregate rate-limit Redis unavailable) → the @@ -123,6 +131,7 @@ _emit_for_transport_error, _protocol_header_value, _safe_json, + is_fallback_decision_source, ) from nullrun.uuid7 import uuid7_str @@ -192,10 +201,6 @@ def _invalidate_gate_cache_for_chain(workflow_id: str | None, chain_id: str | No _GATE_CACHE.pop(k, None) return len(keys_to_drop) -# Tracks which runtime instances have already forced strict mode, -# preventing re-initialization from regressing back to permissive. -_STRICT_MODE_FORCED: set[str] = set() - # Production-environment detection for security opt-out # enforcement. ``NULLRUN_SKIP_BUDGET_CHECK=1`` is documented as a @@ -272,28 +277,27 @@ def _is_production_environment(api_url: str | None = None) -> bool: return False -def register_strict_mode_forced(tool_name: str) -> None: - """Mark ``tool_name`` as needing strict mode. - - Called by ``@sensitive(impact=...)`` at decoration time. The - name stays in the module-level set until process exit; it - is intentionally not cleared by ``shutdown()`` so a - second-runtime reinit does not silently drop a tool out of - strict mode. - """ - _STRICT_MODE_FORCED.add(tool_name) - - -def is_strict_mode_forced(tool_name: str) -> bool: - """Return True if ``tool_name`` was decorated with ``@sensitive``. - - Complements ``runtime.is_sensitive_tool(tool_name)`` which - reads the per-runtime registry. The two are OR'd in - ``runtime.execute`` so that a tool whose registration is - lost to runtime reinit still gets the strict /execute - round-trip it asked for. - """ - return tool_name in _STRICT_MODE_FORCED +# B1 (2026-09-30): `register_strict_mode_forced`, `is_strict_mode_forced` +# and the module-level `_STRICT_MODE_FORCED` set are REMOVED. +# +# They existed for exactly one purpose: to force a tool through +# /execute even when the caller had asked for `mode="inline"`. With +# the inline bypass gone, every call is a round-trip unconditionally +# and there is nothing left to force. +# +# The set was already orphaned before this change — its only +# documented writer, a `@sensitive(impact=...)` decorator, no longer +# exists in the SDK, so `register_strict_mode_forced` had zero +# callers and `is_strict_mode_forced` was reachable only from the +# inline branch. Left in place they would read as a live mechanism +# and invite someone to wire them back up. +# +# NOT removed: the per-runtime sensitivity registry +# (`add_sensitive_tool`, `register_sensitive_tools`, +# `remove_sensitive_tool`, `is_sensitive_tool`, +# `get_sensitive_tools`). That is a documented public surface, and +# whether it should outlive the inline bypass is a separate +# decision — flagged, not taken here. SERVER_MINTED_RESERVATION_MAX_AGE_SECONDS: float = 295.0 @@ -1808,6 +1812,28 @@ def check_control_plane(self, workflow_id: str) -> None: # there's no workflow to check, so we no-op. resolved = self._resolve_workflow_id(workflow_id or None) if not resolved: + # F5/F6 (2026-09-30): the no-op is correct — a never-bound + # API key has no control-plane state to poll — but it was + # completely silent, which makes a BROKEN binding + # indistinguishable from a working one. If the key's + # 1:1 workflow binding is lost (bad migration, restored + # backup, wrong key), the kill/pause gate stops running + # and the only symptom is an agent that ignores the + # dashboard. + # + # Counted, not raised: raising here would break the + # documented never-bound-key configuration. Debug-level + # logging, because for a legitimately unbound key this + # fires on every call. + logger.debug( + "check_control_plane: no workflow resolved " + "(contextvar and API-key binding both empty) — " + "kill/pause gate skipped for this call." + ) + try: + metrics.inc_runtime("control_plane_no_workflow_total") + except Exception: # noqa: BLE001 — metrics never gate + pass return workflow_id = resolved @@ -1986,6 +2012,22 @@ def check_workflow_budget(self) -> None: # /gate intentionally keeps the wire minimal. workflow_id = self._resolve_workflow_id(get_workflow_id()) if not workflow_id: + # F5/F6 (2026-09-30): counted rather than silent. See the + # matching note in `check_control_plane` — a lost key + # binding must not look like normal operation. `check_calls` + # above proves the pre-flight was ENTERED; this proves it + # was skipped for want of a workflow, which together are + # what an operator needs to tell "gate ran and allowed" + # from "gate never ran". + logger.debug( + "check_workflow_budget: no workflow resolved " + "(contextvar and API-key binding both empty) — " + "budget pre-flight skipped for this call." + ) + try: + metrics.inc_runtime("budget_preflight_no_workflow_total") + except Exception: # noqa: BLE001 — metrics never gate + pass return # Use the real model name from the call context if the user set @@ -2077,6 +2119,20 @@ def check_workflow_budget(self) -> None: # Cache miss or expired — go to the server, then store. try: response = self._transport.check(check_req) + except NullRunAuthenticationError: + # DEF-MP-TS12-ENF-01 (RUN_ID 20260929T1338, 2026-09-29): + # a 401 is a CREDENTIAL failure, not a transport + # failure, and must NOT be read as "allowed". + # + # `_retry_with_backoff` raises `NullRunAuthError` (a + # `NullRunAuthenticationError`) on any 401 and + # re-raises it WITHOUT retrying. Pre-fix the + # `except` clauses below swallowed it and returned + # None, which the caller reads as "no block" — the + # agent proceeded on a request the backend had + # refused. Classification is by TYPE here, never by + # inspecting the message. + raise except (httpx.HTTPError, NullRunError) as exc: # Narrow catch: fail-OPEN only on transport + # classified SDK errors. Internal bugs @@ -2090,6 +2146,11 @@ def check_workflow_budget(self) -> None: else: try: response = self._transport.check(check_req) + except NullRunAuthenticationError: + # Same rationale as the cached branch above. Ordering + # matters: this arm precedes the broad `except + # Exception`, which is a superset. + 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") @@ -2099,16 +2160,18 @@ def check_workflow_budget(self) -> None: decision = response.get("decision", "allow") decision_source = response.get("decision_source", DecisionSource.GATEWAY) - # Only fail-OPEN on EXPLICIT synthetic responses - # (decision_source starts with "fallback" or is one of the - # classified TransportErrorSource values). Real backend + # Only fail-OPEN on EXPLICIT synthetic responses. Real backend # decisions (decision_source="gateway") are honoured. - if decision_source.startswith("fallback") or decision_source in { - TransportErrorSource.NETWORK_ERROR, - TransportErrorSource.GATEWAY_ERROR, - TransportErrorSource.BREAKER_OPEN, - TransportErrorSource.AUTH_ERROR, - }: + # + # DEF-MP-TS12-ENF-01: this was the copy that got `AUTH_ERROR` + # removed (Fix D) — a credential failure must not be + # reinterpreted as "transport error, carry on". The same + # predicate was ALSO written out in + # `decorators._run_tool_policy_gate`, and that copy still had + # `AUTH_ERROR` and tested an uppercase `"FALLBACK_"` prefix + # that no transport code path produces. Both now call the + # single definition, `transport.is_fallback_decision_source`. + if is_fallback_decision_source(decision_source): logger.warning( f"check_workflow_budget: synthetic decision_source=" f"{decision_source!r}, treating as transport error" @@ -2894,8 +2957,8 @@ def execute( Args: tool_name: Name of the tool to execute input_data: Tool input parameters - mode: Execution mode ("auto", "inline", "strict") - - "auto": auto-select based on tool risk + mode: Execution mode ("auto", "strict"). "inline" was + removed in 0.19.0 and now raises. on_transport_error: Optional callback for transport-error handling; prefer the typed exception path. business_impact: Typed action payload (Money impact for @@ -2930,22 +2993,68 @@ def execute( omitting ``mode``. Earlier SDKs silently switched to "inline" for non-sensitive tools, which was the source of the DEF-TS12-01 audit cycle. - - "inline": explicit opt-out of /execute. Skips ALL - enforcement (budget / rate / tool-block); returns - a synthetic local allow. Use only when the caller - knows the tool is safe and wants to skip the - gateway round-trip. Cannot be combined with - sensitive tools — sensitive tools always go to - /execute even when "inline" is requested. - "strict": explicit gateway round-trip (same wire behaviour as "auto", but useful for audit clarity when the caller wants the intent on the wire). + "inline" was removed in 0.19.0 (B1 / ADR-061) and + now raises `NullRunConfigError`. It skipped /execute + entirely — budget, rate-limit and tool-block all + bypassed — and its only guard was a sensitivity + check, so whether a call was enforced depended on + whether someone had remembered to mark the tool + sensitive. Both remaining values contact the gateway + unconditionally. + Raises: NullRunBlockedException: If decision is "block" + NullRunConfigError: If mode="inline" (removed, see above) """ from nullrun.context import get_trace_id, get_workflow_id + # B1 (2026-09-30): `mode="inline"` is GONE. + # + # It returned a synthesised local `allow` without contacting + # the gateway, so budget, rate limit and tool_block were all + # skipped — the SDK's own explanation said so, and it was the + # single largest hole in "the gate decides". The only guard + # was a sensitivity check, which meant the safety of a tool + # call depended on whether someone had remembered to mark it + # sensitive. + # + # It is also vestigial in the other direction: `mode` is sent + # on the wire but the backend does not read it + # (`transport.py:1223`, "Wire-present but unused by backend"). + # So the parameter's ONLY real function was deciding whether + # to skip enforcement. + # + # This raises rather than silently coercing to "strict". A + # caller who asked for inline believes they have a fast local + # path; quietly giving them a round-trip is a semantic change + # they cannot see, and a silent coercion is how a bypass gets + # reintroduced later. A loud, named error is the honest + # version of the same change. + # + # It sits at the very TOP of the method, before any context + # resolution or setup: a removed argument is a caller-side + # programming error, and refusing it before doing any work is + # what makes the refusal a guarantee rather than an + # incidental ordering. + if mode == "inline": + from nullrun.breaker.exceptions import NullRunConfigError + + raise NullRunConfigError( + 'mode="inline" has been removed. It skipped /execute ' + "entirely, so budget, rate-limit and tool-block policies " + "were all bypassed — a synthesised local `allow` the " + "gateway never made. There is no replacement: every " + "call now goes through /execute. If you were using " + 'inline to avoid a round-trip, use mode="auto" (the ' + "default) and treat the extra latency as the cost of " + "actually being enforced. See ADR-061.", + error_code="NR-S001", + ) + organization_id = self.organization_id or "local" workflow_id = get_workflow_id() trace_id = get_trace_id() or str(uuid.uuid4()) @@ -2958,43 +3067,13 @@ def execute( # cannot be silently bypassed because the SDK caller used the # default ``mode="auto"``. # - # Explicit opt-out paths: - # 1. ``mode="inline"`` (explicit opt-in by the caller) — - # returns the local allow WITHOUT contacting the gateway. - # Documented as the only way to skip /execute. Use - # sparingly: skips ALL enforcement, not just budget. - # 2. ``mode="strict"`` (explicit opt-in by the caller) — - # forces /execute round-trip regardless of tool - # sensitivity. Identical wire behaviour to ``"auto"``, - # but useful when the caller wants the intent on the - # wire for audit clarity. - # - # The two sensitivity checks below still gate the inline - # fast-path — sensitive tools cannot be silently skipped - # even if the caller explicitly asks for ``mode="inline"``. - # They also gate the /execute round-trip when ``mode="auto"`` - # resolved to ``"strict"`` (no behavioural change there). + # ``mode="strict"`` is the only other accepted value. It is + # identical to ``"auto"`` on the wire and exists so a caller + # can put the intent on the wire for audit clarity. There is + # no opt-out: every call is a /execute round-trip. if mode == "auto": mode = "strict" - # For inline mode with non-sensitive tools, skip execute and use local enforcement. - # Sensitive tools always go through /execute even when the - # caller asked for ``mode="inline"`` — fail-CLOSED stance per - # memory `sensitive-tool-fail-closed`. - if mode == "inline" and not ( - self.is_sensitive_tool(tool_name) or is_strict_mode_forced(tool_name) - ): - return { - "decision": "allow", - "decision_source": DecisionSource.LOCAL, - "explanation": ( - "Inline mode: local enforcement only. Caller explicitly opted " - "out of /execute — budget / rate / tool-block policies bypassed." - ), - "policy_hash": None, - "allow_execution": True, - } - # Strict mode or sensitive tool: call /execute endpoint # (no local_mode branch -- api_key is now required, see T3-S2). # Keep one operation_id across the initial request and the diff --git a/src/nullrun/toolbox/mcp.py b/src/nullrun/toolbox/mcp.py index 45dd6a2..d2e5fa8 100644 --- a/src/nullrun/toolbox/mcp.py +++ b/src/nullrun/toolbox/mcp.py @@ -145,7 +145,11 @@ class MCPAdapter: adapter = MCPAdapter(server_name="github", mcp_client=conn) # Now every `adapter.call_tool` stamps the gate with - # the canonical class + the cached annotations. + # the canonical class + the cached annotations AND is + # gated by /api/v1/execute before the MCP server is + # called. The runtime is resolved from the global + # registry (or NULLRUN_API_KEY) on the same terms + # `@protect` resolves it — pass `runtime=` to override. result = adapter.call_tool("create_issue", {"repo": "acme/api"}) """ @@ -162,20 +166,27 @@ def __init__( # calls just like they do to local functions decorated with # ``@protect``. # - # When ``runtime`` is None the adapter falls back to the legacy - # contextvar-only path (``set_mcp_tool_context``) so callers - # who already wrap their agentic loop in ``@protect`` continue - # to work — but those callers MUST verify that their - # ``@protect``-decorated wrapper actually invokes - # ``check_workflow_budget`` BEFORE the MCP call returns, - # otherwise the gate is a post-hoc advisory only. + # When ``runtime`` is None the adapter resolves the global + # runtime itself, on the same terms ``@protect`` does + # (``get_active_runtime()`` then ``NullRunRuntime.get_instance()``). + # B2 (2026-09-30 / ADR-061) removed the previous behaviour, + # which was: ``runtime=None`` meant "no /execute round-trip at + # all". The MCP call went straight through and the only thing + # the operator got was a contextvar that a *later* ``@protect`` + # wrapper might or might not read — a post-hoc annotation, not + # enforcement, and the module's own documented example + # (``MCPAdapter(server_name=..., mcp_client=conn)``) took + # exactly that ungated path. # - # Why the runtime is opt-in rather than auto-discovered: - # ``MCPAdapter`` is intentionally decoupled from the runtime - # singleton so it stays importable in test fixtures and - # documentation snippets without forcing ``nullrun.init()``. - # The audit-grade fix is to give callers a one-line way to - # wire enforcement without breaking the toolbox-only pattern. + # Resolution is lazy (at ``call_tool``, not at construction) + # so the adapter stays importable and constructible in test + # fixtures and documentation snippets without forcing + # ``nullrun.init()`` — that was the original reason for the + # decoupling, and it is preserved. A configuration error at + # call time (no API key) propagates loudly rather than + # degrading to an ungated call, matching ``@protect``'s + # fail-loud invariant: a missing API key is a hard error, not + # a silent allow-all. runtime: Any | None = None, ) -> None: if not server_name: @@ -203,11 +214,36 @@ def __init__( # every ``call_tool``. When provided, ``call_tool`` blocks on # ``runtime.execute(...)`` returning decision="block" so a # permissive MCP server cannot bypass the operator's - # tool-block / budget / approval policies. See the constructor - # docstring for the trade-off between the gate path and the - # contextvar-only path. + # tool-block / budget / approval policies. When None, the + # global runtime is resolved lazily at ``call_tool`` time — + # see the constructor docstring. self._runtime = runtime + def _resolve_runtime(self) -> Any: + """The runtime this adapter gates through. + + B2 (2026-09-30): the explicit ``runtime=`` wins; otherwise the + global runtime is resolved on the same terms ``@protect`` + resolves it. Never returns None — there is no ungated path. + + ``@protect``'s own resolver is not imported: it also triggers + ``auto_instrument()``, which is the decorator's job and would + be a surprising side effect for a caller who handed us an MCP + client. The two lines of resolution order are what matter, and + they are pinned by a test. + """ + if self._runtime is not None: + return self._runtime + from nullrun._registry import get_active_runtime + from nullrun.runtime import NullRunRuntime + + active = get_active_runtime() + if active is not None: + return active + # Propagates NullRunAuthenticationError when NULLRUN_API_KEY is + # unset. Deliberate — see the constructor docstring. + return NullRunRuntime.get_instance() + def _default_list_tools(self) -> Iterable[Any]: tools = self._mcp_client.list_tools() # Accept any iterable — caller might return a list, @@ -299,10 +335,11 @@ def call_tool( client-specific kwargs without changing the public surface. - Gate enforcement: when an MCPAdapter is constructed with - ``runtime=`` set, ``call_tool`` routes the invocation through - ``runtime.execute(...)`` (the /api/v1/execute gate endpoint) - BEFORE the underlying MCP client is called. + Gate enforcement: ``call_tool`` ALWAYS routes the invocation + through ``runtime.execute(...)`` (the /api/v1/execute gate + endpoint) BEFORE the underlying MCP client is called. The + runtime is the one passed to the constructor, or the global + runtime resolved on the same terms ``@protect`` resolves it. ``decision="block"`` raises ``NullRunBlockedException`` and the MCP client is NOT called. ``decision="allow"`` proceeds to the MCP client. ``decision="require_approval"`` raises @@ -310,10 +347,11 @@ def call_tool( caller can route the user through the approval flow and retry with ``approval_id=``. - When ``runtime`` is None, ``call_tool`` falls through to the - contextvar-only path — the call proceeds without any - /api/v1/execute round-trip and the next ``@protect``-decorated - wrapper picks up the contextvar on its next ``/check`` request. + B2 (2026-09-30): there is no ungated path. ``runtime=None`` + used to mean "skip the round-trip entirely"; it now means + "resolve the global runtime". A configuration error at this + point (no API key) propagates rather than degrading to an + ungated call. Returns the underlying client's result (when allowed). Raises ``NullRunBlockedException`` on gate block; raises the @@ -366,61 +404,62 @@ def call_tool( # unless an ``approval_id`` is supplied) — both short-circuit # to the call site without touching ``self._mcp_client``. # - # who relied on the contextvar-only path continue to work. - # New integrations should pass ``runtime=`` so the - # tool-block / budget / approval policies actually apply. - if self._runtime is not None: - execute_input = arguments if arguments is not None else {} - # Every MCP tool call routed through the runtime contacts - # /api/v1/execute unconditionally — there is no - # ``mode=`` opt-out for audit bypass. - execute_result = self._runtime.execute( + # B2 (2026-09-30): this is no longer conditional. The previous + # ``if self._runtime is not None:`` meant a default-constructed + # adapter — including the one in this module's own docstring — + # called the MCP server with no gate at all. + runtime = self._resolve_runtime() + execute_input = arguments if arguments is not None else {} + # Every MCP tool call routed through the runtime contacts + # /api/v1/execute unconditionally — there is no + # ``mode=`` opt-out for audit bypass. + execute_result = runtime.execute( + tool_name=tool_name, + input_data=execute_input, + ) + decision = execute_result.get("decision") + if decision == "block": + # ``NullRunBlockedException`` is raised by + # ``runtime.execute`` internally; this guard is for + # defense-in-depth in case the runtime returns a + # synthetic block (e.g. PERMISSIVE fallback in + # tests) and the exception path was bypassed. + from nullrun.breaker.exceptions import NullRunBlockedException + + raise NullRunBlockedException( + workflow_id=execute_result.get("workflow_id") or "unknown", + reason=execute_result.get( + "explanation", + "MCP gate blocked call", + ), + tool_name=tool_name, + error_code="NR-T003", + user_action=( + f"MCPAdapter.call_tool({tool_name!r}) was blocked " + "by the NullRun gate. The MCP client was NOT " + "invoked. Inspect the operator's tool-block / " + "budget / approval policy to allow this call." + ), + ) + if decision == "require_approval": + from nullrun.breaker.exceptions import NullRunBlockedException + + approval_id = execute_result.get("approval_id") or "" + raise NullRunBlockedException( + workflow_id=execute_result.get("workflow_id") or "unknown", + reason=execute_result.get( + "explanation", + "MCP gate requires operator approval", + ), tool_name=tool_name, - input_data=execute_input, + error_code="NR-A010" if not approval_id else "NR-A001", + user_action=( + f"MCPAdapter.call_tool({tool_name!r}) requires " + "operator approval before the MCP client is " + "invoked. Route the user through the approval " + f"flow and retry with approval_id={approval_id!r}." + ), ) - decision = execute_result.get("decision") - if decision == "block": - # ``NullRunBlockedException`` is raised by - # ``runtime.execute`` internally; this guard is for - # defense-in-depth in case the runtime returns a - # synthetic block (e.g. PERMISSIVE fallback in - # tests) and the exception path was bypassed. - from nullrun.breaker.exceptions import NullRunBlockedException - - raise NullRunBlockedException( - workflow_id=execute_result.get("workflow_id") or "unknown", - reason=execute_result.get( - "explanation", - "MCP gate blocked call", - ), - tool_name=tool_name, - error_code="NR-T003", - user_action=( - f"MCPAdapter.call_tool({tool_name!r}) was blocked " - "by the NullRun gate. The MCP client was NOT " - "invoked. Inspect the operator's tool-block / " - "budget / approval policy to allow this call." - ), - ) - if decision == "require_approval": - from nullrun.breaker.exceptions import NullRunBlockedException - - approval_id = execute_result.get("approval_id") or "" - raise NullRunBlockedException( - workflow_id=execute_result.get("workflow_id") or "unknown", - reason=execute_result.get( - "explanation", - "MCP gate requires operator approval", - ), - tool_name=tool_name, - error_code="NR-A010" if not approval_id else "NR-A001", - user_action=( - f"MCPAdapter.call_tool({tool_name!r}) requires " - "operator approval before the MCP client is " - "invoked. Route the user through the approval " - f"flow and retry with approval_id={approval_id!r}." - ), - ) # Call through. We deliberately do NOT catch the # underlying client's exceptions — the SDK caller diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index 401b031..49e8ca3 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -57,8 +57,8 @@ _OTEL_AVAILABLE = True except ImportError: _OTEL_AVAILABLE = False - trace = None # type: ignore[assignment] - TraceContextTextMapPropagator = None # type: ignore[assignment] + trace = None # type: ignore[assignment, misc] + TraceContextTextMapPropagator = None # type: ignore[assignment, misc] logger = logging.getLogger(__name__) @@ -410,6 +410,48 @@ class DecisionSource: LOCAL = "local" +def is_fallback_decision_source(source: object) -> bool: + """True when ``source`` marks a SYNTHETIC decision, not a real one. + + A decision is synthetic when the transport degraded instead of + reaching the gateway. ADR-008's fail-OPEN/CLOSED rules then apply; + a real ``gateway`` decision is always honoured regardless. + + Two shapes exist, both produced by this module: + + * ``DecisionSource.FALLBACK`` (``"fallback"``) — the generic + degradation, e.g. ``fallback_mode=STRICT`` or a gateway response + that could not be parsed into a decision. + * a ``TransportErrorSource`` member (``"NETWORK_ERROR"``, + ``"GATEWAY_ERROR"``, ``"BREAKER_OPEN"``) — returned only when the + caller passed ``on_transport_error="open"`` or ``"closed"``. + ``TransportErrorSource`` is a ``str`` Enum, so these compare + equal to their uppercase string form. + + ``AUTH_ERROR`` is deliberately NOT a fallback. It is a credential + failure, not an unreachable gate: the transport re-raises + ``NullRunAuthenticationError`` rather than degrading, and treating + it as a transport error would let a bad API key read as "engine + unavailable, carry on". DEF-MP-TS12-ENF-01. + + This function exists because the predicate was previously written + out twice — in ``runtime.check_workflow_budget`` and in + ``decorators._run_tool_policy_gate`` — and the copies had already + drifted: one matched lowercase ``"fallback"`` and the other + uppercase ``"FALLBACK_"`` (a prefix no code path produces, so that + copy's first clause could never fire), and only one had + ``AUTH_ERROR`` removed. Two hand-maintained copies of a + security-relevant predicate is one copy too many. + """ + if not isinstance(source, str): + return False + return source == DecisionSource.FALLBACK or source in { + TransportErrorSource.NETWORK_ERROR.value, + TransportErrorSource.GATEWAY_ERROR.value, + TransportErrorSource.BREAKER_OPEN.value, + } + + @dataclass class FlushConfig: """Configuration for transport flush behavior.""" @@ -978,8 +1020,10 @@ def _send_batch_with_retry_info(self, batch: list[dict[str, Any]]) -> "SendResul """Send batch to server. Returns SendResult with retry info. Wrapped by _retry_with_backoff.""" logger.debug(f"Sending batch of {len(batch)} events to {self.api_url}/api/v1/track/batch") body = _signed_request_body({"events": batch}) - headers = self._build_signed_headers(body=body) + # S008: re-sign per attempt — see the long note at + # `do_execute_request` (same defect, same fix). + # # Inner function is the unit of retry: # * 5xx → retry helper backs off. 429 honors Retry-After. # * 4xx (other than 429) → return as-is; these are real client bugs @@ -988,7 +1032,7 @@ def _post_batch() -> httpx.Response: resp = self._client.post( f"{self.api_url}/api/v1/track/batch", content=body, - headers=headers, + headers=self._build_signed_headers(body=body), ) if resp.status_code >= 500 or resp.status_code == 429: # raise_for_status turns this into HTTPStatusError; the retry @@ -1191,13 +1235,29 @@ def execute( gate_request["tools"] = list(tools) body = _signed_request_body(gate_request) - headers = self._build_signed_headers(body=body) + # S008 / DEF-MP-TS12-ENF-01 (2026-09-29): sign INSIDE the + # retry closure. Pre-fix `headers` was built once here, so + # every one of the (up to 10) retries replayed a byte-identical + # signature. The backend's S008 replay guard + # (`hmac:replay:{key_fp}:{sig_hash}`, `hmac_verify.rs`) marks + # the first occurrence and rejects the rest as HMAC_REPLAY — + # so a single transient 5xx turned the whole retry budget into + # a wall of replay rejections, and the 401 that came back was + # indistinguishable from a genuinely invalid key. + # + # `_build_signed_headers` recomputes `int(time.time())` and the + # HMAC on every call, so each attempt now carries a distinct + # `sig_hash` and is not a replay. The backend deferred exactly + # this fix ("the Python SDK builds its signed headers ONCE + # outside the retry closure ... Tracked as S008 v2") pending + # this change. A true single-use `X-Nonce` remains a protocol + # change and is deliberately still out of scope. def do_execute_request() -> httpx.Response: return self._client.post( f"{self.api_url}/api/v1/execute", content=body, - headers=headers, + headers=self._build_signed_headers(body=body), timeout=5.0, ) @@ -1514,8 +1574,15 @@ def check( gate_request["parent_execution_id"] = _parent_execution_id body = _signed_request_body(gate_request) - headers = self._build_signed_headers(body=body) + # S008 / DEF-MP-TS12-ENF-01 (2026-09-29): sign INSIDE the + # retry closure. Pre-fix `headers` was built once, so each of + # the 3 retries replayed a byte-identical signature and the + # backend's S008 guard rejected the retry as HMAC_REPLAY. That + # 401 is what surfaced to `check_workflow_budget` as a + # credential error during the TS-12 cycle (prod x684). See the + # long note at `do_execute_request` for the full rationale. + # # ``_retry_with_backoff`` with ``retry_on_5xx=True`` and # ``max_retries=3`` (per audit recommendation: "less than # 10 — /gate is critical and too many retries amplify @@ -1529,7 +1596,7 @@ def _do_gate_post() -> httpx.Response: return self._client.post( f"{self.api_url}/api/v1/gate", content=body, - headers=headers, + headers=self._build_signed_headers(body=body), timeout=5.0, ) @@ -3273,6 +3340,7 @@ def _parse_error_envelope( "HEADER_PROTOCOL", "NULLRUN_PROTOCOL_VERSION", "DecisionSource", + "is_fallback_decision_source", "FallbackMode", "FlushConfig", "ExecuteConfig", diff --git a/tests/test_auth_fail_closed.py b/tests/test_auth_fail_closed.py new file mode 100644 index 0000000..c62a058 --- /dev/null +++ b/tests/test_auth_fail_closed.py @@ -0,0 +1,113 @@ +"""tests/test_auth_fail_closed.py — 401 must not read as "allowed". + +DEF-MP-TS12-ENF-01 (RUN_ID 20260929T1338, 2026-09-29). + +`check_workflow_budget` re-read a 401 as permission to proceed: + + 1. `_retry_with_backoff` raises `NullRunAuthError` on any 401 and + re-raises it WITHOUT retrying. + 2. Both `except` arms in `check_workflow_budget` swallowed it and + returned None, which the caller reads as "no block". + 3. `TransportErrorSource.AUTH_ERROR` was also in the synthetic + fail-OPEN set, so a 401 arriving as a returned response rather + than a raise was reclassified as a transport error. + +The agent therefore executed a call the backend had refused. + +The fix classifies by TYPE (`NullRunAuthenticationError`), never by +inspecting the message, and preserves ADR-008's transport fail-OPEN +policy unchanged. The three-branch contract these tests pin: + + connection/transport failure -> fail-OPEN (proceed) + 401 authentication failure -> raise + enforcement 4xx -> enforcement exception +""" + +from __future__ import annotations + +import httpx +import pytest +import respx + +from nullrun.breaker.exceptions import ( + NullRunAuthError, + NullRunBudgetError, +) + +BASE_URL = "https://api.test.nullrun.io" +GATE_URL = f"{BASE_URL}/api/v1/gate" + + +class TestAuthFailClosed: + def test_401_raises_instead_of_failing_open(self, make_runtime, mock_api): + """The Blocker itself: a refused key must not become "allowed".""" + respx.post(GATE_URL).mock( + return_value=httpx.Response(401, json={"error_code": "HMAC_REPLAY"}) + ) + rt = make_runtime() + with pytest.raises(NullRunAuthError): + rt.check_workflow_budget() + + def test_401_in_cache_enabled_path_also_raises( + self, make_runtime, mock_api + ): + """The cached branch has its own except arm — pin it too. + + Pre-fix both arms swallowed independently, so fixing only the + non-cached path would have left the chain-mode hole open. + `chain_id` is read from a contextvar, so the cached path is + reached by entering a chain scope. + """ + import uuid + + from nullrun.context import chain + + respx.post(GATE_URL).mock( + return_value=httpx.Response(401, json={"error_code": "HMAC_REPLAY"}) + ) + rt = make_runtime() + with chain(str(uuid.uuid4())): + with pytest.raises(NullRunAuthError): + rt.check_workflow_budget() + + def test_transport_failure_still_fails_open(self, make_runtime, mock_api): + """ADR-008 preserved: a dead backend must not freeze the agent. + + This is the counter-test to the two above. If a future change + widens the fail-CLOSED arm to all exceptions, this fails — + which is the point: the fix must narrow auth, not close the + gate. + """ + respx.post(GATE_URL).mock( + side_effect=httpx.ConnectError("connection refused") + ) + rt = make_runtime() + # No exception: fail-OPEN, the call proceeds. + result = rt.check_workflow_budget() + assert result is None, "transport failure must still fail open" + + def test_enforcement_4xx_still_raises_budget_error( + self, make_runtime, mock_api + ): + """A real enforcement block keeps raising the budget exception. + + This is the case the backend half of DEF-MP-TS12-ENF-01 + produces: with the status-mapping fix, + BUDGET_WORKFLOW_BLOCKED arrives as 402 and must surface as + NullRunBudgetError, not be swallowed. + """ + respx.post(GATE_URL).mock( + return_value=httpx.Response( + 402, + json={ + "decision": "block", + "decision_source": "gateway", + "explanation": "BUDGET_WORKFLOW_BLOCKED", + "error_code": "BUDGET_WORKFLOW_BLOCKED", + "details": {"max_budget_cents": 100}, + }, + ) + ) + rt = make_runtime() + with pytest.raises(NullRunBudgetError): + rt.check_workflow_budget() diff --git a/tests/test_fallback_decision_source.py b/tests/test_fallback_decision_source.py new file mode 100644 index 0000000..0d0be49 --- /dev/null +++ b/tests/test_fallback_decision_source.py @@ -0,0 +1,213 @@ +"""tests/test_fallback_decision_source.py — one definition of "synthetic". + +Audit 2026-09-30, following DEF-MP-TS12-ENF-01 (RUN_ID 20260929T1338). + +The defect +---------- +The predicate "was this decision synthesised by a degrading transport, +or did it come from the gateway?" decides whether ADR-008 fail-OPEN or +fail-CLOSED applies. It was written out TWICE, and the two copies had +already drifted: + +* ``runtime.check_workflow_budget`` tested + ``startswith("fallback")`` — lowercase — and, after Fix D, excluded + ``TransportErrorSource.AUTH_ERROR``. +* ``decorators._run_tool_policy_gate`` tested + ``startswith("FALLBACK_")`` — UPPERCASE. No transport code path + produces a decision_source above ``DecisionSource.FALLBACK + == "fallback"``, so that clause could never fire. It also still + listed ``AUTH_ERROR``, the exact hole Fix D had closed one file + away. + +So the decorator's copy was simultaneously dead in its first clause +and wrong in its second. + +The fix is a single definition, ``transport.is_fallback_decision_source``, +called from both sites. The pins below cover the truth table AND the +drift hazard: a test that only checks the truth table would pass +against the old duplicated code, because each copy was individually +plausible. What actually needs pinning is that there is only ONE copy. +""" + +from __future__ import annotations + +import ast +import inspect +import pathlib +import textwrap + +import pytest + +from nullrun.breaker.exceptions import TransportErrorSource +from nullrun.transport import DecisionSource, is_fallback_decision_source + + +class TestTruthTable: + @pytest.mark.parametrize( + "source", + [ + DecisionSource.FALLBACK, + TransportErrorSource.NETWORK_ERROR.value, + TransportErrorSource.GATEWAY_ERROR.value, + TransportErrorSource.BREAKER_OPEN.value, + ], + ) + def test_synthetic_sources_are_recognised(self, source): + """Values the transport actually produces when it degrades.""" + assert is_fallback_decision_source(source) is True, ( + f"{source!r} marks a synthetic decision, so ADR-008's " + "fail-OPEN/CLOSED rule applies rather than honouring a " + "decision the gateway never made." + ) + + @pytest.mark.parametrize( + "source", + [ + DecisionSource.GATEWAY, + DecisionSource.CACHED, + DecisionSource.LOCAL, + ], + ) + def test_real_sources_are_honoured(self, source): + """A real decision must never be reinterpreted as synthetic. + + This is the property that keeps a genuine `block` from being + downgraded to a fail-OPEN allow. + """ + assert is_fallback_decision_source(source) is False, ( + f"{source!r} is a real decision source — treating it as " + "synthetic would discard the gateway's answer." + ) + + def test_auth_error_is_not_a_transport_error(self): + """DEF-MP-TS12-ENF-01: a bad API key is not an unreachable gate. + + Classifying auth as a transport error is what let a 401 read + as "engine unavailable, carry on" — the agent is told + 'allowed' on a call the gate refused. The transport re-raises + `NullRunAuthenticationError` rather than degrading, so this + value can only arrive from a caller that explicitly asked for + `on_transport_error="open"`. Honouring it as a credential + failure is correct in that case too. + """ + assert is_fallback_decision_source(TransportErrorSource.AUTH_ERROR.value) is False + + @pytest.mark.parametrize("source", [None, "", 123, object(), b"fallback"]) + def test_non_string_inputs_are_safe(self, source): + """A malformed envelope must not crash the gate. + + `decision_source` arrives from a parsed JSON body; anything + can be in it. Returning False (honour the decision) is the + safe direction — the `decision` field is still checked + separately. + """ + assert is_fallback_decision_source(source) is False + + def test_case_sensitivity(self): + """Pins the exact defect: `FALLBACK_` vs `fallback`. + + The decorator's copy matched an uppercase prefix that no code + path emits. If this test ever passes for `"FALLBACK_..."`, + the predicate has drifted back to matching a value the + transport never returns. + """ + assert is_fallback_decision_source("FALLBACK_NETWORK_ERROR") is False + assert is_fallback_decision_source(DecisionSource.FALLBACK) is True + + +def _decision_source_comparisons(src: str) -> list[str]: + """Every `decision_source` comparison in `src`, as source snippets. + + AST, not substring: the fix is precisely a comment-and-code change, + and a text search matches the comment that documents the old + defect — the self-defeating-pin hazard. Only real Compare nodes + may satisfy a pin. + """ + tree = ast.parse(textwrap.dedent(src)) + out: list[str] = [] + for node in ast.walk(tree): + if isinstance(node, ast.Compare): + snippet = ast.unparse(node) + if "decision_source" in snippet: + out.append(f"line {node.lineno}: {snippet[:100]}") + return out + + +class TestSingleDefinition: + """The drift hazard itself — the part a truth-table test misses.""" + + def _source(self, rel: str) -> str: + root = pathlib.Path(__file__).resolve().parent.parent + return (root / "src" / "nullrun" / rel).read_text(encoding="utf-8") + + def test_decorator_delegates(self): + """`decorators` must call the shared predicate, not re-derive it.""" + from nullrun import decorators + + src = inspect.getsource(decorators._run_tool_policy_gate) + assert "is_fallback_decision_source" in src, ( + "`_run_tool_policy_gate` must classify via " + "`transport.is_fallback_decision_source`." + ) + # Scoped to `decision_source` on purpose. The function + # legitimately maps `TransportErrorSource.AUTH_ERROR` to error + # code NR-A003 in the *typed* transport-error arm — that is + # correct and unrelated to the fallback predicate. A blanket + # "AUTH_ERROR must not appear" pin would flag valid code. + inline = _decision_source_comparisons(src) + assert not inline, ( + "`_run_tool_policy_gate` still tests `decision_source` " + "inline instead of delegating — that inline set was the " + "copy that kept AUTH_ERROR and matched a dead uppercase " + "prefix:\n " + "\n ".join(inline) + ) + + def test_runtime_delegates(self): + from nullrun import runtime + + src = inspect.getsource(runtime.NullRunRuntime.check_workflow_budget) + assert "is_fallback_decision_source" in src, ( + "`check_workflow_budget` must classify via the shared " + "predicate." + ) + inline = _decision_source_comparisons(src) + assert not inline, ( + "`check_workflow_budget` still tests `decision_source` " + "inline:\n " + "\n ".join(inline) + ) + + def test_no_other_inline_copies_exist(self): + """No third copy may grow anywhere in the package. + + Walks the AST of every module under ``src/nullrun`` looking for + a module-level test of `decision_source` against a transport + error literal. A new call site must import the shared helper, + not re-open the set. + """ + root = pathlib.Path(__file__).resolve().parent.parent + offenders: list[str] = [] + for path in (root / "src" / "nullrun").rglob("*.py"): + text = path.read_text(encoding="utf-8") + try: + tree = ast.parse(text) + except SyntaxError: + continue + for node in ast.walk(tree): + # A `TransportErrorSource.X in {…}` test, or a + # `startswith(...)` on a decision_source. + if not isinstance(node, ast.Compare): + continue + snippet = ast.unparse(node) + if "TransportErrorSource" in snippet and ( + "in {" in snippet.replace(" ", " ") + or "not in" in snippet + ): + # Allowed only inside the shared definition. + if path.name == "transport.py": + continue + offenders.append(f"{path.name}:{node.lineno}: {snippet[:80]}") + assert not offenders, ( + "inline TransportErrorSource membership tests found outside " + "transport.py — each is a hand-maintained copy of the " + "fallback predicate:\n " + "\n ".join(offenders) + ) diff --git a/tests/test_inline_bypass_closed.py b/tests/test_inline_bypass_closed.py new file mode 100644 index 0000000..fe079e2 --- /dev/null +++ b/tests/test_inline_bypass_closed.py @@ -0,0 +1,262 @@ +"""tests/test_inline_bypass_closed.py — `mode="inline"` is gone. + +B1, 2026-09-30 (ADR-061). Part of the DEF-MP-TS12-ENF-01 cluster that +followed RUN_ID 20260929T1338. + +The bypass +---------- +``runtime.execute(..., mode="inline")`` returned a synthesised local +``allow`` without contacting the gateway:: + + return { + "decision": "allow", + "decision_source": DecisionSource.LOCAL, + "explanation": "Inline mode: local enforcement only. Caller + explicitly opted out of /execute — budget / + rate / tool-block policies bypassed.", + ... + } + +So budget, rate limit and tool_block were all skipped, and the only +guard was a sensitivity check — meaning whether a call was enforced +depended on whether someone had remembered to mark the tool sensitive. +``DecisionSource.LOCAL`` is deliberately not a "synthetic" source +(it is a real local decision as far as every consumer of +``is_fallback_decision_source`` is concerned), so downstream code +honoured it exactly as it would honour a gateway allow. + +The parameter was vestigial in the other direction too: ``mode`` goes +on the wire but the backend does not read it (``transport.py:1223``, +"Wire-present but unused by backend"). Its only real function was +deciding whether to skip enforcement. + +The fix raises instead of silently coercing to "strict": a caller who +asked for inline believes they have a fast local path, and quietly +handing them a round-trip is a semantic change they cannot see. +""" + +from __future__ import annotations + +import ast +import pathlib + +import pytest + +from nullrun.breaker.exceptions import NullRunConfigError +from nullrun.runtime import NullRunRuntime + + +@pytest.fixture +def rt(): + """A runtime with no transport — inline must be refused before any I/O. + + Built via ``object.__new__`` deliberately: the point of the test is + that the refusal happens at the TOP of ``execute``, before the + runtime touches its transport, so a real constructor (which + authenticates over the network) would test nothing. + """ + return object.__new__(NullRunRuntime) + + +class TestInlineRefused: + def test_inline_raises(self, rt): + with pytest.raises(NullRunConfigError): + rt.execute("some_tool", {"args": {}}, mode="inline") + + def test_error_names_the_removal_and_the_absence_of_a_replacement(self, rt): + """The message is the migration path — a user has to read it. + + A bare TypeError or "invalid mode" would leave the caller with + no idea what to do instead, which is how people end up + reaching for a bypass. + """ + with pytest.raises(NullRunConfigError) as exc: + rt.execute("some_tool", {"args": {}}, mode="inline") + msg = str(exc.value) + assert "inline" in msg + # Must say what the thing WAS, so the user understands the + # security consequence of what they were getting. + assert "bypass" in msg.lower(), ( + f"the error must state that inline bypassed enforcement, " + f"not just that the value is invalid: {msg!r}" + ) + # Must point somewhere. + assert "auto" in msg, f"the error must name the replacement: {msg!r}" + + def test_error_carries_a_code(self, rt): + with pytest.raises(NullRunConfigError) as exc: + rt.execute("some_tool", {"args": {}}, mode="inline") + assert exc.value.error_code, ( + "a config error without an error_code cannot be triaged from a log" + ) + + @pytest.mark.parametrize("tool", ["charge_card", "send_email", "x"]) + def test_refused_for_every_tool_including_sensitive_ones(self, rt, tool): + """The old guard let SENSITIVE tools through to /execute. + + That is precisely why the check was the problem: enforcement + of an ordinary tool was a configuration detail, and a tool + that nobody remembered to mark ran ungated. Now the refusal + does not depend on the tool at all. + """ + with pytest.raises(NullRunConfigError): + rt.execute(tool, {"args": {}}, mode="inline") + + @pytest.mark.parametrize("mode", ["auto", "strict"]) + def test_surviving_modes_are_not_refused(self, rt, mode): + """Only "inline" is gone — the other two must still get through. + + These two proceed to the /execute round-trip, which the bare + runtime object cannot perform. Reaching the transport rather + than raising `NullRunConfigError` is exactly the proof wanted: + the mode check let them past. + """ + with pytest.raises(Exception) as exc: + rt.execute("some_tool", {"args": {}}, mode=mode) + assert not isinstance(exc.value, NullRunConfigError), ( + f"mode={mode!r} must still be accepted; only 'inline' is removed. Got {exc.value!r}" + ) + + +class TestNoBypassRemains: + """The bypass must be gone from the CODE, not just unreachable. + + A test that only checks the raise would pass while a commented-out + or refactored copy of the local-allow dict sat in the file — which + is exactly the shape of the defect being closed. + """ + + def _execute_source(self) -> str: + import inspect + + return inspect.getsource(NullRunRuntime.execute) + + def test_no_local_decision_source_synthesis(self): + """`DecisionSource.LOCAL` was how a local allow was labelled. + + A synthesised local allow must not be constructible in + `execute` any more. + """ + assert "DecisionSource.LOCAL" not in self._execute_source(), ( + "`execute` must not synthesise a local allow — that was the " + "inline bypass. Every decision must come from the gateway." + ) + + def test_no_allow_execution_short_circuit(self): + """The bypass returned `allow_execution: True` without a call.""" + src = self._execute_source() + assert '"allow_execution": True' not in src, ( + "`execute` must not return a hard-coded allow_execution=True " + "before consulting the gateway — that was the inline branch" + ) + + def test_inline_is_only_ever_refused_not_honoured(self): + """`inline` may appear, but only in a refusal. + + Parsed rather than grepped, so the comment explaining the + removal cannot satisfy the pin — the self-defeating-pin hazard + this SDK has hit twice now. + """ + tree = ast.parse(_dedent(self._execute_source())) + inline_comparisons = [] + for node in ast.walk(tree): + if isinstance(node, ast.Compare) and "inline" in ast.unparse(node): + inline_comparisons.append((node.lineno, ast.unparse(node))) + assert inline_comparisons, ( + 'the `mode == "inline"` check must still be present — it is ' + "what raises. If it was deleted, callers get a silent " + "behaviour change instead of a named error." + ) + # Every mention must be a comparison (the guard), never an + # assignment or a pass-through into the wire body. + for lineno, snippet in inline_comparisons: + assert snippet.startswith("mode == 'inline'") or snippet.startswith( + 'mode == "inline"' + ), ( + f"line {lineno}: unexpected use of 'inline' — {snippet!r}. " + "It must only be compared against, never assigned or " + "forwarded." + ) + + +def _dedent(src: str) -> str: + import textwrap + + return textwrap.dedent(src) + + +class TestOrphanedRegistryRemoved: + """The strict-mode registry existed only to serve the bypass. + + `register_strict_mode_forced` had zero callers even before this + change (its only documented writer, a `@sensitive` decorator, no + longer exists). `is_strict_mode_forced` was reachable only from + the inline branch. Left in place they read as a live mechanism and + invite someone to wire them back up. + """ + + def test_strict_mode_forced_symbols_are_gone(self): + import nullrun.runtime as runtime_mod + + for name in ( + "register_strict_mode_forced", + "is_strict_mode_forced", + "_STRICT_MODE_FORCED", + ): + assert not hasattr(runtime_mod, name), ( + f"{name} is dead after the inline bypass was closed — it " + "existed only to force strict mode past inline. Dead " + "security machinery is how a bypass gets reintroduced." + ) + + def test_sensitivity_registry_is_untouched(self): + """The per-runtime registry is a separate, still-public surface. + + Whether it should outlive the inline bypass is a separate + decision. This test exists so that removing it later is a + deliberate act rather than an accident of B1's cleanup. + """ + for name in ( + "add_sensitive_tool", + "register_sensitive_tools", + "remove_sensitive_tool", + "is_sensitive_tool", + "get_sensitive_tools", + ): + assert hasattr(NullRunRuntime, name), ( + f"{name} is still part of the documented public surface — " + "B1 removed the inline bypass, not the sensitivity API" + ) + + +class TestPackageHasNoStaleReferences: + def test_no_module_still_honours_inline(self): + """No other module may reintroduce the bypass. + + `transport.py` still accepts and forwards `mode` on the wire + (the backend ignores it). What must not exist anywhere is code + that TREATS inline as a reason to skip the round-trip. + """ + root = pathlib.Path(__file__).resolve().parent.parent + offenders = [] + for path in (root / "src" / "nullrun").rglob("*.py"): + text = path.read_text(encoding="utf-8") + if "inline" not in text: + continue + try: + tree = ast.parse(text) + except SyntaxError: + continue + for node in ast.walk(tree): + if isinstance(node, ast.Compare) and ast.unparse(node).startswith( + 'mode == "inline"' + ): + # The single legitimate site: the refusal in + # runtime.execute. Anything else is a bypass. + if path.name == "runtime.py": + continue + offenders.append(f"{path.name}:{node.lineno}") + assert not offenders, ( + "modules other than runtime.py compare mode to 'inline' — " + "that is a second bypass site:\n " + "\n ".join(offenders) + ) diff --git a/tests/test_langchain_enforcement.py b/tests/test_langchain_enforcement.py new file mode 100644 index 0000000..5e852cd --- /dev/null +++ b/tests/test_langchain_enforcement.py @@ -0,0 +1,372 @@ +"""tests/test_langchain_enforcement.py — a real gate decision must not +degrade into a fail-OPEN transport error on the LangChain callback path. + +Audit 2026-09-30, following DEF-MP-TS12-ENF-01 (RUN_ID 20260929T1338). + +The defect +---------- +``NullRunCallback.on_llm_start`` wrapped ``check_workflow_budget()`` in +``except BaseException`` and logged at ``debug``. That conflated two +categorically different outcomes: + +* **transport failure** — the gate could not be reached. ADR-008's + fail-OPEN policy applies, and is deliberate: a dead backend must not + freeze the agent, and ``/track`` reconciles the cost afterwards. +* **a real gate decision** — budget exhausted, workflow KILL/PAUSE. + The gate was reached and said no. Fail-OPEN does NOT apply here. + +Under the broad catch, an exhausted budget became an invisible debug +line and the LLM call proceeded. + +Why the callback cannot simply re-raise +--------------------------------------- +LangChain **swallows** exceptions raised from a callback handler: it +logs ``Error in callback`` and continues. Verified against +the installed langchain-core, and asserted below so a future langchain +release that changes this is caught rather than silently relied upon. + +So the fix is a handoff: the callback stashes the decision, and the +``@protect`` boundary — which can abort — raises it. This module pins +all three links in that chain: the swallow behaviour, the stash, and +the raise. +""" + +from __future__ import annotations + +import pytest + +from nullrun.breaker.exceptions import ( + NullRunBudgetError, + WorkflowKilledInterrupt, + WorkflowPausedException, +) +from nullrun.instrumentation.langgraph import ( + _ENFORCEMENT_EXCEPTIONS, + drain_deferred_enforcement, + record_deferred_enforcement, +) + +ENFORCEMENT = ( + NullRunBudgetError, + WorkflowKilledInterrupt, + WorkflowPausedException, +) + + +@pytest.fixture(autouse=True) +def _clear_deferred(): + """No test may inherit a stashed decision from another.""" + drain_deferred_enforcement() + yield + drain_deferred_enforcement() + + +class TestFrameworkContract: + def test_langchain_swallows_callback_exceptions(self): + """The premise of the whole fix, pinned against real langchain. + + If a future langchain-core propagates callback exceptions, the + handoff becomes unnecessary (though still correct) — but the + test must fail loudly so someone re-evaluates the design rather + than the assumption silently rotting. + """ + pytest.importorskip("langchain_core") + from langchain_core.callbacks import BaseCallbackHandler + from langchain_core.callbacks.manager import CallbackManager + + class _Boom(BaseCallbackHandler): + def on_llm_start(self, serialized, prompts, **kwargs): + raise RuntimeError("enforcement block") + + swallowed = True + try: + CallbackManager([_Boom()]).on_llm_start({}, ["hi"]) + except RuntimeError: + swallowed = False + assert swallowed, ( + "langchain-core now PROPAGATES exceptions from on_llm_start. " + "The callback handoff in instrumentation/langgraph.py is still " + "correct, but the rationale comment and the ERROR log are now " + "over-cautious — revisit, do not silently drift." + ) + + +def _excepted_types(method) -> list[list[str]]: + """Every `except` clause in `method`, in source order, as names. + + AST rather than substring search on purpose. A text search for + ``except BaseException`` also matches the COMMENT that documents + the pre-fix code — the self-defeating pin this SDK has already + been bitten by once (see `test_langgraph_optional._BLOCK_AND_PROBE`). + Only real handler nodes may satisfy a source pin. + """ + import ast + import inspect + import textwrap + + tree = ast.parse(textwrap.dedent(inspect.getsource(method))) + found: list[list[str]] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.ExceptHandler) or node.type is None: + continue + t = node.type + if isinstance(t, ast.Name): + found.append([t.id]) + elif isinstance(t, ast.Tuple): + found.append([e.id for e in t.elts if isinstance(e, ast.Name)]) + else: + found.append([""]) + return found + + +class TestEnforcementClassification: + @pytest.mark.parametrize("exc_cls", ENFORCEMENT) + def test_real_decisions_are_classified_as_enforcement(self, exc_cls): + assert exc_cls in _ENFORCEMENT_EXCEPTIONS, ( + f"{exc_cls.__name__} is a real gate decision and must be caught " + "by the enforcement arm BEFORE the broad transport arm. If it " + "is not, an exhausted budget / KILL becomes a fail-open " + "transport error." + ) + assert issubclass(exc_cls, Exception), ( + f"{exc_cls.__name__} must subclass Exception — the transport arm " + "catches Exception, so a BaseException-only type would escape " + "the split entirely." + ) + + def test_enforcement_arm_precedes_transport_arm(self): + """The except-ordering is load-bearing, so pin it in the AST. + + `except _ENFORCEMENT_EXCEPTIONS` must precede the broad + `except Exception` in `on_llm_start`. Reversed, the broad arm + wins and every real decision is downgraded to fail-OPEN. + """ + from nullrun.instrumentation.langgraph import NullRunCallback + + arms = _excepted_types(NullRunCallback.on_llm_start) + assert ["_ENFORCEMENT_EXCEPTIONS"] in arms, ( + "the enforcement arm must exist in on_llm_start; got " + f"{arms}" + ) + assert ["Exception"] in arms, ( + f"the transport arm must still exist; got {arms}" + ) + assert arms.index(["_ENFORCEMENT_EXCEPTIONS"]) < arms.index( + ["Exception"] + ), ( + "the enforcement arm must be listed BEFORE the broad transport " + f"arm — Python takes the first matching clause, so reversed " + f"order silently reinstates the defect. Arms: {arms}" + ) + + def test_no_bare_baseexception_catch_remains(self): + """`except BaseException` is what conflated the two categories.""" + from nullrun.instrumentation.langgraph import NullRunCallback + + arms = _excepted_types(NullRunCallback.on_llm_start) + assert ["BaseException"] not in arms, ( + "on_llm_start must not catch BaseException: it conflates a " + f"transport failure (fail-OPEN is correct) with a real gate " + f"decision (fail-OPEN is a bug). Arms: {arms}" + ) + + +class TestDeferredHandoff: + def test_record_then_drain_roundtrip(self): + exc = NullRunBudgetError(workflow_id="w1", reason="budget exhausted") + record_deferred_enforcement(exc) + assert drain_deferred_enforcement() is exc + + def test_drain_clears(self): + """A decision must fire once, not on every subsequent call.""" + record_deferred_enforcement( + NullRunBudgetError(workflow_id="w1", reason="r") + ) + assert drain_deferred_enforcement() is not None + assert drain_deferred_enforcement() is None + + def test_drain_empty_is_none(self): + assert drain_deferred_enforcement() is None + + def test_oldest_decision_wins(self): + """A queue, not a slot — the first block is the one that bit.""" + first = NullRunBudgetError(workflow_id="w1", reason="first") + second = WorkflowKilledInterrupt(workflow_id="w1", reason="killed") + record_deferred_enforcement(first) + record_deferred_enforcement(second) + assert drain_deferred_enforcement() is first + assert drain_deferred_enforcement() is second + + def test_isolated_per_thread(self): + """One chain's block must not abort a concurrent chain. + + LangGraph runs chains in a thread pool; a module-level list + would cross-contaminate them. + """ + import threading + + record_deferred_enforcement( + NullRunBudgetError(workflow_id="w1", reason="main") + ) + seen: list[object] = [] + + def _worker() -> None: + seen.append(drain_deferred_enforcement()) + + t = threading.Thread(target=_worker) + t.start() + t.join() + assert seen == [None], "a worker thread must not see another thread's decision" + # The main thread's decision survived. + assert drain_deferred_enforcement() is not None + + def test_record_never_raises(self): + """A stashing failure must not make enforcement worse.""" + + class _Unhashable: + __hash__ = None # type: ignore[assignment] + + # Any value at all must be accepted without raising. + record_deferred_enforcement(_Unhashable()) # type: ignore[arg-type] + drain_deferred_enforcement() + + +class _StubRuntime: + """A runtime whose every gate passes. + + Patched into ``decorators._get_or_create_runtime`` — the function + ``_protect_body`` actually calls. Patching a name the decorator + never reads (``get_runtime``) would construct a real + ``NullRunRuntime``, which authenticates against the network and + fails with a 401 before the test means anything. + """ + + def _bump_protect_count(self): + return None + + def check_control_plane(self, *a, **k): + return None + + def check_workflow_budget(self, *a, **k): + return None + + def execute(self, *a, **k): + return {"decision": "allow", "decision_source": "gateway"} + + 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 + + +@pytest.fixture +def stub_runtime(monkeypatch): + """Swap the real (network-authenticating) runtime for a passing stub.""" + from nullrun import decorators + + stub = _StubRuntime() + monkeypatch.setattr( + decorators, "_get_or_create_runtime", lambda: stub, raising=True + ) + return stub + + +class TestProtectBoundaryRaises: + def test_protect_raises_deferred_decision(self, stub_runtime): + """End-to-end: a stashed block aborts the protected body. + + This is the link that makes the fix mean anything — without it + the callback's stash is write-only and enforcement is lost + exactly as before. + """ + from nullrun import decorators + + @decorators.protect + def _tool() -> str: + return "BODY_RAN" + + record_deferred_enforcement( + NullRunBudgetError(workflow_id="w-test", reason="budget exhausted") + ) + with pytest.raises(NullRunBudgetError): + _tool() + + def test_no_deferred_decision_lets_body_run(self, stub_runtime): + """The counter-test: the fix must not block when nothing is owed. + + Guards the over-correction — a boundary that raises + unconditionally would freeze every agent, which is a worse + failure than the bug being fixed. + """ + from nullrun import decorators + + @decorators.protect + def _tool() -> str: + return "BODY_RAN" + + assert _tool() == "BODY_RAN" + + def test_decision_is_consumed_not_replayed(self, stub_runtime): + """A block fires once; the next call is not poisoned by it.""" + from nullrun import decorators + + @decorators.protect + def _tool() -> str: + return "BODY_RAN" + + record_deferred_enforcement( + NullRunBudgetError(workflow_id="w-test", reason="budget exhausted") + ) + with pytest.raises(NullRunBudgetError): + _tool() + # The drain cleared it — recovery is the operator's call + # (raise the budget, kill the workflow), not something the + # SDK silently second-guesses. + assert _tool() == "BODY_RAN" + + def test_async_wrapper_also_raises(self, stub_runtime): + """The async path is a separate wrapper — it must drain too. + + A fix that only lands in the sync wrapper leaves every asyncio + agent ungated, which is most real deployments. + """ + import asyncio + + from nullrun import decorators + + @decorators.protect + async def _atool() -> str: + return "BODY_RAN" + + record_deferred_enforcement( + WorkflowKilledInterrupt(workflow_id="w-test", reason="killed") + ) + with pytest.raises(WorkflowKilledInterrupt): + asyncio.run(_atool()) + + def test_helper_is_a_noop_without_the_adapter(self, monkeypatch): + """`_raise_deferred_enforcement` must tolerate a missing adapter. + + `instrumentation.langgraph` is only imported when LangChain is + in play. The helper's import is lazy for exactly that reason, + so a non-LangChain consumer must not hit an ImportError at + every `@protect` call. + """ + import builtins + + from nullrun import decorators + + real_import = builtins.__import__ + + def _blocked(name, *a, **k): + if name == "nullrun.instrumentation.langgraph": + raise ImportError("no langchain here") + return real_import(name, *a, **k) + + monkeypatch.setattr(builtins, "__import__", _blocked) + # Must not raise. + decorators._raise_deferred_enforcement("t") diff --git a/tests/test_langgraph_optional.py b/tests/test_langgraph_optional.py new file mode 100644 index 0000000..243229f --- /dev/null +++ b/tests/test_langgraph_optional.py @@ -0,0 +1,213 @@ +"""tests/test_langgraph_optional.py — langchain-core is an optional extra. + +DEF-MP-TS12-SDK-05 (RUN_ID 20260929T1338, 2026-09-29). + +`langchain-core` is a `dev` extra, not a core dependency — but +``instrumentation/langgraph.py`` imported it unconditionally, and the +chain is: + + NullRunRuntime.__init__ -> instrumentation.auto (make_dedup_state) + -> instrumentation.langgraph -> langchain_core.callbacks + +So a clean ``pip install nullrun`` followed by ``init()`` died with +``ModuleNotFoundError: No module named 'langchain_core'`` for every +consumer who does not use LangChain — i.e. most of them. + +Why this test runs in a SUBPROCESS +---------------------------------- +A ``sys.meta_path`` blocker inserted in-process cannot model a missing +dependency, because ``langchain_core`` is already in ``sys.modules`` by +the time the test runs (pytest imports the SDK). Blocking only the +import path would test nothing. The only faithful simulation of "this +package is not installed" is a fresh interpreter that never had the +chance to import it. + +So each case below spawns ``python -c`` with a meta-path blocker that +raises ModuleNotFoundError for ``langchain_core``, and asserts the +real outcome of ``import nullrun`` / ``NullRunRuntime(...)``. +""" + +from __future__ import annotations + +import subprocess +import sys +import textwrap + +import pytest + +# Blocker + probe, run in a clean interpreter. The probe deliberately +# points at an unroutable URL: pre-fix the run died earlier, at the +# langchain_core import; post-fix it must get PAST the import and fail +# (or succeed) on the network instead. Either way the ModuleNotFoundError +# for langchain_core must not appear. +# +# `_RAISE` is substituted per-case: `ModuleNotFoundError` models the +# package being ABSENT, plain `ImportError` models it being PRESENT BUT +# BROKEN (e.g. a pydantic v1/v2 mismatch inside langchain's own import +# chain). Both must degrade to the `object` fallback. The second is the +# case the original fix missed — it caught only the narrower +# `ModuleNotFoundError`, so a broken-but-installed langchain-core still +# crashed `import nullrun` with the original DEF-MP-TS12-SDK-05 traceback. +_BLOCK_AND_PROBE_TEMPLATE = textwrap.dedent( + """ + import sys + import importlib.abc + import importlib.machinery + + class _BlockLangChainCore(importlib.abc.MetaPathFinder, importlib.abc.Loader): + # The previous version of this fixture used the legacy PEP 302 + # ``find_module`` / ``load_module`` pair, which the modern + # import machinery (3.12+, namespace packages) bypasses — so the + # blocker was never consulted and ``BaseCallbackHandler`` imported + # successfully, masking the regression this test exists to catch. + # ``find_spec`` is the protocol finders MUST implement today. + def find_spec(self, name, path, target=None): + if name == "langchain_core" or name.startswith("langchain_core."): + return importlib.machinery.ModuleSpec(name, self) + def create_module(self, spec): + return None + def exec_module(self, module): + raise {raise_expr} + + sys.meta_path.insert(0, _BlockLangChainCore()) + for _m in [m for m in sys.modules if m.startswith("langchain_core")]: + del sys.modules[_m] + + import nullrun + print("IMPORT_OK") + + from nullrun.instrumentation.langgraph import BaseCallbackHandler + print("FALLBACK:" + BaseCallbackHandler.__name__) + + from nullrun.runtime import NullRunRuntime + try: + NullRunRuntime( + api_key="nr_live_testkey123456", + api_url="https://nullrun.invalid", + polling=False, + ) + print("INIT_OK") + except ModuleNotFoundError as exc: + if "langchain_core" in str(exc): + print("INIT_LANGCHAIN_MISSING:" + str(exc)) + else: + print("INIT_OTHER_MODULENOTFOUND:" + str(exc)) + except Exception as exc: + # Network / auth failure against the unroutable host is the + # EXPECTED outcome: it proves we got past the import. + print("INIT_REACHED_NETWORK:" + type(exc).__name__) + """ +) + +# The package is not installed at all. +_BLOCK_AND_PROBE = _BLOCK_AND_PROBE_TEMPLATE.format( + raise_expr='ModuleNotFoundError("No module named \'%s\'" % module.__name__)' +) + +# The package IS installed but its own import chain is broken. This is +# the pydantic-v1/v2 case, and the reason the guard must catch +# `ImportError` rather than only its `ModuleNotFoundError` subclass. +_BROKEN_AND_PROBE = _BLOCK_AND_PROBE_TEMPLATE.format( + raise_expr='ImportError("cannot import name X from pydantic (v1/v2 mismatch)")' +) + + +def _run_probe(source: str | None = None) -> str: + proc = subprocess.run( + [sys.executable, "-c", source or _BLOCK_AND_PROBE], + capture_output=True, + text=True, + timeout=120, + ) + return proc.stdout + proc.stderr + + +class TestLangGraphOptional: + def test_import_nullrun_without_langchain_core(self): + """`import nullrun` must not require langchain-core.""" + out = _run_probe() + assert "IMPORT_OK" in out, ( + "`import nullrun` failed with langchain-core absent — the SDK " + "is unusable for non-LangChain consumers.\n" + out + ) + + def test_init_without_langchain_core_does_not_crash_on_import(self): + """`init()` must not die on the langchain_core import. + + Pre-fix this printed + ``INIT_LANGCHAIN_MISSING:No module named 'langchain_core'``. + Post-fix it must reach the network layer instead, proving the + import chain is clean. + """ + out = _run_probe() + assert "INIT_LANGCHAIN_MISSING" not in out, ( + "NullRunRuntime.__init__ still requires langchain-core. The import " + "chain runtime -> instrumentation.auto -> instrumentation.langgraph " + "-> langchain_core.callbacks must be optional.\n" + out + ) + assert ("INIT_OK" in out) or ("INIT_REACHED_NETWORK" in out), ( + "init() did not reach the network layer with langchain-core " + "absent — it failed for an unexpected reason.\n" + out + ) + + def test_langchain_present_still_works(self): + """Guard the fix did not break the langchain-installed path. + + Without langchain-core the callback falls back to an `object` + base. This asserts the real base class is still used when the + dependency IS present, so LangChain registration still works. + """ + from nullrun.instrumentation.langgraph import BaseCallbackHandler + + try: + from langchain_core.callbacks import ( + BaseCallbackHandler as RealBase, + ) + except ModuleNotFoundError: + pytest.skip("langchain-core not installed in this environment") + + assert BaseCallbackHandler is RealBase, ( + "with langchain-core installed, NullRunCallback must subclass the " + "real BaseCallbackHandler so LangChain recognises the handler" + ) + + def test_langchain_broken_not_just_absent(self): + """A PRESENT-BUT-BROKEN langchain-core must not crash the import. + + This is the case the original fix missed. It guarded with + `except ModuleNotFoundError`, which covers only "this module does + not exist". A langchain-core that is installed but whose own + dependency chain is broken — the pydantic v1/v2 mismatch being + the common one — raises a plain `ImportError` from *inside* that + chain, which the narrow guard did not catch, so it propagated + out of module scope and `import nullrun` died with the original + DEF-MP-TS12-SDK-05 traceback. + + Every sibling guard in the SDK catches `ImportError`; this site + was the lone outlier. The fallback (`object`) is correct for + every failure mode here, so the fix is behaviour-preserving. + """ + out = _run_probe(_BROKEN_AND_PROBE) + assert "IMPORT_OK" in out, ( + "a broken-but-installed langchain-core must not break " + "`import nullrun` — the guard must catch ImportError, not only " + "ModuleNotFoundError.\n" + out + ) + assert "FALLBACK:object" in out, ( + "with langchain-core broken, NullRunCallback must fall back to " + "an `object` base.\n" + out + ) + + def test_langchain_broken_does_not_break_init(self): + """`init()` must also survive a broken langchain-core. + + Complements the import-level check: the original defect killed + users at `NullRunRuntime(...)`, not at `import nullrun`, so a + test that only asserts the import would miss the regression. + """ + out = _run_probe(_BROKEN_AND_PROBE) + assert ("INIT_OK" in out) or ("INIT_REACHED_NETWORK" in out), ( + "NullRunRuntime.__init__ still fails when langchain-core is " + "installed-but-broken. It must reach the network layer the " + "same way it does when the package is absent.\n" + out + ) diff --git a/tests/test_mcp_adapter.py b/tests/test_mcp_adapter.py index 6194a51..49e622d 100644 --- a/tests/test_mcp_adapter.py +++ b/tests/test_mcp_adapter.py @@ -86,6 +86,37 @@ def _isolate_mcp_context(): _clean_context() +@pytest.fixture(autouse=True) +def _default_gate_runtime(): + """Give every test in this module a working, allow-all gate. + + B2 (2026-09-30): ``MCPAdapter(runtime=None)`` no longer means + "skip the gate" — it means "resolve the global runtime". Without + this fixture, every test here that constructs an adapter without + an explicit ``runtime=`` would try to build a real + ``NullRunRuntime`` from the environment and die on a missing + ``NULLRUN_API_KEY`` — which would say nothing about the behaviour + under test (contextvar stamping, cache refresh, kwarg + pass-through) and everything about the test's environment. + + Tests that care about the gate pass their own ``_StubRuntime`` + explicitly; the explicit argument wins over this one. The B2 + resolution-order tests live in + ``tests/test_mcp_adapter_gate_closed.py``, which deliberately has + no such fixture so it can observe the real resolution. + + ``_StubRuntime`` is resolved at fixture-call time, not import + time — the name is defined ~450 lines below this point. + """ + from nullrun._registry import get_registry + + registry = get_registry() + previous = registry.get() + registry.set(_StubRuntime()) + yield + registry.set(previous) if previous is not None else registry.clear() + + # A representative `github`-shaped inventory ----------------------------- @@ -524,14 +555,18 @@ def test_call_tool_idempotent_under_repeated_invocations(): tool-block / budget / approval policies did NOT apply to MCP invocations, only to local functions. -These tests pin the post-v3.53 behavior: when an MCPAdapter is -constructed with ``runtime=`` set, ``call_tool`` invokes +These tests pin the post-v3.53 behavior: ``call_tool`` invokes ``runtime.execute(...)`` synchronously BEFORE the MCP client. ``decision="block"`` raises ``NullRunBlockedException`` and the MCP client is NOT called. ``decision="require_approval"`` raises -``NullRunBlockedException`` with the approval_id attached. The -legacy contextvar-only path stays reachable for back-compat -when ``runtime`` is not provided. +``NullRunBlockedException`` with the approval_id attached. + +B2 (2026-09-30): this is no longer conditional on ``runtime=`` +being passed. The post-v3.53 shape kept a "legacy contextvar-only +path" for adapters constructed without one — the remaining half of +the hole v3.53 opened. ``runtime=None`` now means "resolve the +global runtime"; the autouse ``_default_gate_runtime`` fixture +supplies one for this module. """ @@ -661,21 +696,31 @@ def test_call_tool_with_runtime_require_approval_raises_with_approval_id(): assert excinfo.value.tool_name == "delete_repo" -def test_call_tool_without_runtime_uses_legacy_contextvar_path(): - """v3.53 audit #5 — back-compat: callers that omit ``runtime=`` - get the legacy contextvar-only path. No /api/v1/execute call - is made; the next ``@protect``-wrapped function picks up the - contextvar on its next ``/check`` request. - - Pins that introducing the runtime parameter did not break - existing integrations that rely on the contextvar pattern. +def test_call_tool_without_runtime_still_consults_the_gate(): + """B2 (2026-09-30) — ``runtime=None`` means "resolve the global + runtime", not "skip the gate". + + This test previously asserted the opposite: that an adapter + constructed without ``runtime=`` called the MCP client directly, + with no /api/v1/execute round-trip at all. That was the bypass. + It was introduced as a deliberate back-compat accommodation + ("callers who omit runtime= get the legacy contextvar-only + path") — and it is exactly the hole: the operator's tool-block, + budget and approval policies applied to a locally-declared + function but not to a remote MCP call, on the same agent, in the + same loop. + + Resolution-order tests are in + ``tests/test_mcp_adapter_gate_closed.py``; this one only pins + that the default-constructed adapter is gated and that the + contextvar stamping survived. """ client = _MockMcpClient(_github_inventory()) adapter = MCPAdapter(server_name="github", mcp_client=client) - # No runtime was passed. The MCP client is called directly. adapter.call_tool("get_file_contents", {"path": "README.md"}) assert client.calls == [("get_file_contents", {"path": "README.md"})] - # The contextvar was still stamped — legacy behavior preserved. + # The contextvar stamping is unchanged — it is the /execute + # round-trip that is now unconditional. assert get_call_mcp_class() == "mcp" ann = get_call_mcp_annotations() assert ann["read_only"] is True diff --git a/tests/test_mcp_adapter_gate_closed.py b/tests/test_mcp_adapter_gate_closed.py new file mode 100644 index 0000000..f55e8b7 --- /dev/null +++ b/tests/test_mcp_adapter_gate_closed.py @@ -0,0 +1,352 @@ +"""tests/test_mcp_adapter_gate_closed.py — MCPAdapter is never ungated. + +B2, 2026-09-30 (ADR-061). Continues the cluster started by +DEF-MP-TS12-ENF-01 (QA cycle RUN_ID 20260929T1338). + +The bypass +---------- +v3.53 added ``runtime.execute(...)`` to ``MCPAdapter.call_tool``, but +made it conditional:: + + if self._runtime is not None: + execute_result = self._runtime.execute(...) + # ... otherwise: call the MCP server directly + +``runtime`` defaulted to ``None``, so a default-constructed adapter +called the MCP server with no gate at all. The only thing the +operator got was a contextvar (``set_mcp_tool_context``) that a +*later* ``@protect`` wrapper might read on its *next* ``/check`` — +post-hoc annotation, not enforcement. The module's own documented +example took exactly that path:: + + adapter = MCPAdapter(server_name="github", mcp_client=conn) + result = adapter.call_tool("create_issue", {"repo": "acme/api"}) + +So the operator's ``mcp_destructive_policy`` / +``mcp_readonly_policy`` applied to a locally-declared function but +not to a remote MCP call — same agent, same loop, different +enforcement. And nothing in the return value, the log, or the audit +trail distinguished the two. + +The fix makes ``runtime=None`` mean "resolve the global runtime", +resolved on the same terms ``@protect`` resolves it. Resolution is +lazy (at ``call_tool``, not at construction) so the adapter stays +constructible in fixtures and doc snippets without ``nullrun.init()`` +— the original and legitimate reason for the decoupling. + +This module deliberately has NO autouse gate-runtime fixture, so it +observes the real resolution order. +""" + +from __future__ import annotations + +import ast +import inspect +import pathlib +from typing import Any + +import pytest + +from nullrun._registry import get_registry +from nullrun.breaker.exceptions import ( + NullRunAuthenticationError, + NullRunBlockedException, +) +from nullrun.toolbox.mcp import MCPAdapter + +# --------------------------------------------------------------------------- +# Doubles +# --------------------------------------------------------------------------- + + +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: + 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}" + + +class _RecordingRuntime: + """Allow-all runtime that records what it was asked.""" + + def __init__(self, payload=None): + self.calls: list[dict[str, Any]] = [] + self._payload = payload or { + "decision": "allow", + "decision_source": "gateway", + "explanation": "allow", + } + + def execute(self, **kwargs): + self.calls.append(kwargs) + return dict(self._payload) + + +@pytest.fixture(autouse=True) +def _clean_registry(): + """No runtime bound, and no API key in the environment. + + Both matter: a bound runtime would make the resolution-order + assertions pass for the wrong reason, and an ambient + ``NULLRUN_API_KEY`` would make the fail-loud assertion pass for + the wrong reason. + """ + import os + + registry = get_registry() + previous = registry.get() + registry.clear() + had_key = "NULLRUN_API_KEY" in os.environ + old_key = os.environ.pop("NULLRUN_API_KEY", None) + try: + yield + finally: + if had_key and old_key is not None: + os.environ["NULLRUN_API_KEY"] = old_key + if previous is not None: + registry.set(previous) + else: + registry.clear() + + +def _inventory(): + return [ + _Tool( + "create_issue", + _Ann(read=False, destructive=True, open_world=True), + ), + _Tool("get_file_contents", _Ann(read=True, destructive=False)), + ] + + +def _adapter(**kw) -> tuple[MCPAdapter, _MockMcpClient]: + client = _MockMcpClient(_inventory()) + return MCPAdapter(server_name="github", mcp_client=client, **kw), client + + +# --------------------------------------------------------------------------- +# The bypass +# --------------------------------------------------------------------------- + + +class TestDefaultAdapterIsGated: + def test_call_tool_consults_the_gate_by_default(self): + """The core of B2: no ``runtime=`` no longer means no gate.""" + runtime = _RecordingRuntime() + get_registry().set(runtime) + adapter, client = _adapter() + adapter.call_tool("get_file_contents", {"path": "README.md"}) + assert len(runtime.calls) == 1, ( + "a default-constructed MCPAdapter called the MCP server " + "without consulting /execute — that is the bypass B2 closes" + ) + assert runtime.calls[0]["tool_name"] == "get_file_contents" + assert runtime.calls[0]["input_data"] == {"path": "README.md"} + + def test_gate_runs_before_the_mcp_client(self): + """Order is the whole point — a gate consulted afterwards is + a receipt, not a gate.""" + order: list[str] = [] + + class _OrderRuntime(_RecordingRuntime): + def execute(self, **kw): + order.append("gate") + return super().execute(**kw) + + get_registry().set(_OrderRuntime()) + + client = _MockMcpClient(_inventory()) + original = client.call_tool + + def _tracked(name, arguments=None, **kw): + order.append("mcp") + return original(name, arguments, **kw) + + client.call_tool = _tracked + MCPAdapter(server_name="github", mcp_client=client).call_tool("get_file_contents", {}) + assert order == ["gate", "mcp"], f"/execute must precede the MCP call; got {order}" + + def test_explicit_runtime_beats_the_registry(self): + """A caller who passes ``runtime=`` gets that one.""" + registry_runtime = _RecordingRuntime() + explicit = _RecordingRuntime() + get_registry().set(registry_runtime) + adapter, _ = _adapter(runtime=explicit) + adapter.call_tool("get_file_contents", {}) + assert len(explicit.calls) == 1 + assert registry_runtime.calls == [], "an explicit runtime= must win over the registry" + + +class TestFailLoudNotFailOpen: + def test_missing_api_key_raises_instead_of_calling_mcp(self): + """No API key is a configuration error, not permission to + run ungated. + + This is the invariant ``@protect`` already holds (see + ``decorators._get_or_create_runtime``: "a missing API key must + be a hard error not a silent allow-all"). MCPAdapter must not + be a quieter door into the same state. + """ + adapter, client = _adapter() + with pytest.raises(NullRunAuthenticationError): + adapter.call_tool("get_file_contents", {}) + assert client.calls == [], "the MCP server was called with no gate and no API key" + + def test_the_error_says_how_to_fix_it(self): + adapter, _ = _adapter() + with pytest.raises(NullRunAuthenticationError) as exc: + adapter.call_tool("get_file_contents", {}) + assert "API_KEY" in str(exc.value), "the error must name the missing setting, not just fail" + + +class TestBlockAndApprovalStillShortCircuit: + def test_block_never_reaches_the_mcp_server(self): + runtime = _RecordingRuntime( + { + "decision": "block", + "decision_source": "gateway", + "explanation": "destructive tool blocked", + "workflow_id": "wf-1", + } + ) + get_registry().set(runtime) + adapter, client = _adapter() + with pytest.raises(NullRunBlockedException): + adapter.call_tool("create_issue", {"repo": "acme/api"}) + assert client.calls == [], "a blocked MCP call reached the server" + + def test_require_approval_never_reaches_the_mcp_server(self): + runtime = _RecordingRuntime( + { + "decision": "require_approval", + "decision_source": "gateway", + "explanation": "needs approval", + "workflow_id": "wf-1", + "approval_id": "apr-1", + } + ) + get_registry().set(runtime) + adapter, client = _adapter() + with pytest.raises(NullRunBlockedException) as exc: + adapter.call_tool("create_issue", {"repo": "acme/api"}) + assert client.calls == [] + # NR-A001 is the "approval exists, route the user through the + # flow" code; NR-A010 is the "no approval row" variant. + assert exc.value.error_code == "NR-A001" + + +class TestContextvarStampingSurvived: + def test_annotations_are_still_stamped(self): + """B2 changed when the gate runs, not what is forwarded. + + ``set_mcp_tool_context`` is what makes the v3.31 umbrella + policies (``mcp_destructive_policy``) fire at all — no SDK on + the planet calls it otherwise. Dropping it while closing the + bypass would have traded one hole for another. + """ + from nullrun.context import get_call_mcp_annotations, get_call_mcp_class + + get_registry().set(_RecordingRuntime()) + adapter, _ = _adapter() + adapter.call_tool("get_file_contents", {"path": "README.md"}) + assert get_call_mcp_class() == "mcp" + ann = get_call_mcp_annotations() + assert ann["read_only"] is True + assert ann["destructive"] is False + + def test_unknown_tool_stamps_invalid_and_still_gates(self): + from nullrun.context import get_call_mcp_class + + runtime = _RecordingRuntime() + get_registry().set(runtime) + adapter, _ = _adapter() + with pytest.raises(KeyError): + adapter.call_tool("phantom_tool", {}) + assert get_call_mcp_class() == "invalid" + assert len(runtime.calls) == 1, ( + "an unknown tool must still be gated before the server's " + "KeyError — a permissive server could otherwise invent " + "tool names that skip the cache" + ) + + +class TestNoConditionalGateInTheCode: + """The bypass must be gone from the CODE, not merely unreachable.""" + + def test_call_tool_has_no_conditional_runtime_gate(self): + """Parsed, not grepped — the comment explaining the removal + must not be able to satisfy the pin. + """ + src = inspect.getsource(MCPAdapter.call_tool) + tree = ast.parse(inspect.cleandoc(src)) + for node in ast.walk(tree): + if isinstance(node, ast.Compare) and "runtime" in ast.unparse(node): + pytest.fail( + f"call_tool line {node.lineno} still branches on the " + f"runtime: {ast.unparse(node)!r}. The gate is " + "unconditional — a conditional is the bypass." + ) + + def test_legacy_wording_is_gone_from_the_module(self): + """No docstring may still advertise the ungated path. + + A stale docstring is how a bypass gets reintroduced: someone + reads the docs, believes ``runtime=`` is optional, and wires + around the gate. + """ + path = pathlib.Path(inspect.getfile(MCPAdapter)) + tree = ast.parse(path.read_text(encoding="utf-8")) + offenders = [] + for node in ast.walk(tree): + if not isinstance(node, ast.Constant) or not isinstance(node.value, str): + continue + text = node.value + if "contextvar-only path" in text or "legacy contextvar" in text: + offenders.append(f"line {node.lineno}") + assert not offenders, ( + "mcp.py still documents an ungated contextvar-only path:\n " + "\n ".join(offenders) + ) + + def test_no_module_offers_an_ungated_adapter_constructor(self): + """No second, ungated entry point elsewhere in the toolbox.""" + root = pathlib.Path(__file__).resolve().parent.parent + toolbox = root / "src" / "nullrun" / "toolbox" + offenders = [] + for path in toolbox.rglob("*.py"): + text = path.read_text(encoding="utf-8") + if "MCPAdapter" not in text: + continue + # A module that constructs an adapter and then calls + # call_tool without a runtime, outside a test. + for node in ast.walk(ast.parse(text)): + if ( + isinstance(node, ast.Call) + and ast.unparse(node.func).endswith("MCPAdapter") + and not any(kw.arg == "runtime" for kw in node.keywords) + and "test" not in str(path).lower() + ): + offenders.append(f"{path.name}:{node.lineno}") + assert not offenders, "a module constructs MCPAdapter without runtime=:\n " + "\n ".join( + offenders + ) diff --git a/tests/test_s008_resign_per_retry.py b/tests/test_s008_resign_per_retry.py new file mode 100644 index 0000000..9bf2878 --- /dev/null +++ b/tests/test_s008_resign_per_retry.py @@ -0,0 +1,144 @@ +"""tests/test_s008_resign_per_retry.py — S008 replay-guard regression. + +DEF-MP-TS12-ENF-01 (RUN_ID 20260929T1338, 2026-09-29). + +The backend's S008 replay guard stores ``hmac:replay:{key_fp}:{sig_hash}`` +on first sight of a signature and rejects every repeat as +``HMAC_REPLAY`` (fail-CLOSED). The SDK built its signed headers ONCE, +outside the retry closure, on the three paths that retry: + + * ``Transport.check`` (/gate, 3 retries) + * ``Transport.execute`` (/execute, 10 retries) + * ``_send_batch_with_retry_info`` (/track/batch, 10 retries) + +Attempt 1 registered the signature; every retry replayed it +byte-for-byte and was rejected. One transient 5xx therefore consumed +the whole retry budget on replay rejections, and the resulting 401 was +indistinguishable from a genuinely invalid API key — which is how the +TS-12 cycle logged ``HMAC_REPLAY x684`` in production. + +The fix signs inside the retry closure. These tests assert the +DISTINCTNESS of the signature across attempts, not merely that a +signature is present: a test that only checked "the request was +signed" would pass against the broken code too. +""" + +from __future__ import annotations + +import time + +import httpx +import pytest +import respx + +from nullrun.transport import Transport + + +@pytest.fixture +def signed_transport(): + t = Transport( + api_url="https://api.test.nullrun.io", + api_key="test-key-12345678", + secret_key="test-secret-abcdefgh", + ) + yield t + t.stop() + + +@pytest.fixture +def ticking_clock(monkeypatch): + """Make `time.time()` advance on every call. + + ``_build_signed_headers`` signs with ``int(time.time())`` — a + second-resolution clock, which is exactly why a retry inside the + same second reproduces the previous signature. The real retry path + sleeps between attempts; rather than a wall-clock wait (the suite + caps sleeps), we advance the clock per call so successive + signatures necessarily differ. If the SDK signs once outside the + closure, this fixture makes no difference and the signature is + still identical — which is what the assertions below catch. + """ + real = time.time + state = {"n": 0} + + def _fake() -> float: + state["n"] += 1 + return real() + state["n"] + + monkeypatch.setattr("nullrun.transport.time.time", _fake, raising=True) + return state + + +def _signatures(route) -> list[str | None]: + return [call.request.headers.get("X-Signature") for call in route.calls] + + +class TestS008ResignPerRetry: + @respx.mock + def test_gate_retry_signs_each_attempt_freshly(self, signed_transport, ticking_clock): + route = respx.post("https://api.test.nullrun.io/api/v1/gate").mock( + side_effect=[ + httpx.Response(503, json={"error_code": "REDIS_UNAVAILABLE"}), + httpx.Response(200, json={"decision": "allow"}), + ] + ) + result = signed_transport.check({"workflow_id": "wf-s008-test"}) + + assert route.call_count == 2, "the 503 should have triggered exactly one retry" + assert result.get("decision") == "allow" + sigs = _signatures(route) + assert sigs[0] and sigs[1], "both attempts must be signed" + assert sigs[0] != sigs[1], ( + "S008 regression: both /gate attempts carried the SAME X-Signature. " + "The backend replay guard rejects the retry as HMAC_REPLAY, turning " + "one transient 5xx into a credential error." + ) + + @respx.mock + def test_execute_retry_signs_each_attempt_freshly(self, signed_transport, ticking_clock): + signed_transport._execute_max_retries = 2 + route = respx.post("https://api.test.nullrun.io/api/v1/execute").mock( + side_effect=[ + httpx.Response(500, json={"error_code": "INTERNAL"}), + httpx.Response(200, json={"decision": "allow"}), + ] + ) + signed_transport.execute( + organization_id="org-s008-test", + execution_id="exec-s008-test", + trace_id="trace-s008-test", + tool="search", + input_data={"q": "s008"}, + ) + + assert route.call_count == 2 + sigs = _signatures(route) + assert sigs[0] and sigs[1] + assert sigs[0] != sigs[1], "S008 regression: /execute replayed one signature" + + @respx.mock + def test_track_batch_retry_signs_each_attempt_freshly(self, signed_transport, ticking_clock): + signed_transport._track_max_retries = 2 + route = respx.post("https://api.test.nullrun.io/api/v1/track/batch").mock( + side_effect=[ + httpx.Response(500, json={"error_code": "INTERNAL"}), + httpx.Response(200, json={"ok": True}), + ] + ) + signed_transport._send_batch_with_retry_info([{"event": "test"}]) + + assert route.call_count == 2 + sigs = _signatures(route) + assert sigs[0] and sigs[1] + assert sigs[0] != sigs[1], "S008 regression: /track/batch replayed one signature" + + def test_signing_not_dropped_anywhere(self, signed_transport): + """Constraint: only the three retrying sites move. + + Signing must still happen everywhere — the fix relocates the + call, it does not remove it. + """ + headers = signed_transport._build_signed_headers(body='{"a":1}') + assert headers.get("X-Signature") + assert headers.get("X-Signature-Timestamp") + assert headers.get("X-NULLRUN-PROTOCOL") diff --git a/tests/test_unresolved_workflow_observability.py b/tests/test_unresolved_workflow_observability.py new file mode 100644 index 0000000..11dfeaa --- /dev/null +++ b/tests/test_unresolved_workflow_observability.py @@ -0,0 +1,185 @@ +"""tests/test_unresolved_workflow_observability.py — a skipped gate must +be distinguishable from a passing one. + +Audit 2026-09-30, findings F5/F6 (follow-up to RUN_ID 20260929T1338). + +The defect +---------- +Two pre-flight gates no-op when no workflow can be resolved: + + * ``check_control_plane`` — the kill/pause gate + * ``check_workflow_budget`` — the budget pre-flight + +Both are CORRECT to no-op. ``_resolve_workflow_id`` returns None only +for an API key that was never workflow-bound, and a never-bound key +legitimately has no control-plane state and no per-workflow budget. +Raising would break that documented configuration. + +What was wrong is that the no-op was completely silent. Consider a +key whose 1:1 workflow binding is lost — a bad migration, a restored +backup, the wrong key. Both gates stop running, and the only symptom +is an agent that ignores the dashboard and spends without a budget, +with nothing in the logs, nothing in metrics, and no error. A working +deployment and a silently-ungated one look identical. + +The fix keeps the behaviour and makes the skip countable. These tests +pin that, and — more importantly — pin that the skip does NOT become +an exception, because "observable" must not quietly turn into +"breaks the never-bound-key case". +""" + +from __future__ import annotations + +import pytest + +from nullrun import runtime as rt +from nullrun.breaker.exceptions import TransportErrorSource # noqa: F401 + + +class _RecordingMetrics: + """Stands in for the metrics module, recording every counter name.""" + + def __init__(self, real): + self._real = real + self.calls: list[str] = [] + + def inc_runtime(self, name, *a, **k): + self.calls.append(name) + return self._real.inc_runtime(name, *a, **k) + + def __getattr__(self, item): + return getattr(self._real, item) + + +@pytest.fixture +def counted(monkeypatch): + """Count every runtime metric without discarding the real ones.""" + rec = _RecordingMetrics(rt.metrics) + monkeypatch.setattr(rt, "metrics", rec) + return rec + + +@pytest.fixture +def unbound_runtime(monkeypatch): + """A runtime whose workflow can never resolve. + + Only `_resolve_workflow_id` is stubbed. Every other attribute is + the real implementation, so the test exercises the real control + flow up to the skip rather than a hand-built mock path. + """ + stub = object.__new__(rt.NullRunRuntime) + monkeypatch.setattr( + rt.NullRunRuntime, "_resolve_workflow_id", lambda self, *a, **k: None + ) + return stub + + +class TestControlPlaneSkip: + def test_skip_is_counted(self, unbound_runtime, counted): + unbound_runtime.check_control_plane(None) + assert "control_plane_no_workflow_total" in counted.calls, ( + "a skipped kill/pause gate must be countable, or a lost key " + "binding is indistinguishable from normal operation" + ) + + def test_skip_does_not_raise(self, unbound_runtime): + """Observable must not become fatal. + + A never-bound API key is a supported configuration. If this + raises, the observability fix has broken it. + """ + unbound_runtime.check_control_plane(None) # must not raise + + def test_skip_does_not_poll_the_network(self, unbound_runtime, monkeypatch): + """The no-op must stay a no-op, not a request with no consumer.""" + + def _boom(*a, **k): + raise AssertionError("control plane polled with no workflow") + + monkeypatch.setattr( + rt.NullRunRuntime, "_fetch_remote_state", _boom, raising=True + ) + unbound_runtime.check_control_plane(None) + + +class TestBudgetPreflightSkip: + def test_skip_is_counted(self, unbound_runtime, counted): + unbound_runtime.check_workflow_budget() + assert "budget_preflight_no_workflow_total" in counted.calls, ( + "a skipped budget pre-flight must be countable" + ) + + def test_skip_does_not_raise(self, unbound_runtime): + unbound_runtime.check_workflow_budget() # must not raise + + def test_entered_and_skipped_are_distinguishable( + self, unbound_runtime, counted + ): + """The operator needs to tell 'ran and allowed' from 'never ran'. + + `check_calls` is bumped on entry to the pre-flight, and the new + counter is bumped only when it is skipped. Both present = + "the gate was reached but had no workflow"; only `check_calls` + = "the gate ran". + """ + unbound_runtime.check_workflow_budget() + assert "check_calls" in counted.calls + assert "budget_preflight_no_workflow_total" in counted.calls + + +class TestMetricsNeverGate: + def test_new_counter_failure_does_not_break_the_skip( + self, unbound_runtime, monkeypatch + ): + """A failure while recording the skip must not propagate. + + The skip is already a no-op; raising while counting it would + convert "no workflow bound" into "your agent is broken", which + is strictly worse than what it replaced. This is why the new + counters are wrapped, matching the `skip_budget_*` counters + directly above them in the same function. + + Scoped to the two counters this change adds. The pre-existing + `check_calls` increment earlier in `check_workflow_budget` is + unguarded, but it is not this change's to assert about, and + `inc_runtime` is an in-memory increment under a lock that has + no realistic failure mode in the first place. + """ + new_counters = { + "control_plane_no_workflow_total", + "budget_preflight_no_workflow_total", + } + + class _PartiallyBrokenMetrics: + def inc_runtime(self, name, *a, **k): + if name in new_counters: + raise RuntimeError("counter write failed") + return None + + def __getattr__(self, item): + return lambda *a, **k: None + + monkeypatch.setattr(rt, "metrics", _PartiallyBrokenMetrics()) + unbound_runtime.check_control_plane(None) # must not raise + unbound_runtime.check_workflow_budget() # must not raise + + +class TestNoOverCorrection: + def test_observable_skip_does_not_warn_per_call(self, unbound_runtime, caplog): + """Debug level, not warning. + + For a legitimately never-bound key this branch runs on EVERY + protected call. Logging it at warning would turn a supported + configuration into a wall of noise, which is how real signals + get ignored. + """ + import logging + + with caplog.at_level(logging.DEBUG, logger="nullrun.runtime"): + unbound_runtime.check_control_plane(None) + records = [r for r in caplog.records if r.name == "nullrun.runtime"] + assert records, "the skip should still be debug-logged" + assert all(r.levelno == logging.DEBUG for r in records), ( + "the unresolved-workflow skip must not log above DEBUG — it " + "fires on every call for a legitimately unbound key" + ) diff --git a/uv.lock b/uv.lock index c748813..c81ba7a 100644 --- a/uv.lock +++ b/uv.lock @@ -636,7 +636,7 @@ wheels = [ [[package]] name = "nullrun" -version = "0.18.5" +version = "0.19.0" source = { editable = "." } dependencies = [ { name = "httpx" },