test: pin the #579 fix that nothing was pinning, in both directions

tests/test_radiant_reserves_use_the_radiant_interval.py

@@ -20,81 +20,206 @@
 
 Seven sites is why this is a scanner and not seven fixes.
 
-WHAT THIS COVERS, AND WHAT IT DOES NOT. The scanner is line-based: it catches a
-CALL SITE that names a Radiant field and the Bitcoin interval together — the shape
-that actually shipped. It cannot see a regression INSIDE
-`_radiant_reserve_blocks`, whose parameter is a generic `reserve` with no field
-name on the line.
-
-That half is covered by `test_assess_claim_finality_parity_sweep_byte_equivalent`,
-whose reference derives the interval from the rule independently and now sweeps a
-SECONDS-tagged policy. Verified rather than assumed: switching the helper back to
-the Bitcoin interval fails the parity sweep, and adding a fresh call site that
-bypasses the helper fails this file. Neither guard covers both, and saying so is
-the point — a scanner presented as complete is worse than one with a stated edge.
+WHAT THIS COVERS, AND WHAT IT DOES NOT. The scan is per CALL, over the AST. It was
+per LINE, over a regex, and that missed a site that had shipped: in
+`claim_executor._check_value_cap` the field and the interval sit on two different
+lines of one call, so no line contained both. Measured — reverting that site passed
+the entire suite (10,993 tests), its own module, AND this file. The behavioural
+guard for it now lives in `TestTheValueCapReadsTheRadiantInterval`.
+
+IT IS ALSO SYMMETRIC NOW. A Bitcoin reserve converted with the RADIANT interval is
+the same defect and was covered by nothing. Found by accident: a bad restore
+rewrote `btc_claim_reorg_depth` to use `rxd_block_interval_s` and the whole suite
+passed. The field-to-chain map is read off `MarginPolicy` by prefix rather than
+hand-typed, so a new `rxd_`/`btc_` reserve is covered the day it is added — the
+previous hand-kept tuple is the artifact that let #511's `rxd_claim_inclusion` be
+left off a list in the first place.
+
+Still not covered: a regression INSIDE `_radiant_reserve_blocks`, whose parameter is
+a generic `reserve` with no field name anywhere in the call. That half is covered by
+`test_assess_claim_finality_parity_sweep_byte_equivalent`, whose reference derives
+the interval from the rule independently and sweeps a SECONDS-tagged policy.
+Verified rather than assumed: switching the helper back to the Bitcoin interval
+fails the parity sweep, and adding a fresh call site that bypasses the helper fails
+this file. Neither guard covers both, and saying so is the point — a scanner
+presented as complete is worse than one with a stated edge.
 """
 
 from __future__ import annotations
 
-import re
+import ast
 from pathlib import Path
 
 import pytest
 
 _ROOT = Path(__file__).resolve().parent.parent
 _SRC = _ROOT / "src"
 
-#: Fields that count RADIANT blocks.
-_RADIANT_FIELDS = ("rxd_claim_burial", "rxd_claim_inclusion")
+#: Which interval belongs to which chain. The ONLY two spellings that exist.
+_INTERVAL_FOR = {"rxd": "rxd_block_interval_s", "btc": "block_interval_s"}
 
-#: A conversion naming a Radiant field and the BITCOIN interval in one expression.
-#: `rxd_block_interval_s` contains `block_interval_s`, so the pattern must exclude
-#: the correct spelling explicitly rather than by substring.
-_BAD = re.compile(
-    r"(?<!rxd_)block_interval_s\s*=?\s*[^,)\n]*(?:" + "|".join(_RADIANT_FIELDS) + r")"
-    r"|(?:" + "|".join(_RADIANT_FIELDS) + r")[^)\n]{0,80}?(?<!rxd_)block_interval_s"
-)
+
+def _chain_tagged_reserves() -> dict[str, str]:
+    """Every ``Timelock`` field on :class:`MarginPolicy` that names a chain, DERIVED.
+
+    This was a hand-typed tuple of two field names. A hand-kept list of the things a
+    guard covers is the same artifact that produced the bug it guards: ``#511`` added
+    ``rxd_claim_inclusion`` and it was left off the validation list, fail-open. Reading
+    the prefix off the dataclass means a new ``rxd_``/``btc_`` reserve is covered the
+    day it is added, by nobody remembering anything.
+    """
+    from pyrxd.btc_wallet.taproot import Timelock
+    from pyrxd.gravity.swap_coordinator import MarginPolicy
+
+    tagged = {}
+    for name, field in MarginPolicy.__dataclass_fields__.items():
+        if field.type not in (Timelock, "Timelock"):
+            continue
+        for chain in _INTERVAL_FOR:
+            if name.startswith(f"{chain}_"):
+                tagged[name] = chain
+    if not tagged:
+        raise AssertionError("no chain-tagged reserve fields found — the derivation is broken")
+    return tagged
+
+
+def _identifiers(node: ast.AST) -> set[str]:
+    """Every attribute and bare name mentioned anywhere inside *node*.
+
+    Exact identifiers, which is the point of using the AST rather than the text:
+    ``rxd_block_interval_s`` CONTAINS ``block_interval_s`` as a substring, so the
+    line-based pattern this replaces had to exclude the correct spelling by hand.
+    """
+    out: set[str] = set()
+    for child in ast.walk(node):
+        if isinstance(child, ast.Attribute):
+            out.add(child.attr)
+        elif isinstance(child, ast.Name):
+            out.add(child.id)
+    return out
 
 
 def _offenders() -> list[str]:
+    """Call sites converting a chain's reserve with the OTHER chain's interval.
+
+    Per CALL, not per LINE. The line-based version could not see the shape that
+    shipped in `claim_executor._check_value_cap`, where the field and the interval sit
+    on two different lines of one call — measured: reverting that site passed the
+    entire suite, and this file, and was caught by nothing.
+    """
+    tagged = _chain_tagged_reserves()
     hits = []
     for path in sorted(_SRC.rglob("*.py")):
-        for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1):
-            stripped = line.strip()
-            if stripped.startswith("#") or "test" in path.name:
+        tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path))
+        for node in ast.walk(tree):
+            if not isinstance(node, ast.Call):
                 continue
-            if _BAD.search(line):
-                hits.append(f"{path.relative_to(_ROOT)}:{lineno}: {stripped[:100]}")
+            names = _identifiers(node)
+            for field, chain in tagged.items():
+                if field not in names:
+                    continue
+                wrong = _INTERVAL_FOR[next(c for c in _INTERVAL_FOR if c != chain)]
+                if wrong in names and _INTERVAL_FOR[chain] not in names:
+                    hits.append(f"{path.relative_to(_ROOT)}:{node.lineno}: {field} ({chain}) converted with {wrong}")
     return hits
 
 
 def test_the_scanner_catches_the_shipped_defect() -> None:
     """Non-vacuity: a scanner matching nothing is indistinguishable from a clean tree."""
-    shipped = "        burial_blocks = _reserve_to_blocks(policy.rxd_claim_burial, policy.block_interval_s)"
-    assert _BAD.search(shipped), "the pattern must fire on the line that actually shipped"
+    tree = ast.parse("blocks = _reserve_to_blocks(policy.rxd_claim_burial, policy.block_interval_s)")
+    call = next(n for n in ast.walk(tree) if isinstance(n, ast.Call))
+    names = _identifiers(call)
+    assert "rxd_claim_burial" in names and "block_interval_s" in names
+    assert "rxd_block_interval_s" not in names
+
+
+def test_the_scanner_sees_a_call_SPLIT_OVER_LINES() -> None:
+    """The shape that defeated the line-based version, and the reason for the rewrite.
+
+    In `claim_executor._check_value_cap` the field and the interval are on separate
+    lines of one call. A per-line regex matches neither line."""
+    src = (
+        "burial = self._policy.rxd_claim_burial.normalize_to(\n"
+        "    TimeUnit.BLOCKS, block_interval_s=self._policy.block_interval_s\n"
+        ").value\n"
+    )
+    call = next(
+        n for n in ast.walk(ast.parse(src)) if isinstance(n, ast.Call) and "rxd_claim_burial" in _identifiers(n)
+    )
+    names = _identifiers(call)
+    assert "block_interval_s" in names and "rxd_block_interval_s" not in names
 
 
 def test_the_scanner_accepts_the_CORRECT_spelling() -> None:
     """Honest path. `rxd_block_interval_s` contains `block_interval_s` as a substring,
