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/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/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/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/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/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/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/src/nullrun/runtime.py b/src/nullrun/runtime.py index c83d217..cec2d47 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 @@ -77,13 +78,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 @@ -109,6 +111,7 @@ NullRunBackendError, NullRunBlockedException, NullRunBudgetError, + NullRunDecision, NullRunDeniedError, NullRunError, NullRunInfrastructureError, @@ -1053,10 +1056,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 @@ -2270,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, @@ -2339,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. @@ -2524,23 +2554,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 +3874,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 +3961,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 +3993,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 +4113,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, @@ -4125,6 +4340,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/src/nullrun/transport.py b/src/nullrun/transport.py index 1a881ab..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`. @@ -1570,6 +1588,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 @@ -3221,6 +3270,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 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_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 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 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) 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" diff --git a/tests/test_gate_business_impact_wire.py b/tests/test_gate_business_impact_wire.py new file mode 100644 index 0000000..60623a4 --- /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 DIGEST_PREFIX, BusinessImpact +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"] diff --git a/tests/test_protect_approval_roundtrip.py b/tests/test_protect_approval_roundtrip.py new file mode 100644 index 0000000..3950dae --- /dev/null +++ b/tests/test_protect_approval_roundtrip.py @@ -0,0 +1,366 @@ +"""`@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 DIGEST_PREFIX, BusinessImpact + +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" + + 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"] + + +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 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_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" + ) 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 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 " 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" },