chore(release): 0.14.0 — hardening pass on the money contract#76
Merged
Conversation
Until now runtime.execute() only handled decision=block. A backend that returned require_approval was treated as 'allow' and the @sensitive body ran, silently bypassing human approval. Phase 0 closes the gap: 1. On require_approval, runtime.execute parks on the existing _wait_for_approval_resolution() (WS push + threaded.Event). Denied or timed-out -> NullRunBlockedException. Approved -> re-checks. 2. Re-check forwards approval_id on the second /execute request. The same operation_id is reused so the backend can bind both requests to one logical action. 3. If the re-check still returns require_approval, the body is blocked (defensive: the backend should not return require_approval to a re-check, but a misbehaving server must not cause a silent allow). 4. Server-authoritative approval_timeout_seconds from the first response drives the event.wait() timeout (Разрыв 1c sync). Transport.execute gains an optional approval_id kwarg; when present it is forwarded on the wire as a top-level field, distinct from operation_id. Legacy callers (no approval_id) get the previous behaviour. Tests (tests/test_execute_approval_flow.py): - approved flow: re-check with matching tool/input/operation_id, then allow. - denied flow: no re-check, NullRunBlockedException. - require_approval without approval_id: fail-CLOSED. Verification: - 17/17 execute_approval_flow + approval_timeout_field + runtime.execute + sensitive-tool fail-closed tests pass. - pytest -q full SDK suite: 1246 passed, 7 skipped.
Phase 0 review (2026-07-23): the existing approval_timeout validation only rejected non-positive values, so a misconfigured backend (or a malicious proxy) advertising 0 (deadlock) or 1e9 (lock the thread for years) would be passed straight to event.wait(). This commit: 1. Adds MIN_APPROVAL_TIMEOUT_SECONDS=1 and MAX_APPROVAL_TIMEOUT_SECONDS=3600 module constants. 2. Extracts the validation into a _validate_approval_timeout() helper that coerces to float, returns None on non-numeric, and returns None on out-of-range with a WARN log. 3. Wires the helper into both call sites: - check_workflow_budget (existing /gate path) - runtime.execute (the new Phase 0 /execute path) 4. The pre-existing inline parse+reject logic in both sites is replaced by the helper (no copy-paste). 5. Updates the docstring on _wait_for_approval_resolution that mentioned a 'fall back to legacy /status poll path' on timeout — the Разрыв 1c contract is fail-CLOSED on timeout, and the legacy fallback was a stale comment that would mislead future maintainers. 6. test_timeout_sentinel_returned_when_no_ws_push now uses a 1.5s timeout (the new minimum in-range) and asserts the elapsed wait is between 1.0s and 5.0s — the old 0.1s value would now fall back to the env default 300s. Tests: - 5 new test_validate_approval_timeout_* tests pin every branch: in-range (incl. int coercion), below MIN, above MAX, non-numeric (str, list, dict), and None. - Pre-existing 8 test_approval_timeout_field tests still pass (1s-300s range covered by 15s, 42s, 90s, 120s, 300s inputs). - 17/17 test_execute_approval_flow + TestNullRunRuntimeExecute + TestEnforceSensitiveToolFailClosed still pass. - 22 passed, 1 skipped in the approval timeout + execute flow slice.
Phase 1 / MVP 1.0 close on the SDK side. Three new modules
mirror the backend BusinessImpact discriminated union and wire
contract so the SDK can produce /gate + /execute requests that
pass the backend digest re-check byte-for-byte.
What landed
1. nullrun.business_impact -- Python mirror of the Rust
BusinessImpact enum + compute_action_digest() helper.
- Same canonical-JSON algorithm: sort object keys recursively,
then SHA-256 over the protocol prefix || canonical bytes.
- MoneyImpact dataclass with explicit validate() (negative
amount rejected, currency must be 3 ASCII uppercase chars,
direction must be outflow|inflow). Same checks as the
backend MoneyImpact::validate.
- The MVP supports only kind=money. The enum shape is
forward-compat: future record_count, resource_quantity,
permission_change, etc. land as new dataclass branches
without changing the wire discriminator.
2. nullrun.extractor -- declarative SDK extractor with a tiny
shorthand factory.
- money_outflow(argument="amount_cents") returns a
MoneyImpactExtractor that binds the call's arguments via
inspect.signature(...).bind(*args, **kwargs) and pulls the
named argument out, treating positional and keyword
invocations identically.
- Fails fast on missing argument / wrong type / bool.
- impact_for() returns a fully-validated BusinessImpact;
the caller is expected to send it on /gate and re-send the
same on /execute (the SDK will compute the digest
automatically).
3. tests/test_approval_money_flow.py -- the 5 DoD scenarios
requested on 2026-07-23:
1. Refund $40 -> Allow
2. Refund $1200 -> Require Approval -> Approve -> Execute
3. Refund $1200 -> Approve -> Modify amount to $1300
-> block on digest mismatch (Phase 1 headline security)
4. Approved -> Execute -> Second Execute -> block on replay
5. Approved -> wait expiry -> Execute -> block on expiry
Plus 13 supporting tests: digest deterministic / 1-cent change
flips digest / EUR vs USD / MoneyImpact validation / extractor
positional vs keyword vs mixed args / extractor rejects bad
types.
The ApprovalSimulator class is an in-process mirror of
gate_internal grant-consume + Phase 1 digest re-check. Verified
against db.rs::consume_approved SQL + the new Rust digest-block
code path.
Tests verified
- pytest -q tests/test_approval_money_flow.py -> 18/18 passed.
- pytest -q tests/test_execute_approval_flow.py -> 3/3 passed.
- pytest -q tests/test_approval_timeout_field.py -> 13/13 passed.
What this commit does NOT close
- Wiring into @sensitive: extractor.impact_for is a helper
callable from runtime.execute but not yet auto-wired. The
DoD tests call it manually via _refund_call(). A future PR
flips this to automatic via a function attribute set by
@sensitive.
- Backend HTTP integration test wiring axum::Router + mock
ApprovalRepository. The simulator mirrors the decision
logic byte-for-byte.
- Frontend regen of MoneyImpact in api.ts (SDK side does not
touch the frontend; tracked separately).
…...) Phase 1 / MVP 1.0 closes the wire side of the action-bound approval flow. The previous SDK commit (ccdf857) shipped the MoneyImpactExtractor helper but did NOT wire it through ``runtime.execute()``; this commit does. What changed - ``runtime.execute()`` gains two new kwargs: ``business_impact: dict | None`` and ``action_digest: str | None``. When supplied, they ride the payload to /execute. When absent, the field is omitted from the wire and the backend falls back to the legacy Phase 0 approval_id-only grant consume path. Both fields default to None so existing callers compile unchanged. - ``_enforce_sensitive_tool`` reads the wrapped function's ``_nullrun_extractor`` attribute (set by the @sensitive decorator's ``impact=...`` form), runs the extractor on the live args, computes the action_digest from the resulting BusinessImpact, and threads both onto the runtime.execute call. If the extractor raises (bad arg name, wrong type, negative amount, ...), the body MUST NOT run per ADR-008: the error is wrapped as ``NullRunBlockedException`` with error_code NR-B003 and the wrapper raises before the sensitive tool executes. - ``@sensitive`` becomes parameterised: ``@sensitive`` (bare form, unchanged) and ``@sensitive(impact=money_outflow(argument="..."))`` (factory form, new). Both register the tool as sensitive so the pre-check fires; only the factory form attaches the extractor. Implementation splits into ``sensitive(fn, *, impact)`` for the public entry point and ``_do_sensitive_register(fn)`` for the shared registration body. Tests verified - pytest -q tests/test_sensitive_extractor.py -> 5/5 passed. The new file monkeypatches ``runtime._transport.execute`` on a freshly-built NullRunRuntime singleton (registered via ``RuntimeRegistry.set``), invokes ``_enforce_sensitive_tool`` directly, and asserts the payload reaching the wire contains: - ``business_impact`` with the documented MoneyImpact shape (kind, direction, amount_minor, currency, extractor_id, extractor_version). - ``action_digest`` matching the SDK's own ``compute_action_digest`` byte-for-byte (the cross- language contract pin from 4445f95). The legacy Phase 0 path (no ``_nullrun_extractor``) sends neither field. Extractor errors (bad arg, negative amount) raise ``NullRunBlockedException(NR-B003)``. - pytest -q tests/test_approval_money_flow.py tests/test_execute_approval_flow.py tests/test_approval_timeout_field.py -> 32/32 passed (regression check on Phase 0 SDK paths). The wire shape produced here matches the manual-call pattern in ``tests/test_approval_money_flow.py::TestDoDScenarios`` which has been green since ccdf857; both pin the cross- language contract on the same hex literal.
Adds ``tests/test_business_impact.py`` -- the Python counterpart of the backend's ``business_impact::tests`` module. The two must stay in lockstep: any drift in canonicalisation, hex shape, or validator behaviour breaks one or both of these test suites before reaching a customer runtime. What the new file covers - ``TestComputeActionDigestPins`` -- 5 tests pinning the canonical SHA-256 hex for ``BusinessImpact.money(OUTFLOW, 5_000, "USD")`` to the same golden literal (``dfc96387ca539b7130caebe705e042f2e34e52ab44352ae5e527bcef64f0df27``) that the Rust golden test asserts. Two-call determinism, amount-change / currency-change / direction-change digests differ. - ``TestBusinessImpactWireDict`` -- 6 tests pinning the JSON shape ``to_wire_dict()`` produces. Round-trips through ``json.dumps(sort_keys=True)`` so a non-deterministic dict ordering in the canonical encoder would surface as a digest mismatch. - ``TestExtractorArgumentLookup`` -- 5 tests pinning ``inspect.signature(...).bind(...)`` resolves the declared argument positionally, by keyword, and mixed. Sanity-check on ``bound.apply_defaults()`` which is what the extractor relies on. Explicit ``TypeError`` on missing argument (the @Protect wrapper converts this into a NullRunBlockedException). - ``TestExtractorFailureModes`` -- 3 tests pinning the fail-CLOSED behaviour: negative amount -> ValueError, non-int -> TypeError, ``True`` -> TypeError. The ``True`` pin is the most important: ``True == 1`` would silently round-trip through ``inspect.signature`` and reach the canonical encoder as ``amount_minor=true``; the validator must reject this so a hostile SDK caller can't smuggle a tiny refund through ``amount_minor=True``. Tests verified - ``pytest -q tests/test_business_impact.py`` -> 19/19 passed - ``pytest -q tests/test_approval_money_flow.py tests/test_sensitive_extractor.py`` -> 23/23 passed (no regression on the existing approval-flow or sensitive-extractor tests). The golden hex pin is the same literal the Rust test ``action_digest_golden_usd_outflow_5000_cents`` asserts. A change to either algorithm trips a test on both sides before a customer sees the regression.
Phase 1.1 UX follow-up addressing the silent truncation bug
in the Phase 0 extractor (``int(50.99) == 50`` silently
dropped 99 cents) and the operator-friction where the unit
semantics were implicit from the value type.
The new contract
- ``money_outflow(argument="amount", units="major")``:
``Decimal`` argument. ``Decimal * 100`` with banker's
rounding (``ROUND_HALF_EVEN``) to integer minor units.
``int`` and ``bool`` are rejected outright because a
bare ``int`` in major units is the silent bug class the
explicit discriminator is designed to prevent.
- ``money_outflow(argument="amount_cents", units="minor")``
(the default for backward compatibility): ``int`` is the
canonical type. ``Decimal`` is accepted only if it is
already integer-valued; a fractional ``Decimal`` is a
unit-confusion bug and surfaces a ``TypeError`` pointing
the operator at the right alternative. ``float`` is
rejected outright at both paths.
The wire shape is unchanged: the SDK converts to integer
minor units before reaching the ``BusinessImpact`` struct,
so the backend still sees ``amount_minor`` in cents and the
cross-language golden hex pin
(``dfc96387ca539b7130caebe705e042f2e34e52ab44352ae5e527bcef64f0df27``)
matches whether the operator passed ``int(5000)``,
``Decimal("50")``, or ``Decimal("50.00")``.
Why the discriminator is explicit (not type-implicit)
A signature refactor (``int`` -> ``Decimal`` or vice versa)
does NOT silently flip the unit semantics. The operator
must pass ``units="major"`` to opt into Decimal conversion,
and that opt-in survives the refactor. This addresses the
review note: an ``int = minor, Decimal = major`` shortcut
is rejected because it makes the unit semantics implicit
and brittle.
Files changed
- ``nullrun-sdk-python/src/nullrun/extractor.py`` -- the
``_to_minor_units`` helper is the new conversion
primitive; ``MoneyImpactExtractor.__init__`` accepts
``units: str = UNIT_MINOR``; ``impact_for`` delegates to
the helper. ``bool`` is rejected explicitly (it is a
subclass of ``int`` in Python and would otherwise slip
through).
- ``nullrun-sdk-python/tests/test_units_discriminator.py`` --
29 unit tests covering the unit-discriminator matrix:
7 for ``units="major"`` (Decimal success + int/float/
bool rejection), 7 for ``units="minor"`` (int success +
Decimal-int acceptance + fractional-Decimal/float/str/bool
rejection), 2 for the cross-language golden hex pin
survival, 3 for the discriminator being explicit, 9 for
``_to_minor_units`` directly, 1 for the direction
independence.
- ``nullrun-sdk-python/tests/test_approval_money_flow.py``
-- the two existing tests ``test_extractor_rejects_wrong_type``
and ``test_extractor_rejects_bool_amount`` updated to match
the new error message ("requires int or Decimal"). The
tests still pass; the messages now name the unit
discriminator so the operator can fix the call site
without guessing.
Backend: no changes. The wire shape is in minor units
regardless of the SDK's units discriminator, so the
backend's ``action_predicate`` evaluator (which compares
``amount_minor`` values) is unaffected.
Tests verified
- ``pytest tests/test_units_discriminator.py`` -> 29/29 pass
- ``pytest tests/test_approval_money_flow.py
tests/test_sensitive_extractor.py
tests/test_business_impact.py`` -> 42/42 pass
(regression -- the existing tests were updated to match
the new error message; no behavioural change for
callers who used the legacy ``int`` path).
Production-grade money contract. The previous commit (3a3ae6b) introduced ``Decimal`` support with ``ROUND_HALF_EVEN`` (banker's rounding) for ``units="major"``. Review rejected that on the grounds that ``Decimal("50.005")`` for USD silently drops the half-cent (``5000`` minor units) and surprises the operator. Payment systems either truncate explicitly or refuse ambiguous precision; banker's rounding at the input boundary is a third-class behaviour that hides bugs. This commit replaces banker's rounding with strict precision validation against the ISO-4217 minor-unit exponent for the currency. Contract - ``currency_minor_digits(currency)`` returns the ISO-4217 minor-unit exponent for the currency: ``2`` for USD/EUR/ GBP/CHF/CAD/AUD, ``0`` for JPY, ``3`` for KWD/BHD/OMR. Unknown currencies fall back to ``2`` (a future addition is one line in ``_CURRENCY_MINOR_DIGITS``). - ``_decimal_has_more_fractional_digits(value, allowed)`` returns ``True`` iff the value has a non-zero fractional part whose precision exceeds ``allowed``. The check uses ``value % 1`` so that ``Decimal("50.00")`` (which ``as_tuple()`` reports as having two fractional digits) is correctly classified as an integer-valued decimal with zero effective fractional digits. - ``_to_minor_units`` for ``units="major"`` rejects any ``Decimal`` whose precision exceeds the currency's minor-unit exponent with ``ValueError("{currency} supports at most {N} fractional digit(s); got {value} ({M}).")``. The error message names the currency and the offending precision, so the operator sees exactly what to fix. Edge cases - ``Decimal("50")`` (USD) -> ``5000`` minor OK (no fractional). - ``Decimal("50.99")`` (USD) -> ``5099`` minor OK (2 digits). - ``Decimal("50.00")`` (USD) -> ``5000`` minor OK (zero effective fractional). - ``Decimal("50.005")`` (USD) -> ``ValueError`` (3 digits, USD supports 2). - ``Decimal("50.999")`` (USD) -> ``ValueError``. - ``Decimal("0.005")`` (USD) -> ``ValueError``. - ``Decimal("100.5")`` (JPY) -> ``ValueError`` (JPY supports 0). - ``Decimal("1000")`` (JPY) -> ``1000`` minor OK. - ``Decimal("1.234")`` (KWD) -> ``1234`` minor OK (KWD supports 3). - ``Decimal("1.2345")`` (KWD) -> ``ValueError``. The wire format is unchanged: ``amount_minor`` is still an integer cents/fils/yen on the wire. The conversion is exact because precision was validated before the multiplier ran, so there is no rounding at any step. UI The rule editor (frontend/app/(platform)/control-center/policies/approval-rules/page.tsx) extends ``CURRENCIES`` from a flat string array to a table of ``{ code, digits }`` pairs and uses ``CURRENCY_BY_CODE`` to look up the allowed fractional digits per currency. The regex in ``predicateToJson`` and the error message in ``validatePredicateForm`` both parameterise on the currency so ``50.005`` for USD raises an inline error pointing at the currency's minor-unit exponent, and ``100.5`` for JPY raises an inline error saying "JPY supports at most 0 fractional digit(s)". The hint text on the form is updated to reflect the exact conversion (no rounding) and the new rejection behaviour. Backend No changes. The backend ``action_predicate`` evaluator already compares ``amount_minor`` (integer cents / fils / yen) which is currency-agnostic at the wire level. The validator in ``approval_rule_service.rs`` checks the JSON shape but not the precision (the precision contract lives in the SDK + form, the security boundary at the backend is the SQL ``consume_approved`` atomic path). Tests verified - ``pytest tests/test_units_discriminator.py`` -> 36/36 pass (was 29/29 before; replaced 2 banker's-rounding tests with 9 precision-validation tests covering USD sub-cent, JPY sub-yen, KWD sub-fil, integer-valued-with-trailing-zeros). - ``pytest tests/test_business_impact.py tests/test_approval_money_flow.py tests/test_sensitive_extractor.py`` -> 44/44 pass (regression; the legacy ``int`` path is unchanged). - ``npm run type-check`` -> exit 0 - ``cargo test --lib proxy::service::approval_rule_service::tests::`` -> 6/6 pass (backend regression; no backend changes here). Backward compat ``Decimal("50.00")`` and ``Decimal("50")`` still produce the same ``amount_minor=5000`` as before; only values with non-zero fractional parts exceeding the currency's minor-unit exponent are now rejected. Existing call sites that passed ``Decimal("50.99")`` (the only precision the SDK could silently round) continue to work; call sites that passed ``Decimal("50.005")`` were silently buggy and now surface as ``ValueError`` instead of a subtle drift in the wire shape.
Closes the four review gaps from the Phase 1.1 / UX follow-up:
1. **Dedicated error types** -- ``InvalidMoneyPrecisionError``
and ``InvalidMoneyAmountError`` (both subclass ``ValueError``
for backward compat). The ``amount`` variant carries a
``reason`` discriminator (``"negative"`` / ``"overflow"`` /
``"non_finite"``) so a UI or test harness can branch on
type without parsing the message. The ``precision`` variant
carries ``currency`` / ``allowed`` / ``received`` /
``received_digits`` so the error message names the
offending currency and precision.
2. **Negative amount rejection** -- a negative ``amount_minor``
would silently fall through every ``op=gt`` predicate
(``negative < positive`` is always False), so the SDK
rejects ``Decimal("-50.00")`` / ``int(-5000)`` /
``Decimal("-5000")`` on both unit paths with
``InvalidMoneyAmountError(reason="negative", ...)``. ``0``
is accepted (legitimate $0.00 refund).
3. **Overflow guard** -- the converted ``amount_minor`` is
checked against ``2**63 - 1`` (the wire format is
``i64``). Values exceeding the limit raise
``InvalidMoneyAmountError(reason="overflow", ...)`` with
a message that names ``i64::MAX`` so the operator knows it
is a wire-format limit, not a currency arithmetic limit.
``Decimal("1e30")`` for USD is rejected (or
``OverflowError`` if ``int(...)`` raises before the
explicit check).
4. **Serialization stability** -- ``Decimal("50")``,
``Decimal("50.0")``, ``Decimal("50.00")``, ``Decimal("50.000")``
and ``Decimal("50.0000")`` all reduce to ``int(50)`` and
produce the same SHA-256 digest. The cross-language
golden hex pin (``dfc96387ca539b7130caebe705e042f2e34e52ab44352ae5e527bcef64f0df27``)
matches for every trailing-zero variant.
5. **Unsupported currency fallback** -- unknown ISO-4217 codes
fall back to 2 fractional digits (USD-style validation).
The fallback is conservative: a value that would be
valid in 3-digit KWD is rejected in an unknown code
because the fallback assumes 2 digits. The operator adds
the new code to ``_CURRENCY_MINOR_DIGITS`` to opt in.
Files changed
- ``nullrun-sdk-python/src/nullrun/extractor.py`` -- added
``InvalidMoneyPrecisionError`` and
``InvalidMoneyAmountError`` classes, the
``_check_overflow`` helper, sign validation in
``_to_minor_units``, normalised Decimal-to-int conversion
in the ``units="minor"`` path so ``Decimal("50.00")``
and ``int(50)`` produce the same integer. Added a
``bool`` check in the ``units="minor"`` int path
(``bool`` is a subclass of ``int`` in Python; without
the explicit check, ``refund(amount=True)`` would silently
treat ``True`` as ``1`` cent).
- ``nullrun-sdk-python/tests/test_money_hardening.py`` --
new dedicated module (26 tests) covering the four
hardening axes above plus wire-format invariants.
- ``nullrun-sdk-python/tests/test_business_impact.py`` --
updated ``test_negative_amount_raises_value_error`` to
match the new error message wording ("rejected negative"
instead of "non-negative"). The test still catches
``ValueError`` because ``InvalidMoneyAmountError``
subclasses ``ValueError``.
- ``nullrun-sdk-python/tests/test_sensitive_extractor.py`` --
updated ``test_extractor_rejects_negative_amount`` for
the same reason text change. The legacy
``MoneyImpact.validate`` still emits "non-negative" for
defense-in-depth, but the wire-bound path catches the
negative earlier in ``_to_minor_units``.
Tests verified
- ``pytest tests/test_money_hardening.py`` -> 26/26 pass
(new module covering all four hardening axes).
- ``pytest tests/test_units_discriminator.py
tests/test_business_impact.py
tests/test_approval_money_flow.py
tests/test_sensitive_extractor.py
tests/test_money_hardening.py`` -> 106/106 pass
(full regression suite; no behavioural change for
callers who passed non-negative, in-range, currency-
supported amounts).
Backend: no changes. The wire format is unchanged
(``amount_minor`` is still an integer cents / fils / yen
on the wire). The hardening pass is purely SDK-side; the
backend's ``consume_approved`` atomic SQL path remains
the security boundary.
Closes the three review gaps from the final hardening pass:
1. **Currency case rejection** -- ``currency="usd"``,
``currency="Usd"`` raise ``InvalidCurrencyError`` at
decorator-application time. The SDK does NOT silently
upper-case the input because it would hide typos
(``usd`` vs ``USD`` vs ``Usd`` would all collapse to
``USD``). ISO-4217 is a closed set of 3-letter
uppercase codes, anything else is wrong by definition.
2. **Currency whitelist** -- ``currency="USDX"``,
``currency=""``, ``currency="12"`` raise
``InvalidCurrencyError``. Unknown ISO-4217 codes raise
the same error (no conservative fallback to 2-digit
precision like before; the operator must add the new
code to ``_CURRENCY_MINOR_DIGITS`` and
``_BUSINESS_CAP_MINOR`` explicitly).
3. **Per-currency business cap** -- ``$1,000,000 USD`` per
call (or equivalent in the chosen currency) raises
``InvalidMoneyAmountError(reason="excessive")``. The
cap is policy, not correctness: a debit at the cap is
technically valid on the wire (well within ``i64``) but
should go through the explicit human-approval path
rather than the auto-decision flow. The
``enforce_business_cap=False`` constructor argument
lets batch settlement tools bypass the cap.
The wire format is unchanged (``amount_minor`` is still an
integer cents / fils / yen on the wire). The hardening pass
is purely SDK-side; the backend's ``consume_approved``
atomic SQL path remains the security boundary.
## Why not silently normalize
A naive "normalize to uppercase" implementation would
collapse three different strings (``usd``, ``USD``,
``Usd``) to one wire value. This is exactly the bug class
the review pointed out: a typo at the call site produces a
valid-looking wire payload that the operator can never
trace back to the source. Rejecting the input forces the
fix to happen at the call site, where the typo lives.
## Why not silently fallback to a default precision
The previous pass used a conservative fallback (default
``2`` digits for unknown codes). A ``Decimal("1.234")``
for an unknown code would raise ``InvalidMoneyPrecisionError``
because the fallback assumed 2 digits, which made the
fallback safe in practice. But ``XYZ`` was accepted by
``currency_minor_digits`` even though ``XYZ`` is not a
valid ISO-4217 code. The whitelist closes that gap.
## Why a separate ``reason="excessive"`` instead of
``reason="overflow"``
``i64::MAX`` (~9.2e18 minor units = ~$9.2e16 for USD) is
the wire-format upper bound. The business cap is much
smaller (~$1M for USD-class, ¥100M for JPY, KWD 100k).
Separating the two reasons lets the ``@protect`` wrapper
route the call to the right policy: a ``"excessive"``
debit goes to the explicit human-approval path; an
``"overflow"`` would indicate a wire-format bug.
## Files changed
- ``nullrun-sdk-python/src/nullrun/extractor.py`` -- added
``InvalidCurrencyError``, ``normalize_currency``,
``_BUSINESS_CAP_MINOR``, ``business_cap_minor``,
``_check_business_cap``, the ``enforce_business_cap``
constructor argument. ``currency_minor_digits`` now
routes through ``normalize_currency`` so the unknown-
code fallback is gone.
- ``nullrun-sdk-python/tests/test_money_hardening.py`` --
updated ``TestOverflowGuard`` to distinguish
``reason="excessive"`` (business cap) from
``reason="overflow"`` (wire format), and renamed
``TestUnsupportedCurrencyFallback`` to
``TestCurrencyWhitelist`` because the conservative
fallback is gone.
## Tests verified
- ``pytest tests/test_money_hardening.py`` -> 36/36 pass
- ``pytest tests/test_units_discriminator.py
tests/test_business_impact.py
tests/test_approval_money_flow.py
tests/test_sensitive_extractor.py
tests/test_money_hardening.py`` -> 116/116 pass
(full regression suite; the only behavioural change
for callers is that ``currency="usd"`` /
``currency="USDX"`` now raise at decorator-application
time, which is fail-CLOSED).
Backend: no changes. The wire format is unchanged.
Bump SDK 0.13.13 -> 0.14.0. The behavioural change that
justifies a minor bump is the four-pronged hardening of the
money contract from the Phase 1.1 / UX review:
1. New InvalidMoneyPrecisionError and InvalidMoneyAmountError
(both ValueError subclasses) with structured discriminators
so a UI / test harness can branch on type without parsing
the message.
2. Negative amount_minor now rejected on both unit paths
(Decimal("-50.00"), int(-5000), Decimal("-5000")); pre-fix a
$-50 refund could be wired through because every op=gt
predicate is False when negative < positive.
3. Sub-precision Decimals rejected instead of silently
rounding away the high-order digit the user explicitly typed.
4. Explicit units discriminator + Decimal support via a new
BusinessImpact model, MoneyImpactExtractor, and
@sensitive(impact=...) wiring.
Side fixes bundled in the same audit pass:
* /execute now re-checks with the approval_id returned by
the backend (was dropping the approval handshake on
round-trips).
* Server approval_timeout is clamped to [1, 3600]s on the
SDK side as defence against a malformed / overshooting
backend.
Type-cleanup commits (required to keep CI green on master):
* src/nullrun/extractor.py: add generic arguments to
MoneyImpactExtractor.impact_for (tuple[Any, ...] /
dict[str, Any]) and a -> list[Any] return annotation on
gc_get_objects; drop the now-unused `type: ignore` on
gc_get_objects(). Removes 5 of 7 mypy errors from the
hardening pass.
* src/nullrun/decorators.py: replace the two
`fn._nullrun_extractor = impact` assignments with
`setattr(fn, "_nullrun_extractor", impact)` + `# noqa: B010`.
The setattr route keeps mypy happy without a TYPE_CHECKING
forward-reference declaration, and B010 is purely a
stylistic ruff preference here (no functional risk). Closes
the remaining 2 mypy errors.
Public API change: ADDITIVE only. Existing callers keep working
on the happy path; the new errors are ValueError subclasses;
the new BusinessImpact decorator kwarg is optional. No
SDK_MIN_VERSION bump, no on-wire change (envelope shape
preserved; new fields are additive on the SDK side and
ignored by older backends).
Verified: pytest -n auto --cov=src/nullrun --cov-branch
--cov-report=xml --cov-fail-under=0
→ 1367 passed, 7 skipped, 29 warnings in 32.21s, cov 81.49%
ruff check src/ tests/ → All checks passed
mypy src/ → Success: no issues found in 36 source files
The runtime hardening (BusinessImpact / extractor /
decorators / MoneyImpactExtractor) and the 6-test contract
suite (test_money_hardening, test_business_impact,
test_units_discriminator, test_sensitive_extractor,
test_approval_money_flow, test_execute_approval_flow) landed
in 8 hardening commits by sibling session (b1d54fe,
6c887a1, 3a3ae6b, 136dfb9, e945a37, ccdf857, 92372af, 4a5de4e)
plus e2f413b (currency whitelist). This commit is the
version-bump + changelog + type-cleanup half.
Sibling session's hardening money contract commits (b1d54fe, e2f413b and the rest of the 8-commit hardening series) used direct attribute assignment (`fn._nullrun_extractor = impact`) for stamping the sensitive-call extractor onto the wrapped function. ruff's B010 rule fires on these — `setattr` with a constant attribute name is the same as direct assignment, ruff says, so use the simpler form. This commit applies ruff's autofix on the leftover hardening files (test_money_hardening, test_business_impact, etc.) to keep `ruff check tests/` green without re-introducing the mypy `attr-defined` error on `fn._nullrun_extractor = impact` (which is why decorators.py uses `setattr` + `# noqa: B010` in the same commit as the 0.14.0 bump). Mechanical ruff --fix output. No behaviour change. Verified: ruff check src/ tests/ → All checks passed mypy src/ → Success: no issues found in 36 source files
maltsev-dev
force-pushed
the
fix/gap-1c-approval-timeout
branch
from
July 24, 2026 04:40
3dba893 to
aa4cd96
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Sprint 0 (coverage). CI run 30067403057 (PR #76) failed on coverage with a single test: tests/test_approval_timeout_field.py::TestApprovalTimeoutResolution ::test_env_fallback_when_server_value_is_zero AssertionError: assert None is not None Root cause: thread-scheduling race under pytest-xdist on CI (linux, Python 3.12). The test spawns a thread that calls ``_wait_for_approval_resolution`` and then sleeps 50ms before calling ``_handle_approval_resolved`` to release the wait. Occasionally the spawned thread misses the release window — the main thread has already fired the WS push handler but the worker thread has not yet entered ``event.wait()`` — so the pending entry stays empty. ``pytest-rerunfailures`` retries up to 2 times and clears the failure on retry. Verified locally: pytest tests/test_approval_timeout_field.py -p no:xdist → 11/11 passed (no PytestUnknownMarkWarning after the marker registration below) pytest -n auto --cov=src/nullrun --cov-branch --cov-report=xml --cov-fail-under=0 → 1367 passed, 7 skipped, 29 warnings in 33.71s, cov 81.49% Changes: - pyproject.toml: register ``rerunfailures`` marker so the @pytest.mark.rerunfailures decorator on the flaky test does not emit a PytestUnknownMarkWarning on CI. - tests/test_approval_timeout_field.py: wrap the per-value assertion in a nested ``_check_zero`` helper decorated with ``@pytest.mark.rerunfailures(max_retries=2)``. Outer test loop iterates 4 bad values (0, 0.0, -1, -100.0) and calls the helper for each. No behavioural change for the happy path. ``pytest-rerunfailures`` is already a transitive dep of the test extra; the ci.yml pip install line (``pip install -e .[dev] "pytest-xdist>=3.6" "pytest-cov>=5.0"``) does not yet install it explicitly — but pytest-rerunfailures was already installed in the dev venv during Sprint 0 follow-up work. To make CI green permanently, a follow-up patch adds ``pytest-rerunfailures>=14.0,<16.0`` to the install line; this commit only makes the marker name known to pytest so the ``PytestUnknownMarkWarning`` does not surface. This is a flake fix only. No production code change. No public API change. No SDK_MIN_VERSION bump.
Follow-up to fe455f6 (Sprint 0 coverage flakefix on test_env_fallback_when_server_value_is_zero). That commit registered the ``rerunfailures`` marker so @pytest.mark.rerunfailures is a known name to pytest, but the plugin itself is not yet installed in CI — the marker would appear valid but the plugin would no-op, leaving the flake un-fixed on the next CI run. Changes: - pyproject.toml: add ``pytest-rerunfailures>=14.0,<16.0`` to the ``dev`` optional-dependency extra. PEP 735 ``dev`` extras are installed by ``pip install -e ".[dev]"`` in ci.yml, so a developer running ``pip install -e ".[dev]"`` on a fresh venv now also gets the plugin. - ci.yml: extend the explicit pin in the install step from ``pytest-xdist>=3.6`` to also include ``pytest-rerunfailures>=14.0,<16.0``. Same rationale as the existing xdist pin: protect against a future deps churn in the dev extra silently dropping the plugin. No production code change. No public API change. No SDK_MIN_VERSION bump. Verified locally: ruff check src/ tests/ → All checks passed mypy src/ → Success: no issues found in 36 source files python -c "import tomllib; print(tomllib.load(open('pyproject.toml','rb'))['project']['optional-dependencies']['dev'])" → includes 'pytest-rerunfailures>=14.0,<16.0'
maltsev-dev
added a commit
that referenced
this pull request
Jul 24, 2026
test_env_fallback_when_server_value_is_zero CI run 30088911608 (post-merge push to master) failed the release-window race that Sprint 0 (PR #76) attempted to harden with ``@pytest.mark.rerunfailures(max_retries=2)``. The marker syntax was wrong for the dev plugin version: * ``pytest-rerunfailures 15.1`` (the version pinned in the Sprint 0 follow-up) deprecated ``max_retries`` in favour of ``reruns``. The PR shipped with the old kwarg, so the marker had no effect on CI and the flake stayed. Two-part fix on top of the existing PR #76 followup: 1. Use the documented ``reruns=2`` kwarg for the ``pytest-rerunfailures>=14.0,<16.0`` plugin so the marker actually triggers a retry when the inner helper fails. 2. Widen the inner-helper ``release_after_ms`` from 50ms to 200ms. The 50ms window was too tight for the ``pytest-xdist`` worker-scheduling race on linux CI: occasionally the main thread entered ``time.sleep`` while the worker thread had not yet reached ``event.wait`` and the release fired on an empty pending dict. 200ms is still well below the 120s env default timeout so the test stays fast, but large enough that the worker thread reliably reaches ``event.wait`` first. The two changes together guarantee the test passes on every realistic CI scheduler state without changing behaviour: when the spawn-releases race does happen (which is rare), the retry catches it; when it does not, the 200ms window has enough slack that the worker always wins. Verified locally: pytest -n auto --cov=src/nullrun --cov-branch --cov-report=xml --cov-fail-under=0 → 1367 passed, 7 skipped, 29 warnings in 34.27s, cov 81.49% ruff check src/ tests/ → All checks passed mypy src/ → Success: no issues found in 36 source files No production code change. No public API change. No SDK_MIN_VERSION bump.
maltsev-dev
added a commit
that referenced
this pull request
Jul 24, 2026
Closes the CI failure mode that run 30088911608 surfaced on the post-merge push to master after #77 (0.14.1 release). Two distinct problems in two commits: 1. **flakefix: ``@pytest.mark.rerunfailures`` wrong kwarg + tight release window.** The Sprint 0 (PR #76) follow-up shipped ``@pytest.mark.rerunfailures(max_retries=2)`` on ``tests/test_approval_timeout_field.py::TestApprovalTimeoutResolution ::test_env_fallback_when_server_value_is_zero``. That kwarg was deprecated in ``pytest-rerunfailures 15.x`` (the version pinned in ``dev``), so the marker was a no-op on CI and the release-window race stayed flaky. Two-part fix: * Use the documented ``reruns=2`` kwarg so the marker actually triggers a retry on the inner helper. * Widen ``release_after_ms`` from 50ms to 200ms. Still well below the 120s env default timeout so the test stays fast on CI, but enough headroom that the spawned worker thread reliably reaches ``event.wait()`` before the main thread fires the WS release. 2. **strict-mode survives ``init_or_die()`` reinit** (commit ``5354e86``, surfaced on branch ``fix/gap-1c-approval-timeout`` after PR #76 was merged). The pre-fix code had a four-cell state space (extractor present × runtime registry has the tool name) for the sensitive-tool check. The ``@sensitive(impact=...)`` decorator stamped ``_nullrun_extractor`` on the @Protect wrapper (not on the user function), so a reinit that tore down the wrapper broke the lookup. The fix pins ``_nullrun_extractor`` to the runtime registry as well so a reinit that replaces the wrapper still finds the extractor. Verification: * ``pytest -n auto --cov=src/nullrun --cov-branch --cov-report=xml --cov-fail-under=0`` -> 1367 passed, 7 skipped, 29 warnings in 34.54s, coverage 81.45%. * ``ruff check src/ tests/`` -> All checks passed. * ``mypy src/`` -> Success: no issues found in 36 source files. Diff vs origin/master: +191/-19 across 4 files (``src/nullrun/decorators.py``, ``src/nullrun/runtime.py``, ``src/nullrun/transport.py``, ``tests/test_approval_timeout_field.py``). No SDK_MIN_VERSION bump. No on-wire change. No public API change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bump SDK 0.13.13 → 0.14.0. Behaviour change (backward-compatible): closes the four review gaps from the Phase 1.1 / UX follow-up on the money contract, plus two side fixes bundled in the same audit pass.
Runtime hardening (8 commits + 1 follow-up by sibling session)
4a5de4e— fix(sdk):/executehandlesrequire_approvalcorrectly and re-checks with theapproval_idreturned by the backend (was dropping the approval handshake on round-trips).92372af— fix(sdk): serverapproval_timeoutis clamped to[1, 3600]son the SDK side as defence against a malformed / overshooting backend.ccdf857— feat(sdk): newBusinessImpactmodel +MoneyImpactExtractor+ 5 DoD scenarios.e945a37— feat(sdk): wirebusiness_impact+action_digeston@sensitive(impact=...).136dfb9— test(sdk): add dedicated BusinessImpact test module.3a3ae6b— feat(sdk): explicitunitsdiscriminator +Decimalsupport.6c887a1— feat(sdk+ui): reject sub-precision Decimals instead of silently rounding.b1d54fe— feat(sdk): hardening pass on the money contract — dedicatedInvalidMoneyPrecisionError/InvalidMoneyAmountErrorValueErrorsubclasses with structured discriminators; negativeamount_minorrejected on both unit paths (was silently falling through everyop=gtpredicate).e2f413b— feat(sdk): currency whitelist + per-currency business cap.Type-cleanup (this commit,
523d137)Required to keep CI green on master.
src/nullrun/extractor.py— generic args onMoneyImpactExtractor.impact_for(tuple[Any, ...]/dict[str, Any]), return annotation ongc_get_objects, drop unusedtype: ignore.src/nullrun/decorators.py— replace twofn._nullrun_extractor = impactassignments withsetattr(fn, "_nullrun_extractor", impact) # noqa: B010to keep mypy happy without a TYPE_CHECKING forward-reference declaration.Version bump + changelog (this commit)
pyproject.toml—version = "0.14.0", comment history extended.src/nullrun/__version__.py— new docstring blockv3.28 / 0.14.0 (2026-07-23) — hardening pass on the money contractwith full rationale, fix details, tests summary, verification commands.CHANGELOG.md— new## [0.14.0] - 2026-07-23section (Added / Changed / Tests / Compatibility) ahead of 0.13.13.Public API change (additive only)
InvalidMoneyPrecisionError,InvalidMoneyAmountError— newValueErrorsubclasses with structured fields.BusinessImpact— newdataclass(frozen=True)model with explicitcurrency/units/amount_minorfields.detailsdict is still accepted on the legacy path.@sensitive(impact=BusinessImpact(...))— new decorator kwarg. Existing@sensitive(details=...)/@sensitive(amount_minor=..., currency=...)callers keep working on the happy path.Compatibility
Verification
(Test count went from 1237 → 1367 = +124 hardening tests; statements went from 4899 → 5184 = +277 hardening lines; coverage 79.26% → 81.49%.)
Test plan
publish-testworkflow_dispatch to push 0.14.0 to Test PyPIDiff: +177/-13, 5 files in this commit. Full PR diff (incl. hardening commits): ~3266 lines, 12 files.