-    so a naive pattern would flag every correct call site — a guard that refuses
-    valid work, on the very line that fixes the bug."""
-    fixed = "        burial_blocks = _reserve_to_blocks(policy.rxd_claim_burial, policy.rxd_block_interval_s)"
-    assert not _BAD.search(fixed)
+    so a naive pattern flags every correct call site — a guard refusing valid work, on
+    the very line that fixes the bug."""
+    tree = ast.parse("b = _reserve_to_blocks(policy.rxd_claim_burial, policy.rxd_block_interval_s)")
+    names = _identifiers(next(n for n in ast.walk(tree) if isinstance(n, ast.Call)))
+    assert "block_interval_s" not in names, "the AST must not see a substring"
+
+
+def test_the_scan_is_SYMMETRIC() -> None:
+    """A Bitcoin reserve converted with the RADIANT interval is the same defect.
+
+    Not hypothetical: while verifying this file I accidentally rewrote
+    `btc_claim_reorg_depth` to use `rxd_block_interval_s`, and the whole suite —
+    10,993 tests — passed. The old scanner only looked one way."""
+    assert _chain_tagged_reserves()["btc_claim_reorg_depth"] == "btc"
+    tree = ast.parse(
+        "d = policy.btc_claim_reorg_depth.normalize_to(BLOCKS, block_interval_s=policy.rxd_block_interval_s)"
+    )
+    names = _identifiers(next(n for n in ast.walk(tree) if isinstance(n, ast.Call)))
+    assert "btc_claim_reorg_depth" in names and "rxd_block_interval_s" in names
 
 
-def test_no_radiant_reserve_is_converted_with_the_bitcoin_interval() -> None:
+def test_no_reserve_is_converted_with_the_other_chains_interval() -> None:
     offenders = _offenders()
     assert not offenders, (
-        "a RADIANT-chain reserve is being converted with the BITCOIN interval:\n  "
+        "a chain's reserve is being converted with the OTHER chain's interval:\n  "
         + "\n  ".join(offenders)
         + "\n\nUse `_radiant_reserve_blocks(policy, reserve)`, which picks the interval once."
     )
 
 
