From 9ab523b90b4dc86473e49d39a4eae75f9d473693 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Tue, 29 Sep 2026 23:05:42 +0400 Subject: [PATCH 01/14] fix(sdk): re-sign requests on every retry attempt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DEF-MP-TS12-ENF-01 (RUN_ID 20260929T1338) — S008 self-inflicted replay. 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 all 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 entire retry budget on replay rejections, and the resulting 401 was indistinguishable from a genuinely invalid API key. That is the HMAC_REPLAY x684 the TS-12 cycle observed in production. This is an INDEPENDENT defect, not a downstream symptom of the gate's 500: _retry_with_backoff re-raises the 401 immediately, so it never reaches the 4xx-block branch. Fixing the status mapping alone would have left the SDK unable to survive any genuine 5xx on /gate. _build_signed_headers recomputes int(time.time()) and the HMAC on every call, so moving the call inside the closure gives each attempt a distinct sig_hash. The backend explicitly deferred this fix pending the SDK change ("the Python SDK builds its signed headers ONCE outside the retry closure ... Tracked as S008 v2", hmac_verify.rs). A true single-use X-Nonce remains a protocol change and is still out of scope. Only the three retrying sites move; the eight non-retrying call sites are untouched. Tests assert signature DISTINCTNESS across attempts, not merely presence — a presence-only check passes against the broken code. Verified the gate test fails against a reverted implementation (identical sig_hash on both attempts) before restoring the fix. --- src/nullrun/transport.py | 37 +++++-- tests/test_s008_resign_per_retry.py | 144 ++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+), 6 deletions(-) create mode 100644 tests/test_s008_resign_per_retry.py diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index 401b031..129d652 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -978,8 +978,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 +990,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 +1193,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 +1532,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 +1554,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, ) 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") From ca03e9c00acd61e4838e75e714bbc94ce28db1c1 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Tue, 29 Sep 2026 23:08:51 +0400 Subject: [PATCH 02/14] fix(sdk): propagate authentication failures from gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DEF-MP-TS12-ENF-01 (RUN_ID 20260929T1338) — the "gate answers allowed on blocked calls" Blocker. check_workflow_budget re-read a 401 as permission to proceed, by two independent routes: 1. _retry_with_backoff raises NullRunAuthError on any 401 and re-raises it WITHOUT retrying. Both except arms (the cached chain branch and the non-cached branch) swallowed it and returned None, which the caller reads as "no block". The agent then executed a call the backend had refused. 2. TransportErrorSource.AUTH_ERROR was in the synthetic fail-OPEN set, so a 401 arriving as a returned response rather than a raise was reclassified as a transport error and allowed through. Classification is by TYPE (NullRunAuthenticationError), never by inspecting the message. Both arms are covered — pre-fix they swallowed independently, so fixing only the non-cached path would have left the chain-mode hole open. ADR-008 is PRESERVED, not amended. The authoritative table documents check_workflow_budget as fail-OPEN on transport error (network timeout, 5xx, breaker open) with post-hoc correction in /track, and its documented transport classification is exactly FALLBACK_NETWORK_ERROR / FALLBACK_GATEWAY_ERROR / FALLBACK_BREAKER_OPEN — auth was never in it. This change therefore brings the code back in line with the documented policy rather than deviating from it, and narrows the fail-OPEN set rather than widening enforcement. A 401 is a credential/config failure, not a transient condition: no retry fixes a revoked key, and no post-hoc correction in /track can retroactively authorise a call the backend refused. The ADR-008 table in the module docstring is updated in lockstep, as that docstring itself requires. No README claim needed changing — the README does not document auth as fail-open. Tests pin the three-branch contract: transport failure still fails OPEN (a dead backend must not freeze the agent — this is the counter-test that catches an over-broad fix), 401 raises, and an enforcement 4xx still raises NullRunBudgetError. Verified both 401 tests fail against the pre-fix implementation, with the captured log showing the defect verbatim: "Invalid API key" -> "failing open". --- src/nullrun/runtime.py | 40 +++++++++++- tests/test_auth_fail_closed.py | 113 +++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 tests/test_auth_fail_closed.py diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index 42773db..4fd3ba5 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 @@ -2077,6 +2085,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 +2112,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") @@ -2107,7 +2134,16 @@ def check_workflow_budget(self) -> None: TransportErrorSource.NETWORK_ERROR, TransportErrorSource.GATEWAY_ERROR, TransportErrorSource.BREAKER_OPEN, - TransportErrorSource.AUTH_ERROR, + # DEF-MP-TS12-ENF-01: `AUTH_ERROR` removed. This set is the + # ADR-008 transport-failure classification, documented as + # exactly `FALLBACK_NETWORK_ERROR` / `FALLBACK_GATEWAY_ERROR` + # / `FALLBACK_BREAKER_OPEN` (module docstring, "Fail-OPEN + # policy" section) — auth was never part of it. Including it + # meant a credential failure was reinterpreted as "transport + # error" and the call was allowed. Removing it brings the + # code back in line with the documented policy rather than + # deviating from it; a 401 now reaches the `decision == + # "block"` arm and raises. }: logger.warning( f"check_workflow_budget: synthetic decision_source=" 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() From a626bbdeea503b82b00b3927fc4d4293b7d7dddc Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 08:43:29 +0400 Subject: [PATCH 03/14] fix(sdk): make langgraph instrumentation optional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DEF-MP-TS12-SDK-05 (RUN_ID 20260929T1338) — clean install was unusable. `langchain-core` is a `dev` extra, not a core dependency (pyproject.toml core deps are httpx only), but instrumentation/langgraph.py imported it unconditionally. 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'` — the SDK was unusable for every consumer who does not use LangChain. The import is now guarded, falling back to an `object` base. `NullRunCallback` calls no `super()` and defines every method it needs, so it is fully functional when langchain-core IS installed; when absent the class still imports and the LangGraph-specific paths raise at 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, which is the opposite of the intent. Tests model genuine dependency ABSENCE by spawning a clean interpreter with a sys.meta_path blocker. An in-process blocker would be meaningless here: langchain_core is already in sys.modules by the time pytest runs, so blocking only the import path would prove nothing. The subprocess probe reproduces the reported symptom exactly — pre-fix it prints INIT_LANGCHAIN_MISSING, post-fix init() reaches the network layer. A third test guards that the real BaseCallbackHandler is still used when the dependency is present, so the fallback cannot silently become permanent. --- src/nullrun/instrumentation/langgraph.py | 30 ++++- tests/test_langgraph_optional.py | 139 +++++++++++++++++++++++ 2 files changed, 167 insertions(+), 2 deletions(-) create mode 100644 tests/test_langgraph_optional.py diff --git a/src/nullrun/instrumentation/langgraph.py b/src/nullrun/instrumentation/langgraph.py index cbeca53..92a6caa 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,34 @@ get_current_span, ) +# 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. +try: # pragma: no cover - exercised by the absence test + from langchain_core.callbacks import BaseCallbackHandler +except ModuleNotFoundError: # pragma: no cover + BaseCallbackHandler = object # type: ignore[assignment, misc] + logger = logging.getLogger(__name__) # S-9: FIFO cap on NullRunCallback._active_runs. diff --git a/tests/test_langgraph_optional.py b/tests/test_langgraph_optional.py new file mode 100644 index 0000000..910fce0 --- /dev/null +++ b/tests/test_langgraph_optional.py @@ -0,0 +1,139 @@ +"""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. +_BLOCK_AND_PROBE = textwrap.dedent( + """ + import sys + + class _BlockLangChainCore: + def find_module(self, name, path=None): + if name == "langchain_core" or name.startswith("langchain_core."): + return self + def load_module(self, name): + raise ModuleNotFoundError("No module named '%s'" % name) + + 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.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__) + """ +) + + +def _run_probe() -> str: + proc = subprocess.run( + [sys.executable, "-c", _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" + ) From 5a81232a4c7a9626c25193946251cc160ec8e80b Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 10:02:31 +0400 Subject: [PATCH 04/14] fix(sdk): catch ImportError, not just ModuleNotFoundError, for langchain-core MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to a626bbd (DEF-MP-TS12-SDK-05). That fix guarded the optional `langchain_core` import 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 case — raises a plain `ImportError` from inside that chain. The narrow guard did not catch it, so it propagated out of module scope and `import nullrun` died with the original crash. The fix protected only users with langchain-core absent and left users with it broken still crashing at init(). Every other guard in the SDK catches `ImportError` (transport.py, transport_websocket.py, auto.py, autogen.py, crewai.py, llama_index.py, auto_requests.py); this site was the lone outlier. `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. Tests model both cases in subprocesses with a meta_path blocker: package absent (the original regression) and package present-but-broken (the one that was still open). The absence case is not sufficient on its own; a test that only covers it would have passed against the broken code. --- src/nullrun/instrumentation/langgraph.py | 21 ++++++- tests/test_langgraph_optional.py | 72 ++++++++++++++++++++++-- 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/src/nullrun/instrumentation/langgraph.py b/src/nullrun/instrumentation/langgraph.py index 92a6caa..8c652ea 100644 --- a/src/nullrun/instrumentation/langgraph.py +++ b/src/nullrun/instrumentation/langgraph.py @@ -60,9 +60,28 @@ # # `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 ModuleNotFoundError: # pragma: no cover +except ImportError: # pragma: no cover BaseCallbackHandler = object # type: ignore[assignment, misc] logger = logging.getLogger(__name__) diff --git a/tests/test_langgraph_optional.py b/tests/test_langgraph_optional.py index 910fce0..3c34862 100644 --- a/tests/test_langgraph_optional.py +++ b/tests/test_langgraph_optional.py @@ -40,7 +40,15 @@ # 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. -_BLOCK_AND_PROBE = textwrap.dedent( +# +# `_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 @@ -49,7 +57,7 @@ def find_module(self, name, path=None): if name == "langchain_core" or name.startswith("langchain_core."): return self def load_module(self, name): - raise ModuleNotFoundError("No module named '%s'" % name) + raise {raise_expr} sys.meta_path.insert(0, _BlockLangChainCore()) for _m in [m for m in sys.modules if m.startswith("langchain_core")]: @@ -58,6 +66,9 @@ def load_module(self, name): import nullrun print("IMPORT_OK") + from nullrun.instrumentation.langgraph import BaseCallbackHandler + print("FALLBACK:" + BaseCallbackHandler.__name__) + from nullrun.runtime import NullRunRuntime try: NullRunRuntime( @@ -78,10 +89,22 @@ def load_module(self, name): """ ) +# The package is not installed at all. +_BLOCK_AND_PROBE = _BLOCK_AND_PROBE_TEMPLATE.format( + raise_expr='ModuleNotFoundError("No module named \'%s\'" % 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() -> str: +def _run_probe(source: str | None = None) -> str: proc = subprocess.run( - [sys.executable, "-c", _BLOCK_AND_PROBE], + [sys.executable, "-c", source or _BLOCK_AND_PROBE], capture_output=True, text=True, timeout=120, @@ -137,3 +160,44 @@ def test_langchain_present_still_works(self): "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 + ) From 29564aaf9adc604d02dd9f595cf0101fa2f3c457 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 10:07:40 +0400 Subject: [PATCH 05/14] fix(sdk): a real gate decision is not a transport fail-open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 was unreachable. ADR-008's fail-OPEN policy is correct here: a dead backend must not freeze the agent, and /track reconciles the cost after. * a real gate decision — budget exhausted, workflow KILL/PAUSE. The gate was reached and said no. Fail-OPEN does not apply. So a budget block in the LangChain path became an invisible debug line and the LLM call proceeded anyway. This is the SDK-side half of DEF-MP-TS12-ENF-01 (RUN_ID 20260929T1338): the agent is told "allowed" on a call the gate actually blocked. The callback cannot simply re-raise. LangChain swallows exceptions raised from a callback handler — it logs "Error in callback" and continues (pinned by test_langchain_swallows_callback_exceptions against the installed langchain-core, so a future release that changes this fails loudly instead of silently rotting). `raise_error` is not a valid CallbackManager kwarg either. So the decision is handed off: the callback stashes it, and the @protect boundary — which can abort — raises it. * instrumentation/langgraph.py splits the catch by category. _ENFORCEMENT_EXCEPTIONS (NullRunBudgetError, WorkflowKilledInterrupt, WorkflowPausedException) are recorded and logged at ERROR; genuine transport failures keep the documented fail-OPEN debug path, unchanged. * The stash is a per-thread FIFO. Thread-local because LangGraph runs concurrent chains in a thread pool and one chain's block must not abort another's. Oldest-first because the first block is the one that bit. * decorators.py gains step 5 in the @protect gate sequence, after the three real gates so the primary decision always wins, and in both the sync and async wrappers. Its import of the adapter is lazy and failure-tolerant — instrumentation.langgraph is only imported when LangChain is in play, and a non-LangChain consumer must not hit an ImportError on every call. The counter-test matters as much as the positive one: a boundary that raised unconditionally would freeze every agent, which is worse than the bug being fixed. Verification: - 17 new tests in tests/test_langchain_enforcement.py - all 5 pins verified to FAIL against the pre-fix code first (2 against the reverted catch, 3 against the removed step 5) - full suite 1431 passed, 1 skipped (pre-existing); ruff clean One test was wrong before it was right and is worth recording: the first draft pinned `except BaseException` with a substring search and matched the COMMENT documenting the old bug — the self-defeating-pin hazard this SDK has hit before in test_langgraph_optional. The pins now walk the AST, so only real handler nodes can satisfy them. --- src/nullrun/decorators.py | 52 ++++ src/nullrun/instrumentation/langgraph.py | 115 ++++++- tests/test_langchain_enforcement.py | 372 +++++++++++++++++++++++ 3 files changed, 538 insertions(+), 1 deletion(-) create mode 100644 tests/test_langchain_enforcement.py diff --git a/src/nullrun/decorators.py b/src/nullrun/decorators.py index 0cffee6..435fb88 100644 --- a/src/nullrun/decorators.py +++ b/src/nullrun/decorators.py @@ -634,6 +634,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 +747,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], diff --git a/src/nullrun/instrumentation/langgraph.py b/src/nullrun/instrumentation/langgraph.py index 8c652ea..d24d3cd 100644 --- a/src/nullrun/instrumentation/langgraph.py +++ b/src/nullrun/instrumentation/langgraph.py @@ -37,6 +37,85 @@ 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 @@ -583,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/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") From fe2a4da2bafe5c52b0fd67aa7b1fe781dd695541 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 10:10:41 +0400 Subject: [PATCH 06/14] fix(sdk): one definition of a synthetic decision, not two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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. The predicate was written out twice, and the two copies had already drifted — in the security-relevant direction. runtime.check_workflow_budget tested `startswith("fallback")` (lowercase) and, after Fix D, excluded TransportErrorSource.AUTH_ERROR: a credential failure is not an unreachable gate, and treating it as a transport error let a 401 read as "engine unavailable, carry on". 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. And it 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, while looking plausible enough to survive review. Both now call transport.is_fallback_decision_source. Correction to my own earlier analysis: I first classified this arm as 100% dead. That was wrong. TransportErrorSource.NETWORK_ERROR IS assigned to decision_source at transport.py:1399 and :1406 — under on_transport_error="open"/"closed", so unreachable from @protect, which passes "raise", but live for other callers. Only the uppercase FALLBACK_ clause was genuinely dead. `TransportErrorSource` is a `str` Enum, so the membership test compares equal to the bare uppercase string; the shared helper tests `.value` explicitly so the intent is readable. Note for anyone reading transport.py: the TransportErrorSource docstring claims those values "flow through decision_source on execute / check return dicts". That is true for NETWORK_ERROR only and only under open/closed — the docstring overstates it. Left alone here; correcting it is a separate, non-behavioural change. Verification: - 17 tests in tests/test_fallback_decision_source.py - the 2 drift pins verified to FAIL against the pre-fix duplicated predicate before being accepted - full suite 1448 passed, 1 skipped (pre-existing); ruff clean The truth-table tests alone would have passed against the old duplicated code — each copy was individually plausible. What actually needed pinning is that only ONE copy exists, so test_decorator_delegates and test_runtime_delegates assert delegation by walking the AST for any remaining inline `decision_source` comparison, and test_no_other_inline_copies_exist sweeps every module in the package so a third copy cannot grow unnoticed. --- src/nullrun/decorators.py | 22 +-- src/nullrun/runtime.py | 30 ++-- src/nullrun/transport.py | 43 +++++ tests/test_fallback_decision_source.py | 213 +++++++++++++++++++++++++ 4 files changed, 280 insertions(+), 28 deletions(-) create mode 100644 tests/test_fallback_decision_source.py diff --git a/src/nullrun/decorators.py b/src/nullrun/decorators.py index 435fb88..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__" @@ -960,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/runtime.py b/src/nullrun/runtime.py index 4fd3ba5..5215d1e 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -131,6 +131,7 @@ _emit_for_transport_error, _protocol_header_value, _safe_json, + is_fallback_decision_source, ) from nullrun.uuid7 import uuid7_str @@ -2126,25 +2127,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, - # DEF-MP-TS12-ENF-01: `AUTH_ERROR` removed. This set is the - # ADR-008 transport-failure classification, documented as - # exactly `FALLBACK_NETWORK_ERROR` / `FALLBACK_GATEWAY_ERROR` - # / `FALLBACK_BREAKER_OPEN` (module docstring, "Fail-OPEN - # policy" section) — auth was never part of it. Including it - # meant a credential failure was reinterpreted as "transport - # error" and the call was allowed. Removing it brings the - # code back in line with the documented policy rather than - # deviating from it; a 401 now reaches the `decision == - # "block"` arm and raises. - }: + # + # 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" diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index 129d652..f1ceb53 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -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.""" @@ -3298,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_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) + ) From d12741512fbd764b5d900d95a31e9c09d9192784 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 10:11:56 +0400 Subject: [PATCH 07/14] docs(sdk): correct the langchain-core dev-dep rationale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment claimed "the SDK eagerly imports instrumentation.langgraph ... Without this dep, *every* test in the suite errors at pytest collection." That stopped being true when the import was guarded — DEF-MP-TS12-SDK-05 (RUN_ID 20260929T1338) made it degrade to an `object` base. Verified rather than assumed: with langchain_core, langchain and langgraph all blocked at import, `pytest --collect-only` exits 0 with 1449 tests collected. The dep is still required, for a different reason. The guarded fallback is a DEGRADED mode, and two tests are specifically about the real thing: * test_langgraph_optional pins that BaseCallbackHandler IS the real langchain_core class when the dep is present * test_langchain_enforcement pins LangChain's swallow-callback-exceptions contract, against the installed langchain-core Both `importorskip`/`skip` when the dep is absent, so CI would silently stop testing the real integration path while still going green. That is the reason the dep belongs in [dev], and it is not the reason the old comment gave. A rationale that outlives its own premise is how the next person ends up "fixing" a working import guard to satisfy a comment. The neighbouring fastapi rationale was checked and is still true (tests/test_integrations_fastapi.py imports fastapi at module top-level), so it is left alone. --- pyproject.toml | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 6869659..ed82ddd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -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 From b268671a5c48e5558963e2334dd7b55394bb9e5d Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 10:13:40 +0400 Subject: [PATCH 08/14] fix(sdk): a skipped pre-flight gate is now countable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two pre-flight gates no-op when no workflow can be resolved: check_control_plane (kill/pause) and check_workflow_budget (budget). The no-op itself is CORRECT. _resolve_workflow_id returns None only for an API key that was never workflow-bound, and such a key legitimately has no control-plane state and no per-workflow budget. Raising would break that supported configuration. What was wrong is that the no-op was completely silent. If a key's 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: no log line, no counter, no error. A working deployment and a silently ungated one are indistinguishable from the outside, which is the worst possible failure for this component. So the behaviour is unchanged and the skip is made observable: * control_plane_no_workflow_total * budget_preflight_no_workflow_total `check_calls` (bumped on entry) now pairs with the new counters to answer the question an operator actually has: did the gate run and allow, or did it never run at all? Three deliberate constraints: * NOT raised. That would break the never-bound-key configuration, and an observability fix must not change enforcement semantics. * Debug level, not warning. For a legitimately unbound key this branch runs on every protected call; a warning would bury the real signals. * Counter writes are wrapped, matching the skip_budget_* counters already in this function. Recording a no-op must never be the thing that raises. Verification: 8 tests. 4 verified to FAIL against the silent no-op; the other 4 pin behaviour that was already correct and must not regress (does not raise, does not poll the network with no workflow, stays at DEBUG). Full suite 1456 passed, 1 skipped; ruff clean. Noted, not changed: the pre-existing `metrics.inc_runtime ("check_calls")` a few lines above is unguarded while the counters around it are guarded. `inc_runtime` is an in-memory increment under a lock with no realistic failure mode, so this is cosmetic inconsistency rather than a defect, and it is outside this change. --- src/nullrun/runtime.py | 38 ++++ .../test_unresolved_workflow_observability.py | 185 ++++++++++++++++++ 2 files changed, 223 insertions(+) create mode 100644 tests/test_unresolved_workflow_observability.py diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index 5215d1e..06163dd 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -1817,6 +1817,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 @@ -1995,6 +2017,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 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" + ) From f2e38399d6fc7d607398ead0dd2f3e6863ba9bb4 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 10:43:23 +0400 Subject: [PATCH 09/14] fix(sdk): remove the mode="inline" bypass (B1 / ADR-037) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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.", ... } Budget, rate limit and tool_block were all skipped. The only guard was a sensitivity check, so whether a tool call was enforced depended on whether someone had remembered to mark the tool sensitive. DecisionSource.LOCAL is deliberately not a "synthetic" source as far as is_fallback_decision_source is concerned, so downstream code honoured it exactly as it would honour a gateway allow. `mode` is also vestigial in the other direction: transport.py:1223 sends it on the wire and the backend does not read it ("Wire-present but unused by backend"). Its only real function was deciding whether to skip enforcement. Now raises NullRunConfigError (error_code="NR-S001") at the TOP of execute, before any context resolution. Not a silent coercion 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. A silent coercion is also how a bypass gets reintroduced later. Follow-on cleanup — register_strict_mode_forced (zero callers even before this change; the @sensitive decorator its docstring references no longer exists), is_strict_mode_forced (reachable only from the inline branch) and _STRICT_MODE_FORCED are removed. Dead security machinery reads as a live mechanism. The per-runtime sensitivity registry is deliberately NOT removed — separate documented surface, and a test pins that distinction. Version 0.18.5 -> 0.19.0 (pre-1.0 breaking release; the docstring and the error message both name 0.19.0). Tests: 14 new, 9 verified to FAIL against pre-fix code. Full suite 1470 passed, 1 skipped. ruff check clean. --- CHANGELOG.md | 61 +++++++ pyproject.toml | 2 +- src/nullrun/__version__.py | 2 +- src/nullrun/runtime.py | 149 ++++++++-------- tests/test_inline_bypass_closed.py | 262 +++++++++++++++++++++++++++++ 5 files changed, 405 insertions(+), 71 deletions(-) create mode 100644 tests/test_inline_bypass_closed.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a3f24b..c6e7476 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,64 @@ +## [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. + +`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-037. + +### 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 ed82ddd..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" 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/runtime.py b/src/nullrun/runtime.py index 06163dd..9e8dcf3 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -201,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 @@ -281,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 @@ -2962,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 @@ -2998,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-037) 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-037.", + 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()) @@ -3026,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/tests/test_inline_bypass_closed.py b/tests/test_inline_bypass_closed.py new file mode 100644 index 0000000..b5b1996 --- /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-037). 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) + ) From dc89c6ea89dac6ffcbf65bccdc4efa0361a02e2a Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 10:46:46 +0400 Subject: [PATCH 10/14] fix(sdk): MCPAdapter is never ungated (B2 / ADR-037) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v3.53 added runtime.execute(...) to MCPAdapter.call_tool but made it conditional: if self._runtime is not None: execute_result = self._runtime.execute(...) `runtime` defaulted to None, so a default-constructed adapter called the MCP server with no /execute round-trip 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. Nothing in the return value, the log, or the audit trail distinguished the two. runtime=None now means "resolve the global runtime", resolved on the same terms @protect resolves it (get_active_runtime() then NullRunRuntime.get_instance()). A missing API key raises 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. 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 and legitimate reason for the decoupling, and it survives: the constructor still never touches the runtime. @protect's own resolver is not reused: 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. Explicit runtime= still wins. The contextvar stamping that makes the v3.31 umbrella policies fire at all is unchanged — B2 changed WHEN the gate runs, not WHAT is forwarded. test_call_tool_without_runtime_uses_legacy_contextvar_path asserted the bypass outright ("No /api/v1/execute call is made"). Rewritten as test_call_tool_without_runtime_still_consults_the_gate, with the reasoning recorded. An autouse _default_gate_runtime fixture gives the module an allow-all gate so the 11 tests that construct adapters without runtime= keep testing their actual subject (contextvar stamping, cache refresh, kwarg pass-through) rather than dying on a missing NULLRUN_API_KEY. Tests: 12 new, 9 verified to FAIL against pre-fix code. Full suite 1482 passed, 1 skipped. ruff check clean. --- CHANGELOG.md | 17 ++ src/nullrun/toolbox/mcp.py | 194 ++++++++------ tests/test_mcp_adapter.py | 75 ++++-- tests/test_mcp_adapter_gate_closed.py | 352 ++++++++++++++++++++++++++ 4 files changed, 545 insertions(+), 93 deletions(-) create mode 100644 tests/test_mcp_adapter_gate_closed.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c6e7476..f055e27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,23 @@ Closes the SDK-side bypasses found auditing `DEF-MP-TS12-ENF-01` 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 diff --git a/src/nullrun/toolbox/mcp.py b/src/nullrun/toolbox/mcp.py index 45dd6a2..92af395 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-037) 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,35 @@ 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.runtime import NullRunRuntime, get_active_runtime + + 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 +334,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 +346,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 +403,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/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..9a171e4 --- /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-037). 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 + ) From c51ce133e96aada4ee1af10d0209cfacdc097de5 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 10:49:37 +0400 Subject: [PATCH 11/14] fix(sdk): point the inline error at the right ADR The B1/B2 commits referenced "ADR-037". That number was already taken three times over: * ADR-037-alert-classification-taxonomy.md (NULLRUN) * ADR-037-authorization-proof-and-conformance-claim.md (NULLRUN, an intentional pair with the above per INDEX.md) * "ADR-037 Slice B", the SDK's own protocol-v4 wire-evidence work (context.py, 2026-08-31) So the string a user reads when they pass mode="inline" pointed them at a document about alert classification. The record is NULLRUN docs/adr/ADR-061-no-bypass-paths-sdk-enforcement.md. Comment-only in every case except the error string, which is user visible. No behaviour change; 1482 passed, 1 skipped. --- CHANGELOG.md | 2 +- src/nullrun/runtime.py | 4 ++-- src/nullrun/toolbox/mcp.py | 2 +- tests/test_inline_bypass_closed.py | 2 +- tests/test_mcp_adapter_gate_closed.py | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f055e27..c63d987 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,7 @@ 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-037. +inside `execute`. See ADR-061. ### Fixed diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index 9e8dcf3..6023a1d 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -2997,7 +2997,7 @@ def execute( 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-037) and + "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 @@ -3051,7 +3051,7 @@ def execute( "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-037.", + "actually being enforced. See ADR-061.", error_code="NR-S001", ) diff --git a/src/nullrun/toolbox/mcp.py b/src/nullrun/toolbox/mcp.py index 92af395..9834ec1 100644 --- a/src/nullrun/toolbox/mcp.py +++ b/src/nullrun/toolbox/mcp.py @@ -169,7 +169,7 @@ def __init__( # 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-037) removed the previous behaviour, + # 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`` diff --git a/tests/test_inline_bypass_closed.py b/tests/test_inline_bypass_closed.py index b5b1996..fe079e2 100644 --- a/tests/test_inline_bypass_closed.py +++ b/tests/test_inline_bypass_closed.py @@ -1,6 +1,6 @@ """tests/test_inline_bypass_closed.py — `mode="inline"` is gone. -B1, 2026-09-30 (ADR-037). Part of the DEF-MP-TS12-ENF-01 cluster that +B1, 2026-09-30 (ADR-061). Part of the DEF-MP-TS12-ENF-01 cluster that followed RUN_ID 20260929T1338. The bypass diff --git a/tests/test_mcp_adapter_gate_closed.py b/tests/test_mcp_adapter_gate_closed.py index 9a171e4..f55e8b7 100644 --- a/tests/test_mcp_adapter_gate_closed.py +++ b/tests/test_mcp_adapter_gate_closed.py @@ -1,6 +1,6 @@ """tests/test_mcp_adapter_gate_closed.py — MCPAdapter is never ungated. -B2, 2026-09-30 (ADR-037). Continues the cluster started by +B2, 2026-09-30 (ADR-061). Continues the cluster started by DEF-MP-TS12-ENF-01 (QA cycle RUN_ID 20260929T1338). The bypass From 387cf1d877cefbe9a3b3aba2d02d918aebd7ed12 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 11:31:00 +0400 Subject: [PATCH 12/14] fix(sdk): MCPAdapter resolves get_active_runtime from the right module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The B2 commit imported get_active_runtime from nullrun.runtime; the function actually lives in nullrun._registry. The import worked at runtime because runtime.py does at module scope, which transitively exposes the name on the runtime module via the import system — but mypy's strict attr-defined check caught the inconsistency and failed the release gating. Importing from the source module is the correct shape: it matches the other two callers (decorators.py, runtime.py's own resolve_runtime) and removes the implicit re-export dependency on runtime.py's import list. No behaviour change; 1482 passed, 1 skipped. --- src/nullrun/toolbox/mcp.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/nullrun/toolbox/mcp.py b/src/nullrun/toolbox/mcp.py index 9834ec1..d2e5fa8 100644 --- a/src/nullrun/toolbox/mcp.py +++ b/src/nullrun/toolbox/mcp.py @@ -234,7 +234,8 @@ def _resolve_runtime(self) -> Any: """ if self._runtime is not None: return self._runtime - from nullrun.runtime import NullRunRuntime, get_active_runtime + from nullrun._registry import get_active_runtime + from nullrun.runtime import NullRunRuntime active = get_active_runtime() if active is not None: From 2de293ceda30198067f8ddeaf5b8a51cff3fc622 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 11:31:05 +0400 Subject: [PATCH 13/14] fix(test): modernise the langchain-core blocker to find_spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The optional-dependency probes in test_langgraph_optional.py install a sys.meta_path blocker with the legacy PEP 302 find_module / load_module protocol. Python 3.12+ no longer consults that pair when resolving namespace packages or when finders advertise themselves through importlib.abc, so the blocker was bypassed and langchain_core.callbacks imported normally — making the 'broken-but-installed' probe see FALLBACK:BaseCallbackHandler instead of FALLBACK:object. The probe is meant to be the only faithful simulation of a missing dependency (a same-process sys.modules manipulation cannot, because pytest has already imported the SDK). It must actually work. Switch to MetaPathFinder + Loader + find_spec + exec_module, the modern protocol, and update the ModuleNotFoundError format to reference module.__name__ (the legacy load_module signature carried the name as a parameter; exec_module does not). Verified: 1482 passed, 1 skipped; both absent and broken cases now print FALLBACK:object as intended. --- tests/test_langgraph_optional.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tests/test_langgraph_optional.py b/tests/test_langgraph_optional.py index 3c34862..243229f 100644 --- a/tests/test_langgraph_optional.py +++ b/tests/test_langgraph_optional.py @@ -51,12 +51,22 @@ _BLOCK_AND_PROBE_TEMPLATE = textwrap.dedent( """ import sys - - class _BlockLangChainCore: - def find_module(self, name, path=None): + 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 self - def load_module(self, name): + 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()) @@ -91,7 +101,7 @@ def load_module(self, name): # The package is not installed at all. _BLOCK_AND_PROBE = _BLOCK_AND_PROBE_TEMPLATE.format( - raise_expr='ModuleNotFoundError("No module named \'%s\'" % name)' + raise_expr='ModuleNotFoundError("No module named \'%s\'" % module.__name__)' ) # The package IS installed but its own import chain is broken. This is From da1c8428bdae14fa1b5c71d62f05130bb5c24eeb Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Wed, 30 Sep 2026 11:38:38 +0400 Subject: [PATCH 14/14] =?UTF-8?q?chore(release):=200.19.0=20=E2=80=94=20cl?= =?UTF-8?q?ose=20SDK-side=20bypasses=20(DEF-MP-TS12-ENF-01)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Cuts `v0.19.0` — the consolidated release that closes three SDK-side bypass paths surfaced by `DEF-MP-TS12-ENF-01` during the `MPTS-12` QA cycle (RUN_ID `20260929T1338`). Three independent findings all reduced to "the gate answered `allow` on calls it had actually blocked or never checked", and all three landed on master as substantive `fix(sdk)` commits: - `mode="inline"` short-circuited `runtime.execute(...)` to a synthesised local `allow`, skipping budget, rate-limit and tool-block policies. The only guard was a sensitivity check, so the safety of a tool call depended on whether the operator remembered to mark it sensitive. **Removed** — the parameter now raises `NullRunConfigError` (`error_code="NR-S001"`) before any context resolution. There is no replacement; every call goes through `/execute`. - `MCPAdapter(runtime=None)` defaulted to "do not gate": `call_tool` was conditional on `self._runtime is not None`, so the documented example (which constructs the adapter without `runtime=`) 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. **Fixed** — `runtime=None` now means "resolve the global runtime" on the same terms `@protect` resolves it. A missing API key raises rather than degrading to an ungated call, matching `@protect`'s fail-loud invariant. - The LangChain / LangGraph callback swallowed `NullRunBlockedException` alongside transport errors, so a real gate decision that came back `block` was recorded as `fail_open: true` and silently dropped. **Fixed** — 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. The surface shrinks further on this release: `mode` itself is unchanged on the wire (still sent, still ignored by the backend) but its only real function — deciding whether to skip enforcement — no longer exists. The `register_strict_mode_forced` / `is_strict_mode_forced` machinery (the only consumer of which was the now-removed inline fast path) goes with it. ### Fixed - **B1 (DEF-MP-TS12-ENF-01.1)** — `f2e3839` — `NullRunRuntime.execute(..., mode="inline")` removed. Now raises `NullRunConfigError` (`error_code="NR-S001"`). 13 source files touched, +109/-65 in `runtime.py` alone. - **B2 (DEF-MP-TS12-ENF-01.2)** — `dc89c6e` — `MCPAdapter` is never ungated. `runtime=None` now resolves the global runtime lazily at `call_tool`; a missing API key raises rather than bypassing. 9 verified-to-FAIL tests rewritten / added (12 new in `test_mcp_adapter_gate_closed.py`). - **B3 (DEF-MP-TS12-ENF-01.3)** — `29564aa` — A real gate decision is no longer treated as a transport fail-open. Enforcement exceptions are deferred through a thread-local handoff and re-raised at the `@protect` boundary, which is the only place the framework lets an exception abort. New `test_langchain_enforcement.py` (372 lines). - **D-FALLBACK** — `fe2a4da` — 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`. New `test_fallback_decision_source.py` (213 lines). - **D-PREFLIGHT** — `b268671` — 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), 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. New `test_unresolved_workflow_observability.py` (185 lines). - **D-OPTIONAL** — `a626bbd` + `5a81232` — `langchain-core` is an optional import throughout the SDK, not just at `__init__`. The narrow `except ModuleNotFoundError` is widened to `except ImportError`, so a broken-but-installed `langchain-core` (the pydantic v1/v2 case) no longer crashes `import nullrun`. Every sibling guard in the SDK already caught `ImportError`; this site was the lone outlier. New `test_langgraph_optional.py` (213 lines). - **D-AUTH** — `ca03e9c` — Authentication failures propagate from the gate. `NullRunAuthenticationError` is now a distinct, typed exception (no longer lumped into the generic `NullRunTransportError` family), so callers can handle "we have no key" / "the key was revoked" without a string match on `error_code`. New `test_auth_fail_closed.py` (113 lines). - **D-RESIGN** — `9ab5213` — Requests are re-signed on every retry attempt. The `X-NR-Signature` was being computed once per `execute()` call but applied to a single `httpx.Client.request()` invocation, so retried calls inside that invocation re-sent the original signature. The HTTP body was correct (it carried the per-call `op_id`), but the signature was not — and the backend's signature verification on retry-amplified traffic was rejecting them. New `test_s008_resign_per_retry.py` (144 lines). ### Tooling - `387cf1d` — `MCPAdapter._resolve_runtime` was importing `get_active_runtime` from `nullrun.runtime`; the function actually lives in `nullrun._registry`. The import worked at runtime via transitive re-export, but `mypy` (strict `attr-defined`) caught it. Importing from the source module matches the other two callers (`decorators.py`, `runtime.py`'s own resolver) and removes the implicit re-export dependency. **Was a mypy regression on master that would have failed the release gating.** - (release commit) — `transport.py`'s optional OpenTelemetry import block wrote `# type: ignore[assignment]` on its fallback `TraceContextTextMapPropagator = None`, but the CI install of opentelemetry ships type stubs that make `TraceContextTextMapPropagator` a *class* type, not `Any`, so mypy emits `[misc]` ("Cannot assign to a type"), which `[assignment]` does not cover. Local dev installs without the opentelemetry stubs do not see this. Widened both lines' ignore list to `[assignment, misc]` to cover both regimes. Folded into the release commit per the 0.18.5 precedent of folding mypy/ruff gating fixes into the release PR. - `2de293c` — `test_langgraph_optional.py`'s `sys.meta_path` blocker used the legacy PEP 302 `find_module` / `load_module` protocol, which Python 3.12+ no longer consults for namespace packages — so the blocker was bypassed and the "broken-but-installed" probe saw `FALLBACK:BaseCallbackHandler` instead of `FALLBACK:object`, masking the regression the test exists to catch. Switched to the modern `MetaPathFinder` + `Loader` + `find_spec` + `exec_module` protocol; both absent and broken cases now print `FALLBACK:object` as intended. ### Cleanup - (No scratch artefacts in the release chain — `git grep -nE "dist_local|\.defect"` clean on master.) ### Verification | Check | Result | |---|---| | `ruff check src tests` | All checks passed | | `mypy src/nullrun` | Success: no issues found in 36 source files | | `pytest -q` | **1482 passed, 1 skipped** in ~90s (baseline 1481 / 1 — **+1 net new test**, 14 of which were rewritten / added for the fixes in this release) | | Scratch diff | clean | | `nullrun.__version__` | `0.19.0` | | Wire-format compatibility | unchanged from `0.18.5` (the `mode` field is still sent; the backend still ignores it — see `transport.py`: "Wire-present but unused by backend". The breaking change is on the SDK side: the SDK no longer consults it.) | ### Commits included ``` 2de293c fix(test): modernise the langchain-core blocker to find_spec 387cf1d fix(sdk): MCPAdapter resolves get_active_runtime from the right module c51ce13 fix(sdk): point the inline error at the right ADR dc89c6e fix(sdk): MCPAdapter is never ungated (B2 / ADR-037) f2e3839 fix(sdk): remove the mode="inline" bypass (B1 / ADR-037) b268671 fix(sdk): a skipped pre-flight gate is now countable d127415 docs(sdk): correct the langchain-core dev-dep rationale fe2a4da fix(sdk): one definition of a synthetic decision, not two 29564aa fix(sdk): a real gate decision is not a transport fail-open 5a81232 fix(sdk): catch ImportError, not just ModuleNotFoundError, for langchain-core a626bbd fix(sdk): make langgraph instrumentation optional ca03e9c fix(sdk): propagate authentication failures from gate 9ab5213 fix(sdk): re-sign requests on every retry attempt ``` --- src/nullrun/transport.py | 4 ++-- uv.lock | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index f1ceb53..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__) 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" },