diff --git a/CHANGELOG.md b/CHANGELOG.md index dd34cf5..7265d1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,131 @@ +## [0.22.2] - 2026-10-16 + +Patch release, no protocol change. Three unrelated bugs closed in one +shipping window: a status field that structurally could not report +the failure it existed to report (`backend_reachable` initialised to +`None` and never written), a packaging extra that the SDK's own +`ImportError` told users to install but that `pyproject.toml` never +declared, and a pre-flight that priced at the input rate and so +silently under-reserved every real LLM call. None is a wire +change — all three are SDK-side. The status and packaging fixes are +behaviourally invisible to a healthy configuration; the reserve fix +is visible to anyone whose `max_output_tokens` is currently being +ignored. + +### Migration + +Three things differ from 0.22.1. None is silent on a healthy +configuration, but the third is a new opt-in signal that the wire +contract was always supposed to carry: + +1. **`NullRunStatus` reflects a backend that has actually been + attempted.** A backend unreachable since `init()` used to read + `backend_reachable=None` and so the DEGRADED headline stayed + `STATE_OK` — a monitoring surface that structurally could not + report the failure it exists to report. The writer is now + called from every site that learns whether the backend answered + (both `/gate` paths in `check_workflow_budget`, both + `/auth/verify` outcomes); "answered" means transport-level, not + HTTP-2xx — a 402/403/429 is the backend replying and stays + reachable, only DNS / refused / TLS / timeout sets it False. +2. **`pip install nullrun[websocket]` actually installs `websockets`.** + The error message named an extra `pyproject.toml` never declared + (only the opentelemetry pair and `dev` were listed). The failure + mode was worse than a missing feature: a user told precisely how + to install it typed it, and got a *second* error. `websockets` + was in `uv.lock` only as a transitive of `langsmith`, so the + WS wire test's coverage depended on a dependency edge nobody + here controls. Now declared with floor `14.0` (the `additional_headers` + spelling replaced `extra_headers` in the asyncio client in + 14.0; an older install would `TypeError` at connect time, + later and in production traffic, instead of at the + `ImportError` the user was told to fix). +3. **`set_call_context(max_output_tokens=…)` reaches `/gate`.** + The pre-flight has always sent `estimated_tokens=1`, which + prices at the INPUT rate and covers no output at all. Output + bills at the completion rate (5× input on the live catalog), + so a real 20k-token Opus response costs 50¢ against a 1¢ + reserve: `/track` then rejects the row with + `CONSUME_OVERBUDGET` and the period counter under-counts the + spend. Now the caller's declared cap is sent when set, the key + is ABSENT (not present-and-null) when not, and a cap of zero + or less is treated as "not declared" rather than silently + under-reserving. The via-edge box's `token_split()` falls back + to the declared cap before the one-token estimate, never the + other way. + +### Fixed + +- **DEF-STATUS-WRITER-01** — `NullRunStatus.backend_reachable` was + initialised to `None` in `__init__` and read in `status()` — and + never assigned anywhere in the SDK. The comment above the + initialisation claimed the pair was "set in `_authenticate` and + updated on every successful / failed backend call thereafter"; + no such writer had ever existed. Two silent consequences: the + DEGRADED headline was gated on `is False` so it never fired + (pinned to `None`), and the MISCONFIGURED arm was gated on + `_last_backend_attempt_at is not None` so it was dead for the + same reason. Fix: one writer `_note_backend_attempt(ok)` called + from the four sites that actually learn whether the backend + answered. "Ok" means "the backend answered", NOT "the call + succeeded": a 402/403/429 and a 401 are the backend replying, + and only a transport-level failure sets it False. Collapsing + those would brand a healthy server that correctly refused a + call as unreachable and drive DEGRADED off a credential + problem. The pair is read under a new lock because gate calls + arrive on the tool thread while `_authenticate` runs on the + caller's thread. (`b076643`, +100/-8; test files + `test_status_backend_reachable_has_a_writer.py`, + `test_status_ws_connected_reads_real_attribute.py`) + +- **DEF-SDK-16 (packaging: declare the `websocket` extra)** — + `WebSocketConnection.connect` raises + `websockets library not available. Install with: pip install + nullrun[websocket]`, but `nullrun` had no `websocket` extra. + `[project.optional-dependencies]` declared exactly two — the + opentelemetry pair and `dev` — and `websockets` appeared + nowhere in `pyproject.toml`. Two declarations, not one: + `websocket = ["websockets>=14.0,<17.0"]` and `dev` also gains + `websockets` (so `tests/test_ws_push.py` resolves through a + declared edge, not through `langchain-core -> langsmith -> + websockets` which nobody here controls). A new source pin + asserts the general property: every `nullrun[]` install + hint appearing anywhere under `src/` must name an extra + `pyproject.toml` actually declares. (`a8cb823`, +24/-1; + `tests/test_websocket_extra_is_declared.py`) + +### Added + +- **DEF-MOT-01 (reserve: forward caller-declared + `max_output_tokens` to `/gate`)** — per ADR-016 §2.1 the + backend prices an output reserve from TWO independent signals + (the caller's declared cap and its own per-model catalog cap) + and refuses to reserve from either one alone. The SDK's half + of that contract is now narrow and specific: when the caller + set a cap it is sent, when the caller did not the key is + ABSENT (not present-and-null — a null is a value the backend + has to defend against, not an honest "I don't know"), and a + cap of zero or less is treated as "not declared" rather than + silently under-reserving. New ContextVar + `_call_max_output_tokens_var`, reader + `get_call_max_output_tokens()`, and a `max_output_tokens=` + parameter on `set_call_context(...)`. The via-edge box's + `token_split()` falls back to the declared cap before the + one-token estimate; every step down errs toward charging + MORE, never less. (`f095c46`, +251/-9; test file + `tests/test_max_output_tokens_reserve.py`) + +### Verification + +| Check | Result | +|---|---| +| `ruff check src tests` | All checks passed | +| `mypy src/nullrun` | Success: no issues found in 39 source files | +| `pytest -q` | **1913 passed, 12 skipped** in 156.34s (vs baseline 1849 / 12 — **+64 new tests** from `test_status_backend_reachable_has_a_writer`, `test_status_ws_connected_reads_real_attribute`, `test_websocket_extra_is_declared`, `test_max_output_tokens_reserve`) | +| Scratch diff | clean (`dist_local/`, `*.defect*` absent) | +| `nullrun.__version__` | `0.22.2` | +| Wire-format compatibility | unchanged from 0.22.1 (no protocol change; the `max_output_tokens` key is new-but-OPTIONAL, the backend ignores its absence as before) | + ## [0.22.1] - 2026-10-15 Patch release, no protocol change. The SDK's public exception surface diff --git a/pyproject.toml b/pyproject.toml index bac0f27..97d4f8c 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.22.1" +version = "0.22.2" # 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" @@ -77,7 +77,31 @@ opentelemetry = [ "opentelemetry-api>=1.26.0,<2.0", "opentelemetry-sdk>=1.26.0,<2.0", ] +# DEF-SDK-16 (RUN_ID 20261015T1809). ``transport_websocket.connect`` +# raises ``ImportError("… Install with: pip install nullrun[websocket]")`` +# when the ``websockets`` import fails — but no ``websocket`` extra was +# ever declared, so the command in the error message did not exist and +# pip rejected it. The extra below makes the message true. +# +# The floor is 14.0, not "any version": the transport calls +# ``websockets.connect(url, additional_headers=...)``. The +# ``additional_headers`` spelling belongs to the asyncio client that +# became the default ``websockets.connect`` in 14.0; earlier releases +# spelled the same argument ``extra_headers``, so a lower floor +# resolves and then fails at connect time with a TypeError instead of +# at import time. +websocket = [ + "websockets>=14.0,<17.0", +] dev = [ + # ``tests/test_ws_push.py`` imports ``websockets`` at module scope + # and spins up a real local WS server. It used to pass only because + # ``langchain-core`` -> ``langsmith`` -> ``websockets`` happened to + # pull it in transitively; a langchain/langsmith release that drops + # that edge would turn the whole module into a collection error. + # Declared directly so the test suite's WS coverage depends on the + # websocket extra existing, not on someone else's dependency graph. + "websockets>=14.0,<17.0", "pytest>=8.0", "pytest-asyncio>=0.23", # Sprint 0 (coverage): ``pytest-rerunfailures`` is used on a @@ -135,6 +159,15 @@ dev = [ # bound as the `[fastapi]` extra so CI and end users agree on the # minimum. `httpx` (already a core dep) covers `TestClient`. "fastapi>=0.100,<1.0", + # `tests/test_websocket_extra_is_declared.py` parses + # `pyproject.toml` at collection time to pin the general + # "every `nullrun[]` install hint under `src/` names + # an extra `pyproject.toml` actually declares" property. + # `tomllib` is stdlib from 3.11; on 3.10 the test must fall + # back to `tomli`, and the fallback is only safe if the dep + # is declared. Conditional on `<3.11` so the upper-bound + # environments do not pull a redundant package. + "tomli>=2.0; python_version < '3.11'", ] [project.scripts] diff --git a/src/nullrun/__version__.py b/src/nullrun/__version__.py index 1191ed4..a73bf62 100644 --- a/src/nullrun/__version__.py +++ b/src/nullrun/__version__.py @@ -5,5 +5,5 @@ string and the SDK_MIN_VERSION constant. """ -__version__ = "0.22.1" +__version__ = "0.22.2" __platform_version__ = "1.0.0" diff --git a/src/nullrun/context.py b/src/nullrun/context.py index 04feec5..392352e 100644 --- a/src/nullrun/context.py +++ b/src/nullrun/context.py @@ -42,6 +42,23 @@ # enforced on /track only). _call_model_var: ContextVar[str | None] = ContextVar("call_model", default=None) _call_tools_var: ContextVar[tuple[str, ...]] = ContextVar("call_tools", default=()) +# The `max_output_tokens` the caller intends to request from the +# provider — Anthropic/OpenAI's `max_tokens` / `max_output_tokens`. +# +# The pre-flight has always sent `estimated_tokens=1`, which prices at +# the INPUT rate and covers no output at all. Output is billed at the +# completion rate (5× the input rate on the live catalog), so a real +# 20k-token Opus response costs 50¢ against a 1¢ reserve: /track then +# rejects the row with CONSUME_OVERBUDGET and the period counter +# under-counts the spend. +# +# `None` means "the caller didn't say", and the backend keeps the old +# arithmetic — a missing signal must not invent a reserve. The backend +# bounds whatever arrives here by its own per-model catalog cap, so a +# client can only ever LOWER the reserve, never inflate it. +_call_max_output_tokens_var: ContextVar[int | None] = ContextVar( + "call_max_output_tokens", default=None +) # Per-call MCP tool class + annotations. Set via the # ``set_mcp_tool_context`` helper when the SDK recognises an MCP # server. The gate honors `tool_class` over its own @@ -142,6 +159,18 @@ def get_call_tools() -> tuple[str, ...]: return _call_tools_var.get() +def get_call_max_output_tokens() -> int | None: + """The declared output cap for the next pre-flight, or ``None``. + + ``None`` is the honest default: the SDK cannot invent an output + size for a call it does not make. The backend treats the pair + (declared, catalog cap) as the only inputs it will price an output + reserve from — see ADR-016 §2.1 — and refuses to reserve from + either signal alone. + """ + return _call_max_output_tokens_var.get() + + def get_call_mcp_class() -> str | None: """Canonical tool class for the next ``/check`` call. @@ -802,9 +831,10 @@ def create_child_span_with_id(parent: SpanContext, span_id: str) -> SpanContext: def set_call_context( model: str | None = None, tools: list[str] | tuple[str, ...] | None = None, + max_output_tokens: int | None = None, ) -> None: - """Set per-call context (model name, tool list) for the next /gate - pre-flight check. + """Set per-call context (model name, tool list, output cap) for the + next /gate pre-flight check. Replaces the previous fake ``model="budget-precheck"`` and ``estimated_tokens=1`` always-default / always-empty pre-flight. @@ -821,11 +851,25 @@ def set_call_context( ``blocked_tools`` aggregate and returns block on any match. Pass ``None`` to leave the current value unchanged, ``[]`` to clear. + max_output_tokens: The output cap you are about to send the + provider — Anthropic's ``max_tokens``, OpenAI's + ``max_output_tokens``. Sent only when you pass a positive + integer; omitting it keeps the pre-flight exactly as it + was, which under-reserves output rather than guessing at + a number. The backend clamps this to its own per-model + catalog cap, so a value larger than the model can emit is + not a way to raise your own reserve. """ if model is not None: _call_model_var.set(model) if tools is not None: _call_tools_var.set(tuple(tools)) + if max_output_tokens is not None: + # A cap of zero or less is not a cap; forwarding it would let a + # caller declare "no output" and reserve nothing at all. Treat + # it as "not declared" instead of silently under-reserving. + if max_output_tokens > 0: + _call_max_output_tokens_var.set(max_output_tokens) def set_mcp_tool_context( diff --git a/src/nullrun/edge.py b/src/nullrun/edge.py index 679b5e0..5dcd0ab 100644 --- a/src/nullrun/edge.py +++ b/src/nullrun/edge.py @@ -386,14 +386,23 @@ def edge_transport_from_env(env: dict[str, str] | None = None) -> EdgeTransport def token_split(check_request: dict[str, Any]) -> tuple[int, int]: """The token counts this call is reported with. - Explicit `edge_input_tokens` / `edge_output_tokens` win. Absent - them, `estimated_tokens` is reported as OUTPUT tokens — output is - what the price is dominated by for most models, and reporting the - whole estimate as output errs toward charging more rather than - less. Under-reporting is the one direction this cannot take, since - the box cannot check the number and will simply believe it. + Precedence for output: explicit `edge_output_tokens`, then the + declared `max_output_tokens`, then `estimated_tokens`. Each step + down is a weaker answer to "how big will the output be" — a known + size, then the upper bound the caller is about to ask the provider + for, then a blob the runtime only ever sets to 1. + + Every step errs toward charging MORE, never less. That is + deliberate and load-bearing: the box cannot check the number, it + will believe it, and output is what the price is dominated by for + most models (5× the input rate on the live catalog). Reporting the + declared cap when it is the only real signal costs a little + over-reserve on calls that finish short; reporting `1` instead + under-reserves every real call. """ out = check_request.get("edge_output_tokens") + if out is None: + out = check_request.get("max_output_tokens") if out is None: out = check_request.get("estimated_tokens") or 0 inp = check_request.get("edge_input_tokens") or 0 diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index 4651768..8d4febb 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -785,8 +785,28 @@ def __init__( self._zero_activity_lock = threading.Lock() # Layer 3: backend connectivity timestamps for the status - # snapshot. Set in ``_authenticate`` and updated on every - # successful / failed backend call thereafter. + # snapshot. Written ONLY by ``_note_backend_attempt`` — + # ``status()`` reads them, but nothing else may assign them. + # + # Pre-fix this comment claimed they were "set in + # ``_authenticate`` and updated on every successful / failed + # backend call thereafter". No such writer existed anywhere in + # the SDK: the two attributes were initialised to ``None`` at + # line 790 and read at line 1053, and never assigned again. + # So ``NullRunStatus.backend_reachable`` was structurally pinned + # to ``None`` for the life of the process, and because the + # headline derivation gates DEGRADED on + # ``backend_reachable is False`` (line 1105), a backend that had + # been down since ``init()`` still reported ``STATE_OK``. The + # same two attributes also gate the MISCONFIGURED arm through + # ``_last_backend_attempt_at`` (line 1100), so that arm was + # dead for the same reason. + # + # Paired fields, so they are written together under one lock — + # gate calls arrive on the tool thread while ``_authenticate`` + # runs on the caller's thread, and a torn read could pair a + # fresh timestamp with a stale verdict. + self._backend_attempt_lock = threading.Lock() self._last_backend_attempt_at: float | None = None self._last_backend_attempt_ok: bool | None = None @@ -1049,12 +1069,20 @@ def status(self) -> Any: api_key_valid = True # --- Connectivity --- + # Read under the lock so the pair is consistent: a gate call on + # the tool thread can land between the two reads below and + # pair a fresh timestamp with the previous verdict. The verdict + # is None only when no attempt has been recorded since init. + with self._backend_attempt_lock: + last_attempt_at = self._last_backend_attempt_at + last_attempt_ok = self._last_backend_attempt_ok backend_reachable: bool | None = None - if self._last_backend_attempt_at is not None: - # ``_last_backend_attempt_ok`` is set to True on - # a successful HTTP response, False on a transport - # error. ``None`` if no attempt since init. - backend_reachable = self._last_backend_attempt_ok + if last_attempt_at is not None: + # ``_last_backend_attempt_ok`` is True when the backend + # ANSWERED (any HTTP status, including a refusal) and False + # only on a transport-level failure. ``None`` means no + # attempt has been recorded since init. + backend_reachable = last_attempt_ok ws_connected: bool | None = None if self._ws_connection is not None: @@ -1097,7 +1125,7 @@ def status(self) -> Any: # --- Headline state derivation --- # Order matters: most specific first. if self.api_key is None or ( - self.organization_id is None and self._last_backend_attempt_at is not None + self.organization_id is None and last_attempt_at is not None ): headline = STATE_MISCONFIGURED elif ( @@ -1218,6 +1246,32 @@ def _record_error( ) ) + def _note_backend_attempt(self, ok: bool) -> None: + """Record the outcome of one backend HTTP attempt. + + The single writer for ``_last_backend_attempt_at`` / + ``_last_backend_attempt_ok``. ``status()`` derives + ``backend_reachable`` from them and gates the DEGRADED headline + on ``backend_reachable is False``, so every path that learns + whether the backend answered MUST funnel through here — a path + that raises on transport failure without calling this leaves the + field at its previous value, which is the bug this method + exists to close. + + ``ok`` means "the backend answered", NOT "the call succeeded". + A 402/403/429 is an answer and must count as reachable; only a + transport-level failure (DNS, refused, TLS, timeout) sets it + False. Collapsing those two would mark a healthy backend that + correctly refused a call as unreachable. + + Best-effort by construction: assigns two attributes under a + lock and never raises, so a status read can never be broken by + this bookkeeping. + """ + with self._backend_attempt_lock: + self._last_backend_attempt_at = time.monotonic() + self._last_backend_attempt_ok = ok + def _emit_sdk_error( self, err: BaseException, @@ -1376,11 +1430,24 @@ def _authenticate(self) -> None: self._transport.secret_key = new_secret_key logger.info(f"Authenticated: organization_id={self.organization_id}") + # A 200 from /auth/verify is the strongest liveness + # signal the SDK has. Recording it here is what makes + # `backend_reachable` True rather than None on the + # happy path. + self._note_backend_attempt(True) else: # 5xx routes to NullRunBackendError (backend-class # error, per CLAUDE.md §13). 401/NR-A003 = key # rejected (true auth failure). Other 4xx / NR-A001 # covers 403 etc. + # + # A non-2xx status is still the backend ANSWERING — a + # 401 is a reachable backend with a bad key, and a 5xx + # is a reachable backend in trouble. Recording False + # here would report "unreachable" for a server that just + # replied, and would drive the DEGRADED headline off a + # credential problem instead of a connectivity one. + self._note_backend_attempt(True) status = response.status_code correlation_id = response.headers.get("x-correlation-id") if 500 <= status < 600: @@ -1414,6 +1481,14 @@ def _authenticate(self) -> None: # the rest of the SDK. ``NullRunTransportError.__init__`` # already sets ``error_code="NR-B001"`` and the standard # retryable ``user_action``. + # + # This is the FIRST backend attempt in a normal run, so it + # is what makes ``backend_reachable`` observable at all + # when the backend is down from the start. Without it a + # caller that only ever calls ``status()`` (never a tool) + # would read ``None`` — "never tested" — for a backend that + # had just refused the connection outright. + self._note_backend_attempt(False) err = NullRunTransportError( f"Auth request failed: {e}. Cannot establish secure connection to NullRun. " f"Refusing to operate in unprotected mode.", @@ -2341,6 +2416,7 @@ def check_workflow_budget(self) -> None: ) from nullrun.context import ( get_call_impact, + get_call_max_output_tokens, get_call_mcp_annotations, get_call_mcp_class, get_call_model, @@ -2407,6 +2483,16 @@ def check_workflow_budget(self) -> None: "stream": False, } + # The declared output cap, when the caller set one. Kept out of + # the dict literal because "not declared" must be ABSENT on the + # wire, not present-and-null: the backend prices an output + # reserve only when it has BOTH this number and its own per-model + # catalog cap (ADR-016 §2.1), and a null would be one signal + # short of that pair rather than an honest "I don't know". + call_max_output = get_call_max_output_tokens() + if call_max_output is not None: + check_req["max_output_tokens"] = call_max_output + # `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 @@ -2510,6 +2596,10 @@ def check_workflow_budget(self) -> None: # Cache miss or expired — go to the server, then store. try: response = self._transport.check(check_req) + # The backend ANSWERED. A refusal (402/403/429) is + # still an answer and must not be read as + # unreachable. + self._note_backend_attempt(True) except NullRunAuthenticationError: # DEF-MP-TS12-ENF-01 (RUN_ID 20260929T1338, 2026-09-29): # a 401 is a CREDENTIAL failure, not a transport @@ -2538,6 +2628,7 @@ def check_workflow_budget(self) -> None: # classified SDK errors. Internal bugs # (KeyError, AttributeError) surface rather # than silently allowing an unbounded call. + self._note_backend_attempt(False) logger.warning(f"check_workflow_budget: /gate unavailable, failing open: {exc}") metrics.inc_runtime("gate_fail_open_total") return @@ -2560,6 +2651,9 @@ def check_workflow_budget(self) -> None: else: try: response = self._transport.check(check_req) + # Same rationale as the cached branch: a refusal is an + # answer, not an outage. + self._note_backend_attempt(True) except NullRunAuthenticationError: # Same rationale as the cached branch above. Ordering # matters: this arm precedes the broad `except @@ -2579,6 +2673,7 @@ def check_workflow_budget(self) -> None: # ``except Exception``. raise except Exception as exc: # noqa: BLE001 + self._note_backend_attempt(False) logger.warning(f"check_workflow_budget: /gate unavailable, failing open: {exc}") metrics.inc_runtime("gate_fail_open_total") return diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index fe1a270..7ac8559 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -3629,6 +3629,16 @@ def check( "check_type": check_request.get("check_type"), "model": check_request.get("model"), "estimated_tokens": check_request.get("estimated_tokens"), + # Declared output cap, forwarded only when the caller set + # one. The backend needs it together with its own per-model + # catalog cap to size an output reserve; sent as null it + # would be a half-answer, and the backend prices `None` as + # "no reserve" (ADR-016 §2.1). + **( + {"max_output_tokens": check_request["max_output_tokens"]} + if check_request.get("max_output_tokens") is not None + else {} + ), "operation_id": check_request.get("operation_id") or str(uuid.uuid4()), # Forward the per-call `tools` list so the backend's # `gate/internal.rs::check_tool_block` can match each diff --git a/src/nullrun/transport_websocket.py b/src/nullrun/transport_websocket.py index 6f2173c..ef60ac1 100644 --- a/src/nullrun/transport_websocket.py +++ b/src/nullrun/transport_websocket.py @@ -20,6 +20,11 @@ # only consumes it. from nullrun.transport import generate_hmac_signature, verify_hmac_signature +# ``websockets`` is an optional dependency (the ``websocket`` extra). +# The ImportError below names that extra, so the two have to stay in +# step — DEF-SDK-16 was exactly the case where the message pointed at +# an extra pyproject.toml never declared, making the suggested command +# uninstallable. ``test_websocket_extra_is_declared`` pins it. try: import websockets diff --git a/tests/conftest.py b/tests/conftest.py index d0f08d7..1bc0cb0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -18,7 +18,11 @@ def reset_runtime(): import nullrun.actions as _act import nullrun.decorators as _dec import nullrun.runtime as _rt_mod - from nullrun.context import _call_model_var, _call_tools_var + from nullrun.context import ( + _call_max_output_tokens_var, + _call_model_var, + _call_tools_var, + ) from nullrun.runtime import NullRunRuntime NullRunRuntime.reset_instance() @@ -30,6 +34,7 @@ def reset_runtime(): _rt_mod._runtime = None _call_model_var.set(None) _call_tools_var.set(()) + _call_max_output_tokens_var.set(None) yield @@ -51,6 +56,7 @@ def reset_runtime(): _rt_mod._runtime = None _call_model_var.set(None) _call_tools_var.set(()) + _call_max_output_tokens_var.set(None) @pytest.fixture diff --git a/tests/test_max_output_tokens_reserve.py b/tests/test_max_output_tokens_reserve.py new file mode 100644 index 0000000..c6e9039 --- /dev/null +++ b/tests/test_max_output_tokens_reserve.py @@ -0,0 +1,162 @@ +"""The declared output cap reaches the wire — and its absence stays absent. + +ADR-016 §2.1: the backend prices an output reserve from TWO independent +signals — the caller's declared `max_output_tokens` and its own per-model +catalog cap — and refuses to reserve from either one alone. That makes +the SDK's half of the contract narrow and specific: + + 1. When the caller set a cap, it is sent. + 2. When the caller did NOT, the key is ABSENT from the body. Not + present-and-null: a null is not "I don't know", it is a value the + backend has to defend against, and sending one turns an honest gap + into a half-answer. + 3. A nonsensical cap (zero, negative) is not forwarded — it would let + a caller declare "no output" and reserve nothing. + +The bug this closes is silent and one-directional. The pre-flight has +always sent `estimated_tokens=1`, which prices at the INPUT rate and +covers no output at all. Output bills at the completion rate — 5× input +on the live catalog — so a real 20k-token Opus response costs 50¢ +against a 1¢ reserve. /track then rejects the row with +CONSUME_OVERBUDGET and the period counter under-counts the spend. Nothing +in that path raises; the org just sees less spend than it made. +""" + +from __future__ import annotations + +import json + +import httpx +import pytest +import respx + +from nullrun.context import get_call_max_output_tokens, set_call_context +from nullrun.edge import token_split + +BASE_URL = "https://api.test.nullrun.io" +GATE_URL = f"{BASE_URL}/api/v1/gate" + + +@pytest.fixture +def captured_bodies(): + """Capture every /gate body and answer `allow`, so the assertion is + about what was SENT rather than about a decision.""" + 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 + + +class TestMaxOutputTokensReachesTheWire: + def test_a_declared_cap_is_sent(self, make_runtime, mock_api, captured_bodies): + """The whole feature: a cap the caller declared must be visible + to the backend, otherwise `Unavailable` holds and the reserve + stays at the old 1¢ that under-reserves output by ~50×.""" + rt = make_runtime() + set_call_context(model="claude-opus-4.8", max_output_tokens=20_000) + rt.check_workflow_budget() + + assert captured_bodies, "no /gate call was captured" + assert captured_bodies[-1].get("max_output_tokens") == 20_000, ( + "the declared output cap must reach /gate — without it the " + "backend has one signal where it requires two and refuses " + "to reserve any output at all" + ) + + def test_no_cap_means_the_key_is_absent_not_null( + self, make_runtime, mock_api, captured_bodies + ): + """Absence is the load-bearing case: 76 of the 111 catalog rows + have no cap either, and the default pre-flight must not + manufacture a number to pair with them.""" + rt = make_runtime() + set_call_context(model="claude-opus-4.8") + rt.check_workflow_budget() + + body = captured_bodies[-1] + assert "max_output_tokens" not in body, ( + "with no declared cap the key must be OMITTED, not sent as " + "null. A null reads as 'the caller said zero output', which " + "is a claim the backend would have to reject rather than an " + "honest 'unknown', and it silently disables the reserve on " + "every model whose catalog cap IS backfilled" + ) + + @pytest.mark.parametrize("bogus", [0, -1, -4096]) + def test_a_nonsense_cap_is_treated_as_not_declared( + self, make_runtime, mock_api, captured_bodies, bogus + ): + """A non-positive cap is not a cap. Forwarding it would be a + free way to reserve nothing on a call that does emit output — + the exact under-reservation this exists to close, reachable + through the new field.""" + rt = make_runtime() + set_call_context(model="claude-opus-4.8", max_output_tokens=bogus) + rt.check_workflow_budget() + + assert "max_output_tokens" not in captured_bodies[-1], ( + f"{bogus=} was forwarded. A cap of zero or less would let a " + "caller reserve nothing while still making a real LLM call" + ) + + +class TestContextVarItself: + """The plumbing under the wire path: a cap that never reaches the + getter cannot reach /gate, and the getter is a public symbol users + can read back.""" + + def test_the_getter_round_trips_what_was_set(self): + assert get_call_max_output_tokens() is None + set_call_context(max_output_tokens=8_192) + assert get_call_max_output_tokens() == 8_192 + + def test_omitting_the_argument_leaves_the_previous_value_alone(self): + """Same contract as `model` and `tools`: a later + `set_call_context(tools=...)` that says nothing about the cap + must not silently clear it. Dropping the reserve because an + unrelated field was set would be an invisible regression for + anyone configuring this per-tool.""" + set_call_context(max_output_tokens=8_192) + set_call_context(tools=["read_file"]) + assert get_call_max_output_tokens() == 8_192 + + +class TestEdgeTokenSplit: + """The via-edge box cannot check any number it is given — it + believes it. So every fallback in `token_split` must move TOWARD a + larger output estimate, never toward a smaller one.""" + + def test_explicit_edge_output_still_wins(self): + """An explicitly reported output size is a better answer than + any upper bound, so it keeps priority.""" + _, out = token_split( + {"edge_output_tokens": 512, "max_output_tokens": 20_000, "estimated_tokens": 1} + ) + assert out == 512 + + def test_the_declared_cap_beats_the_one_token_estimate(self): + _, out = token_split({"max_output_tokens": 20_000, "estimated_tokens": 1}) + assert out == 20_000, ( + "with a declared cap available, falling back to " + "estimated_tokens=1 re-creates the cloud path's " + "under-reservation on the edge path" + ) + + def test_nothing_declared_still_falls_back_to_the_estimate(self): + """Status quo preserved for callers who set no cap — the edge + path must not start refusing or erroring on the old shape.""" + _, out = token_split({"estimated_tokens": 1}) + assert out == 1 \ No newline at end of file diff --git a/tests/test_status_backend_reachable_has_a_writer.py b/tests/test_status_backend_reachable_has_a_writer.py new file mode 100644 index 0000000..7fbc332 --- /dev/null +++ b/tests/test_status_backend_reachable_has_a_writer.py @@ -0,0 +1,222 @@ +"""``status().backend_reachable`` was structurally pinned to ``None``. + +## What was observed + +QA (RUN_ID 20261013T1200) reported the SDK printing "reachable" +while the backend was down. Reading the source, the field could not +have said anything else: + +* ``runtime.py`` initialised ``_last_backend_attempt_at`` and + ``_last_backend_attempt_ok`` to ``None`` in ``__init__``. +* ``status()`` read them and produced ``backend_reachable``. +* **Nothing in the SDK ever assigned either attribute.** + +A grep for the writer returned only the two initialisations and three +reads (including this file). The comment above the initialisation +claimed they were "set in ``_authenticate`` and updated on every +successful / failed backend call thereafter" — that writer had never +existed. + +## Why it is a real bug, not cosmetic + +``status()`` derives the DEGRADED headline partly from +``backend_reachable is False``. With the field pinned to ``None`` +that arm never fired, so a backend that had been unreachable since +``init()`` still reported ``STATE_OK``. The same pinning made the +MISCONFIGURED arm dead: it is gated on +``self.organization_id is None and self._last_backend_attempt_at is +not None``, and the second half was always ``None``. + +A monitoring surface that structurally cannot report the failure it +exists to report is worse than no surface: the caller trusts ``ok``. + +## The fix + +One writer, ``_note_backend_attempt(ok)``, called from the sites that +actually learn whether the backend answered: both ``/gate`` paths in +``check_workflow_budget`` and both ``/auth/verify`` outcomes. + +The distinction the tests below pin is *answered* vs *reachable*: +a 402/403/429 and a 401 are the backend replying, so they count as +reachable. Only a transport failure sets it False. Collapsing those +would brand a healthy server that correctly refused a call as down. +""" + +from __future__ import annotations + +import threading + +from nullrun.observability.status import _RecentErrorRing +from nullrun.runtime import NullRunRuntime + + +def _runtime(**overrides) -> NullRunRuntime: + """A runtime with only the attributes ``status()`` reads. + + Built via ``object.__new__`` so the connectivity branch is reached + without an authenticate-and-connect cycle — these tests are about + how ``backend_reachable`` is computed, not about the transport. + The attribute set is spelled out rather than mocked because a + missing one surfaces as an ``AttributeError`` at a line unrelated + to the assertion (the same reason + ``test_status_ws_connected_reads_real_attribute`` does it this way). + """ + rt = object.__new__(NullRunRuntime) + rt._ws_connection = None + rt._ws_stop_event = threading.Event() + rt.organization_id = "org-test" + rt.api_key = "nr_live_testkey" + rt._api_key_valid = True + rt._recent_errors = _RecentErrorRing(capacity=10) + rt._remote_states = {} + rt.workflow_id = None + rt.api_url = "https://api.example.invalid" + # The real lock type: `status()` acquires it. + rt._backend_attempt_lock = threading.Lock() + rt._last_backend_attempt_at = None + rt._last_backend_attempt_ok = None + for key, value in overrides.items(): + setattr(rt, key, value) + return rt + + +class TestBackendReachableHasAWriter: + def test_never_tested_stays_none(self): + """No attempt recorded → ``None``, not a fabricated verdict.""" + assert _runtime().status().backend_reachable is None + + def test_recorded_success_reports_true(self): + """A recorded answer must surface as ``True``.""" + rt = _runtime() + rt._note_backend_attempt(True) + assert rt.status().backend_reachable is True + + def test_recorded_transport_failure_reports_false(self): + """The regression: a recorded transport failure must surface. + + Pre-fix this returned ``None``, so the DEGRADED branch was + unreachable. + """ + rt = _runtime() + rt._note_backend_attempt(False) + assert rt.status().backend_reachable is False + + def test_transport_failure_drives_headline_to_degraded(self): + """``backend_reachable is False`` must actually reach the headline. + + This is the property the field exists for. Asserting only on + the field would still pass if the derivation arm were deleted. + """ + from nullrun.observability.status import STATE_DEGRADED, STATE_OK + + rt = _runtime() + assert rt.status().state == STATE_OK + + rt._note_backend_attempt(False) + assert rt.status().state == STATE_DEGRADED + + def test_a_refusal_counts_as_an_answer(self): + """A 401/402/403/429 is the backend ANSWERING. + + Connectivity is not the subject of that failure, and reporting + it as unreachable would send a caller chasing the network + instead of its key or its budget. The refusal status lives in + the transport response, not here — what ``_note_backend_attempt`` + receives from the ``_authenticate`` non-2xx arm is ``True`` for + exactly this reason, and this is the assertion that keeps that + arm from being "simplified" into a ``False`` later. + """ + rt = _runtime() + rt._note_backend_attempt(True) + assert rt.status().backend_reachable is True + + def test_latest_attempt_wins(self): + """The field tracks the MOST RECENT attempt, not any attempt. + + A backend that recovers must stop reporting unreachable, or the + DEGRADED state sticks forever. + """ + rt = _runtime() + rt._note_backend_attempt(False) + rt._note_backend_attempt(True) + assert rt.status().backend_reachable is True + + def test_last_attempt_is_never_unset_by_a_later_attempt(self): + """``_note_backend_attempt`` must never write ``None``. + + A writer that assigned ``None`` back would re-open the original + bug: the timestamp would be set while the verdict was absent, + which the reader treats as "never tested". + """ + rt = _runtime() + rt._note_backend_attempt(False) + rt._note_backend_attempt(True) + assert rt._last_backend_attempt_at is not None + assert rt._last_backend_attempt_ok is True + + +class TestWriterIsActuallyWired: + """The method existing is not the same as it being called. + + A helper with no call sites is exactly how the field became + permanently ``None`` in the first place: the reader and the writer + were written together and the writer was then never invoked. These + pin the call sites in production source. + """ + + @staticmethod + def _runtime_source() -> str: + from pathlib import Path + + import nullrun + + return (Path(nullrun.__file__).parent / "runtime.py").read_text( + encoding="utf-8" + ) + + def test_gate_paths_call_the_writer(self): + source = self._runtime_source() + assert source.count("_note_backend_attempt(True)") >= 3, ( + "both /gate paths in check_workflow_budget and the /auth/verify " + "success path must record that the backend answered" + ) + assert source.count("_note_backend_attempt(False)") >= 3, ( + "both /gate fail-open paths and the /auth/verify transport-error " + "path must record a transport failure" + ) + + def test_writer_has_no_none_assignment(self): + """The invariant that keeps the field truthful.""" + source = self._runtime_source() + body_start = source.index("def _note_backend_attempt") + body = source[body_start : body_start + 2000] + assert "_last_backend_attempt_ok = None" not in body, ( + "_note_backend_attempt must never write None into " + "_last_backend_attempt_ok — that resurrects the pinned-None bug" + ) + + +class TestConcurrentAttemptsDoNotTear: + def test_status_read_is_safe_during_writes(self): + """A status read concurrent with gate traffic must not raise. + + ``status()`` acquires ``_backend_attempt_lock``; before the fix + it read two attributes with no lock at all. This is a smoke + test for the lock, not a proof of atomicity. + """ + rt = _runtime() + stop = threading.Event() + + def writer(): + while not stop.is_set(): + rt._note_backend_attempt(True) + rt._note_backend_attempt(False) + + t = threading.Thread(target=writer, daemon=True) + t.start() + try: + for _ in range(200): + assert rt.status().backend_reachable in (True, False) + finally: + stop.set() + t.join(timeout=5) \ No newline at end of file diff --git a/tests/test_status_ws_connected_reads_real_attribute.py b/tests/test_status_ws_connected_reads_real_attribute.py index e016615..c450c92 100644 --- a/tests/test_status_ws_connected_reads_real_attribute.py +++ b/tests/test_status_ws_connected_reads_real_attribute.py @@ -89,6 +89,10 @@ def _runtime_with_ws(conn: object) -> NullRunRuntime: # on it, and a bare list fails there instead of at the assertion. rt._recent_errors = _RecentErrorRing(capacity=10) rt._remote_states = {} + # `status()` acquires this lock to read the backend-attempt pair + # atomically, so a fixture that omits it raises at the `with` and + # never reaches the WS assertion. + rt._backend_attempt_lock = threading.Lock() rt._last_backend_attempt_ok = None rt._last_backend_attempt_at = None rt.workflow_id = None diff --git a/tests/test_websocket_extra_is_declared.py b/tests/test_websocket_extra_is_declared.py new file mode 100644 index 0000000..61b77d5 --- /dev/null +++ b/tests/test_websocket_extra_is_declared.py @@ -0,0 +1,163 @@ +"""``pip install nullrun[websocket]`` named an extra that did not exist. + +## What was observed + +QA (RUN_ID 20261015T1809, DEF-SDK-16) hit ``ImportError`` from +``WebSocketConnection.connect`` and followed the instruction the +message gave: + + websockets library not available. Install with: + pip install nullrun[websocket] + +pip rejected it — ``nullrun`` has no ``websocket`` extra. +``[project.optional-dependencies]`` declared exactly two: the +``opentelemetry`` pair and ``dev``. ``websockets`` appeared nowhere in +``pyproject.toml`` at all; it was present in ``uv.lock`` only as a +transitive dependency of ``langsmith``, itself pulled in by +``langchain-core``. + +So the failure mode was worse than a missing feature: a user without +the library is told exactly how to install it, types it, and gets a +second error instead of the feature. The only way to reach the WS +transport was to know the real package name independently of the SDK's +own advice. + +## What is pinned here + +The general property, not the one string: + +* every ``nullrun[]`` install hint that appears in ``src/`` + names an extra ``pyproject.toml`` actually declares, and +* the ``websocket`` extra resolves to a ``websockets`` specifier. + +A new optional import added later with an uninstallable hint fails +here rather than in a user's terminal. +""" + +from __future__ import annotations + +import re +import sys +from pathlib import Path + +import pytest + +# `tomllib` is stdlib from Python 3.11; on 3.10 (the package's +# lower bound) the test must fall back to `tomli`, which the +# `dev` extra declares conditionally on `python_version < '3.11'`. +# A direct `import tomllib` fails at COLLECTION time on 3.10, +# before any test in the suite can run, so this fallback is the +# difference between "3.10 in CI green" and "3.10 errors the +# whole module at import". +import sys + +if sys.version_info >= (3, 11): + import tomllib +else: # pragma: no cover - exercised on Python 3.10 in CI + import tomli as tomllib + +_REPO_ROOT = Path(__file__).resolve().parents[1] +_SRC = _REPO_ROOT / "src" +_PYPROJECT = _REPO_ROOT / "pyproject.toml" + +# ``pip install nullrun[websocket]``, ``nullrun[dev,websocket]``, … +# Matched in source and docs alike; the assertion below is that every +# extra named in one of these has a declaration behind it. +_HINT_RE = re.compile(r"nullrun\[([a-z0-9_,.-]+)\]") + + +def _declared_extras() -> set[str]: + with _PYPROJECT.open("rb") as handle: + pyproject = tomllib.load(handle) + return set(pyproject["project"].get("optional-dependencies", {})) + + +def _hint_files() -> list[Path]: + files = [ + path + for path in _SRC.rglob("*.py") + if "__pycache__" not in path.parts + ] + return files + + +@pytest.mark.parametrize( + "path", _hint_files(), ids=lambda p: p.name +) +def test_install_hints_name_declared_extras(path: Path) -> None: + declared = _declared_extras() + for match in _HINT_RE.finditer(path.read_text(encoding="utf-8")): + for extra in match.group(1).split(","): + assert extra.strip() in declared, ( + f"{path.name} tells the user to install " + f"nullrun[{extra.strip()}], but pyproject.toml " + f"declares no such extra (declared: {sorted(declared)})" + ) + + +def test_websocket_extra_is_declared() -> None: + assert "websocket" in _declared_extras() + + +def test_websocket_extra_pulls_websockets() -> None: + with _PYPROJECT.open("rb") as handle: + pyproject = tomllib.load(handle) + specifiers = pyproject["project"]["optional-dependencies"]["websocket"] + assert any(s.startswith("websockets") for s in specifiers), specifiers + + +def test_error_message_names_the_declared_extra() -> None: + """The ImportError itself must point at the real extra. + + This is the assertion that would have caught DEF-SDK-16 directly: + the message is the only place a user learns the install command, so + if the extra is renamed or dropped the message has to fail here. + """ + declared = _declared_extras() + source = (_SRC / "nullrun" / "transport_websocket.py").read_text( + encoding="utf-8" + ) + hints = _HINT_RE.findall(source) + assert hints, "the ImportError lost its install hint entirely" + for group in hints: + for extra in group.split(","): + assert extra.strip() in declared + + +def test_floor_is_14_not_older() -> None: + """``additional_headers`` is the 14.0+ spelling. + + ``transport_websocket`` calls + ``websockets.connect(url, additional_headers=...)``. Releases before + 14.0 spelled that argument ``extra_headers``, so a floor below 14 + installs cleanly and then raises TypeError at connect time — later, + in production traffic, rather than at the ImportError the user was + told to fix. + """ + with _PYPROJECT.open("rb") as handle: + pyproject = tomllib.load(handle) + specifier = next( + s + for s in pyproject["project"]["optional-dependencies"]["websocket"] + if s.startswith("websockets") + ) + assert ">=14" in specifier, specifier + + +def test_dev_extra_also_carries_websockets() -> None: + """CI installs ``.[dev]`` only. + + ``tests/test_ws_push.py`` imports ``websockets`` at module scope. It + used to resolve through ``langchain-core`` -> ``langsmith`` -> + ``websockets``, so the WS wire test's coverage depended on a + dependency edge nobody here controls. Declared directly, that + coverage survives an upstream prune. + """ + with _PYPROJECT.open("rb") as handle: + pyproject = tomllib.load(handle) + dev = pyproject["project"]["optional-dependencies"]["dev"] + assert any(s.startswith("websockets") for s in dev), dev + + +if __name__ == "__main__": # pragma: no cover + sys.exit(pytest.main([__file__, "-v"])) \ No newline at end of file diff --git a/uv.lock b/uv.lock index d8300bb..abbb578 100644 --- a/uv.lock +++ b/uv.lock @@ -636,7 +636,7 @@ wheels = [ [[package]] name = "nullrun" -version = "0.22.1" +version = "0.22.2" source = { editable = "." } dependencies = [ { name = "httpx" }, @@ -656,11 +656,16 @@ dev = [ { name = "pytest-xdist" }, { name = "respx" }, { name = "ruff" }, + { name = "tomli", marker = "python_full_version < '3.11'" }, + { name = "websockets" }, ] opentelemetry = [ { name = "opentelemetry-api" }, { name = "opentelemetry-sdk" }, ] +websocket = [ + { name = "websockets" }, +] [package.metadata] requires-dist = [ @@ -679,8 +684,11 @@ requires-dist = [ { name = "pytest-xdist", marker = "extra == 'dev'", specifier = ">=3.6" }, { name = "respx", marker = "extra == 'dev'", specifier = ">=0.21" }, { name = "ruff", marker = "extra == 'dev'", specifier = ">=0.5" }, + { name = "tomli", marker = "python_full_version < '3.11' and extra == 'dev'", specifier = ">=2.0" }, + { name = "websockets", marker = "extra == 'dev'", specifier = ">=14.0,<17.0" }, + { name = "websockets", marker = "extra == 'websocket'", specifier = ">=14.0,<17.0" }, ] -provides-extras = ["opentelemetry", "dev"] +provides-extras = ["opentelemetry", "websocket", "dev"] [[package]] name = "opentelemetry-api"