-@pytest.mark.parametrize("field", _RADIANT_FIELDS)
-def test_the_fields_this_guards_still_exist(field: str) -> None:
-    """Stale-guard ratchet: if a field is renamed, this scanner silently stops
-    covering it and would keep passing forever."""
-    from pyrxd.gravity.swap_coordinator import MarginPolicy
+def test_the_derivation_still_finds_the_fields_this_guards() -> None:
+    """Stale-guard ratchet. If every field were renamed out from under the prefix
+    rule, the scan would pass forever by covering nothing."""
+    tagged = _chain_tagged_reserves()
+    assert {"rxd_claim_burial", "rxd_claim_inclusion", "btc_claim_reorg_depth"} <= set(tagged)
+
+
+# --- the scan must prove it actually scanned -------------------------------
+#
+# `_offenders()` returning [] is the PASS condition, and it also returns [] when it
+# walked nothing at all: a repointed `_SRC`, a changed glob, or the source moving
+# under it all read as "clean". A guard whose success and whose vacuity produce the
+# same output is not a guard — the same defect this file's own subject had, and the
+# same one that let the browser render-drift suite pass over unrendered fields.
 
-    assert field in MarginPolicy.__dataclass_fields__, f"{field} no longer exists; update this scanner"
+
+def _calls_mentioning(field: str) -> int:
+    """How many Call nodes in the scanned tree mention *field* at all."""
+    total = 0
+    for path in sorted(_SRC.rglob("*.py")):
+        for node in ast.walk(ast.parse(path.read_text(encoding="utf-8"), filename=str(path))):
+            if isinstance(node, ast.Call) and field in _identifiers(node):
+                total += 1
+    return total
+
+
+def test_the_scan_reaches_a_plausible_amount_of_source() -> None:
+    files = list(_SRC.rglob("*.py"))
+    assert len(files) > 50, f"only {len(files)} source files found — is _SRC still the package root?"
+
+
+@pytest.mark.parametrize("field", sorted(_chain_tagged_reserves()))
+def test_each_guarded_field_is_actually_REACHED_by_the_scan(field: str) -> None:
+    """The sharper check. A field nobody converts anywhere is a field this scanner
+    cannot be protecting, however many files it walks — and it would pass forever."""
+    count = _calls_mentioning(field)
+    assert count > 0, (
+        f"{field!r} is derived as a chain-tagged reserve but appears in NO call in the "
+        f"scanned source, so the scan covers it vacuously. Either the field is dead "
+        f"(remove it) or the scan is no longer reading the right tree."
+    )

tests/test_watch_claim_executor.py

@@ -8,6 +8,7 @@
 
 from __future__ import annotations
 
+import dataclasses
 import hashlib
 import json
 import os
@@ -19,7 +20,7 @@
 from pyrxd.btc_wallet.htlc_leg import BitcoinTaprootLeg
 from pyrxd.btc_wallet.keys import generate_keypair
 from pyrxd.btc_wallet.payment import BtcUtxo
