diff --git a/.gitignore b/.gitignore index 9b6eb16..a4dd02d 100644 --- a/.gitignore +++ b/.gitignore @@ -79,3 +79,10 @@ docs/postman/ # (mirrors the convention that `dist/` is ignored: built / scratch # outputs never belong in VCS regardless of where they were created). src/**/*.defect* + +# Top-level files dropped by mis-decoded absolute Windows paths +# (e.g. `git status C:\Users\Anatolii\...` writes a file named +# `C:UsersAnatolii` to the repo root when the path separator is +# lost and the byte is rendered as the U+FFFD placeholder). +# Quoted because gitignore treats `:` as a path separator on its own. +"/C:UsersAnatolii*" diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f92d02..dd34cf5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,139 @@ +## [0.22.1] - 2026-10-15 + +Patch release, no protocol change. The SDK's public exception surface +is restored after QA pass u3h (2026-10-15) reported 24 classes the +SDK RAISES were not importable from the top-level `nullrun` +namespace. The classes existed, `_V3_ERROR_CODE_MAP` constructed +them, and enforcement was never affected — only the documented +public name was missing. The defect survived because the documented +catch (`except NullRunError:`) kept matching the instance; a narrower +`except NullRunApprovalDeniedError:` only fails when someone goes +looking for the name. + +The second behavioural fix closes a discarded-coroutine class of bug +on the WS dispatch path: `on_policy_invalidated` was wired as +`async def` while the dispatcher calls it without `await`, so the +callback was created and silently discarded (the dispatcher's +`try/except` cannot see an un-awaited coroutine either). The +`logger.info` and the user-supplied callback never fired — only a +stderr `RuntimeWarning` that nobody reads. Same bug class as the +approval-resolved fix from 2026-07-24, missed on the sibling +callback. + +The third change aligns stale docstrings with current behaviour: +the kill-vs-on_error-hook asymmetry between sync and async `@protect` +was documented as a `BaseException` bypass that never existed (the +class is `WorkflowKilledInterrupt -> NullRunError -> BreakerError -> +Exception` — verified), and chasing the truth exposed an actual +behavioural split governed by `unify_block`, not by base-class +membership. The new `tests/test_guard_kill_semantics.py` pins the +real behaviour so the comments describing it cannot drift the way the +old ones did. + +### Migration + +None on a healthy configuration. Three things differ from 0.22.0: + +1. **24 more exception names are reachable via `from nullrun import + X`** (DEF-PUBEX-01). Code that imports a previously-missing name + will now succeed. The names are also added to `__all__` so + tab-completion and `from nullrun import *` reach them. See the + `Fixed` section below for the full list and rationale by tier + (infrastructure / transport / decision / approval). +2. **`on_policy_invalidated` fires on every WS message** (DEF-WS-SYNC-01). + Pre-0.22.1 the wrapper was an un-awaited coroutine; the + `logger.info` and the user-supplied callback never ran. Code that + subscribed to policy-cache invalidations expecting a callback + will now actually get one. +3. **No wire or backend change.** This is purely SDK-side restoration + and comment alignment. + +### Fixed + +- **DEF-PUBEX-01 (public exception surface)** — 24 classes the SDK + RAISES were missing from `nullrun.__init__._LAZY_EXPORTS` and + `__all__`. `from nullrun import NullRunApprovalDeniedError` + raised `ImportError` while `except NullRunError:` in the same + file kept matching the instance, so the defect survived every + existing test. Now registered in `_LAZY_EXPORTS`, grouped by tier + (root / markers, infrastructure, transport, decision, approval), + and reachable via `from nullrun import X`. `BreakerError` and + `TransportErrorSource` are deliberately absent from `__all__`: + the first is the pre-Layer-1 root alias kept for old cookbook + code (already offered by tab-completion via `NullRunError`), the + second is an enum a caller passes to `RateLimitError`, not + something to catch. `NullRunUnclassifiedRefusalError` lives in + `nullrun.breaker.categories`, not `.exceptions`, and is the + reason a caller must NOT assume every block carries a policy + verdict — ADR-062 §2.2 requires it to RAISE when a refusal + carries no category the SDK trusts. The full list: BreakerError, + NullRunDecision, NullRunInfrastructureError, + NullRunTransportError, BreakerTransportError, + InsecureTransportError, NullRunProtocolError, + NullRunMalformedGateResponseError, NullRunRateLimitRedisError, + NullRunBudgetRecheckFailedError, NullRunExecutionNotFoundError, + RateLimitError, TransportErrorSource, NullRunUnclassifiedRefusalError, + NullRunDeniedError, NullRunChainError, + NullRunConsumeOverbudgetError, NullRunWorkflowInactiveError, + NullRunApprovalNotYetApprovedError, + NullRunApprovalResponseMissingError, NullRunApprovalDeniedError, + NullRunApprovalExpiredError, NullRunApprovalReplayRejectedError, + NullRunApprovalDigestMismatchError, + NullRunApprovalToolDigestMismatchError. +- **DEF-WS-SYNC-01 (sync `on_policy_invalidated` wrapper)** — + `Transport.connect_websocket` wired `on_policy_invalidated` as + `async def` while the WS dispatch path calls it without `await`. + Constructing an un-awaited coroutine raises nothing, so the + dispatcher's `try/except` could not catch it either. The body + never ran; the `logger.info` and the user-supplied + `on_policy_invalidated` never fired. Same shape as the approval + fix from 2026-07-24, missed on the sibling callback. Now a plain + `def`. `wrapped_key_rotated` stays `async def` because it awaits + `self._refetch_credentials()` and its dispatcher DOES await it. + +### Cleanup + +- Dropped `C:UsersAnatolii` (216 bytes — contents are a one-line + `fatal: ambiguous argument` message from a previous shell that + mis-decoded an absolute Windows path and wrote the error output + into the repo root). `.gitignore` extended with `"/C:UsersAnatolii*"` + so the shape cannot recur; the broader class + ("Windows path decoded without its separator") produces + single-byte replacement-char filenames that don't pattern-match + cleanly, so the entry is narrow on purpose. + +### Documentation + +- **DEF-DRIFT-01 (kill-vs-on_error-hook asymmetry)** — the + `@protect` / `on_error` / `check_workflow_budget` / `messages.NR-B007` + docstring blocks were aligned with current behaviour. The + `BaseException` explanation for the hook bypass was wrong + (`WorkflowKilledInterrupt -> NullRunError -> BreakerError -> + Exception`, verified); the real split is the `unify_block` flag + in `_protect_body`. Sync `@protect` re-wraps as + `NullRunBlockedException` (error code `NR-W002`), the hook fires; + async `@protect` lets the raw `WorkflowKilledInterrupt` + propagate, the hook does NOT fire. `decision == "throttle"` does + not exist — the only producer is an uncalled phase-2 schema stub. + Fail-OPEN is narrower than "any transport error": a 5xx WITH a + refusal body is HONOURED (decision_source=`GATEWAY`, blocks); + only unreachable / synthetic-source paths fail open. `NR-B007` + was removed 2026-09-10 (it only ever fired on the never-sent + `throttle` decision); `WorkflowPausedException` is NOT zombie + and must not be lumped in with `NR-B007` — it is still raised + for a genuinely paused workflow. + +### Verification + +| Check | Result | +|---|---| +| `ruff check src tests` | All checks passed | +| `mypy src/nullrun` | Success: no issues found in 39 source files | +| `pytest -q` | **1849 passed, 12 skipped** in 124.76s (vs baseline 1828 / 12 — **+21 new tests** from `test_public_exception_surface`, `test_policy_invalidated_ws_sync_callback`, `test_guard_kill_semantics`) | +| Scratch diff | clean (`dist_local/`, `*.defect*` absent) | +| `nullrun.__version__` | `0.22.1` | +| Wire-format compatibility | unchanged from 0.22.0 | + ## [0.22.0] - 2026-10-09 The SDK's enforcement question can now go to a local box that holds a diff --git a/pyproject.toml b/pyproject.toml index 5dfd007..bac0f27 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.0" +version = "0.22.1" # 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/__init__.py b/src/nullrun/__init__.py index f79fefb..6151eae 100644 --- a/src/nullrun/__init__.py +++ b/src/nullrun/__init__.py @@ -133,8 +133,10 @@ def on_error(hook): What does NOT fire the hook: - *:class:`WorkflowKilledInterrupt` (BaseException subclass) - — kill is a non-recoverable signal, not an error. + *:class:`WorkflowKilledInterrupt` on the async ``@protect`` + path — kill is a non-recoverable signal, not an error. On the + sync path it is re-wrapped as ``NullRunBlockedException`` + (error code ``NR-W002``) and that wrapper DOES fire the hook. * Non-``NullRunError`` exceptions (e.g. raw ``httpx`` errors from SDK-internal code paths not yet migrated to the structured hierarchy). @@ -484,6 +486,85 @@ def my_agent: "NullRunBackendError": ("nullrun.breaker.exceptions", "NullRunBackendError"), "NullRunBudgetError": ("nullrun.breaker.exceptions", "NullRunBudgetError"), "NullRunToolBlockedError": ("nullrun.breaker.exceptions", "NullRunToolBlockedError"), + # The rest of the exception hierarchy. + # + # 2026-10-15 (QA u3h): 24 classes the SDK can RAISE were missing + # from this table, so `from nullrun import + # NullRunApprovalDeniedError` raised ImportError while + # `except NullRunError:` in the caller's own code still caught + # the instance. The classes existed, `_V3_ERROR_CODE_MAP` + # constructed them, and enforcement was never affected — only the + # public import was broken. The failure was invisible because the + # documented catch is the BASE class: a cookbook written against + # `except NullRunError:` passes in production and the missing + # narrower name is only discovered when a user goes looking for + # it. That is the whole point of the typed catalog (ADR-062 + # §2.2), so the names have to be reachable. + # + # Grouped by tier, so a reader can see the hierarchy without + # reading exceptions.py: + # root / markers — catch-all `except` targets + # infrastructure — the SDK could not reach or read the backend + # decision — the backend reached a deliberate verdict + # transport — network / protocol shape + "BreakerError": ("nullrun.breaker.exceptions", "BreakerError"), + "NullRunDecision": ("nullrun.breaker.exceptions", "NullRunDecision"), + "NullRunInfrastructureError": ("nullrun.breaker.exceptions", "NullRunInfrastructureError"), + # Infrastructure — transport, protocol, backend-unreadable. + "NullRunTransportError": ("nullrun.breaker.exceptions", "NullRunTransportError"), + "BreakerTransportError": ("nullrun.breaker.exceptions", "BreakerTransportError"), + "InsecureTransportError": ("nullrun.breaker.exceptions", "InsecureTransportError"), + "NullRunProtocolError": ("nullrun.breaker.exceptions", "NullRunProtocolError"), + "NullRunMalformedGateResponseError": ( + "nullrun.breaker.exceptions", + "NullRunMalformedGateResponseError", + ), + "NullRunRateLimitRedisError": ("nullrun.breaker.exceptions", "NullRunRateLimitRedisError"), + "NullRunBudgetRecheckFailedError": ("nullrun.breaker.exceptions", "NullRunBudgetRecheckFailedError"), + "NullRunExecutionNotFoundError": ("nullrun.breaker.exceptions", "NullRunExecutionNotFoundError"), + "RateLimitError": ("nullrun.breaker.exceptions", "RateLimitError"), + "TransportErrorSource": ("nullrun.breaker.exceptions", "TransportErrorSource"), + # `resolve_refusal_category` raises this when the gate refuses + # without a category the SDK trusts (ADR-062 §2.2: an + # unclassifiable refusal must RAISE, never be guessed). It lives + # in `breaker.categories`, not `breaker.exceptions`, and is the + # reason a caller must NOT assume every block carries a policy + # verdict. + "NullRunUnclassifiedRefusalError": ( + "nullrun.breaker.categories", + "NullRunUnclassifiedRefusalError", + ), + # Decision — the backend refused, and the reason is knowable. + "NullRunDeniedError": ("nullrun.breaker.exceptions", "NullRunDeniedError"), + "NullRunChainError": ("nullrun.breaker.exceptions", "NullRunChainError"), + "NullRunConsumeOverbudgetError": ("nullrun.breaker.exceptions", "NullRunConsumeOverbudgetError"), + "NullRunWorkflowInactiveError": ("nullrun.breaker.exceptions", "NullRunWorkflowInactiveError"), + # The approval family. One class per wire code (NR-A010 … NR-A015) + # so a caller can branch on WHICH approval failure occurred + # instead of string-matching the message. The base + # `NullRunBlockedException` collapses all of them into one. + "NullRunApprovalNotYetApprovedError": ( + "nullrun.breaker.exceptions", + "NullRunApprovalNotYetApprovedError", + ), + "NullRunApprovalResponseMissingError": ( + "nullrun.breaker.exceptions", + "NullRunApprovalResponseMissingError", + ), + "NullRunApprovalDeniedError": ("nullrun.breaker.exceptions", "NullRunApprovalDeniedError"), + "NullRunApprovalExpiredError": ("nullrun.breaker.exceptions", "NullRunApprovalExpiredError"), + "NullRunApprovalReplayRejectedError": ( + "nullrun.breaker.exceptions", + "NullRunApprovalReplayRejectedError", + ), + "NullRunApprovalDigestMismatchError": ( + "nullrun.breaker.exceptions", + "NullRunApprovalDigestMismatchError", + ), + "NullRunApprovalToolDigestMismatchError": ( + "nullrun.breaker.exceptions", + "NullRunApprovalToolDigestMismatchError", + ), # Layer 2: on_error context type "ErrorContext": ("nullrun.observability.error_hooks", "ErrorContext"), # Layer 3: status dataclasses @@ -522,8 +603,22 @@ def my_agent: # nullrun/_handle.py for the rationale). Pair with @nullrun.protect # so a typical ``run an agent and print a friendly message on # failure`` script needs no explicit try/except around - # NullRunError. WorkflowKilledInterrupt (BaseException) still - # propagates — kill is never swallowed. + # NullRunError. + # + # Kill behaviour is ASYMMETRIC between the two @protect + # wrappers, and this comment used to paper over it by calling + # WorkflowKilledInterrupt a BaseException: + # async @protect → raw WorkflowKilledInterrupt propagates → + # guard re-raises it explicitly (see _handle.py) → the + # agent loop sees the kill. + # sync @protect → the wrapper substitutes + # NullRunBlockedException (error_code NR-W002), which is a + # NullRunDecision and NOT a WorkflowKilledInterrupt + # subclass → guard's isinstance re-raise does not match → + # it prints a report and sys.exit(1). + # So `with nullrun.guard()` + a sync @protect turns a kill into + # a graceful exit. Pinned by + # tests/test_guard_kill_semantics.py. # # History: 0.18.4 renamed this from ``handle`` to ``guard``. # The module file name is still ``_handle.py`` for the @@ -627,6 +722,43 @@ def __dir__() -> list[str]: "NullRunMcpReadonlyBypassBlockedError", "NullRunMcpApprovalRequiredError", "NullRunApprovalDbUnavailableError", + # The narrower `except` targets. Same 2026-10-15 reasoning as the + # `_LAZY_EXPORTS` block above: every name here is a class the SDK + # raises, and a caller cannot branch on a name that tab-completion + # does not offer. Ordering follows the tier hierarchy — + # infrastructure, then transport, then decision, then approval — + # so the list reads top-down as "how bad is this". + # + # `BreakerError` is deliberately absent: it is the pre-Layer-1 + # root alias, kept only so `except BreakerError` in old cookbook + # code keeps matching. It is reachable via + # `nullrun.breaker.exceptions` and adds nothing to the + # tab-completion list, where `NullRunError` is the name to use. + # `TransportErrorSource` is likewise absent — it is an enum a + # caller passes to `RateLimitError`, not something to catch. + "NullRunDecision", + "NullRunInfrastructureError", + "NullRunTransportError", + "BreakerTransportError", + "InsecureTransportError", + "NullRunProtocolError", + "NullRunMalformedGateResponseError", + "NullRunRateLimitRedisError", + "NullRunBudgetRecheckFailedError", + "NullRunExecutionNotFoundError", + "RateLimitError", + "NullRunUnclassifiedRefusalError", + "NullRunDeniedError", + "NullRunChainError", + "NullRunConsumeOverbudgetError", + "NullRunWorkflowInactiveError", + "NullRunApprovalNotYetApprovedError", + "NullRunApprovalResponseMissingError", + "NullRunApprovalDeniedError", + "NullRunApprovalExpiredError", + "NullRunApprovalReplayRejectedError", + "NullRunApprovalDigestMismatchError", + "NullRunApprovalToolDigestMismatchError", # User-facing message catalog — the single entry point for # turning an SDK exception into a string safe to display to # end users. ``set_user_message`` lets a deployment brand its diff --git a/src/nullrun/__version__.py b/src/nullrun/__version__.py index 2e19310..1191ed4 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.0" +__version__ = "0.22.1" __platform_version__ = "1.0.0" diff --git a/src/nullrun/decorators.py b/src/nullrun/decorators.py index b142b71..d18ac1a 100644 --- a/src/nullrun/decorators.py +++ b/src/nullrun/decorators.py @@ -617,13 +617,19 @@ def g:... # ``ToolParamsExtractor(include_all=True)`` here and stored it # on the function via ``_nullrun_extractor``. That path was # removed because it forced the SDK to know what an "extractor" - # is. The 0.18.2 design is simpler: ``@protect`` has no - # per-function state. Every call constructs an opaque - # ``NoImpact`` envelope locally and forwards it to - # ``runtime.execute(...)``. All policy decisions + # is. ``@protect`` has no per-function state. Every call builds + # a ``BusinessImpact`` envelope locally (``_build_call_impact``) + # and forwards it to ``runtime.execute(...)``. + # + # ADR-065 (0.21.0) changed WHICH envelope: it used to be a + # constant ``NoImpact`` on every call, which hashed to a constant + # and so bound each stored approval to no action at all. It is now + # a ``tool_call`` envelope carrying the tool name and its masked + # kwargs, built once per call before the /gate pre-flight so + # /gate and /execute carry identical bytes. All policy decisions # (allow / block / require-approval) live on the backend; the - # SDK only relays (tool_name, kwargs, args) and renders the - # decision back into an exception class. + # SDK only relays the envelope and renders the decision back into + # an exception class. @contextlib.contextmanager def _protect_body(args: tuple[Any, ...], kwargs: dict[str, Any], unify_block: bool): @@ -722,7 +728,7 @@ def _protect_body(args: tuple[Any, ...], kwargs: dict[str, Any], unify_block: bo # 4. Per-tool policy gate via /execute. Runs on EVERY # @protect call (no extractor / no short-circuit). The # SDK is policy-blind; it ships tool_name + args + kwargs - # + the NoImpact envelope/digest to the backend and the + # + the tool_call envelope/digest to the backend and the # backend decides allow/block/require-approval. _run_tool_policy_gate(runtime, fn, args, kwargs) @@ -753,10 +759,25 @@ def _protect_body(args: tuple[Any, ...], kwargs: dict[str, Any], unify_block: bo # Layer 2: fire the on_error hook. Kill/pause is a # user-visible state change (the dashboard did # this) so most observability hooks want to know - # about it. Note: the underlying kill signal - # itself (WorkflowKilledInterrupt) does NOT fire - # the hook (BaseException bypass) — only this - # re-wrapped form does. + # about it. + # + # Only THIS form fires the hook, and the reason is + # `unify_block`, not the exception's base class. An + # earlier version of this comment claimed a + # "BaseException bypass": `WorkflowKilledInterrupt` + # derives from NullRunError -> BreakerError -> + # Exception (verified), so there is no BaseException + # in play to bypass. The real split is the wrapper + # flag — see `_protect_body`'s docstring: + # + # sync (unify_block=True) → re-wrapped here, + # hook fires with error_code NR-W002 / NR-W003. + # async (unify_block=False) → falls through to the + # bare `raise`, raw WorkflowKilledInterrupt + # propagates, hook does NOT fire. + # + # If you change the async behaviour, change this + # comment in the same commit. runtime._emit_sdk_error(err, stage="decorator", workflow_id=exc.workflow_id) raise err from exc raise diff --git a/src/nullrun/messages.py b/src/nullrun/messages.py index ac0cf99..73ca1f4 100644 --- a/src/nullrun/messages.py +++ b/src/nullrun/messages.py @@ -144,10 +144,16 @@ # the next /gate will mint a fresh reservation against the current # available budget. "NR-B006": "Your request couldn't be completed because the available capacity changed. Please try again.", - # zombie class — never raised on a wire or runtime path - # (runtime.py:2116 raises WorkflowPausedException on - # decision=="throttle"). Catalog entry removed to keep + # zombie ERROR CODE — ``NullRunBudgetThrottleError`` (NR-B007) was + # removed 2026-09-10: it only ever fired on decision=="throttle", + # which the backend never sends. Catalog entry removed to keep # messages in sync with the exception module. + # + # NOTE: ``WorkflowPausedException`` itself is NOT zombie and must + # not be lumped in with this — it is still raised for a genuinely + # paused workflow (``runtime.py`` pause pre-flight, and the + # ``pause`` action handler in ``actions.py``). It simply never + # carried NR-B007. # "NR-B007": "...", # NR-O001: consume > reserve + ε tolerance. ADR-005 invariant; # the SDK rejects rather than silently re-reserving. User-facing diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index b87db6b..4651768 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -2167,18 +2167,52 @@ def check_workflow_budget(self) -> None: budget never gets to spend tokens. Decision → exception mapping: - "block" → NullRunBudgetError (NR-B004, hard policy / - reservation error; 2026-09-08 typed signal) - "throttle"→ WorkflowPausedException (insufficient budget, can resume) "allow" → return - - Fail-OPEN: any transport error (network, timeout, 5xx) is logged - at warning level and the caller proceeds. This mirrors the - pattern in `check_control_plane` -- a transient backend outage - must never freeze the user's agent. The /track fast path also - does not gate on budget, so the worst case under /gate failure - is that budget enforcement is advisory until the gateway - recovers. + "block" / "require_approval" / "deny" → typed by the + dispatch that follows. The mapping is by MACHINE-READABLE + WIRE CODE (``_build_v3_error_code_map``), not by the + decision string: ``RATE_LIMIT_EXCEEDED`` → + ``RateLimitError``, ``TOOL_BLOCKED`` → + ``NullRunToolBlockedError``, ``CIRCUIT_BREAKER_TRIPPED`` + → the typed breaker class, and a budget refusal → + ``NullRunBudgetError`` (NR-B004). Since 0.21.0 a rate + limit no longer surfaces as ``NullRunBudgetError``, so + code branching on the class branches on the real cause. + A refusal with no machine-readable code still raises + ``NullRunBudgetError`` (legacy tier, pinned). + + There is NO ``"throttle"`` decision. The arm that mapped + it was removed in 0.22.0 as unreachable — the backend's + only producer of that string is an uncalled phase-2 + schema stub. Code written against the old docstring will + catch the wrong class. + + Fail-OPEN is NARROWER than "any transport error". Precisely: + + - Network error / timeout / 5xx with NO refusal body → + the transport synthesizes a block carrying + ``decision_source=FALLBACK``; ``is_fallback_decision_ + source`` matches, so we log, count + ``gate_fail_open_total``, and PROCEED. + - 5xx (or any status) WITH a real refusal body → ADR-062 + §2.2 classifies it and returns + ``decision_source=GATEWAY``. That is a real backend + decision, so it is HONOURED and BLOCKS. A 5xx is not + automatically fail-open. + - Credential failure (``NullRunAuthenticationError``), + malformed body, and ``NullRunUnclassifiedRefusalError`` + all `raise` rather than fail open. Ordering matters: + each precedes the bare ``except Exception`` arm, which is + a superset of all three. + - A non-fallback synthetic source is likewise treated as a + transport error and fails open. + + The rationale for the fail-open arms is that a transient backend + outage must never freeze the user's agent, and `/track` does not + gate on budget, so the worst case while `/gate` is unreachable is + that enforcement is advisory until the gateway recovers. That + rationale covers UNREACHABLE, not REFUSING — which is why the + ADR-062 arms were split out above. Uses `estimated_tokens=1` (the minimum the API accepts). Goal is the binary question "is there any budget left?", not cost diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index 60d7402..fe1a270 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -3586,7 +3586,13 @@ def check( Returns: Dict with: - - decision: "allow" | "block" | "throttle" + - decision: one of ``allow`` | ``block`` | + ``soft_pass`` | ``require_approval`` | ``deny`` + (the canonical set is + ``Runtime._KNOWN_GATE_DECISIONS``; anything else is + rejected rather than guessed at). There is NO + ``"throttle"`` — the backend's only producer of that + string is an uncalled phase-2 schema stub. - reservation_id: Optional reservation ID - remaining_budget_cents: Remaining budget - projected_cost_cents: Projected cost for this operation @@ -3758,8 +3764,8 @@ def _do_gate_post() -> httpx.Response: if response.status_code == 200: return response.json() # type: ignore[no-any-return] # 4xx is a REAL gate decision — surface it through the - # existing block / throttle / soft_pass dispatch in - # runtime.check_workflow_budget. The runtime's + # existing block / soft_pass / require_approval / deny + # dispatch in runtime.check_workflow_budget. The runtime's # ``decision_source != fallback`` check honours the wire # decision and raises ``NullRunBudgetError`` via its # existing ``decision=="block"`` arm. The wire @@ -4001,7 +4007,15 @@ async def connect_websocket( # 0.7.0 thin client: no local policy cache; the next /gate or /execute # call re-reads from the backend. Just forward the notification. - async def wrapped_policy_invalidated(ws_id: str, policy_id: str, new_version: int) -> None: + # + # Synchronous adapter: ``transport_websocket.py`` dispatches + # ``self.on_policy_invalidated(...)`` without awaiting it, so an + # `async def` here produced an un-awaited coroutine — the body + # never ran, and because constructing one raises nothing the + # dispatcher's `try/except` could not catch it either. The + # callback was created and silently discarded. Same shape as + # `wrapped_approval_resolved` below, for the same reason. + def wrapped_policy_invalidated(ws_id: str, policy_id: str, new_version: int) -> None: logger.info(f"Policy {policy_id} invalidated (v{new_version})") if on_policy_invalidated: on_policy_invalidated(ws_id, policy_id, new_version) @@ -4147,10 +4161,18 @@ def track_single( ``_capture_server_minted_execution_id``) * ``workflow_id`` (str, the workflow the call belongs to) * ``tokens`` (int, sum of input + output tokens) - * ``cost_cents`` (int, ``0`` — backend computes the - authoritative cost from tokens + the org's - pricing policy; sending a wrong number risks - double-billing, see _WIRE_STRIP_FIELDS in runtime.py) + * ``cost_cents`` (int — NOT SENT as of ADR-068 §4f. + The backend computes the authoritative cost + from tokens + the org's pricing policy. The + SDK reports tokens because only it knows + them; it never offers a price. The field is + stripped from the wire by + ``_WIRE_STRIP_FIELDS`` (runtime.py) — an + older SDK that still sends it is accepted + and dropped server-side via + ``#[serde(default)]``. ``local_cost_cents`` + in the RETURN value is unchanged and is + never on the wire.) * ``cost_source`` (str, ``"provisional"`` / ``"authoritative"`` per — SDK always emits ``"provisional"``) @@ -4182,9 +4204,10 @@ def track_single( NullRunAuthenticationError: 401/403. Wire contract: ``TrackRequestRaw`` is - ``{workflow_id, tokens, cost_cents,...}``; ``execution_id`` - is replaced by ``reservation_id``, ``actual_cost_cents`` is - replaced by ``cost_cents`` (the SDK always sends 0 — see + ``{workflow_id, tokens,...}``; ``execution_id`` + is replaced by ``reservation_id``, ``actual_cost_cents`` + is replaced by nothing (ADR-068 §4f removed the + client-priced field entirely — see ``_WIRE_STRIP_FIELDS``), and ``api_key_id`` is derived server-side from the request auth, not supplied by the SDK. """ diff --git a/tests/test_guard_kill_semantics.py b/tests/test_guard_kill_semantics.py new file mode 100644 index 0000000..777934f --- /dev/null +++ b/tests/test_guard_kill_semantics.py @@ -0,0 +1,113 @@ +"""What ``nullrun.guard()`` does with a kill — and why it differs +between the sync and async ``@protect`` wrappers. + +Why this test exists (2026-10-15) +--------------------------------- +A QA pass flagged a comment in ``decorators.py`` that claimed +``WorkflowKilledInterrupt`` was a ``BaseException`` subclass and +therefore "bypassed" the ``on_error`` hook. It is not: the class is + + WorkflowKilledInterrupt -> NullRunError -> BreakerError -> Exception + +so the explanation was wrong. Chasing the truth exposed something +the wrong explanation had been hiding — an actual behavioural split: + +* async ``@protect`` (``unify_block=False``) lets the raw + ``WorkflowKilledInterrupt`` propagate. ``guard`` re-raises it + explicitly, so the kill reaches the agent loop. +* sync ``@protect`` (``unify_block=True``) substitutes + ``NullRunBlockedException`` (error_code ``NR-W002``), which is a + ``NullRunDecision`` and **not** a ``WorkflowKilledInterrupt`` + subclass. ``guard``'s ``isinstance`` re-raise therefore does not + match: it prints a report and calls ``sys.exit(1)``. + +These tests pin the real behaviour so the comments describing it +cannot go stale the way the old one did. +""" + +from __future__ import annotations + +import pytest + +from nullrun import NullRunBlockedException, WorkflowKilledInterrupt, guard +from nullrun.breaker.exceptions import WorkflowPausedException + + +class TestKillIsAnExceptionSubclass: + """The premise every stale comment got wrong.""" + + def test_killed_is_an_exception_not_a_base_exception_only_type(self): + # If this fails, WorkflowKilledInterrupt became a + # BaseException-only subtype again and every docstring saying + # "Exception subclass" is wrong. + assert issubclass(WorkflowKilledInterrupt, Exception), ( + "WorkflowKilledInterrupt must be an Exception subclass. " + "Docs, middleware rationale and the fastapi 503 path all " + "depend on this; they previously described it as a " + "BaseException subclass, which it has not been since the " + "Layer-1 hierarchy migration." + ) + + def test_paused_is_an_exception_subclass(self): + assert issubclass(WorkflowPausedException, Exception) + + +class TestSyncWrapperSubstitution: + """The sync/async asymmetry that the stale comment concealed.""" + + def test_blocked_is_not_a_workflow_killed_interrupt(self): + # This single fact is what makes `guard` behave differently on + # the two paths: its `isinstance(exc, WorkflowKilledInterrupt)` + # re-raise cannot match the sync wrapper's substitute. + assert not issubclass(NullRunBlockedException, WorkflowKilledInterrupt), ( + "If NullRunBlockedException became a WorkflowKilledInterrupt " + "subclass, guard's re-raise would start matching on the sync " + "path too and the asymmetry documented in __init__.py would " + "be gone — update that comment in the same commit." + ) + + def test_blocked_carries_the_kill_error_code(self): + err = NullRunBlockedException( + workflow_id="wf-1", + reason="killed", + error_code="NR-W002", + ) + assert err.error_code == "NR-W002" + + +class TestGuardPropagatesRawKill: + """Async path: the kill escapes ``guard`` untouched.""" + + def test_guard_reraises_workflow_killed_interrupt(self): + with pytest.raises(WorkflowKilledInterrupt): + with guard(): + raise WorkflowKilledInterrupt("wf-1", reason="killed") + + +class TestGuardTurnsSyncBlockIntoExit: + """Sync path: ``guard`` prints and exits instead of propagating.""" + + def test_guard_exits_instead_of_propagating_the_blocked_wrapper( + self, capsys, monkeypatch + ): + monkeypatch.setattr("sys.exit", _raise_system_exit) + with pytest.raises(SystemExit) as info: + with guard(exit_code=7): + raise NullRunBlockedException( + workflow_id="wf-1", + reason="killed", + error_code="NR-W002", + ) + assert info.value.code == 7, ( + "guard must honour its exit_code argument on the sync-block " + "path. If this regressed to propagating the kill, remove " + "this test and fix the __init__.py comment." + ) + assert capsys.readouterr().err.strip() != "", ( + "guard must write a report to stderr before exiting, " + "otherwise the kill vanishes with no trace." + ) + + +def _raise_system_exit(code: int): + raise SystemExit(code) \ No newline at end of file diff --git a/tests/test_policy_invalidated_ws_sync_callback.py b/tests/test_policy_invalidated_ws_sync_callback.py new file mode 100644 index 0000000..02b0bc1 --- /dev/null +++ b/tests/test_policy_invalidated_ws_sync_callback.py @@ -0,0 +1,136 @@ +""" +Regression (2026-10-10): ``Transport.connect_websocket`` wired +``on_policy_invalidated`` as an ``async def`` while the WS dispatch +path calls it **synchronously**. + +Same bug class as ``test_approval_ws_sync_callback.py`` (fixed +2026-07-24), missed on the sibling callback. Observed live on a real +init against production:: + + RuntimeWarning: coroutine 'Transport.connect_websocket.. + wrapped_policy_invalidated' was never awaited + self.on_policy_invalidated(organization_id, policy_id, new_version) + +The body never runs. Worse than the approval case: the dispatcher's +``try/except`` cannot catch it either, because constructing an +un-awaited coroutine raises nothing. The callback is created and +silently discarded, so the ``logger.info`` and the user-supplied +``on_policy_invalidated`` never fire. + +The file already documented the rule one function below the defect:: + + # Synchronous adapter: dispatch is dict-only, not awaitable. An + # async def would produce a coroutine the handler ignores. + def wrapped_approval_resolved(payload): ... + +``wrapped_key_rotated`` is genuinely ``async`` and correctly so — it +awaits ``self._refetch_credentials()``, and its dispatcher DOES +await it. ``on_policy_invalidated`` awaits nothing, so it must be +sync. These tests pin both halves so neither drifts back. +""" + +from __future__ import annotations + +import asyncio +import inspect + +from nullrun.transport import Transport +from nullrun.transport_websocket import WebSocketConnection + + +def _capture_wrapped_callbacks(**callbacks) -> dict[str, object]: + """Run ``connect_websocket`` and return what it hands the connection. + + The wrappers live in a closure, so we capture the kwargs at the + ``WebSocketConnection(...)`` boundary instead of re-implementing + them. The real ``__init__`` is skipped — we only need the values. + """ + captured: dict[str, object] = {} + + real_init = WebSocketConnection.__init__ + real_connect = WebSocketConnection.connect + + def _capturing_init(self, *args, **kwargs): + for name in ( + "on_policy_invalidated", + "on_key_rotated", + "on_approval_resolved", + "on_state_change", + ): + captured[name] = kwargs.get(name) + + async def _no_connect(self): # pragma: no cover - never reached + return None + + WebSocketConnection.__init__ = _capturing_init # type: ignore[method-assign] + WebSocketConnection.connect = _no_connect # type: ignore[method-assign] + try: + transport = Transport(api_url="https://api.nullrun.io", api_key="test-key") + asyncio.run(transport.connect_websocket(organization_id="org-1", **callbacks)) + finally: + WebSocketConnection.__init__ = real_init + WebSocketConnection.connect = real_connect + return captured + + +def test_wrapped_policy_invalidated_is_synchronous(): + """``on_policy_invalidated`` must be a plain ``def``. + + The dispatcher calls it without ``await`` and inside a + ``try/except`` that can never see an un-awaited coroutine — so + the wrong shape fails silently rather than loudly. + """ + captured = _capture_wrapped_callbacks() + wrapped = captured["on_policy_invalidated"] + + assert not inspect.iscoroutinefunction(wrapped), ( + "on_policy_invalidated wrapper must be a plain function; " + "async def produces an un-awaited coroutine, the dispatcher " + "discards it, and neither the log line nor the user callback " + "ever runs." + ) + assert callable(wrapped) + + +def test_wrapped_policy_invalidated_invokes_callback_synchronously(): + """Calling the wrapper must run the body — and run it once. + + This is the assertion the 2026-07-24 approval test had and the + policy callback never got: an ``async def`` wrapper still passes a + naive ``callable()`` check and still passes an + ``iscoroutinefunction`` check on the WRONG object. Only calling it + and observing the side effect distinguishes "defined async" from + "defined sync but forgot to run". + """ + received: list[tuple] = [] + + captured = _capture_wrapped_callbacks( + on_policy_invalidated=lambda ws_id, policy_id, ver: received.append( + (ws_id, policy_id, ver) + ) + ) + wrapped = captured["on_policy_invalidated"] + + wrapped("org-1", "pol-42", 7) + + assert received == [("org-1", "pol-42", 7)], ( + "the wrapper did not forward synchronously — with an async def " + "the callback is never invoked" + ) + + +def test_wrapped_key_rotated_stays_async(): + """``on_key_rotated`` IS legitimately async — pin it. + + It awaits ``self._refetch_credentials()`` and its dispatcher DOES + await it, so it must stay ``async def``. Written so a future + "make everything sync" sweep does not silently break key rotation + (the credentials refetch would become a fire-and-forget coroutine). + """ + captured = _capture_wrapped_callbacks() + wrapped = captured["on_key_rotated"] + + assert inspect.iscoroutinefunction(wrapped), ( + "on_key_rotated must stay async — it awaits " + "_refetch_credentials() and the dispatcher awaits it" + ) \ No newline at end of file diff --git a/tests/test_public_exception_surface.py b/tests/test_public_exception_surface.py new file mode 100644 index 0000000..b680bfb --- /dev/null +++ b/tests/test_public_exception_surface.py @@ -0,0 +1,249 @@ +"""Every exception class the SDK can raise must be importable from the +top-level ``nullrun`` namespace, and must appear in ``__all__``. + +Why this test exists (QA u3h, 2026-10-15) +----------------------------------------- +24 classes the SDK RAISES were missing from +``nullrun.__init__._LAZY_EXPORTS``. ``from nullrun import +NullRunApprovalDeniedError`` raised ``ImportError`` while +``except NullRunError:`` in the same file still caught the instance +— because ``NullRunApprovalDeniedError`` IS a ``NullRunError``. + +That asymmetry is what made the defect survive: enforcement was +never touched (the classes existed, ``_V3_ERROR_CODE_MAP`` +constructed them), and the documented catch is the BASE class. A +cookbook written against ``except NullRunError:`` passes in +production, so the missing narrower name is only discovered when +someone goes looking for it. The typed catalog exists precisely so a +caller can branch on WHICH refusal occurred — a name that cannot be +imported cannot be branched on. + +Two failure modes, so two tests: + +1. ``test_every_raisable_exception_is_importable_from_top_level`` — + enumerates the exception classes by parsing the source, then + imports each one. Catches a class being REMOVED from the export + tables. +2. ``test_a_new_exception_class_cannot_be_added_without_an_export`` — + fails if a class is ADDED to ``exceptions.py`` / ``categories.py`` + and not exported. This is the direction the original defect would + have been caught in, had the test existed. + +Both read the source rather than hardcode a list, so neither goes +stale. +""" + +from __future__ import annotations + +import ast +import re +from pathlib import Path + +import pytest + +import nullrun +from nullrun.breaker import categories as cat_mod +from nullrun.breaker import exceptions as exc_mod + +SRC_ROOT = Path(__file__).resolve().parents[1] / "src" / "nullrun" + +# Modules that define raisable exceptions. `categories.py` holds +# NullRunUnclassifiedRefusalError, which ADR-062 §2.2 requires the SDK +# to RAISE when a refusal carries no category it trusts — putting it +# in the scan is what stops the "an unclassifiable refusal must raise" +# guarantee from having an unimportable outcome. +EXCEPTION_MODULES = (SRC_ROOT / "breaker" / "exceptions.py", SRC_ROOT / "breaker" / "categories.py") + +# Modules that own the corresponding live objects, for the identity +# assertion. +LIVE_MODULES = (exc_mod, cat_mod) + + +def _defined_exception_names() -> list[str]: + """Every ``class X(Exception-ish)`` defined in the scanned modules. + + AST rather than regex: a class name inside a docstring or a + comment must not count, and the existing source-pin tests in this + repo have already been burned by a pin reading text that a + comment could satisfy (memory `pin-cannot-forbid-string-its-own-doc-quotes`). + """ + names: list[str] = [] + for path in EXCEPTION_MODULES: + tree = ast.parse(path.read_text(encoding="utf-8")) + for node in ast.walk(tree): + if not isinstance(node, ast.ClassDef): + continue + # Only exception classes: the base is a name we can resolve + # without importing, so this cannot recurse. + if not node.bases: + continue + base_names = [b.id for b in node.bases if isinstance(b, ast.Name)] + # Resolve transitively against the modules under scan plus + # the live modules. `Exception` / `BaseException` / + # `Enum` terminate the walk. + if _is_exception_base(base_names): + names.append(node.name) + return sorted(set(names)) + + +def _is_exception_base(base_names: list[str]) -> bool: + """True when any base is (or derives from) ``Exception``. + + Note what is NOT here: ``Enum`` and ``str`` are not exception + bases. ``DecisionCategory(str, Enum)`` and + ``TransportErrorSource(str, Enum)`` live in the scanned modules and + are values a caller may want to name, but a caller never + ``except``s them — so they are outside what this module promises + and must not be counted into the scan. + """ + if any(b in {"Exception", "BaseException"} for b in base_names): + return True + for module in LIVE_MODULES: + for b in base_names: + candidate = getattr(module, b, None) + if isinstance(candidate, type) and issubclass(candidate, Exception): + return True + return False + + +def _exported_names() -> set[str]: + """Names registered in ``_LAZY_EXPORTS``.""" + src = (SRC_ROOT / "__init__.py").read_text(encoding="utf-8") + table = src.split("_LAZY_EXPORTS: dict", 1)[1].split("\n}", 1)[0] + return set(re.findall(r'"([A-Za-z_]\w*)":\s*\(\s*"', table)) + + +RAISABLE = _defined_exception_names() +EXPORTED = _exported_names() + + +def test_the_scan_actually_found_something(): + """Guard against a vacuous pass. + + If the AST walk ever stops matching (a refactor to + ``ExceptionGroup``, a moved module), every test below would pass + with an empty name list and the guard would report green while + checking nothing — the exact + "declared ok, nothing ran" failure class. Pin a floor at the + known size and assert a few names that must be in the set. + """ + # 39 = every exception class in exceptions.py + the one in + # categories.py. Enums are deliberately not counted (see + # `_is_exception_base`), so this number must not be inflated by + # them and must not drift upward by accident either. + assert len(RAISABLE) == 39, ( + f"expected the exception scan to find exactly 39 classes, found " + f"{len(RAISABLE)}. The scan is the premise of every test in " + f"this module — if it silently stopped matching, they would " + f"all pass vacuously." + ) + for must_exist in ( + "NullRunError", + "NullRunApprovalDeniedError", + "NullRunUnclassifiedRefusalError", + "WorkflowKilledInterrupt", + ): + assert must_exist in RAISABLE, ( + f"{must_exist} must be in the scanned set; its absence " + f"means the AST walk is broken, not that the class is gone." + ) + + +def test_every_raisable_exception_is_importable_from_top_level(): + """Each class must be reachable as ``from nullrun import X``. + + This is the regression for the reported defect: the class exists, + ``__getattr__`` has no entry, and the import fails while the + instance still satisfies ``except NullRunError:``. + """ + failures = [] + for name in RAISABLE: + try: + exec(f"from nullrun import {name}", {}) # noqa: S102 + except ImportError: + failures.append(name) + assert not failures, ( + f"{len(failures)} exception class(es) the SDK raises cannot be " + f"imported from the top-level `nullrun` namespace: " + f"{sorted(failures)}. A user cannot `except` a name they cannot " + f"import. Add each to `_LAZY_EXPORTS` in " + f"src/nullrun/__init__.py. (Note the trap this defect hid " + f"behind: `except NullRunError:` kept working, so no test that " + f"only catches the base would ever notice.)" + ) + + +def test_a_new_exception_class_cannot_be_added_without_an_export(): + """A class added to the modules must be exported in the same change. + + The other direction from the reported defect, and the one worth + pinning: it turns "remember to also update ``__init__.py``" into a + red test instead of a silent hole that surfaces months later as a + user report. + + There are currently ZERO deliberate omissions — every raisable + exception is exported, including the pre-Layer-1 root alias + ``BreakerError``. If a future omission is genuinely wanted, add it + to ALLOWED_UNEXPORTED with a comment saying why; the list is empty + so any new entry is visible in the diff. + """ + unexported = sorted(name for name in RAISABLE if name not in EXPORTED) + # Compared against the documented list rather than `[]`, so the + # escape hatch in the message below is real: adding a name to + # ALLOWED_UNEXPORTED must turn this test green, or the message is + # lying about how to proceed. + assert unexported == sorted(ALLOWED_UNEXPORTED), ( + f"these exception classes are raisable but not registered in " + f"_LAZY_EXPORTS: {unexported}. A new class landed in " + f"exceptions.py / categories.py without a matching entry in " + f"src/nullrun/__init__.py _LAZY_EXPORTS. If the omission is " + f"deliberate, extend ALLOWED_UNEXPORTED below with the reason; " + f"otherwise add the export." + ) + + +# Documented, deliberate omissions from `_LAZY_EXPORTS`. Empty on +# purpose: `BreakerError` IS exported, because old cookbook code that +# catches it must keep working. If this list is ever filled in, each +# entry needs a comment saying why the class is raisable but should +# not be nameable from the top level. +ALLOWED_UNEXPORTED: set[str] = set() + + +def test_allowed_unexported_is_not_broader_than_the_scan(): + """Every documented omission must still be a real class. + + Otherwise a rename would leave a stale key in + ALLOWED_UNEXPORTED that silently permits the NEW name to go + unexported — the exemption would outlive its justification. + """ + stale = sorted(set(ALLOWED_UNEXPORTED) - set(RAISABLE)) + assert not stale, ( + f"ALLOWED_UNEXPORTED names classes that no longer exist: " + f"{stale}. Remove the stale exemption — it would otherwise " + f"let a future same-named class skip the export silently." + ) + + +@pytest.mark.parametrize( + "name", + [ + # The names the QA matrix reported as top_level_missing, kept + # explicit so a regression names the offending class instead + # of dumping the whole scan. + "NullRunApprovalNotYetApprovedError", + "NullRunApprovalResponseMissingError", + "NullRunApprovalDeniedError", + "NullRunApprovalExpiredError", + "NullRunApprovalReplayRejectedError", + "NullRunApprovalDigestMismatchError", + "NullRunApprovalToolDigestMismatchError", + "NullRunUnclassifiedRefusalError", + ], +) +def test_reported_missing_names_are_importable(name: str): + """Named regression cases from the QA report (NR-A010 … NR-A015).""" + assert hasattr(nullrun, name), ( + f"nullrun.{name} must be reachable — this is one of the " + f"top_level_missing names from the QA matrix." + ) \ No newline at end of file diff --git a/uv.lock b/uv.lock index 10d4c14..d8300bb 100644 --- a/uv.lock +++ b/uv.lock @@ -636,7 +636,7 @@ wheels = [ [[package]] name = "nullrun" -version = "0.22.0" +version = "0.22.1" source = { editable = "." } dependencies = [ { name = "httpx" },