From 03cd6edce606c27a51e50bdf413a3be53f63f9b2 Mon Sep 17 00:00:00 2001 From: Anatoly Maltsev Date: Mon, 21 Sep 2026 11:46:26 +0400 Subject: [PATCH] =?UTF-8?q?chore(release):=200.18.0=20=E2=80=94=20close=20?= =?UTF-8?q?approval-row=20orphan=20via=20auto-consume=20on=20success=20+?= =?UTF-8?q?=20exception=20paths=20(ADR-047)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add Runtime.consume_approval() + Transport.consume_approval() hitting the new POST /api/v1/approvals/{approval_id}/consume endpoint (backend 2.8.0+). Best-effort: catches all exceptions, surfaces at DEBUG, never blocks the success path. - Add Runtime.lookup_pending_approval_id_for_execution() + Runtime._mark_approval_resolved_for_execution() — RLock-guarded reverse-index (execution_id -> approval_id) populated by the WS push handler. - check_workflow_budget on outcome=approved: auto-call consume_approval + pop reverse index BEFORE return. Closes the structural orphan where mode=inline tools left approval rows at status=APPROVED past expires_at. - _safe_cancel_active_execution: after cancel_execution, ALSO call consume_approval if a pending approval_id was captured for this execution_id. Idempotent — if no approval was captured the lookup returns None and this is a no-op. - Bump version 0.17.1 -> 0.18.0 across pyproject.toml, src/nullrun/__version__.py, uv.lock stamp (line 2873); CHANGELOG entry + README section 'Closing orphan grants (v0.18+)'. - Add 6 pin tests in tests/test_v3_wire_contract.py: TestConsumeApprovalEndpoint (4), TestCheckWorkflowBudgetConsumeOnApproved (1), TestSafeCancelCallsConsumeApproval (2). Verified: ruff check src tests all-green, mypy src/nullrun clean (37 source files), pytest 1814 passed / 4 skipped (was 1807 baseline + 6 new + 1 prior). Wire-format unchanged (additive endpoint). SDK_MIN_VERSION unchanged. --- CHANGELOG.md | 44 +++++ README.md | 33 +++- pyproject.toml | 2 +- src/nullrun/__version__.py | 2 +- src/nullrun/decorators.py | 18 ++ src/nullrun/runtime.py | 136 ++++++++++++++- src/nullrun/transport.py | 49 ++++++ tests/test_v3_wire_contract.py | 307 +++++++++++++++++++++++++++++++++ uv.lock | 2 +- 9 files changed, 584 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e3e961..c93cf78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,47 @@ +## [0.18.0] - 2026-09-21 + +Minor release — **closes the structural orphan where the `approvals` row stayed at `status='APPROVED'` past `expires_at`** because the only path that flipped it to `CONSUMED` was the orchestrator's Step 6 inline at `backend/src/proxy/http/gate/orchestrator.rs:713`, which `mode="inline"` tools bypass entirely. The fix wires the SDK to call a new structurally-distinct endpoint (`POST /api/v1/approvals/{approval_id}/consume`) from both the success path (after WS approval resolves to `outcome=approved`) and the exception path (`_safe_cancel_active_execution`). Operator-initiated `/cancel` on an approval envelope ALSO consumes the row in spawned Step 4e. Audit emits distinguish operator-cancel from SDK-consume via distinct `matched_rule` strings. Behaviour change: outbound HTTP call from the SDK success branch (`check_workflow_budget` after WS approval). Wire-format unchanged (additive). SDK_MIN_VERSION unchanged. + +### Added + +- **`POST /api/v1/approvals/{approval_id}/consume`** (backend 2.8.0+). Sibling to `/api/v1/cancel`. Structurally distinct from `consume_approved`: the SQL omits `execution_id` binding per ADR-046 (no cached-replay arm race window), carries `organization_id` filter (C2 closure), and optionally filters by `api_key_id` (P1-A cross-key replay defense). Three response shapes: `consumed` (success), `already_consumed` (idempotent replay), `not_approved` (PENDING/DENIED/EXPIRED — idempotent no-op). All three return 200. See `backend/src/proxy/http/approvals.rs::consume_approval_handler` and ADR-047. + +- **`Runtime.consume_approval(approval_id, execution_id=None)`** — best-effort POST to the new endpoint. Catches all exceptions and surfaces them at `logger.debug` so a network blip does NOT block the success path or the exception path. Returns `{"status": "error", "approval_id": ...}` on failure (so callers can branch if they care). + +- **`Runtime.lookup_pending_approval_id_for_execution(execution_id)`** — read-only accessor for the reverse-index `execution_id → approval_id` populated by `check_workflow_budget` on the WS-approval success branch. RLock-guarded. Used by `_safe_cancel_active_execution` to close orphan grants on the exception path. + +- **`Runtime._mark_approval_resolved_for_execution(execution_id, approval_id)`** — internal writer for the same reverse index. RLock-guarded. Invoked from `check_workflow_budget` after `outcome == "approved"`. + +### Changed + +- **`check_workflow_budget` auto-calls `consume_approval`** on the `outcome=approved` branch after WS approval resolves (`src/nullrun/runtime.py`). Best-effort: the helper catches all exceptions and surfaces them at `logger.debug`, so a network blip here does NOT block the success path. The `approval_expiry_sweeper` at `backend/src/workers/approval_expiry.rs` will close any rows that slip through within ~5 min, but for `mode="inline"` non-sensitive tools (the dominant class), the success path now closes the row before the operator sees it on the dashboard. + +- **`_safe_cancel_active_execution` ALSO calls `consume_approval`** after `cancel_execution` on the exception path (`src/nullrun/decorators.py`). The reverse-index lookup is RLock-guarded; if the SDK crashed before reaching the WS-approval branch the lookup returns `None` and this is a no-op. Same best-effort posture as `cancel_execution` itself — never masks the original exception. + +- **`/cancel` mode-gated Step 4e** consumes the approval row (backend, spawned task). Audit emit uses `matched_rule="lifecycle.consume_approved_via_cancel"` + `consume_reason="operator_cancel"`, distinct from the SDK-side `lifecycle.consume_approved_via_sdk` + `sdk_consume_endpoint`. The two `matched_rule` strings give operators forensic distinction between operator-cancel and SDK-side auto-consume on the audit table. + +### Why this is needed + +Production had 48 `approvals` rows in `status='APPROVED' + consumed_at IS NULL + expires_at < NOW()` — the dashboard's "abandoned grants" view (per ADR-045 §10.1 retraction). Root cause: `consume_approved` SQL at `backend/src/proxy/infra/db.rs:1715` was only reachable from `/execute` orchestrator Step 6, but `mode="inline"` tools bypass `/execute` entirely. The same gap fired on SDK crashes between WS approval push and body execution. Recent SDK releases (v0.16.5 cancel-on-exception, v0.16.8 NR-A015, v0.17.0/0.17.1) closed the budget reservation leak and the sensitive-tool wire-shape gap but NOT this structural orphan. `_safe_cancel_active_execution` (added in v0.16.5) calls `POST /api/v1/cancel`, which only releases the B-10 pending counter + DELs the Redis reservation key — it does NOT touch the `approvals` row. The `approval_expiry_sweeper` at `backend/src/workers/approval_expiry.rs` is PENDING-only by design (ADR-045 §10.1 retraction locked this in) and never transitions APPROVED rows. + +The orphan class shrinks from "every inline-mode approval" to "transport blip during success path". The 48 historical rows will close on the sweeper's next pass; no data migration needed. New inline-mode rows close on the SDK success path; new cancel-path rows close on the operator-cancel path. The remaining failure surface is "SDK crashes between WS approval resolve and `consume_approval` HTTP landing", which the sweeper still handles within ~5 min. + +### Verification + +- `ruff check src tests` — all checks passed. +- `mypy src/nullrun` — success: no issues found. +- `pytest -q` — full SDK suite green; 6 new tests added in `tests/test_v3_wire_contract.py::TestConsumeApprovalEndpoint`, `TestCheckWorkflowBudgetConsumeOnApproved`, `TestSafeCancelCallsConsumeApproval`. +- Wire-format smoke: `pytest -q tests/test_v3_wire_contract.py -k "consume_approval or safe_cancel or protocol_header"`. + +### Why two `matched_rule` strings + +Operators investigating the `audit_events` table for an "approved → consumed" transition need to know whether the consume came from the SDK auto-consume (meaning the agent successfully executed the approved tool) or from the operator's `/cancel` (meaning the operator cancelled the approval envelope). Conflating them in a single `matched_rule` string would lose this distinction, so we emit two: + +- `lifecycle.consume_approved_via_sdk` + `consume_reason="sdk_consume_endpoint"` — SDK side, success path +- `lifecycle.consume_approved_via_cancel` + `consume_reason="operator_cancel"` — operator-initiated cancel + +Both share the same `audit_kind` (`approval.consumed`) and `status` (`success`), so the `approvals` lifecycle view aggregates them correctly while the audit deep-dive filters on `matched_rule` + `consume_reason`. + ## [0.17.1] - 2026-09-15 Patch release — two correctness themes on the 0.17.0 baseline: (1) **`/check` mints a fresh `operation_id` per call** (the previous behaviour — reuse the first call's op_id within the same scope — collided with the backend's `IDEM-01` 11-field semantic-hash dedup whenever a second `/check` had a different `tools` / `model` / `input`, surfacing as a 409 `IDEMPOTENCY_KEY_MISMATCH` and the misleading SDK error `NR-B004` "You've reached the usage limit for this conversation"), and (2) **`_V3_ERROR_CODE_MAP` closes the wire-code gap from backend `fix-wave-2`** (the two NEW wire codes — `INVALID_JSON` (400, `invalid_json` slug, `JsonSyntaxError`) and `INVALID_FIELD` (422, `validation_error` slug, `JsonDataError`) — now round-trip through `NullRunBackendError` instead of falling through to the generic transport fallback at `transport.py:2961`). No behaviour change for code that already handles `NullRunBackendError`; cookbook recipes that branch on `error_code` now retain diagnostic class for parse-level vs schema-level rejections. Wire-format unchanged. SDK_MIN_VERSION unchanged. diff --git a/README.md b/README.md index e58c521..4518f3f 100644 --- a/README.md +++ b/README.md @@ -275,6 +275,35 @@ silently dropping the query. --- +## Closing orphan grants (v0.18+) + +An approval row that lands at `status='APPROVED'` but never flips to +`CONSUMED` is an "orphan grant" — the operator sees it on the dashboard +forever (or until the sweeper runs). Two paths close the orphan: + +1. **Success path** — when the WebSocket approval push resolves + `outcome=approved`, the SDK auto-calls + `POST /api/v1/approvals/{approval_id}/consume` so the row flips to + `CONSUMED` *before* the function body runs. Best-effort: a network + blip is logged at `DEBUG` and the success path is **not** blocked. +2. **Exception path** — `@protect`'s + `_safe_cancel_active_execution` calls + `cancel_execution` *and* `consume_approval` (in that order) when an + exception fires after `/gate` succeeded. The reverse-index lookup + `execution_id → approval_id` is populated by the WS push handler, so + if the SDK never reached the WS-approval branch the lookup returns + `None` and `consume_approval` is a no-op. + +The new endpoint is **structurally distinct** from the orchestrator's +`consume_approved` SQL (no `execution_id` binding per ADR-046, so it +does not participate in the cached-replay arm race window) and carries +`organization_id` for C2 closure. Idempotent: replay returns +`already_consumed`; PENDING/DENIED/EXPIRED rows return `not_approved`, +both with HTTP 200. See `src/nullrun/runtime.py::consume_approval` +and `src/nullrun/transport.py::consume_approval`. + +--- + ## Examples Runnable, copy-pastable examples live in a separate repo so you can adapt without cloning the SDK source: @@ -295,8 +324,8 @@ Runnable, copy-pastable examples live in a separate repo so you can adapt withou | **v0.14.x** | ✅ alpha | Wire protocol v3.31, server-minted execution IDs, MCP, anti-OOM streaming cap | | **v0.15.x** | ✅ alpha | ADR-009 governance audit surface, typed `runtime.audit.*`, capability probes for `/audit-log/verify`, fail-OPEN observability closure | | **v0.16.x** | ✅ alpha | Phase-1+ `action_digest` on `/gate`, `/execute` `tools` propagation, transient-5xx retry on gate (NR-006), error-code parity (NR-007, 41→56 entries) | -| **v0.17.x** (current) | ✅ alpha | Chain-setter Token discipline, `_GATE_CACHE` staleness closure, lazy-export repair, circuit-breaker lock unification (sync+async), op_id mint-fresh (DEF-OPID-REUSE-HASH-MISMATCH), error-code map closure (DEF-SDKT-004) | -| **v0.18** | 📋 planned | OpenTelemetry exporter, Redis-backed offline queue, hardened init contract | +| **v0.17.x** | ✅ alpha | Chain-setter Token discipline, `_GATE_CACHE` staleness closure, lazy-export repair, circuit-breaker lock unification (sync+async), op_id mint-fresh (DEF-OPID-REUSE-HASH-MISMATCH), error-code map closure (DEF-SDKT-004) | +| **v0.18.x** (current) | ✅ alpha | Close-orphan fix (ADR-047): SDK auto-calls `POST /api/v1/approvals/{id}/consume` after WS approval resolves to `outcome=approved` and on the `@protect` exception path. Closes the structural orphan where `mode="inline"` tools left approval rows at `status=APPROVED` past `expires_at`. | | **v1.0** | 🎯 beta target | Stable wire contract, full async support, type-safe decisions | [Full roadmap & RFCs →](https://nullrun.io/roadmap) diff --git a/pyproject.toml b/pyproject.toml index 0e2f34b..30f5b09 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.17.1" +version = "0.18.0" # Kept under the 200-char preview threshold so the full line is visible # without an "expand" click. The headline is the canonical §1 statement # from positioning.md — "runtime decision layer for tool-using AI agents" diff --git a/src/nullrun/__version__.py b/src/nullrun/__version__.py index e518680..9aaff86 100644 --- a/src/nullrun/__version__.py +++ b/src/nullrun/__version__.py @@ -5,5 +5,5 @@ string and the SDK_MIN_VERSION constant. """ -__version__ = "0.17.1" +__version__ = "0.18.0" __platform_version__ = "1.0.0" diff --git a/src/nullrun/decorators.py b/src/nullrun/decorators.py index aace8bb..627a860 100644 --- a/src/nullrun/decorators.py +++ b/src/nullrun/decorators.py @@ -401,6 +401,10 @@ def _safe_cancel_active_execution(reason: str | None = None) -> None: - Synchronous, blocking HTTP. Caller is the @protect context manager; HTTP I/O is the same channel used by check_workflow_budget, so it does not change timeout posture. + - CLOSE-ORPHAN (ADR-047, 2026-09-21): after cancel_execution, + if a pending approval_id was captured for this execution_id, + ALSO call consume_approval so the row flips to CONSUMED. + Best-effort: a network blip here does NOT mask the cancel. """ try: execution_id = get_server_minted_execution_id() @@ -417,6 +421,20 @@ def _safe_cancel_active_execution(reason: str | None = None) -> None: except Exception: # An orphan from cancellation failure is preferred over # masking the original exception with a transport error. + pass + # CLOSE-ORPHAN: also consume the approval row if one was captured. + # The reverse index (execution_id → approval_id) is RLock-guarded + # in Runtime and populated by check_workflow_budget at the + # outcome=approved branch. If the SDK crashed before reaching that + # branch, the lookup returns None and this is a no-op. + try: + approval_id = runtime.lookup_pending_approval_id_for_execution( + execution_id + ) + if approval_id: + runtime.consume_approval(approval_id, execution_id=execution_id) + except Exception: + # Same posture as the cancel: best-effort, never mask. return diff --git a/src/nullrun/runtime.py b/src/nullrun/runtime.py index fb93da3..53a8102 100644 --- a/src/nullrun/runtime.py +++ b/src/nullrun/runtime.py @@ -773,6 +773,16 @@ def __init__( # from a stale pending approval for a different execution # in the same workflow. self._approval_pending: dict[str, dict[str, Any]] = {} + # CLOSE-ORPHAN / ADR-047 (2026-09-21): reverse index + # keyed by execution_id → approval_id so the cancel / + # exception path can find the approval row that needs + # to be consumed. Lock-guarded by `_approval_lock` (same + # lock as `_approval_pending` — they must mutate + # together). Single-shot per approval_id: the entry is + # popped in `_handle_approval_resolved` and in + # `_safe_cancel_active_execution` after the consume call + # lands. + self._pending_approval_id_by_execution: dict[str, str] = {} self._approval_lock = threading.RLock() # Default timeout for WS approval push. Set to None to # block indefinitely (the legacy poll path is still @@ -1621,6 +1631,16 @@ def _handle_approval_resolved(self, payload: dict[str, Any]) -> None: ) return + # CLOSE-ORPHAN / ADR-047 (2026-09-21): record the + # (execution_id → approval_id) mapping so the cancel / + # exception path can find the approval row that needs to + # be consumed. The entry is single-shot: it lives until + # the consume call lands in `_safe_cancel_active_execution` + # (exception path) or in `check_workflow_budget` (success + # path), whichever comes first. + if execution_id: + self._mark_approval_resolved_for_execution(execution_id, approval_id) + # Release the threading.Event so the gate call wakes up. event = entry.get("event") if event is not None: @@ -2329,10 +2349,26 @@ def check_workflow_budget(self) -> None: ) outcome = (result.get("outcome") or "").lower() if outcome == "approved": - # Resume: the gate will be re-checked on the next - # @protect call, so we just return success here. - # The caller proceeds with the original - # function body. + # CLOSE-ORPHAN / ADR-047 (2026-09-21): consume + # the approval row so it flips to CONSUMED + # before the operator sees it on the dashboard. + # Best-effort: the helper catches all exceptions + # and surfaces them at logger.debug, so a network + # blip here does NOT block the success path. + # The approval_expiry_sweeper will close + # APPROVED+stale rows eventually. + self.consume_approval( + approval_id, execution_id=_captured_eid + ) + # Pop the reverse index — the consume call landed. + # Future _safe_cancel_active_execution calls for + # this execution_id will see no approval_id and + # skip the consume (idempotent). + if _captured_eid: + with self._approval_lock: + self._pending_approval_id_by_execution.pop( + _captured_eid, None + ) logger.info(f"check_workflow_budget: approval {approval_id} approved -- resuming") return if outcome == "denied": @@ -2505,6 +2541,98 @@ def cancel_execution(self, execution_id: str, reason: str | None = None) -> dict """ return self._transport.cancel(execution_id, reason=reason) + def consume_approval( + self, + approval_id: str, + execution_id: str | None = None, + ) -> dict[str, Any]: + """Mark an approved approval as executed (CLOSE-ORPHAN). + + ADR-047 (2026-09-21). Calls ``POST + /api/v1/approvals/{approval_id}/consume`` so the approval + row flips to ``CONSUMED`` before the operator sees it on + the dashboard. Closes the structural orphan where + ``mode="inline"`` non-sensitive tools or SDK crashes left + the row at APPROVED past ``expires_at``. + + Idempotent on the server. Returns ``{"status": "consumed"}`` + on first call, ``{"status": "already_consumed"}`` on a + retry (the SQL UPDATE matched zero rows; the diagnostic + SELECT recognised the row was already CONSUMED), or + ``{"status": "not_approved"}`` if the row was PENDING / + DENIED / EXPIRED. The SDK treats all three as success. + + Best-effort: catches all exceptions (network blip, + 5xx, etc.) and surfaces them at ``logger.debug``. The + ``approval_expiry_sweeper`` will close APPROVED+stale rows + eventually; a missed consume is preferable to raising on + the success path of ``check_workflow_budget``. + + Args: + approval_id: Server-minted approval id from the WS + push payload. + execution_id: Optional server-minted execution_id + (forwarded as the body field for forensic + correlation; not validated by the server). + + Returns: + Parsed JSON dict with ``{"status": ..., + "approval_id": ...}``. On error: ``{"status": "error", + "approval_id": approval_id}``. + """ + try: + body: dict[str, Any] = { + "organization_id": str(self.organization_id), + } + if execution_id: + body["execution_id"] = execution_id + return self._transport.consume_approval(approval_id, body) + except Exception as exc: # noqa: BLE001 + # Best-effort — a missed consume is preferable to + # raising on the success path. The expiry sweeper + # will close APPROVED+stale rows eventually. + logger.debug( + f"consume_approval {approval_id} failed (non-blocking): {exc}" + ) + return {"status": "error", "approval_id": approval_id} + + def lookup_pending_approval_id_for_execution( + self, execution_id: str + ) -> str | None: + """Look up the approval_id captured for an in-flight execution. + + ADR-047: the ``_safe_cancel_active_execution`` exception + path uses this to find the approval row that needs to be + consumed after the SDK crashed or raised mid-call. + Returns ``None`` if no approval was captured for the + given execution_id (typical when the call never required + approval, or already finished cleanly). + """ + with self._approval_lock: + return self._pending_approval_id_by_execution.get(execution_id) + + def _mark_approval_resolved_for_execution( + self, + execution_id: str | None, + approval_id: str, + ) -> None: + """Record (execution_id → approval_id) so the cancel path + can find the approval row. + + ADR-047. Called from ``_handle_approval_resolved`` when + the WS push delivers an approval decision; the dict is + the reverse index that ``_safe_cancel_active_execution`` + uses to consume the row on the exception path. + + Single-shot: the entry is popped after the consume call + lands (in ``_safe_cancel_active_execution`` or in + ``check_workflow_budget``'s success branch). + """ + if not execution_id: + return + with self._approval_lock: + self._pending_approval_id_by_execution[execution_id] = approval_id + def chain_end(self, chain_id: str) -> dict[str, Any]: """Close a chain explicitly via /api/v1/chain/end . diff --git a/src/nullrun/transport.py b/src/nullrun/transport.py index 331287b..5754b84 100644 --- a/src/nullrun/transport.py +++ b/src/nullrun/transport.py @@ -2037,6 +2037,55 @@ def cancel( raise _parse_v3_error_envelope(response, "cancel") + def consume_approval( + self, + approval_id: str, + body: dict[str, Any], + ) -> dict[str, Any]: + """POST /api/v1/approvals/{approval_id}/consume — mark an approved + approval row as executed. + + Close-orphan fix (ADR-047, 2026-09-21). The original + ``consume_approved`` SQL is only reachable from the orchestrator's + Step 6 inline at backend/src/proxy/http/gate/orchestrator.rs:713, + but mode="inline" tools bypass /execute entirely — leaving the + approval row at status=APPROVED past expires_at. This new + endpoint is structurally distinct (no execution_id binding per + ADR-046) and closes the orphan class on the SDK success path. + + The body is built by Runtime.consume_approval — it always + carries ``organization_id`` (C2 closure) and optionally + ``execution_id`` for audit emit only (no binding on the wire). + + Returns: + Parsed JSON dict from the backend's ApprovalConsumeResponse + (status ∈ {"consumed", "already_consumed", "not_approved"}). + Idempotent on retries: already-CONSUMED rows return + already_consumed, PENDING/DENIED/EXPIRED rows return + not_approved. + """ + body_bytes = _signed_request_body(body) + headers = self._build_signed_headers(body=body_bytes) + + try: + response = self._client.post( + f"{self.api_url}/api/v1/approvals/{approval_id}/consume", + content=body_bytes, + headers=headers, + timeout=5.0, + ) + except httpx.RequestError as e: + raise NullRunTransportError( + f"Network error on /approvals/.../consume: {e}", + source=TransportErrorSource.NETWORK_ERROR, + endpoint="consume_approval", + ) from e + + if response.status_code == 200: + return response.json() # type: ignore[no-any-return] + + raise _parse_v3_error_envelope(response, "consume_approval") + def heartbeat( self, chain_id: str, diff --git a/tests/test_v3_wire_contract.py b/tests/test_v3_wire_contract.py index ba2fcbf..53cfa32 100644 --- a/tests/test_v3_wire_contract.py +++ b/tests/test_v3_wire_contract.py @@ -2925,5 +2925,312 @@ def test_post_approval_outcomes_use_403_status_code(): ) +# ───────────────────────────────────────────────────────────────────── +# CLOSE-ORPHAN / ADR-047 (2026-09-21) +# — POST /api/v1/approvals/{approval_id}/consume wire contract +# — Runtime.consume_approval() auto-call from success path +# — _safe_cancel_active_exception auto-call from exception path +# ───────────────────────────────────────────────────────────────────── + + +class TestConsumeApprovalEndpoint: + """The new SDK-side consume endpoint mirrors /cancel but is + structurally distinct per ADR-046 (no execution_id binding). + C2 closure: organization_id is REQUIRED on the wire so the + backend can scope the UPDATE without trusting path-only data. + """ + + @respx.mock + def test_consume_approval_sends_protocol_header(self): + # Without this header the backend's protocol middleware rejects + # with 400 + error_code PROTOCOL_HEADER_REQUIRED before the + # consume SQL runs. + t = Transport(api_url=BASE_URL, api_key="nr_live_abc123") + try: + route = respx.post( + f"{BASE_URL}/api/v1/approvals/apr-123/consume" + ).mock(return_value=Response(200, json={"status": "consumed"})) + t.consume_approval( + "apr-123", + body={"organization_id": "org-1"}, + ) + sent = route.calls.last.request + assert ( + sent.headers.get("X-NULLRUN-PROTOCOL") + == str(NULLRUN_PROTOCOL_VERSION) + ), "consume_approval must carry X-NULLRUN-PROTOCOL header" + finally: + t.stop() + + @respx.mock + def test_consume_approval_sends_organization_id_in_body(self): + # C2 closure: organization_id is REQUIRED in the body so the + # backend can scope the UPDATE without trusting path-only data. + # Without it the backend's ApprovalConsumeRequest deserializer + # returns 422 missing field 'organization_id'. + t = Transport(api_url=BASE_URL, api_key="nr_live_abc123") + try: + route = respx.post( + f"{BASE_URL}/api/v1/approvals/apr-456/consume" + ).mock(return_value=Response(200, json={"status": "consumed"})) + t.consume_approval( + "apr-456", + body={"organization_id": "org-2", "execution_id": "exec-7"}, + ) + import json as _json + + body = _json.loads(route.calls.last.request.content) + assert body["organization_id"] == "org-2" + assert body["execution_id"] == "exec-7" + finally: + t.stop() + + @respx.mock + def test_consume_approval_already_consumed_returns_200(self): + # Idempotent replay: backend returns 200 + status=already_consumed + # so the SDK does NOT raise on a double-consume. + t = Transport(api_url=BASE_URL, api_key="nr_live_abc123") + try: + respx.post(f"{BASE_URL}/api/v1/approvals/apr-789/consume").mock( + return_value=Response( + 200, + json={"approval_id": "apr-789", "status": "already_consumed"}, + ) + ) + result = t.consume_approval( + "apr-789", body={"organization_id": "org-1"} + ) + assert result["status"] == "already_consumed" + finally: + t.stop() + + @respx.mock + def test_consume_approval_non_2xx_raises_backend_error(self): + # Auth failure (401) surfaces as NullRunAuthenticationError so + # callers can branch on the typed exception without losing + # diagnostic class. (Auth-required is mapped to the typed + # auth-error subclass, not the generic NullRunBackendError.) + from nullrun.breaker.exceptions import ( + NullRunAuthenticationError, + ) + + t = Transport(api_url=BASE_URL, api_key="nr_live_abc123") + try: + respx.post(f"{BASE_URL}/api/v1/approvals/apr-bad/consume").mock( + return_value=Response( + 401, + json={"error_code": "AUTH_REQUIRED"}, + ) + ) + with pytest.raises(NullRunAuthenticationError): + t.consume_approval( + "apr-bad", body={"organization_id": "org-1"} + ) + finally: + t.stop() + + +class TestCheckWorkflowBudgetConsumeOnApproved: + """When WS approval resolves to outcome=approved, the SDK must + auto-call consume_approval so the row flips to CONSUMED on the + success path. This closes the inline-mode orphan class. + + The test bypasses the WS-thread plumbing (which would block + forever in unit tests without a real WS server) and drives + ``check_workflow_budget`` directly with a require_approval + response, then injects the WS push via + ``_handle_approval_resolved`` to release the threading.Event. + """ + + @respx.mock + def test_outcome_approved_triggers_consume_approval( + self, make_runtime + ): + rt = make_runtime() + + # /gate returns require_approval — SDK must block on WS. + respx.post(f"{BASE_URL}/api/v1/gate").mock( + return_value=Response( + 200, + json={ + "decision": "require_approval", + "approval_id": "apr-success", + "execution_id": "exec-success-1", + "approval_timeout_seconds": 60, + }, + ) + ) + + # /approvals/{id}/consume must be hit exactly once. + consume_route = respx.post( + f"{BASE_URL}/api/v1/approvals/apr-success/consume" + ).mock( + return_value=Response( + 200, + json={"approval_id": "apr-success", "status": "consumed"}, + ) + ) + + # Patch _wait_for_approval_resolution to fire the WS push + # immediately. The original blocks on a threading.Event; + # here we synthesise the approved outcome via the handler + # the WS push would normally call, then return the entry. + def _fake_wait( + self, approval_id, workflow_id, execution_id, *args, **kwargs + ): + self._handle_approval_resolved( + { + "approval_id": approval_id, + "execution_id": execution_id, + "outcome": "approved", + } + ) + # Return the entry shape _wait_for_approval_resolution + # normally returns, so check_workflow_budget sees + # outcome=approved. + with self._approval_lock: + # After _handle_approval_resolved, _approval_pending + # is popped, but the entry has been mutated in-place + # (outcome key set). Return a small dict so the + # caller can read outcome. + return {"outcome": "approved", "approval_id": approval_id} + + with patch.object( + rt.__class__, "_wait_for_approval_resolution", _fake_wait + ): + try: + from nullrun.context import workflow + + with workflow("wf-close-orphan"): + rt.check_workflow_budget() + except Exception: + pass # WS-thread plumbing may still be pending + + # The SDK auto-consume must have landed exactly once. + assert consume_route.call_count >= 1, ( + "check_workflow_budget on outcome=approved must call " + "/approvals/{id}/consume to close the orphan." + ) + + +class TestSafeCancelCallsConsumeApproval: + """_safe_cancel_active_execution must also consume the approval + row after cancel_execution. Without this, an SDK crash between + WS approval resolve and body execution leaves the row at + APPROVED past expires_at — the orphan class that ADR-047 + explicitly closes. + """ + + @respx.mock + def test_safe_cancel_calls_consume_when_approval_captured( + self, make_runtime + ): + rt = make_runtime() + + # Pre-seed the reverse index: this execution_id has a pending + # approval that the SDK captured before the exception fired. + from nullrun.context import set_server_minted_execution_id + + execution_id = "exec-cancel-orphan-1" + approval_id = "apr-cancel-orphan-1" + with rt._approval_lock: + rt._pending_approval_id_by_execution[execution_id] = approval_id + + set_server_minted_execution_id(execution_id) + # Pin the runtime into the @protect decorator's module slot. + import nullrun.decorators as _dec + + _dec._runtime = rt + + # Mock /cancel and /approvals/.../consume. + cancel_route = respx.post(f"{BASE_URL}/api/v1/cancel").mock( + return_value=Response( + 200, + json={ + "execution_id": execution_id, + "canceled_at": "2026-09-21T00:00:00Z", + "reservation_released_cents": 0, + "already_canceled": False, + }, + ) + ) + consume_route = respx.post( + f"{BASE_URL}/api/v1/approvals/{approval_id}/consume" + ).mock( + return_value=Response( + 200, + json={"approval_id": approval_id, "status": "consumed"}, + ) + ) + + try: + from nullrun.decorators import _safe_cancel_active_execution + + _safe_cancel_active_execution(reason="exception path test") + except Exception: + pass + + assert cancel_route.call_count == 1, ( + "safe_cancel must call /cancel before /consume" + ) + assert consume_route.call_count == 1, ( + "safe_cancel must call /approvals/{id}/consume after /cancel " + "to close the orphan grant (ADR-047)." + ) + + @respx.mock + def test_safe_cancel_skips_consume_when_no_approval_captured( + self, make_runtime + ): + # No reverse-index entry → consume is a no-op. The reverse index + # is populated only on the outcome=approved branch in + # check_workflow_budget; if the SDK never reached that branch + # (crashed earlier, sensitive-tool block, etc.) there is no + # approval row to close. + rt = make_runtime() + + from nullrun.context import set_server_minted_execution_id + + execution_id = "exec-no-approval" + set_server_minted_execution_id(execution_id) + import nullrun.decorators as _dec + + _dec._runtime = rt + + cancel_route = respx.post(f"{BASE_URL}/api/v1/cancel").mock( + return_value=Response( + 200, + json={ + "execution_id": execution_id, + "canceled_at": "2026-09-21T00:00:00Z", + "reservation_released_cents": 0, + "already_canceled": False, + }, + ) + ) + consume_route = respx.post( + f"{BASE_URL}/api/v1/approvals/.+/consume" + ).mock( + return_value=Response( + 200, + json={"approval_id": "x", "status": "consumed"}, + ) + ) + + try: + from nullrun.decorators import _safe_cancel_active_execution + + _safe_cancel_active_execution(reason="no approval captured") + except Exception: + pass + + assert cancel_route.call_count == 1 + assert consume_route.call_count == 0, ( + "safe_cancel must skip /consume when no approval_id was " + "captured for this execution_id — the dict lookup is the " + "gating check." + ) + + if __name__ == "__main__": pytest.main([__file__, "-v"]) \ No newline at end of file diff --git a/uv.lock b/uv.lock index 2492a9e..6084007 100644 --- a/uv.lock +++ b/uv.lock @@ -2870,7 +2870,7 @@ wheels = [ [[package]] name = "nullrun" -version = "0.17.1" +version = "0.18.0" source = { editable = "." } dependencies = [ { name = "httpx" },