-from pyrxd.btc_wallet.taproot import btc_txid_from_raw
+from pyrxd.btc_wallet.taproot import Timelock, TimeUnit, btc_txid_from_raw
 from pyrxd.gravity.swap_coordinator import MarginPolicy
 from pyrxd.gravity.swap_state import NegotiatedTerms, SwapRecord, SwapState
 from pyrxd.gravity.watch import (
@@ -911,3 +912,77 @@ async def test_a_REACHABLE_state_still_broadcasts(self) -> None:
         """
         ex, _leg, rec, _ = await _armed_executor(confs=24, btc_confs=10)
         assert await ex.execute("s1", rec, _claim_decision()) is ExecOutcome.BROADCAST
+
+
+class TestTheValueCapReadsTheRadiantInterval:
+    """`_check_value_cap` converts a RADIANT burial, so it must use RADIANT's interval.
+
+    #579 fixed seven sites; this one had NO test that failed when reverted. Measured
+    rather than argued: switching it back to `policy.block_interval_s` passed all
+    10,993 tests, `test_watch_claim_executor.py`, and the scanner written for exactly
+    this defect class — which is line-based and cannot see a call split over two
+    lines, the shape here.
+
+    It survives because it is INERT while every fixture tags the burial in BLOCKS:
+    `normalize_to` is then the identity and the interval argument is never read. No
+    test in this file supplied a SECONDS-tagged burial, so the wrong argument and the
+    right one produced identical output. **A test whose fixture cannot express the
+    defect passes for a reason unrelated to the code being correct.**
+
+    So the fixture below deliberately makes the two intervals DIFFER (600 vs 300) and
+    picks an amount BETWEEN the two ceilings. Equal values hide conflations; this is
+    the smallest fixture in which the two readings disagree.
+    """
+
+    #: 1800 s is 6 Radiant blocks (ceiling 3000) or 3 Bitcoin blocks (ceiling 1500).
+    BURIAL_S, RXD_INTERVAL, BTC_INTERVAL = 1800, 300.0, 600.0
+    COST, FACTOR = 1000, 2.0
+    CEILING_RADIANT, CEILING_BITCOIN = 3_000, 1_500
+    #: Between the two. Legitimate at the real ceiling, refused at the wrong one.
+    AMOUNT = 2_000
+
+    def _policy(self) -> MarginPolicy:
+        return dataclasses.replace(
+            MarginPolicy.estimated(),
+            block_interval_s=self.BTC_INTERVAL,
+            rxd_block_interval_s=self.RXD_INTERVAL,
+            rxd_claim_burial=Timelock(self.BURIAL_S, TimeUnit.SECONDS),
+        )
+
+    async def _gate(self, amount: int):
+        terms, _p, raw, txid, locator, _b = await _build_real_claim(radiant_amount=amount)
+        ex = ClaimExecutor(
+            resolve_leg=_resolver(_FakeRadiantLeg(_FakeChainIO())),
+            claim_status_source=_FakeStatusSource(claim_txid=txid),
+            claim_bytes_source=_FakeBytesSource({txid: raw}),
+            policy=self._policy(),
+            network="mainnet",
+            reorg_cost_per_block=self.COST,
+            reorg_safety_factor=self.FACTOR,
+            claim_dust_ceiling=10_000,
+            enable_autonomous_mainnet_custody=True,
+        )
+        rec = SwapRecord(state=SwapState.SECRET_REVEALED, terms=terms, counterchain_locator=locator)
+        return ex._check_value_cap(rec)
+
+    def test_the_two_intervals_really_do_disagree_here(self) -> None:
+        """Guards the fixture itself. If these ever coincide the test below proves
+        nothing while still passing — the failure mode it exists to catch."""
+        policy = self._policy()
+        radiant = policy.rxd_claim_burial.normalize_to(TimeUnit.BLOCKS, block_interval_s=policy.rxd_block_interval_s)
+        bitcoin = policy.rxd_claim_burial.normalize_to(TimeUnit.BLOCKS, block_interval_s=policy.block_interval_s)
+        assert radiant.value == 6 and bitcoin.value == 3
+        assert self.CEILING_BITCOIN < self.AMOUNT < self.CEILING_RADIANT
+
+    async def test_an_honest_claim_inside_the_RADIANT_ceiling_is_allowed(self) -> None:
+        """The assertion that fails when #579 is reverted here. With Bitcoin's
+        interval the burial reads 3 blocks, the ceiling halves to 1500, and this
+        legitimate 2000-photon claim is refused — a guard refusing valid work, and
+        for an autonomous claim a refusal can mean the swap is not claimed at all."""
+        assert await self._gate(self.AMOUNT) is None
+
+    async def test_a_claim_above_the_RADIANT_ceiling_is_still_refused(self) -> None:
+        """The other half. A gate that allows everything would also pass the test
+        above, so pin that it still says no — and that it names the real ceiling."""
+        reason = await self._gate(self.CEILING_RADIANT + 1)
+        assert reason is not None and str(self.CEILING_RADIANT) in reason