From be1bf931d572fb9b4456c6868be2ffb250747f4f Mon Sep 17 00:00:00 2001 From: Subodh Bansode Date: Sun, 11 Oct 2026 13:30:35 +0530 Subject: [PATCH 1/7] fix(browser): preserve user downloads when attached (#281) --- sentient/browser/service.py | 97 +++++++++++- tests/browser/test_profiles.py | 226 ++++++++++++++++++++++++++++ tests/browser/test_profiles_live.py | 33 +++- 3 files changed, 347 insertions(+), 9 deletions(-) diff --git a/sentient/browser/service.py b/sentient/browser/service.py index 73909a89..9dc4cfb7 100644 --- a/sentient/browser/service.py +++ b/sentient/browser/service.py @@ -69,6 +69,7 @@ DOWNLOAD_CANCEL_TIMEOUT_S = 5.0 DOWNLOAD_TASK_RETENTION_S = 300.0 DOWNLOAD_TASK_CLEANUP_INTERVAL_S = 30.0 +ATTACHED_MATCH_TIMEOUT_S = 2.0 # models often pass the whole snapshot line ("[e4] button \"Place order\"") instead of just "e4" _REF_RE = re.compile(r"\b(e\d+)\b", re.IGNORECASE) @@ -238,6 +239,9 @@ def __init__(self, app): self._download_completed_at: dict[asyncio.Task[str], float] = {} self._download_cleanup_task: asyncio.Task | None = None self._download_lock = asyncio.Lock() + self._browser_cdp: Any = None + self._attached_downloads: list[dict[str, Any]] = [] + self._attached_match_timeout_s = ATTACHED_MATCH_TIMEOUT_S # ------------------------------------------------------------------ lifecycle async def start(self) -> None: @@ -602,6 +606,12 @@ async def _attach(self, prof: Any) -> Any: self._browser = browser self._attached = True self._engine = None + with contextlib.suppress(Exception): + cdp = await browser.new_browser_cdp_session() + self._browser_cdp = cdp + cdp.on("Browser.downloadWillBegin", self._on_cdp_download_will_begin) + cdp.on("Browser.downloadProgress", self._on_cdp_download_progress) + await cdp.send("Browser.setDownloadBehavior", {"behavior": "default", "eventsEnabled": True}) return context async def _ensure(self, ctx: Any = None) -> Any: @@ -657,6 +667,11 @@ async def _close_context(self) -> None: self._focused = None self._download_pages.clear() self._internal_pages.clear() + if self._browser_cdp is not None: + with contextlib.suppress(Exception): + await self._browser_cdp.detach() + self._browser_cdp = None + self._attached_downloads.clear() async def _flush_storage(self, ctx: Any) -> None: """Let a launched browser write site storage to disk before it closes: close its tabs with their unload @@ -750,6 +765,8 @@ def _on_context_closed(self, context: Any) -> None: self._snap = None self._focused = None self._download_pages.clear() + self._browser_cdp = None + self._attached_downloads.clear() if not self._closing: # the user closed the visible window (or their attached browser) self._spawn(self._publish_status()) @@ -865,9 +882,42 @@ async def _cleanup_download_tasks(self) -> None: finally: self._download_cleanup_task = None + def _on_cdp_download_will_begin(self, payload: dict[str, Any]) -> None: + guid = payload.get("guid") + if not guid: + return + entry = { + "guid": guid, + "frameId": payload.get("frameId", ""), + "url": payload.get("url", ""), + "filename": self._download_filename(payload.get("suggestedFilename") or "download"), + "future": asyncio.get_running_loop().create_future(), + "consumed": False, + } + self._attached_downloads.append(entry) + if len(self._attached_downloads) > 100: + self._attached_downloads = [e for e in self._attached_downloads if not e.get("consumed")][-50:] + + def _on_cdp_download_progress(self, payload: dict[str, Any]) -> None: + guid = payload.get("guid") + state = payload.get("state") + file_path = payload.get("filePath") + if not guid: + return + for entry in self._attached_downloads: + if entry["guid"] == guid: + if state == "completed" and file_path and not entry["future"].done(): + entry["future"].set_result(file_path) + elif state == "canceled" and not entry["future"].done(): + entry["future"].cancel() + break + @staticmethod def _download_filename(download: Any) -> str: - raw_name = str(getattr(download, "suggested_filename", None) or "download") + if isinstance(download, str): + raw_name = download + else: + raw_name = str(getattr(download, "suggested_filename", None) or "download") name = raw_name.replace("\\", "/").rsplit("/", 1)[-1] return re.sub(r"[^\w.\- ()]+", "_", name).strip(" .")[:160] or "download" @@ -923,7 +973,50 @@ def save_failure(exc: BaseException) -> str: try: async with timeout: target = await reserve_target() - await download.save_as(str(target)) + if self._attached: + matched = None + page = getattr(download, "page", None) + if page is not None and self._context is not None: + page_frames: set[str] = set() + try: + page_cdp = await self._context.new_cdp_session(page) + try: + tree = await page_cdp.send("Page.getFrameTree") + + def _collect_frame_ids(node: dict[str, Any]) -> set[str]: + fids = {node.get("frame", {}).get("id", "")} + for child in node.get("childFrames", []): + fids.update(_collect_frame_ids(child)) + return fids + + page_frames = _collect_frame_ids(tree.get("frameTree", {})) + finally: + with contextlib.suppress(Exception): + await page_cdp.detach() + except Exception: + pass + + if page_frames: + match_timeout = getattr(self, "_attached_match_timeout_s", ATTACHED_MATCH_TIMEOUT_S) + match_deadline = time.monotonic() + match_timeout + while True: + for entry in self._attached_downloads: + if entry.get("frameId") in page_frames and not entry.get("consumed"): + matched = entry + break + if matched is not None or time.monotonic() >= match_deadline: + break + await asyncio.sleep(0.05) + + if matched is not None: + matched["consumed"] = True + file_path = await matched["future"] + target.unlink(missing_ok=True) + shutil.move(file_path, str(target)) + else: + await download.save_as(str(target)) + else: + await download.save_as(str(target)) except asyncio.CancelledError as exc: try: cancel_error = await request_download_cancel() diff --git a/tests/browser/test_profiles.py b/tests/browser/test_profiles.py index 3bfbb52f..047c392a 100644 --- a/tests/browser/test_profiles.py +++ b/tests/browser/test_profiles.py @@ -6,6 +6,7 @@ import http.server import json import threading +import time from pathlib import Path import pytest @@ -645,6 +646,231 @@ async def new_page(self): assert nested_popup in svc._download_pages +async def test_attached_browser_resets_download_behavior_to_default(monkeypatch): + svc = BrowserService(make_app()) + cdp_calls = [] + + class MockCDP: + def on(self, event, handler): + pass + + async def send(self, command, params): + cdp_calls.append((command, params)) + + class MockBrowser: + contexts = [object()] + + def on(self, event, handler): + pass + + async def new_browser_cdp_session(self): + return MockCDP() + + class MockChromium: + async def connect_over_cdp(self, ws, timeout=None): + return MockBrowser() + + svc._pw = type("MockPW", (), {"chromium": MockChromium()})() + monkeypatch.setattr(safety, "devtools_endpoint", lambda ep: ("http://127.0.0.1:9222", None)) + async def _mock_ws(ep): + return "ws://127.0.0.1:9222" + monkeypatch.setattr(browser_service, "devtools_ws_url", _mock_ws) + + prof = BrowserProfileConfig(kind="attach", endpoint="http://127.0.0.1:9222") + await svc._attach(prof) + + assert svc._attached is True + assert cdp_calls == [("Browser.setDownloadBehavior", {"behavior": "default", "eventsEnabled": True})] + + +class _MockDownload: + def __init__(self, filename="report.pdf", url="https://example.com/report.pdf", page=None): + self.suggested_filename = filename + self.url = url + self.page = page + self.saved_to = None + + async def save_as(self, path): + self.saved_to = path + Path(path).write_text("saved via download.save_as", encoding="utf-8") + + async def cancel(self): + pass + + +class _MockPageCDP: + def __init__(self, frame_id="sentient-frame"): + self.frame_id = frame_id + self.detached = False + + async def send(self, command, params=None): + if command == "Page.getFrameTree": + return {"frameTree": {"frame": {"id": self.frame_id}, "childFrames": []}} + return {} + + async def detach(self): + self.detached = True + + +class _MockContext: + def __init__(self, cdp_session=None): + self.cdp_session = cdp_session + + async def new_cdp_session(self, page): + if isinstance(self.cdp_session, Exception): + raise self.cdp_session + return self.cdp_session + + +async def test_attached_download_matches_and_moves_valid_cdp_entry(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + svc._attached_match_timeout_s = 0.05 + page_cdp = _MockPageCDP("sentient-frame-1") + svc._context = _MockContext(page_cdp) + + src_file = tmp_path / "browser-download.pdf" + src_file.write_text("content from browser", encoding="utf-8") + + user_future = asyncio.get_running_loop().create_future() + user_entry = { + "guid": "g-user", + "frameId": "user-frame-99", + "filename": "report.pdf", + "url": "https://example.com/report.pdf", + "future": user_future, + "consumed": False, + } + sentient_future = asyncio.get_running_loop().create_future() + sentient_future.set_result(str(src_file)) + sentient_entry = { + "guid": "g-sentient", + "frameId": "sentient-frame-1", + "filename": "report.pdf", + "url": "https://example.com/report.pdf", + "future": sentient_future, + "consumed": False, + } + svc._attached_downloads.extend([user_entry, sentient_entry]) + + rel = await svc._save_download(_MockDownload("report.pdf", page=object())) + + assert rel == "downloads/report.pdf" + assert user_entry["consumed"] is False + assert not user_future.done() + assert sentient_entry["consumed"] is True + assert page_cdp.detached is True + saved_file = tmp_path / rel + assert saved_file.read_text(encoding="utf-8") == "content from browser" + + +async def test_user_tab_entry_never_consumed_even_when_sentient_cdp_arrives_late(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + svc._attached_match_timeout_s = 0.3 + page_cdp = _MockPageCDP("sentient-frame-1") + svc._context = _MockContext(page_cdp) + + src_file = tmp_path / "browser-download-late.pdf" + src_file.write_text("late content from browser", encoding="utf-8") + + user_future = asyncio.get_running_loop().create_future() + user_entry = { + "guid": "g-user", + "frameId": "user-frame-99", + "filename": "report.pdf", + "url": "https://example.com/report.pdf", + "future": user_future, + "consumed": False, + } + svc._attached_downloads.append(user_entry) + + task = asyncio.create_task(svc._save_download(_MockDownload("report.pdf", page=object()))) + await asyncio.sleep(0.05) + assert user_entry["consumed"] is False + + sentient_future = asyncio.get_running_loop().create_future() + sentient_future.set_result(str(src_file)) + sentient_entry = { + "guid": "g-sentient", + "frameId": "sentient-frame-1", + "filename": "report.pdf", + "url": "https://example.com/report.pdf", + "future": sentient_future, + "consumed": False, + } + svc._attached_downloads.append(sentient_entry) + + rel = await task + assert rel == "downloads/report.pdf" + assert user_entry["consumed"] is False + assert not user_future.done() + assert sentient_entry["consumed"] is True + assert (tmp_path / rel).read_text(encoding="utf-8") == "late content from browser" + + +async def test_frame_tree_lookup_failure_uses_safe_fallback_without_consuming_user_entry(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + svc._attached_match_timeout_s = 0.05 + svc._context = _MockContext(RuntimeError("CDP detached")) + + user_future = asyncio.get_running_loop().create_future() + user_entry = { + "guid": "g-user", + "frameId": "user-frame-99", + "filename": "report.pdf", + "url": "https://example.com/report.pdf", + "future": user_future, + "consumed": False, + } + svc._attached_downloads.append(user_entry) + + dl = _MockDownload("report.pdf", page=object()) + rel = await svc._save_download(dl) + + assert rel == "downloads/report.pdf" + assert user_entry["consumed"] is False + assert not user_future.done() + assert dl.saved_to is not None + assert (tmp_path / rel).read_text(encoding="utf-8") == "saved via download.save_as" + + +async def test_bounded_wait_terminates_when_no_matching_event_arrives(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + svc._attached_match_timeout_s = 0.05 + page_cdp = _MockPageCDP("sentient-frame-1") + svc._context = _MockContext(page_cdp) + + user_future = asyncio.get_running_loop().create_future() + user_entry = { + "guid": "g-user", + "frameId": "user-frame-99", + "filename": "report.pdf", + "url": "https://example.com/report.pdf", + "future": user_future, + "consumed": False, + } + svc._attached_downloads.append(user_entry) + + dl = _MockDownload("report.pdf", page=object()) + t0 = time.monotonic() + rel = await svc._save_download(dl) + elapsed = time.monotonic() - t0 + + assert rel == "downloads/report.pdf" + assert user_entry["consumed"] is False + assert not user_future.done() + assert dl.saved_to is not None + assert elapsed >= 0.04 + assert elapsed < 0.5 + + def test_edge_downloads_hub_is_ignored_and_active_page_is_restored(): svc = BrowserService(make_app()) diff --git a/tests/browser/test_profiles_live.py b/tests/browser/test_profiles_live.py index 6a6da6ae..e37541fb 100644 --- a/tests/browser/test_profiles_live.py +++ b/tests/browser/test_profiles_live.py @@ -4,6 +4,7 @@ import asyncio import contextlib +import json import subprocess import sys import time @@ -124,6 +125,20 @@ async def own_browser(tmp_path): if exe is None: pytest.skip("no Edge or Chrome installed") data = tmp_path / "user-browser" + user_downloads = tmp_path / "user-downloads" + user_downloads.mkdir(parents=True, exist_ok=True) + prefs_dir = data / "Default" + prefs_dir.mkdir(parents=True, exist_ok=True) + prefs_file = prefs_dir / "Preferences" + prefs_file.write_text( + json.dumps({ + "download": { + "default_directory": str(user_downloads.resolve()), + "prompt_for_download": False, + } + }), + encoding="utf-8", + ) args = [exe, "--headless=new", "--remote-debugging-port=0", f"--user-data-dir={data}", "--no-first-run", "--no-default-browser-check", "about:blank"] if sys.platform.startswith("linux"): @@ -152,7 +167,7 @@ async def own_browser(tmp_path): proc.wait(timeout=15) -async def test_attach_to_a_running_browser_and_detach_leaves_it_running(browser, site, own_browser): +async def test_attach_to_a_running_browser_and_detach_leaves_it_running(browser, site, own_browser, tmp_path): port = own_browser app = browser.app await browser.create_profile("mine", kind="attach", endpoint=f"localhost:{port}") @@ -165,15 +180,19 @@ async def test_attach_to_a_running_browser_and_detach_leaves_it_running(browser, assert status["attached"] and status["profile"] == "mine" and status["engine"] is None assert len(status["tabs"]) >= 2 # the user's own tab plus the one Sentient opened for itself - user_page = next(page for page in browser._context.pages if page is not browser._active) + user_page = next(page for page in browser._context.pages if page not in browser._download_pages) await user_page.goto(f"{site}/download.html") downloads_before = browser._download_tasks.copy() - async with user_page.expect_download(timeout=10_000) as download_info: - await user_page.get_by_role("link", name="Download report").click() - user_download = await download_info.value - assert user_download.suggested_filename == "report.txt" + await user_page.get_by_role("link", name="Download report").click() + downloaded_file = tmp_path / "user-downloads" / "report.txt" + deadline = time.monotonic() + 10 + while time.monotonic() < deadline and not ( + downloaded_file.exists() and not (tmp_path / "user-downloads" / "report.txt.crdownload").exists() + ): + await asyncio.sleep(0.1) + assert downloaded_file.exists() + assert downloaded_file.read_text(encoding="utf-8") == "Sentient browser download test.\n" assert browser._download_tasks == downloads_before - await user_download.cancel() download_page = await bt.browser_open.call(ctx, {"url": f"{site}/download.html", "profile": "mine"}) download_ref = next( From e98a93d67b06a5d53436163c95da668b74eb81bf Mon Sep 17 00:00:00 2001 From: Subodh Bansode Date: Sun, 11 Oct 2026 14:50:22 +0530 Subject: [PATCH 2/7] fix(browser): handle attached download shutdown and cancellation --- sentient/browser/service.py | 35 +++++-- tests/browser/test_profiles.py | 165 +++++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 7 deletions(-) diff --git a/sentient/browser/service.py b/sentient/browser/service.py index 9dc4cfb7..60d58f8e 100644 --- a/sentient/browser/service.py +++ b/sentient/browser/service.py @@ -671,7 +671,7 @@ async def _close_context(self) -> None: with contextlib.suppress(Exception): await self._browser_cdp.detach() self._browser_cdp = None - self._attached_downloads.clear() + self._clear_attached_downloads() async def _flush_storage(self, ctx: Any) -> None: """Let a launched browser write site storage to disk before it closes: close its tabs with their unload @@ -766,7 +766,7 @@ def _on_context_closed(self, context: Any) -> None: self._focused = None self._download_pages.clear() self._browser_cdp = None - self._attached_downloads.clear() + self._clear_attached_downloads() if not self._closing: # the user closed the visible window (or their attached browser) self._spawn(self._publish_status()) @@ -882,6 +882,17 @@ async def _cleanup_download_tasks(self) -> None: finally: self._download_cleanup_task = None + def _clear_attached_downloads(self, exc: Exception | None = None) -> None: + entries = list(self._attached_downloads) + self._attached_downloads.clear() + err = exc or BrowserError("The browser closed before the download completed.") + for entry in entries: + fut = entry.get("future") + if fut is not None and not fut.done(): + fut.set_exception(err) + with contextlib.suppress(Exception): + fut.exception() + def _on_cdp_download_will_begin(self, payload: dict[str, Any]) -> None: guid = payload.get("guid") if not guid: @@ -906,10 +917,15 @@ def _on_cdp_download_progress(self, payload: dict[str, Any]) -> None: return for entry in self._attached_downloads: if entry["guid"] == guid: - if state == "completed" and file_path and not entry["future"].done(): - entry["future"].set_result(file_path) - elif state == "canceled" and not entry["future"].done(): - entry["future"].cancel() + fut = entry.get("future") + if fut is not None and not fut.done(): + if state == "completed" and file_path: + fut.set_result(file_path) + elif state == "canceled": + fname = entry.get("filename") or "download" + fut.set_exception(BrowserError(f"Download '{fname}' was canceled by the browser.")) + with contextlib.suppress(Exception): + fut.exception() break @staticmethod @@ -968,6 +984,9 @@ def save_failure(exc: BaseException) -> str: reason = str(exc).strip().splitlines()[0][:300] or type(exc).__name__ return f"Couldn't save downloaded file '{name}': {reason}" + def abort_closed_browser() -> None: + raise BrowserError("The browser closed before the download completed.") + timeout = asyncio.timeout(DOWNLOAD_MAX_DURATION_S) target: Path | None = None try: @@ -1004,7 +1023,7 @@ def _collect_frame_ids(node: dict[str, Any]) -> set[str]: if entry.get("frameId") in page_frames and not entry.get("consumed"): matched = entry break - if matched is not None or time.monotonic() >= match_deadline: + if matched is not None or time.monotonic() >= match_deadline or self._context is None: break await asyncio.sleep(0.05) @@ -1013,6 +1032,8 @@ def _collect_frame_ids(node: dict[str, Any]) -> set[str]: file_path = await matched["future"] target.unlink(missing_ok=True) shutil.move(file_path, str(target)) + elif self._context is None: + abort_closed_browser() else: await download.save_as(str(target)) else: diff --git a/tests/browser/test_profiles.py b/tests/browser/test_profiles.py index 047c392a..fe905285 100644 --- a/tests/browser/test_profiles.py +++ b/tests/browser/test_profiles.py @@ -871,6 +871,171 @@ async def test_bounded_wait_terminates_when_no_matching_event_arrives(tmp_path, assert elapsed < 0.5 +async def test_closing_context_fails_pending_cdp_future_without_hanging(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + page_cdp = _MockPageCDP("sentient-frame-1") + ctx = _MockContext(page_cdp) + svc._context = ctx + + sentient_future = asyncio.get_running_loop().create_future() + sentient_entry = { + "guid": "g-pending", + "frameId": "sentient-frame-1", + "filename": "report.pdf", + "url": "https://example.com/report.pdf", + "future": sentient_future, + "consumed": False, + } + svc._attached_downloads.append(sentient_entry) + + save_task = asyncio.create_task(svc._save_download(_MockDownload("report.pdf", page=object()))) + await asyncio.sleep(0.01) + assert not save_task.done() + assert (tmp_path / "downloads" / "report.pdf").exists() + + # Trigger context closure via the actual context-closed event path + svc._on_context_closed(ctx) + + with pytest.raises(BrowserError) as exc_info: + await asyncio.wait_for(save_task, timeout=1.0) + assert "browser closed before the download completed" in str(exc_info.value) + assert not save_task.cancelled() + assert len(svc._attached_downloads) == 0 + assert sentient_future.done() + assert isinstance(sentient_future.exception(), BrowserError) + # Verify the reserved target file placeholder was cleaned up + assert not (tmp_path / "downloads" / "report.pdf").exists() + + +async def test_closing_context_during_match_wait_fails_without_hanging(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + page_cdp = _MockPageCDP("sentient-frame-1") + ctx = _MockContext(page_cdp) + svc._context = ctx + + # No entries in _attached_downloads yet, so it enters the while True matching wait loop + save_task = asyncio.create_task(svc._save_download(_MockDownload("report.pdf", page=object()))) + await asyncio.sleep(0.01) + assert not save_task.done() + assert (tmp_path / "downloads" / "report.pdf").exists() + + t0 = time.monotonic() + # Context closes while save_task is in the wait loop + svc._on_context_closed(ctx) + + with pytest.raises(BrowserError) as exc_info: + await asyncio.wait_for(save_task, timeout=1.0) + elapsed = time.monotonic() - t0 + + assert "browser closed before the download completed" in str(exc_info.value) + assert not save_task.cancelled() + assert elapsed < 0.5 + # Verify the reserved target file placeholder was cleaned up + assert not (tmp_path / "downloads" / "report.pdf").exists() + + +async def test_both_close_paths_are_idempotent_and_safe_for_futures(): + svc = BrowserService(make_app()) + svc._attached = True + loop = asyncio.get_running_loop() + + pending_fut = loop.create_future() + completed_fut = loop.create_future() + completed_fut.set_result("/tmp/done.pdf") + cancelled_fut = loop.create_future() + cancelled_fut.cancel() + failed_fut = loop.create_future() + failed_fut.set_exception(ValueError("original failure")) + + svc._attached_downloads.extend([ + {"guid": "g-pending", "frameId": "frame-1", "future": pending_fut, "consumed": False}, + {"guid": "g-done", "frameId": "frame-1", "future": completed_fut, "consumed": False}, + {"guid": "g-cancelled", "frameId": "frame-1", "future": cancelled_fut, "consumed": False}, + {"guid": "g-failed", "frameId": "frame-1", "future": failed_fut, "consumed": False}, + ]) + + # First close path + svc._clear_attached_downloads() + assert pending_fut.done() + assert isinstance(pending_fut.exception(), BrowserError) + # fut.exception() did not hide failure from subsequent await + with pytest.raises(BrowserError): + await pending_fut + # Completed, cancelled, and failed futures were not altered + assert completed_fut.result() == "/tmp/done.pdf" + assert cancelled_fut.cancelled() + assert isinstance(failed_fut.exception(), ValueError) + assert len(svc._attached_downloads) == 0 + + # Second close path on already cleared list is safe and idempotent + mock_ctx = object() + svc._context = mock_ctx + svc._on_context_closed(mock_ctx) + assert len(svc._attached_downloads) == 0 + assert isinstance(pending_fut.exception(), BrowserError) + + +async def test_cdp_canceled_event_reported_as_download_error_not_task_cancellation(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + page_cdp = _MockPageCDP("sentient-frame-1") + svc._context = _MockContext(page_cdp) + + sentient_future = asyncio.get_running_loop().create_future() + sentient_entry = { + "guid": "g-canceled", + "frameId": "sentient-frame-1", + "filename": "canceled_file.pdf", + "url": "https://example.com/file.pdf", + "future": sentient_future, + "consumed": False, + } + svc._attached_downloads.append(sentient_entry) + + save_task = asyncio.create_task(svc._save_download(_MockDownload("canceled_file.pdf", page=object()))) + await asyncio.sleep(0.01) + assert not save_task.done() + + svc._on_cdp_download_progress({"guid": "g-canceled", "state": "canceled"}) + + with pytest.raises(BrowserError) as exc_info: + await asyncio.wait_for(save_task, timeout=1.0) + assert "was canceled by the browser" in str(exc_info.value) + assert not save_task.cancelled() + + +async def test_genuine_save_download_task_cancellation_propagates(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + page_cdp = _MockPageCDP("sentient-frame-1") + svc._context = _MockContext(page_cdp) + + sentient_future = asyncio.get_running_loop().create_future() + sentient_entry = { + "guid": "g-stay-pending", + "frameId": "sentient-frame-1", + "filename": "report.pdf", + "future": sentient_future, + "consumed": False, + } + svc._attached_downloads.append(sentient_entry) + + save_task = asyncio.create_task(svc._save_download(_MockDownload("report.pdf", page=object()))) + await asyncio.sleep(0.01) + assert not save_task.done() + + save_task.cancel() + with pytest.raises(asyncio.CancelledError): + await save_task + assert save_task.cancelled() + + def test_edge_downloads_hub_is_ignored_and_active_page_is_restored(): svc = BrowserService(make_app()) From f8fe7447b85e5938c2858e3c71bb983d80bfbcce Mon Sep 17 00:00:00 2001 From: Subodh Bansode Date: Sun, 11 Oct 2026 16:44:23 +0530 Subject: [PATCH 3/7] fix(browser): prevent cross-matching attached downloads with conflicting URLs --- sentient/browser/service.py | 95 +++++++++- tests/browser/test_profiles.py | 280 +++++++++++++++++++++++++++- tests/browser/test_profiles_live.py | 4 +- 3 files changed, 367 insertions(+), 12 deletions(-) diff --git a/sentient/browser/service.py b/sentient/browser/service.py index 60d58f8e..578e2de5 100644 --- a/sentient/browser/service.py +++ b/sentient/browser/service.py @@ -883,6 +883,7 @@ async def _cleanup_download_tasks(self) -> None: self._download_cleanup_task = None def _clear_attached_downloads(self, exc: Exception | None = None) -> None: + """Safely fail and clear all tracked attached-browser CDP downloads during context shutdown.""" entries = list(self._attached_downloads) self._attached_downloads.clear() err = exc or BrowserError("The browser closed before the download completed.") @@ -893,7 +894,33 @@ def _clear_attached_downloads(self, exc: Exception | None = None) -> None: with contextlib.suppress(Exception): fut.exception() + def _prune_attached_downloads(self) -> None: + """Evict completed download records while keeping pending downloads reachable.""" + if len(self._attached_downloads) <= 100: + return + retained: list[dict[str, Any]] = [] + for entry in self._attached_downloads: + fut = entry.get("future") + if (fut is not None and not fut.done()) or not entry.get("consumed"): + retained.append(entry) + if len(retained) > 100: + pending = [e for e in retained if e.get("future") is not None and not e["future"].done()] + finished = [e for e in retained if e.get("future") is None or e["future"].done()] + max_finished = max(0, 100 - len(pending)) + retained = pending + finished[-max_finished:] + if len(retained) > 100: + to_evict = retained[:-100] + retained = retained[-100:] + for entry in to_evict: + fut = entry.get("future") + if fut is not None and not fut.done(): + fut.set_exception(BrowserError("Download record evicted.")) + with contextlib.suppress(Exception): + fut.exception() + self._attached_downloads = retained + def _on_cdp_download_will_begin(self, payload: dict[str, Any]) -> None: + """Record a CDP Browser.downloadWillBegin event to attribute downloads to frames.""" guid = payload.get("guid") if not guid: return @@ -906,10 +933,10 @@ def _on_cdp_download_will_begin(self, payload: dict[str, Any]) -> None: "consumed": False, } self._attached_downloads.append(entry) - if len(self._attached_downloads) > 100: - self._attached_downloads = [e for e in self._attached_downloads if not e.get("consumed")][-50:] + self._prune_attached_downloads() def _on_cdp_download_progress(self, payload: dict[str, Any]) -> None: + """Handle CDP Browser.downloadProgress events to complete or fail awaiting download tasks.""" guid = payload.get("guid") state = payload.get("state") file_path = payload.get("filePath") @@ -928,6 +955,53 @@ def _on_cdp_download_progress(self, payload: dict[str, Any]) -> None: fut.exception() break + @staticmethod + def _match_cdp_entry( + entries: list[dict[str, Any]], + page_frames: set[str], + dl_url: str, + dl_name: str, + ) -> dict[str, Any] | None: + """Find the best matching unconsumed CDP download entry for a given frame tree and download metadata.""" + best_entry: dict[str, Any] | None = None + best_score = 0 + for entry in entries: + if entry.get("frameId") not in page_frames or entry.get("consumed"): + continue + e_url = entry.get("url", "") + e_name = entry.get("filename", "") + + url_matches = bool(dl_url and e_url and dl_url == e_url) + name_matches = bool(dl_name and e_name and dl_name not in {"", "download"} and dl_name == e_name) + url_conflicts = bool(dl_url and e_url and dl_url != e_url) + name_conflicts = bool( + dl_name + and e_name + and dl_name not in {"", "download"} + and e_name not in {"", "download"} + and dl_name != e_name + ) + + if url_conflicts: + continue + if name_conflicts and not url_matches: + continue + + score = 0 + if url_matches and name_matches: + score = 3 + elif url_matches or name_matches: + score = 2 + elif not name_conflicts: + score = 1 + + if score > best_score: + best_score = score + best_entry = entry + if score == 3: + break + return best_entry + @staticmethod def _download_filename(download: Any) -> str: if isinstance(download, str): @@ -1018,11 +1092,11 @@ def _collect_frame_ids(node: dict[str, Any]) -> set[str]: if page_frames: match_timeout = getattr(self, "_attached_match_timeout_s", ATTACHED_MATCH_TIMEOUT_S) match_deadline = time.monotonic() + match_timeout + dl_url = str(getattr(download, "url", "") or "") while True: - for entry in self._attached_downloads: - if entry.get("frameId") in page_frames and not entry.get("consumed"): - matched = entry - break + matched = self._match_cdp_entry( + self._attached_downloads, page_frames, dl_url, name + ) if matched is not None or time.monotonic() >= match_deadline or self._context is None: break await asyncio.sleep(0.05) @@ -1030,8 +1104,13 @@ def _collect_frame_ids(node: dict[str, Any]) -> set[str]: if matched is not None: matched["consumed"] = True file_path = await matched["future"] - target.unlink(missing_ok=True) - shutil.move(file_path, str(target)) + + def _finalize_file() -> None: + assert target is not None + target.unlink(missing_ok=True) + shutil.move(file_path, str(target)) + + await asyncio.to_thread(_finalize_file) elif self._context is None: abort_closed_browser() else: diff --git a/tests/browser/test_profiles.py b/tests/browser/test_profiles.py index fe905285..9bdf60cd 100644 --- a/tests/browser/test_profiles.py +++ b/tests/browser/test_profiles.py @@ -649,10 +649,11 @@ async def new_page(self): async def test_attached_browser_resets_download_behavior_to_default(monkeypatch): svc = BrowserService(make_app()) cdp_calls = [] + cdp_handlers = {} class MockCDP: def on(self, event, handler): - pass + cdp_handlers[event] = handler async def send(self, command, params): cdp_calls.append((command, params)) @@ -680,7 +681,9 @@ async def _mock_ws(ep): await svc._attach(prof) assert svc._attached is True - assert cdp_calls == [("Browser.setDownloadBehavior", {"behavior": "default", "eventsEnabled": True})] + assert ("Browser.setDownloadBehavior", {"behavior": "default", "eventsEnabled": True}) in cdp_calls + assert "Browser.downloadWillBegin" in cdp_handlers + assert "Browser.downloadProgress" in cdp_handlers class _MockDownload: @@ -997,7 +1000,9 @@ async def test_cdp_canceled_event_reported_as_download_error_not_task_cancellati } svc._attached_downloads.append(sentient_entry) - save_task = asyncio.create_task(svc._save_download(_MockDownload("canceled_file.pdf", page=object()))) + save_task = asyncio.create_task( + svc._save_download(_MockDownload("canceled_file.pdf", url="https://example.com/file.pdf", page=object())) + ) await asyncio.sleep(0.01) assert not save_task.done() @@ -1036,6 +1041,275 @@ async def test_genuine_save_download_task_cancellation_propagates(tmp_path, monk assert save_task.cancelled() +async def test_cdp_download_event_handlers_lifecycle_and_edge_cases(monkeypatch): + svc = BrowserService(make_app()) + cdp_handlers = {} + + class MockCDP: + def on(self, event, handler): + cdp_handlers[event] = handler + + async def send(self, command, params): + pass + + class MockBrowser: + contexts = [object()] + + def on(self, event, handler): + pass + + async def new_browser_cdp_session(self): + return MockCDP() + + class MockChromium: + async def connect_over_cdp(self, ws, timeout=None): + return MockBrowser() + + svc._pw = type("MockPW", (), {"chromium": MockChromium()})() + async def _mock_ws(ep): + return "ws://127.0.0.1:9222" + monkeypatch.setattr(browser_service, "devtools_ws_url", _mock_ws) + + prof = BrowserProfileConfig(kind="attach", endpoint="http://127.0.0.1:9222") + await svc._attach(prof) + + on_begin = cdp_handlers["Browser.downloadWillBegin"] + on_progress = cdp_handlers["Browser.downloadProgress"] + + # 1. Missing GUID in downloadWillBegin is ignored + on_begin({"frameId": "f-1", "url": "https://example.com/a.pdf"}) + assert len(svc._attached_downloads) == 0 + + # 2. Valid GUID in downloadWillBegin records entry + on_begin({ + "guid": "g-1", + "frameId": "f-1", + "url": "https://example.com/a.pdf", + "suggestedFilename": "a.pdf", + }) + assert len(svc._attached_downloads) == 1 + entry = svc._attached_downloads[0] + assert entry["guid"] == "g-1" + assert entry["frameId"] == "f-1" + assert entry["filename"] == "a.pdf" + assert not entry["future"].done() + + # 3. downloadProgress with unknown or missing GUID is safely ignored + on_progress({"state": "completed", "filePath": "/tmp/a.pdf"}) + on_progress({"guid": "unknown-guid", "state": "completed", "filePath": "/tmp/a.pdf"}) + assert not entry["future"].done() + + # 4. downloadProgress completed sets result + on_progress({"guid": "g-1", "state": "completed", "filePath": "/tmp/a.pdf"}) + assert entry["future"].done() + assert entry["future"].result() == "/tmp/a.pdf" + + # 5. Subsequent progress on already completed future is safely ignored + on_progress({"guid": "g-1", "state": "completed", "filePath": "/tmp/other.pdf"}) + assert entry["future"].result() == "/tmp/a.pdf" + + # 6. downloadProgress canceled sets BrowserError + on_begin({"guid": "g-2", "frameId": "f-1", "suggestedFilename": "cancel.pdf"}) + entry2 = svc._attached_downloads[-1] + on_progress({"guid": "g-2", "state": "canceled"}) + assert entry2["future"].done() + assert isinstance(entry2["future"].exception(), BrowserError) + assert "was canceled by the browser" in str(entry2["future"].exception()) + + +async def test_attached_downloads_pruning_protects_pending_futures(): + svc = BrowserService(make_app()) + loop = asyncio.get_running_loop() + + # Add a pending download (e.g. Sentient download started early) + early_pending_fut = loop.create_future() + early_entry = { + "guid": "g-early", + "frameId": "sentient-frame", + "filename": "important.pdf", + "future": early_pending_fut, + "consumed": False, + } + svc._attached_downloads.append(early_entry) + + # Now simulate 120 completed user-tab downloads + for i in range(120): + done_fut = loop.create_future() + done_fut.set_result(f"/tmp/user_{i}.pdf") + svc._attached_downloads.append({ + "guid": f"g-user-{i}", + "frameId": "user-frame", + "filename": f"user_{i}.pdf", + "future": done_fut, + "consumed": False, + }) + svc._prune_attached_downloads() + + # The early pending future was NOT evicted and remains in _attached_downloads + assert any(e["guid"] == "g-early" for e in svc._attached_downloads) + assert not early_pending_fut.done() + + # Simulating progress completion works because entry was preserved + svc._on_cdp_download_progress({ + "guid": "g-early", + "state": "completed", + "filePath": "/tmp/important.pdf", + }) + assert early_pending_fut.done() + assert early_pending_fut.result() == "/tmp/important.pdf" + + +async def test_concurrent_downloads_in_same_frame_do_not_swap_files(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + page_cdp = _MockPageCDP("frame-same") + svc._context = _MockContext(page_cdp) + + file_a = tmp_path / "browser_download_a.pdf" + file_a.write_text("content-A", encoding="utf-8") + file_b = tmp_path / "browser_download_b.csv" + file_b.write_text("content-B", encoding="utf-8") + + # Reordered CDP events arrive: file B's event arrives BEFORE file A's event + fut_b = asyncio.get_running_loop().create_future() + fut_b.set_result(str(file_b)) + entry_b = { + "guid": "g-b", + "frameId": "frame-same", + "filename": "data.csv", + "url": "https://example.com/data.csv", + "future": fut_b, + "consumed": False, + } + svc._attached_downloads.append(entry_b) + + fut_a = asyncio.get_running_loop().create_future() + fut_a.set_result(str(file_a)) + entry_a = { + "guid": "g-a", + "frameId": "frame-same", + "filename": "report.pdf", + "url": "https://example.com/report.pdf", + "future": fut_a, + "consumed": False, + } + svc._attached_downloads.append(entry_a) + + # Save tasks run concurrently for download A and download B + dl_a = _MockDownload("report.pdf", url="https://example.com/report.pdf", page=object()) + dl_b = _MockDownload("data.csv", url="https://example.com/data.csv", page=object()) + + rel_a, rel_b = await asyncio.gather( + svc._save_download(dl_a), + svc._save_download(dl_b), + ) + + assert rel_a == "downloads/report.pdf" + assert rel_b == "downloads/data.csv" + assert (tmp_path / rel_a).read_text(encoding="utf-8") == "content-A" + assert (tmp_path / rel_b).read_text(encoding="utf-8") == "content-B" + + +async def test_concurrent_downloads_same_filename_different_urls_do_not_swap(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + page_cdp = _MockPageCDP("frame-same") + svc._context = _MockContext(page_cdp) + + file_v1 = tmp_path / "browser_v1_export.csv" + file_v1.write_text("v1-content", encoding="utf-8") + file_v2 = tmp_path / "browser_v2_export.csv" + file_v2.write_text("v2-content", encoding="utf-8") + + # Arrange CDP events so the v2 conflicting event arrives FIRST + fut_v2 = asyncio.get_running_loop().create_future() + fut_v2.set_result(str(file_v2)) + entry_v2 = { + "guid": "g-v2", + "frameId": "frame-same", + "filename": "export.csv", + "url": "https://example.com/api/v2/export.csv", + "future": fut_v2, + "consumed": False, + } + svc._attached_downloads.append(entry_v2) + + # v1 CDP event arrives slightly later + fut_v1 = asyncio.get_running_loop().create_future() + fut_v1.set_result(str(file_v1)) + entry_v1 = { + "guid": "g-v1", + "frameId": "frame-same", + "filename": "export.csv", + "url": "https://example.com/api/v1/export.csv", + "future": fut_v1, + "consumed": False, + } + + async def add_v1_later(): + await asyncio.sleep(0.05) + svc._attached_downloads.append(entry_v1) + + add_task = asyncio.create_task(add_v1_later()) + + # Task for v1 starts while only conflicting v2 entry is in _attached_downloads + dl_v1 = _MockDownload("export.csv", url="https://example.com/api/v1/export.csv", page=object()) + dl_v2 = _MockDownload("export.csv", url="https://example.com/api/v2/export.csv", page=object()) + + rel_v1, rel_v2, _ = await asyncio.gather( + svc._save_download(dl_v1), + svc._save_download(dl_v2), + add_task, + ) + + # Neither task adopted the other download's file + assert (tmp_path / rel_v1).read_text(encoding="utf-8") == "v1-content" + assert (tmp_path / rel_v2).read_text(encoding="utf-8") == "v2-content" + assert entry_v1["consumed"] is True + assert entry_v2["consumed"] is True + + +def test_match_cdp_entry_scoring_and_isolation(): + loop = asyncio.new_event_loop() + f1 = loop.create_future() + f2 = loop.create_future() + f3 = loop.create_future() + loop.close() + + entries = [ + {"frameId": "f-other", "url": "https://x.com/a.pdf", "filename": "a.pdf", "future": f1, "consumed": False}, + {"frameId": "f-mine", "url": "https://x.com/diff.zip", "filename": "diff.zip", "future": f2, "consumed": False}, + {"frameId": "f-mine", "url": "https://x.com/target.pdf", "filename": "target.pdf", "future": f3, "consumed": False}, + ] + + # Foreign frame is rejected + assert BrowserService._match_cdp_entry(entries, {"f-mine"}, "https://x.com/a.pdf", "a.pdf") is None + + # Exact match in page_frames is picked over conflicting entry + matched = BrowserService._match_cdp_entry(entries, {"f-mine"}, "https://x.com/target.pdf", "target.pdf") + assert matched is entries[2] + + # Conflicting URL is rejected even when filename matches + assert BrowserService._match_cdp_entry( + entries, {"f-mine"}, "https://x.com/other-url", "target.pdf" + ) is None + + # Rejection of conflicting entries when no matching entry exists + assert BrowserService._match_cdp_entry(entries, {"f-mine"}, "https://x.com/missing.txt", "missing.txt") is None + + # Missing URL metadata still matches by filename if no conflict + assert BrowserService._match_cdp_entry(entries, {"f-mine"}, "", "target.pdf") is entries[2] + + # Missing URL metadata does not match when filename conflicts + assert BrowserService._match_cdp_entry(entries, {"f-mine"}, "", "other.pdf") is None + + # Consumed entry is rejected + entries[2]["consumed"] = True + assert BrowserService._match_cdp_entry(entries, {"f-mine"}, "https://x.com/target.pdf", "target.pdf") is None + + def test_edge_downloads_hub_is_ignored_and_active_page_is_restored(): svc = BrowserService(make_app()) diff --git a/tests/browser/test_profiles_live.py b/tests/browser/test_profiles_live.py index e37541fb..17245c71 100644 --- a/tests/browser/test_profiles_live.py +++ b/tests/browser/test_profiles_live.py @@ -190,7 +190,9 @@ async def test_attach_to_a_running_browser_and_detach_leaves_it_running(browser, downloaded_file.exists() and not (tmp_path / "user-downloads" / "report.txt.crdownload").exists() ): await asyncio.sleep(0.1) - assert downloaded_file.exists() + folder = downloaded_file.parent + folder_contents = sorted(p.name for p in folder.iterdir()) if folder.exists() else [] + assert downloaded_file.exists(), f"download missing; folder has {folder_contents}" assert downloaded_file.read_text(encoding="utf-8") == "Sentient browser download test.\n" assert browser._download_tasks == downloads_before From 30feab29b4da1846a42a1e3c7b4385a856c66e57 Mon Sep 17 00:00:00 2001 From: Subodh Bansode Date: Sun, 11 Oct 2026 17:51:31 +0530 Subject: [PATCH 4/7] fix(browser): handle attached popup downloads safely --- CHANGELOG.md | 6 +- docs/API.md | 6 +- sentient/browser/service.py | 104 ++++++++++++++++++------ tests/browser/test_profiles.py | 119 ++++++++++++++++++++++++++-- tests/browser/test_profiles_live.py | 39 +++++++++ 5 files changed, 239 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afc5d882..af8394a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,8 +27,10 @@ and versions follow [Semantic Versioning](https://semver.org/). background work keep using your other models, and it can't be the memory search model. Anthropic may change how this is counted or allowed. - **Browser downloads:** files Sentient downloads while using a website are saved to `downloads/` in your Files - folder, and the browser result says where. In an attached browser, only downloads from Sentient's own tabs are - saved. + folder, and the browser result says where. In an attached browser, only downloads from Sentient's own tabs and + their popups (including `target="_blank"`) are saved, while downloads from other user tabs remain in the browser's + native Downloads folder. When attached, if a download cannot be matched to a confirmed browser download record, + it returns a clear error rather than falsely reporting success. - **Push to talk and dictation into any app:** hold Ctrl+Alt+Shift+T (Cmd+Option+Shift+T on a Mac) anywhere, speak, and let go: what you said goes to Sentient as a chat message, and the answer is read aloud. Press Ctrl+Alt+Shift+D, speak, and press it again (or just pause): your words are typed where your cursor is, in any app. A small bar at the diff --git a/docs/API.md b/docs/API.md index 3605fa06..e299cc6d 100644 --- a/docs/API.md +++ b/docs/API.md @@ -1448,8 +1448,10 @@ Every new tool declares a `Risk`; approvals behave as in section 1. profiles`. Every safety rule below applies the same in every profile, attached ones included. - Tools (plugin `browser`). Failures return `{error}` with a message the model can act on. - Downloads from launched profiles are accepted by Playwright; in an attached profile, only downloads from the tab - Sentient opens and popups opened from Sentient-owned tabs are handled (not the user's existing tabs). Every handled - download is saved under `downloads/` in Sentient's Files folder. Browser tool results (including `browser_scroll`) + Sentient opens and popups opened from Sentient-owned tabs (including `target="_blank"`, tracked via opener target discovery) + are handled (not the user's existing tabs). When attached, if no matching browser download record is identified, the + download fails with a clear error in `download_errors` rather than falsely reporting success. Every handled download + is saved under `downloads/` in Sentient's Files folder. Browser tool results (including `browser_scroll`) may include an optional `downloads` array of relative paths such as `["downloads/report.pdf"]`. `browser_open`, `browser_click`, `browser_type`, `browser_press`, `browser_select` and `browser_back` wait up to 0.3 s for a download to start during the action. Opening a URL that directly starts a download returns a normal diff --git a/sentient/browser/service.py b/sentient/browser/service.py index 578e2de5..47ba747b 100644 --- a/sentient/browser/service.py +++ b/sentient/browser/service.py @@ -242,6 +242,7 @@ def __init__(self, app): self._browser_cdp: Any = None self._attached_downloads: list[dict[str, Any]] = [] self._attached_match_timeout_s = ATTACHED_MATCH_TIMEOUT_S + self._target_openers: dict[str, tuple[str, ...]] = {} # ------------------------------------------------------------------ lifecycle async def start(self) -> None: @@ -606,12 +607,17 @@ async def _attach(self, prof: Any) -> Any: self._browser = browser self._attached = True self._engine = None - with contextlib.suppress(Exception): + try: cdp = await browser.new_browser_cdp_session() self._browser_cdp = cdp + cdp.on("Target.targetCreated", self._on_cdp_target_created) + cdp.on("Target.targetInfoChanged", self._on_cdp_target_created) cdp.on("Browser.downloadWillBegin", self._on_cdp_download_will_begin) cdp.on("Browser.downloadProgress", self._on_cdp_download_progress) + await cdp.send("Target.setDiscoverTargets", {"discover": True}) await cdp.send("Browser.setDownloadBehavior", {"behavior": "default", "eventsEnabled": True}) + except Exception: + pass return context async def _ensure(self, ctx: Any = None) -> Any: @@ -884,6 +890,7 @@ async def _cleanup_download_tasks(self) -> None: def _clear_attached_downloads(self, exc: Exception | None = None) -> None: """Safely fail and clear all tracked attached-browser CDP downloads during context shutdown.""" + self._target_openers.clear() entries = list(self._attached_downloads) self._attached_downloads.clear() err = exc or BrowserError("The browser closed before the download completed.") @@ -894,6 +901,31 @@ def _clear_attached_downloads(self, exc: Exception | None = None) -> None: with contextlib.suppress(Exception): fut.exception() + def _on_cdp_target_created(self, payload: dict[str, Any]) -> None: + """Record targetId -> opener associations so popup downloads can be linked to Sentient tabs.""" + target_info = payload.get("targetInfo", {}) + target_id = target_info.get("targetId") + opener_id = target_info.get("openerId") + opener_frame_id = target_info.get("openerFrameId") + openers = tuple(filter(None, (opener_id, opener_frame_id))) + if target_id and openers: + self._target_openers[target_id] = openers + if len(self._target_openers) > 500: + for k in list(self._target_openers.keys())[:100]: + self._target_openers.pop(k, None) + + def _expand_frames_with_popups(self, frames: set[str]) -> set[str]: + """Expand frame IDs to include popup targets opened by any frame in the tree.""" + expanded = set(frames) + added = True + while added: + added = False + for target_id, openers in list(self._target_openers.items()): + if target_id not in expanded and any(op in expanded for op in openers): + expanded.add(target_id) + added = True + return expanded + def _prune_attached_downloads(self) -> None: """Evict completed download records while keeping pending downloads reachable.""" if len(self._attached_downloads) <= 100: @@ -1061,6 +1093,9 @@ def save_failure(exc: BaseException) -> str: def abort_closed_browser() -> None: raise BrowserError("The browser closed before the download completed.") + def abort_missing_attached_download() -> None: + raise BrowserError(f"Download '{name}' could not be associated with a browser download record.") + timeout = asyncio.timeout(DOWNLOAD_MAX_DURATION_S) target: Path | None = None try: @@ -1069,30 +1104,39 @@ def abort_closed_browser() -> None: if self._attached: matched = None page = getattr(download, "page", None) - if page is not None and self._context is not None: - page_frames: set[str] = set() - try: - page_cdp = await self._context.new_cdp_session(page) + page_frames: set[str] = set() + dl_url = str(getattr(download, "url", "") or "") + if self._context is not None: + candidate_pages: list[Any] = [] + if page is not None and not getattr(page, "is_closed", lambda: False)(): + candidate_pages.append(page) + for p in self._download_pages: + if p not in candidate_pages and not getattr(p, "is_closed", lambda: False)(): + candidate_pages.append(p) + + for cand in candidate_pages: try: - tree = await page_cdp.send("Page.getFrameTree") - - def _collect_frame_ids(node: dict[str, Any]) -> set[str]: - fids = {node.get("frame", {}).get("id", "")} - for child in node.get("childFrames", []): - fids.update(_collect_frame_ids(child)) - return fids - - page_frames = _collect_frame_ids(tree.get("frameTree", {})) - finally: - with contextlib.suppress(Exception): - await page_cdp.detach() - except Exception: - pass + page_cdp = await self._context.new_cdp_session(cand) + try: + tree = await page_cdp.send("Page.getFrameTree") + + def _collect_frame_ids(node: dict[str, Any]) -> set[str]: + fids = {node.get("frame", {}).get("id", "")} + for child in node.get("childFrames", []): + fids.update(_collect_frame_ids(child)) + return fids + + page_frames.update(_collect_frame_ids(tree.get("frameTree", {}))) + finally: + with contextlib.suppress(Exception): + await page_cdp.detach() + except Exception: + pass if page_frames: + page_frames = self._expand_frames_with_popups(page_frames) match_timeout = getattr(self, "_attached_match_timeout_s", ATTACHED_MATCH_TIMEOUT_S) match_deadline = time.monotonic() + match_timeout - dl_url = str(getattr(download, "url", "") or "") while True: matched = self._match_cdp_entry( self._attached_downloads, page_frames, dl_url, name @@ -1107,14 +1151,28 @@ def _collect_frame_ids(node: dict[str, Any]) -> set[str]: def _finalize_file() -> None: assert target is not None - target.unlink(missing_ok=True) - shutil.move(file_path, str(target)) + try: + os.replace(file_path, str(target)) + except OSError: + target.unlink(missing_ok=True) + shutil.move(file_path, str(target)) await asyncio.to_thread(_finalize_file) elif self._context is None: abort_closed_browser() else: - await download.save_as(str(target)) + if page_frames: + for entry in self._attached_downloads: + if ( + entry.get("frameId") in page_frames + and not entry.get("consumed") + and ( + (dl_url and entry.get("url") == dl_url) + or (name and entry.get("filename") == name) + ) + ): + entry["consumed"] = True + abort_missing_attached_download() else: await download.save_as(str(target)) except asyncio.CancelledError as exc: diff --git a/tests/browser/test_profiles.py b/tests/browser/test_profiles.py index 9bdf60cd..f8aa800f 100644 --- a/tests/browser/test_profiles.py +++ b/tests/browser/test_profiles.py @@ -5,6 +5,7 @@ import asyncio import http.server import json +import os import threading import time from pathlib import Path @@ -814,7 +815,7 @@ async def test_user_tab_entry_never_consumed_even_when_sentient_cdp_arrives_late assert (tmp_path / rel).read_text(encoding="utf-8") == "late content from browser" -async def test_frame_tree_lookup_failure_uses_safe_fallback_without_consuming_user_entry(tmp_path, monkeypatch): +async def test_frame_tree_lookup_failure_raises_without_consuming_user_entry(tmp_path, monkeypatch): monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) svc = BrowserService(make_app()) svc._attached = True @@ -833,13 +834,13 @@ async def test_frame_tree_lookup_failure_uses_safe_fallback_without_consuming_us svc._attached_downloads.append(user_entry) dl = _MockDownload("report.pdf", page=object()) - rel = await svc._save_download(dl) + with pytest.raises(BrowserError, match="could not be associated with a browser download record"): + await svc._save_download(dl) - assert rel == "downloads/report.pdf" assert user_entry["consumed"] is False assert not user_future.done() - assert dl.saved_to is not None - assert (tmp_path / rel).read_text(encoding="utf-8") == "saved via download.save_as" + assert dl.saved_to is None + assert not (tmp_path / "downloads" / "report.pdf").exists() async def test_bounded_wait_terminates_when_no_matching_event_arrives(tmp_path, monkeypatch): @@ -863,13 +864,14 @@ async def test_bounded_wait_terminates_when_no_matching_event_arrives(tmp_path, dl = _MockDownload("report.pdf", page=object()) t0 = time.monotonic() - rel = await svc._save_download(dl) + with pytest.raises(BrowserError, match="could not be associated with a browser download record"): + await svc._save_download(dl) elapsed = time.monotonic() - t0 - assert rel == "downloads/report.pdf" assert user_entry["consumed"] is False assert not user_future.done() - assert dl.saved_to is not None + assert dl.saved_to is None + assert not (tmp_path / "downloads" / "report.pdf").exists() assert elapsed >= 0.04 assert elapsed < 0.5 @@ -1570,3 +1572,104 @@ async def save_download(download): assert not svc._download_tasks assert not svc._download_completed_at assert not svc._download_names + + +async def test_attached_popup_download_associated_via_target_opener(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + page_cdp = _MockPageCDP("opener-frame-1") + svc._context = _MockContext(page_cdp) + + # Record target opener mapping: popup target was opened by opener-frame-1 + svc._on_cdp_target_created({ + "targetInfo": { + "targetId": "popup-target-1", + "openerId": "opener-frame-1", + "openerFrameId": "opener-frame-1", + } + }) + + file_popup = tmp_path / "browser_download_popup.txt" + file_popup.write_text("popup download text", encoding="utf-8") + + fut = asyncio.get_running_loop().create_future() + fut.set_result(str(file_popup)) + entry = { + "guid": "g-popup", + "frameId": "popup-target-1", + "filename": "newtab.txt", + "url": "https://example.com/newtab.txt", + "future": fut, + "consumed": False, + } + svc._attached_downloads.append(entry) + + dl = _MockDownload("newtab.txt", url="https://example.com/newtab.txt", page=object()) + rel = await svc._save_download(dl) + + assert rel == "downloads/newtab.txt" + assert entry["consumed"] is True + assert (tmp_path / rel).read_text(encoding="utf-8") == "popup download text" + + +async def test_attached_download_timeout_marks_stale_records_consumed(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + svc._attached_match_timeout_s = 0.05 + page_cdp = _MockPageCDP("sentient-frame-1") + svc._context = _MockContext(page_cdp) + + stale_fut = asyncio.get_running_loop().create_future() + stale_entry = { + "guid": "g-stale", + "frameId": "sentient-frame-1", + "filename": "data.csv", + "url": "https://example.com/data.csv", + "future": stale_fut, + "consumed": False, + } + svc._attached_downloads.append(stale_entry) + + # Download with conflicting URL arrives, so stale_entry is rejected + dl = _MockDownload("data.csv", url="https://example.com/different-url.csv", page=object()) + with pytest.raises(BrowserError, match="could not be associated with a browser download record"): + await svc._save_download(dl) + + # After fallback, stale record in page_frames matching filename was marked consumed + assert stale_entry["consumed"] is True + + +async def test_attached_finalize_file_fallback_on_os_error(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + page_cdp = _MockPageCDP("f-1") + svc._context = _MockContext(page_cdp) + + src_file = tmp_path / "browser_file.txt" + src_file.write_text("file content", encoding="utf-8") + + fut = asyncio.get_running_loop().create_future() + fut.set_result(str(src_file)) + entry = { + "guid": "g-1", + "frameId": "f-1", + "filename": "test.txt", + "url": "https://example.com/test.txt", + "future": fut, + "consumed": False, + } + svc._attached_downloads.append(entry) + + def mock_replace(src, dst): + raise OSError("Cross-device link") + + monkeypatch.setattr(os, "replace", mock_replace) + + dl = _MockDownload("test.txt", url="https://example.com/test.txt", page=object()) + rel = await svc._save_download(dl) + + assert rel == "downloads/test.txt" + assert (tmp_path / rel).read_text(encoding="utf-8") == "file content" diff --git a/tests/browser/test_profiles_live.py b/tests/browser/test_profiles_live.py index 17245c71..5c05955b 100644 --- a/tests/browser/test_profiles_live.py +++ b/tests/browser/test_profiles_live.py @@ -252,3 +252,42 @@ async def test_writes_in_unload_handlers_survive_a_switch(browser, site): await bt.browser_open.call(ctx, {"url": f"{site}/help.html", "profile": "default"}) stored = await browser._active.evaluate("() => [localStorage.getItem('bye'), localStorage.getItem('hide')]") assert stored == ["before", "yes"] + + +async def test_attached_browser_target_blank_download_saves_non_zero_file(browser, site, own_browser): + port = own_browser + app = browser.app + await browser.create_profile("popup", kind="attach", endpoint=f"localhost:{port}") + ctx = make_ctx(app) + + snap_open = await bt.browser_open.call(ctx, {"url": f"{site}/download.html", "profile": "popup"}) + assert snap_open.get("profile") == "popup" + await browser._active.evaluate( + "() => {" + " const a = document.createElement('a');" + " a.href = '/direct-download';" + " a.target = '_blank';" + " a.textContent = 'Download popup report';" + " document.body.appendChild(a);" + "}" + ) + snap = await bt.browser_snapshot.call(ctx, {}) + ref = next( + line.split("]")[0][1:] + for line in snap["text"].splitlines() + if 'link "Download popup report"' in line + ) + res = await bt.browser_click.call(ctx, {"ref": ref}) + download_paths = res.get("downloads", []) + if not download_paths: + pending = [task for task in browser._download_tasks if not task.done()] + if pending: + await asyncio.wait_for(asyncio.gather(*pending), timeout=10) + later = await bt.browser_snapshot.call(ctx, {}) + download_paths = later.get("downloads", []) + assert download_paths == ["downloads/direct-report.txt"] + saved = paths.files_dir() / download_paths[0] + assert saved.exists() + assert saved.stat().st_size > 0 + assert saved.read_text(encoding="utf-8") == "Direct browser-open download test.\n" + assert "download_errors" not in res From 4a8d608dfc8f94e89c1290ff0487261b91b91cf9 Mon Sep 17 00:00:00 2001 From: Subodh Bansode Date: Sun, 11 Oct 2026 20:04:26 +0530 Subject: [PATCH 5/7] fix(browser): preserve concurrent download records on timeout --- sentient/browser/service.py | 11 ---------- tests/browser/test_profiles.py | 37 ++++++++++++++++++++++------------ 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/sentient/browser/service.py b/sentient/browser/service.py index 47ba747b..7b1d4675 100644 --- a/sentient/browser/service.py +++ b/sentient/browser/service.py @@ -1161,17 +1161,6 @@ def _finalize_file() -> None: elif self._context is None: abort_closed_browser() else: - if page_frames: - for entry in self._attached_downloads: - if ( - entry.get("frameId") in page_frames - and not entry.get("consumed") - and ( - (dl_url and entry.get("url") == dl_url) - or (name and entry.get("filename") == name) - ) - ): - entry["consumed"] = True abort_missing_attached_download() else: await download.save_as(str(target)) diff --git a/tests/browser/test_profiles.py b/tests/browser/test_profiles.py index f8aa800f..8265f68b 100644 --- a/tests/browser/test_profiles.py +++ b/tests/browser/test_profiles.py @@ -1613,7 +1613,7 @@ async def test_attached_popup_download_associated_via_target_opener(tmp_path, mo assert (tmp_path / rel).read_text(encoding="utf-8") == "popup download text" -async def test_attached_download_timeout_marks_stale_records_consumed(tmp_path, monkeypatch): +async def test_concurrent_same_filename_different_url_timeout_does_not_consume_active_record(tmp_path, monkeypatch): monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) svc = BrowserService(make_app()) svc._attached = True @@ -1621,24 +1621,35 @@ async def test_attached_download_timeout_marks_stale_records_consumed(tmp_path, page_cdp = _MockPageCDP("sentient-frame-1") svc._context = _MockContext(page_cdp) - stale_fut = asyncio.get_running_loop().create_future() - stale_entry = { - "guid": "g-stale", + file_second = tmp_path / "browser_download_second.csv" + file_second.write_text("second download content", encoding="utf-8") + + fut2 = asyncio.get_running_loop().create_future() + fut2.set_result(str(file_second)) + entry2 = { + "guid": "g-second", "frameId": "sentient-frame-1", - "filename": "data.csv", - "url": "https://example.com/data.csv", - "future": stale_fut, + "filename": "export.csv", + "url": "https://example.com/second.csv", + "future": fut2, "consumed": False, } - svc._attached_downloads.append(stale_entry) + svc._attached_downloads.append(entry2) - # Download with conflicting URL arrives, so stale_entry is rejected - dl = _MockDownload("data.csv", url="https://example.com/different-url.csv", page=object()) + # First download with conflicting URL arrives; its matching window expires + dl1 = _MockDownload("export.csv", url="https://example.com/first.csv", page=object()) with pytest.raises(BrowserError, match="could not be associated with a browser download record"): - await svc._save_download(dl) + await svc._save_download(dl1) + + # The second download's record must not have been consumed by the first download's fallback + assert entry2["consumed"] is False - # After fallback, stale record in page_frames matching filename was marked consumed - assert stale_entry["consumed"] is True + # Second download arrives and should match and save successfully + dl2 = _MockDownload("export.csv", url="https://example.com/second.csv", page=object()) + rel2 = await svc._save_download(dl2) + assert rel2 == "downloads/export.csv" + assert entry2["consumed"] is True + assert (tmp_path / rel2).read_text(encoding="utf-8") == "second download content" async def test_attached_finalize_file_fallback_on_os_error(tmp_path, monkeypatch): From 6a62987567e07bb7df82a7b20af7de9db78d2296 Mon Sep 17 00:00:00 2001 From: Subodh Bansode Date: Sun, 11 Oct 2026 21:06:48 +0530 Subject: [PATCH 6/7] fix(browser): fail attach cleanly when cdp download tracking fails --- sentient/browser/service.py | 64 ++++++++++++++-- tests/browser/test_profiles.py | 135 +++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 7 deletions(-) diff --git a/sentient/browser/service.py b/sentient/browser/service.py index 7b1d4675..3cb56891 100644 --- a/sentient/browser/service.py +++ b/sentient/browser/service.py @@ -201,6 +201,27 @@ def _friendly_playwright_error(exc: BaseException) -> str: return msg.splitlines()[0][:300] if msg else type(exc).__name__ +async def _shield_wait(aw: Any, timeout: float | None = None) -> None: + """Await a cleanup future or coroutine to completion even if the current task is cancelled.""" + fut = asyncio.ensure_future(aw) + loop = asyncio.get_running_loop() + end_time = (loop.time() + timeout) if timeout is not None else None + while not fut.done(): + remaining = (end_time - loop.time()) if end_time is not None else None + if remaining is not None and remaining <= 0: + fut.cancel() + break + try: + if remaining is not None: + await asyncio.wait_for(asyncio.shield(fut), timeout=remaining) + else: + await asyncio.shield(fut) + except asyncio.CancelledError: + pass + except BaseException: + break + + class BrowserService(Service): name = "browser" @@ -603,21 +624,50 @@ async def _attach(self, prof: Any) -> Any: self._last_error = f"Couldn't attach to the browser at {endpoint}: {str(exc).splitlines()[0][:200]}" raise BrowserError(self._last_error) from exc context = browser.contexts[0] if browser.contexts else await browser.new_context() - browser.on("disconnected", lambda *_: self._on_context_closed(context)) - self._browser = browser - self._attached = True - self._engine = None + cdp = None try: cdp = await browser.new_browser_cdp_session() - self._browser_cdp = cdp cdp.on("Target.targetCreated", self._on_cdp_target_created) cdp.on("Target.targetInfoChanged", self._on_cdp_target_created) cdp.on("Browser.downloadWillBegin", self._on_cdp_download_will_begin) cdp.on("Browser.downloadProgress", self._on_cdp_download_progress) await cdp.send("Target.setDiscoverTargets", {"discover": True}) await cdp.send("Browser.setDownloadBehavior", {"behavior": "default", "eventsEnabled": True}) - except Exception: - pass + except BaseException as exc: + if cdp is not None: + detach_fn = getattr(cdp, "detach", None) + if callable(detach_fn): + try: + res = detach_fn() + if asyncio.iscoroutine(res) or isinstance(res, asyncio.Future): + await _shield_wait(res) + except BaseException: + pass + self._browser_cdp = None + self._clear_attached_downloads() + self._attached = False + self._browser = None + close_fn = getattr(browser, "close", None) + if callable(close_fn): + try: + res = close_fn() + if asyncio.iscoroutine(res) or isinstance(res, asyncio.Future): + await _shield_wait(res, timeout=CLOSE_TIMEOUT_S) + except BaseException: + pass + if isinstance(exc, asyncio.CancelledError) or not isinstance(exc, Exception): + raise + self._last_error = ( + f"Couldn't attach to the browser at {endpoint}: " + f"failed to set up download tracking: {str(exc).splitlines()[0][:200]}" + ) + raise BrowserError(self._last_error) from exc + + browser.on("disconnected", lambda *_: self._on_context_closed(context)) + self._browser = browser + self._browser_cdp = cdp + self._attached = True + self._engine = None return context async def _ensure(self, ctx: Any = None) -> Any: diff --git a/tests/browser/test_profiles.py b/tests/browser/test_profiles.py index 8265f68b..03ed6d87 100644 --- a/tests/browser/test_profiles.py +++ b/tests/browser/test_profiles.py @@ -687,6 +687,141 @@ async def _mock_ws(ep): assert "Browser.downloadProgress" in cdp_handlers +@pytest.mark.parametrize( + ("fail_stage", "exc_to_raise", "expected_err_type", "expected_match"), + [ + ( + "session", + RuntimeError("session creation failed"), + BrowserError, + "failed to set up download tracking.*session creation failed", + ), + ( + "send", + RuntimeError("setDownloadBehavior failed"), + BrowserError, + "failed to set up download tracking.*setDownloadBehavior failed", + ), + ("session", asyncio.CancelledError(), asyncio.CancelledError, None), + ("send", asyncio.CancelledError(), asyncio.CancelledError, None), + ], +) +async def test_attached_browser_cdp_setup_failure_and_cancellation_cleanup( + monkeypatch, fail_stage, exc_to_raise, expected_err_type, expected_match +): + svc = BrowserService(make_app()) + closed = [] + detached = [] + + class MockCDP: + def on(self, event, handler): + pass + + async def send(self, command, params): + if fail_stage == "send" and command == "Browser.setDownloadBehavior": + raise exc_to_raise + + async def detach(self): + await asyncio.sleep(0.01) + detached.append(True) + + class MockBrowser: + contexts = [object()] + + def on(self, event, handler): + pass + + async def new_browser_cdp_session(self): + if fail_stage == "session": + raise exc_to_raise + return MockCDP() + + async def close(self): + await asyncio.sleep(0.01) + closed.append(True) + + class MockChromium: + async def connect_over_cdp(self, ws, timeout=None): + return MockBrowser() + + svc._pw = type("MockPW", (), {"chromium": MockChromium()})() + monkeypatch.setattr(safety, "devtools_endpoint", lambda ep: ("http://127.0.0.1:9222", None)) + + async def _mock_ws(ep): + return "ws://127.0.0.1:9222" + + monkeypatch.setattr(browser_service, "devtools_ws_url", _mock_ws) + + prof = BrowserProfileConfig(kind="attach", endpoint="http://127.0.0.1:9222") + + if expected_match: + with pytest.raises(expected_err_type, match=expected_match): + await svc._attach(prof) + else: + with pytest.raises(expected_err_type): + await svc._attach(prof) + + assert svc._attached is False + assert svc._browser is None + assert svc._browser_cdp is None + assert closed == [True] + if fail_stage == "send": + assert detached == [True] + else: + assert detached == [] + assert len(svc._attached_downloads) == 0 + if isinstance(exc_to_raise, asyncio.CancelledError): + assert svc._last_error is None + else: + assert svc._last_error is not None + + +async def test_attached_browser_cleanup_error_does_not_mask_original_error(monkeypatch): + svc = BrowserService(make_app()) + + class MockCDP: + def on(self, event, handler): + pass + + async def send(self, command, params): + raise RuntimeError("original send failure") + + async def detach(self): + raise RuntimeError("detach error") + + class MockBrowser: + contexts = [object()] + + def on(self, event, handler): + pass + + async def new_browser_cdp_session(self): + return MockCDP() + + async def close(self): + raise RuntimeError("close error") + + class MockChromium: + async def connect_over_cdp(self, ws, timeout=None): + return MockBrowser() + + svc._pw = type("MockPW", (), {"chromium": MockChromium()})() + monkeypatch.setattr(safety, "devtools_endpoint", lambda ep: ("http://127.0.0.1:9222", None)) + + async def _mock_ws(ep): + return "ws://127.0.0.1:9222" + + monkeypatch.setattr(browser_service, "devtools_ws_url", _mock_ws) + + prof = BrowserProfileConfig(kind="attach", endpoint="http://127.0.0.1:9222") + with pytest.raises(BrowserError, match=r"failed to set up download tracking.*original send failure"): + await svc._attach(prof) + + assert svc._attached is False + assert svc._browser is None + assert svc._browser_cdp is None + + class _MockDownload: def __init__(self, filename="report.pdf", url="https://example.com/report.pdf", page=None): self.suggested_filename = filename From 00a2a2647aac882fc105caaa12fbd340f5f2d705 Mon Sep 17 00:00:00 2001 From: Subodh Bansode Date: Mon, 12 Oct 2026 00:37:50 +0530 Subject: [PATCH 7/7] fix(browser): handle attached popup downloads safely --- CHANGELOG.md | 2 +- docs/API.md | 2 +- sentient/browser/service.py | 80 +++++++++++---- tests/browser/test_profiles.py | 182 +++++++++++++++++++++++++++++++++ 4 files changed, 244 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af8394a3..d128a9ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,7 @@ and versions follow [Semantic Versioning](https://semver.org/). - **Browser downloads:** files Sentient downloads while using a website are saved to `downloads/` in your Files folder, and the browser result says where. In an attached browser, only downloads from Sentient's own tabs and their popups (including `target="_blank"`) are saved, while downloads from other user tabs remain in the browser's - native Downloads folder. When attached, if a download cannot be matched to a confirmed browser download record, + native Downloads folder. Sentient's own downloads briefly pass through the browser's download folder before being moved to the downloads directory. When attached, if a download cannot be matched to a confirmed browser download record, it returns a clear error rather than falsely reporting success. - **Push to talk and dictation into any app:** hold Ctrl+Alt+Shift+T (Cmd+Option+Shift+T on a Mac) anywhere, speak, and let go: what you said goes to Sentient as a chat message, and the answer is read aloud. Press Ctrl+Alt+Shift+D, diff --git a/docs/API.md b/docs/API.md index e299cc6d..63e941fc 100644 --- a/docs/API.md +++ b/docs/API.md @@ -1449,7 +1449,7 @@ Every new tool declares a `Risk`; approvals behave as in section 1. - Tools (plugin `browser`). Failures return `{error}` with a message the model can act on. - Downloads from launched profiles are accepted by Playwright; in an attached profile, only downloads from the tab Sentient opens and popups opened from Sentient-owned tabs (including `target="_blank"`, tracked via opener target discovery) - are handled (not the user's existing tabs). When attached, if no matching browser download record is identified, the + are handled (not the user's existing tabs). Sentient's own downloads briefly pass through the browser's download folder before being moved to the downloads directory. When attached, if no matching browser download record is identified, the download fails with a clear error in `download_errors` rather than falsely reporting success. Every handled download is saved under `downloads/` in Sentient's Files folder. Browser tool results (including `browser_scroll`) may include an optional `downloads` array of relative paths such as `["downloads/report.pdf"]`. diff --git a/sentient/browser/service.py b/sentient/browser/service.py index 3cb56891..79a157fc 100644 --- a/sentient/browser/service.py +++ b/sentient/browser/service.py @@ -70,6 +70,7 @@ DOWNLOAD_TASK_RETENTION_S = 300.0 DOWNLOAD_TASK_CLEANUP_INTERVAL_S = 30.0 ATTACHED_MATCH_TIMEOUT_S = 2.0 +ATTACHED_UNCONSUMED_TTL_S = 30.0 # models often pass the whole snapshot line ("[e4] button \"Place order\"") instead of just "e4" _REF_RE = re.compile(r"\b(e\d+)\b", re.IGNORECASE) @@ -977,28 +978,59 @@ def _expand_frames_with_popups(self, frames: set[str]) -> set[str]: return expanded def _prune_attached_downloads(self) -> None: - """Evict completed download records while keeping pending downloads reachable.""" - if len(self._attached_downloads) <= 100: - return + """Evict completed download records while keeping active in-flight downloads reachable.""" + now = time.monotonic() retained: list[dict[str, Any]] = [] for entry in self._attached_downloads: fut = entry.get("future") - if (fut is not None and not fut.done()) or not entry.get("consumed"): + is_pending = fut is not None and not fut.done() + consumed = entry.get("consumed", False) + created_at = entry.get("created_at") + + # Active in-flight download: consumed and awaiting completion + if consumed and is_pending: + retained.append(entry) + continue + + # Unconsumed record: keep while within TTL + if not consumed: + if created_at is None or (now - created_at) <= ATTACHED_UNCONSUMED_TTL_S: + retained.append(entry) + else: + if is_pending: + fut.set_exception(BrowserError("Download record expired without matching.")) + with contextlib.suppress(Exception): + fut.exception() + continue + + # Finished consumed record: retain recent for short retention + if created_at is None or (now - created_at) <= 10.0: retained.append(entry) + if len(retained) > 100: - pending = [e for e in retained if e.get("future") is not None and not e["future"].done()] + inflight = [e for e in retained if e.get("consumed") and e.get("future") is not None and not e["future"].done()] + unconsumed_pending = [e for e in retained if not e.get("consumed") and e.get("future") is not None and not e["future"].done()] finished = [e for e in retained if e.get("future") is None or e["future"].done()] - max_finished = max(0, 100 - len(pending)) - retained = pending + finished[-max_finished:] - if len(retained) > 100: - to_evict = retained[:-100] - retained = retained[-100:] - for entry in to_evict: + + keep_inflight = inflight[-100:] + evict_inflight = inflight[:-100] + + remaining_slots = max(0, 100 - len(keep_inflight)) + keep_unconsumed = unconsumed_pending[-remaining_slots:] if remaining_slots > 0 else [] + evict_unconsumed = [e for e in unconsumed_pending if e not in keep_unconsumed] + remaining_slots -= len(keep_unconsumed) + + keep_finished = finished[-remaining_slots:] if remaining_slots > 0 else [] + + for entry in evict_inflight + evict_unconsumed: fut = entry.get("future") if fut is not None and not fut.done(): fut.set_exception(BrowserError("Download record evicted.")) with contextlib.suppress(Exception): fut.exception() + + kept_ids = {id(e) for e in keep_inflight} | {id(e) for e in keep_unconsumed} | {id(e) for e in keep_finished} + retained = [e for e in retained if id(e) in kept_ids] self._attached_downloads = retained def _on_cdp_download_will_begin(self, payload: dict[str, Any]) -> None: @@ -1013,6 +1045,7 @@ def _on_cdp_download_will_begin(self, payload: dict[str, Any]) -> None: "filename": self._download_filename(payload.get("suggestedFilename") or "download"), "future": asyncio.get_running_loop().create_future(), "consumed": False, + "created_at": time.monotonic(), } self._attached_downloads.append(entry) self._prune_attached_downloads() @@ -1043,19 +1076,27 @@ def _match_cdp_entry( page_frames: set[str], dl_url: str, dl_name: str, + now: float | None = None, ) -> dict[str, Any] | None: """Find the best matching unconsumed CDP download entry for a given frame tree and download metadata.""" best_entry: dict[str, Any] | None = None best_score = 0 + current_time = now if now is not None else time.monotonic() for entry in entries: if entry.get("frameId") not in page_frames or entry.get("consumed"): continue + created_at = entry.get("created_at") + if created_at is not None and (current_time - created_at) > ATTACHED_UNCONSUMED_TTL_S: + continue e_url = entry.get("url", "") e_name = entry.get("filename", "") + # Strict URL agreement when both are available + if dl_url and e_url and dl_url != e_url: + continue + url_matches = bool(dl_url and e_url and dl_url == e_url) name_matches = bool(dl_name and e_name and dl_name not in {"", "download"} and dl_name == e_name) - url_conflicts = bool(dl_url and e_url and dl_url != e_url) name_conflicts = bool( dl_name and e_name @@ -1064,15 +1105,13 @@ def _match_cdp_entry( and dl_name != e_name ) - if url_conflicts: - continue if name_conflicts and not url_matches: continue score = 0 if url_matches and name_matches: score = 3 - elif url_matches or name_matches: + elif url_matches or (name_matches and not (dl_url and e_url)): score = 2 elif not name_conflicts: score = 1 @@ -1160,9 +1199,10 @@ def abort_missing_attached_download() -> None: candidate_pages: list[Any] = [] if page is not None and not getattr(page, "is_closed", lambda: False)(): candidate_pages.append(page) - for p in self._download_pages: - if p not in candidate_pages and not getattr(p, "is_closed", lambda: False)(): - candidate_pages.append(p) + else: + for p in self._download_pages: + if not getattr(p, "is_closed", lambda: False)(): + candidate_pages.append(p) for cand in candidate_pages: try: @@ -1184,12 +1224,12 @@ def _collect_frame_ids(node: dict[str, Any]) -> set[str]: pass if page_frames: - page_frames = self._expand_frames_with_popups(page_frames) match_timeout = getattr(self, "_attached_match_timeout_s", ATTACHED_MATCH_TIMEOUT_S) match_deadline = time.monotonic() + match_timeout while True: + active_frames = self._expand_frames_with_popups(page_frames) matched = self._match_cdp_entry( - self._attached_downloads, page_frames, dl_url, name + self._attached_downloads, active_frames, dl_url, name ) if matched is not None or time.monotonic() >= match_deadline or self._context is None: break diff --git a/tests/browser/test_profiles.py b/tests/browser/test_profiles.py index 03ed6d87..4f4e41ae 100644 --- a/tests/browser/test_profiles.py +++ b/tests/browser/test_profiles.py @@ -1819,3 +1819,185 @@ def mock_replace(src, dst): assert rel == "downloads/test.txt" assert (tmp_path / rel).read_text(encoding="utf-8") == "file content" + + +async def test_attached_popup_target_created_after_matching_starts_is_discovered(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + page_cdp = _MockPageCDP("opener-frame-1") + svc._context = _MockContext(page_cdp) + + file_popup = tmp_path / "browser_download_late_popup.txt" + file_popup.write_text("late popup text", encoding="utf-8") + + fut = asyncio.get_running_loop().create_future() + fut.set_result(str(file_popup)) + entry = { + "guid": "g-late-popup", + "frameId": "popup-target-1", + "filename": "late_popup.txt", + "url": "https://example.com/late_popup.txt", + "future": fut, + "consumed": False, + } + svc._attached_downloads.append(entry) + + opener_page = type("Page", (), {"is_closed": lambda self: False, "main_frame": object()})() + dl = _MockDownload("late_popup.txt", url="https://example.com/late_popup.txt", page=opener_page) + + async def late_target_created(): + await asyncio.sleep(0.06) + svc._on_cdp_target_created({ + "targetInfo": { + "targetId": "popup-target-1", + "openerId": "opener-frame-1", + "openerFrameId": "opener-frame-1", + } + }) + + target_task = asyncio.create_task(late_target_created()) + rel = await svc._save_download(dl) + await target_task + + assert rel == "downloads/late_popup.txt" + assert entry["consumed"] is True + assert (tmp_path / rel).read_text(encoding="utf-8") == "late popup text" + + +async def test_attached_unmatched_download_cleans_up_reservation_and_reports_error(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + svc._attached_match_timeout_s = 0.05 + page_cdp = _MockPageCDP("sentient-frame-1") + svc._context = _MockContext(page_cdp) + + dl = _MockDownload("unmatched.pdf", url="https://example.com/unmatched.pdf", page=object()) + + svc._on_download(dl) + assert len(svc._download_tasks) == 1 + task = next(iter(svc._download_tasks)) + + with pytest.raises(BrowserError, match="could not be associated with a browser download record"): + await task + + # Reserved file must be cleaned up + assert not (tmp_path / "downloads" / "unmatched.pdf").exists() + + result = await svc._include_downloads({"ok": True}, set()) + assert result.get("ok") is True + assert "downloads" not in result + assert result.get("download_errors") == [ + "Download 'unmatched.pdf' could not be associated with a browser download record." + ] + + +async def test_stale_unconsumed_cdp_records_are_not_matched_and_get_evicted(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + loop = asyncio.get_running_loop() + + fut = loop.create_future() + stale_entry = { + "guid": "g-stale", + "frameId": "frame-1", + "filename": "old.pdf", + "url": "https://example.com/old.pdf", + "future": fut, + "consumed": False, + "created_at": time.monotonic() - 35.0, + } + svc._attached_downloads.append(stale_entry) + + matched = BrowserService._match_cdp_entry(svc._attached_downloads, {"frame-1"}, "https://example.com/old.pdf", "old.pdf") + assert matched is None + + svc._prune_attached_downloads() + assert stale_entry not in svc._attached_downloads + assert fut.done() + assert isinstance(fut.exception(), BrowserError) + + +async def test_prune_attached_downloads_preserves_inflight_and_evicts_overflow_promptly(): + svc = BrowserService(make_app()) + loop = asyncio.get_running_loop() + + inflight_fut = loop.create_future() + inflight_entry = { + "guid": "g-inflight", + "frameId": "frame-1", + "filename": "inflight.pdf", + "url": "https://example.com/inflight.pdf", + "future": inflight_fut, + "consumed": True, + "created_at": time.monotonic(), + } + svc._attached_downloads.append(inflight_entry) + + overflow_futs = [] + for i in range(120): + f = loop.create_future() + overflow_futs.append(f) + svc._attached_downloads.append({ + "guid": f"g-{i}", + "frameId": "frame-other", + "filename": f"file_{i}.pdf", + "url": f"https://example.com/file_{i}.pdf", + "future": f, + "consumed": False, + "created_at": time.monotonic(), + }) + + svc._prune_attached_downloads() + + assert inflight_entry in svc._attached_downloads + assert not inflight_fut.done() + + evicted_futs = [f for f in overflow_futs if f.done()] + assert len(evicted_futs) > 0 + for f in evicted_futs: + assert isinstance(f.exception(), BrowserError) + assert "evicted" in str(f.exception()) + + +async def test_candidate_pages_scoped_to_download_page_when_valid(tmp_path, monkeypatch): + monkeypatch.setattr(paths, "files_dir", lambda: tmp_path) + svc = BrowserService(make_app()) + svc._attached = True + svc._attached_match_timeout_s = 0.05 + + other_page = type("Page", (), {"is_closed": lambda self: False, "main_frame": object()})() + dl_page = type("Page", (), {"is_closed": lambda self: False, "main_frame": object()})() + svc._download_pages.update((other_page, dl_page)) + + queried_pages = [] + + class MockContextWithSpy: + async def new_cdp_session(self, page): + queried_pages.append(page) + return _MockPageCDP("dl-frame") + + svc._context = MockContextWithSpy() + + file_test = tmp_path / "browser_download_test.txt" + file_test.write_text("scoped test", encoding="utf-8") + fut = asyncio.get_running_loop().create_future() + fut.set_result(str(file_test)) + + entry = { + "guid": "g-scoped", + "frameId": "dl-frame", + "filename": "scoped.txt", + "url": "https://example.com/scoped.txt", + "future": fut, + "consumed": False, + } + svc._attached_downloads.append(entry) + + dl = _MockDownload("scoped.txt", url="https://example.com/scoped.txt", page=dl_page) + rel = await svc._save_download(dl) + + assert rel == "downloads/scoped.txt" + assert queried_pages == [dl_page]