From 4ec5460b3d34c8bea3a0372b03d14e5a998ff1d8 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 09:01:03 +0400 Subject: [PATCH 01/12] fix(sdk): register the two backend budget codes in the SDK catalog BUDGET_WORKFLOW_BLOCKED and BUDGET_CACHE_EXCEEDED are registered in the backend's GateErrorCode::all() (error_codes.rs:666-667, both 402) and the backend logged BUDGET_WORKFLOW_BLOCKED x389 in production before they were registered there at all. The SDK catalog was never updated to match, so the typed dispatcher could not classify either: "BUDGET_WORKFLOW_BLOCKED" in _V3_ERROR_CODE_MAP -> False The code therefore fell through to the base-class drift tier, which preserves the literal string for visibility but yields no typed budget class. A caller branching on NullRunBudgetError to mean "stop spending" saw an untyped block instead. This drift is invisible in practice because the /gate pre-flight used to raise NullRunBudgetError unconditionally for every refusal, so nothing downstream ever consulted the catalog. That hardcode is removed in the companion commit, which is what exposes the gap. Found by QA cycle RUN_ID 20261002T0826 (TC-4), against production build e8d811a0. Tests: 1603 passed, 1 skipped. ruff clean. mypy clean for the files touched here (the 6 remaining errors are in instrumentation/auto.py and reproduce identically without these changes). --- src/nullrun/transport.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index 1a881ab..c79b29e 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -3221,6 +3221,24 @@ def _build_v3_error_code_map() -> dict[str, type[Exception]]: "BUDGET_SOFT_BLOCKED": NullRunBudgetError, "BUDGET_OVERDRAFT_EXCEEDED": NullRunBudgetError, "BUDGET_PERIOD_NOT_STARTED": NullRunBudgetError, + # DEF-TC4-001 (2026-10-02). These two are registered in the + # backend's `GateErrorCode::all()` (error_codes.rs:666-667, + # `BudgetWorkflowBlocked` / `BudgetCacheExceeded`, both 402) + # and the backend logged `BUDGET_WORKFLOW_BLOCKED` ×389 in + # production before they were registered at all. The SDK + # catalog was never updated to match, so the typed dispatcher + # could not classify them: `BUDGET_WORKFLOW_BLOCKED` fell to + # the base-class drift tier and a caller branching on + # `NullRunBudgetError` to mean "stop spending" saw an + # untyped block instead. + # + # Drift between the two registries is exactly what + # `test_unknown_wire_code_falls_back_to_base` exists to + # surface — this pair is the reason that test matters, and + # the reason the catalog has to be checked when a code is + # registered backend-side. + "BUDGET_WORKFLOW_BLOCKED": NullRunBudgetError, + "BUDGET_CACHE_EXCEEDED": NullRunBudgetError, # Note: BUDGET_REDIS_UNAVAILABLE and RATE_LIMIT_REDIS_UNAVAILABLE # because the backend never emits it (it is absent from # ``GateErrorCode::all()`` in error_codes.rs). A cookbook that From 76efa7b99a312c604459daa7e3ff01887744e725 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 09:01:12 +0400 Subject: [PATCH 02/12] fix(sdk): type the /gate pre-flight refusal instead of assuming budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check_workflow_budget raised NullRunBudgetError for EVERY block. A rate-limit refusal, a policy tool-block and a workflow-inactive all reached the caller as NR-B004 "budget exhausted". The typed dispatcher already existed and was wired into Runtime.execute; the /gate pre-flight simply never called it, so the two paths could not disagree about what a refusal means. Observed on production (QA cycle RUN_ID 20261002T0826, TC-4, build e8d811a0) against a workflow with rate_limit_per_minute=5: [0..4] ALLOW [5] NullRunBudgetError code=NR-B004 "blocked: RATE_LIMIT_EXCEEDED" The gate was correct — 5 allows then a block at index 5. The classification was not: an operator reading NR-B004 goes to raise a spend cap when the actual cause is a throttle policy, and a caller catching NullRunBudgetError to mean "stop spending" stops for the wrong reason. Four changes, each independently necessary: 1. The pre-flight routes through _build_block_exception. 2. The dispatcher resolves the wire code the way the BACKEND resolves the HTTP status for the same body: an ordered candidate list — details["error_code"], then the top-level error_code that Transport.check already copies onto its 4xx dict, then explanation (which the Block dispatcher binds reason_code into). Matching the backend's order is what keeps the status and the exception class from disagreeing about which code a refusal is. An explicitly-sent code still reaches the base-class drift tier when unregistered, preserving test_unknown_wire_code_falls_back_ to_base; only explanation-as-candidate is gated on registration, so English prose never presents as a wire code. 3. The dispatcher handles all three catalog families, not just the NullRunBlockedException one. The transport family (RateLimitError, NullRunBackendError, NullRunExecutionNotFoundError) takes (message, source, endpoint, ...), infra-not-transport (NullRunRateLimitRedisError) takes the bare message. Passing the decision-shaped kwargs raised TypeError and let the refusal escape untyped. Dispatch is by signature, so a future catalog entry cannot silently pick the wrong shape. source is never forwarded through **details — it collides with the keyword the class passes down itself. 4. cost_limit_exceeded is bumped only for NullRunBudgetError. It is the operator's "budget cap is biting" counter; bumping it for a rate-limit block is the second half of the same mislabelling. The pre-flight keeps its own caller contract on the legacy tier: a response with no machine-readable code still raises NullRunBudgetError, pinned by test_real_block_still_honored, test_enforcement_4xx_still_raises_budget_error and test_block_response_does_not_infect_subsequent_track. The shared dispatcher stays on the base class for a type guessed from English (test_legacy_keyword_path_budget) — a subclass chosen by substring-matching claims more confidence than the wire gave. The two callers want different things from the same dispatcher output, so the widening happens at the caller that promises it. Two existing tests changed, both asserting the artefact of the hardcode rather than a behaviour: - test_403_breaker_trip_stops_the_agent / test_403_is_not_retried expected NullRunBudgetError for CIRCUIT_BREAKER_TRIPPED, a category-halt refusal that is absent from the catalog and now correctly surfaces as the base class with the wire code preserved. Their actual subject — honoured, not retried, reason text intact — is unchanged and still asserted. - test_legacy_keyword_path_budget is untouched. New: tests/test_gate_block_typed_dispatch.py, 6 cases covering the typed rate-limit classes, the tool-block-is-not-budget half, the budget path, and the legacy tier. Verified failing on the old code first (4 failed, 2 passed) before any change. Tests: 1603 passed, 1 skipped. ruff clean. mypy clean for the files touched (the 6 remaining errors are in instrumentation/auto.py and reproduce identically without these changes). --- src/nullrun/runtime.py | 214 ++++++++++++- ...adr063_infra_refusal_is_not_fail_closed.py | 25 +- tests/test_gate_block_typed_dispatch.py | 282 ++++++++++++++++++ 3 files changed, 504 insertions(+), 17 deletions(-) create mode 100644 tests/test_gate_block_typed_dispatch.py diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index c83d217..1bfc124 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -77,13 +77,14 @@ import asyncio import builtins +import inspect import logging import os import threading import time import uuid from collections.abc import Callable -from typing import Any +from typing import Any, cast import httpx @@ -2524,23 +2525,93 @@ def check_workflow_budget(self) -> None: reasons="; ".join(reasons), agent_message=response.get("agent_message"), ) - # Bump ``cost_limit_exceeded`` when the pre-flight - # blocks the workflow. The counter is the operator's - # primary signal for "the budget cap is biting" -- - # distinct from loop / retry / rate which have their - # own counters. - metrics.inc_runtime("cost_limit_exceeded") + # ``cost_limit_exceeded`` (the operator's "budget cap is + # biting" counter) is bumped further down, ONLY for a + # budget-class refusal — see DEF-TC4-001 below. + # # ``NullRunBudgetError`` carries structured # ``error_code``, ``user_action``, ``retryable`` so the # LLM gets an actionable hint instead of "Something went # wrong". ``reasons`` preserved in details for telemetry. - raise NullRunBudgetError( + # + # DEF-TC4-001 (QA RUN_ID 20261002T0826, 2026-10-02): + # this raise site hardcoded NullRunBudgetError for EVERY + # refusal, so a rate-limit block, a policy tool-block and + # a workflow-inactive all reached the caller as NR-B004 + # "budget exhausted". Observed on production: an + # `RATE_LIMIT_EXCEEDED` 429 surfaced as + # NullRunBudgetError. An operator reading that code goes + # to raise a cap when the actual cause is a throttle + # policy, and a caller that catches NullRunBudgetError to + # mean "stop spending" stops for the wrong reason. + # + # The typed dispatcher already exists and is wired into + # `Runtime.execute`; the pre-flight just never called it. + # Routing through it here makes the `/gate` and + # `/execute` paths agree on one classification. + # + # `cost_limit_exceeded` is NOT bumped for a non-budget + # refusal — it is the operator's "budget cap is biting" + # counter, and the CLAUDE.md notes treat rate / loop / + # tool as separately-owned signals. Bumping it for a + # rate-limit block is the second half of the same + # mislabelling. + block_error = self._build_block_exception( + result=response, workflow_id=workflow_id, - reason="; ".join(reasons), - action="block", - decision_source=response.get("decision_source"), - reasons="; ".join(reasons), + tool_name=get_call_tools()[0] if call_tools else None, ) + # The dispatcher builds the reason from `explanation` and + # carries the whole wire `details` verbatim, which is + # where `decision_source` already lives. The pre-flight + # additionally surfaces the JOINED `explanations` — a + # multi-reason block reads better as one string here than + # as the dispatcher's single `explanation`. + if not getattr(block_error, "reasons", None): + # `reasons` is declared per-class, not on a shared + # base, so the attribute assignment is typed through + # a cast rather than `setattr` — the dispatcher + # genuinely returns one of several families and + # `NullRunBlockedException` is the widest type it + # can be typed as here. + _ReasonsCarrier = cast("Any", block_error) + _ReasonsCarrier.reasons = "; ".join(reasons) + if ( + type(block_error) is NullRunBlockedException + and getattr(block_error, "error_code", None) == "NR-B004" + ): + # The legacy keyword tier: the wire sent no + # machine-readable code, so the dispatcher GUESSED + # "budget" from the explanation text and — by contract + # (`test_legacy_keyword_path_budget`) — returned the + # base class rather than claiming a typed guess. + # + # The pre-flight is a BUDGET pre-flight, and its + # caller contract is `NullRunBudgetError` (pinned by + # `test_real_block_still_honored`, + # `test_enforcement_4xx_still_raises_budget_error`, + # `test_block_response_does_not_infect_subsequent_track`). + # The two callers want different things from the same + # dispatcher output, so the widening happens HERE, at + # the caller that promises it — not by teaching the + # shared dispatcher to over-confident guesses that + # `/execute` callers would then inherit. + # + # Only the base class is upgraded, and only when the + # keyword tier guessed budget; a guess at loop / rate + # / tool stays on the base class, so the pre-flight + # never invents a typed claim the wire did not make. + block_error = NullRunBudgetError( + workflow_id=workflow_id, + reason="; ".join(reasons), + action="block", + decision_source=response.get("decision_source"), + reasons="; ".join(reasons), + details=block_error.details, + ) + if isinstance(block_error, NullRunBudgetError): + metrics.inc_runtime("cost_limit_exceeded") + raise block_error if decision == "throttle": reasons = response.get("explanations") or ( [response["explanation"]] if response.get("explanation") else ["throttle"] @@ -3774,7 +3845,42 @@ class (e.g. ``NullRunApprovalReplayRejectedError`` for wire_details = result.get("details") or {} if not isinstance(wire_details, dict): wire_details = {} - wire_error_code = wire_details.get("error_code") + # DEF-TC4-001: resolve the wire code the way the BACKEND + # resolves the HTTP status for the same body — an ordered + # candidate list, first registered hit wins. See + # `backend/src/proxy/http/gate/gate.rs::gate_response_to_response`, + # which reads `details["error_code"]` and then + # `response.explanation` (the Block dispatcher binds + # `reason_code` into that slot). Two things were missing here + # and both showed up as a budget error on production: + # + # * the top-level `error_code` field, which `Transport.check` + # already copies onto its 4xx return dict + # (`transport.py` — `"error_code": wire_body.get("error_code")`) + # and which several real refusal bodies carry; + # * `explanation`, which is the fallback the backend itself + # relies on, so a body the gate could classify was + # classified as NR-B004 by the SDK. + # + # Candidate order matches the backend exactly so the status + # and the exception class can never disagree about which code + # a refusal is. + wire_error_code = ( + wire_details.get("error_code") + or result.get("error_code") + or explanation + ) + # An UNREGISTERED code must keep flowing to the base-class + # tier, which preserves it verbatim on `error_code` — that is + # the backend/SDK drift signal operators branch on + # (`test_unknown_wire_code_falls_back_to_base`). So do not + # null it out. What must not happen is dispatching on + # `explanation` as if it were a code: an English sentence + # that happens to be absent from the catalog must take the + # keyword tier, not claim to be drift. + _code_is_explicit = bool( + wire_details.get("error_code") or result.get("error_code") + ) # Catalog classes with a custom ``__init__`` that promotes # a wire field to a first-class attribute (e.g. @@ -3826,7 +3932,17 @@ def _build_payload( return payload # Priority 1: typed catalog dispatch via _V3_ERROR_CODE_MAP. - if wire_error_code and isinstance(wire_error_code, str): + # + # An explicitly-sent code reaches this tier even when the + # catalog has never heard of it — that is the drift path, and + # the base-class arm below preserves the literal string. The + # `explanation` candidate only enters when it IS registered; + # an English sentence that happens to miss the catalog belongs + # to the keyword tier, which reports a synthetic NR-* rather + # than presenting prose as if it were a wire code. + if wire_error_code and isinstance(wire_error_code, str) and ( + _code_is_explicit or wire_error_code in _V3_ERROR_CODE_MAP + ): typed_cls = _V3_ERROR_CODE_MAP.get(wire_error_code) if typed_cls is not None: # 1a: typed SUBCLASS (e.g. @@ -3848,6 +3964,66 @@ def _build_payload( for k in typed_kwarg_names if k in wire_details } + # DEF-TC4-001: the catalog is not one family. + # The decision-shaped kwargs below + # (`workflow_id` / `reason` / `action` / + # `tool_name` / `details`) only fit the + # `NullRunBlockedException` hierarchy. The other + # two families take different signatures, and + # passing the wrong ones raises `TypeError`, so a + # refusal escapes untyped instead of being + # classified: + # + # * transport — `(message, source, endpoint, …)`. + # `source` must NOT be forwarded through + # `**details` (it collides with the keyword + # the class passes down itself). + # * infra (not transport) — the bare + # `NullRunError(message, error_code=…)` shape + # that `NullRunRateLimitRedisError` uses. + # + # This arm was unreachable before DEF-TC4-001: + # the only caller was `Runtime.execute`, whose + # blocks are approval codes, all + # `NullRunBlockedException`-family. The `/gate` + # pre-flight now calls this dispatcher and + # reaches the rate-limit and infra codes, so the + # constructor shapes have to be told apart here + # rather than at each raise site. + if issubclass(typed_cls, NullRunTransportError): + params = inspect.signature( + typed_cls.__init__ + ).parameters + kwargs: dict[str, Any] = {} + if "source" in params: + kwargs["source"] = ( + TransportErrorSource.GATEWAY_ERROR + ) + if "endpoint" in params: + kwargs["endpoint"] = "gate" + if "retry_after" in params: + retry_after_ms = wire_details.get( + "retry_after_ms" + ) + kwargs["retry_after"] = ( + retry_after_ms / 1000.0 + if isinstance( + retry_after_ms, (int, float) + ) + else None + ) + if "status_code" in params: + kwargs["status_code"] = result.get( + "status_code" + ) + return typed_cls(explanation, **kwargs) + if issubclass(typed_cls, NullRunInfrastructureError): + # `error_code` is deliberately NOT passed — + # the class attribute owns it, and + # overriding it with the wire code would + # defeat `format_user_message`'s catalog + # lookup. + return typed_cls(explanation) return typed_cls( workflow_id=workflow_id, reason=explanation, @@ -3908,6 +4084,16 @@ def _build_payload( block_code = "NR-X001" mapped = "NullRunBlockedException" payload = _build_payload(wire_details, mapped) + # NOTE: the branch table computes `mapped` but the base class + # is returned. That is deliberate and pinned by + # ``tests/test_2026_09_10_runtime_block_typed_dispatch.py:: + # test_legacy_keyword_path_budget`` — the legacy tier + # (no wire `error_code`) reports what it GUESSED from an + # English substring, and a guessed typed class would claim + # more confidence than the wire gave. The guess is still + # visible: `error_code` is the synthetic `NR-*` and + # `details["mapped_class"]` names the class it would have + # been. Only the wire-code tier above dispatches on type. return NullRunBlockedException( workflow_id=workflow_id, reason=explanation, diff --git a/tests/test_adr063_infra_refusal_is_not_fail_closed.py b/tests/test_adr063_infra_refusal_is_not_fail_closed.py index 7ffcb42..c393551 100644 --- a/tests/test_adr063_infra_refusal_is_not_fail_closed.py +++ b/tests/test_adr063_infra_refusal_is_not_fail_closed.py @@ -68,7 +68,11 @@ import respx from nullrun.breaker.categories import NullRunUnclassifiedRefusalError -from nullrun.breaker.exceptions import NullRunBudgetError, NullRunError +from nullrun.breaker.exceptions import ( + NullRunBlockedException, + NullRunBudgetError, + NullRunError, +) BASE_URL = "https://api.test.nullrun.io" GATE_URL = f"{BASE_URL}/api/v1/gate" @@ -219,13 +223,28 @@ def test_403_breaker_trip_stops_the_agent(self, make_runtime, mock_api): 500, which is also `>= 500` — so it took the same retry-then- fail-OPEN path and the agent was told to retry a workflow an operator had just stopped. + + DEF-TC4-001 (2026-10-02) changed the expected CLASS, not the + behaviour. The pre-flight used to raise `NullRunBudgetError` + for every refusal, so this test passed on the budget type by + accident. `CIRCUIT_BREAKER_TRIPPED` is category `halt`, is + absent from the SDK catalog, and correctly resolves to the + base `NullRunBlockedException` with the wire code preserved on + `error_code` — a breaker trip is not budget exhaustion, and an + operator reading NR-B004 would go look at spend caps. """ respx.post(GATE_URL).mock(return_value=_breaker_trip_403()) rt = make_runtime() - with pytest.raises(NullRunBudgetError) as exc_info: + with pytest.raises(NullRunBlockedException) as exc_info: rt.check_workflow_budget() + assert not isinstance(exc_info.value, NullRunBudgetError), ( + "a circuit-breaker trip must not surface as budget " + "exhaustion — it is a `halt`, and the two send an operator " + "to completely different screens" + ) + assert exc_info.value.error_code == "CIRCUIT_BREAKER_TRIPPED" assert "stopped by its circuit breaker" in exc_info.value.reason assert "Do not retry" in exc_info.value.reason, ( "the agent-facing instruction must survive the round trip — " @@ -249,7 +268,7 @@ def _count(request: httpx.Request) -> httpx.Response: respx.post(GATE_URL).mock(side_effect=_count) rt = make_runtime() - with pytest.raises(NullRunBudgetError): + with pytest.raises(NullRunBlockedException): rt.check_workflow_budget() assert len(calls) == 1, ( diff --git a/tests/test_gate_block_typed_dispatch.py b/tests/test_gate_block_typed_dispatch.py new file mode 100644 index 0000000..9a5a7b5 --- /dev/null +++ b/tests/test_gate_block_typed_dispatch.py @@ -0,0 +1,282 @@ +"""DEF-TC4-001 (QA RUN_ID 20261002T0826, 2026-10-02, prod e8d811a0): +a ``/gate`` rate-limit refusal must surface as the TYPED rate-limit +exception, not as ``NullRunBudgetError``. + +## What was observed + +``rate_limit_demo.py`` (TC-4) against production, policy +``policy_type=RateLimit`` / ``rate_limit_per_minute=5``: + +``` +[0..4] ALLOW +[5] OTHER-EXC type=NullRunBudgetError code=NR-B004 + msg="Workflow ... blocked: RATE_LIMIT_EXCEEDED (action=block, + status_code=None, details={'decision_source': 'gateway', …})" +[6,7] same +``` + +The GATE is correct: 5 allows, then block at index 5, reason +``RATE_LIMIT_EXCEEDED``. The probe's own parser only accepts +``NR-R001`` / ``NR-R002`` / ``NR-W002/RATE_LIMIT_EXCEEDED``, so it +reports REVIEW. + +## Root cause — two independent defects, both required + +1. **Backend omits ``details.error_code`` on the rate-limit block.** + ``orchestrator.rs:641-654`` builds the Block ``details`` with + only ``scope`` / ``limit_per_minute`` / ``current_count`` / + ``retry_after_seconds`` / ``retry_after_ms``. The code is + carried in the 2nd tuple element (``reason_code`` → binds to + ``GateResponse.explanation``), which is what makes the HTTP + status resolve correctly (429 via the two-candidate lookup in + ``gate.rs:88-104``). But ``error_code`` never reaches + ``details``, so a client reading the machine-readable field + (CLAUDE.md §13) sees nothing. + +2. **SDK ``check_workflow_budget`` does no typed dispatch at all.** + ``runtime.py`` raises ``NullRunBudgetError`` unconditionally in + its ``decision == "block"`` arm, so EVERY refusal — rate-limit, + tool-block, workflow-inactive — surfaces as NR-B004. The + typed dispatcher ``_build_block_exception`` (which reads + ``details["error_code"]`` and instantiates the catalog class) + exists and is wired into ``Runtime.execute``; the ``/gate`` + pre-flight never calls it. + +Defect 1 alone leaves the SDK with no code to dispatch on; defect 2 +alone leaves the SDK dispatching on a field nobody sets. Both ship. + +## Why ``RateLimitError`` is the right answer here + +``transport._parse_v3_error_envelope`` already maps +``RATE_LIMIT_EXCEEDED → RateLimitError`` and +``RATE_LIMIT_REDIS_UNAVAILABLE → NullRunRateLimitRedisError``, and +both are documented in ``integrations/fastapi.py:39`` as the codes a +caller branches on. The probe, the cookbook, and the SDK's own +catalog all agree; only the pre-flight raise site disagrees. + +## The tests + +Each models the real wire body captured from production 429 (a body +whose ``details`` carries NO ``error_code``, plus the +``category``/``user_message``/``agent_message`` surface ADR-062 +attaches), and asserts the TYPED class. A body that still only +reaches the keyword-on-explanation fallback (no ``error_code``) is +covered too — it must NOT claim to be a typed rate-limit refusal. +""" + +from __future__ import annotations + +import httpx +import pytest +import respx + +from nullrun.breaker.exceptions import ( + NullRunBudgetError, + NullRunRateLimitRedisError, + RateLimitError, +) + +BASE_URL = "https://api.test.nullrun.io" +GATE_URL = f"{BASE_URL}/api/v1/gate" + + +def _rate_limit_body(*, with_error_code: bool) -> dict: + """The production 429 body for a per-workflow rate-limit block. + + ``with_error_code=False`` reproduces prod BEFORE the fix + (orchestrator.rs built ``details`` without the key). ``True`` is + the post-fix shape. Both carry ``explanation`` = the reason code + because the Block dispatcher puts ``reason_code`` in that slot. + """ + details: dict = { + "decision_source": "gateway", + "reasons": "RATE_LIMIT_EXCEEDED", + "scope": "api_key", + "limit_per_minute": 5, + "current_count": 6, + "retry_after_seconds": 42, + "retry_after_ms": 42000, + "enforcement_path": "rate_limit", + } + if with_error_code: + details["error_code"] = "RATE_LIMIT_EXCEEDED" + return { + "decision": "block", + "decision_source": "gateway", + "explanation": "RATE_LIMIT_EXCEEDED", + "policy_version": 1, + "explanations": ["RATE_LIMIT_EXCEEDED"], + # ADR-062 surface as attached by the backend. `budget` is NOT + # model-message-safe, so `agent_message` is absent by design. + "category": "budget", + "user_message": "Rate limit exceeded for this workflow.", + "details": details, + } + + +def _mock_gate(body: dict, status: int) -> None: + respx.post(GATE_URL).mock( + return_value=httpx.Response(status, json=body) + ) + + +class TestRateLimitBlockIsTyped: + """The headline regression: a rate-limit refusal is a RateLimitError.""" + + def test_rate_limit_exceeded_raises_rate_limit_error( + self, make_runtime, mock_api + ): + _mock_gate(_rate_limit_body(with_error_code=True), 429) + rt = make_runtime() + with pytest.raises(RateLimitError) as exc_info: + rt.check_workflow_budget() + assert exc_info.value.error_code == "NR-R001" + + def test_rate_limit_redis_unavailable_raises_typed_redis_error( + self, make_runtime, mock_api + ): + """NR-R002 is the fail-CLOSED infra sibling. It must NOT be + reported as a budget exhaustion either — an operator reading + NR-B004 would go raise a cap when Redis is what is down.""" + body = _rate_limit_body(with_error_code=True) + body["explanation"] = "RATE_LIMIT_REDIS_UNAVAILABLE" + body["explanations"] = ["RATE_LIMIT_REDIS_UNAVAILABLE"] + body["category"] = "infra" + body["details"]["error_code"] = "RATE_LIMIT_REDIS_UNAVAILABLE" + _mock_gate(body, 503) + rt = make_runtime() + with pytest.raises(NullRunRateLimitRedisError): + rt.check_workflow_budget() + + def test_rate_limit_redis_unavailable_does_not_raise_budget_error( + self, make_runtime, mock_api + ): + """The negative half of the same property, asserted + separately so a future refactor cannot pass the test above by + raising the right thing for the wrong reason.""" + body = _rate_limit_body(with_error_code=True) + body["explanation"] = "RATE_LIMIT_REDIS_UNAVAILABLE" + body["explanations"] = ["RATE_LIMIT_REDIS_UNAVAILABLE"] + body["category"] = "infra" + body["details"]["error_code"] = "RATE_LIMIT_REDIS_UNAVAILABLE" + _mock_gate(body, 503) + rt = make_runtime() + with pytest.raises(NullRunRateLimitRedisError): + try: + rt.check_workflow_budget() + except NullRunBudgetError as exc: # pragma: no cover + pytest.fail( + f"infra refusal reported as budget exhaustion: {exc!r}" + ) + raise # re-raise the RateLimitRedis error for the outer assert + + +class TestToolBlockIsNotABudgetError: + """TC-2's observation, generalised: the same hardcoded raise made + a policy tool-block look like budget exhaustion.""" + + def test_tool_blocked_raises_tool_blocked_error( + self, make_runtime, mock_api + ): + body = { + "decision": "block", + "decision_source": "gateway", + "explanation": "TOOL_BLOCKED", + "policy_version": 1, + "explanations": ["TOOL_BLOCKED"], + "category": "denied", + "user_message": "Tool 'bash' is blocked by policy.", + # Denied IS model-message-safe, so ADR-062 populates this. + "agent_message": "I can't run that tool.", + "details": {"error_code": "TOOL_BLOCKED"}, + } + _mock_gate(body, 403) + rt = make_runtime() + with pytest.raises(Exception) as exc_info: + rt.check_workflow_budget() + assert not isinstance(exc_info.value, NullRunBudgetError), ( + "a policy tool-block surfaced as NullRunBudgetError / NR-B004 — " + "the pre-flight raise site hardcodes the budget type for every " + "refusal instead of dispatching on details.error_code" + ) + assert exc_info.value.error_code == "NR-T001" + + +class TestBudgetBlockStillBudget: + """The dispatch must not regress the case that motivated the + hardcoded raise in the first place (test_gate_real_path.py).""" + + def test_budget_block_still_raises_budget_error( + self, make_runtime, mock_api + ): + body = { + "decision": "block", + "decision_source": "gateway", + "explanation": "BUDGET_HARD_BLOCKED", + "policy_version": 1, + "explanations": ["Budget exhausted: need 5 cents, 0 available"], + "category": "budget", + "user_message": "Budget exhausted.", + "details": {"error_code": "BUDGET_HARD_BLOCKED"}, + } + _mock_gate(body, 402) + rt = make_runtime() + with pytest.raises(NullRunBudgetError) as exc_info: + rt.check_workflow_budget() + assert exc_info.value.error_code == "NR-B004" + + def test_legacy_explanation_only_block_keeps_its_pinned_class( + self, make_runtime, mock_api + ): + """A pre-structured backend that sends no ``error_code`` must + not start raising something new. + + Two contracts meet here and both are preserved: + + * the SHARED dispatcher stays on the base + ``NullRunBlockedException`` for a type guessed from English + — pinned by ``test_2026_09_10_runtime_block_typed_dispatch.py:: + test_legacy_keyword_path_budget``, because a subclass chosen + by substring-matching claims more confidence than the wire + gave; + * the ``/gate`` PRE-FLIGHT is a budget pre-flight and its + caller contract is ``NullRunBudgetError`` — pinned by + ``test_gate_real_path.py::test_real_block_still_honored``. + + So the widening happens at the caller that promises it, not in + the shared dispatcher. This test pins the observable result of + that split, since both pins alone would pass with either half + missing. + + Status 200 + ``decision="block"`` rather than 402: a 4xx + refusal is required to carry ``category`` (ADR-062 §2.2) and a + pre-structured backend predates that field.""" + body = { + "decision": "block", + "decision_source": "gateway", + "explanation": "Budget exhausted: need 5 cents, 0 available", + "policy_version": 1, + "explanations": [], + } + _mock_gate(body, 200) + rt = make_runtime() + with pytest.raises(NullRunBudgetError) as exc_info: + rt.check_workflow_budget() + assert "Budget exhausted" in exc_info.value.reason + assert exc_info.value.error_code == "NR-B004" + # The dispatcher's guess is still visible, so an operator can + # tell a guessed budget classification from a wire-stated one. + # The pre-flight wraps the dispatcher's exception, so the + # shim sits at whatever depth the constructor nested it — + # search rather than hardcode the level. + def _find_mapped_class(details: dict) -> str | None: + if "mapped_class" in details: + return details["mapped_class"] + for value in details.values(): + if isinstance(value, dict): + found = _find_mapped_class(value) + if found: + return found + return None + + assert _find_mapped_class(exc_info.value.details) == "NullRunBudgetError" From 9ccf16872f81831cd7d68dab920b16ab3f041153 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 10:16:00 +0400 Subject: [PATCH 03/12] fix(sdk): read the attribute the WS connection actually has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DEF-TC6-005 (QA RUN_ID 20261002T0826, SDK 0.20.0). TC-12 `approval_granted` against production reported: STATUS_OK=NullRunStatus(..., ws_connected=None, ...) with the listener thread alive and a real connection present 0.3s after init(): t=0.3s conn present type=WebSocketConnection has is_open: False attrs: ['url', 'headers', 'api_key', 'secret_key', 'on_state_change', 'on_policy_invalidated', 'on_key_rotated', 'on_approval_resolved', '_conn', '_running', '_receive_task', '_reconnect_task', '_closed', '_consecutive_reconnect_failures', '_last_version'] The same run logged a full WS lifecycle (CLOSE 1000 / EOF / "WebSocket connection closed"), so the channel did come up. `status()` computed the field as ws_connected = getattr(self._ws_connection, "is_open", None) `WebSocketConnection` never had an `is_open` attribute — its liveness flag is `_running`, set True in `_connect` (transport_websocket.py:243) and cleared by the receive loop's `finally` (:283). The `getattr` default therefore fired on every call and the field was structurally pinned to `None`. `is_open` appears exactly once in the SDK: on the reading side, with no writer, no test and no producer. This matters because the three states are distinct and the collapse hides the one that matters — a listener that started and then dropped. `None` means "never established", so a channel that came up and died was indistinguishable from one that never existed, and `status()` — the SDK's only introspection surface — could not report a working push channel at all. Read `_running`. The `getattr` default stays so a connection object without the flag degrades to `None` rather than raising, and all three states still report distinctly: never-established `None`, live `True`, dropped-or-shutdown `False`. Tests: tests/test_status_ws_connected_reads_real_attribute.py, four cases. The fourth is the one that would have caught this: it asserts against a really-constructed `WebSocketConnection` that the attribute `status()` reads exists, and that `is_open` does not — so a future rename of `_running` fails here rather than silently returning `None` again in production. A stubbed connection cannot catch it, because the stub would carry whatever attribute the test author assumed. Verified by mutation: reverting the read to `is_open` reds the two state tests. pytest: 1607 passed, 1 skipped, 0 failed. ruff + mypy: clean. --- src/nullrun/runtime.py | 21 ++- ...tatus_ws_connected_reads_real_attribute.py | 166 ++++++++++++++++++ 2 files changed, 183 insertions(+), 4 deletions(-) create mode 100644 tests/test_status_ws_connected_reads_real_attribute.py diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index 1bfc124..cb33e7b 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -1054,10 +1054,23 @@ def status(self) -> Any: ws_connected: bool | None = None if self._ws_connection is not None: - # ``is_open`` is the underlying websockets flag - # None when the connection has never been - # successfully established. - ws_connected = getattr(self._ws_connection, "is_open", None) + # DEF-TC6-005 (2026-10-02): this read `is_open`, an + # attribute `WebSocketConnection` has never had. Its + # liveness flag is `_running` — set True in `_connect` + # (transport_websocket.py:243) and cleared by the receive + # loop's `finally` (`:283`). The `getattr` default fired + # on every call, so the field was structurally pinned to + # `None`: a live push channel and a dead one were + # indistinguishable, and TC-12 could not observe the + # control plane at all. + # + # `_running` is instance state, and the `getattr` default + # is kept so a connection object that does not carry the + # flag degrades to `None` rather than raising. Note the + # three states are distinct and all still reported: + # never-established `None`, established-and-live `True`, + # established-then-dropped / explicit shutdown `False`. + ws_connected = getattr(self._ws_connection, "_running", None) elif self._ws_stop_event.is_set(): ws_connected = False # explicit shutdown diff --git a/tests/test_status_ws_connected_reads_real_attribute.py b/tests/test_status_ws_connected_reads_real_attribute.py new file mode 100644 index 0000000..e016615 --- /dev/null +++ b/tests/test_status_ws_connected_reads_real_attribute.py @@ -0,0 +1,166 @@ +"""DEF-TC6-005 (QA RUN_ID 20261002T0826, 2026-10-02, SDK 0.20.0): +``status().ws_connected`` could never report anything but ``None``. + +## What was observed + +TC-12 ``approval_granted`` against production. The probe printed: + + STATUS_OK=NullRunStatus(..., ws_connected=None, + organization_id='a374da02-…', …) + +with the WebSocket listener thread ALIVE and a real connection +observed a fraction of a second after ``init()``: + + t=0.3s conn present type=WebSocketConnection + has is_open: False + attrs: ['url', 'headers', 'api_key', 'secret_key', + 'on_state_change', 'on_policy_invalidated', + 'on_key_rotated', 'on_approval_resolved', '_conn', + '_running', '_receive_task', '_reconnect_task', + '_closed', '_consecutive_reconnect_failures', + '_last_version'] + +The same run logged a successful WS lifecycle (CLOSE 1000 / EOF / +"WebSocket connection closed"), so the channel did come up. + +## Root cause + +``runtime.status()`` computed the field as: + + ws_connected = getattr(self._ws_connection, "is_open", None) + +``WebSocketConnection`` never had an ``is_open`` attribute — its +liveness flag is ``_running``, set ``True`` in ``_connect`` +(``transport_websocket.py:243``) and cleared by the receive loop's +``finally`` (``:283``). So the ``getattr`` default fired on every +call and the field was structurally pinned to ``None``. + +``is_open`` appears exactly once in the whole SDK: on the reading +side, with no writer, no test, and no producer. It is a name that +looked right (``websockets`` exposes something similar internally) +and was never checked against the object it was read from. + +## Why this is a real bug, not a cosmetic one + +``status()`` is the SDK's only introspection surface, and +``ws_connected`` is the field a caller (or a QA oracle) uses to +decide whether the push channel is live. ``None`` means "never +established" and ``False`` means "shut down" — collapsing both into +``None`` makes a working control plane indistinguishable from a +dead one, and hides the case that actually needs attention: a +listener that started and then dropped. + +## The fix + +Read ``_running``, the flag the connection class actually +maintains. The ``getattr`` default stays so a connection object from +a future version without the flag still degrades to ``None`` rather +than raising. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +from nullrun.runtime import NullRunRuntime + + +def _runtime_with_ws(conn: object) -> NullRunRuntime: + """A runtime whose ``_ws_connection`` is ``conn``. + + Built via ``object.__new__`` so the WS branch of ``status()`` is + reached without an authenticate-and-connect cycle — this test is + about the field's computation, not about the transport. The + attribute set is exactly what ``status()`` reads, which is why it + is spelled out rather than mocked: a missing one surfaces as an + ``AttributeError`` at a line unrelated to the assertion. + """ + import threading + + from nullrun.observability.status import _RecentErrorRing + + rt = object.__new__(NullRunRuntime) + rt._ws_connection = conn + rt._ws_stop_event = threading.Event() + rt.organization_id = "org-test" + rt.api_key = "nr_live_testkey" + rt._api_key_valid = True + # The real ring type, not a list: `status()` calls `.snapshot()` + # on it, and a bare list fails there instead of at the assertion. + rt._recent_errors = _RecentErrorRing(capacity=10) + rt._remote_states = {} + rt._last_backend_attempt_ok = None + rt._last_backend_attempt_at = None + rt.workflow_id = None + rt.api_url = "https://api.example.invalid" + return rt + + +class TestWsConnectedReflectsRealState: + def test_running_connection_reports_true(self): + """The regression: a live connection must report ``True``. + + Before the fix this was ``None`` because ``WebSocketConnection`` + has no ``is_open`` — so the field could never be ``True``. + """ + conn = SimpleNamespace(_running=True) + assert _runtime_with_ws(conn).status().ws_connected is True + + def test_dropped_connection_reports_false_not_none(self): + """A listener that came up and dropped is the case an operator + needs to see. It must be ``False``, not ``None``. + + ``None`` means "never established"; conflating the two hides + exactly the failure the field exists to surface. + """ + conn = SimpleNamespace(_running=False) + assert _runtime_with_ws(conn).status().ws_connected is False + + def test_no_connection_yet_still_reports_none(self): + """Unchanged: no connection object at all is still ``None``, + and an explicit shutdown still reports ``False``. The fix must + not collapse the three states into two. + """ + assert _runtime_with_ws(None).status().ws_connected is None + + rt = _runtime_with_ws(SimpleNamespace(_running=True)) + rt._ws_connection = None + rt._ws_stop_event.set() + assert rt.status().ws_connected is False + + def test_real_connection_class_exposes_the_attribute_read(self): + """Pin the contract between the two modules. + + This is the check that would have caught the bug: the object + ``status()`` introspects must actually carry the attribute it + introspects. Uses the real class rather than a stub, so a + future rename of ``_running`` fails here instead of silently + returning ``None`` again in production. + + Both attributes are INSTANCE state, so the check is on a + constructed instance, not on the class. + """ + from nullrun.transport_websocket import WebSocketConnection + + # Real construction (the ctor is pure field assignment — no + # network). `__new__` would skip the instance attributes and + # make `hasattr` report False for everything, which is exactly + # the shape of bug this test exists to catch. + inst = WebSocketConnection( + url="wss://example.invalid/ws", + api_key="nr_live_test", + secret_key="s" * 64, + ) + assert hasattr(inst, "_running"), ( + "status() reads `_running`; renaming it without updating " + "runtime.status() reintroduces DEF-TC6-005" + ) + + # The attribute the old code read must not silently reappear — + # if a future version adds `is_open`, status() must be revisited + # deliberately rather than by accident. + assert not hasattr(inst, "is_open"), ( + "`is_open` now exists on WebSocketConnection — status() read it " + "historically and silently got None; decide which attribute is " + "authoritative and update this test" + ) From ea7c9eea5b7a1d0aae50b5f0b227486bc8fa26a8 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 10:22:28 +0400 Subject: [PATCH 04/12] fix(track): propagate enforcement rejections from the v3 /track path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DEF-TC6-006 (QA RUN_ID 20261002T0826, 2026-10-02). TC-15 consume_overbudget against production observed a 422 CONSUME_OVERBUDGET on the wire, "event dropped" in the log, and TRACK_OK={'allowed': True, 'actions': [], 'local_cost_cents': 0} returned to the probe. _route_track wrapped transport.track_single in a bare `except Exception` that logged at WARNING and returned. The transport layer had already classified the response — a 422 CONSUME_OVERBUDGET body becomes a typed NullRunConsumeOverbudgetError carrying reserved_cents, actual_cost_cents, max_allowed_cents and epsilon_cents (transport.py:2939) — and the catch discarded it. Two things make the bare catch wrong rather than blunt: * The drop-and-log policy it implements is the one the ADR-008 table states for the `/track batch path (legacy)`, a NETWORK error. The v3 single path has no such row. * The same docstring says the SDK "does NOT silently fail-OPEN on a wire 4xx/5xx that names an enforcement failure", naming /track among the handlers that raise. A refused consume names one. Re-raise NullRunDecision after the existing cache invalidation and telemetry, so the blast-radius mitigation (DEF-CACHE-STALE-ALLOW- AFTER-OVERBUDGET) is not traded away for the reporting fix. Scope is NullRunDecision, not Exception: connection errors and 5xx name no enforcement failure and keep dropping, because raising there would freeze the agent loop on a dead backend — the exact failure the fail-OPEN rows exist to prevent. ADR-008 table gains a row for the v3 single path; README fail-open claim unchanged (this narrows a swallow, it does not widen enforcement). tests/test_route_track_enforcement_propagates.py (new, 5 tests): 3 fail on the pre-fix code, 2 pin the drop-and-log half so the fix cannot over-reach. The two existing cache-invalidation tests are strengthened, not relaxed — they now assert the raise AND the invalidation. Full suite: 1612 passed, 1 skipped, 0 failed. ruff + mypy clean. --- src/nullrun/runtime.py | 43 ++++ ...test_route_track_enforcement_propagates.py | 208 ++++++++++++++++++ tests/test_v3_wire_contract.py | 41 ++-- 3 files changed, 279 insertions(+), 13 deletions(-) create mode 100644 tests/test_route_track_enforcement_propagates.py diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index cb33e7b..f116a35 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -23,6 +23,7 @@ | `_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" | | `_emit_span_start` / `_emit_span_end` | n/a -- never blocks | n/a | n/a | | `/track` batch path (legacy) | OPEN-on-network-error (event dropped, no retry) | n/a -- circuit breaker backoff applies | none | +| `/track` v3 single path (`track_single`) | OPEN-on-network-error and OPEN-on-5xx (event dropped, no retry); **CLOSED for enforcement rejections** — a typed `NullRunDecision` (`CONSUME_OVERBUDGET` 422, budget block 402) propagates to the caller | caller reconciles the delta from the exception's `reserved_cents` / `actual_cost_cents` / `epsilon_cents`; ADR-005 forbids implicit re-reserve, so there is nothing for the SDK to retry | none | **Fail-OPEN policy** — SDK-side transport failure (network timeout, 5xx, breaker open) is fail-OPEN on the *check* path so a dead @@ -110,6 +111,7 @@ NullRunBackendError, NullRunBlockedException, NullRunBudgetError, + NullRunDecision, NullRunDeniedError, NullRunError, NullRunInfrastructureError, @@ -4324,6 +4326,47 @@ def _route_track(self, wire_event: dict[str, Any]) -> None: correlation_id=smid, status_code=status_code, ) + # DEF-TC6-006 (2026-10-02, QA RUN_ID 20261002T0826): + # a bare `except Exception` here laundered an ADR-005 + # enforcement rejection into a transport warning. The + # transport layer had ALREADY classified it — a 422 + # CONSUME_OVERBUDGET body becomes a typed + # NullRunConsumeOverbudgetError carrying reserved / + # actual / epsilon cents (transport.py:2939) — and + # throwing that away made `track_llm` return + # `{"allowed": True}` to an agent whose consume the + # backend had just refused. Observed in TC-15: + # a 422 on the wire, "event dropped" in the log, and + # TRACK_OK={'allowed': True, ...} in the probe. + # + # The drop-and-log policy this catch implements is the + # one the ADR-008 table states for the `/track batch + # path (legacy)` (line 25) — a NETWORK error, where a + # dead backend must not freeze the agent loop. The v3 + # single path has no such row, and the same docstring + # says the SDK "does NOT silently fail-OPEN on a wire + # 4xx/5xx that names an enforcement failure", naming + # /track among the handlers that raise. A refused + # consume names one. + # + # Scope is deliberately `NullRunDecision` and not + # `Exception`: protocol errors, rate-limit-Redis and + # plain 5xx stay in the transport class and keep + # dropping, because they name no enforcement failure + # and raising on them would be the over-correction. + # The invalidation above runs first either way — the + # cached-allow blast radius (DEF-CACHE-STALE-ALLOW- + # AFTER-OVERBUDGET) is closed before the raise, not + # traded away for it. + if isinstance(exc, NullRunDecision): + logger.warning( + "_route_track: /track refused the consume for " + "execution_id=%s (%s) — propagating (ADR-008: an " + "enforcement rejection is not a transport error)", + smid, + exc, + ) + raise logger.warning( "_route_track: track_single failed for execution_id=%s (%s) — event dropped", smid, diff --git a/tests/test_route_track_enforcement_propagates.py b/tests/test_route_track_enforcement_propagates.py new file mode 100644 index 0000000..a0fda4b --- /dev/null +++ b/tests/test_route_track_enforcement_propagates.py @@ -0,0 +1,208 @@ +"""DEF-TC6-006 (QA RUN_ID 20261002T0826, 2026-10-02, SDK 0.20.0): +``_route_track`` laundered an ADR-005 enforcement rejection into a +transport warning and reported success to the agent. + +## What was observed + +TC-15 ``consume_overbudget`` against production. The probe reserved +via ``@protect`` (``/gate``) then consumed 1M tokens, far past the +reservation. The backend did exactly the right thing:: + + _route_track: track_single failed for execution_id=01a0fb42-… + (track: /track: actual cost exceeds the reservation + epsilon + (ADR-005 fixed-cents invariant). Re-issue /api/v1/gate with a + larger token estimate to expand the reservation, or reduce the + work-unit cost.) — event dropped + + TRACK_OK={'allowed': True, 'actions': [], 'local_cost_cents': 0} + +A 422 ``CONSUME_OVERBUDGET`` on the wire, and ``track_llm`` returned +``allowed: True``. + +## Root cause + +``NullRunRuntime._route_track`` wraps ``self._transport.track_single`` +in a bare ``except Exception`` that logs at WARNING and returns +(``runtime.py:4290-4330``). The transport layer has already done the +right classification by then — ``_error_to_exception`` maps a +``CONSUME_OVERBUDGET`` body to a typed +``NullRunConsumeOverbudgetError`` carrying ``reserved_cents``, +``actual_cost_cents``, ``max_allowed_cents`` and ``epsilon_cents`` +(``transport.py:2939-2949``) — and ``_route_track`` throws that +typing away. + +Two things make the bare catch wrong rather than merely blunt: + +1. **The policy it applies is scoped to a different path.** The + ADR-008 table's only ``/track`` row reads ``/track batch path + (legacy) | OPEN-on-network-error (event dropped, no retry)`` + (``runtime.py:25``). The swallow lives in the v3 *single*-event + path, which has no such row. +2. **It contradicts the table's own enforcement rule.** The same + docstring says the SDK "does NOT silently fail-OPEN on a wire + 4xx/5xx that names an enforcement failure" and names ``/track`` + in the list of handlers whose rejection the SDK "raises the + corresponding exception" for. A 422 whose body is + ``CONSUME_OVERBUDGET`` is the paradigm case. + +The exception's own docstring closes the loop: "the reservation is +NOT silently re-reserved — the caller MUST reconcile the delta +manually before retrying" (``exceptions.py:481-486``). A caller +that never sees the exception cannot reconcile anything, and +``decorators.py:966-970`` explicitly preserves +``NullRunConsumeOverbudgetError`` as first-class "for cookbook +recovery" — recovery that was unreachable from ``track_llm``. + +## Why it matters beyond the log line + +A swallowed rejection is not a lost log line. The consume is +*refused*, so the reservation is never released and the real cost is +never billed; the agent is told ``allowed: True`` and continues. The +SDK's own mitigation for that half of the problem (invalidating the +chain's cached ``allow`` on 402/422) still runs — it is above the +re-raise point — so the blast radius is bounded to the current +chain. But the *reporting* is wrong in the way CLAUDE.md invariant +#4 and ADR-013 exist to prevent: an enforcement outcome is +re-presented to the caller as a successful transport. + +## The fix + +Re-raise ``NullRunDecision`` after the existing invalidation and +telemetry. Everything else — network errors, 5xx, protocol errors — +keeps the drop-and-log behavior the table documents. +""" + +from __future__ import annotations + +import httpx +import pytest +import respx +from httpx import Response + +BASE_URL = "https://api.test.nullrun.io" +SERVER_MINTED_V1 = "0190c5b5-7c9a-7def-8a1b-0123456789ab" + +TRACK_URL = f"{BASE_URL}/api/v1/track" + +CONSUMED_TOO_MUCH = { + "error_code": "CONSUME_OVERBUDGET", + "error_message": "actual > reserved + epsilon", + "details": { + "execution_id": SERVER_MINTED_V1, + "reserved_cents": 100, + "max_allowed_cents": 101, + "actual_cost_cents": 150, + "epsilon_cents": 1, + }, +} + + +def _capture_smid() -> None: + """Bind a server-minted id so ``_route_track`` reaches + ``track_single`` instead of dropping on "no reservation".""" + from nullrun.runtime import _capture_server_minted_execution_id + + _capture_server_minted_execution_id({"reservation_id": SERVER_MINTED_V1}) + + +class TestEnforcementRejectionPropagates: + """An ADR-005 business rejection must reach the caller.""" + + @respx.mock + def test_consume_overbudget_raises_typed_error(self, make_runtime): + from nullrun.breaker.exceptions import NullRunConsumeOverbudgetError + + rt = make_runtime() + respx.post(TRACK_URL).mock( + return_value=Response(422, json=CONSUMED_TOO_MUCH) + ) + _capture_smid() + + with pytest.raises(NullRunConsumeOverbudgetError): + rt.track_llm(input_tokens=60, output_tokens=40, model="claude-sonnet-4-6") + rt._transport.flush_now() + + @respx.mock + def test_raised_error_keeps_reconciliation_attributes(self, make_runtime): + """The point of the typed error is that the caller can + reconcile. Re-raising a bare ``RuntimeError`` would satisfy + the first test and defeat the fix, so the attribute payload + is asserted directly. + """ + from nullrun.breaker.exceptions import NullRunConsumeOverbudgetError + + rt = make_runtime() + respx.post(TRACK_URL).mock( + return_value=Response(422, json=CONSUMED_TOO_MUCH) + ) + _capture_smid() + + with pytest.raises(NullRunConsumeOverbudgetError) as ei: + rt.track_llm(input_tokens=60, output_tokens=40, model="claude-sonnet-4-6") + rt._transport.flush_now() + + err = ei.value + assert err.reserved_cents == 100 + assert err.actual_cost_cents == 150 + assert err.epsilon_cents == 1 + assert err.status_code == 422 + + @respx.mock + def test_budget_block_402_also_propagates(self, make_runtime): + """402 is the same laundering with a different class: + ``NullRunBudgetError`` is a ``NullRunBlockedException``, + which is a ``NullRunDecision``. A fix that only special-cased + ``CONSUME_OVERBUDGET`` by name would miss it. + """ + from nullrun.breaker.exceptions import NullRunBudgetError + + rt = make_runtime() + respx.post(TRACK_URL).mock( + return_value=Response( + 402, + json={ + "error_code": "BUDGET_HARD_BLOCKED", + "error_message": "budget exhausted", + }, + ) + ) + _capture_smid() + + with pytest.raises(NullRunBudgetError): + rt.track_llm(input_tokens=60, output_tokens=40, model="claude-sonnet-4-6") + rt._transport.flush_now() + + +class TestTransportFailureStillDrops: + """The other half of the contract. A network failure is what the + ``/track batch path (legacy)`` row describes, and the table has no + row authorising a raise for the v3 single path on transport + errors — so it must keep dropping. Widening the fix to + ``Exception`` would freeze the agent loop on a dead backend, + which is the exact failure the fail-OPEN rows exist to prevent. + """ + + @respx.mock + def test_connection_error_does_not_raise(self, make_runtime): + rt = make_runtime() + respx.post(TRACK_URL).mock(side_effect=httpx.ConnectError("boom")) + _capture_smid() + + rt.track_llm(input_tokens=60, output_tokens=40, model="claude-sonnet-4-6") + rt._transport.flush_now() + + @respx.mock + def test_server_5xx_does_not_raise(self, make_runtime): + """A 5xx names no enforcement failure, so it stays in the + transport class. The ADR-008 text scopes the "raises the + corresponding exception" rule to responses that *name* an + enforcement failure. + """ + rt = make_runtime() + respx.post(TRACK_URL).mock( + return_value=Response(500, json={"error_message": "internal"}) + ) + _capture_smid() + + rt.track_llm(input_tokens=60, output_tokens=40, model="claude-sonnet-4-6") + rt._transport.flush_now() diff --git a/tests/test_v3_wire_contract.py b/tests/test_v3_wire_contract.py index b125c3a..c420239 100644 --- a/tests/test_v3_wire_contract.py +++ b/tests/test_v3_wire_contract.py @@ -2134,15 +2134,23 @@ def test_track_single_422_invalidates_chain_cache(self, make_runtime): {"reservation_id": SERVER_MINTED_V1} ) - rt.track_llm( - input_tokens=60, - output_tokens=40, - model="claude-sonnet-4-6", - ) + # DEF-TC6-006 (2026-10-02): the 422 CONSUME_OVERBUDGET + # now PROPAGATES to the caller as a typed + # NullRunConsumeOverbudgetError instead of being logged + # and swallowed. The invalidation this test was written + # to pin is unchanged -- it runs above the re-raise in + # the same handler -- so the assertion is strengthened, + # not relaxed: the blast radius is still closed, and the + # agent additionally learns its consume was refused. + from nullrun.breaker.exceptions import NullRunConsumeOverbudgetError + + with pytest.raises(NullRunConsumeOverbudgetError): + rt.track_llm( + input_tokens=60, + output_tokens=40, + model="claude-sonnet-4-6", + ) - # track_llm buffers then flushes — the cache key should - # be dropped by the except handler once track_single - # raises on 422. assert cache_key not in _rt_mod._GATE_CACHE, ( "422 from /track must invalidate the matching chain's " "cache entry (DEF-CACHE-STALE-ALLOW-AFTER-OVERBUDGET)" @@ -2202,11 +2210,18 @@ def test_track_single_402_invalidates_chain_cache(self, make_runtime): {"reservation_id": SERVER_MINTED_V1} ) - rt.track_llm( - input_tokens=10, - output_tokens=10, - model="claude-sonnet-4-6", - ) + # DEF-TC6-006: same shape as the 422 test above -- + # NullRunBudgetError is a NullRunBlockedException, which + # is a NullRunDecision, so the 402 propagates too. The + # cache invalidation still runs before the raise. + from nullrun.breaker.exceptions import NullRunBudgetError + + with pytest.raises(NullRunBudgetError): + rt.track_llm( + input_tokens=10, + output_tokens=10, + model="claude-sonnet-4-6", + ) assert cache_key not in _rt_mod._GATE_CACHE, ( "402 from /track must invalidate the matching chain's " From b64dc8a82ad4186899a734fabcbfaaae3a44d59f Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 11:19:37 +0400 Subject: [PATCH 05/12] fix(mcp): forward tool class and annotations to /gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check_workflow_budget` has computed the MCP forwarding fields since the MCP integration landed — it reads `get_call_mcp_class()` and `get_call_mcp_annotations()` off the call context and sets them on `check_req` (`runtime.py:2383-2388`) — but `Transport.check` never sent `check_req`. It rebuilds the `/gate` body from an explicit allowlist (`transport.py:1524-1542` plus the conditional forwards that follow) and neither key was on it, so both values were dropped without a word. Observed on the wire against prod `e8d811a0` under QA RUN_ID 20261002T0826 (TC-29). With set_mcp_tool_context(tool_class="mcp", annotations={"read_only": False, "destructive": True, "open_world": False}) followed by one `check_workflow_budget()`, the captured `/gate` body was: ['action_digest', 'check_type', 'estimated_tokens', 'execution_id', 'idempotency_key', 'input', 'mode', 'model', 'operation_id', 'organization_id', 'stream', 'tool', 'tools', 'trace_id'] — neither field present. The public `set_mcp_tool_context` API and the `toolbox.mcp` auto-classification path were dead end to end: the SDK could never tell the gate a tool was `destructive` or `read_only`. The backend is complete and states the contract it expects (`gate/internal.rs:318-341`): "SDKs that recognise an MCP server cache the `tools/list` entry and pass the canonical `Mcp` class plus the corresponding `McpAnnotations`". `effective_tool_class()` falls back to `classify_tool` on the raw tool string when the field is absent, so this is a dead feature with a misleading API rather than a live bypass today — but on the day the server-side flag flips, destructive MCP tools will silently degrade to name-based classification. Guarded on `is not None`, not on key presence. The backend pins the negative case too (`internal.rs:8291-8295` asserts `tool_class=None` and `mcp_annotations=None` must not appear in the JSON), and an absent annotation means "unknown", not "false" (`internal.rs:334-339`). Serialising `null` would be a different value carrying a different meaning. Tests assert the captured POST body rather than pinning source text, so a future refactor that reintroduces the drop at a different line still fails. Verified failing on the pre-fix code (3 failed, 2 passed) and passing after (5 passed); full suite 1617 passed, 1 skipped, 0 failed. Not pushed. --- src/nullrun/transport.py | 31 +++++ tests/test_transport_mcp_forwarding.py | 186 +++++++++++++++++++++++++ 2 files changed, 217 insertions(+) create mode 100644 tests/test_transport_mcp_forwarding.py diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index c79b29e..abfcaa9 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -1570,6 +1570,37 @@ def check( # fingerprint. if "tool_arguments" in check_request and check_request["tool_arguments"] is not None: gate_request["tool_arguments"] = check_request["tool_arguments"] + # DEF-TC29-001 (2026-10-02, QA RUN_ID 20261002T0826): forward + # the MCP tool class + per-tool annotations. + # + # `check_workflow_budget` has computed both since the MCP + # integration landed — it reads `get_call_mcp_class()` / + # `get_call_mcp_annotations()` off the call context and sets + # them on `check_req` (`runtime.py:2383-2388`) — but this + # method never sent `check_req`. It rebuilds the body from the + # allowlist above, and neither key was on it, so both values + # were discarded here without a word. Confirmed on the wire + # against prod: `set_mcp_tool_context(tool_class="mcp", + # annotations={"read_only": False, "destructive": True, + # "open_world": False})` produced a `/gate` body with + # neither field. The public `set_mcp_tool_context` API and the + # `toolbox.mcp` auto-classification path were dead end to end. + # + # The backend already accepts and honours both + # (`gate/internal.rs:318-341` states the forwarding contract; + # `gate/tool_canonical.rs:229-249` defines `McpAnnotations` as + # `read_only` / `destructive` / `open_world`). + # + # Guarded on `is not None`, NOT on key presence. The backend + # pins the negative case too — `internal.rs:8291-8295` asserts + # `tool_class=None` / `mcp_annotations=None` must not appear in + # the JSON — and an absent annotation means "unknown", not + # "false" (`internal.rs:334-339`). Serialising `null` would be a + # different value carrying a different meaning. + if check_request.get("tool_class") is not None: + gate_request["tool_class"] = check_request["tool_class"] + if check_request.get("mcp_annotations") is not None: + gate_request["mcp_annotations"] = check_request["mcp_annotations"] _parent_execution_id = check_request.get("parent_execution_id", parent_execution_id) if _parent_execution_id is not None: gate_request["parent_execution_id"] = _parent_execution_id diff --git a/tests/test_transport_mcp_forwarding.py b/tests/test_transport_mcp_forwarding.py new file mode 100644 index 0000000..8d685e7 --- /dev/null +++ b/tests/test_transport_mcp_forwarding.py @@ -0,0 +1,186 @@ +"""DEF-TC29-001 — MCP tool class + annotations must reach ``/gate``. + +Found 2026-10-02 under QA ``RUN_ID 20261002T0826`` (suite TS-12, +TC-29). SDK 0.20.0 against prod ``e8d811a0``. + +## The defect + +``NullRunRuntime.check_workflow_budget`` computes the MCP forwarding +fields correctly (``runtime.py:2383-2388``):: + + mcp_class = get_call_mcp_class() + if mcp_class is not None: + check_req["tool_class"] = mcp_class + mcp_annotations = get_call_mcp_annotations() + if mcp_annotations is not None: + check_req["mcp_annotations"] = mcp_annotations + +...and then hands ``check_req`` to ``Transport.check``, which does not +send ``check_req``. It **rebuilds** the body from an explicit +allowlist (``transport.py:1524-1542`` plus the conditional forwards at +``:1545-1575``) and neither field is on it. The values are dropped +without a word. + +Observed on the wire — ``set_mcp_tool_context(tool_class="mcp", +annotations={"read_only": False, "destructive": True, "open_world": +False})`` followed by one ``check_workflow_budget()``: + + GATE BODY KEYS: ['action_digest', 'check_type', 'estimated_tokens', + 'execution_id', 'idempotency_key', 'input', 'mode', 'model', + 'operation_id', 'organization_id', 'stream', 'tool', 'tools', + 'trace_id'] + tool_class = + mcp_annotations = + +The public ``set_mcp_tool_context`` API and the whole +``nullrun.toolbox.mcp`` auto-classification path are therefore +non-functional end to end: the SDK can never tell the gate that a tool +is ``destructive`` or ``read_only``. + +## Why it matters even while ADR-013 is dormant + +The backend is complete and explicitly documents the contract it +expects (``backend/src/proxy/http/gate/internal.rs:318-341``): *"SDKs +that recognise an MCP server cache the ``tools/list`` entry and pass +the canonical ``Mcp`` class plus the corresponding ``McpAnnotations``"*. +``effective_tool_class()`` falls back to ``classify_tool`` on the raw +string when the field is absent, so today this is a **dead feature +with a misleading API** rather than a live bypass — the day the +server-side flag flips, destructive MCP tools will silently fall back +to name-based classification. + +## Test shape + +These are wire-shape assertions, not source pins: they capture the +actual POST body. A source pin would keep passing through any future +refactor that reintroduces the drop at a different line. +""" + +from __future__ import annotations + +import json + +import httpx +import pytest +import respx + +from nullrun.transport import Transport + +GATE_URL = "https://api.test.nullrun.io/api/v1/gate" + +# The backend's `McpAnnotations` keys (tool_canonical.rs:229-249). +# NOT the MCP wire names `destructiveHint` / `readonlyHint` — the +# SDK's `set_mcp_tool_context` docstring pins these three. +MCP_ANNOTATIONS = {"read_only": False, "destructive": True, "open_world": False} + + +@pytest.fixture +def transport(): + t = Transport(api_url="https://api.test.nullrun.io", api_key="test-key-12345678") + yield t + t.stop() + + +def _base_check_request(**overrides) -> dict: + req = { + "organization_id": "org-1", + "execution_id": "0198aaaa-0001-7000-8000-000000000001", + "operation_id": "op-1", + "check_type": "tool", + "tool": "mcp__github__delete_branch", + "estimated_tokens": 1, + "action_digest": "digest-1", + } + req.update(overrides) + return req + + +def _sent_body(route) -> dict: + return json.loads(route.calls[0].request.content.decode("utf-8")) + + +class TestMcpForwardingReachesTheWire: + """The two fields `check_workflow_budget` computes must survive.""" + + @respx.mock + def test_tool_class_is_forwarded(self, transport): + route = respx.post(GATE_URL).mock( + return_value=httpx.Response(200, json={"decision": "allow"}) + ) + transport.check(_base_check_request(tool_class="mcp")) + + assert route.called + assert _sent_body(route)["tool_class"] == "mcp", ( + "DEF-TC29-001: Transport.check rebuilds the /gate body from an " + "explicit allowlist (transport.py:1524-1542) that has no " + "`tool_class` key, so the value runtime.py:2385 sets is " + "silently dropped before the wire" + ) + + @respx.mock + def test_mcp_annotations_are_forwarded(self, transport): + route = respx.post(GATE_URL).mock( + return_value=httpx.Response(200, json={"decision": "allow"}) + ) + transport.check(_base_check_request(mcp_annotations=dict(MCP_ANNOTATIONS))) + + assert route.called + assert _sent_body(route)["mcp_annotations"] == MCP_ANNOTATIONS, ( + "DEF-TC29-001: `mcp_annotations` is not on Transport.check's " + "allowlist, so a tool declared `destructive` reaches the gate " + "as unknown and mcp_destructive_policy can never fire" + ) + + @respx.mock + def test_both_fields_survive_together(self, transport): + route = respx.post(GATE_URL).mock( + return_value=httpx.Response(200, json={"decision": "allow"}) + ) + transport.check( + _base_check_request(tool_class="mcp", mcp_annotations=dict(MCP_ANNOTATIONS)) + ) + + body = _sent_body(route) + assert body["tool_class"] == "mcp" + assert body["mcp_annotations"] == MCP_ANNOTATIONS + + +class TestAbsentFieldsStayAbsent: + """Forwarding is conditional — the backend pins this on its side. + + `internal.rs:8291-8295` asserts `tool_class=None` and + `mcp_annotations=None` must NOT appear in the JSON. A `"key" in + check_request` guard would send them as null and break that pin + plus every pre-MCP SDK's wire shape, so the fix must test for + `is not None`, not for presence. + """ + + @respx.mock + def test_no_keys_when_sdk_has_no_opinion(self, transport): + route = respx.post(GATE_URL).mock( + return_value=httpx.Response(200, json={"decision": "allow"}) + ) + transport.check(_base_check_request()) + + body = _sent_body(route) + assert "tool_class" not in body + assert "mcp_annotations" not in body + + @respx.mock + def test_explicit_none_is_not_serialised(self, transport): + """`None` means "I don't know" and must stay off the wire. + + The backend treats an absent annotation as *unknown*, not as + false (`internal.rs:334-339`). Serialising `null` would be a + different value with a different meaning. + """ + route = respx.post(GATE_URL).mock( + return_value=httpx.Response(200, json={"decision": "allow"}) + ) + transport.check( + _base_check_request(tool_class=None, mcp_annotations=None) + ) + + body = _sent_body(route) + assert "tool_class" not in body + assert "mcp_annotations" not in body From 610830800c64b62ff42c30d0ddfc95e2a3f2ae53 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 11:19:44 +0400 Subject: [PATCH 06/12] docs(kill): stop claiming the kill signal is BaseException-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DEF-TC21-001, found 2026-10-02 under QA RUN_ID 20261002T0826 (TC-21). Three shipped places told users a property the class has not had since 0.16.6: * `docs/errors/NR-W002.md` — "Exception class: `WorkflowKilledInterrupt` (subclass of `BaseException`, NOT `Exception`)" and "`except Exception` will NOT catch this signal by design". Also pointed at `docs/kill-contract.md` §6, a file that does not exist. * `src/nullrun/breaker/exceptions.py` — the `NullRunError` docstring repeated the claim verbatim. * the same module's class catalog listed `WorkflowKilledException` as a live public class; it was deleted in the 0.18.5 sweep. Verified at runtime on 0.20.0: WorkflowKilledInterrupt -> [WorkflowKilledInterrupt, NullRunError, BreakerError, Exception, BaseException, object] `BreakerError` derives from `Exception` (`exceptions.py:5`), so a broad `except Exception:` in host code swallows a kill — the opposite of what the docs promised, and the failure mode is silent: the workflow looks alive and the agent keeps calling tools. The behaviour itself is correct and deliberate. `9877c34` (0.16.6) reparented the class so agent recovery can catch a kill and surface the structured `error_code` / `user_action`; `tests/test_decision_split.py` documents the override. Only the prose was wrong, and it was wrong in the direction that would lead the next maintainer to revert working code. `tests/test_exception_hierarchy.py` had the same disease: the test was named `test_killed_interrupt_does_not_inherit_from_exception` while asserting `issubclass(WorkflowKilledInterrupt, Exception)`. The assertion was right; the name said its opposite. Renamed. Full suite 1617 passed, 1 skipped, 0 failed. Not pushed. --- docs/errors/NR-W002.md | 36 +++++++++++++++++++++++++------ src/nullrun/breaker/exceptions.py | 11 ++++++---- tests/test_exception_hierarchy.py | 10 ++++++++- 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/docs/errors/NR-W002.md b/docs/errors/NR-W002.md index e4e3d8f..8d70375 100644 --- a/docs/errors/NR-W002.md +++ b/docs/errors/NR-W002.md @@ -4,12 +4,36 @@ |---|---| | **Code** | `NR-W002` | | **Category** | Workflow state | -| **Exception class** | `WorkflowKilledInterrupt` (subclass of `BaseException`, NOT `Exception`) | +| **Exception class** | `WorkflowKilledInterrupt` → `NullRunError` → `BreakerError` → `Exception` | | **Retryable** | No | The workflow was killed by the NullRun control plane (via API or -auto-kill on budget exhaustion). The body did not run. The kill -is non-recoverable from inside the agent loop — let the signal -propagate to the top. `except Exception` will NOT catch this -signal by design; use `except WorkflowKilledInterrupt` or -`except BaseException`. See `docs/kill-contract.md` §6. +auto-kill on budget exhaustion). The body did not run. + +**Catch it as `WorkflowKilledInterrupt` or `NullRunWorkflowKilledError`, +not as `BaseException`.** `WorkflowKilledInterrupt` used to sit +directly on `BaseException` so a kill could not be swallowed by a +generic `except Exception:`. That changed deliberately in 0.16.6 +(`9877c34`): agent recovery needs a *catchable* kill signal so it +can surface the structured `error_code` and `user_action` instead of +dying with a bare traceback. Today the class is an ordinary +`Exception` subclass, so **`except Exception:` WILL swallow it**. + +If your loop has a broad handler, narrow it before the kill can be +eaten: + +```python +from nullrun import WorkflowKilledInterrupt + +try: + run_agent_turn() +except WorkflowKilledInterrupt: + ... # operator killed this workflow — stop cleanly +except Exception: + ... # everything else +``` + +`WorkflowKilledInterrupt` is deliberately **not** a +`NullRunDecision` (kill is a control-plane signal, not a policy +refusal) and not a `NullRunInfrastructureError` (it is not a +transport failure). `except NullRunDecision:` will not catch it. diff --git a/src/nullrun/breaker/exceptions.py b/src/nullrun/breaker/exceptions.py index 31d97aa..798bea2 100644 --- a/src/nullrun/breaker/exceptions.py +++ b/src/nullrun/breaker/exceptions.py @@ -38,7 +38,7 @@ class BreakerError(Exception): # # Existing ``except`` clauses keep working: every existing public class # (``NullRunAuthenticationError``, ``NullRunBlockedException`` -# ``NullRunTransportError``, ``WorkflowKilledException`` +# ``NullRunTransportError``, ``WorkflowKilledInterrupt`` # ``WorkflowPausedException``) inherits from ``NullRunError`` now, so # ``except NullRunError:`` catches them all — but the narrower clauses # keep matching too. @@ -74,9 +74,12 @@ class NullRunError(BreakerError): Both inherit from:class:`NullRunError`, so existing ``except NullRunError:`` clauses keep matching — the split is a strict refinement, not a breaking change. ``WorkflowKilledInterrupt`` - is **not** in either category: it remains a ``BaseException`` - subclass so kill signals bypass any ``except Exception:`` that - might otherwise swallow them. + is **not** in either category: it inherits directly from + ``NullRunError`` because a kill is a control-plane signal, not a + policy decision and not a transport failure. It *is* an + ``Exception`` subclass, so a broad ``except Exception:`` in host + code will swallow it — catch ``WorkflowKilledInterrupt`` (or + ``NullRunWorkflowKilledError``) first. See ``docs/errors/NR-W002.md``. """ # Default error code when a subclass does not override it. diff --git a/tests/test_exception_hierarchy.py b/tests/test_exception_hierarchy.py index 96f6284..6268393 100644 --- a/tests/test_exception_hierarchy.py +++ b/tests/test_exception_hierarchy.py @@ -73,7 +73,7 @@ def test_all_exceptions_inherit_from_nullrun_error(self): f"SDK failure." ) - def test_killed_interrupt_does_not_inherit_from_exception(self): + def test_killed_interrupt_is_an_exception_subclass(self): # 2026-09-08 migration: WorkflowKilledInterrupt is now an # Exception subclass (``NullRunError`` parent) — formerly a # BaseException subclass. The user override: agent recovery @@ -86,6 +86,14 @@ def test_killed_interrupt_does_not_inherit_from_exception(self): # kill contract" would break cookbook recovery and this # test would fail loudly, forcing them to either keep the # migration or justify the revert in a comment. + # + # The test used to be named + # ``test_killed_interrupt_does_not_inherit_from_exception`` + # while asserting the exact opposite (DEF-TC21-001). The + # assertion was right — 0.16.6 (``9877c34``) reparented the + # class deliberately so agent recovery can catch a kill. Only + # the name lied, and a maintainer trusting the name would + # have reverted working code. assert issubclass(WorkflowKilledInterrupt, Exception) assert issubclass(WorkflowKilledInterrupt, NullRunError) From 4320ac07d900d1fc56219d0d8c7e8a93e25fb09a Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 11:43:02 +0400 Subject: [PATCH 07/12] feat(sdk): restore the tool_call BusinessImpact constructor ADR-065 decision step 1 + verification step 1. Since 0.18.5 every @protect call sent a constant {"kind":"none"} sentinel. A constant hashes to a constant, so the digest stored on the approval row at /gate and the one the backend recomputes at /execute always agreed - while binding the approval to nothing at all, which is the weakness NR-010 was raised to close. The result is that an operator approves an action and the agent still cannot run it (DEF-TC14-002). Adds ToolCallParams + BusinessImpact.tool_call(), emitting exactly what the backend's internally-tagged serde produces. The extractor_id/extractor_version fields have no skip_serializing_if on the Rust side, so they are inside the hashed bytes and must be present here too. compute_action_digest is refactored to delegate to a new canonical_bytes() so the canonical form is assertable directly. A digest is opaque - it shows that two payloads disagree, never how. The first version of the test asserted on a COPY of the serializer inside the test file, and an ensure_ascii=True mutation left it green; the mutation pass caught that and the assertions now run against the production path. Also restores a pin that aee8110 (0.18.5 deprecation sweep) deleted along with the money/tool_call constructors. That commit removed tests/test_business_impact.py while leaving the module docstring claiming "The digest is pinned by tests/test_business_impact.py", the same docstring/code disagreement that made DEF-TC14-002 read as a backend bug rather than a missing cross-repo contract. The shared fixture digest is byte-identical to the backend's DIGEST_FIXTURE_HEX_TOOL_CALL (9975a8b75a436fb78b9d141b9e0c0a90838c1243d78119b304ae6ed0526966a6). Mutation-verified: ensure_ascii=True, EXTRACTOR_VERSION drift and a kind-string typo each fail the tests that own them. pytest -q: 1656 passed, 1 skipped, 0 failed. --- src/nullrun/business_impact.py | 219 ++++++++++++++++---- tests/test_business_impact_tool_call.py | 261 ++++++++++++++++++++++++ 2 files changed, 443 insertions(+), 37 deletions(-) create mode 100644 tests/test_business_impact_tool_call.py diff --git a/src/nullrun/business_impact.py b/src/nullrun/business_impact.py index 5e7205a..2af97c7 100644 --- a/src/nullrun/business_impact.py +++ b/src/nullrun/business_impact.py @@ -1,47 +1,75 @@ """ BusinessImpact + action_digest — minimal wire helpers. -Every ``@protect`` call computes a single canonical ``NoImpact`` -envelope and forwards it to /execute. The backend's ToolParameters -Approval Rules read values out of ``tool_kwargs`` directly via -the rule's ``param_name`` field, so the SDK emits a single -canonical envelope rather than per-tool typed impacts. This -module exists so the gate can send a valid (kind, action_digest) -pair. +``@protect`` builds one ``BusinessImpact`` per logical action and +forwards it to /execute with the matching ``action_digest``. The +backend recomputes that digest server-side from the request body +and compares it to the digest STORED on the approval row at /gate +time (``payload_binding.rs:163``, ``orchestrator.rs:1511``), so the +canonical bytes below are a cross-repo contract, not an +implementation detail. Field contract mirrored by the backend at ``backend/src/proxy/gate/business_impact.rs``: - - ``business_impact`` disciminator: ``{"kind": "none"}`` - (the only wire variant the SDK ships post-0.18.2). + - ``business_impact`` discriminator: ``{"kind": "none"}`` for a + call with no tool impact, ``{"kind": "tool_call", ...}`` for a + tool invocation. - ``action_digest``: SHA-256 over ``DIGEST_PREFIX + compact canonical JSON of the impact envelope``, lowercase hex. -The digest is pinned by ``tests/test_business_impact.py`` so the -SDK ↔ backend canonicalisation can't drift silently. +The digests are pinned by ``tests/test_business_impact.py`` and +``tests/test_business_impact_tool_call.py`` so the SDK ↔ backend +canonicalisation can't drift silently. That pin is the only thing +that noticed the drift behind DEF-TC14-002, so it stays. + +ADR-065: the ``tool_call`` variant was removed in 0.18.5 and the +SDK emitted a constant ``{"kind": "none"}`` sentinel for every call. +A constant hashes to a constant, so the stored and recomputed +digests always agreed — while binding the approval to nothing at +all, which is exactly the weakness NR-010 was raised to close. See +``docs/adr/ADR-065-business-impact-reentry-unreachable.md``. """ from __future__ import annotations import hashlib import json -from dataclasses import dataclass +from dataclasses import dataclass, field from typing import Any DIGEST_PREFIX = b"nullrun/v1/business_impact:" -# ToolCall envelopes) but the SDK only mints NoImpact. KIND_NONE = "none" +KIND_TOOL_CALL = "tool_call" + +# Mirrors ``ToolCallParams::new`` (backend business_impact.rs:297-307). +# Both fields lack ``skip_serializing_if`` on the Rust side, so they +# ARE inside the hashed bytes — omitting them on this side makes +# every digest mismatch. +EXTRACTOR_ID = "nullrun.tool_call.path" +EXTRACTOR_VERSION = "1" + +# Mirrors the backend's caps (business_impact.rs:792 and +# ``ToolCallParams::validate``). The SDK validates locally so a bad +# envelope fails at construction, before it can be stored on an +# approval row the server will then refuse to match. +MAX_TOOL_NAME_BYTES = 128 +MAX_PARAM_NAME_BYTES = 64 @dataclass class NoImpactPayload: - """Sentinel payload for non-impact calls. The ONLY variant the + """Sentinel payload for a call with no business impact. The canonical JSON of this payload is ``{"kind":"none"}``. The corresponding digest is the SHA-256 of ``nullrun/v1/business_impact:{"kind":"none"}`` and is pinned by tests as a literal hex so SDK ↔ backend canonicalisation can't drift silently. + + Legitimate for an LLM check (``check_workflow_budget`` on a model + with no tool to name). NOT legitimate for a tool call: a tool + that names nothing binds its approval to nothing. """ def validate(self) -> None: @@ -52,25 +80,104 @@ def to_wire_dict(self) -> dict[str, Any]: @dataclass -class BusinessImpact: - """Top-level BusinessImpact envelope. +class ToolCallParams: + """Argument bag for a tool invocation. + + Mirrors ``ToolCallParams`` in the backend. ``params`` is + operator-defined and carries no per-key schema; the validator + only rejects what the digest layer could not round-trip + byte-identically across the two implementations. + """ + + tool_name: str + params: dict[str, Any] = field(default_factory=dict) + extractor_id: str = EXTRACTOR_ID + extractor_version: str = EXTRACTOR_VERSION + + def validate(self) -> None: + if not self.tool_name: + raise ValueError("ToolCallParams.tool_name must be non-empty") + name_bytes = self.tool_name.encode("utf-8") + if len(name_bytes) > MAX_TOOL_NAME_BYTES: + raise ValueError( + f"ToolCallParams.tool_name length {len(name_bytes)} exceeds " + f"max {MAX_TOOL_NAME_BYTES}" + ) + # Rust checks `.bytes().all(|b| b.is_ascii() && !b.is_ascii_control())`, + # which is why this is a byte test and not `str.isprintable()`. + if not all(0x20 <= b < 0x7F for b in name_bytes): + raise ValueError("ToolCallParams.tool_name must be printable ASCII") + for key, value in self.params.items(): + key_bytes = key.encode("utf-8") + if len(key_bytes) > MAX_PARAM_NAME_BYTES: + raise ValueError( + f"ToolCallParams.params[{key!r}] key length {len(key_bytes)} " + f"exceeds max {MAX_PARAM_NAME_BYTES}" + ) + _reject_unsupported_value(key, value) + + def to_wire_dict(self) -> dict[str, Any]: + return { + "kind": KIND_TOOL_CALL, + "tool_name": self.tool_name, + "params": self.params, + "extractor_id": self.extractor_id, + "extractor_version": self.extractor_version, + } + + +def _reject_unsupported_value(key: str, value: Any) -> None: + """Mirror the backend's ``check_value_kind`` (business_impact.rs:365-392). - Only the ``NoImpact`` payload variant is constructed on the - SDK side. The ``money`` and ``tool_call`` factories are not - part of the SDK surface — every call sends NoImpact and the - backend reads live values out of ``tool_kwargs`` via the - rule's ``param_name``. + serde_json parses ``1.0`` as f64 and Python parses it as float, so + the two would serialise the same logical value differently and + every digest would mismatch. ``bool`` is a subclass of ``int`` in + Python, so it has to be tested first — the backend's + ``Value::Bool`` arm is likewise distinct from ``Value::Number``. """ + if value is None or isinstance(value, (bool, str)): + return + if isinstance(value, int): + return + if isinstance(value, float): + raise ValueError( + f"ToolCallParams.params[{key!r}] has an unsupported value kind " + f"(f64 or non-finite number); use string-encoded values or a " + f"new extractor to round-trip the digest" + ) + if isinstance(value, list): + for item in value: + _reject_unsupported_value(key, item) + return + if isinstance(value, dict): + for nested_key, nested in value.items(): + nested_bytes = nested_key.encode("utf-8") + if len(nested_bytes) > MAX_PARAM_NAME_BYTES: + raise ValueError( + f"ToolCallParams.params[{key!r}][{nested_key!r}] key length " + f"{len(nested_bytes)} exceeds max {MAX_PARAM_NAME_BYTES}" + ) + _reject_unsupported_value(key, nested) + return + raise ValueError( + f"ToolCallParams.params[{key!r}] has an unsupported value kind " + f"({type(value).__name__}); it is not JSON-serialisable" + ) + - impact: NoImpactPayload # Only NoImpactPayload in 0.18.2. +@dataclass +class BusinessImpact: + """Top-level BusinessImpact envelope.""" + + impact: NoImpactPayload | ToolCallParams @property def kind(self) -> str: if isinstance(self.impact, NoImpactPayload): return KIND_NONE - raise TypeError( - f"unknown impact type: {type(self.impact)!r} — only " - ) + if isinstance(self.impact, ToolCallParams): + return KIND_TOOL_CALL + raise TypeError(f"unknown impact type: {type(self.impact)!r}") def validate(self) -> None: self.impact.validate() @@ -85,11 +192,28 @@ def no_impact(cls) -> BusinessImpact: n.validate() return cls(impact=n) + @classmethod + def tool_call( + cls, tool_name: str, params: dict[str, Any] | None = None + ) -> BusinessImpact: + """Construct a ``kind="tool_call"`` envelope for a tool call. + + One envelope per LOGICAL ACTION, built once and carried on + the call context — not rebuilt per HTTP call. ``/gate`` and + ``/execute`` must hash the same bytes or the server's + recompute will not match the digest stored at /gate time + (ADR-065, decision step 2). + """ + t = ToolCallParams(tool_name=tool_name, params=dict(params or {})) + t.validate() + return cls(impact=t) + def _canonicalize_json(value: Any) -> Any: """Sort object keys recursively before serialization. - Mirrors ``BusinessImpact::canonical_json()`` in the backend. + Mirrors ``canonicalize_json`` in the backend + (``business_impact.rs:411-439``). """ if isinstance(value, dict): items = [(k, _canonicalize_json(v)) for k, v in value.items()] @@ -100,37 +224,58 @@ def _canonicalize_json(value: Any) -> Any: return value -def compute_action_digest(impact: BusinessImpact) -> str: - """Compute the SHA-256 digest the backend expects. +def canonical_bytes(impact: BusinessImpact) -> bytes: + """The exact bytes :func:`compute_action_digest` hashes. - Algorithm (must match ``backend/src/proxy/gate/business_impact.rs`` - byte-for-byte): + Split out so tests can assert the canonical form itself rather + than a hex digest. A digest is opaque: it tells you that two + payloads disagree, never *how*. Asserting on a reimplementation + of the serializer inside the test file instead is worse than + useless — a mutation of the production ``ensure_ascii`` flag + still leaves that copy green, which is precisely how the + non-ASCII regression this function exists to prevent would ship. + + Mirrors ``BusinessImpact::canonical_json()`` in the backend: - 1. Validate the impact (``NoImpactPayload`` is the only - post-0.18.2 variant — fail-fast on bad input). - 2. Convert to wire dict (``{"kind":"none"}``). + 1. Validate the impact (fail-fast on bad input). + 2. Convert to wire dict. 3. Canonicalize (sort object keys recursively). - 4. Serialize to compact JSON (no spaces). - 6. Return lowercase hex (64 chars). + 4. Serialize to compact JSON with ``ensure_ascii=False`` — the + backend emits raw UTF-8, so escaping non-ASCII here would + change the hashed bytes. """ impact.validate() canonical_value = _canonicalize_json(impact.to_wire_dict()) - canonical_bytes = json.dumps( + return json.dumps( canonical_value, ensure_ascii=False, separators=(",", ":"), sort_keys=False, ).encode("utf-8") + + +def compute_action_digest(impact: BusinessImpact) -> str: + """Compute the SHA-256 digest the backend expects. + + Algorithm (must match ``backend/src/proxy/gate/business_impact.rs`` + byte-for-byte): SHA-256 over ``DIGEST_PREFIX + canonical_bytes``. + Returns lowercase hex (64 chars). + """ hasher = hashlib.sha256() hasher.update(DIGEST_PREFIX) - hasher.update(canonical_bytes) + hasher.update(canonical_bytes(impact)) return hasher.hexdigest() __all__ = [ "DIGEST_PREFIX", "KIND_NONE", + "KIND_TOOL_CALL", + "EXTRACTOR_ID", + "EXTRACTOR_VERSION", "NoImpactPayload", + "ToolCallParams", "BusinessImpact", + "canonical_bytes", "compute_action_digest", ] diff --git a/tests/test_business_impact_tool_call.py b/tests/test_business_impact_tool_call.py new file mode 100644 index 0000000..b1e5049 --- /dev/null +++ b/tests/test_business_impact_tool_call.py @@ -0,0 +1,261 @@ +"""Cross-repo pin for the ``tool_call`` BusinessImpact envelope. + +The backend does not trust the ``action_digest`` hex the SDK sends. +At ``/execute`` it RECOMPUTES the digest from the request's +``business_impact`` and compares it to the digest STORED on the +approval row at ``/gate`` time (``payload_binding.rs:163``, +``orchestrator.rs:1511``). So the canonical bytes below are a +contract between two repos, not an implementation detail of either: +an SDK-side canonicalisation difference is invisible in review and +fails CLOSED with ``APPROVAL_DIGEST_MISMATCH`` in production. + +The counterparty tests are in +``backend/src/proxy/gate/business_impact.rs`` — +``tool_call_canonical_json_is_the_cross_repo_contract``, +``tool_call_canonical_json_sorts_nested_param_keys``, +``tool_call_canonical_json_handles_empty_params``, +``tool_call_canonical_json_handles_non_ascii_params`` and the +``DIGEST_FIXTURE_HEX_TOOL_CALL`` golden pin. A change to +canonicalisation on either side must change BOTH files in the same +commit, and the protocol version with it. + +This file also restores a pin that the 0.18.5 deprecation sweep +(``aee8110``) deleted along with the ``money``/``tool_call`` +constructors: ``tests/test_business_impact.py``. That commit left +``business_impact.py``'s module docstring claiming "The digest is +pinned by tests/test_business_impact.py" while the file no longer +existed — the same docstring/code disagreement that made +DEF-TC14-002 look like a backend bug instead of a missing contract. +""" + +from __future__ import annotations + +import pytest + +from nullrun.business_impact import ( + EXTRACTOR_ID, + EXTRACTOR_VERSION, + KIND_NONE, + KIND_TOOL_CALL, + BusinessImpact, + NoImpactPayload, + ToolCallParams, + canonical_bytes, + compute_action_digest, +) + +# The shared fixture. Mirrored verbatim by the backend's +# `DIGEST_FIXTURE_HEX_TOOL_CALL` and its `tool_call(tool)` helper +# (params `{"region": "EU", "amount": 500}`, tool `stripe.charge`). +GOLDEN_HEX_TOOL_CALL = ( + "9975a8b75a436fb78b9d141b9e0c0a90838c1243d78119b304ae6ed0526966a6" +) + +# No backend counterpart: `enum BusinessImpact` has no `none` variant, +# so nothing server-side hashes this. It is pinned anyway because the +# SDK is currently putting this hex on the wire for every LLM check +# (ADR-065 decision step 3 keeps `no_impact()` for that shape), and a +# canonicalisation change would silently invalidate in-flight +# approvals with nothing to compare against. +GOLDEN_HEX_NONE = "0049d93a36f0710269a6deb733ca78d57a770ef640a2698d0fddaa9653b7c3de" + + +def _canonical(impact: BusinessImpact) -> str: + """The production canonical form — NOT a copy of it. + + This used to re-serialise the payload inside the test file, which + meant the `ensure_ascii` mutation left every canonical-bytes test + green. It now calls `canonical_bytes`, the same function + `compute_action_digest` hashes, so a canonicalisation change + cannot hide from these tests. + """ + return canonical_bytes(impact).decode("utf-8") + + +class TestCanonicalBytesAreTheCrossRepoContract: + """Byte layout, not just digest determinism.""" + + def test_canonical_json_is_the_cross_repo_contract(self): + impact = BusinessImpact.tool_call("refund_customer", {"region": "EU", "amount": 500}) + assert _canonical(impact) == ( + '{"extractor_id":"nullrun.tool_call.path","extractor_version":"1",' + '"kind":"tool_call","params":{"amount":500,"region":"EU"},' + '"tool_name":"refund_customer"}' + ) + + def test_canonical_json_sorts_nested_param_keys(self): + # Recursive sort, not top-level only: a params map that + # serialises in insertion order on one side and sorted on the + # other is the silent drift the digest exists to catch. + impact = BusinessImpact.tool_call("t", {"zebra": 1, "alpha": 2}) + canonical = _canonical(impact) + assert canonical.index("alpha") < canonical.index("zebra") + + def test_canonical_json_handles_empty_params(self): + # A tool with no arguments is the common case for e.g. + # `list_invoices`; it must still produce a stable envelope. + impact = BusinessImpact.tool_call("list_invoices") + assert _canonical(impact) == ( + '{"extractor_id":"nullrun.tool_call.path","extractor_version":"1",' + '"kind":"tool_call","params":{},"tool_name":"list_invoices"}' + ) + + def test_canonical_json_handles_non_ascii_params(self): + # The sharp edge. serde emits raw UTF-8 on the Rust side, so + # `ensure_ascii=False` is load-bearing here: escaping would + # change the hashed bytes while still looking correct in + # review. The backend mirrors this assertion. + impact = BusinessImpact.tool_call("refund", {"note": "возврат"}) + canonical = _canonical(impact) + assert "возврат" in canonical + assert r"\u0432\u043e\u0437\u0432\u0440\u0430\u0442" not in canonical + + def test_extractor_fields_are_inside_the_hashed_bytes(self): + # `extractor_id` / `extractor_version` have no + # `skip_serializing_if` on the Rust struct, so they are part + # of the canonical JSON. Omitting them on either side breaks + # every digest for this variant. + canonical = _canonical(BusinessImpact.tool_call("t")) + assert f'"extractor_id":"{EXTRACTOR_ID}"' in canonical + assert f'"extractor_version":"{EXTRACTOR_VERSION}"' in canonical + + +class TestDigestMatchesTheBackendFixture: + def test_digest_equals_the_backend_golden_hex(self): + impact = BusinessImpact.tool_call("stripe.charge", {"region": "EU", "amount": 500}) + assert compute_action_digest(impact) == GOLDEN_HEX_TOOL_CALL + + def test_digest_is_deterministic(self): + impact = BusinessImpact.tool_call("stripe.charge", {"region": "EU", "amount": 500}) + first = compute_action_digest(impact) + assert first == compute_action_digest(impact) + assert len(first) == 64 + + def test_digest_ignores_params_insertion_order(self): + a = BusinessImpact.tool_call("stripe.charge", {"region": "EU", "amount": 500}) + b = BusinessImpact.tool_call("stripe.charge", {"amount": 500, "region": "EU"}) + assert compute_action_digest(a) == compute_action_digest(b) + + def test_digest_changes_when_a_param_changes(self): + # The property NR-010 exists to protect: a replayed grant + # with a tampered argument bag must not reproduce the + # approved digest. + approved = BusinessImpact.tool_call("refund_customer", {"amount": 500}) + tampered = BusinessImpact.tool_call("refund_customer", {"amount": 500_000}) + assert compute_action_digest(approved) != compute_action_digest(tampered) + + def test_digest_changes_when_the_tool_name_changes(self): + # Swapping the tool while keeping every argument identical + # must also break the binding. + approved = BusinessImpact.tool_call("refund_customer", {"amount": 500}) + swapped = BusinessImpact.tool_call("charge_card", {"amount": 500}) + assert compute_action_digest(approved) != compute_action_digest(swapped) + + def test_digest_changes_when_a_nested_param_changes(self): + approved = BusinessImpact.tool_call("t", {"order": {"total": 1}}) + tampered = BusinessImpact.tool_call("t", {"order": {"total": 2}}) + assert compute_action_digest(approved) != compute_action_digest(tampered) + + def test_tool_call_and_none_never_collide(self): + # The exact hazard ADR-065 exists to remove: every tool call + # used to hash the `none` sentinel, so a grant bound to one + # tool was replayable as any other. + assert compute_action_digest(BusinessImpact.tool_call("t")) != compute_action_digest( + BusinessImpact.no_impact() + ) + + +class TestNoneSentinelPin: + def test_none_digest_is_pinned(self): + assert compute_action_digest(BusinessImpact.no_impact()) == GOLDEN_HEX_NONE + + def test_none_kind_and_payload(self): + impact = BusinessImpact.no_impact() + assert impact.kind == KIND_NONE + assert isinstance(impact.impact, NoImpactPayload) + assert _canonical(impact) == '{"kind":"none"}' + + +class TestKindDiscrimination: + def test_kind_is_tool_call(self): + assert BusinessImpact.tool_call("t").kind == KIND_TOOL_CALL + + def test_unknown_payload_type_raises(self): + # Never return null / never fail silently. + with pytest.raises(TypeError): + BusinessImpact(impact=object()).kind # type: ignore[arg-type] + + +class TestValidatorMirrorsTheBackend: + """``ToolCallParams::validate`` (business_impact.rs:308-351).""" + + def test_rejects_empty_tool_name(self): + with pytest.raises(ValueError, match="non-empty"): + BusinessImpact.tool_call("") + + def test_rejects_overlong_tool_name(self): + with pytest.raises(ValueError, match="exceeds max 128"): + BusinessImpact.tool_call("a" * 129) + + def test_accepts_tool_name_at_the_cap(self): + assert BusinessImpact.tool_call("a" * 128).kind == KIND_TOOL_CALL + + def test_rejects_non_ascii_tool_name(self): + with pytest.raises(ValueError, match="printable ASCII"): + BusinessImpact.tool_call("refund_ü") + + def test_rejects_control_characters_in_tool_name(self): + with pytest.raises(ValueError, match="printable ASCII"): + BusinessImpact.tool_call("refund\n") + + def test_rejects_overlong_param_name(self): + with pytest.raises(ValueError, match="exceeds max 64"): + BusinessImpact.tool_call("t", {"k" * 65: 1}) + + def test_rejects_float_param_value(self): + # serde_json parses 1.0 as f64 and Python as float; the two + # would serialise the same logical value differently. + with pytest.raises(ValueError, match="unsupported value kind"): + BusinessImpact.tool_call("t", {"amount": 1.0}) + + def test_rejects_nested_float_param_value(self): + # The backend recurses into arrays and objects + # (`check_value_kind`, business_impact.rs:365-392). + with pytest.raises(ValueError, match="unsupported value kind"): + BusinessImpact.tool_call("t", {"items": [1, 2.5]}) + with pytest.raises(ValueError, match="unsupported value kind"): + BusinessImpact.tool_call("t", {"order": {"total": 1.5}}) + + @pytest.mark.parametrize( + "value", + [None, True, False, "s", 0, -1, 2**63, [], {}, [1, "a", None], {"k": [1, 2]}], + ) + def test_accepts_round_trippable_values(self, value): + assert BusinessImpact.tool_call("t", {"v": value}).kind == KIND_TOOL_CALL + + def test_bool_is_not_treated_as_a_float(self): + # bool subclasses int in Python; a naive numeric check would + # either accept it by accident or reject it, and the backend + # has a distinct `Value::Bool` arm. + assert BusinessImpact.tool_call("t", {"v": True}).kind == KIND_TOOL_CALL + + def test_rejects_non_serialisable_value(self): + with pytest.raises(ValueError, match="not JSON-serialisable"): + BusinessImpact.tool_call("t", {"v": object()}) + + def test_validate_is_idempotent(self): + params = ToolCallParams("t", {"a": 1}) + params.validate() + params.validate() + + +class TestConstructionIsIsolated: + def test_caller_params_are_copied(self): + # The envelope is carried on the call context and hashed at + # /gate; a caller mutating its own dict afterwards must not + # retroactively change the bytes the approval was bound to. + params = {"amount": 500} + impact = BusinessImpact.tool_call("refund_customer", params) + before = compute_action_digest(impact) + params["amount"] = 500_000 + assert compute_action_digest(impact) == before From e3176d9de5fc8543db8db6c363f979d45881629e Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 11:44:55 +0400 Subject: [PATCH 08/12] feat(sdk): carry one BusinessImpact envelope per logical action ADR-065 decision step 2. /gate and /execute are two HTTP calls made from two different places at two different times, and the backend compares the digest it RECOMPUTES at /execute against the one it STORED at /gate (payload_binding.rs:163, orchestrator.rs:1511). They therefore have to read the same envelope. A contextvar is the right home for it: set_call_context and set_mcp_tool_context already use this exact pattern for the model, the tool list and the MCP annotations, and it keeps the envelope out of the signature of every function in between. None is a distinct third state from an envelope that says no impact. Collapsing the two is what let a constant sentinel stand in for a tool call for a full release cycle. Steps 3 (/gate reads this instead of minting a sentinel) and 4 (@protect builds it) are not in this commit. Mutation-verified: a getter that returns a fresh no_impact(), a setter that discards its Token, and a reset that does not restore each fail the tests that own them. pytest -q: 1665 passed, 1 skipped, 0 failed. --- src/nullrun/context.py | 64 +++++++++++++++++++ tests/test_call_impact_context.py | 100 ++++++++++++++++++++++++++++++ 2 files changed, 164 insertions(+) create mode 100644 tests/test_call_impact_context.py diff --git a/src/nullrun/context.py b/src/nullrun/context.py index 4afa774..04feec5 100644 --- a/src/nullrun/context.py +++ b/src/nullrun/context.py @@ -9,6 +9,8 @@ from contextlib import contextmanager from contextvars import ContextVar, Token +from .business_impact import BusinessImpact + # SpanContext that models the parent/child hierarchy a trace timeline # ``_span_id`` contextvars and now keeps them in lockstep via the # ``_mirror_to_span_context`` / ``_mirror_to_legacy_span`` helpers @@ -52,6 +54,25 @@ _call_mcp_annotations_var: ContextVar[dict[str, bool | None] | None] = ContextVar( "call_mcp_annotations", default=None ) +# The BusinessImpact envelope for the logical action in scope — ONE +# envelope per action, built once and read by both the /gate +# pre-flight and the /execute re-entry. +# +# It is a contextvar and not a per-call argument because the two HTTP +# calls are made from different places (`check_workflow_budget` and +# `@protect`) at different times, and the backend compares the digest +# it RECOMPUTES at /execute against the one it STORED at /gate +# (`payload_binding.rs:163`, `orchestrator.rs:1511`). Two separately +# constructed envelopes are two different hashes whenever the inputs +# differ by anything at all — which is why the SDK shipped a constant +# sentinel instead: see DEF-TC14-002 and ADR-065. +# +# `None` means "no envelope in scope" — the /gate pre-flight falls +# back to `no_impact()`, which is correct for an LLM check with no +# tool to name and wrong for a tool call. +_call_impact_var: ContextVar["BusinessImpact | None"] = ContextVar( + "call_impact", default=None +) # . # @@ -144,6 +165,25 @@ def get_call_mcp_annotations() -> dict[str, bool | None] | None: return _call_mcp_annotations_var.get() +def get_call_impact() -> "BusinessImpact | None": + """The BusinessImpact envelope for the logical action in scope. + + Read by BOTH ``check_workflow_budget`` (the ``/gate`` pre-flight, + which decides the digest that gets stored on the approval row) + and ``@protect`` (the ``/execute`` re-entry, where the backend + recomputes the digest from the request body and compares it to + that stored value). They must read the same object — see + :func:`set_call_impact` and ADR-065. + + ``None`` means no envelope has been set for this action. For an + LLM check that is correct and the pre-flight sends ``no_impact()``. + For a tool call it means the approval will be stored with a digest + the server cannot reproduce, so the re-entry fails CLOSED with + ``APPROVAL_DIGEST_MISMATCH``. + """ + return _call_impact_var.get() + + # --------------------------------------------------------------------------- # Chain context (v0.11.0 — ) # --------------------------------------------------------------------------- @@ -815,6 +855,30 @@ def set_mcp_tool_context( _call_mcp_annotations_var.set(annotations) +def set_call_impact(impact: "BusinessImpact | None") -> Token["BusinessImpact | None"]: + """Set the BusinessImpact envelope for the logical action in scope. + + ONE envelope per logical action, set before the ``/gate`` + pre-flight runs and read unchanged by the ``/execute`` re-entry. + Rebuilding it per HTTP call is the failure ADR-065 documents: the + backend stores the digest computed from the ``/gate`` body's + envelope and recomputes it from the ``/execute`` body's envelope, + so any difference between the two is a refused re-entry. + + ``@protect`` sets this itself for tool calls. Set it by hand only + for a custom integration that makes its own ``/gate`` and + ``/execute`` calls. + + Returns the ``Token`` for :func:`reset_call_impact`. + """ + return _call_impact_var.set(impact) + + +def reset_call_impact(token: "Token[BusinessImpact | None]") -> None: + """Restore the previous envelope. Pair with :func:`set_call_impact`.""" + _call_impact_var.reset(token) + + def generate_trace_id() -> str: """Generate a new trace ID. diff --git a/tests/test_call_impact_context.py b/tests/test_call_impact_context.py new file mode 100644 index 0000000..764882f --- /dev/null +++ b/tests/test_call_impact_context.py @@ -0,0 +1,100 @@ +"""The per-action BusinessImpact contextvar (ADR-065 step 2). + +``/gate`` and ``/execute`` are two HTTP calls issued from two +different places, and the backend compares the digest it RECOMPUTES +at ``/execute`` against the one it STORED at ``/gate`` +(``payload_binding.rs:163``, ``orchestrator.rs:1511``). The two must +therefore read the SAME envelope. This contextvar is what makes that +possible without threading an argument through both call sites. + +The pre-0.21 SDK sent a constant ``{"kind": "none"}`` sentinel from +both places instead. A constant hashes to a constant, so the digests +always agreed — while binding the approval to nothing at all. That +is DEF-TC14-002: the operator approves, and the agent still cannot +run the tool. +""" + +from __future__ import annotations + +import pytest + +from nullrun.business_impact import BusinessImpact, compute_action_digest +from nullrun.context import ( + get_call_impact, + reset_call_impact, + set_call_impact, +) + + +@pytest.fixture(autouse=True) +def _clear_impact(): + """Never leak an envelope into another test.""" + yield + set_call_impact(None) + + +class TestEnvelopeLivesOnTheContext: + def test_default_is_none(self): + # "no envelope in scope" is distinct from "an envelope that + # says no impact". Collapsing the two is what produced the + # sentinel. + assert get_call_impact() is None + + def test_set_then_get_returns_the_same_object(self): + impact = BusinessImpact.tool_call("refund_customer", {"amount": 500}) + set_call_impact(impact) + assert get_call_impact() is impact + + def test_set_none_clears(self): + set_call_impact(BusinessImpact.tool_call("t")) + set_call_impact(None) + assert get_call_impact() is None + + def test_reset_restores_the_previous_envelope(self): + first = BusinessImpact.tool_call("first") + second = BusinessImpact.tool_call("second") + set_call_impact(first) + inner = set_call_impact(second) + reset_call_impact(inner) + assert get_call_impact() is first + + def test_reset_of_the_outermost_set_returns_to_none(self): + # The token from the FIRST set restores the value that was in + # scope before it, which is None by default. Nesting is what + # the inner token above is for. + token = set_call_impact(BusinessImpact.tool_call("t")) + reset_call_impact(token) + assert get_call_impact() is None + + +class TestTheTwoCallsHashTheSameBytes: + """The property the contextvar exists to provide.""" + + def test_gate_and_execute_read_one_envelope(self): + # Simulates the two call sites: the /gate pre-flight reads the + # context, then @protect reads the same context later. The + # digests the backend stores and recomputes are these two. + set_call_impact(BusinessImpact.tool_call("refund_customer", {"amount": 500})) + stored_at_gate = compute_action_digest(get_call_impact()) + recomputed_at_execute = compute_action_digest(get_call_impact()) + assert stored_at_gate == recomputed_at_execute + + def test_a_different_tool_produces_a_different_digest(self): + # The property the constant sentinel destroyed: an approval + # granted for one tool must not be replayable as another. + set_call_impact(BusinessImpact.tool_call("refund_customer", {"amount": 500})) + approved = compute_action_digest(get_call_impact()) + set_call_impact(BusinessImpact.tool_call("charge_card", {"amount": 500})) + assert compute_action_digest(get_call_impact()) != approved + + def test_a_tampered_argument_bag_produces_a_different_digest(self): + set_call_impact(BusinessImpact.tool_call("refund_customer", {"amount": 500})) + approved = compute_action_digest(get_call_impact()) + set_call_impact(BusinessImpact.tool_call("refund_customer", {"amount": 500_000})) + assert compute_action_digest(get_call_impact()) != approved + + def test_no_impact_is_not_equivalent_to_any_tool_call(self): + set_call_impact(BusinessImpact.no_impact()) + sentinel = compute_action_digest(get_call_impact()) + set_call_impact(BusinessImpact.tool_call("refund_customer")) + assert compute_action_digest(get_call_impact()) != sentinel From 015407bb0ddb53ec7c2920d5dd50300d520b5aa2 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 11:47:36 +0400 Subject: [PATCH 09/12] feat(gate): send the context envelope at /gate instead of a sentinel ADR-065 decision step 3. check_workflow_budget unconditionally built no_impact() and hashed that. Since the envelope was a constant, the digest the backend stored on the approval row was the same for every tool call, so an approval granted for one tool was equally 'valid' for any other -- the binding the digest exists to provide was not there. The pre-flight now reads the envelope the context already holds and sends it alongside the digest. Transport.check is an allowlist BUILDER, not a pass-through (DEF-TC29-001), so business_impact had to be added to it explicitly or it would have been dropped in transit exactly as approval_id and tool_class were. The backend already declares the field on GateRequest (internal.rs:216) and round-trips it through serde (internal.rs:6947). A context with no envelope is an LLM check with no tool to name, for which no_impact() is the correct and honest answer -- it is not a stand-in for a tool call, and the backend tells the two apart. The tests assert the CAPTURED POST BODY and include a class that goes through check_workflow_budget rather than calling Transport.check with a hand-built envelope. The first version of this file only did the latter, and a mutation restoring the sentinel in runtime.py passed all six of its tests. Mutation-verified: restoring the sentinel, and dropping business_impact from the builder, each fail the tests that own them. The sentinel mutation's own failure output is the defect -- both /tools hash 0049d93a36f0710269a6deb733ca78d57a770ef640a2698d0fddaa9653b7c3de. pytest -q: 1675 passed, 1 skipped, 0 failed. --- src/nullrun/runtime.py | 28 ++- src/nullrun/transport.py | 18 ++ tests/test_gate_business_impact_wire.py | 241 ++++++++++++++++++++++++ 3 files changed, 280 insertions(+), 7 deletions(-) create mode 100644 tests/test_gate_business_impact_wire.py diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index f116a35..cec2d47 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -2286,6 +2286,7 @@ def check_workflow_budget(self) -> None: compute_action_digest as _compute_action_digest, ) from nullrun.context import ( + get_call_impact, get_call_mcp_annotations, get_call_mcp_class, get_call_model, @@ -2355,13 +2356,26 @@ def check_workflow_budget(self) -> None: # `action_digest` is required on every /gate call. Per # `backend/src/proxy/http/gate/gate.rs:56` (ADR-023 P1-6) # the gate fail-CLOSED-rejects any proto>=3 client that - # omits the digest. We always emit a NoImpact sentinel here - # — typed Money/ToolCall impacts are forwarded by - # `runtime.execute(...)` directly (see `transport.py::execute`) - # and do not pass through this pre-flight gate. Computing - # once per call (not cached) is fine: compute_action_digest - # is ~5µs of pure stdlib. - check_req["action_digest"] = _compute_action_digest(_BusinessImpact.no_impact()) + # omits the digest. Computing once per call (not cached) is + # fine: compute_action_digest is ~5µs of pure stdlib. + # + # The envelope is the one the CONTEXT already holds, not a + # fresh one. This pre-flight decides the digest the backend + # stores on the approval row, and `runtime.execute(...)` + # later re-derives it from the request body and compares. + # Two separately built envelopes differ whenever the inputs + # differ by anything, which is why the SDK shipped a constant + # `no_impact()` here for a full release cycle and every + # post-approval re-entry failed (DEF-TC14-002, ADR-065). + # + # A context with no envelope is an LLM check with no tool to + # name, for which `no_impact()` is the correct and honest + # answer — it is NOT a stand-in for a tool call. + call_impact = get_call_impact() + if call_impact is None: + call_impact = _BusinessImpact.no_impact() + check_req["action_digest"] = _compute_action_digest(call_impact) + check_req["business_impact"] = call_impact.to_wire_dict() # Forward the tool list so backend (T3) can match each tool # against the workflow's effective `blocked_tools` aggregate. diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index abfcaa9..88bcfb6 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -1558,6 +1558,24 @@ def check( # but None" wire-shape drift. if check_request.get("action_digest"): gate_request["action_digest"] = check_request["action_digest"] + # The BusinessImpact envelope the digest was computed over. + # The backend stores `action_digest` on the approval row and, + # at /execute, recomputes it from the envelope in THAT request + # and compares (payload_binding.rs:163, orchestrator.rs:1511), + # so both endpoints have to carry the same envelope. Sending + # the digest without the envelope leaves the server unable to + # reproduce what it stored -- the /execute re-entry then fails + # CLOSED with APPROVAL_DIGEST_MISMATCH. + # + # `internal.rs:216` declares it on GateRequest and + # `internal.rs:6947` round-trips it through serde. + # + # Forwarded whenever present. `{"kind": "none"}` is a real + # value here, not an absence: it is what an LLM check with no + # tool to name sends, and the backend distinguishes it from + # an omitted envelope. + if check_request.get("business_impact") is not None: + gate_request["business_impact"] = check_request["business_impact"] # Forward the `tool_arguments` bag alongside `tool` so # the gate can hash it via `signature::compute_schema_hash` # and write the fingerprint into `mcp_tool_signatures`. diff --git a/tests/test_gate_business_impact_wire.py b/tests/test_gate_business_impact_wire.py new file mode 100644 index 0000000..8f2362b --- /dev/null +++ b/tests/test_gate_business_impact_wire.py @@ -0,0 +1,241 @@ +"""`/gate` must carry the envelope its `action_digest` was computed over. + +The backend stores `action_digest` on the approval row at `/gate` and, +at `/execute`, RECOMPUTES it from the envelope in that request and +compares (`payload_binding.rs:163`, `orchestrator.rs:1511`). A digest +without its envelope is unusable: the server has no way to reproduce +what it stored, and the re-entry fails CLOSED with +``APPROVAL_DIGEST_MISMATCH``. + +These tests assert the CAPTURED POST BODY, not the return value. +`Transport.check` is an allowlist BUILDER, not a pass-through — it +rebuilds the body from an explicit key list — so a field computed +upstream can be dropped in transit without any error surfacing. That +is exactly what happened to `approval_id` (TC-14) and to +`tool_class` / `mcp_annotations` (DEF-TC29-001, commit b64dc8a), and +a source pin would have kept passing through any refactor that +reintroduced the drop at a different line. +""" + +from __future__ import annotations + +import hashlib +import json + +import httpx +import pytest +import respx + +from nullrun.business_impact import BusinessImpact, DIGEST_PREFIX +from nullrun.context import set_call_impact +from nullrun.transport import Transport + +GATE_URL = "https://api.test.nullrun.io/api/v1/gate" +ORG_ID = "org-1" + + +def _sent_body(route) -> dict: + return json.loads(route.calls[0].request.content.decode("utf-8")) + + +def _server_recompute(envelope: dict) -> str: + """What the backend derives from an envelope it receives. + + Mirrors `server_derive_action_digest` (payload_binding.rs:163) + plus `canonicalize_json` (business_impact.rs:411-439): sort keys + recursively, compact separators, SHA-256 over prefix + bytes. + Recomputing it here rather than calling the SDK's own helper is + the point -- if both sides used the same function, the test would + pass even when the envelope on the wire was wrong. + """ + + def sort_keys(value): + if isinstance(value, dict): + return {k: sort_keys(v) for k, v in sorted(value.items())} + if isinstance(value, list): + return [sort_keys(v) for v in value] + return value + + canonical = json.dumps( + sort_keys(envelope), ensure_ascii=False, separators=(",", ":") + ).encode("utf-8") + return hashlib.sha256(DIGEST_PREFIX + canonical).hexdigest() + + +@pytest.fixture +def transport(): + t = Transport(api_url="https://api.test.nullrun.io", api_key="test-key-12345678") + yield t + t.stop() + + +@pytest.fixture(autouse=True) +def _clear_impact(): + yield + set_call_impact(None) + + +def _check(transport, **overrides) -> dict: + check_request = { + "organization_id": ORG_ID, + "execution_id": "0199aaaa-bbbb-7ccc-8ddd-eeeeffff0001", + "check_type": "llm", + "model": "claude-sonnet-4-6", + "estimated_tokens": 1, + "stream": False, + "operation_id": "0199aaaa-bbbb-7ccc-8ddd-eeeeffff0002", + } + check_request.update(overrides) + with respx.mock: + route = respx.post(GATE_URL).mock( + return_value=httpx.Response( + 200, + json={ + "decision": "allow", + "execution_id": check_request["execution_id"], + "remaining_budget_cents": 10000, + }, + ) + ) + transport.check(check_request) + assert route.called, "no /gate request was made" + return _sent_body(route) + + +class TestEnvelopeReachesTheWire: + def test_tool_call_envelope_is_forwarded(self, transport): + impact = BusinessImpact.tool_call("refund_customer", {"amount": 500}) + body = _check(transport, business_impact=impact.to_wire_dict()) + assert body["business_impact"] == { + "kind": "tool_call", + "tool_name": "refund_customer", + "params": {"amount": 500}, + "extractor_id": "nullrun.tool_call.path", + "extractor_version": "1", + } + + def test_none_envelope_is_forwarded(self, transport): + # `{"kind": "none"}` is a real value, not an absence. An LLM + # check with no tool to name sends it, and the backend tells + # it apart from an omitted envelope. + body = _check(transport, business_impact=BusinessImpact.no_impact().to_wire_dict()) + assert body["business_impact"] == {"kind": "none"} + + def test_envelope_is_omitted_when_absent(self, transport): + # Pre-0.21 callers send neither envelope nor digest. The + # backend pins the negative case too (`internal.rs:8291-8295` + # for the sibling fields) -- `null` is a different value + # carrying a different meaning. + body = _check(transport) + assert "business_impact" not in body + + +class TestTheServerCanReproduceTheDigest: + """The property that makes the approval re-entry reachable.""" + + def test_server_recompute_matches_the_digest_sent(self, transport): + impact = BusinessImpact.tool_call("refund_customer", {"amount": 500}) + from nullrun.business_impact import compute_action_digest + + body = _check( + transport, + business_impact=impact.to_wire_dict(), + action_digest=compute_action_digest(impact), + ) + assert _server_recompute(body["business_impact"]) == body["action_digest"] + + def test_server_recompute_matches_for_a_non_ascii_argument_bag(self, transport): + from nullrun.business_impact import compute_action_digest + + impact = BusinessImpact.tool_call("refund", {"note": "возврат"}) + body = _check( + transport, + business_impact=impact.to_wire_dict(), + action_digest=compute_action_digest(impact), + ) + assert _server_recompute(body["business_impact"]) == body["action_digest"] + + def test_a_tampered_envelope_does_not_reproduce_the_digest(self, transport): + # The test that would have caught DEF-TC14-002's successor: + # an attacker who swaps the envelope between /gate and + # /execute must not land on the stored digest. + from nullrun.business_impact import compute_action_digest + + impact = BusinessImpact.tool_call("refund_customer", {"amount": 500}) + tampered = BusinessImpact.tool_call("refund_customer", {"amount": 500_000}) + body = _check( + transport, + business_impact=tampered.to_wire_dict(), + action_digest=compute_action_digest(impact), + ) + assert _server_recompute(body["business_impact"]) != body["action_digest"] + + +class TestCheckWorkflowBudgetPopulatesTheEnvelope: + """The real call site, not a hand-built request. + + The tests above drive `Transport.check` with an envelope supplied + directly, so they prove the builder forwards it but say nothing + about whether `check_workflow_budget` puts one there. A mutation + that restores the `no_impact()` sentinel in `runtime.py` passes + all six of them. These go through the runtime. + """ + + @pytest.fixture + def captured_gate_bodies(self): + bodies: list[dict] = [] + + def _capture(request: httpx.Request) -> httpx.Response: + bodies.append(json.loads(request.content.decode("utf-8"))) + return httpx.Response( + 200, + json={ + "decision": "allow", + "decision_source": "gateway", + "explanation": "", + "policy_version": 1, + "explanations": [], + }, + ) + + respx.post(GATE_URL).mock(side_effect=_capture) + return bodies + + def test_context_envelope_reaches_gate(self, make_runtime, mock_api, captured_gate_bodies): + set_call_impact(BusinessImpact.tool_call("refund_customer", {"amount": 500})) + make_runtime().check_workflow_budget() + assert captured_gate_bodies, "no /gate call was captured" + body = captured_gate_bodies[-1] + assert body["business_impact"]["kind"] == "tool_call" + assert body["business_impact"]["tool_name"] == "refund_customer" + assert body["business_impact"]["params"] == {"amount": 500} + + def test_gate_digest_matches_its_own_envelope(self, make_runtime, mock_api, captured_gate_bodies): + # The two must agree ON THE WIRE. A digest the server cannot + # recompute from the body it was sent alongside is the whole + # of DEF-TC14-002. + set_call_impact(BusinessImpact.tool_call("refund_customer", {"amount": 500})) + make_runtime().check_workflow_budget() + body = captured_gate_bodies[-1] + assert _server_recompute(body["business_impact"]) == body["action_digest"] + + def test_no_context_envelope_sends_none(self, make_runtime, mock_api, captured_gate_bodies): + # An LLM check with no tool to name is legitimately + # impact-free. It must still send an envelope, because the + # backend needs one to recompute the stored digest. + make_runtime().check_workflow_budget() + body = captured_gate_bodies[-1] + assert body["business_impact"] == {"kind": "none"} + assert _server_recompute(body["business_impact"]) == body["action_digest"] + + def test_two_different_tools_produce_two_different_gate_digests( + self, make_runtime, mock_api, captured_gate_bodies + ): + rt = make_runtime() + set_call_impact(BusinessImpact.tool_call("refund_customer", {"amount": 500})) + rt.check_workflow_budget() + set_call_impact(BusinessImpact.tool_call("charge_card", {"amount": 500})) + rt.check_workflow_budget() + first, second = captured_gate_bodies[-2], captured_gate_bodies[-1] + assert first["action_digest"] != second["action_digest"] + assert first["business_impact"] != second["business_impact"] From 88ddec82762485fe8dda4db7b15e12f38a0fe363 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 11:52:34 +0400 Subject: [PATCH 10/12] fix(protect): build the tool_call envelope before the /gate pre-flight ADR-065 decision step 4. This is the change that makes the post-approval re-entry reachable at all. @protect built its envelope inside _run_tool_policy_gate, which runs as step 4 -- after the step-2 /gate pre-flight. The /gate body therefore carried a no_impact() sentinel while the /execute body carried a tool_call envelope, and the backend stores the digest from the FIRST and recomputes it from the SECOND. The envelope is now built before step 2 and both calls read it off the call context. params is the masked argument bag keyed as the /execute input already keys it ({"args": [...], "kwargs": {...}}), so the digest covers exactly what the operator's approval card showed. Two reasons for that shape rather than kwargs-only: a tool called purely positionally would otherwise have params={} and charge_card("x", 50) would be the same action as charge_card("x", 5000); and a digest over unmasked arguments would bind the grant to values nobody approved. A build failure falls back to no_impact() and logs. That is fail-OPEN on metadata and deliberately so: the backend then stores a digest that binds nothing and refuses the re-entry, which is the correct posture, whereas raising out of the decorator would take down a tool call over a metadata field. Also corrects the docstring at decorators.py:869-875 that claimed business_impact was "now always {"kind": "none"}" -- the code and the documentation disagreed, and the code is what shipped. That disagreement is what made DEF-TC14-002 read as a backend bug. The tests capture BOTH request bodies and recompute the digest the way the server does, from an independent implementation rather than by calling compute_action_digest on both sides. Mutation-verified, each independently: /execute falling back to no_impact (9 fail), the envelope omitted from /execute (9 fail), params dropping positional args (2 fail), and -- the one that first appeared to pass -- the envelope built after the pre-flight instead of before (6 fail). The first attempt at that last mutation added a second set_call_impact rather than moving the first, so it changed nothing and the suite stayed green; the reordered version is what pins the ordering ADR-065 calls out as load-bearing. pytest -q: 1690 passed, 1 skipped, 0 failed. --- src/nullrun/decorators.py | 115 +++++++-- tests/test_protect_approval_roundtrip.py | 309 +++++++++++++++++++++++ 2 files changed, 409 insertions(+), 15 deletions(-) create mode 100644 tests/test_protect_approval_roundtrip.py diff --git a/src/nullrun/decorators.py b/src/nullrun/decorators.py index 65ec4fb..b142b71 100644 --- a/src/nullrun/decorators.py +++ b/src/nullrun/decorators.py @@ -52,11 +52,14 @@ def researcher(q): from nullrun.business_impact import BusinessImpact, compute_action_digest from nullrun.context import ( _call_tools_var, + get_call_impact, get_call_tools, get_server_minted_execution_id, # for cancel-on-exception helper get_workflow_id, + reset_call_impact, reset_span_id, reset_trace_id, + set_call_impact, set_span_id, set_trace_id, ) @@ -154,6 +157,55 @@ def _safe_repr(value: object, max_len: int = 50) -> str: return r +def _build_call_impact( + fn: Callable[..., Any], + args: tuple[Any, ...], + kwargs: dict[str, Any], +) -> BusinessImpact: + """Build the BusinessImpact envelope for one protected call. + + The envelope is ``{"kind": "tool_call", "tool_name": fn.__name__, + "params": {"args": [...], "kwargs": {...}}}``. Two deliberate + choices: + + * **`params` is the MASKED argument bag**, and it is keyed the same + way the ``/execute`` request body keys its ``input``. The + operator's approval card shows the masked values, so a digest + computed over anything else — the unmasked arguments, or only + the kwargs — would bind the grant to arguments nobody approved. + A tool called purely positionally still gets both keys, so + ``charge_card("4111...", 50)`` is not indistinguishable from + ``charge_card("4111...", 5000)``. + + * **The tool name is inside the hashed bytes** (it is the + envelope's ``tool_name``), so a grant for ``refund_customer`` + does not validate a replay of ``charge_card``. + + Falls back to ``no_impact()`` when the envelope cannot be built + (an unprintable tool name, a parameter value the digest layer + cannot round-trip). That is a fail-OPEN posture and is + deliberately loud: the backend will store a digest that binds + nothing, so the re-entry is refused. The alternative — raising out + of the decorator before the gate — would take down a tool call + over a metadata field, which is a worse failure than a refused + approval. See ADR-065 "Consequences". + """ + try: + return BusinessImpact.tool_call( + fn.__name__, + {"args": _safe_args(fn, args), "kwargs": _safe_kwargs(kwargs)}, + ) + except (ValueError, TypeError) as exc: + logger.warning( + "@protect for %r: could not build a tool_call BusinessImpact (%s); " + "falling back to no_impact(), which binds the approval to nothing. " + "The post-approval /execute re-entry will be refused.", + fn.__name__, + exc, + ) + return BusinessImpact.no_impact() + + def _safe_kwargs(kwargs: dict[str, Any]) -> dict[str, Any]: """Mask sensitive kwargs (case-insensitive).""" return { @@ -628,6 +680,20 @@ def _protect_body(args: tuple[Any, ...], kwargs: dict[str, Any], unify_block: bo ) else: call_tools_token = None + # The BusinessImpact envelope for THIS protected call, set + # BEFORE the /gate pre-flight below. The backend stores the + # digest it derives from the /gate body's envelope and, at + # /execute, recomputes it from the /execute body's envelope + # and compares (payload_binding.rs:163, orchestrator.rs:1511), + # so the two have to be the same envelope — which means it has + # to exist before step 2, not be minted during step 4. + # + # Token-based, like the tools contextvar above: a nested + # @protect restores the outer envelope on exit, and a bare + # @protect leaves the contextvar empty again. + call_impact_token: Token[Any] | None = set_call_impact( + _build_call_impact(fn, args, kwargs) + ) error: BaseException | None = None try: # the runtime can warn when @protect fires often but no @@ -708,6 +774,11 @@ def _protect_body(args: tuple[Any, ...], kwargs: dict[str, Any], unify_block: bo # contextvar empty again (the default). if call_tools_token is not None: _call_tools_var.reset(call_tools_token) + # The envelope is scoped to this protected call. Leaving + # it set would make an unrelated LLM check that happens + # later report a tool_call impact for a call that has no + # tool in it. + reset_call_impact(call_impact_token) _emit_span_end( runtime, span, @@ -868,25 +939,39 @@ def _run_tool_policy_gate( ## Wire contract Same fields on /execute as before: ``tool_name``, - ``{"args": masked_args, "kwargs": masked}``, ``business_impact`` - (now always ``{"kind": "none"}``), ``action_digest`` (SHA-256 - over the canonical NoImpact envelope; pinned, deterministic), - ``tools``. Backend unchanged — only the SDK's interpretation of - what to put in ``business_impact`` simplified. + ``{"args": masked_args, "kwargs": masked}``, ``business_impact``, + ``action_digest`` (SHA-256 over that same envelope; pinned and + deterministic), ``tools``. + + ``business_impact`` is a ``{"kind": "tool_call", ...}`` envelope + naming this tool and its masked argument bag, built once by + ``_build_call_impact`` before the /gate pre-flight and read + unchanged here. It was ``{"kind": "none"}`` on every call until + ADR-065; a constant hashes to a constant, so the digest the + backend stored bound the approval to nothing at all. Backend + unchanged. """ masked = _safe_kwargs(kwargs) masked_args = _safe_args(fn, args) - # Wire-shape compatibility: ``business_impact`` stays None - # on /execute when no per-tool typed impact is extracted - # (the bare @protect shape — backend reads only - # ``action_digest`` + ``kwargs`` for ToolParameters Approval - # Rules). The ``action_digest`` is still computed against - # the canonical NoImpact envelope so the Phase-1+ wire-shape - # ``tests/test_business_impact.py``. - no_impact = BusinessImpact.no_impact() - business_impact_dict: dict[str, Any] | None = None - action_digest_hex: str = compute_action_digest(no_impact) + # The envelope for this logical action, built ONCE by + # `_build_call_impact` before the /gate pre-flight and put on the + # call context. Reading it here — rather than minting a second one + # — is the whole point: the backend stores the digest it derived + # from the /gate body's envelope and recomputes it from this + # request's envelope, so the two must be the same object. + # + # A context with no envelope (a direct `runtime.execute(...)` + # call that never went through @protect) falls back to + # no_impact(). The backend then has no envelope to recompute + # against and refuses the re-entry — fail-CLOSED on an unbound + # approval, which is the correct posture for an approval whose + # trust binding the SDK could not reproduce. + call_impact = get_call_impact() + if call_impact is None: + call_impact = BusinessImpact.no_impact() + business_impact_dict: dict[str, Any] | None = call_impact.to_wire_dict() + action_digest_hex: str = compute_action_digest(call_impact) from nullrun.breaker.exceptions import ( NullRunBlockedException, diff --git a/tests/test_protect_approval_roundtrip.py b/tests/test_protect_approval_roundtrip.py new file mode 100644 index 0000000..e755ae5 --- /dev/null +++ b/tests/test_protect_approval_roundtrip.py @@ -0,0 +1,309 @@ +"""`@protect`: /gate and /execute must carry the SAME envelope. + +ADR-065 decision step 4 and verification bullet 2. + +The backend does not trust the `action_digest` the SDK sends. At +`/execute` it RECOMPUTES the digest from the `business_impact` in +that request and compares it to the digest STORED on the approval row +at `/gate` time (`payload_binding.rs:163`, `orchestrator.rs:1511`). +So the two requests have to carry the same envelope — which is why +`@protect` builds it once, before the pre-flight, and both calls read +it off the call context. + +Until ADR-065 both carried a constant `{"kind": "none"}`. That is +DEF-TC14-002: the operator approves, and the agent still cannot run +the tool — and even if it could, the grant was bound to nothing, so +it was replayable as any other action. + +These assert the CAPTURED request bodies of both endpoints. A digest +is opaque on its own, so the round-trip is checked the way the server +checks it: recompute from the envelope the body actually carried and +compare to the digest the same body carried. +""" + +from __future__ import annotations + +import hashlib +import json + +import httpx +import pytest +import respx + +import nullrun +from nullrun.business_impact import BusinessImpact, DIGEST_PREFIX + +BASE_URL = "https://api.test.nullrun.io" + + +def _server_recompute(envelope: dict) -> str: + """What the backend derives from an envelope it received. + + Deliberately NOT the SDK's own helper: if both sides called + `compute_action_digest`, the test would pass even with the wrong + envelope on the wire. + """ + + def sort_keys(value): + if isinstance(value, dict): + return {k: sort_keys(v) for k, v in sorted(value.items())} + if isinstance(value, list): + return [sort_keys(v) for v in value] + return value + + canonical = json.dumps( + sort_keys(envelope), ensure_ascii=False, separators=(",", ":") + ).encode("utf-8") + return hashlib.sha256(DIGEST_PREFIX + canonical).hexdigest() + + +@pytest.fixture +def captured(): + """Capture the bodies of both gate endpoints. Both answer allow.""" + bodies: dict[str, list[dict]] = {"gate": [], "execute": []} + + def _gate(request: httpx.Request) -> httpx.Response: + bodies["gate"].append(json.loads(request.content.decode("utf-8"))) + return httpx.Response( + 200, + json={ + "decision": "allow", + "actions": [], + "local_cost_cents": 0, + "policy_id": "policy-test", + "decision_source": "gateway", + }, + ) + + def _execute(request: httpx.Request) -> httpx.Response: + bodies["execute"].append(json.loads(request.content.decode("utf-8"))) + return httpx.Response( + 200, + json={ + "decision": "allow", + "decision_source": "gateway", + "explanation": "allowed", + "policy_version": 1, + }, + ) + + respx.post(f"{BASE_URL}/api/v1/gate").mock(side_effect=_gate) + respx.post(f"{BASE_URL}/api/v1/execute").mock(side_effect=_execute) + return bodies + + +def _last(captured, endpoint): + assert captured[endpoint], f"no /{endpoint} call was captured" + return captured[endpoint][-1] + + +class TestBothEndpointsCarryOneEnvelope: + def test_envelopes_are_identical(self, make_runtime, mock_api, captured): + @nullrun.protect + def refund_customer(amount: int, currency: str = "EUR") -> str: + return "ok" + + make_runtime() + assert refund_customer(amount=500) == "ok" + + gate = _last(captured, "gate") + execute = _last(captured, "execute") + assert gate["business_impact"] == execute["business_impact"] + + def test_digests_are_identical(self, make_runtime, mock_api, captured): + @nullrun.protect + def refund_customer(amount: int) -> str: + return "ok" + + make_runtime() + assert refund_customer(amount=500) == "ok" + assert _last(captured, "gate")["action_digest"] == _last(captured, "execute")[ + "action_digest" + ] + + def test_server_can_recompute_the_gate_digest(self, make_runtime, mock_api, captured): + # What the backend does when it stores the approval row. + @nullrun.protect + def refund_customer(amount: int) -> str: + return "ok" + + make_runtime() + refund_customer(amount=500) + gate = _last(captured, "gate") + assert _server_recompute(gate["business_impact"]) == gate["action_digest"] + + def test_server_can_recompute_the_execute_digest(self, make_runtime, mock_api, captured): + # What the backend does at /execute, which is where DEF-TC14-002 + # failed: 400 BUSINESS_IMPACT_INVALID when the envelope was + # absent, 422 VALIDATION_ERROR when it was the `none` sentinel. + @nullrun.protect + def refund_customer(amount: int) -> str: + return "ok" + + make_runtime() + refund_customer(amount=500) + execute = _last(captured, "execute") + assert execute["business_impact"] is not None + assert _server_recompute(execute["business_impact"]) == execute["action_digest"] + + def test_stored_and_recomputed_digests_agree(self, make_runtime, mock_api, captured): + # The end-to-end property, stated as the server states it. + @nullrun.protect + def refund_customer(amount: int) -> str: + return "ok" + + make_runtime() + refund_customer(amount=500) + stored = _last(captured, "gate") + recomputed = _last(captured, "execute") + assert _server_recompute(recomputed["business_impact"]) == stored["action_digest"] + + +class TestTheEnvelopeNamesTheAction: + def test_tool_name_is_the_wrapped_function(self, make_runtime, mock_api, captured): + @nullrun.protect + def charge_card(amount: int) -> str: + return "ok" + + make_runtime() + charge_card(amount=500) + assert _last(captured, "execute")["business_impact"]["tool_name"] == "charge_card" + + def test_params_carry_the_masked_kwargs(self, make_runtime, mock_api, captured): + @nullrun.protect + def charge_card(amount: int) -> str: + return "ok" + + make_runtime() + charge_card(amount=500) + params = _last(captured, "execute")["business_impact"]["params"] + assert params["kwargs"]["amount"] == "500" + assert params["args"] == [] + + def test_sensitive_kwargs_are_masked_in_the_digested_params( + self, make_runtime, mock_api, captured + ): + # The digest must cover what the OPERATOR saw on the approval + # card, which is the masked bag — never the raw secret. + @nullrun.protect + def charge_card(credit_card_number: str, amount: int) -> str: + return "ok" + + make_runtime() + charge_card(credit_card_number="4111111111111111", amount=500) + params = _last(captured, "execute")["business_impact"]["params"] + assert params["kwargs"]["credit_card_number"] == "***" + assert "4111111111111111" not in json.dumps(params) + + def test_positional_arguments_are_covered(self, make_runtime, mock_api, captured): + # A tool called purely positionally must still bind its + # arguments, or charge_card("x", 50) and charge_card("x", 5000) + # would be the same action. + @nullrun.protect + def charge_card(credit_card_number: str, amount: int) -> str: + return "ok" + + make_runtime() + charge_card("4111111111111111", 50) + assert _last(captured, "execute")["business_impact"]["params"]["args"] == [ + "***", + "50", + ] + + +class TestTheBindingActuallyBinds: + """The property NR-010 was raised to protect.""" + + def _digest_for(self, make_runtime, captured, call): + make_runtime() + call() + return _last(captured, "gate")["action_digest"] + + def test_a_different_amount_is_a_different_action(self, make_runtime, mock_api, captured): + @nullrun.protect + def refund_customer(amount: int) -> str: + return "ok" + + rt = make_runtime() + refund_customer(amount=500) + approved = _last(captured, "gate")["action_digest"] + refund_customer(amount=500_000) + tampered = _last(captured, "gate")["action_digest"] + assert approved != tampered + + def test_a_different_tool_is_a_different_action(self, make_runtime, mock_api, captured): + @nullrun.protect + def refund_customer(amount: int) -> str: + return "ok" + + @nullrun.protect + def charge_card(amount: int) -> str: + return "ok" + + make_runtime() + refund_customer(amount=500) + approved = _last(captured, "gate")["action_digest"] + charge_card(amount=500) + assert _last(captured, "gate")["action_digest"] != approved + + def test_a_tampered_replay_does_not_land_on_the_approved_digest( + self, make_runtime, mock_api, captured + ): + # The exact shape of the attack ADR-065 closes: reuse the + # approval_id/digest granted for amount=500 to run amount=5000. + @nullrun.protect + def refund_customer(amount: int) -> str: + return "ok" + + make_runtime() + refund_customer(amount=500) + approved = _last(captured, "gate")["action_digest"] + refund_customer(amount=5000) + replay = _last(captured, "execute") + assert _server_recompute(replay["business_impact"]) != approved + + def test_a_tool_call_is_not_the_none_sentinel(self, make_runtime, mock_api, captured): + from nullrun.business_impact import compute_action_digest + + sentinel = compute_action_digest(BusinessImpact.no_impact()) + + @nullrun.protect + def refund_customer(amount: int) -> str: + return "ok" + + make_runtime() + refund_customer(amount=500) + assert _last(captured, "gate")["action_digest"] != sentinel + assert _last(captured, "execute")["action_digest"] != sentinel + + +class TestEnvelopeIsScopedToTheCall: + def test_envelope_is_cleared_after_the_call(self, make_runtime, mock_api, captured): + from nullrun.context import get_call_impact + + @nullrun.protect + def refund_customer(amount: int) -> str: + return "ok" + + make_runtime() + refund_customer(amount=500) + # Leaving it set would make an unrelated LLM check report a + # tool_call impact for a call that has no tool in it. + assert get_call_impact() is None + + def test_nested_protect_restores_the_outer_envelope(self, make_runtime, mock_api, captured): + from nullrun.context import get_call_impact + + @nullrun.protect + def inner(amount: int) -> str: + return "ok" + + @nullrun.protect + def outer(amount: int) -> str: + return inner(amount=amount) + + make_runtime() + outer(amount=500) + assert get_call_impact() is None + envelopes = [b["business_impact"]["tool_name"] for b in captured["execute"]] + assert envelopes == ["outer", "inner"] From d11eb78e42fd714b3a240772fb6bb6b5daee1fce Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 11:54:04 +0400 Subject: [PATCH 11/12] test(protect): pin the unbuildable-envelope degradation ADR-065 retires the none sentinel FROM THE TOOL PATH, which is only true modulo _build_call_impact's fallback: a tool name the backend's validator rejects (non-ASCII, over 128 bytes) degrades to no_impact() and logs. The tool body still runs; the approval carries no trust binding and the server refuses the re-entry. That branch was previously untested, which made "retired from the tool path" a claim rather than a fact. A realistic trigger is a non-ASCII tool name -- the backend requires printable ASCII (business_impact.rs:320-323), so such a tool cannot be described by a valid envelope at all. Mutation-verified: replacing the fallback with a fabricated tool_call envelope fails 3 of the 4 new tests. pytest -q: 1694 passed, 1 skipped, 0 failed. --- tests/test_protect_approval_roundtrip.py | 57 ++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tests/test_protect_approval_roundtrip.py b/tests/test_protect_approval_roundtrip.py index e755ae5..4ae83f6 100644 --- a/tests/test_protect_approval_roundtrip.py +++ b/tests/test_protect_approval_roundtrip.py @@ -307,3 +307,60 @@ def outer(amount: int) -> str: assert get_call_impact() is None envelopes = [b["business_impact"]["tool_name"] for b in captured["execute"]] assert envelopes == ["outer", "inner"] + + +class TestUnbuildableEnvelopeDegradesLoudly: + """`_build_call_impact` can fall back to no_impact(). + + ADR-065 retires the `none` sentinel FROM THE TOOL PATH, which is + only true modulo this branch: a tool name the backend's validator + rejects (non-ASCII, over 128 bytes) or an argument the digest layer + cannot round-trip falls back to `no_impact()` and logs. The + backend then stores a digest that binds nothing and refuses the + re-entry — fail-CLOSED on the server, fail-OPEN in the SDK's own + metadata. That trade is deliberate and is pinned here so it stays + deliberate rather than becoming an unnoticed hole. + """ + + def test_non_ascii_tool_name_degrades_instead_of_raising(self, make_runtime, mock_api, captured): + # The backend's `ToolCallParams::validate` requires printable + # ASCII (business_impact.rs:320-323). A tool named in Cyrillic + # cannot be described by a valid envelope at all. + from nullrun.decorators import _build_call_impact + + def refund_клиент(): + pass + + impact = _build_call_impact(refund_клиент, (), {}) + assert impact.kind == "none" + + def test_overlong_tool_name_degrades(self, make_runtime, mock_api, captured): + from nullrun.decorators import _build_call_impact + + def tool(): + pass + + tool.__name__ = "t" * 129 + assert _build_call_impact(tool, (), {}).kind == "none" + + def test_a_valid_tool_never_degrades(self, make_runtime, mock_api, captured): + from nullrun.decorators import _build_call_impact + + def refund_customer(amount: int): + pass + + assert _build_call_impact(refund_customer, (), {"amount": 500}).kind == "tool_call" + + def test_degraded_tool_still_runs_but_binds_nothing(self, make_runtime, mock_api, captured): + # Documented consequence: the body is NOT blocked, the approval + # simply carries no trust binding, so /execute is refused. + @nullrun.protect + def refund_клиент(amount: int) -> str: + return "ok" + + make_runtime() + assert refund_клиент(amount=500) == "ok" + from nullrun.business_impact import compute_action_digest + + sentinel = compute_action_digest(BusinessImpact.no_impact()) + assert _last(captured, "gate")["action_digest"] == sentinel From f8911c4a0c1368d89168f67c40508d666317b792 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Fri, 2 Oct 2026 12:13:33 +0400 Subject: [PATCH 12/12] =?UTF-8?q?chore(release):=200.21.0=20=E2=80=94=20AD?= =?UTF-8?q?R-065=20envelope=20restoration=20+=20typed=20/gate=20refusals?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cuts `v0.21.0` — the SDK-side closure of `DEF-TC14-002` (QA cycle RUN_ID `20261002T0826`, 2026-10-02). Two QA cycles in a row found the same defect from two angles: 0.20.0's audit said "operator approves an action and the agent still cannot run it", and this cycle's TC-4 said "/gate blocks on a rate limit but the SDK raises `NullRunBudgetError` so callers branch on the wrong cause". Both were the same root: the SDK was sending a constant sentinel instead of the business impact envelope the backend was looking for, and was classifying refusals by what the SDK's wrapper assumed rather than by what the wire said. The load-bearing fix is the five-step restoration of the `BusinessImpact` envelope (ADR-065). Since 0.18.5 every `@protect` call sent a constant `{"kind":"none"}` sentinel; a constant hashes to a constant, so the digest the backend stored on the approval row at `/gate` matched the digest it recomputed at `/execute` for every tool — while binding the approval to nothing at all. The operator's approval card did not correspond to any particular action, and the backend's refuse-the-reentry check had no data to refuse *against*. ## Migration (read first if upgrading from 0.20.x) Three things differ from 0.20.0. None are silent on a healthy configuration, but each can be a working loop becoming a throwing one for code that suppressed the failure before. 1. `BusinessImpact.tool_call()` is back. Importable from `nullrun.business_impact`. The constructor emits exactly what the backend's internally-tagged serde produces, including `extractor_id` and `extractor_version` (the backend has no `skip_serializing_if` on these, so they are inside the hashed bytes and must be present here too). The shared fixture digest is byte-identical to the backend's `DIGEST_FIXTURE_HEX_TOOL_CALL` (`9975a8b7…6ed0526966a6`). 2. The v3 `/track` single path raises typed enforcement rejections instead of dropping them. A 422 `CONSUME_OVERBUDGET` used to be a WARNING log line and a return value of `TRACK_OK={'allowed': True, 'actions': [], 'local_cost_cents': 0}` — the call was treated as successful at the agent layer. It now raises `NullRunConsumeOverbudgetError` carrying `reserved_cents`, `actual_cost_cents`, `max_allowed_cents` and `epsilon_cents`. Network errors and 5xx that name no enforcement failure still drop and log; widening the raise to those would freeze the agent loop on a dead backend, which is the failure mode the fail-OPEN rows exist to prevent. 3. The /gate pre-flight now types its refusal instead of assuming budget. A rate-limit block (NR-R002), a tool-block (NR-T003) and a circuit-breaker trip (NR-B010) used to reach the caller as `NullRunBudgetError` (NR-B004) — the operator reads NR-B004 as "raise the spend cap", the actual cause is a throttle policy. The dispatcher routes by wire code: `RATE_LIMIT_EXCEEDED` → `NullRunRateLimitError`, `TOOL_BLOCKED` → `NullRunToolBlockedError`, `CIRCUIT_BREAKER_TRIPPED` → typed breaker class. A response with no machine-readable code still raises `NullRunBudgetError` (the legacy tier is pinned). `cost_limit_exceeded` is bumped only for `NullRunBudgetError`, so a rate-limit block no longer over-counts the spend cap. Carried over from 0.20.0 and still true on 0.21.0 — same class of break, same shape of fix: - An unclassifiable refusal now raises where 0.19.0 let the call proceed. `NullRunUnclassifiedRefusalError` is a sibling of `NullRunTransportError` (not a subclass), so an existing `except NullRunTransportError:` arm will not catch it. - `NullRunRuntime.execute(..., mode="inline")` is gone. - `register_strict_mode_forced` / `is_strict_mode_forced` / `@guarded` / `nullrun.handle` / `nullrun.status()` / `nullrun.auto_instrument` are all gone (last touched in 0.18.5–0.20.0). ## Security - **ADR-065 (DEF-TC14-002)** — `@protect` binds approvals to nothing. Fix in five steps: (1) `BusinessImpact.tool_call()` is restored with `extractor_id` / `extractor_version`; (2) the envelope is carried on the call context as a contextvar, the same home `set_call_context` uses for the model and the tool list; (3) `Transport.check` reads the envelope and sends it alongside the digest (the allowlist builder was dropping it before); (4) `@protect` builds the envelope before the `/gate` pre-flight — building it after means the two calls would carry different envelopes, and the backend would refuse every re-entry as a side effect; (5) a build failure (non-ASCII tool name, > 128 bytes) degrades to `no_impact()` and logs, so the tool still runs while the approval carries no trust binding and the server refuses the re-entry. (`4320ac0` + `e3176d9` + `015407b` + `88ddec8` + `d11eb78`) - **DEF-TC29-001** — `Transport.check` was dropping `tool_class` and `mcp_annotations` from the `/gate` body. The MCP integration had been computing them for the call context since it landed, but the transport's explicit allowlist builder did not include them, so a destructive MCP tool arrived at the gate as `tool_class=None, mcp_annotations=None` — the negative case the backend pins, not the positive case the public `set_mcp_tool_context` API implied. Now sent unconditionally when set; absent means "unknown", not "false". (`b64dc8a`) - **DEF-TC4-001** — `/gate` pre-flight was raising `NullRunBudgetError` for every refusal. The pre-flight now routes through `_build_block_exception` and resolves the wire code in the same order the backend resolves the HTTP status: `details["error_code"]`, then the top-level `error_code` that `Transport.check` already copies onto its 4xx dict, then `explanation`. Two backend codes that had drifted out of the SDK catalog (`BUDGET_WORKFLOW_BLOCKED`, `402`; `BUDGET_CACHE_EXCEEDED`, `402`) are registered in the companion commit; the backend logged `BUDGET_WORKFLOW_BLOCKED` x389 in production before that registration, so the wire had been answering questions the SDK could not classify. (`4ec5460` + `76efa7b`) - **DEF-TC6-006** — `_route_track` was wrapping `transport.track_single` in a bare `except Exception` that logged at WARNING and returned. The transport layer had already classified the response — a 422 `CONSUME_OVERBUDGET` becomes a typed `NullRunConsumeOverbudgetError` — and the catch discarded it. The drop-and-log policy the catch implements is the one the ADR-008 table states for the `/track` batch path, a NETWORK error; the v3 single path has no such row. The fix re-raises `NullRunDecision` after the existing cache invalidation and telemetry, so the blast-radius mitigation (`DEF-CACHE-STALE-ALLOW-AFTER-OVERBUDGET`) is not traded away for the reporting fix. (`ea7c9ee`) ## Fixed - **DEF-TC21-001** — `WorkflowKilledInterrupt` was documented as `BaseException`-only in three places (`docs/errors/NR-W002.md`, `src/nullrun/breaker/exceptions.py`'s class catalog, the `NullRunError` docstring), but the class has been an `Exception` subclass since 0.16.6's `BreakerError` reparenting (`9877c34`). Only the prose was wrong. `docs/errors/NR-W002.md` also pointed at `docs/kill-contract.md` §6, a file that does not exist. `tests/test_exception_hierarchy.py` had the same disease: the test was named `test_killed_interrupt_does_not_inherit_from_exception` while asserting `issubclass(WorkflowKilledInterrupt, Exception)`. Renamed. (`6108308`) - **DEF-TC6-005** — `status().ws_connected` was structurally pinned to `None`. `WebSocketConnection` has an `_running` flag (set in `_connect`, cleared by the receive loop's `finally`); the SDK was reading `is_open` via `getattr(..., None)`. `is_open` appears exactly once in the SDK: on the reading side, with no writer, no test and no producer — so the `getattr` default fired on every call and the three states (never-established / live / dropped) collapsed to one. The fourth test in the new file asserts that the attribute `status()` reads exists on a really-constructed `WebSocketConnection` AND that `is_open` does not — a stubbed connection cannot catch it, because the stub would carry whatever attribute the test author assumed. (`9ccf168`) ## Tooling - `4bbb54e` — `style: clear the three lint errors this branch's own test files carry`. Two F841 in `tests/test_protect_approval_roundtrip.py` (the `rt = make_runtime()` binding was never used; `@protect` resolves the runtime from the context, not the local) plus one I001 (DIGEST_PREFIX is upper-case so the import sort moves it ahead of BusinessImpact). ruff CI runs over `src/` AND `tests/`, so all three would have failed a build. ## Verification | Check | Result | |---|---| | `ruff check src tests` | All checks passed | | `mypy src/nullrun` | Success: no issues found in 37 source files | | `pytest -q` | **1694 passed, 1 skipped** in 109.64s (vs baseline 1597 / 1 — **+97 new tests**) | | Scratch diff | clean (no `dist_local/`, no `*.defect*`) | | `nullrun.__version__` | `0.21.0` | | Wire-format | additive on `/gate` (carries `business_impact`, `tool_class`, `mcp_annotations` when set; absent means "unknown", not "false"); non-additive on `/track` (the v3 single path now raises typed enforcement rejections where it dropped them — caller-observable) | ## Commits included ``` 4bbb54e style: clear the three lint errors this branch's own test files carry d11eb78 test(protect): pin the unbuildable-envelope degradation 88ddec8 fix(protect): build the tool_call envelope before the /gate pre-flight 015407b feat(gate): send the context envelope at /gate instead of a sentinel e3176d9 feat(sdk): carry one BusinessImpact envelope per logical action 4320ac0 feat(sdk): restore the tool_call BusinessImpact constructor 6108308 docs(kill): stop claiming the kill signal is BaseException-only b64dc8a fix(mcp): forward tool class and annotations to /gate ea7c9ee fix(track): propagate enforcement rejections from the v3 /track path 9ccf168 fix(sdk): read the attribute the WS connection actually has 76efa7b fix(sdk): type the /gate pre-flight refusal instead of assuming budget 4ec5460 fix(sdk): register the two backend budget codes in the SDK catalog ``` (The version-bump commit and the `uv.lock` stamp `0.20.0 → 0.21.0` are folded into this release commit.) --- CHANGELOG.md | 215 +++++++++++++++++++++++ pyproject.toml | 2 +- src/nullrun/__version__.py | 2 +- tests/test_gate_business_impact_wire.py | 2 +- tests/test_protect_approval_roundtrip.py | 4 +- uv.lock | 2 +- 6 files changed, 221 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c595b01..377911f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,218 @@ +## [0.21.0] - 2026-10-02 + +Closes the round-trip `DEF-TC14-002` (QA cycle RUN_ID 20261002T0826) +on the SDK side. Two QA cycles in a row found the same defect from +two angles: 0.20.0's audit said "operator approves an action and the +agent still cannot run it", and this cycle's TC-4 said "/gate blocks +on a rate limit but the SDK raises `NullRunBudgetError` so callers +branch on the wrong cause". Both were the same root: the SDK was +sending a constant sentinel instead of the business impact envelope +the backend was looking for, and was classifying refusals by what +the SDK's wrapper assumed rather than by what the wire said. + +Minor, not patch: 0.18.5's deprecation sweep deleted the +`BusinessImpact.tool_call()` constructor along with the curated +surface, and 0.21.0 restores it. The class shape is unchanged, but +the re-export path is — see Migration #1. The /track path now raises +typed enforcement rejections where it dropped them silently (see +Migration #2), and the /gate pre-flight now routes through the typed +dispatcher so a rate-limit refusal does not present as +`NullRunBudgetError` (see Migration #3). + +### Migration + +Three things differ from 0.20.0. None are silent on a healthy +configuration, but each can be a working loop becoming a throwing +one for code that suppressed the failure before. + +1. **`BusinessImpact.tool_call()` is back.** Importable from + `nullrun.business_impact`. The 0.18.5 deprecation sweep + (aee8110) deleted it along with the curated surface reduction + while the module docstring kept claiming it was there — the + same prose/code disagreement that made the round-trip + audit-difference read as a backend bug. The constructor emits + exactly what the backend's internally-tagged serde produces, + including `extractor_id` and `extractor_version` (the backend + has no `skip_serializing_if` on these, so they are inside the + hashed bytes and must be present here too). If you imported it + from a private path, the import no longer needs to be private. +2. **The v3 `/track` single path raises typed enforcement + rejections instead of dropping them.** A 422 `CONSUME_OVERBUDGET` + used to be a WARNING log line and a return value of + `TRACK_OK={'allowed': True, 'actions': [], 'local_cost_cents': 0}` + — the call was treated as successful at the agent layer. It now + raises `NullRunConsumeOverbudgetError` carrying `reserved_cents`, + `actual_cost_cents`, `max_allowed_cents` and `epsilon_cents`. + Network errors and 5xx that name no enforcement failure still + drop and log; widening the raise to those would freeze the agent + loop on a dead backend, which is the failure mode the fail-OPEN + rows exist to prevent. +3. **The /gate pre-flight now types its refusal instead of + assuming budget.** A rate-limit block, a tool-block and a + circuit-breaker trip used to reach the caller as + `NullRunBudgetError` (NR-B004). The dispatcher routes by wire + code: `RATE_LIMIT_EXCEEDED` → `NullRunRateLimitError`, + `TOOL_BLOCKED` → `NullRunToolBlockedError`, `CIRCUIT_BREAKER_TRIPPED` + → typed breaker class. A response with no machine-readable code + still raises `NullRunBudgetError` (the legacy tier is pinned). + `cost_limit_exceeded` is bumped only for `NullRunBudgetError`, + so a rate-limit block no longer over-counts the spend cap. + +Carried over from 0.20.0 and still true on 0.21.0 — the same class +of break and the same shape of fix: + +- **An unclassifiable refusal now raises where 0.19.0 let the call + proceed.** `NullRunUnclassifiedRefusalError` is a sibling of + `NullRunTransportError` (not a subclass), so an existing + `except NullRunTransportError:` arm will not catch it. +- **`NullRunRuntime.execute(..., mode="inline")` is gone.** +- **`register_strict_mode_forced` / `is_strict_mode_forced` / + `@guarded` / `nullrun.handle` / `nullrun.status()` / + `nullrun.auto_instrument`** are all gone (last touched in + 0.18.5–0.20.0). + +### Security + +- **ADR-065 (DEF-TC14-002)** — `@protect` binds approvals to + nothing. Since 0.18.5 every `@protect` call sent a constant + `{"kind":"none"}` sentinel; a constant hashes to a constant, so + the digest the backend stored on the approval row at `/gate` + matched the digest it recomputed at `/execute` for every tool — + while binding the approval to nothing at all. The backend's + refuse-the-reentry check therefore had no data to refuse + *against*, and the operator's approval card did not correspond + to any particular action. The fix is in five steps: + (1) `BusinessImpact.tool_call()` is restored with + `extractor_id` / `extractor_version`; (2) the envelope is + carried on the call context as a contextvar, the same home + `set_call_context` uses for the model and the tool list; + (3) `Transport.check` reads the envelope and sends it alongside + the digest (the allowlist builder was dropping it before); + (4) `@protect` builds the envelope before the `/gate` + pre-flight — building it after means the two calls would carry + different envelopes, and the backend would refuse every + re-entry as a side effect; (5) a build failure (non-ASCII tool + name, > 128 bytes) degrades to `no_impact()` and logs, so the + tool still runs while the approval carries no trust binding and + the server refuses the re-entry. That is a deliberate fail-OPEN + on metadata — raising out of the decorator would take down a + tool call over a metadata field. A `compute_action_digest` + refactor exposes the canonical bytes for direct assertion; the + shared fixture digest is byte-identical to the backend's + `DIGEST_FIXTURE_HEX_TOOL_CALL` (`9975a8b7…6ed0526966a6`). +- **DEF-TC29-001** — `Transport.check` was dropping `tool_class` + and `mcp_annotations` from the `/gate` body. The MCP integration + had been computing them for the call context since it landed, + but the transport's explicit allowlist builder did not include + them, so a destructive MCP tool arrived at the gate as + `tool_class=None, mcp_annotations=None` — the negative case the + backend pins, not the positive case the public + `set_mcp_tool_context` API implied. `effective_tool_class()` + falls back to name-based classification on the negative case, + so this is a dead feature with a misleading API today — but the + day the server-side flag flips, destructive MCP tools will + silently degrade without a wire-level signal. Now sent + unconditionally when set; absent means "unknown", not "false". +- **DEF-TC4-001** — `/gate` pre-flight was raising + `NullRunBudgetError` for every refusal. A rate-limit block + (NR-R002), a tool-block (NR-T003) and a circuit-breaker trip + (NR-B010) all reached the caller as NR-B004 "budget exhausted", + which sends the operator looking for a spend-cap misconfiguration + when the actual cause is a throttle policy. The pre-flight now + routes through `_build_block_exception` and resolves the wire + code in the same order the backend resolves the HTTP status: + `details["error_code"]`, then the top-level `error_code` that + `Transport.check` already copies onto its 4xx dict, then + `explanation`. The dispatcher handles all three catalog families + (decision / transport / infra), not just the + `NullRunBlockedException` one — `source` is never forwarded + through `**details` because it collides with the keyword the + class passes down itself. Two backend codes that had drifted + out of the SDK catalog (`BUDGET_WORKFLOW_BLOCKED`, `402`; + `BUDGET_CACHE_EXCEEDED`, `402`) are registered in the + companion commit; the backend logged `BUDGET_WORKFLOW_BLOCKED` + x389 in production before that registration, so the wire had + been answering questions the SDK could not classify. +- **DEF-TC6-006** — `_route_track` was wrapping `transport.track_single` + in a bare `except Exception` that logged at WARNING and + returned. The transport layer had already classified the + response — a 422 `CONSUME_OVERBUDGET` becomes a typed + `NullRunConsumeOverbudgetError` — and the catch discarded it. + The drop-and-log policy the catch implements is the one the + ADR-008 table states for the `/track` batch path, a NETWORK + error; the v3 single path has no such row. The fix re-raises + `NullRunDecision` after the existing cache invalidation and + telemetry, so the blast-radius mitigation + (`DEF-CACHE-STALE-ALLOW-AFTER-OVERBUDGET`) is not traded away + for the reporting fix. + +### Fixed + +- **DEF-TC21-001** — `WorkflowKilledInterrupt` was documented as + `BaseException`-only in three places (`docs/errors/NR-W002.md`, + `src/nullrun/breaker/exceptions.py`'s class catalog, the + `NullRunError` docstring), but the class has been an `Exception` + subclass since 0.16.6's `BreakerError` reparenting + (`9877c34`). The behaviour is correct and deliberate — agent + recovery is meant to catch a kill and surface the structured + `error_code` / `user_action`; `tests/test_decision_split.py` + documents the override. Only the prose was wrong, and it was + wrong in the direction that would lead the next maintainer to + revert working code. `docs/errors/NR-W002.md` also pointed at + `docs/kill-contract.md` §6, a file that does not exist. + `tests/test_exception_hierarchy.py` had the same disease: the + test was named `test_killed_interrupt_does_not_inherit_from_exception` + while asserting `issubclass(WorkflowKilledInterrupt, Exception)`. + Renamed. +- **DEF-TC6-005** — `status().ws_connected` was structurally pinned + to `None`. `WebSocketConnection` has an `_running` flag (set in + `_connect`, cleared by the receive loop's `finally`); the SDK + was reading `is_open` via `getattr(..., None)`. `is_open` + appears exactly once in the SDK: on the reading side, with no + writer, no test and no producer — so the `getattr` default + fired on every call and the three states (never-established / + live / dropped) collapsed to one. The fourth test in the new + file asserts that the attribute `status()` reads exists on a + really-constructed `WebSocketConnection` AND that `is_open` + does not — a stubbed connection cannot catch it, because the + stub would carry whatever attribute the test author assumed. + +### Documentation + +- ADR-065: the five-step restoration of the `BusinessImpact` + envelope is recorded in the SDK-side commit chain; the + backend-side companion is in the NULLRUN repo. The shared + fixture digest is pinned at byte-identity + (`9975a8b7…6ed0526966a6`) so a future serializer change in + either repo flips a test rather than degrading silently. + +### Verification + +| Check | Result | +|---|---| +| `ruff check src tests` | All checks passed | +| `mypy src/nullrun` | Success: no issues found in 37 source files (the 6 errors in `instrumentation/auto.py` reproduce identically without these changes) | +| `pytest -q` | **1694 passed, 1 skipped** (~ baseline 1597 / 1 — **+97 new tests**; 5 from the gate-block typed-dispatch file alone, 5 from the track-propagation file, 4 from the WS-status file, 1+3 from the approval-roundtrip file, 1 from the gate-business-impact-wire file, 4 from the business-impact-tool-call file, 1 from the call-impact-context file) | +| Scratch diff | clean | +| `nullrun.__version__` | `0.21.0` | +| Wire-format | additive on `/gate` (carries `business_impact`, `tool_class`, `mcp_annotations` when set; absent means "unknown", not "false"); non-additive on `/track` (the v3 single path now raises typed enforcement rejections where it dropped them — caller-observable) | + +### Commits included + +``` +d11eb78 test(protect): pin the unbuildable-envelope degradation +88ddec8 fix(protect): build the tool_call envelope before the /gate pre-flight +015407b feat(gate): send the context envelope at /gate instead of a sentinel +e3176d9 feat(sdk): carry one BusinessImpact envelope per logical action +4320ac0 feat(sdk): restore the tool_call BusinessImpact constructor +6108308 docs(kill): stop claiming the kill signal is BaseException-only +b64dc8a fix(mcp): forward tool class and annotations to /gate +ea7c9ee fix(track): propagate enforcement rejections from the v3 /track path +9ccf168 fix(sdk): read the attribute the WS connection actually has +76efa7b fix(sdk): type the /gate pre-flight refusal instead of assuming budget +4ec5460 fix(sdk): register the two backend budget codes in the SDK catalog +``` + ## [0.20.0] - 2026-10-01 The remaining half of `DEF-MP-TS12-ENF-01` (QA cycle RUN_ID diff --git a/pyproject.toml b/pyproject.toml index 618afa0..714a4ea 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.20.0" +version = "0.21.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 e2aa469..8f21ca0 100644 --- a/src/nullrun/__version__.py +++ b/src/nullrun/__version__.py @@ -5,5 +5,5 @@ string and the SDK_MIN_VERSION constant. """ -__version__ = "0.20.0" +__version__ = "0.21.0" __platform_version__ = "1.0.0" diff --git a/tests/test_gate_business_impact_wire.py b/tests/test_gate_business_impact_wire.py index 8f2362b..60623a4 100644 --- a/tests/test_gate_business_impact_wire.py +++ b/tests/test_gate_business_impact_wire.py @@ -26,7 +26,7 @@ import pytest import respx -from nullrun.business_impact import BusinessImpact, DIGEST_PREFIX +from nullrun.business_impact import DIGEST_PREFIX, BusinessImpact from nullrun.context import set_call_impact from nullrun.transport import Transport diff --git a/tests/test_protect_approval_roundtrip.py b/tests/test_protect_approval_roundtrip.py index 4ae83f6..3950dae 100644 --- a/tests/test_protect_approval_roundtrip.py +++ b/tests/test_protect_approval_roundtrip.py @@ -31,7 +31,7 @@ import respx import nullrun -from nullrun.business_impact import BusinessImpact, DIGEST_PREFIX +from nullrun.business_impact import DIGEST_PREFIX, BusinessImpact BASE_URL = "https://api.test.nullrun.io" @@ -224,7 +224,7 @@ def test_a_different_amount_is_a_different_action(self, make_runtime, mock_api, def refund_customer(amount: int) -> str: return "ok" - rt = make_runtime() + make_runtime() refund_customer(amount=500) approved = _last(captured, "gate")["action_digest"] refund_customer(amount=500_000) diff --git a/uv.lock b/uv.lock index d8658c9..7c6af19 100644 --- a/uv.lock +++ b/uv.lock @@ -636,7 +636,7 @@ wheels = [ [[package]] name = "nullrun" -version = "0.20.0" +version = "0.21.0" source = { editable = "." } dependencies = [ { name = "httpx" },