From b63d57ca6aadc3cbbe32f8c7a7be5212182ccd4e Mon Sep 17 00:00:00 2001 From: ciregenz Date: Tue, 28 Jul 2026 23:58:43 -0700 Subject: [PATCH] [eric] browser: a check that could not observe returns unknown, never a false negative --- .../agents/browser/browser_delivery_check.py | 28 ++++- .../agents/browser/browser_send_script.py | 7 +- backend/tests/test_observation_honesty.py | 114 ++++++++++++++++++ 3 files changed, 142 insertions(+), 7 deletions(-) create mode 100644 backend/tests/test_observation_honesty.py diff --git a/backend/apps/agents/browser/browser_delivery_check.py b/backend/apps/agents/browser/browser_delivery_check.py index f49d3700..963fce0c 100644 --- a/backend/apps/agents/browser/browser_delivery_check.py +++ b/backend/apps/agents/browser/browser_delivery_check.py @@ -11,7 +11,7 @@ Gmail) and this module is never consulted, so proven sends keep their exact spee import asyncio import json import re -from typing import Awaitable, Callable +from typing import Awaitable, Callable, Optional from urllib.parse import urlparse from typeguard import typechecked @@ -52,15 +52,28 @@ def delivery_probe_expression(payload: str) -> str: @typechecked -async def payload_visible(payload: str, browser_id: str, tab_id: str, execute_tool: ToolRunner) -> bool: +async def payload_visible( + payload: str, browser_id: str, tab_id: str, execute_tool: ToolRunner +) -> Optional[bool]: + """True = seen on the page, False = looked and it is NOT there, None = could not look. + + The third case is not pedantry. Returning False for a probe that timed out or came back + unreadable is asserting absence from a failed observation, and that is the same mistake as a + receipt claiming delivery it never saw, pointed the other way: it tells the user a post did not + land when nobody actually checked. Measured tonight, the identical shape in the test harness + scored every unreadable verification as a successful delete and left six posts on a real + account while reporting them cleaned. + """ try: r = await asyncio.wait_for(execute_tool( "BrowserEvaluate", {"expression": delivery_probe_expression(payload)}, browser_id, tab_id), timeout=6.0) except Exception: - return False + return None v = browser_submit_click.parse_eval_value(r) - return bool(isinstance(v, dict) and v.get("visible")) + if not isinstance(v, dict) or "visible" not in v: + return None + return bool(v.get("visible")) @typechecked @@ -125,10 +138,13 @@ async def ghost_delivery_confirmed( only if the payload is visible now and STILL visible a few seconds later. A post that never rendered, or rendered then vanished, returns False, so we never claim a delivery the site ate. Pure page reads (no navigation), invisible to the site.""" - if not await payload_visible(payload, browser_id, tab_id, execute_tool): + # `is not True` deliberately: an unknown must NOT confirm. This is the one place where + # collapsing unknown into "no" is right, because the caller is deciding whether to CLAIM a + # delivery, and withholding an uncertain claim is the safe direction. + if await payload_visible(payload, browser_id, tab_id, execute_tool) is not True: return False await asyncio.sleep(3.5) - return await payload_visible(payload, browser_id, tab_id, execute_tool) + return await payload_visible(payload, browser_id, tab_id, execute_tool) is True @typechecked diff --git a/backend/apps/agents/browser/browser_send_script.py b/backend/apps/agents/browser/browser_send_script.py index 35a1b209..efd0700d 100644 --- a/backend/apps/agents/browser/browser_send_script.py +++ b/backend/apps/agents/browser/browser_send_script.py @@ -148,9 +148,14 @@ async def complete_send( # their measured speed. delivered = await browser_delivery_check.payload_visible( payload, browser_id, tab_id, execute_tool) - if not delivered: + if delivered is False: logger.info("[browser-sendscript] by-name click cleared the composer but the payload " "never rendered; treating as NOT delivered") + elif delivered is None: + # We could not look. That is not the same as looking and finding nothing, and saying + # "it did not render" here would be inventing a failure out of a broken probe. + logger.info("[browser-sendscript] by-name click cleared the composer but the delivery " + "probe was unreadable; leaving delivery UNKNOWN") if rejected: # We are not guessing here: the page said no. Saying "unverified" would send the user off to # check something we already know the answer to. diff --git a/backend/tests/test_observation_honesty.py b/backend/tests/test_observation_honesty.py new file mode 100644 index 00000000..c35febd8 --- /dev/null +++ b/backend/tests/test_observation_honesty.py @@ -0,0 +1,114 @@ +"""A check that could not observe must say "unknown", never "no". + +This bug class cost most of a night. The identical shape appeared in three places, and in every one +it manufactured a confident false answer: + + 1. the send receipt a cleared composer was read as delivered even when the site had refused + 2. the campaign verifier a verify turn that could not reach the profile scored the post "not landed" + 3. the cleanup accounting a recheck that could not read the page scored the post "deleted" + +(3) is the one that did real damage: it reported a clean account while six test posts sat live on +it, twice. All three are the same mistake, absence of evidence recorded as evidence of absence, and +patching them one at a time is treating symptoms. + +The rule, and what this file pins: + + - An observation returns True / False / None, where None means the observation did not happen. + - Collapsing None is allowed in exactly one direction: when deciding whether to CLAIM something, + unknown must behave as "do not claim". Withholding an uncertain claim is safe; asserting a + negative from a failed look is not. +""" +import asyncio + +import pytest + +from backend.apps.agents.browser import browser_delivery_check as dc + + +def p_eval(value): + async def run(tool, args, browser_id, tab_id): + return {"value": value} + return run + + +async def p_boom(tool, args, browser_id, tab_id): + raise RuntimeError("probe died") + + +async def p_hang(tool, args, browser_id, tab_id): + await asyncio.sleep(30) + return {} + + +# --- payload_visible is a three-valued observation ------------------------------------------- + +@pytest.mark.asyncio +async def test_seen_is_true(): + assert await dc.payload_visible("hi", "b", "t", p_eval({"visible": True})) is True + + +@pytest.mark.asyncio +async def test_looked_and_absent_is_false(): + """The genuine negative: the probe ran and the text is not on the page.""" + assert await dc.payload_visible("hi", "b", "t", p_eval({"visible": False})) is False + + +@pytest.mark.asyncio +async def test_a_failed_probe_is_unknown_not_absent(): + """The regression. Returning False here tells the user a post did not land when nobody looked.""" + assert await dc.payload_visible("hi", "b", "t", p_boom) is None + + +@pytest.mark.asyncio +async def test_a_hung_probe_is_unknown(): + assert await dc.payload_visible("hi", "b", "t", p_hang) is None + + +@pytest.mark.asyncio +async def test_an_unreadable_shape_is_unknown(): + """A dict without the key is not a negative answer; it is a broken answer.""" + assert await dc.payload_visible("hi", "b", "t", p_eval({"nope": 1})) is None + assert await dc.payload_visible("hi", "b", "t", p_eval("garbage")) is None + + +# --- the one legitimate collapse: do not CLAIM on unknown ------------------------------------ + +@pytest.mark.asyncio +async def test_ghost_confirmation_refuses_to_claim_on_unknown(): + """Deciding whether to assert a delivery: unknown must behave as "do not assert".""" + assert await dc.ghost_delivery_confirmed("hi", "b", "t", p_boom) is False + + +@pytest.mark.asyncio +async def test_ghost_confirmation_still_confirms_a_real_survival(): + assert await dc.ghost_delivery_confirmed("hi", "b", "t", p_eval({"visible": True})) is True + + +@pytest.mark.asyncio +async def test_ghost_confirmation_returns_a_hard_bool(): + """Its contract is binary by design (claim / do not claim); leaking a None here would make an + unknown look like a confirmed delivery to any `if` further up.""" + for probe in (p_boom, p_eval({"visible": True}), p_eval({"visible": False})): + got = await dc.ghost_delivery_confirmed("hi", "b", "t", probe) + assert got is True or got is False + + +# --- the rejection probe collapses the other way, and that is also correct -------------------- + +@pytest.mark.asyncio +async def test_a_broken_rejection_probe_does_not_invent_a_refusal(): + """Mirror image: send_rejected decides whether to assert a FAILURE, so unknown must behave as + "do not assert" there too. Same rule, opposite polarity, because the claim is inverted.""" + assert await dc.send_rejected("b", "t", p_boom) is False + + +@pytest.mark.asyncio +async def test_the_send_script_distinguishes_absent_from_unlooked(): + """The caller must branch on `is False` / `is None`, not truthiness. `if not delivered` treats + an unknown exactly like a proven absence, which is the bug this whole file exists for.""" + src = dc.__file__.replace("browser_delivery_check.py", "browser_send_script.py") + with open(src) as f: + text = f.read() + assert "delivered is False" in text + assert "delivered is None" in text + assert "if not delivered:" not in text, "truthiness collapses unknown into absent"