From a7a84044d8f2bd027929f880cc4b299262291478 Mon Sep 17 00:00:00 2001 From: ahmad-ajmal Date: Thu, 22 Oct 2026 17:45:00 +0100 Subject: [PATCH] fix: durability fixes --- agent_core/core/hooks/usage.py | 121 +++++ agent_core/core/impl/image_gen/interface.py | 13 +- agent_core/core/impl/llm/interface.py | 14 +- agent_core/core/impl/video_gen/interface.py | 9 +- agent_core/core/impl/vlm/interface.py | 66 ++- agent_core/core/models/output.py | 51 ++ app/agent_app/manager.py | 123 +++-- app/agent_app/op_smoke.py | 4 + app/agent_app/test_data_safety.py | 8 +- app/agent_base.py | 6 +- app/config/settings.json | 2 +- app/console.py | 19 + app/data/action/render_ui.py | 9 +- app/generative_ui.py | 41 ++ app/mini_browser/core.py | 42 +- app/triggers/runtime.py | 101 ++-- app/ui_layer/adapters/base.py | 44 +- app/ui_layer/adapters/browser_adapter.py | 12 +- .../src/components/Chat/QuestionBox.tsx | 7 +- .../components/GenerativeUI/GenerativeUI.tsx | 15 +- .../src/components/GenerativeUI/runtime.ts | 63 +++ .../src/store/slices/messagesSlice.ts | 34 +- .../frontend/src/store/slices/uiSlice.ts | 6 +- .../frontend/src/store/uiState/catalog.ts | 7 +- .../src/store/uiState/defineUiState.ts | 12 +- .../src/store/uiState/legacyMigration.ts | 17 + .../frontend/src/store/uiState/persistence.ts | 18 +- .../browser/frontend/src/types/index.ts | 1 + .../frontend/tests/agent-app-media.html | 29 ++ .../frontend/tests/agent-app-media.test.ts | 29 ++ .../frontend/tests/generative-ui.test.ts | 259 +++++++++- .../browser/frontend/tests/harness.tsx | 22 +- .../frontend/tests/question-queue.test.ts | 53 ++ app/ui_layer/commands/builtin/clear.py | 15 +- app/ui_layer/controller/ui_controller.py | 3 + app/ui_layer/events/event_bus.py | 37 +- app/usage/reporter.py | 35 +- craftbot.py | 10 +- main.py | 3 + pytest.ini | 4 +- run.py | 9 +- tests/conftest.py | 48 +- tests/integrations/test_import_surface.py | 4 +- tests/integrations/test_provider_listeners.py | 5 +- tests/qa_runtime.py | 72 +++ tests/test_app_readiness.py | 94 ++++ tests/test_craftbot_service.py | 2 +- tests/test_generative_ui.py | 33 ++ tests/test_installer_stream_routing.py | 109 ----- tests/test_mini_browser_fleet.py | 87 ++++ tests/test_prompt_profile.py | 51 +- tests/test_qa_runtime.py | 34 ++ tests/test_question_dismissal.py | 156 ++++++ tests/test_session_clear.py | 458 +++++++++++++++++- tests/test_state_sync.py | 7 +- tests/test_usage_delivery.py | 178 +++++++ tests/test_vlm_regressions.py | 292 +++++++++++ 57 files changed, 2623 insertions(+), 380 deletions(-) create mode 100644 agent_core/core/hooks/usage.py create mode 100644 agent_core/core/models/output.py create mode 100644 app/console.py create mode 100644 app/ui_layer/browser/frontend/tests/agent-app-media.html create mode 100644 app/ui_layer/browser/frontend/tests/agent-app-media.test.ts create mode 100644 app/ui_layer/browser/frontend/tests/question-queue.test.ts create mode 100644 tests/qa_runtime.py create mode 100644 tests/test_app_readiness.py delete mode 100644 tests/test_installer_stream_routing.py create mode 100644 tests/test_qa_runtime.py create mode 100644 tests/test_question_dismissal.py create mode 100644 tests/test_usage_delivery.py create mode 100644 tests/test_vlm_regressions.py diff --git a/agent_core/core/hooks/usage.py b/agent_core/core/hooks/usage.py new file mode 100644 index 000000000..ebfd31399 --- /dev/null +++ b/agent_core/core/hooks/usage.py @@ -0,0 +1,121 @@ +"""Deliver async hooks in the request's context and drain them at shutdown. + +The loop belongs to a request, not to a long-lived model instance. Worker +threads inherit that ownership through ContextVars, including session context. +""" + +from __future__ import annotations + +import asyncio +import concurrent.futures +from contextvars import ContextVar +import logging +import threading + +_loop = ContextVar("usage_owner_loop", default=None) +_request_pending = ContextVar("usage_request_pending", default=None) +_pending: set = set() +_lock = threading.Lock() +_logger = logging.getLogger(__name__) + + +def _track(future): + # A Task is bound to its loop. Track a thread-safe completion handle so + # application shutdown can also drain workers owned by other running loops. + completion = concurrent.futures.Future() + with _lock: + _pending.add(completion) + + def done(item): + cancelled = item.cancelled() + error = None if cancelled else item.exception() + with _lock: + _pending.discard(completion) + if cancelled: + completion.cancel() + elif error is not None: + completion.set_exception(error) + _logger.error( + "Usage delivery failed", + exc_info=(type(error), error, error.__traceback__), + ) + else: + completion.set_result(None) + + future.add_done_callback(done) + return future + + +def dispatch_usage(hook, event) -> None: + """Submit once; failed hooks are observable and never automatically retried.""" + + async def deliver(): + await hook(event) + + try: + running = asyncio.get_running_loop() + except RuntimeError: + running = None + owner = _loop.get() or running + if owner is None: + # Synchronous callers have no event loop to own delivery. Finish the + # hook before returning. Local reporters must be safe across loops. + asyncio.run(deliver()) + return + if owner.is_closed(): + raise RuntimeError("Usage request's event loop is closed") + future = ( + owner.create_task(deliver()) + if owner is running + else asyncio.run_coroutine_threadsafe(deliver(), owner) + ) + _track(future) + request = _request_pending.get() + if request is not None: + request.append(future) + + +async def _join(futures): + if futures: + await asyncio.gather( + *( + asyncio.shield(asyncio.wrap_future(f)) + if isinstance(f, concurrent.futures.Future) + else asyncio.shield(f) + for f in futures + ), + return_exceptions=True, + ) + + +async def to_thread_with_usage(function, /, *args, **kwargs): + """Like to_thread, but delivery completes before a successful call returns. + + Cancellation leaves the worker tracked: shutdown drains it before closing + the reporter. No request is resubmitted after an uncertain outcome. + """ + request = [] + owner_token = _loop.set(asyncio.get_running_loop()) + pending_token = _request_pending.set(request) + try: + worker = _track( + asyncio.create_task(asyncio.to_thread(function, *args, **kwargs)) + ) + try: + return await asyncio.shield(worker) + finally: + if worker.done(): + await _join(request) + finally: + _request_pending.reset(pending_token) + _loop.reset(owner_token) + + +async def drain_usage() -> None: + """Wait for workers and hooks, including hooks submitted by finishing workers.""" + while True: + with _lock: + pending = list(_pending) + if not pending: + return + await _join(pending) diff --git a/agent_core/core/impl/image_gen/interface.py b/agent_core/core/impl/image_gen/interface.py index 08c85704e..f6bc19c39 100644 --- a/agent_core/core/impl/image_gen/interface.py +++ b/agent_core/core/impl/image_gen/interface.py @@ -18,6 +18,7 @@ if TYPE_CHECKING: from agent_core.core.errors import ClassifiedError +from agent_core.core.hooks.usage import dispatch_usage, to_thread_with_usage import asyncio import base64 import io @@ -197,9 +198,7 @@ def _report_usage_async( ) -> None: """Report image-generation usage if the hook is set (mirrors VLM). - Best-effort: scheduling onto the running loop can fail when invoked - from a worker thread without one, in which case the usage is dropped - with a warning rather than breaking generation. + Delivery uses the request's owning loop and is drained at shutdown. """ if not self._report_usage: return @@ -212,10 +211,7 @@ def _report_usage_async( output_tokens=output_tokens, cached_tokens=cached_tokens, ) - if self._main_loop is not None: - self._main_loop.call_soon_threadsafe( - lambda: asyncio.create_task(self._report_usage(event)) - ) + dispatch_usage(self._report_usage, event) except Exception as e: logger.warning(f"[IMAGE_GEN] Failed to report usage: {e}") @@ -307,8 +303,7 @@ def generate_image( async def generate_image_async(self, **kwargs) -> List[str]: """Async wrapper — runs generate_image in a thread pool.""" - loop = asyncio.get_event_loop() - return await loop.run_in_executor(None, lambda: self.generate_image(**kwargs)) + return await to_thread_with_usage(self.generate_image, **kwargs) # ──────────────────────── OpenAI provider ──────────────────────────────── diff --git a/agent_core/core/impl/llm/interface.py b/agent_core/core/impl/llm/interface.py index 77249329c..dffb53375 100644 --- a/agent_core/core/impl/llm/interface.py +++ b/agent_core/core/impl/llm/interface.py @@ -13,7 +13,7 @@ from __future__ import annotations -import asyncio +from agent_core.core.hooks.usage import dispatch_usage, to_thread_with_usage import contextvars import re import time @@ -614,13 +614,7 @@ def _report_usage_async( output_tokens=output_tokens, cached_tokens=cached_tokens, ) - # Schedule the async hook on the event loop - try: - loop = asyncio.get_running_loop() - loop.create_task(self._report_usage(event)) - except RuntimeError: - # No running event loop - create one for this call - asyncio.run(self._report_usage(event)) + dispatch_usage(self._report_usage, event) except Exception as e: logger.warning(f"[LLM] Failed to report usage: {e}") @@ -1260,7 +1254,7 @@ async def generate_response_async( ) token = _active_session_key.set(usage_key) try: - return await asyncio.to_thread( + return await to_thread_with_usage( self._generate_response_sync, system_prompt, user_prompt, @@ -2335,7 +2329,7 @@ async def generate_response_with_session_async( self._begin_call(prompt_name=prompt_name, call_type=call_type, task_id=task_id) token = _active_session_key.set(f"{task_id}:{call_type}") try: - return await asyncio.to_thread( + return await to_thread_with_usage( self._generate_response_with_session_sync, task_id, call_type, diff --git a/agent_core/core/impl/video_gen/interface.py b/agent_core/core/impl/video_gen/interface.py index d621985c6..84b4d8eb7 100644 --- a/agent_core/core/impl/video_gen/interface.py +++ b/agent_core/core/impl/video_gen/interface.py @@ -24,6 +24,7 @@ if TYPE_CHECKING: from agent_core.core.errors import ClassifiedError +from agent_core.core.hooks.usage import dispatch_usage, to_thread_with_usage import asyncio import base64 import json @@ -244,10 +245,7 @@ def _report_usage_async( output_tokens=output_tokens, cached_tokens=cached_tokens, ) - if self._main_loop is not None: - self._main_loop.call_soon_threadsafe( - lambda: asyncio.create_task(self._report_usage(event)) - ) + dispatch_usage(self._report_usage, event) except Exception as e: logger.warning(f"[VIDEO_GEN] Failed to report usage: {e}") @@ -396,8 +394,7 @@ def generate_video( async def generate_video_async(self, **kwargs) -> List[str]: """Async wrapper — runs generate_video in a thread pool.""" - loop = asyncio.get_running_loop() - return await loop.run_in_executor(None, lambda: self.generate_video(**kwargs)) + return await to_thread_with_usage(self.generate_video, **kwargs) # ──────────────────────── OpenAI / Sora 2 ──────────────────────────────── diff --git a/agent_core/core/impl/vlm/interface.py b/agent_core/core/impl/vlm/interface.py index 5e23001be..f08663ea4 100644 --- a/agent_core/core/impl/vlm/interface.py +++ b/agent_core/core/impl/vlm/interface.py @@ -12,7 +12,7 @@ from __future__ import annotations -import asyncio +from agent_core.core.hooks.usage import dispatch_usage, to_thread_with_usage import base64 import os import re @@ -28,6 +28,7 @@ ReasoningDecision, resolve_reasoning, ) +from agent_core.core.models.output import OutputIntent from agent_core.utils.token import billable_tokens from agent_core.core.hooks import ( GetTokenCountHook, @@ -235,6 +236,8 @@ def describe_image( system_prompt: str | None = None, user_prompt: str | None = "Describe this image in detail.", log_response: bool = True, + *, + output_intent: OutputIntent | None = None, ) -> str: """Read an image from disk and describe it using the VLM. @@ -258,6 +261,7 @@ def describe_image( system_prompt=system_prompt, user_prompt=user_prompt, log_response=log_response, + output_intent=output_intent, ) def describe_image_bytes( @@ -266,7 +270,9 @@ def describe_image_bytes( system_prompt: str | None = None, user_prompt: str | None = "Describe this image in detail.", log_response: bool = True, - json_mode: bool = True, + json_mode: bool | None = None, + *, + output_intent: OutputIntent | None = None, ) -> str: """Describe an image from raw bytes using the VLM. @@ -279,6 +285,12 @@ def describe_image_bytes( Returns: Textual description of the image. """ + intent = output_intent or OutputIntent("json" if json_mode else "text") + if json_mode is not None and bool(json_mode) != (intent.format == "json"): + raise ValueError("json_mode conflicts with output_intent") + system_prompt = intent.system_prompt(system_prompt) + user_prompt = user_prompt or "Describe this image in detail." + json_mode = intent.format == "json" try: if log_response: logger.info(f"[LLM SEND] system={system_prompt} | user={user_prompt}") @@ -305,7 +317,7 @@ def describe_image_bytes( ) elif self.provider == "gemini": response = self._gemini_describe_bytes( - image_bytes, system_prompt, user_prompt + image_bytes, system_prompt, user_prompt, json_mode=json_mode ) elif self.provider == "byteplus": response = self._byteplus_describe_bytes( @@ -338,7 +350,7 @@ def describe_image_bytes( if log_response: logger.info(f"[LLM RECV] {cleaned}") - return cleaned + return intent.validate(cleaned) except Exception as e: from agent_core.core.errors import ClassifiedError from agent_core.core.impl.llm.errors import classify_llm_error @@ -354,6 +366,8 @@ async def generate_response_async( user_prompt: Optional[str] = None, debug: bool = False, log_response: bool = True, + *, + output_intent: OutputIntent | None = None, ) -> str: """Async wrapper that defers the blocking call to a worker thread.""" if debug: @@ -364,12 +378,13 @@ async def generate_response_async( f.write(image_bytes) logger.info(f"[DEBUG] Image saved to {file_name}") - return await asyncio.to_thread( + return await to_thread_with_usage( self.describe_image_bytes, image_bytes, system_prompt, user_prompt, log_response, + output_intent=output_intent, ) def describe_image_ocr( @@ -506,10 +521,7 @@ def _report_usage_async( output_tokens=output_tokens, cached_tokens=cached_tokens, ) - # Schedule async reporting - asyncio.get_event_loop().call_soon( - lambda: asyncio.create_task(self._report_usage(event)) - ) + dispatch_usage(self._report_usage, event) except Exception as e: logger.warning(f"[VLM] Failed to report usage: {e}") @@ -574,7 +586,7 @@ def _reasoning(self) -> Optional[ReasoningDecision]: ) def _openai_describe_bytes( - self, image_bytes: bytes, sys: str | None, usr: str, json_mode: bool = True + self, image_bytes: bytes, sys: str | None, usr: str, json_mode: bool = False ) -> Dict[str, Any]: """OpenAI/Grok vision request with automatic prompt caching metrics.""" img_b64 = base64.b64encode(image_bytes).decode() @@ -703,10 +715,17 @@ def _ollama_describe_bytes( token_count_output = result.get("eval_count", 0) total_tokens = token_count_input + token_count_output + self._report_usage_async( + "vlm_remote", + self.provider, + self.model, + token_count_input, + token_count_output, + ) return {"tokens_used": total_tokens or 0, "content": content or ""} def _gemini_describe_bytes( - self, image_bytes: bytes, sys: str | None, usr: str + self, image_bytes: bytes, sys: str | None, usr: str, json_mode: bool = False ) -> Dict[str, Any]: """Gemini vision request with implicit caching metrics.""" if not self._gemini_client: @@ -718,7 +737,7 @@ def _gemini_describe_bytes( image_bytes_list=[image_bytes], system_prompt=sys, temperature=self.temperature, - json_mode=False, + json_mode=json_mode, ) # Record cache metrics @@ -801,9 +820,26 @@ def _byteplus_describe_bytes( or choices[0].get("delta", {}).get("content", "") or "" ).strip() - total_tokens = result.get("usage", {}).get("total_tokens", 0) - - return {"tokens_used": total_tokens or 0, "content": content or ""} + usage = result.get("usage") or {} + input_tokens = usage.get("prompt_tokens", 0) + output_tokens = usage.get("completion_tokens", 0) + cached_tokens = (usage.get("prompt_tokens_details") or {}).get( + "cached_tokens", 0 + ) + total_tokens = usage.get("total_tokens", input_tokens + output_tokens) + self._report_usage_async( + "vlm_byteplus", + self.provider, + self.model, + input_tokens, + output_tokens, + cached_tokens, + ) + return { + "tokens_used": total_tokens or 0, + "content": content or "", + "cached_tokens": cached_tokens, + } return {"tokens_used": 0, "content": ""} diff --git a/agent_core/core/models/output.py b/agent_core/core/models/output.py new file mode 100644 index 000000000..9bbbd3881 --- /dev/null +++ b/agent_core/core/models/output.py @@ -0,0 +1,51 @@ +"""Output contracts independent of a provider's wire format.""" + +from __future__ import annotations + +from dataclasses import dataclass +import json +from typing import Any, Literal + + +@dataclass(frozen=True) +class OutputIntent: + format: Literal["text", "ocr", "json"] = "text" + schema: dict[str, Any] | None = None + + def __post_init__(self): + if self.format not in ("text", "ocr", "json"): + raise ValueError(f"Unsupported output format: {self.format}") + if self.schema is not None: + if self.format != "json": + raise ValueError("A JSON schema requires JSON output") + from jsonschema.validators import validator_for + + validator_for(self.schema).check_schema(self.schema) + + def system_prompt(self, original: str | None) -> str | None: + if self.format == "text": + return original + instruction = ( + "Extract all text exactly as it appears, preserving line breaks. " + "Return only raw text without commentary or markdown." + ) + if self.format == "json": + instruction = ( + "Return a valid JSON object only, without markdown or commentary." + ) + if self.schema is not None: + instruction += " The JSON must satisfy this schema: " + json.dumps( + self.schema + ) + return "\n\n".join(part for part in (original, instruction) if part) + + def validate(self, content: str) -> str: + if self.format == "json": + value = json.loads(content) + if not isinstance(value, dict): + raise ValueError("Expected a JSON object from the model") + if self.schema is not None: + from jsonschema import validate + + validate(value, self.schema) + return content diff --git a/app/agent_app/manager.py b/app/agent_app/manager.py index ee1bf7ade..e33f1ea0e 100644 --- a/app/agent_app/manager.py +++ b/app/agent_app/manager.py @@ -4136,61 +4136,94 @@ def uses(name: str) -> bool: return new_start, new_env - async def _wait_for_server(self, port: int, timeout: int = 10) -> bool: - """Wait for a server to start listening on a port.""" - for _ in range(timeout * 2): - if self._is_port_in_use(port): - return True - await asyncio.sleep(0.5) - return False - - async def _wait_for_health_check(self, url: str, timeout: int = 15) -> bool: - """Wait for a server's health endpoint to respond with HTTP 200.""" - import urllib.request - import urllib.error - - for _ in range(timeout * 2): + async def _probe_ready(self, probe, timeout: float, process=None) -> bool: + """Bound retries by elapsed time, keep the loop responsive, stop on exit.""" + loop = asyncio.get_running_loop() + deadline = loop.time() + max(0.0, float(timeout)) + while loop.time() < deadline: + if process is not None and process.poll() is not None: + return False + task = asyncio.create_task(probe()) try: - req = urllib.request.Request(url, method="GET") - with urllib.request.urlopen(req, timeout=2) as response: - if response.status == 200: - return True - except ( - urllib.error.URLError, - urllib.error.HTTPError, - TimeoutError, - OSError, - ): + # Poll child exit without cancelling a healthy but slow probe. + # A single operation may use the remaining overall deadline. + while not task.done(): + remaining = deadline - loop.time() + if remaining <= 0 or ( + process is not None and process.poll() is not None + ): + return False + await asyncio.wait({task}, timeout=min(0.1, remaining)) + ready = task.result() + if ready and (process is None or process.poll() is None): + return True + except (OSError, asyncio.TimeoutError): pass - await asyncio.sleep(0.5) + finally: + if not task.done(): + task.cancel() + await asyncio.gather(task, return_exceptions=True) + remaining = deadline - loop.time() + if remaining > 0: + await asyncio.sleep(min(0.1, remaining)) return False + async def _wait_for_server( + self, port: int, timeout: float = 10, process=None + ) -> bool: + async def probe(): + reader, writer = await asyncio.open_connection("127.0.0.1", port) + writer.close() + await writer.wait_closed() + return True + + return await self._probe_ready(probe, timeout, process) + + async def _wait_for_health_check( + self, url: str, timeout: float = 15, process=None + ) -> bool: + """Read-only HTTP probes with a total deadline; never block the event loop.""" + import aiohttp + + async with aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=2)) as client: + + async def probe(): + try: + async with client.get(url) as response: + return response.status == 200 + except aiohttp.ClientError: + return False + + return await self._probe_ready(probe, timeout, process) + async def _check_health_with_strategy( - self, health_cfg, port: int, process, timeout: int = 30 + self, health_cfg, port: int, process, timeout: float = 30 ) -> bool: - """Check health using configured strategy (http_get, tcp, process_alive, or URL string).""" + """All strategies share deadline, cancellation and process-exit handling.""" if isinstance(health_cfg, str): - # Backward compat: plain URL string - return await self._wait_for_health_check(health_cfg, timeout=timeout) - + return await self._wait_for_health_check( + health_cfg, timeout=timeout, process=process + ) if not isinstance(health_cfg, dict): - # No health config — just check if port is listening - return await self._wait_for_server(port, timeout=timeout) - + return await self._wait_for_server(port, timeout=timeout, process=process) strategy = health_cfg.get("strategy", "tcp") - timeout = health_cfg.get("timeout", timeout) - + timeout = float(health_cfg.get("timeout", timeout)) if strategy == "http_get": - url = health_cfg.get("url", f"http://localhost:{port}") - url = url.replace("{{PORT}}", str(port)) - return await self._wait_for_health_check(url, timeout=timeout) - elif strategy == "tcp": - return await self._wait_for_server(port, timeout=timeout) - elif strategy == "process_alive": - await asyncio.sleep(2) - return process.poll() is None - - return await self._wait_for_server(port, timeout=timeout) + url = health_cfg.get("url", f"http://localhost:{port}").replace( + "{{PORT}}", str(port) + ) + return await self._wait_for_health_check( + url, timeout=timeout, process=process + ) + if strategy == "process_alive": + loop = asyncio.get_running_loop() + stable_at = loop.time() + 2 + + async def probe(): + return loop.time() >= stable_at + + return await self._probe_ready(probe, timeout, process) + return await self._wait_for_server(port, timeout=timeout, process=process) def validate_bridge_token(self, token: str) -> Optional[str]: """ diff --git a/app/agent_app/op_smoke.py b/app/agent_app/op_smoke.py index f763148c1..5f6990ec8 100644 --- a/app/agent_app/op_smoke.py +++ b/app/agent_app/op_smoke.py @@ -241,6 +241,10 @@ def _invoke( request = urllib.request.Request(url, data=data, headers=headers, method=method) try: with urllib.request.urlopen(request, timeout=_REQUEST_TIMEOUT_S) as response: + # Complete bounded response handling before closing the connection. + # A transport failure after a write has an uncertain outcome: report + # it once; never resubmit a potentially mutating operation. + response.read(64 * 1024) return OpSmokeResult(name=name, ok=True, status=response.status, detail="") except urllib.error.HTTPError as e: try: diff --git a/app/agent_app/test_data_safety.py b/app/agent_app/test_data_safety.py index 0675c1f7c..ab1cc59b7 100644 --- a/app/agent_app/test_data_safety.py +++ b/app/agent_app/test_data_safety.py @@ -1965,14 +1965,15 @@ async def _scenario(): class _Gate26(_http26.BaseHTTPRequestHandler): def do_POST(self): + self.rfile.read(int(self.headers.get("Content-Length", "0"))) SEEN26["token"] = self.headers.get("X-A2App-Token") ok = SEEN26["token"] == "tok-26-secret" + body = b'{"ok": true}' if ok else b'{"error": "agent token required"}' self.send_response(200 if ok else 401) self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(body))) self.end_headers() - self.wfile.write( - b'{"ok": true}' if ok else b'{"error": "agent token required"}' - ) + self.wfile.write(body) def log_message(self, *a): pass @@ -1990,6 +1991,7 @@ def log_message(self, *a): assert SEEN26["token"] == "tok-26-secret", "X-A2App-Token never crossed the wire" assert result.ok and result.status == 200, (result.status, result.detail) server26.shutdown() + server26.server_close() print("§26 changed-op smoke agent token: OK") print("\nData-safety acceptance: ALL GREEN") diff --git a/app/agent_base.py b/app/agent_base.py index e545a0284..e0fbf63c5 100644 --- a/app/agent_base.py +++ b/app/agent_base.py @@ -559,8 +559,8 @@ async def clear_session(self, session_id: str) -> bool: Chat-message rows are cleared by the adapter (chat storage is a UI concern); this handles the agent-side state. """ - if session_id in self.busy_sessions: - await self.request_run_stop(session_id) + # Runs can exist as queued continuations without a UI busy marker. + await self.request_run_stop(session_id) if not self.session_manager.clear_session(session_id): return False self._persist_llm_session(session_id) @@ -1737,7 +1737,7 @@ async def request_run_stop(self, session_id: str) -> bool: stopped = await self.session_runtime.request_stop(session_id) except Exception: logger.error(f"[RUN] request_stop failed for {session_id}", exc_info=True) - stopped = False + raise if not stopped: # Nothing was running (stale UI state) — settle the UI to idle. self._emit_run_state(session_id, "idle") diff --git a/app/config/settings.json b/app/config/settings.json index c9bd15ffa..8a6f992e3 100644 --- a/app/config/settings.json +++ b/app/config/settings.json @@ -1,5 +1,5 @@ { - "version": "1.4.3", + "version": "1.4.4", "general": { "agent_name": "CraftBot", "os_language": "en", diff --git a/app/console.py b/app/console.py new file mode 100644 index 000000000..25c86dec5 --- /dev/null +++ b/app/console.py @@ -0,0 +1,19 @@ +"""Consistent Unicode console output for source and installed entry points.""" + +import os +import sys + + +def configure_console() -> None: + if sys.platform != "win32": + return + for stream in (sys.stdout, sys.stderr): + reconfigure = getattr(stream, "reconfigure", None) + if reconfigure is not None: + try: + reconfigure(encoding="utf-8", errors="replace") + except (OSError, ValueError): + pass + # Child interpreters must agree when their output is redirected to a log. + os.environ["PYTHONIOENCODING"] = "utf-8" + os.environ["PYTHONUTF8"] = "1" diff --git a/app/data/action/render_ui.py b/app/data/action/render_ui.py index ded289775..bb647bcc9 100644 --- a/app/data/action/render_ui.py +++ b/app/data/action/render_ui.py @@ -15,7 +15,9 @@ "responsive 360-900px wide, full-height page; the host shows the title. " "Colors must follow host light/dark: only --cb-{bg,surface,text,muted,border,accent,font} " "or light-dark(light,dark), not prefers-color-scheme; redraw canvas on window " - "'craftbot-theme'. Restore window.craftbot.state and " + "'craftbot-theme'. Static native input/select/textarea controls persist automatically: give " + "each a stable id (or data-craftbot-state key). Use persistence='manual' for dynamic/custom " + "canvas/3D/application state: restore window.craftbot.state and " "call window.craftbot.saveState(obj) on change (JSON, 64KB; store deadlines and " "timestamps). Fetched data needs loading/error states, real timestamps and checked " "units. Keep code compact. Afterwards send at most one short sentence; don't ask for " @@ -37,6 +39,10 @@ "type": "string", "description": "Optional: earlier render_ui id in this session, to revise with complete new HTML (keep state keys).", }, + "persistence": { + "type": "string", + "description": "controls (default): persist static native controls by stable id/data-craftbot-state, hydrated before scripts run; manual: explicitly restore/save dynamic DOM or custom state with craftbot.state/saveState; none: stateless output. Never persist secrets.", + }, "libraries": { "type": "array", "items": {"type": "string"}, @@ -76,6 +82,7 @@ async def render_ui(input_data: dict) -> dict: from app.generative_ui import make_artifact, read_images try: + input_data = {"persistence": "controls", **input_data} artifact = make_artifact(input_data) except ValueError as exc: return {"status": "error", "message": str(exc)} diff --git a/app/generative_ui.py b/app/generative_ui.py index 9712d62b7..98230a564 100644 --- a/app/generative_ui.py +++ b/app/generative_ui.py @@ -2,6 +2,7 @@ import re import uuid +from html.parser import HTMLParser from pathlib import Path MAX_HTML_BYTES = 512_000 @@ -15,6 +16,45 @@ IMAGE_NAME = re.compile(r"[A-Za-z_][A-Za-z0-9_]{0,39}") +def persistence_contract(html: str, mode: object) -> str: + """Validate a declared persistence strategy, without inspecting JS wording.""" + if mode not in ("controls", "manual", "none"): + raise ValueError("persistence must be controls, manual or none.") + if mode != "controls": + return mode + + class Controls(HTMLParser): + keys: dict[str, str] = {} + + def handle_starttag(self, tag, attrs): + attrs = dict(attrs) + kind = (attrs.get("type") or "text").lower() + if tag not in ("input", "select", "textarea") or ( + tag == "input" and kind in ("button", "submit", "reset", "image") + ): + return + if kind in ("password", "file", "hidden"): + raise ValueError( + "Sensitive/file/hidden controls require manual persistence; never save secrets." + ) + key = attrs.get("data-craftbot-state") or attrs.get("id") + if ( + not key + or not re.fullmatch(r"[A-Za-z0-9_-]{1,80}", key) + or key in ("__proto__", "constructor", "prototype") + ): + raise ValueError( + "Every persisted control needs a stable id or data-craftbot-state key." + ) + if key in self.keys and not (kind == "radio" and self.keys[key] == "radio"): + raise ValueError(f"Duplicate persistence key: {key}") + self.keys[key] = kind + + parser = Controls() + parser.feed(html) + return mode + + def normalize_origins(value: object, field: str) -> list[str]: """Accept exact public HTTPS origins, never CSP expressions or local hosts.""" if not isinstance(value, list) or len(value) > MAX_ORIGINS: @@ -146,4 +186,5 @@ def make_artifact(data: dict, revision: int = 1) -> dict: "connect_origins": connect_origins, "image_origins": image_origins, "libraries": libraries, + "persistence": persistence_contract(html, data.get("persistence", "manual")), } diff --git a/app/mini_browser/core.py b/app/mini_browser/core.py index a64c6e0a3..79dcfc29b 100644 --- a/app/mini_browser/core.py +++ b/app/mini_browser/core.py @@ -418,6 +418,7 @@ def __init__( # the saved pages reopen at a launch, nothing is saved. self._restoring = False self._reopening: Set[str] = set() # tabs whose reopened page has not loaded + self._reopen_tasks: Dict[str, asyncio.Task] = {} self._pages_saved: Optional[resume.Pages] = None self._pages_handle: Optional[asyncio.TimerHandle] = None self._frame_unsaved: Optional[resume.LastFrame] = None @@ -858,6 +859,10 @@ async def _teardown(self) -> None: self._watchdog_task = None self._cancel_saves() self._reopening.clear() + for task in self._reopen_tasks.values(): + if not task.done(): + task.cancel() + self._reopen_tasks.clear() self._pages_saved = None self._nav_refresh.clear() self._nav_dirty.clear() @@ -1330,7 +1335,7 @@ async def _reopen_pages(self, profile: Path) -> None: tab.url, tab.title, tab.loading = url, page.title, True self._reopening.add(tab.id) reopened[index] = tab - self.spawn(self._reopen(tab, url)) + self._reopen_tasks[tab.id] = self.spawn(self._reopen(tab, url)) viewed = reopened.get(saved.viewed) or next(iter(reopened.values()), None) if viewed is not None: self.viewed_tab_id = viewed.id @@ -1340,11 +1345,38 @@ async def _reopen_pages(self, profile: Path) -> None: async def _reopen(self, tab: Tab, url: str) -> None: try: - await self._open_in_background(tab, url) + await self._goto(tab, url) finally: self._reopening.discard(tab.id) self._schedule_nav_refresh(tab) + async def _wait_for_reopen(self, tab: Tab) -> None: + """Metadata can be restored before Chromium commits the document. + + Only operations on this tab wait. Shield the shared navigation from + cancellation of an individual agent operation; teardown still cancels it. + """ + task = self._reopen_tasks.get(tab.id) + if task is not None: + try: + await asyncio.shield(task) + except asyncio.CancelledError: + if tab.closed: + raise self._closed_error(tab) from None + if not task.cancelled(): + raise + finally: + if task.done() and self._reopen_tasks.get(tab.id) is task: + self._reopen_tasks.pop(tab.id, None) + if tab.closed: + raise self._closed_error(tab) + + def _cancel_reopen(self, tab: Tab) -> None: + restore = self._reopen_tasks.pop(tab.id, None) + if restore is not None and not restore.done(): + restore.cancel() + self._reopening.discard(tab.id) + # ═════════════════════════════════════════════════════════════════════════ # Streaming # ═════════════════════════════════════════════════════════════════════════ @@ -1596,6 +1628,7 @@ async def ui_control(self, tab_id: str, take: bool) -> None: return tab.user_control = take if take: + self._cancel_reopen(tab) tab.touch_user() self._agent_notice( tab, @@ -1737,6 +1770,8 @@ def _note_user_input(self, tab: Tab, takes_control: bool = True) -> None: Runs BEFORE the input is dispatched to the page: an agent operation in flight on the tab sees ``user_control`` at its next step and stops. """ + if takes_control: + self._cancel_reopen(tab) tab.touch_user() self._touch() if ( @@ -1818,6 +1853,7 @@ async def agent_op(self, owner: str, op: str, params: dict) -> dict: try: async with tab.lock: if self.tabs.get(tab.id) is tab: + await self._wait_for_reopen(tab) refused = self._control_refusal(tab, op, params) if refused is not None: return self._finish(refused, tab, owner=owner) @@ -3072,7 +3108,7 @@ def _remove_tab(self, tab: Tab) -> None: position = order.index(tab.id) del self.tabs[tab.id] tab.mark_closed(CLOSED_TAB) # an operation on it ends at once - self._reopening.discard(tab.id) + self._cancel_reopen(tab) self._retire_secrets(tab) self._cuts.pop(tab.id, None) self._stream.forget_tab(tab.id) diff --git a/app/triggers/runtime.py b/app/triggers/runtime.py index 612b7fdd8..0877e68de 100644 --- a/app/triggers/runtime.py +++ b/app/triggers/runtime.py @@ -54,10 +54,9 @@ def remove_session_log_sink(session_id: str) -> None: # type: ignore[misc] # cross-session parallelism (LLM rate limits, local resource pressure). DEFAULT_MAX_CONCURRENT_TURNS = 3 -# How long a user force-stop waits for the cancelled turn to settle before -# finalizing anyway. Cancellation is normally near-instant (LLM HTTP calls -# and async actions unwind at the next await); this guard only exists so a -# stuck await can never leave the UI on "stopping" forever. +# How long a stop/clear request waits before reporting failure. Timed-out +# turns remain stopping until they actually settle; callers must not reset +# context while old work can still write to it. STOP_SETTLE_TIMEOUT_S = 15.0 ReactFn = Callable[[Trigger], Awaitable[None]] @@ -73,11 +72,12 @@ class _StopSignal: await full shutdown (the UI's stop-spinner ends on this). """ - __slots__ = ("requested", "settled") + __slots__ = ("requested", "settled", "processes_stopped") def __init__(self) -> None: self.requested = False self.settled = asyncio.Event() + self.processes_stopped = asyncio.Event() # Sources that must never be folded into another trigger's turn. @@ -322,50 +322,70 @@ async def request_stop(self, session_id: str) -> bool: Returns True when there was a run to stop. Awaits full settlement (bounded by STOP_SETTLE_TIMEOUT_S) before returning, so the caller - can treat the return as "everything is shut". + can treat a successful return as "everything is shut". Raises on + timeout or process-shutdown failure instead of claiming success. """ + signal = self._stops.get(session_id) + if signal is not None: + # Repeated stop/clear requests join the same cancellation. A + # second cancel could interrupt the first turn's cleanup. + await asyncio.wait_for(signal.settled.wait(), STOP_SETTLE_TIMEOUT_S) + return True purged = await self._purge_continuations(session_id) + signal = self._stops.get(session_id) + if signal is not None: + await asyncio.wait_for(signal.settled.wait(), STOP_SETTLE_TIMEOUT_S) + return True turn = self._turns.get(session_id) turn_inflight = turn is not None and not turn.done() if not turn_inflight: - if purged and self._on_stopped: - # The run lived only as queued continuation rows. - await self._on_stopped(session_id) + if purged: + # Fence fresh work while the parked run's async finalizer + # settles. Overlapping requests join this stop as well. + signal = self._stops[session_id] = _StopSignal() + try: + if self._on_stopped: + await self._on_stopped(session_id) + finally: + self._stops.pop(session_id, None) + signal.settled.set() return bool(purged) signal = self._stops.setdefault(session_id, _StopSignal()) signal.requested = True + turn.cancel() - # Kill real OS work first so pool threads blocked on a child process - # unblock, then cancel the turn's await chain. Blocking kill runs in - # a worker thread to keep the event loop free. + # Cancel before yielding so a naturally finishing turn cannot escape + # settlement. The consumer holds this lane until OS work is shut down. try: from agent_core.core.impl.action.cancellation import ( kill_session_processes, ) - await asyncio.get_running_loop().run_in_executor( + kill = asyncio.get_running_loop().run_in_executor( None, kill_session_processes, session_id ) + # A disconnected caller must not release the lane while the + # process-kill worker can still affect this session's next run. + kill.add_done_callback(lambda _: signal.processes_stopped.set()) + await asyncio.shield(kill) except Exception as e: + signal.processes_stopped.set() logger.warning(f"[SessionRuntime] Process kill failed: {e}") + raise - turn.cancel() try: await asyncio.wait_for(signal.settled.wait(), STOP_SETTLE_TIMEOUT_S) except asyncio.TimeoutError: - # The turn is stuck in a non-cancellable await. Finalize anyway: - # the UI must reach idle, and the abandoned coroutine can no - # longer enqueue continuations once its triggers settle. + # Keep the lane stopping: resetting context while the old turn + # can still write would resurrect cleared work. A retry joins + # this signal, and normal settlement still finalizes it once. logger.error( f"[SessionRuntime] Stop settlement timed out for {session_id}; " - "finalizing forcefully." + "the turn is still stopping." ) - self._stops.pop(session_id, None) - await self._purge_continuations(session_id) - if self._on_stopped: - await self._on_stopped(session_id) + raise return True async def _purge_continuations(self, session_id: str) -> int: @@ -421,6 +441,20 @@ def _ensure_loop(self, session_id: str) -> None: name=f"session-loop-{session_id}", ) + async def _run_turn(self, session_id: str, trig: Trigger) -> None: + signal = self._stops.get(session_id) + if signal is not None: + await signal.settled.wait() + try: + async with self._turn_semaphore: + await self._react(trig) + finally: + signal = self._stops.get(session_id) + if signal is not None and signal.requested: + # Also settle a turn that swallowed cancellation or returned + # while cancellation was requested. + raise asyncio.CancelledError + async def _consume(self, session_id: str, queue: SessionTriggerQueue) -> None: """The serial agent loop for one session: claim → react → settle. @@ -490,21 +524,20 @@ async def _consume(self, session_id: str, queue: SessionTriggerQueue) -> None: trig = _merge_triggers(trig, extras) try: - async with self._turn_semaphore: - # The turn runs as its own task so request_stop can - # cancel exactly this turn without tearing down the - # session loop. - turn = asyncio.create_task( - self._react(trig), name=f"turn-{session_id}" - ) - self._turns[session_id] = turn - try: - await turn - finally: - self._turns.pop(session_id, None) + # Track capacity waits too: claimed work must not start + # after a stop/clear simply because another lane was busy. + turn = asyncio.create_task( + self._run_turn(session_id, trig), name=f"turn-{session_id}" + ) + self._turns[session_id] = turn + try: + await turn + finally: + self._turns.pop(session_id, None) except asyncio.CancelledError: signal = self._stops.get(session_id) if signal is not None and signal.requested and self._running: + await signal.processes_stopped.wait() # User force-stop, not process shutdown: the rows were # consumed by user intent — ack them (nack would # redeliver and restart the very work the user diff --git a/app/ui_layer/adapters/base.py b/app/ui_layer/adapters/base.py index c61949f9c..dc5e2011f 100644 --- a/app/ui_layer/adapters/base.py +++ b/app/ui_layer/adapters/base.py @@ -260,9 +260,9 @@ def _subscribe_events(self) -> None: # Default event handlers - subclasses can override for custom behavior # ───────────────────────────────────────────────────────────────────── - def _handle_user_message(self, event: UIEvent) -> None: + def _handle_user_message(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle user message event.""" - asyncio.create_task( + return asyncio.create_task( self._display_chat_message( "You", event.data.get("message", ""), @@ -272,7 +272,7 @@ def _handle_user_message(self, event: UIEvent) -> None: ) ) - def _handle_agent_message(self, event: UIEvent) -> None: + def _handle_agent_message(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle agent message event.""" from app.onboarding import onboarding_manager @@ -302,7 +302,7 @@ def _handle_agent_message(self, event: UIEvent) -> None: ChatMessageOption(label=str(o), value=str(o)) for o in question["options"] ] - asyncio.create_task( + return asyncio.create_task( self._display_chat_message( agent_name, event.data.get("message", ""), @@ -316,9 +316,9 @@ def _handle_agent_message(self, event: UIEvent) -> None: ) ) - def _handle_system_message(self, event: UIEvent) -> None: + def _handle_system_message(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle system message event.""" - asyncio.create_task( + return asyncio.create_task( self._display_chat_message( "System", event.data.get("message", ""), @@ -328,9 +328,9 @@ def _handle_system_message(self, event: UIEvent) -> None: ) ) - def _handle_error_message(self, event: UIEvent) -> None: + def _handle_error_message(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle error message event.""" - asyncio.create_task( + return asyncio.create_task( self._display_chat_message( "Error", event.data.get("message", ""), @@ -339,9 +339,9 @@ def _handle_error_message(self, event: UIEvent) -> None: ) ) - def _handle_info_message(self, event: UIEvent) -> None: + def _handle_info_message(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle info message event.""" - asyncio.create_task( + return asyncio.create_task( self._display_chat_message( "Info", event.data.get("message", ""), @@ -350,11 +350,11 @@ def _handle_info_message(self, event: UIEvent) -> None: ) ) - def _handle_action_start(self, event: UIEvent) -> None: + def _handle_action_start(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle action start event.""" if self.action_panel: session_id = event.data.get("session_id") or "main" - asyncio.create_task( + return asyncio.create_task( self.action_panel.add_item( ActionItem( id=event.data.get("action_id", ""), @@ -367,7 +367,7 @@ def _handle_action_start(self, event: UIEvent) -> None: ) ) - def _handle_action_end(self, event: UIEvent) -> None: + def _handle_action_end(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle action end event.""" if self.action_panel: status = "error" if event.data.get("error") else "completed" @@ -378,7 +378,7 @@ def _handle_action_end(self, event: UIEvent) -> None: # Get output and error data output = event.data.get("output") error_message = event.data.get("error_message") - asyncio.create_task( + return asyncio.create_task( self.action_panel.update_item_by_name( action_name=action_name, session_id=session_id, @@ -394,10 +394,10 @@ def _handle_reasoning(self, event: UIEvent) -> None: it into the session's inline activity feed.""" pass - def _handle_state_change(self, event: UIEvent) -> None: + def _handle_state_change(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle agent state change event.""" if self.status_bar: - asyncio.create_task( + return asyncio.create_task( self.status_bar.set_status(event.data.get("status_message", "")) ) @@ -412,25 +412,25 @@ def _handle_gui_mode_change(self, event: UIEvent) -> None: if self.footage_component: self.footage_component.set_visible(event.data.get("gui_mode", False)) - def _handle_footage_update(self, event: UIEvent) -> None: + def _handle_footage_update(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle footage update event.""" if self.footage_component: - asyncio.create_task( + return asyncio.create_task( self.footage_component.update(event.data.get("image_bytes", b"")) ) - def _handle_footage_clear(self, event: UIEvent) -> None: + def _handle_footage_clear(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle footage clear event.""" if self.footage_component: - asyncio.create_task(self.footage_component.clear()) + return asyncio.create_task(self.footage_component.clear()) def _handle_show_menu(self, event: UIEvent) -> None: """Handle show menu event. Override in Browser adapter.""" pass - def _handle_shutdown(self, event: UIEvent) -> None: + def _handle_shutdown(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle shutdown event. Override in adapters for specific cleanup.""" - asyncio.create_task(self.stop()) + return asyncio.create_task(self.stop()) # ───────────────────────────────────────────────────────────────────── # Helper methods diff --git a/app/ui_layer/adapters/browser_adapter.py b/app/ui_layer/adapters/browser_adapter.py index bf1740473..dd6499394 100644 --- a/app/ui_layer/adapters/browser_adapter.py +++ b/app/ui_layer/adapters/browser_adapter.py @@ -1086,13 +1086,13 @@ async def _handle_enhance_prompt(self, data: Dict[str, Any], ws) -> None: except Exception as e: logger.warning(f"[BROWSER ADAPTER] enhance_prompt failed: {e}") - def _handle_reasoning(self, event: UIEvent) -> None: + def _handle_reasoning(self, event: UIEvent) -> Optional[asyncio.Task]: """Handle reasoning event — add it to the session's activity feed.""" session_id = event.data.get("session_id") or "main" reasoning_id = event.data.get("reasoning_id", "") content = event.data.get("content", "") - asyncio.create_task( + return asyncio.create_task( self._action_panel.add_item( ActionItem( id=reasoning_id, @@ -1105,7 +1105,7 @@ def _handle_reasoning(self, event: UIEvent) -> None: ) ) - def _handle_run_state_change(self, event: UIEvent) -> None: + def _handle_run_state_change(self, event: UIEvent) -> Optional[asyncio.Task]: """Broadcast a session's run state (typing indicator + stop button). ``state`` is "running" | "stopping" | "idle"; ``busy`` is the @@ -1114,7 +1114,7 @@ def _handle_run_state_change(self, event: UIEvent) -> None: session_id = event.data.get("session_id") or "main" busy = bool(event.data.get("busy", False)) state = event.data.get("state") or ("running" if busy else "idle") - asyncio.create_task( + return asyncio.create_task( self._broadcast( { "type": "session_busy", @@ -4573,7 +4573,9 @@ async def _handle_session_clear(self, data: Dict[str, Any]) -> None: # Agent side first: it stops a run in flight, whose last rows # must go with the conversation rather than land after the clear. - await self._controller.agent.clear_session(session_id) + if not await self._controller.agent.clear_session(session_id): + return + await self._controller.event_bus.drain(session_id) try: # Ordered storage worker: runs after any insert still queued # for this session, so no message resurfaces after the clear. diff --git a/app/ui_layer/browser/frontend/src/components/Chat/QuestionBox.tsx b/app/ui_layer/browser/frontend/src/components/Chat/QuestionBox.tsx index 8f6b7e9d5..0cf2d99f5 100644 --- a/app/ui_layer/browser/frontend/src/components/Chat/QuestionBox.tsx +++ b/app/ui_layer/browser/frontend/src/components/Chat/QuestionBox.tsx @@ -61,7 +61,12 @@ export function QuestionBox({ question, queueTotal, onAnswer, onDismiss }: Quest } return ( -
+
{ + // The next keyed box can mount between clicks. Only the first click + // in a pointer sequence may act; keyboard clicks have detail === 0. + if (event.detail > 1) event.stopPropagation() + }}>
{t('chat:question.asking', { sender: question.sender })} diff --git a/app/ui_layer/browser/frontend/src/components/GenerativeUI/GenerativeUI.tsx b/app/ui_layer/browser/frontend/src/components/GenerativeUI/GenerativeUI.tsx index 0de3df368..03675ddb5 100644 --- a/app/ui_layer/browser/frontend/src/components/GenerativeUI/GenerativeUI.tsx +++ b/app/ui_layer/browser/frontend/src/components/GenerativeUI/GenerativeUI.tsx @@ -31,10 +31,16 @@ export function GenerativeUI({ artifact, sessionId, controlsTarget, versions, on timeoutText.current = t('generativeUI.timeout') const [state, saveState] = usePersistedState(UI_STATE.chat.artifactState(JSON.stringify([sessionId, artifact.id]))) const stateRef = useRef(state) - stateRef.current = state const frame = useRef(null) const options = useRef(null) const [attempt, setAttempt] = useState(0) + // Another tab changed progress: rehydrate arbitrary author state by rebuilding + // the document. Local interactions mark stateRef first and keep their frame. + useLayoutEffect(() => { + if (stateRef.current === state) return + stateRef.current = state + setAttempt(value => value + 1) + }, [state]) const [paused, setPaused] = useState(false) const [status, setStatus] = useState<'loading' | 'ready' | 'error'>('loading') const [error, setError] = useState('') @@ -94,7 +100,10 @@ export function GenerativeUI({ artifact, sessionId, controlsTarget, versions, on setStatus(current => current === 'error' ? current : 'ready') break case 'state': - if (validState(data.state)) saveState(data.state) + if (validState(data.state)) { + stateRef.current = data.state + saveState(data.state) + } break } } @@ -108,7 +117,7 @@ export function GenerativeUI({ artifact, sessionId, controlsTarget, versions, on const restart = (reset = false) => { if (options.current?.open) { options.current.open = false; options.current.querySelector('summary')?.focus() } setConfirmReset(false) - if (reset) { stateRef.current = {}; saveState({}) } + if (reset) { stateRef.current = {}; saveState(stateRef.current) } setPaused(false) setAttempt(value => value + 1) } diff --git a/app/ui_layer/browser/frontend/src/components/GenerativeUI/runtime.ts b/app/ui_layer/browser/frontend/src/components/GenerativeUI/runtime.ts index 41165f4d1..9f37e60e5 100644 --- a/app/ui_layer/browser/frontend/src/components/GenerativeUI/runtime.ts +++ b/app/ui_layer/browser/frontend/src/components/GenerativeUI/runtime.ts @@ -66,6 +66,9 @@ export function buildDocument(artifact: GenerativeUIArtifact, state: Record !/^on/i.test(attr.name) && !['nonce', 'integrity', 'srcdoc', 'target', 'ping'].includes(attr.name)) + // Hydrate while inert, before author scripts calculate derived output. + // Old artifacts keep their manual state API; new outputs declare controls. + if (artifact.persistence === 'controls') hydrateControl(node, state) pending.push(node) if ('content' in node) pending.push((node as DefaultTreeAdapterTypes.Template).content) } @@ -117,6 +120,30 @@ export function buildDocument(artifact: GenerativeUIArtifact, state: Record { + if (!el?.matches?.('input,select,textarea') || ['password','file','hidden','button','submit','reset','image'].includes(el.type)) return null; + const key = el.getAttribute('data-craftbot-state') || el.id; + return key && /^[A-Za-z0-9_-]{1,80}$/.test(key) && !['__proto__','constructor','prototype'].includes(key) ? key : null; + }; + const valueFor = el => el.type === 'checkbox' ? el.checked : + el.multiple ? Array.from(el.selectedOptions, option => option.value) : el.value; + const changed = e => { + const el = e.target, key = keyFor(el); + if (!key || (el.type === 'radio' && !el.checked)) return; + window.craftbot.saveState({ ...state, [key]: valueFor(el) }); + }; + addEventListener('input', changed, true); + addEventListener('change', changed, true); + addEventListener('reset', e => queueMicrotask(() => { + const next = { ...state }; + for (const el of e.target.elements || []) { + const key = keyFor(el); + if (key && (el.type !== 'radio' || el.checked)) next[key] = valueFor(el); + } + window.craftbot.saveState(next); + }), true); + } // Links/forms are UI controls, never a way to leave the runtime. addEventListener('click', e => { if (e.target?.closest?.('a, area')) e.preventDefault(); }, true); addEventListener('submit', e => e.preventDefault(), true); @@ -152,3 +179,39 @@ export function buildDocument(artifact: GenerativeUIArtifact, state: Record\n' + serializeOuter(html) } + +/** Restore native controls before any generated script executes. */ +function hydrateControl(node: DefaultTreeAdapterTypes.Element, state: Record) { + if (!['input', 'select', 'textarea'].includes(node.tagName)) return + const attr = (name: string) => node.attrs.find(a => a.name === name)?.value + const kind = (attr('type') || 'text').toLowerCase() + if (['password', 'file', 'hidden', 'button', 'submit', 'reset', 'image'].includes(kind.toLowerCase())) return + const key = attr('data-craftbot-state') || attr('id') + if (!key || ['__proto__', 'constructor', 'prototype'].includes(key) || !Object.prototype.hasOwnProperty.call(state, key)) return + const value = state[key] + const set = (el: DefaultTreeAdapterTypes.Element, name: string, value?: string) => { + el.attrs = el.attrs.filter(a => a.name !== name) + if (value !== undefined) el.attrs.push({ name, value }) + } + if (node.tagName === 'textarea' && typeof value === 'string') { + node.childNodes = [{ nodeName: '#text', value, parentNode: node }] + } else if (node.tagName === 'select') { + const values = Array.isArray(value) ? value : [value] + const walk = (parent: DefaultTreeAdapterTypes.ParentNode) => { + for (const child of parent.childNodes) if ('tagName' in child) { + if (child.tagName === 'option') { + const option = child.attrs.find(a => a.name === 'value')?.value ?? child.childNodes + .map(n => 'value' in n ? n.value : '').join('') + .replace(/[\t\n\f\r ]+/g, ' ').replace(/^ | $/g, '') + set(child, 'selected', values.includes(option) ? '' : undefined) + } + walk(child) + } + } + walk(node) + } else if (kind === 'checkbox' && typeof value === 'boolean') { + set(node, 'checked', value ? '' : undefined) + } else if (kind === 'radio' && typeof value === 'string') { + set(node, 'checked', value === (attr('value') ?? 'on') ? '' : undefined) + } else if (typeof value === 'string') set(node, 'value', value) +} diff --git a/app/ui_layer/browser/frontend/src/store/slices/messagesSlice.ts b/app/ui_layer/browser/frontend/src/store/slices/messagesSlice.ts index e39762376..7e2cfcf49 100644 --- a/app/ui_layer/browser/frontend/src/store/slices/messagesSlice.ts +++ b/app/ui_layer/browser/frontend/src/store/slices/messagesSlice.ts @@ -1,5 +1,6 @@ import { createSlice, current, isDraft, PayloadAction } from '@reduxjs/toolkit' import type { ChatMessage } from '../../types' +import type { AppDispatch, RootState } from '../index' import { register } from '../socket/messageRegistry' import { UI_STATE } from '../uiState' import { findIndexInDraft } from './draftSearch' @@ -228,19 +229,42 @@ register('chat_history', (data, dispatch) => { })) }) -register('chat_clear', (data, dispatch) => { +// Progress may belong to an output outside the loaded/paginated history. +// Keep reset tombstones so a stale legacy tab cannot migrate it back later. +function resetArtifactProgress(sessionId: string | null, dispatch: AppDispatch, state: RootState) { + const prefix = UI_STATE.chat.artifactState('').key + for (const key of Object.keys(state.ui.values)) { + if (!key.startsWith(prefix)) continue + try { + const id = key.slice(prefix.length) + const scope: unknown = JSON.parse(id) + if (Array.isArray(scope) && scope.length === 2 && scope.every(item => typeof item === 'string') && (sessionId === null || scope[0] === sessionId)) { + dispatch(resetUiState(UI_STATE.chat.artifactState(id))) + } + } catch { /* Ignore malformed storage keys. */ } + } +} + +register('chat_clear', (data, dispatch, getState) => { const d = data as { sessionId?: string | null } | undefined + resetArtifactProgress(d?.sessionId ?? null, dispatch, getState()) dispatch(clearSession({ sessionId: d?.sessionId ?? null })) }) -register('session_cleared', (data, dispatch) => { +register('session_cleared', (data, dispatch, getState) => { const d = data as { sessionId?: string } | undefined - if (d?.sessionId) dispatch(clearSession({ sessionId: d.sessionId })) + if (d?.sessionId) { + resetArtifactProgress(d.sessionId, dispatch, getState()) + dispatch(clearSession({ sessionId: d.sessionId })) + } }) -register('session_deleted', (data, dispatch) => { +register('session_deleted', (data, dispatch, getState) => { const d = data as { sessionId?: string } | undefined - if (d?.sessionId) dispatch(dropSession({ sessionId: d.sessionId })) + if (d?.sessionId) { + resetArtifactProgress(d.sessionId, dispatch, getState()) + dispatch(dropSession({ sessionId: d.sessionId })) + } }) // A Generative UI output was closed (possibly on another client): its code diff --git a/app/ui_layer/browser/frontend/src/store/slices/uiSlice.ts b/app/ui_layer/browser/frontend/src/store/slices/uiSlice.ts index bd8e9c3af..033be3140 100644 --- a/app/ui_layer/browser/frontend/src/store/slices/uiSlice.ts +++ b/app/ui_layer/browser/frontend/src/store/slices/uiSlice.ts @@ -23,6 +23,7 @@ const initialState: UiSliceState = { interface UiStateChange { key: string lifetime: UiStateLifetime + writeThrough?: boolean /** undefined resets the key to its default. */ value: unknown } @@ -54,7 +55,7 @@ function applyValue(state: UiSliceState, key: string, value: unknown) { export const { uiStateChanged, uiStateSynced } = uiSlice.actions export default uiSlice.reducer -/** Action creator: back to the descriptor's default (its stored key is removed). */ +/** Back to the descriptor's default, retaining it only when its policy requires. */ export function resetUiState(descriptor: UiStateDescriptor) { return setUiState(descriptor, descriptor.defaultValue) } @@ -64,6 +65,7 @@ export function setUiState(descriptor: UiStateDescriptor, value: T) { return uiStateChanged({ key: descriptor.key, lifetime: descriptor.lifetime, - value: isDefaultUiValue(descriptor, value) ? undefined : value, + writeThrough: descriptor.writeThrough, + value: !descriptor.persistDefault && isDefaultUiValue(descriptor, value) ? undefined : value, }) } diff --git a/app/ui_layer/browser/frontend/src/store/uiState/catalog.ts b/app/ui_layer/browser/frontend/src/store/uiState/catalog.ts index 24b936c9c..44e819057 100644 --- a/app/ui_layer/browser/frontend/src/store/uiState/catalog.ts +++ b/app/ui_layer/browser/frontend/src/store/uiState/catalog.ts @@ -88,8 +88,11 @@ export const UI_STATE = { }), /** By message id: "Show message" details disclosure. */ messageDetailsExpanded: defineUiStateFamily('chat.messageDetailsExpanded', false, 'session'), - /** By [session id, artifact id]: explicit JSON state, separate from generated code. */ - artifactState: defineUiStateFamily>('chat.artifactState', {}, 'session'), + /** By [session id, artifact id]: progress survives tabs/restarts in this browser profile. */ + artifactState: defineUiStateFamily>('chat.artifactState', {}, 'preference', { + persistDefault: true, + writeThrough: true, + }), /** By session id: what the panel shows (outputs or the chat's * browser, one at a time). */ panelMode: defineUiStateFamily('chat.panelMode', 'auto', 'session', { diff --git a/app/ui_layer/browser/frontend/src/store/uiState/defineUiState.ts b/app/ui_layer/browser/frontend/src/store/uiState/defineUiState.ts index 63715f622..34602afc3 100644 --- a/app/ui_layer/browser/frontend/src/store/uiState/defineUiState.ts +++ b/app/ui_layer/browser/frontend/src/store/uiState/defineUiState.ts @@ -21,11 +21,17 @@ export interface UiStateDescriptor { readonly key: string readonly defaultValue: T readonly lifetime: UiStateLifetime + /** Retain a reset value so a legacy storage area cannot resurrect it. */ + readonly persistDefault?: boolean + /** Save discrete shared changes before another tab can read them. */ + readonly writeThrough?: boolean /** Accepts a value read back from storage; rejected values fall back to the default. */ readonly isValid: (value: unknown) => boolean } export interface UiStateOptions { + persistDefault?: boolean + writeThrough?: boolean /** Defaults to "same shape as the default value" (see `matchesShapeOf`). */ isValid?: (value: unknown) => boolean } @@ -40,6 +46,8 @@ export function defineUiState( key, defaultValue, lifetime, + persistDefault: options.persistDefault, + writeThrough: options.writeThrough, isValid: options.isValid ?? matchesShapeOf(defaultValue), }) } @@ -94,8 +102,8 @@ export function resolveUiState(descriptor: UiStateDescriptor, stored: unkn } /** - * Whether a value equals the descriptor's default. Default values are not - * stored at all, which keeps storage lean and makes "reset" a removal. + * Whether a value equals the descriptor's default. Unless persistDefault is + * set, defaults are not stored, keeping storage lean and "reset" a removal. */ export function isDefaultUiValue(descriptor: UiStateDescriptor, value: T): boolean { const defaults: unknown = descriptor.defaultValue diff --git a/app/ui_layer/browser/frontend/src/store/uiState/legacyMigration.ts b/app/ui_layer/browser/frontend/src/store/uiState/legacyMigration.ts index 702409b90..3ab5c5fc8 100644 --- a/app/ui_layer/browser/frontend/src/store/uiState/legacyMigration.ts +++ b/app/ui_layer/browser/frontend/src/store/uiState/legacyMigration.ts @@ -58,6 +58,23 @@ const LEGACY_RULES: LegacyRule[] = [ exactKey('lastSeenMessageIdBySession', UI_STATE.chat.lastSeenMessageIds, json), ] +/** Promote a family's former lifetime without replacing newer shared state. + * Keep the source when storage is unavailable or full, so retry cannot lose it. + * Families retaining their defaults also preserve resets against stale tabs. */ +export function migrateUiStateFamily( + source: UiStorage, + target: UiStorage, + family: (id: string) => UiStateDescriptor, +): void { + const prefix = family('').key + for (const [key, value] of Object.entries(source.readAll())) { + if (!key.startsWith(prefix)) continue + const descriptor = family(key.slice(prefix.length)) + if (!descriptor.isValid(target.read(key)) && descriptor.isValid(value)) target.write(key, value) + if (descriptor.isValid(target.read(key)) || !descriptor.isValid(value)) source.remove(key) + } +} + export function migrateLegacyUiStorage( target: UiStorage, resolveLegacyArea: () => Storage = () => window.localStorage, diff --git a/app/ui_layer/browser/frontend/src/store/uiState/persistence.ts b/app/ui_layer/browser/frontend/src/store/uiState/persistence.ts index a2c9afcf3..43503a6f6 100644 --- a/app/ui_layer/browser/frontend/src/store/uiState/persistence.ts +++ b/app/ui_layer/browser/frontend/src/store/uiState/persistence.ts @@ -1,7 +1,8 @@ import type { Middleware } from '@reduxjs/toolkit' import { uiStateChanged, uiStateSynced, type UiSliceState } from '../slices/uiSlice' import type { UiStateLifetime } from './defineUiState' -import { migrateLegacyUiStorage } from './legacyMigration' +import { migrateLegacyUiStorage, migrateUiStateFamily } from './legacyMigration' +import { UI_STATE } from './catalog' import { UiStorage } from './UiStorage' export type UiStorages = Readonly> @@ -36,6 +37,10 @@ export class UiStatePersister { } } + discard(key: string): void { + this.pending.delete(key) + } + flush(): void { if (this.timer !== null) { clearTimeout(this.timer) @@ -56,6 +61,7 @@ export class UiStatePersister { */ export function loadPersistedUiState(storages: UiStorages = uiStorages): UiSliceState { migrateLegacyUiStorage(storages.preference) + migrateUiStateFamily(storages.session, storages.preference, UI_STATE.chat.artifactState) return { values: { ...storages.preference.readAll(), ...storages.session.readAll() }, } @@ -77,7 +83,13 @@ export function createUiPersistenceMiddleware(storages: UiStorages = uiStorages) window.addEventListener('storage', (event) => { const key = storages.preference.keyOf(event) if (key !== null) { - store.dispatch(uiStateSynced({ key, value: storages.preference.parse(event.newValue) })) + // Events may be delivered after this tab has already saved a newer + // value. Read current storage and avoid restarting an unchanged preview. + const value = storages.preference.read(key) + const current = (store.getState() as { ui: UiSliceState }).ui.values[key] + if (JSON.stringify(current) === JSON.stringify(value)) return + persister.discard(key) + store.dispatch(uiStateSynced({ key, value })) } }) @@ -88,7 +100,7 @@ export function createUiPersistenceMiddleware(storages: UiStorages = uiStorages) persister.enqueue(key, lifetime, value) // Read markers are discrete acknowledgements, not continuous UI // changes. Do not leave them waiting in the coalescing timer. - if (key === 'chat.lastSeenMessageIds') persister.flush() + if (action.payload.writeThrough || key === 'chat.lastSeenMessageIds') persister.flush() } return result } diff --git a/app/ui_layer/browser/frontend/src/types/index.ts b/app/ui_layer/browser/frontend/src/types/index.ts index b708c137c..8912a60c6 100644 --- a/app/ui_layer/browser/frontend/src/types/index.ts +++ b/app/ui_layer/browser/frontend/src/types/index.ts @@ -43,6 +43,7 @@ export interface ChatMessage { } export interface GenerativeUIArtifact { + persistence?: 'controls' | 'manual' | 'none' id: string title: string html: string diff --git a/app/ui_layer/browser/frontend/tests/agent-app-media.html b/app/ui_layer/browser/frontend/tests/agent-app-media.html new file mode 100644 index 000000000..de52c43d1 --- /dev/null +++ b/app/ui_layer/browser/frontend/tests/agent-app-media.html @@ -0,0 +1,29 @@ + +Agent App media QA + +

Agent App media QA

+ + +Waiting + + diff --git a/app/ui_layer/browser/frontend/tests/agent-app-media.test.ts b/app/ui_layer/browser/frontend/tests/agent-app-media.test.ts new file mode 100644 index 000000000..5c4c65a5c --- /dev/null +++ b/app/ui_layer/browser/frontend/tests/agent-app-media.test.ts @@ -0,0 +1,29 @@ +import { expect, test } from '@playwright/test' + +// These flags provide synthetic devices and synthetic screen frames. The +// tests never access the user's microphone, camera or desktop. +test.use({ + launchOptions: { + args: [ + '--use-fake-device-for-media-stream', + '--use-fake-ui-for-media-stream', + '--auto-select-desktop-capture-source=Entire screen', + ], + }, + permissions: ['camera', 'microphone'], +}) + +test('Agent App frames delegate microphone and camera across origins', async ({ page }) => { + await page.goto('/tests/agent-app-media.html') + const frame = page.frameLocator('iframe[title="Cross-origin Agent App media fixture"]') + await frame.getByRole('button', { name: 'Test microphone and camera', exact: true }).click() + await expect(frame.locator('#result')).toHaveText('audio,video') + await expect(page.locator('iframe')).toHaveAttribute('allow', /microphone; camera; display-capture/) +}) + +test('Agent App frames delegate screen capture across origins', async ({ page }) => { + await page.goto('/tests/agent-app-media.html') + const frame = page.frameLocator('iframe[title="Cross-origin Agent App media fixture"]') + await frame.getByRole('button', { name: 'Test screen capture', exact: true }).click() + await expect(frame.locator('#result')).toHaveText('video') +}) diff --git a/app/ui_layer/browser/frontend/tests/generative-ui.test.ts b/app/ui_layer/browser/frontend/tests/generative-ui.test.ts index bafd7db14..cceca27d2 100644 --- a/app/ui_layer/browser/frontend/tests/generative-ui.test.ts +++ b/app/ui_layer/browser/frontend/tests/generative-ui.test.ts @@ -26,6 +26,204 @@ async function custom(page: Page, html: string, connect_origins: string[] = [], const REPORT_BLOCKED = `` const blocked = (page: Page) => preview(page).locator('html') +const cookingStateKey = 'craftbot.ui.chat.artifactState.["demo","cooking"]' +const controlsHTML = ` +` + +test('progress follows new tabs, edits and resets without restarting local interactions', async ({ page, context }) => { + await open(page) + const original = await page.locator('iframe').getAttribute('srcdoc') + await preview(page).getByLabel('Servings').fill('5') + await preview(page).getByLabel('Servings').press('Tab') + await preview(page).getByRole('button', { name: 'Next step' }).click() + expect(await page.locator('iframe').getAttribute('srcdoc')).toBe(original) + await expect.poll(() => page.evaluate(key => JSON.parse(localStorage.getItem(key) || '{}').step, cookingStateKey)).toBe(1) + const other = await context.newPage() // No opener: sessionStorage cannot be copied. + await open(other) + await expect(preview(other).getByLabel('Servings')).toHaveValue('5') + await expect(preview(other).getByRole('heading', { name: 'Step 2 of 4' })).toBeVisible() + await preview(other).getByRole('button', { name: 'Next step' }).click() + await expect(preview(page).getByRole('heading', { name: 'Step 3 of 4' })).toBeVisible() + await page.getByRole('button', { name: 'Switch session' }).click() + await expect(preview(page).getByLabel('Servings')).toHaveValue('2') + await page.getByRole('button', { name: 'Switch session' }).click() + await expect(preview(page).getByLabel('Servings')).toHaveValue('5') + await previewAction(other, 'Reset saved progress') + await expect(preview(page).getByLabel('Servings')).toHaveValue('2') + await expect(preview(page).getByRole('heading', { name: 'Step 1 of 4' })).toBeVisible() + await other.close() +}) + +test('native controls and derived output hydrate when another tab saves', async ({ page, context }) => { + await open(page) + await custom(page, controlsHTML, [], { persistence: 'controls' }) + const other = await context.newPage() + await open(other) + await custom(other, controlsHTML, [], { persistence: 'controls' }) + await preview(page).getByLabel('Count').fill('9') + await expect(preview(other).getByLabel('Count')).toHaveValue('9') + await expect(preview(other).locator('#total')).toHaveText('27') + await preview(other).getByLabel('Count').fill('11') + await expect(preview(page).getByLabel('Count')).toHaveValue('11') + await expect(preview(page).locator('#total')).toHaveText('33') + await other.getByRole('button', { name: 'Cooking', exact: true }).click() + await expect(preview(other).getByLabel('Servings')).toHaveValue('2') + await other.close() +}) + +test('manual and native progress survive closing Chromium and reopening its disk profile', async ({ browser }, testInfo) => { + const profile = testInfo.outputPath('persistent-profile') + let context = await browser.browserType().launchPersistentContext(profile, { headless: true }) + try { + let page = await context.newPage() + await open(page) + await preview(page).getByLabel('Servings').fill('6') + await preview(page).getByLabel('Servings').press('Tab') + await preview(page).getByRole('checkbox').first().check() + await preview(page).getByRole('button', { name: 'Next step' }).click() + await expect.poll(() => page.evaluate(key => JSON.parse(localStorage.getItem(key) || '{}').step, cookingStateKey)).toBe(1) + await custom(page, controlsHTML, [], { persistence: 'controls' }) + await preview(page).getByLabel('Count').fill('13') + await expect.poll(() => page.evaluate(() => JSON.parse(localStorage.getItem('craftbot.ui.chat.artifactState.["demo","custom"]') || '{}').count)).toBe('13') + await context.close() + context = await browser.browserType().launchPersistentContext(profile, { headless: true }) + page = await context.newPage() + await open(page) + await expect(preview(page).getByLabel('Servings')).toHaveValue('6') + await expect(preview(page).getByRole('checkbox').first()).toBeChecked() + await expect(preview(page).getByRole('heading', { name: 'Step 2 of 4' })).toBeVisible() + await custom(page, controlsHTML, [], { persistence: 'controls' }) + await expect(preview(page).getByLabel('Count')).toHaveValue('13') + await expect(preview(page).locator('#total')).toHaveText('39') + } finally { + await context.close() + } +}) + +test('legacy tab progress migrates once and never overwrites durable progress', async ({ page }) => { + await page.addInitScript(key => { + sessionStorage.setItem(key, JSON.stringify({ servings: 4, step: 1 })) + }, cookingStateKey) + await open(page) + await expect(preview(page).getByLabel('Servings')).toHaveValue('4') + expect(await page.evaluate(key => sessionStorage.getItem(key), cookingStateKey)).toBeNull() + await preview(page).getByLabel('Servings').fill('7') + await preview(page).getByLabel('Servings').press('Tab') + await expect.poll(() => page.evaluate(key => JSON.parse(localStorage.getItem(key) || '{}').servings, cookingStateKey)).toBe(7) + await page.reload() // Deliberately reintroduces an old tab's stale value. + await expect(preview(page).getByLabel('Servings')).toHaveValue('7') +}) + +test('valid legacy progress replaces malformed durable data', async ({ page }) => { + await page.addInitScript(key => { + sessionStorage.setItem(key, JSON.stringify({ servings: 4 })) + localStorage.setItem(key, '[1,2]') + }, cookingStateKey) + await open(page) + await expect(preview(page).getByLabel('Servings')).toHaveValue('4') + expect(await page.evaluate(key => JSON.parse(localStorage.getItem(key) || '{}').servings, cookingStateKey)).toBe(4) +}) + +test('an older buffered write cannot undo a reset from another tab', async ({ page, context }) => { + await page.clock.install() + await open(page) + await page.clock.pauseAt(new Date()) + await preview(page).getByLabel('Servings').fill('4') + await preview(page).getByLabel('Servings').press('Tab') + await expect(preview(page).getByText('400 g pasta')).toBeVisible() + const other = await context.newPage() + await open(other) + await previewAction(other, 'Reset saved progress') + await other.evaluate(() => window.dispatchEvent(new Event('pagehide'))) + await expect(preview(page).getByLabel('Servings')).toHaveValue('2') + await page.clock.runFor(500) + expect(await page.evaluate(key => localStorage.getItem(key), cookingStateKey)).toBe('{}') + await other.close() +}) + +for (const action of ['reset', 'close']) test(`durable ${action} cannot resurrect progress from a stale legacy tab`, async ({ page }) => { + await page.addInitScript(key => sessionStorage.setItem(key, JSON.stringify({ servings: 8 })), cookingStateKey) + await open(page) + await expect(preview(page).getByLabel('Servings')).toHaveValue('8') + if (action === 'reset') await previewAction(page, 'Reset saved progress') + else await page.getByRole('button', { name: 'Close Cooking guide', exact: true }).click() + await expect.poll(() => page.evaluate(key => localStorage.getItem(key), cookingStateKey)).toBe('{}') + await page.reload() + if (action === 'close') await outputsButton(page).click() + await expect(preview(page).getByLabel('Servings')).toHaveValue('2') +}) + +test('a new tab sees the latest progress without waiting for a flush timer', async ({ page, context }) => { + await page.clock.install() + await open(page) + await page.clock.pauseAt(new Date()) + await preview(page).getByLabel('Servings').fill('8') + await preview(page).getByLabel('Servings').press('Tab') + await expect(preview(page).getByText('800 g pasta')).toBeVisible() + expect(await page.evaluate(key => JSON.parse(localStorage.getItem(key) || '{}').servings, cookingStateKey)).toBe(8) + const other = await context.newPage() + await open(other) + await expect(preview(other).getByLabel('Servings')).toHaveValue('8') + await other.close() +}) + +test('delayed storage notifications cannot roll back newer local progress', async ({ page }) => { + await open(page) + await preview(page).getByLabel('Servings').fill('9') + await preview(page).getByLabel('Servings').press('Tab') + await expect(preview(page).getByText('900 g pasta')).toBeVisible() + await page.evaluate(key => window.dispatchEvent(new StorageEvent('storage', { + key, storageArea: localStorage, oldValue: '{}', newValue: '{"servings":4}', + })), cookingStateKey) + await expect(preview(page).getByLabel('Servings')).toHaveValue('9') + await expect.poll(() => page.evaluate(key => JSON.parse(localStorage.getItem(key) || '{}').servings, cookingStateKey)).toBe(9) +}) + +for (const event of ['chat_clear', 'session_cleared', 'session_deleted']) test(`${event} resets saved outputs outside loaded history and isolates other chats`, async ({ page, context }) => { + await open(page) + await preview(page).getByLabel('Servings').fill('7') + await preview(page).getByLabel('Servings').press('Tab') + await expect(preview(page).getByText('700 g pasta')).toBeVisible() + const other = await context.newPage() + await open(other) + await expect(preview(other).getByLabel('Servings')).toHaveValue('7') + await page.evaluate(async event => { + const { store } = await import('./harness.tsx') + const { setUiState } = await import('../src/store/slices/uiSlice') + const { UI_STATE } = await import('../src/store/uiState') + const { dispatchInbound } = await import('../src/store/socket/messageRegistry.ts') + store.dispatch(setUiState(UI_STATE.chat.artifactState('["demo","older-output"]'), { count: 17 })) + store.dispatch(setUiState(UI_STATE.chat.artifactState('["other","cooking"]'), { servings: 6 })) + dispatchInbound({ type: event, data: { sessionId: 'demo' } }, store.dispatch, store.getState as never) + }, event) + await expect(preview(other).getByLabel('Servings')).toHaveValue('2') + await other.close() + await expect(preview(page).getByLabel('Servings')).toHaveValue('2') + await expect.poll(() => page.evaluate(() => [ + localStorage.getItem('craftbot.ui.chat.artifactState.["demo","cooking"]'), + localStorage.getItem('craftbot.ui.chat.artifactState.["demo","older-output"]'), + JSON.parse(localStorage.getItem('craftbot.ui.chat.artifactState.["other","cooking"]') || '{}').servings, + ])).toEqual(['{}', '{}', 6]) + await page.reload() + await expect(preview(page).getByLabel('Servings')).toHaveValue('2') +}) + +test('a full durable storage area preserves legacy progress for a later retry', async ({ page }) => { + await page.addInitScript(key => { + sessionStorage.setItem(key, JSON.stringify({ servings: 4 })) + const write = Storage.prototype.setItem + Storage.prototype.setItem = function (key, value) { + if (this === localStorage) throw new DOMException('Full', 'QuotaExceededError') + write.call(this, key, value) + } + }, cookingStateKey) + await open(page) + await expect(preview(page).getByLabel('Servings')).toHaveValue('4') + expect(await page.evaluate(key => JSON.parse(sessionStorage.getItem(key) || '{}').servings, cookingStateKey)).toBe(4) + await preview(page).getByRole('button', { name: 'Next step' }).click() + await expect(preview(page).getByRole('heading', { name: 'Step 2 of 4' })).toBeVisible() +}) + test('cooking controls persist across unmount, reload and revision; reset clears progress', async ({ page }) => { await open(page) await preview(page).getByLabel('Servings').fill('4') @@ -41,7 +239,7 @@ test('cooking controls persist across unmount, reload and revision; reset clears await expect(preview(page).locator('#timer')).toContainText('remaining') await page.getByRole('button', { name: 'Revise interface' }).click() await expect(preview(page).getByLabel('Servings')).toHaveValue('4') - await expect.poll(() => page.evaluate(() => Object.keys(sessionStorage).some(key => key.includes('artifactState')))).toBe(true) + await expect.poll(() => page.evaluate(() => Object.keys(localStorage).some(key => key.includes('artifactState')))).toBe(true) await page.reload() await expect(preview(page).getByLabel('Servings')).toHaveValue('4') await previewAction(page, 'Reset saved progress') @@ -429,7 +627,7 @@ test("outputs are tabs like the browser's, with reload and options at the strip' test('closing an output tab disposes of it: its progress and tab go, the last one closes the panel', async ({ page }) => { await open(page) await page.evaluate(output => window.dispatchEvent(new CustomEvent('fixture-history', { detail: [output] })), notesOutput) - const savedCooking = () => page.evaluate(() => Object.keys(sessionStorage).filter(key => key.includes('artifactState') && key.includes('cooking')).length) + const savedCooking = () => page.evaluate(() => Object.keys(JSON.parse(localStorage.getItem('craftbot.ui.chat.artifactState.["demo","cooking"]') || '{}')).length > 0 ? 1 : 0) await preview(page).getByRole('button', { name: 'Next step' }).click() await expect.poll(savedCooking).toBe(1) const pane = page.getByRole('region', { name: 'Output panel' }) @@ -605,7 +803,7 @@ test('page errors stay out of the panel, forged messages are ignored and oversiz await previewAction(page, 'Reload preview') await expect(preview(page).getByText('Broken example')).toBeVisible() await expect(page.getByRole('alert')).toHaveCount(0) - expect(await page.evaluate(() => JSON.stringify(sessionStorage))).not.toContain('forged') + expect(await page.evaluate(() => JSON.stringify(localStorage))).not.toContain('forged') }) test('document attributes and state strings survive rendering without executing injected markup', async ({ page }) => { @@ -799,3 +997,58 @@ test('stored images reach the page as window.craftbot.images; an unavailable one await page.getByRole('button', { name: 'Try again' }).click() await expect(preview(page).locator('#names')).toHaveText('hero') }) + + +test('declared native controls persist without author save handlers and hydrate before derived scripts', async ({ page }) => { + await open(page) + const html = `
+ + + + + + + + + +
` + await custom(page, html, [], { persistence: 'controls' }) + await preview(page).getByLabel('Budget', { exact: true }).fill('37') + await preview(page).getByLabel('Done', { exact: true }).check() + await preview(page).getByLabel('Notes', { exact: true }).fill('Saved notes') + await preview(page).getByLabel('City', { exact: true }).selectOption('par') + await preview(page).getByLabel('B', { exact: true }).check() + await preview(page).getByLabel('Tags', { exact: true }).selectOption(['x', 'y']) + await preview(page).locator('#secret').fill('never persist me') + await previewAction(page, 'Reload preview') + await expect(preview(page).getByLabel('Budget', { exact: true })).toHaveValue('37') + await expect(preview(page).getByLabel('Done', { exact: true })).toBeChecked() + await expect(preview(page).getByLabel('Notes', { exact: true })).toHaveValue('Saved notes') + await expect(preview(page).getByLabel('City', { exact: true })).toHaveValue('par') + await expect(preview(page).getByLabel('B', { exact: true })).toBeChecked() + await expect(preview(page).getByLabel('Tags', { exact: true })).toHaveValues(['x', 'y']) + await expect(preview(page).locator('#secret')).toHaveValue('') + await expect(preview(page).locator('#derived')).toHaveText('111') + // A revision changes markup but keeps the declared stable keys. + await custom(page, html.replace('value="10"', 'value="20"'), [], { persistence: 'controls', revision: 2 }) + await expect(preview(page).getByLabel('Budget', { exact: true })).toHaveValue('37') + await expect(preview(page).locator('#derived')).toHaveText('111') + await previewAction(page, 'Reset saved progress') + await expect(preview(page).getByLabel('Budget', { exact: true })).toHaveValue('20') + await expect(preview(page).getByLabel('Done', { exact: true })).not.toBeChecked() +}) + + +test('implicit option values and empty radio values restore with native HTML semantics', async ({ page }) => { + await open(page) + await custom(page, ` + + + `, [], { persistence: 'controls' }) + await preview(page).getByLabel('Destination').selectOption({ label: 'New York' }) + await preview(page).getByLabel('Set', { exact: true }).check() + await preview(page).getByLabel('Unset', { exact: true }).check() + await previewAction(page, 'Reload preview') + await expect(preview(page).getByLabel('Destination')).toHaveValue('New York') + await expect(preview(page).getByLabel('Unset', { exact: true })).toBeChecked() +}) diff --git a/app/ui_layer/browser/frontend/tests/harness.tsx b/app/ui_layer/browser/frontend/tests/harness.tsx index a30e691e1..f9e064ef5 100644 --- a/app/ui_layer/browser/frontend/tests/harness.tsx +++ b/app/ui_layer/browser/frontend/tests/harness.tsx @@ -6,8 +6,10 @@ import { configureStore } from '@reduxjs/toolkit' import { Provider } from 'react-redux' import ui, { setUiState } from '../src/store/slices/uiSlice' import agent from '../src/store/slices/agentSlice' +import messages from '../src/store/slices/messagesSlice' import { UI_STATE, createUiPersistenceMiddleware, loadPersistedUiState } from '../src/store/uiState' import { OutputWorkspace, type BrowserPanelMode, type PanelLayout } from '../src/components/GenerativeUI/OutputWorkspace' +import { QuestionBox } from '../src/components/Chat/QuestionBox' import { ChatMessageItem } from '../src/pages/Chat/ChatMessage' import type { GenerativeUIArtifact, ChatMessage } from '../src/types' import i18n from '../src/i18n/config' @@ -16,7 +18,7 @@ import cooking from './fixtures/cooking.html?raw' import weather from './fixtures/weather.html?raw' import design from './fixtures/design.html?raw' -const store = configureStore({ reducer: { ui, agent }, preloadedState: { ui: loadPersistedUiState(), agent: { ...agent(undefined, { type: '@@init' }), profilePictureUrl: 'data:image/svg+xml,%3Csvg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32"%3E%3Ccircle cx="16" cy="16" r="14" fill="%23FF4F18"/%3E%3C/svg%3E' } }, middleware: getDefault => getDefault().concat(createUiPersistenceMiddleware()) }) +export const store = configureStore({ reducer: { ui, agent, messages }, preloadedState: { ui: loadPersistedUiState(), agent: { ...agent(undefined, { type: '@@init' }), profilePictureUrl: 'data:image/svg+xml,%3Csvg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32"%3E%3Ccircle cx="16" cy="16" r="14" fill="%23FF4F18"/%3E%3C/svg%3E' } }, middleware: getDefault => getDefault().concat(createUiPersistenceMiddleware()) }) const examples: Record = { Cooking: { id: 'cooking', title: 'Cooking guide', html: cooking, revision: 1, connect_origins: [] }, Weather: { id: 'weather', title: 'Weather explorer', html: weather, revision: 1, connect_origins: ['https://api.open-meteo.com', 'https://geocoding-api.open-meteo.com'] }, @@ -85,4 +87,20 @@ function Harness() {
} -createRoot(document.getElementById('root')!).render() +// Match the real chat's keyed question replacement: a resolved question +// immediately reveals the next one at the same place. +function QuestionHarness() { + const [resolved, setResolved] = useState<{ id: string; value: string }[]>([]) + const index = resolved.length + 1 + const question: ChatMessage = { + messageId: `question-${index}`, sessionId: 'demo', sender: 'CraftBot', + content: `Question ${index}?`, timestamp: index, style: 'agent', isQuestion: true, + options: [{ label: `Choice ${index}`, value: `choice-${index}` }], + } + const answer = (value: string) => setResolved(items => [...items, { id: question.messageId, value }]) + return
+ {index <= 3 && answer('__dismissed__')} />} + {JSON.stringify(resolved)} +
+} +createRoot(document.getElementById('root')!).render({new URLSearchParams(location.search).has('questions') ? : }) diff --git a/app/ui_layer/browser/frontend/tests/question-queue.test.ts b/app/ui_layer/browser/frontend/tests/question-queue.test.ts new file mode 100644 index 000000000..cd923ea38 --- /dev/null +++ b/app/ui_layer/browser/frontend/tests/question-queue.test.ts @@ -0,0 +1,53 @@ +import { test, expect, type Page } from '@playwright/test' + +const question = (page: Page) => page.getByRole('region', { name: 'Question from agent' }) +const resolved = (page: Page) => page.getByRole('status', { name: 'Resolved questions' }) +const answer = (id: number, value = `choice-${id}`) => ({ id: `question-${id}`, value }) + +async function open(page: Page) { + await page.goto('/tests/generative-ui.html?questions') + await expect(question(page).getByText('Question 1?', { exact: true })).toBeVisible() +} + +for (const count of [2, 3]) test(`${count} clicks cannot answer the next queued question`, async ({ page }) => { + await open(page) + await question(page).getByRole('button', { name: 'Choice 1 (recommended)', exact: true }).click({ clickCount: count }) + await expect(resolved(page)).toHaveText(JSON.stringify([answer(1)])) + await expect(question(page).getByText('Question 2?', { exact: true })).toBeVisible() + // A deliberate new click works immediately, without a cooldown timer. + await question(page).getByRole('button', { name: 'Choice 2 (recommended)', exact: true }).click() + await expect(resolved(page)).toHaveText(JSON.stringify([answer(1), answer(2)])) + await expect(question(page).getByText('Question 3?', { exact: true })).toBeVisible() +}) + +test('double-clicking dismiss cannot dismiss the next question', async ({ page }) => { + await open(page) + await question(page).getByRole('button', { name: 'Dismiss question', exact: true }).dblclick() + await expect(resolved(page)).toHaveText(JSON.stringify([answer(1, '__dismissed__')])) + await expect(question(page).getByText('Question 2?', { exact: true })).toBeVisible() + await question(page).getByRole('button', { name: 'Dismiss question', exact: true }).click() + await expect(resolved(page)).toHaveText(JSON.stringify([answer(1, '__dismissed__'), answer(2, '__dismissed__')])) +}) + +test('double-clicking send cannot submit a saved draft for the next question', async ({ page }) => { + await page.addInitScript(() => sessionStorage.setItem('craftbot.ui.chat.questionAnswerDraft.question-2', JSON.stringify('Unreviewed draft'))) + await open(page) + await question(page).getByRole('textbox').fill('First answer') + await question(page).getByRole('button', { name: 'Send answer', exact: true }).dblclick() + await expect(resolved(page)).toHaveText(JSON.stringify([answer(1, 'First answer')])) + await expect(question(page).getByRole('textbox')).toHaveValue('Unreviewed draft') + await question(page).getByRole('button', { name: 'Send answer', exact: true }).click() + await expect(resolved(page)).toHaveText(JSON.stringify([answer(1, 'First answer'), answer(2, 'Unreviewed draft')])) +}) + +test('keyboard Enter, Space and a typed answer each resolve only their own question', async ({ page }) => { + await open(page) + await question(page).getByRole('button', { name: 'Choice 1 (recommended)', exact: true }).press('Enter') + await expect(resolved(page)).toHaveText(JSON.stringify([answer(1)])) + await question(page).getByRole('button', { name: 'Choice 2 (recommended)', exact: true }).press('Space') + await expect(resolved(page)).toHaveText(JSON.stringify([answer(1), answer(2)])) + await question(page).getByRole('textbox').fill('Keyboard answer') + await question(page).getByRole('textbox').press('Enter') + await expect(resolved(page)).toHaveText(JSON.stringify([answer(1), answer(2), answer(3, 'Keyboard answer')])) + await expect(question(page)).toHaveCount(0) +}) diff --git a/app/ui_layer/commands/builtin/clear.py b/app/ui_layer/commands/builtin/clear.py index 684b10927..199694870 100644 --- a/app/ui_layer/commands/builtin/clear.py +++ b/app/ui_layer/commands/builtin/clear.py @@ -44,22 +44,27 @@ async def execute( # Start the session over on the agent side (stops a run in flight, # resets its context and files). First, so the stopped run's last # rows are cleared with the rest of the conversation. - await self._controller.agent.clear_session(target) + if not await self._controller.agent.clear_session(target): + return CommandResult(success=False, message="Session could not be cleared.") + await self._controller.event_bus.drain(target) # Clear persisted chat + activity rows for this session from app.usage import get_action_storage, get_chat_storage - get_chat_storage().clear_messages(target) + adapter = self._controller.active_adapter + if adapter: + # Use the adapter's ordered worker so an insert already in flight + # cannot land after the delete and restore the old conversation. + await adapter.chat_component.clear(target) + else: + get_chat_storage().clear_messages(target) get_action_storage().clear_items(target) # Tell the UI to drop the session's rendered conversation. Always # session-scoped: a /clear must never touch other sessions. - adapter = self._controller.active_adapter broadcast = getattr(adapter, "broadcast_session_cleared", None) if broadcast is not None: await broadcast(target) - elif adapter: - await adapter.chat_component.clear(target) # Confirm in the now-empty conversation (emitted after the clear so # it survives instead of being wiped with the old rows). diff --git a/app/ui_layer/controller/ui_controller.py b/app/ui_layer/controller/ui_controller.py index 45cb1157c..cc9f7c140 100644 --- a/app/ui_layer/controller/ui_controller.py +++ b/app/ui_layer/controller/ui_controller.py @@ -364,6 +364,9 @@ async def submit_question_answer( doesn't re-ask questions that are still pinned in the UI. """ if dismissed: + # Share the stop button's cancellation and continuation cleanup, + # including runs parked between turns rather than actively busy. + await self.stop_run(session_id) # One event, two audiences (as with the stop button): the SYSTEM # bubble tells the user the interruption landed; the stream copy # tells the agent's next run why its question went unanswered. diff --git a/app/ui_layer/events/event_bus.py b/app/ui_layer/events/event_bus.py index 54f0ef3ab..8a597c0bd 100644 --- a/app/ui_layer/events/event_bus.py +++ b/app/ui_layer/events/event_bus.py @@ -9,7 +9,7 @@ from app.ui_layer.events.event_types import UIEvent, UIEventType # Type aliases for event handlers -EventHandler = Callable[[UIEvent], None] +EventHandler = Callable[[UIEvent], Optional[asyncio.Task]] AsyncEventHandler = Callable[[UIEvent], Coroutine[Any, Any, None]] @@ -51,6 +51,29 @@ def __init__(self, max_history: int = 1000) -> None: self._global_async_handlers: List[AsyncEventHandler] = [] self._event_history: List[UIEvent] = [] self._max_history = max_history + self._pending: Dict[asyncio.Task, Optional[str]] = {} + + def _schedule(self, handler: AsyncEventHandler, event: UIEvent) -> None: + self._track(asyncio.create_task(handler(event)), event) + + def _track(self, task: asyncio.Task, event: UIEvent) -> None: + self._pending[task] = event.task_id or event.data.get("session_id") or "main" + task.add_done_callback(lambda done: self._pending.pop(done, None)) + + def _invoke(self, handler: EventHandler, event: UIEvent) -> None: + # Synchronous adapters return their display task so it has the same + # ownership as a handler registered with subscribe_async. + result = handler(event) + if isinstance(result, asyncio.Task): + self._track(result, event) + + async def drain(self, task_id: str) -> None: + """Settle this session's already-emitted UI work before clearing it.""" + current = asyncio.current_task() + while tasks := [task for task, sid in self._pending.items() + if sid == task_id and task is not current]: + await asyncio.gather(*(asyncio.shield(task) for task in tasks), + return_exceptions=True) def subscribe( self, @@ -152,7 +175,7 @@ def emit(self, event: UIEvent) -> None: # Call type-specific sync handlers for handler in self._handlers.get(event.type, []): try: - handler(event) + self._invoke(handler, event) except Exception: # Log but don't crash - handlers shouldn't break the bus pass @@ -160,14 +183,14 @@ def emit(self, event: UIEvent) -> None: # Call global sync handlers for handler in self._global_handlers: try: - handler(event) + self._invoke(handler, event) except Exception: pass # Schedule type-specific async handlers for handler in self._async_handlers.get(event.type, []): try: - asyncio.create_task(handler(event)) + self._schedule(handler, event) except RuntimeError: # No event loop running - skip async handlers pass @@ -175,7 +198,7 @@ def emit(self, event: UIEvent) -> None: # Schedule global async handlers for handler in self._global_async_handlers: try: - asyncio.create_task(handler(event)) + self._schedule(handler, event) except RuntimeError: pass @@ -197,13 +220,13 @@ async def emit_async(self, event: UIEvent) -> None: # Call sync handlers for handler in self._handlers.get(event.type, []): try: - handler(event) + self._invoke(handler, event) except Exception: pass for handler in self._global_handlers: try: - handler(event) + self._invoke(handler, event) except Exception: pass diff --git a/app/usage/reporter.py b/app/usage/reporter.py index 3a06c0d5c..8171e14b3 100644 --- a/app/usage/reporter.py +++ b/app/usage/reporter.py @@ -10,6 +10,7 @@ import asyncio import logging +import threading from typing import List, Optional from agent_core.core.hooks.types import UsageEventData @@ -56,7 +57,7 @@ def __init__(self, storage: Optional[UsageStorage] = None) -> None: """ self._storage = storage or get_usage_storage() self._buffer: List[UsageEvent] = [] - self._buffer_lock = asyncio.Lock() + self._buffer_lock = threading.RLock() self._flush_task: Optional[asyncio.Task] = None self._started = False @@ -70,11 +71,10 @@ def start_background_flush(self) -> None: """Start the background flush task.""" if self._started: return - self._started = True - try: - loop = asyncio.get_event_loop() + loop = asyncio.get_running_loop() self._flush_task = loop.create_task(self._background_flush_loop()) + self._started = True logger.info("[UsageReporter] Background flush task started") except RuntimeError: # No event loop running - will flush on demand @@ -108,7 +108,7 @@ async def report(self, event: UsageEventData) -> None: cached_tokens=event.cached_tokens, ) - async with self._buffer_lock: + with self._buffer_lock: self._buffer.append(usage_event) # Log immediately for debugging @@ -119,9 +119,9 @@ async def report(self, event: UsageEventData) -> None: f"cached={event.cached_tokens}, total={total_tokens})" ) - # Flush if buffer is large enough - if len(self._buffer) >= self.BATCH_SIZE: - asyncio.create_task(self.flush()) + should_flush = len(self._buffer) >= self.BATCH_SIZE + if should_flush: + await self.flush() async def report_full( self, @@ -152,15 +152,16 @@ async def report_full( session_id=session_id, ) - async with self._buffer_lock: + with self._buffer_lock: self._buffer.append(usage_event) - if len(self._buffer) >= self.BATCH_SIZE: - asyncio.create_task(self.flush()) + should_flush = len(self._buffer) >= self.BATCH_SIZE + if should_flush: + await self.flush() async def flush(self) -> None: """Save buffered events to storage.""" - async with self._buffer_lock: + with self._buffer_lock: if not self._buffer: return @@ -172,7 +173,8 @@ async def flush(self) -> None: try: count = self._storage.insert_events_batch(events) - self._total_reported += count + with self._buffer_lock: + self._total_reported += count logger.info( f"[UsageReporter] Saved {count} usage events " f"(total reported: {self._total_reported})" @@ -181,7 +183,7 @@ async def flush(self) -> None: logger.error(f"[UsageReporter] Failed to save events: {e}") self._total_failed += len(events) # Re-add failed events to buffer - async with self._buffer_lock: + with self._buffer_lock: self._buffer.extend(events) def get_stats(self) -> dict: @@ -204,6 +206,11 @@ async def shutdown(self) -> None: except asyncio.CancelledError: pass + from agent_core.core.hooks.usage import drain_usage + + await drain_usage() + self._started = False + # Flush remaining events try: await self.flush() diff --git a/craftbot.py b/craftbot.py index 98135f1a6..eb8c91bd9 100644 --- a/craftbot.py +++ b/craftbot.py @@ -230,13 +230,9 @@ def _enable_windows_vtp() -> None: _enable_windows_vtp() -# Force UTF-8 output on Windows so Unicode box-drawing characters don't crash -if sys.platform == "win32": - try: - sys.stdout.reconfigure(encoding="utf-8", errors="replace") - sys.stderr.reconfigure(encoding="utf-8", errors="replace") - except Exception: - pass +from app.console import configure_console # noqa: E402 -- after windowed stream initialization + +configure_console() _USE_COLOR = sys.stdout.isatty() diff --git a/main.py b/main.py index 29189f87c..9c12af09a 100644 --- a/main.py +++ b/main.py @@ -7,6 +7,9 @@ import socket import signal import threading +from app.console import configure_console + +configure_console() # --- CONFIGURATION --- # Path to the directory containing the docker-compose.yml file diff --git a/pytest.ini b/pytest.ini index ae89a87c0..6866f48c2 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,8 +1,8 @@ [pytest] markers = - live: real end-to-end tests that hit live integrations and the real LLM (opt-in: `pytest -m live`) + live: real integrations and LLM (opt-in: explicit CRAFTBOT_HOME and `pytest --live-state -m live`) testpaths = tests # By default, skip live tests so plain `pytest` stays fast and offline. -addopts = -m "not live" +addopts = --import-mode=importlib -m "not live" diff --git a/run.py b/run.py index 6bc7dddfb..c94a507f5 100644 --- a/run.py +++ b/run.py @@ -41,6 +41,9 @@ # Single resolved Node runtime — see app/node_runtime.py. install.py # downloads the sidecar when nothing suitable exists; run.py never installs. from app import node_runtime +from app.console import configure_console + +configure_console() multiprocessing.freeze_support() @@ -71,10 +74,10 @@ def _bootstrap_state(): newly shipped default skills (app/state_sync.py: never overwrites a file, never restores one the user deleted). - A dev checkout is skipped: there, code and state are the same tree, which - is what makes a checkout convenient to work in. + When code and state share a tree no copy is needed. An explicit separate + CRAFTBOT_HOME is seeded in a dev checkout too. """ - if _paths.is_dev_checkout(): + if _paths.CODE_ROOT.resolve() == _paths.STATE_ROOT.resolve(): return import shutil as _shutil diff --git a/tests/conftest.py b/tests/conftest.py index 874a9ebdc..3f0a13553 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,6 +1,7 @@ import os import ssl import sys +import tempfile import pytest from pathlib import Path @@ -8,7 +9,9 @@ # Windows SSL-store shim: importing aiohttp in this env can hit a broken # certificate in the Windows cert store (ssl.SSLError [ASN1: NOT_ENOUGH_DATA]). # Swallow the error so collection/imports succeed (mirrors app/main.py shim). -_orig_load_windows_store_certs = ssl.SSLContext._load_windows_store_certs +_orig_load_windows_store_certs = getattr( + ssl.SSLContext, "_load_windows_store_certs", None +) def _safe_load_windows_store_certs(self, storename, purpose): @@ -18,13 +21,54 @@ def _safe_load_windows_store_certs(self, storename, purpose): pass -ssl.SSLContext._load_windows_store_certs = _safe_load_windows_store_certs +if _orig_load_windows_store_certs is not None: + ssl.SSLContext._load_windows_store_certs = _safe_load_windows_store_certs PROJECT_ROOT = Path(__file__).resolve().parent.parent sys.path.insert(0, str(PROJECT_ROOT)) os.chdir(PROJECT_ROOT) +def pytest_addoption(parser): + parser.addoption( + "--live-state", + action="store_true", + default=False, + help="Use the explicitly configured runtime instead of disposable offline state", + ) + + +def pytest_configure(config): + # Runs before collection imports app.paths/config or integration registries. + if config.getoption("--live-state"): + if not os.environ.get("CRAFTBOT_HOME"): + raise pytest.UsageError("--live-state requires an explicit CRAFTBOT_HOME") + return + from tests.qa_runtime import seed_runtime + + config._qa_runtime = tempfile.TemporaryDirectory( + prefix="craftbot-tests-", ignore_cleanup_errors=True + ) + config._qa_previous_home = os.environ.get("CRAFTBOT_HOME") + home = Path(config._qa_runtime.name) + seed_runtime(PROJECT_ROOT, home) + os.environ["CRAFTBOT_HOME"] = str(home) + + +def pytest_unconfigure(config): + runtime = getattr(config, "_qa_runtime", None) + if runtime is not None: + previous = config._qa_previous_home + if previous is None: + os.environ.pop("CRAFTBOT_HOME", None) + else: + os.environ["CRAFTBOT_HOME"] = previous + # Windows may still hold SQLite handles owned by imported singletons. + if "loguru" in sys.modules: + sys.modules["loguru"].logger.remove() + runtime.cleanup() + + @pytest.fixture(autouse=True) def configured_context_window(monkeypatch): """Give every test a configured model.context_window. diff --git a/tests/integrations/test_import_surface.py b/tests/integrations/test_import_surface.py index 2ea96f2be..ec09f3de6 100644 --- a/tests/integrations/test_import_surface.py +++ b/tests/integrations/test_import_surface.py @@ -24,7 +24,9 @@ def _module_names(): - configure(project_root=".") + from app.paths import STATE_ROOT + + configure(project_root=str(STATE_ROOT)) return sorted( m.name for m in pkgutil.walk_packages( diff --git a/tests/integrations/test_provider_listeners.py b/tests/integrations/test_provider_listeners.py index 626c25294..e3c2428dd 100644 --- a/tests/integrations/test_provider_listeners.py +++ b/tests/integrations/test_provider_listeners.py @@ -509,8 +509,9 @@ async def emit(event): # no-op pass providers = default_providers() - # 12 full ports + 5 wave-1 + 6 wave-2 + 2 wave-3 bridges - assert len(providers) == 25 + ids = [provider.id for provider in providers] + assert len(ids) == len(set(ids)), "Provider ids must be unique" + assert {"figma", "supabase", "gmail", "slack", "telegram_bot"} <= set(ids) with_listeners = set() for provider in providers: listener = provider.make_listener(object(), None, emit) diff --git a/tests/qa_runtime.py b/tests/qa_runtime.py new file mode 100644 index 000000000..a996cb1c2 --- /dev/null +++ b/tests/qa_runtime.py @@ -0,0 +1,72 @@ +"""Credential-free, disposable runtime for offline tests and local app QA.""" + +from __future__ import annotations + +import json +from pathlib import Path +import shutil + + +def seed_runtime(code: Path, home: Path) -> None: + """Seed source assets only; never copy databases, logins or user state. + + Require an empty destination so a test cannot overwrite an existing profile. + Live QA may explicitly provision credentials after this function returns. + """ + if home.exists() and any(home.iterdir()): + raise ValueError(f"QA home must be empty: {home}") + home.mkdir(parents=True, exist_ok=True) + for rel in ( + "app/data/action", + "app/data/agent_app_template", + "app/data/agent_file_system_template", + "app/data/agent_profile", + "app/data/playbooks", + "app/data/task_document", + "skills", + "assets", + "agents", + ): + src = code / rel + if src.is_dir(): + shutil.copytree( + src, + home / rel, + ignore=shutil.ignore_patterns( + "__pycache__", + ".git", + "node_modules", + ".credentials", + "*.db", + "*.log", + ), + ) + settings = json.loads( + (code / "app/config/settings.json").read_text(encoding="utf-8") + ) + # A developer checkout may contain local secrets. Carry only configuration, + # with all credential containers removed rather than copied into fixtures. + for key in ( + "api_keys", + "oauth", + "aws", + "aws_credentials", + "credentials", + "model_pools", + ): + settings.pop(key, None) + settings["api_keys_configured"] = {} + settings["auth_mode"] = {} + (home / "app/config").mkdir(parents=True, exist_ok=True) + (home / "app/config/settings.json").write_text( + json.dumps(settings, indent=2), encoding="utf-8" + ) + + +if __name__ == "__main__": + import argparse + + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("home", type=Path, help="New empty state directory") + args = parser.parse_args() + seed_runtime(Path(__file__).resolve().parents[1], args.home.resolve()) diff --git a/tests/test_app_readiness.py b/tests/test_app_readiness.py new file mode 100644 index 000000000..685b68c34 --- /dev/null +++ b/tests/test_app_readiness.py @@ -0,0 +1,94 @@ +"""Readiness deadlines use wall time and don't block or outlive a dead child.""" + +import asyncio +from types import SimpleNamespace +import pytest +from app.agent_app.manager import AgentAppManager + + +def test_hung_probe_obeys_total_deadline_and_keeps_loop_responsive(): + async def run(): + manager = AgentAppManager.__new__(AgentAppManager) + ticks = [] + + async def probe(): + await asyncio.Event().wait() + + async def tick(): + for _ in range(5): + ticks.append(True) + await asyncio.sleep(0.01) + + start = asyncio.get_running_loop().time() + result, _ = await asyncio.gather(manager._probe_ready(probe, 0.15), tick()) + assert result is False + assert 0.12 <= asyncio.get_running_loop().time() - start < 0.5 + assert len(ticks) == 5 + + asyncio.run(run()) + + +@pytest.mark.parametrize("strategy", ["tcp", "http_get", "process_alive"]) +def test_dead_process_is_not_probed_or_retried(strategy): + async def run(): + manager = AgentAppManager.__new__(AgentAppManager) + assert not await manager._check_health_with_strategy( + {"strategy": strategy, "url": "http://127.0.0.1:1"}, + 1, + SimpleNamespace(poll=lambda: 7), + timeout=10, + ) + + asyncio.run(run()) + + +def test_cancellation_reaches_active_probe(): + async def run(): + manager = AgentAppManager.__new__(AgentAppManager) + entered, cancelled = asyncio.Event(), asyncio.Event() + + async def probe(): + entered.set() + try: + await asyncio.Event().wait() + finally: + cancelled.set() + + task = asyncio.create_task(manager._probe_ready(probe, 10)) + await entered.wait() + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + assert cancelled.is_set() + + asyncio.run(run()) + + +def test_healthy_probe_slower_than_half_a_second_can_finish_within_deadline(): + async def run(): + manager = AgentAppManager.__new__(AgentAppManager) + async def probe(): + await asyncio.sleep(.6) + return True + assert await manager._probe_ready(probe, 1) + asyncio.run(run()) + + +def test_process_exit_cancels_an_inflight_probe(): + async def run(): + manager = AgentAppManager.__new__(AgentAppManager) + entered, cancelled = asyncio.Event(), asyncio.Event() + status = [None] + async def probe(): + entered.set() + try: + await asyncio.Event().wait() + finally: + cancelled.set() + task = asyncio.create_task(manager._probe_ready( + probe, 10, SimpleNamespace(poll=lambda: status[0]))) + await entered.wait() + status[0] = 1 + assert not await asyncio.wait_for(task, 1) + assert cancelled.is_set() + asyncio.run(run()) diff --git a/tests/test_craftbot_service.py b/tests/test_craftbot_service.py index 78feb1ec8..eae04e609 100644 --- a/tests/test_craftbot_service.py +++ b/tests/test_craftbot_service.py @@ -138,7 +138,7 @@ def test_macos_frozen_shortcut_starts_installed_agent_executable(tmp_path, monke monkeypatch.setattr(craftbot, "BASE_DIR", str(base_dir)) monkeypatch.setattr(craftbot, "_find_desktop", lambda: str(desktop)) monkeypatch.setattr(craftbot, "_python_exe", lambda: "/bad/python") - monkeypatch.setattr(craftbot, "installed_exe_path", lambda: installed_agent) + monkeypatch.setattr(craftbot, "_installed_launch", lambda: [installed_agent]) craftbot._create_desktop_shortcut_unix(["--frontend-port", "9000"]) diff --git a/tests/test_generative_ui.py b/tests/test_generative_ui.py index af5b3c3c6..705462c35 100644 --- a/tests/test_generative_ui.py +++ b/tests/test_generative_ui.py @@ -36,6 +36,39 @@ def artifact(): ) +@pytest.mark.parametrize( + "html", + [ + "", + "", + "", + '', + '', + '', + '', + ], +) +def test_controls_contract_rejects_unpersistable_or_sensitive_fields(html): + with pytest.raises(ValueError): + make_artifact({"title": "Fields", "html": html, "persistence": "controls"}) + + +def test_controls_contract_accepts_stable_keys_and_manual_custom_outputs(): + html = '' + assert ( + make_artifact({"title": "Form", "html": html, "persistence": "controls"})[ + "persistence" + ] + == "controls" + ) + assert ( + make_artifact( + {"title": "Canvas", "html": "", "persistence": "manual"} + )["persistence"] + == "manual" + ) + + @pytest.mark.parametrize( "change", [ diff --git a/tests/test_installer_stream_routing.py b/tests/test_installer_stream_routing.py deleted file mode 100644 index bb193970f..000000000 --- a/tests/test_installer_stream_routing.py +++ /dev/null @@ -1,109 +0,0 @@ -"""The installer's per-thread stdout routing. - -Two things have to hold at once, and they pull in opposite directions: - - * a worker thread's output goes to the wizard's log panel and nowhere else - (the whole point of routing per thread rather than swapping the process - streams, which would capture every other thread too); - * nothing crashes when the process has no streams at all. The installer is - frozen with console=False, so under pythonw sys.stdout and sys.stderr are - None. print() is a documented no-op in that case, and a wrapper that does - not preserve it turns a harmless library print into AttributeError. -""" - -import io -import sys -import threading - -import pytest - -from installer.api import _install_routed_streams, _NullStream, _ThreadRoutedStream - - -@pytest.fixture -def restore_streams(): - saved = sys.stdout, sys.stderr - yield - sys.stdout, sys.stderr = saved - - -def test_wrapping_absent_streams_keeps_print_a_no_op(restore_streams): - """The frozen, windowed installer: sys.stdout is None.""" - sys.stdout = None - sys.stderr = None - - out, err = _install_routed_streams() - - print("a library logging from a non-routed thread") # must not raise - print("and on stderr", file=sys.stderr) - sys.stdout.flush() - assert sys.stdout.encoding == "utf-8" - assert sys.stdout.isatty() is False - assert isinstance(out, _ThreadRoutedStream) - - -def test_absent_streams_still_route_to_a_worker_sink(restore_streams): - sys.stdout = None - sys.stderr = None - _install_routed_streams() - - sink = io.StringIO() - sys.stdout.route_current_thread(sink) - print("worker line") - sys.stdout.route_current_thread(None) - - assert sink.getvalue() == "worker line\n" - - -def test_only_the_routed_thread_is_captured(restore_streams): - """A per-action swap of sys.stdout is process-wide: the bridge thread - polling get_state, and every other thread, would be captured with it.""" - console = io.StringIO() - sink = io.StringIO() - sys.stdout = _ThreadRoutedStream(console) - - def worker(): - sys.stdout.route_current_thread(sink) - print("from the worker") - sys.stdout.route_current_thread(None) - - thread = threading.Thread(target=worker) - thread.start() - thread.join() - print("from the main thread") - - assert sink.getvalue() == "from the worker\n" - assert console.getvalue() == "from the main thread\n" - - -def test_routing_is_cleared_even_though_the_stream_persists(restore_streams): - console = io.StringIO() - sink = io.StringIO() - sys.stdout = _ThreadRoutedStream(console) - - sys.stdout.route_current_thread(sink) - sys.stdout.route_current_thread(None) - print("after the worker finished") - - assert sink.getvalue() == "" - assert console.getvalue() == "after the worker finished\n" - - -def test_install_is_idempotent(restore_streams): - """Called once per action; it must not wrap a wrapper each time.""" - console = io.StringIO() - sys.stdout = console - sys.stderr = console - - first_out, _ = _install_routed_streams() - second_out, _ = _install_routed_streams() - - assert first_out is second_out - assert first_out._fallback is console - - -def test_null_stream_reports_no_descriptor(): - """Callers that probe fileno() (subprocess wiring, isatty shims) get the - same OSError a detached stream raises, not a wrong file descriptor.""" - with pytest.raises(OSError): - _NullStream().fileno() diff --git a/tests/test_mini_browser_fleet.py b/tests/test_mini_browser_fleet.py index ffe9547dc..6b20e4b31 100644 --- a/tests/test_mini_browser_fleet.py +++ b/tests/test_mini_browser_fleet.py @@ -480,6 +480,93 @@ async def open_user_tabs(core): run(host.shutdown(timeout=30)) +def test_restored_metadata_does_not_allow_reading_before_document_commit(site, paths): + """An event barrier forces the restore race; no timing-dependent sleeps.""" + import_playwright_or_skip() + config.profile_dir("chat-a").mkdir(parents=True) + resume.save_pages( + config.profile_dir("chat-a"), + resume.Pages(pages=(resume.Page(site.url("/set"), "Set"),), viewed=0), + ) + host = make_host() + + async def exercise(core): + release, waiting = asyncio.Event(), asyncio.Event() + goto, wait = core._goto, core._wait_for_reopen + + async def delayed(tab, url): + await release.wait() + await goto(tab, url) + + async def observed(tab): + waiting.set() + await wait(tab) + + core._goto, core._wait_for_reopen = delayed, observed + await core.ensure_started() + reader = asyncio.create_task(core.agent_op("chat-a", "read", {})) + try: + await asyncio.wait_for(waiting.wait(), 5) + assert not reader.done() + tab = core.tabs[core.viewed_tab_id] + assert tab.url == site.url("/set") + assert tab.page.url == "about:blank" + finally: + release.set() + result = await reader + assert result["status"] == "success", result + assert result["url"] == site.url("/set") + + try: + run(host.call("chat-a", exercise)) + finally: + run(host.shutdown(timeout=30)) + + +@pytest.mark.parametrize("human_action", ["navigate", "take_control"]) +def test_human_input_supersedes_a_pending_restored_document(site, paths, human_action): + import_playwright_or_skip() + config.profile_dir("chat-a").mkdir(parents=True) + resume.save_pages(config.profile_dir("chat-a"), resume.Pages( + pages=(resume.Page(site.url("/set"), "Set"),), viewed=0)) + host = make_host() + + async def exercise(core): + entered, release = asyncio.Event(), asyncio.Event() + goto = core._goto + + async def delayed(tab, url): + if url == site.url("/set"): + entered.set() + await release.wait() + await goto(tab, url) + + core._goto = delayed + await core.ensure_started() + await asyncio.wait_for(entered.wait(), 5) + tab = core.tabs[core.viewed_tab_id] + restore = core._reopen_tasks[tab.id] + try: + if human_action == "navigate": + await core.ui_navigate(tab.id, site.url("/page")) + else: + await core.ui_control(tab.id, True) + assert tab.id not in core._reopen_tasks + await asyncio.sleep(0) + assert restore.cancelled() + if human_action == "navigate": + assert tab.page.url == site.url("/page") + else: + assert tab.user_control + finally: + release.set() + + try: + run(host.call("chat-a", exercise)) + finally: + run(host.shutdown(timeout=30)) + + def test_tabs_carry_their_site_icons(site, paths): """Each tab's state carries its site's icon as an inline image: the one the page declares, else the site's /favicon.ico; a blank page has none.""" diff --git a/tests/test_prompt_profile.py b/tests/test_prompt_profile.py index 87679ec94..7cfbbb3b8 100644 --- a/tests/test_prompt_profile.py +++ b/tests/test_prompt_profile.py @@ -2,53 +2,18 @@ """ Tests for the prompt profiler (issue #322, P2). -Covers the cost-aware pricing single-source and the profiler's aggregation over +Covers the token-volume profiler's aggregation over the captured llm_calls table. """ import importlib import os -import tempfile from app.usage.llm_call_storage import LLMCallStorage, LLMCallRow -from app.usage.pricing import get_model_pricing, estimate_cost profiler = importlib.import_module("scripts.prompt_profile") -# ── pricing ────────────────────────────────────────────────────────────────── - - -def test_pricing_longest_match_avoids_shadowing(): - # "gpt-4o" must NOT shadow "gpt-4o-mini" - assert get_model_pricing("gpt-4o-mini")["input"] == 0.15 - assert get_model_pricing("gpt-4o-2024-08")["input"] == 2.50 - assert get_model_pricing("gemini-2.5-pro")["cached"] == 0.125 - assert get_model_pricing("claude-opus-4-8")["input"] == 15.00 - assert get_model_pricing("totally-unknown")["input"] == 1.00 # default - - -def test_estimate_cost_accounts_for_cache(): - c = estimate_cost( - "gemini-2.5-pro", input_tokens=10_000, output_tokens=500, cached_tokens=8_000 - ) - # uncached 2000 @1.25 + cached 8000 @0.125 = 0.0035; output 500 @10 = 0.005 - assert round(c["input_cost"], 6) == 0.0035 - assert round(c["output_cost"], 6) == 0.005 - assert round(c["total_cost"], 6) == 0.0085 - # saved = 8000 * (1.25 - 0.125) / 1e6 - assert round(c["saved"], 6) == 0.009 - - -def test_estimate_cost_clamps_cached_to_input(): - # cached can't exceed input; must not produce negative uncached cost - c = estimate_cost( - "gemini-2.5-pro", input_tokens=100, output_tokens=0, cached_tokens=999 - ) - assert c["input_cost"] >= 0 - assert round(c["input_cost"], 8) == round(100 * 0.125 / 1e6, 8) - - # ── percentile ─────────────────────────────────────────────────────────────── @@ -63,8 +28,8 @@ def test_percentile(): # ── aggregation ────────────────────────────────────────────────────────────── -def _seed(): - db = os.path.join(tempfile.mkdtemp(), "llm_calls.db") +def _seed(tmp_path): + db = os.path.join(tmp_path, "llm_calls.db") s = LLMCallStorage(db_path=db) seed = [ ("SELECT_ACTION_IN_TASK", 2500, 1800, 40, 1200), @@ -90,8 +55,8 @@ def _seed(): return db -def test_aggregate_groups_and_metrics(): - db = _seed() +def test_aggregate_groups_and_metrics(tmp_path): + db = _seed(tmp_path) rows = profiler.load_rows(db, since=None) agg = profiler.aggregate(rows) @@ -103,9 +68,11 @@ def test_aggregate_groups_and_metrics(): assert task["avg_input_tokens"] == 1900 # (1800+2000)/2 # cache hit ratio = (1200+1500)/(1800+2000) = 2700/3800 assert round(task["cache_hit_ratio"], 4) == round(2700 / 3800, 4) - assert task["saved_usd"] > 0 + assert task["total_input_tokens"] == 3800 + assert task["total_output_tokens"] == 95 + assert task["errors"] == 0 - # sorted by cost desc → summarization (4000 in/400 out) is the priciest + # Ranked by total input volume: summarization has 4000 input tokens. assert agg[0]["prompt_name"] == "EVENT_STREAM_SUMMARIZATION" diff --git a/tests/test_qa_runtime.py b/tests/test_qa_runtime.py new file mode 100644 index 000000000..71c10d4e9 --- /dev/null +++ b/tests/test_qa_runtime.py @@ -0,0 +1,34 @@ +"""The test fixture must never inherit a developer's runtime or credentials.""" + +import json +import pytest +from tests.qa_runtime import seed_runtime + + +def test_runtime_seeds_assets_but_strips_credentials_and_databases(tmp_path): + code, home = tmp_path / "code", tmp_path / "home" + config = code / "app/config" + config.mkdir(parents=True) + secrets = { + key: {"secret": "do-not-copy"} + for key in ( + "api_keys", + "oauth", + "aws_credentials", + "credentials", + "model_pools", + ) + } + (config / "settings.json").write_text(json.dumps({"version": "1.4.4", **secrets})) + actions = code / "app/data/action" + actions.mkdir(parents=True) + (actions / "tool.py").write_text("pass") + (actions / "user.db").write_text("private data") + seed_runtime(code, home) + settings = json.loads((home / "app/config/settings.json").read_text()) + assert settings["version"] == "1.4.4" + assert not secrets.keys() & settings.keys() + assert (home / "app/data/action/tool.py").is_file() + assert not list(home.rglob("*.db")) + with pytest.raises(ValueError, match="empty"): + seed_runtime(code, home) diff --git a/tests/test_question_dismissal.py b/tests/test_question_dismissal.py new file mode 100644 index 000000000..8a4bc62a9 --- /dev/null +++ b/tests/test_question_dismissal.py @@ -0,0 +1,156 @@ +"""Dismissing a question uses the same session stop as the chat stop button.""" + +import asyncio +import time +from types import MethodType, SimpleNamespace +from unittest.mock import AsyncMock, Mock + +import pytest + +from app.agent_base import AgentBase +from app.triggers import TriggerService, TriggerSource, TriggerSpec +from app.triggers.runtime import SessionRuntimeManager +from app.triggers.store import TriggerStore +from app.ui_layer.controller.ui_controller import UIController + + +def _controller(agent): + controller = UIController.__new__(UIController) + controller._agent = agent + controller._event_bus = SimpleNamespace(emit=Mock()) + return controller + + +@pytest.mark.parametrize( + "mode,session_id", [("active", "chat-a"), ("queued", None), ("idle", "chat-a")] +) +def test_dismissal_stops_its_run_without_resuming_it_or_stopping_other_sessions( + tmp_path, mode, session_id +): + async def scenario(): + target = session_id or "main" + started = {sid: asyncio.Event() for sid in (target, "other")} + cancelled = {sid: asyncio.Event() for sid in started} + release = asyncio.Event() + late_rows = [] + + async def emit(sid, source, future=False): + return await service.emit( + TriggerSpec( + session_id=sid, + source=source, + description="Synthetic work", + fire_at=time.time() + 60 if future else None, + ) + ) + + async def react(trigger): + sid = trigger.session_id + started[sid].set() + try: + await release.wait() + except asyncio.CancelledError: + cancelled[sid].set() + if sid == target: + # Even a continuation enqueued while unwinding must not + # resurrect the work the user just dismissed. + late_rows.append( + await emit(sid, TriggerSource.RUN_CONTINUATION, future=True) + ) + raise + + runtime = SessionRuntimeManager(react) + store = TriggerStore(db_path=str(tmp_path / "triggers.db")) + service = TriggerService(store, runtime) + finalized = AsyncMock() + runtime.set_stop_finalizer(finalized) + agent = SimpleNamespace( + session_runtime=runtime, + _emit_run_state=Mock(), + _notify_mini_browser=Mock(), + _handle_chat_message=AsyncMock(), + event_stream_manager=SimpleNamespace(log=Mock()), + state_manager=SimpleNamespace(bump_event_stream=Mock()), + ) + agent.request_run_stop = MethodType(AgentBase.request_run_stop, agent) + controller = _controller(agent) + await runtime.start() + try: + other = await emit("other", TriggerSource.USER_MESSAGE) + await asyncio.wait_for(started["other"].wait(), 2) + active = None + if mode == "active": + active = await emit(target, TriggerSource.USER_MESSAGE) + await asyncio.wait_for(started[target].wait(), 2) + continuation = None + if mode != "idle": + continuation = await emit( + target, TriggerSource.RUN_CONTINUATION, future=True + ) + other_continuation = await emit( + "other", TriggerSource.RUN_CONTINUATION, future=True + ) + # Independent future requests are preserved by the shared stop. + scheduled = await emit(target, TriggerSource.SCHEDULED, future=True) + message = await emit(target, TriggerSource.USER_MESSAGE, future=True) + + await asyncio.wait_for( + controller.submit_question_answer( + "", "Continue?", session_id, dismissed=True + ), + 2, + ) + + assert cancelled[target].is_set() == (mode == "active") + assert not cancelled["other"].is_set() + assert store.get(other.trigger_id)["status"] == "CLAIMED" + assert store.get(other_continuation.trigger_id)["status"] == "PENDING" + for row in (scheduled, message): + assert store.get(row.trigger_id)["status"] == "PENDING" + if active: + assert store.get(active.trigger_id)["status"] == "DONE" + assert len(late_rows) == 1 + for row in ([continuation] if continuation else []) + late_rows: + stored = store.get(row.trigger_id) + assert (stored["status"], stored["resolution"]) == ( + "DONE", "cancelled" + ) + assert finalized.await_count == (0 if mode == "idle" else 1) + controller._event_bus.emit.assert_not_called() + agent._handle_chat_message.assert_not_awaited() + agent._notify_mini_browser.assert_called_once_with( + "cancel_owner", target, include_children=True + ) + assert agent.event_stream_manager.log.call_args.kwargs["task_id"] == target + finally: + await runtime.stop() + + asyncio.run(scenario()) + + +def test_dismissal_still_stops_when_recording_the_system_message_fails(): + agent = SimpleNamespace( + request_run_stop=AsyncMock(return_value=True), + event_stream_manager=SimpleNamespace(log=Mock(side_effect=OSError("disk"))), + ) + asyncio.run(_controller(agent).submit_question_answer( + "", "Continue?", "chat-a", dismissed=True + )) + agent.request_run_stop.assert_awaited_once_with("chat-a") + + +@pytest.mark.parametrize("answer", ["Use the first option", " "]) +def test_answering_a_question_does_not_stop_the_run(answer): + agent = SimpleNamespace( + request_run_stop=AsyncMock(), _handle_chat_message=AsyncMock() + ) + controller = _controller(agent) + asyncio.run(controller.submit_question_answer(answer, "Which option?", "chat-a")) + agent.request_run_stop.assert_not_awaited() + if answer.strip(): + payload = agent._handle_chat_message.call_args.args[0] + assert payload["session_id"] == "chat-a" + assert answer in payload["text"] and "Which option?" in payload["text"] + else: + agent._handle_chat_message.assert_not_awaited() + controller._event_bus.emit.assert_not_called() diff --git a/tests/test_session_clear.py b/tests/test_session_clear.py index 2476f936b..f22e6ebf2 100644 --- a/tests/test_session_clear.py +++ b/tests/test_session_clear.py @@ -14,8 +14,10 @@ import asyncio import time from pathlib import Path -from types import SimpleNamespace -from unittest.mock import patch +from types import MethodType, SimpleNamespace +from unittest.mock import AsyncMock, Mock, patch + +import pytest from agent_core.core.impl.event_stream.manager import EventStreamManager from agent_core.core.impl.llm import LLMCallType @@ -212,7 +214,7 @@ async def request_run_stop(session_id): calls.clear() agent.busy_sessions = set() asyncio.run(AgentBase.clear_session(agent, "s1")) - assert calls == [("clear", "s1"), ("save", "s1")] # idle: nothing to stop + assert calls == [("stop", "s1"), ("clear", "s1"), ("save", "s1")] def test_the_main_session_keeps_its_title(tmp_path): @@ -223,3 +225,453 @@ def test_the_main_session_keeps_its_title(tmp_path): assert main.title == "Main" finally: StateSession.end(MAIN_SESSION_ID) + + +@pytest.mark.parametrize("active", [False, True]) +def test_clear_settles_runtime_work_even_without_a_busy_marker(tmp_path, active): + from app.agent_base import AgentBase + from app.triggers import TriggerService, TriggerSource, TriggerSpec + from app.triggers.runtime import SessionRuntimeManager + from app.triggers.store import TriggerStore + + async def scenario(): + started = asyncio.Event() + cancelled = asyncio.Event() + + async def react(trigger): + started.set() + try: + await asyncio.Event().wait() + except asyncio.CancelledError: + cancelled.set() + raise + + runtime = SessionRuntimeManager(react) + store = TriggerStore(db_path=str(tmp_path / "clear-triggers.db")) + service = TriggerService(store, runtime) + agent = SimpleNamespace( + busy_sessions=set(), session_runtime=runtime, + _emit_run_state=Mock(), _notify_mini_browser=Mock(), + session_manager=SimpleNamespace(clear_session=Mock(return_value=True)), + _persist_llm_session=Mock(), + ) + agent.request_run_stop = MethodType(AgentBase.request_run_stop, agent) + runtime.set_stop_finalizer(AsyncMock()) + await runtime.start() + try: + async def emit(sid, source, future=True): + return await service.emit(TriggerSpec(session_id=sid, source=source, + description="Synthetic clear regression", fire_at=time.time() + 60 if future else None)) + if active: + await emit("target", TriggerSource.USER_MESSAGE, future=False) + await asyncio.wait_for(started.wait(), 2) + continuation = await emit("target", TriggerSource.RUN_CONTINUATION) + other = await emit("other", TriggerSource.RUN_CONTINUATION) + scheduled = await emit("target", TriggerSource.SCHEDULED) + assert await AgentBase.clear_session(agent, "target") + row = store.get(continuation.trigger_id) + assert (row["status"], row["resolution"]) == ("DONE", "cancelled") + assert cancelled.is_set() == active + assert store.get(other.trigger_id)["status"] == "PENDING" + assert store.get(scheduled.trigger_id)["status"] == "PENDING" + agent.session_manager.clear_session.assert_called_once_with("target") + agent._persist_llm_session.assert_called_once_with("target") + finally: + await runtime.stop() + + asyncio.run(scenario()) + + +def test_clear_cancels_a_claimed_turn_waiting_for_capacity(tmp_path): + from app.agent_base import AgentBase + from app.triggers import TriggerService, TriggerSource, TriggerSpec + from app.triggers.runtime import SessionRuntimeManager + from app.triggers.store import TriggerStore + + async def scenario(): + running = asyncio.Event() + claimed = asyncio.Event() + release = asyncio.Event() + executed = [] + + async def react(trigger): + executed.append(trigger.session_id) + if trigger.session_id == "other": + running.set() + await release.wait() + + runtime = SessionRuntimeManager(react, max_concurrent_turns=1) + store = TriggerStore(db_path=str(tmp_path / "waiting.db")) + service = TriggerService(store, runtime) + original_claim = service.claim + + def claim(trigger): + original_claim(trigger) + if trigger.session_id == "target": + claimed.set() + + service.claim = claim + finalized = AsyncMock() + runtime.set_stop_finalizer(finalized) + agent = SimpleNamespace( + session_runtime=runtime, _emit_run_state=Mock(), + _notify_mini_browser=Mock(), + session_manager=SimpleNamespace(clear_session=Mock(return_value=True)), + _persist_llm_session=Mock(), + ) + agent.request_run_stop = MethodType(AgentBase.request_run_stop, agent) + await runtime.start() + try: + async def emit(sid): + return await service.emit(TriggerSpec( + session_id=sid, source=TriggerSource.USER_MESSAGE, + description="Capacity regression", + )) + + other = await emit("other") + await asyncio.wait_for(running.wait(), 2) + target = await emit("target") + await asyncio.wait_for(claimed.wait(), 2) + assert await AgentBase.clear_session(agent, "target") + assert store.get(target.trigger_id)["status"] == "DONE" + assert store.get(other.trigger_id)["status"] == "CLAIMED" + finalized.assert_awaited_once_with("target") + assert executed == ["other"] + release.set() + # A fresh request in the cleared chat must still execute. + fresh = await emit("target") + for _ in range(100): + if store.get(fresh.trigger_id)["status"] == "DONE": + break + await asyncio.sleep(.01) + assert store.get(fresh.trigger_id)["status"] == "DONE" + assert executed == ["other", "target"] + finally: + release.set() + await runtime.stop() + + asyncio.run(scenario()) + + +@pytest.mark.parametrize("unwind", ["cancel", "return", "raise"]) +@pytest.mark.parametrize("cancel_requester", [False, True]) +def test_concurrent_stops_join_settlement_before_resetting_context(tmp_path, monkeypatch, unwind, cancel_requester): + from app.agent_base import AgentBase + from app.triggers import TriggerService, TriggerSource, TriggerSpec + from app.triggers.runtime import SessionRuntimeManager + from app.triggers.store import TriggerStore + + async def scenario(): + import threading + + running = asyncio.Event() + cancelled = asyncio.Event() + release_turn = asyncio.Event() + kill_started = asyncio.Event() + release_kill = threading.Event() + loop = asyncio.get_running_loop() + + def kill(sid): + assert sid == "target" + loop.call_soon_threadsafe(kill_started.set) + assert release_kill.wait(3) + + monkeypatch.setattr( + "agent_core.core.impl.action.cancellation.kill_session_processes", kill + ) + + async def react(trigger): + running.set() + try: + await release_turn.wait() + except asyncio.CancelledError: + cancelled.set() + if unwind == "return": + return + if unwind == "raise": + raise RuntimeError("Tool failed during cancellation") + raise + + runtime = SessionRuntimeManager(react) + store = TriggerStore(db_path=str(tmp_path / "racing.db")) + service = TriggerService(store, runtime) + finalized = AsyncMock() + runtime.set_stop_finalizer(finalized) + agent = SimpleNamespace( + session_runtime=runtime, _emit_run_state=Mock(), + _notify_mini_browser=Mock(), + session_manager=SimpleNamespace(clear_session=Mock(return_value=True)), + _persist_llm_session=Mock(), + ) + agent.request_run_stop = MethodType(AgentBase.request_run_stop, agent) + await runtime.start() + tasks = [] + try: + row = await service.emit(TriggerSpec( + session_id="target", source=TriggerSource.USER_MESSAGE, + description="Finish while process shutdown is pending", + )) + await asyncio.wait_for(running.wait(), 2) + tasks.append(asyncio.create_task(AgentBase.clear_session(agent, "target"))) + await asyncio.wait_for(kill_started.wait(), 2) + release_turn.set() + # Cancellation must be requested before waiting for OS shutdown; + # otherwise this turn finishes normally and loses the stop signal. + await asyncio.wait_for(cancelled.wait(), .5) + tasks.append(asyncio.create_task(runtime.request_stop("target"))) + await asyncio.sleep(0) + assert not any(task.done() for task in tasks) + if cancel_requester: + tasks[0].cancel() + with pytest.raises(asyncio.CancelledError): + await tasks[0] + assert not tasks[1].done() + finalized.assert_not_awaited() + agent.session_manager.clear_session.assert_not_called() + release_kill.set() + results = await asyncio.wait_for(asyncio.gather(*tasks, return_exceptions=True), 2) + assert results[1] is True + if cancel_requester: + assert isinstance(results[0], asyncio.CancelledError) + assert await AgentBase.clear_session(agent, "target") + else: + assert results[0] is True + assert store.get(row.trigger_id)["status"] == "DONE" + finalized.assert_awaited_once_with("target") + agent.session_manager.clear_session.assert_called_once_with("target") + finally: + release_kill.set() + release_turn.set() + await runtime.stop() + for task in tasks: + if not task.done(): + task.cancel() + await asyncio.gather(*tasks, return_exceptions=True) + + asyncio.run(scenario()) + + +def test_clear_does_not_reset_context_after_a_stop_timeout(tmp_path, monkeypatch): + from app.agent_base import AgentBase + from app.triggers import TriggerService, TriggerSource, TriggerSpec + from app.triggers.runtime import SessionRuntimeManager + from app.triggers.store import TriggerStore + + async def scenario(): + started = asyncio.Event() + unwinding = asyncio.Event() + release = asyncio.Event() + cancellations = [] + + async def react(trigger): + started.set() + try: + await asyncio.Event().wait() + except asyncio.CancelledError: + cancellations.append(trigger.session_id) + unwinding.set() + await release.wait() + raise + + runtime = SessionRuntimeManager(react) + store = TriggerStore(db_path=str(tmp_path / "timeout.db")) + service = TriggerService(store, runtime) + finalized = AsyncMock() + runtime.set_stop_finalizer(finalized) + agent = SimpleNamespace( + session_runtime=runtime, _emit_run_state=Mock(), + _notify_mini_browser=Mock(), + session_manager=SimpleNamespace(clear_session=Mock(return_value=True)), + _persist_llm_session=Mock(), + ) + agent.request_run_stop = MethodType(AgentBase.request_run_stop, agent) + monkeypatch.setattr("app.triggers.runtime.STOP_SETTLE_TIMEOUT_S", .02) + await runtime.start() + try: + row = await service.emit(TriggerSpec(session_id="target", + source=TriggerSource.USER_MESSAGE, description="Slow tool cleanup")) + await asyncio.wait_for(started.wait(), 2) + with pytest.raises(asyncio.TimeoutError): + await AgentBase.clear_session(agent, "target") + assert unwinding.is_set() + agent.session_manager.clear_session.assert_not_called() + agent._persist_llm_session.assert_not_called() + finalized.assert_not_awaited() + assert store.get(row.trigger_id)["status"] == "CLAIMED" + release.set() + assert await AgentBase.clear_session(agent, "target") + assert cancellations == ["target"] + finalized.assert_awaited_once_with("target") + assert store.get(row.trigger_id)["status"] == "DONE" + finally: + release.set() + await runtime.stop() + + asyncio.run(scenario()) + + +def test_clear_command_orders_storage_and_preserves_other_chat(tmp_path, monkeypatch): + import threading + from concurrent.futures import ThreadPoolExecutor + from app.ui_layer.adapters.browser_adapter import BrowserChatComponent + from app.ui_layer.adapters.base import InterfaceAdapter + from app.ui_layer.commands.builtin.clear import ClearCommand + from app.ui_layer.components.types import ChatMessage + from app.ui_layer.events.event_bus import EventBus + from app.ui_layer.events.event_types import UIEvent, UIEventType + from app.usage.chat_storage import ChatStorage, StoredChatMessage + + storage = ChatStorage(db_path=str(tmp_path / "chat.db")) + storage.insert_message(StoredChatMessage(message_id="other", session_id="other", + sender="agent", style="agent", content="Keep me", timestamp=1)) + insert_started = threading.Event() + release_insert = threading.Event() + original_insert = storage.insert_message + + def insert(message): + insert_started.set() + assert release_insert.wait(3) + original_insert(message) + + monkeypatch.setattr(storage, "insert_message", insert) + monkeypatch.setattr("app.usage.get_chat_storage", lambda: storage) + actions = Mock() + monkeypatch.setattr("app.usage.get_action_storage", lambda: actions) + adapter = SimpleNamespace(_broadcast=AsyncMock(), broadcast_session_cleared=AsyncMock()) + chat = BrowserChatComponent.__new__(BrowserChatComponent) + chat._storage = storage + chat._db = ThreadPoolExecutor(max_workers=1) + chat._messages = [] + chat._trim_at = 100 + chat._adapter = adapter + adapter.chat_component = chat + controller = SimpleNamespace( + agent=SimpleNamespace(clear_session=AsyncMock(return_value=True)), + active_adapter=adapter, event_bus=EventBus(), + ) + command = ClearCommand(controller) + command.emit_message = Mock() + + async def scenario(): + release_ui = asyncio.Event() + release_other = asyncio.Event() + other_pending = asyncio.Event() + + async def delayed_message(label, message, style, session_id=None, **kwargs): + if session_id == "other": + other_pending.set() + await release_other.wait() + return + await release_ui.wait() + await chat.append_message(ChatMessage(sender="agent", style="agent", + content="Delayed old event", message_id="delayed", timestamp=3, + session_id="target")) + + # Use the actual synchronous adapter handler, which schedules the + # asynchronous display task returned to the bus for ownership. + display_adapter = SimpleNamespace(_display_chat_message=delayed_message) + controller.event_bus.subscribe(UIEventType.SYSTEM_MESSAGE, + lambda event: InterfaceAdapter._handle_system_message(display_adapter, event)) + for sid in ("target", "other"): + controller.event_bus.emit(UIEvent(type=UIEventType.SYSTEM_MESSAGE, + task_id=sid, data={"message": "Old event"})) + await asyncio.wait_for(other_pending.wait(), 2) + append = asyncio.create_task(chat.append_message(ChatMessage( + sender="agent", style="agent", content="Old work", message_id="old", + timestamp=2, session_id="target"))) + try: + assert await asyncio.to_thread(insert_started.wait, 2) + clear = asyncio.create_task(command.execute([], session_id="target")) + # Release only after the command has reached its first await. + asyncio.get_running_loop().call_soon(release_ui.set) + asyncio.get_running_loop().call_soon(release_insert.set) + result, _ = await asyncio.wait_for(asyncio.gather(clear, append), 2) + assert result.success + assert storage.get_message_count("target") == 0 + assert storage.get_message_count("other") == 1 + assert chat.get_messages() == [] + actions.clear_items.assert_called_once_with("target") + adapter.broadcast_session_cleared.assert_awaited_once_with("target") + command.emit_message.assert_called_once_with( + "Conversation cleared.", "system", session_id="target") + controller.agent.clear_session.return_value = False + actions.reset_mock() + adapter.broadcast_session_cleared.reset_mock() + command.emit_message.reset_mock() + assert not (await command.execute([], session_id="missing")).success + actions.clear_items.assert_not_called() + adapter.broadcast_session_cleared.assert_not_awaited() + command.emit_message.assert_not_called() + finally: + release_ui.set() + release_other.set() + release_insert.set() + await append + await controller.event_bus.drain("other") + + try: + asyncio.run(scenario()) + finally: + chat._db.shutdown(wait=True) + + +def test_fresh_work_waits_for_parked_run_finalization_and_context_reset(tmp_path): + from app.agent_base import AgentBase + from app.triggers import TriggerService, TriggerSource, TriggerSpec + from app.triggers.runtime import SessionRuntimeManager + from app.triggers.store import TriggerStore + + async def scenario(): + finalizing = asyncio.Event() + release = asyncio.Event() + claimed = asyncio.Event() + executed = asyncio.Event() + reset = Mock(return_value=True) + + async def finalize(sid): + finalizing.set() + await release.wait() + + async def react(trigger): + reset.assert_called_once_with("target") + executed.set() + + runtime = SessionRuntimeManager(react) + store = TriggerStore(db_path=str(tmp_path / "parked.db")) + service = TriggerService(store, runtime) + original_claim = service.claim + + def claim(trigger): + original_claim(trigger) + claimed.set() + + service.claim = claim + runtime.set_stop_finalizer(finalize) + agent = SimpleNamespace(session_runtime=runtime, _emit_run_state=Mock(), + _notify_mini_browser=Mock(), _persist_llm_session=Mock(), + session_manager=SimpleNamespace(clear_session=reset)) + agent.request_run_stop = MethodType(AgentBase.request_run_stop, agent) + await runtime.start() + clear = None + try: + await service.emit(TriggerSpec(session_id="target", + source=TriggerSource.RUN_CONTINUATION, description="Parked run", + fire_at=time.time() + 60)) + clear = asyncio.create_task(AgentBase.clear_session(agent, "target")) + await asyncio.wait_for(finalizing.wait(), 2) + await service.emit(TriggerSpec(session_id="target", + source=TriggerSource.USER_MESSAGE, description="Fresh request")) + await asyncio.wait_for(claimed.wait(), 2) + await asyncio.sleep(0) + reset.assert_not_called() + assert not executed.is_set() + release.set() + assert await asyncio.wait_for(clear, 2) + await asyncio.wait_for(executed.wait(), 2) + finally: + release.set() + await runtime.stop() + if clear is not None: + await clear + + asyncio.run(scenario()) diff --git a/tests/test_state_sync.py b/tests/test_state_sync.py index 90da1d962..d19f2c2a6 100644 --- a/tests/test_state_sync.py +++ b/tests/test_state_sync.py @@ -399,7 +399,10 @@ def _bootstrap_state_function(fake_paths): return namespace["_bootstrap_state"] -def test_run_py_bootstrap_syncs_an_upgraded_install(tmp_path, monkeypatch): +@pytest.mark.parametrize("dev_checkout", [False, True]) +def test_run_py_bootstrap_syncs_an_upgraded_install( + tmp_path, monkeypatch, dev_checkout +): code = tmp_path / "code" state = tmp_path / "state" _seed_state(_make_code(code, release=1), state) @@ -407,7 +410,7 @@ def test_run_py_bootstrap_syncs_an_upgraded_install(tmp_path, monkeypatch): _make_code(code, release=2) monkeypatch.chdir(tmp_path) # _bootstrap_state chdirs; restored afterwards fake_paths = SimpleNamespace( - is_dev_checkout=lambda: False, CODE_ROOT=code, STATE_ROOT=state + is_dev_checkout=lambda: dev_checkout, CODE_ROOT=code, STATE_ROOT=state ) _bootstrap_state_function(fake_paths)() diff --git a/tests/test_usage_delivery.py b/tests/test_usage_delivery.py new file mode 100644 index 000000000..609538bd2 --- /dev/null +++ b/tests/test_usage_delivery.py @@ -0,0 +1,178 @@ +"""Usage ownership, concurrency, cancellation and durable shutdown contracts.""" + +import asyncio +from contextvars import ContextVar +import threading +from types import SimpleNamespace +import pytest + +from agent_core.core.hooks.usage import ( + dispatch_usage, + drain_usage, + to_thread_with_usage, +) +from agent_core.core.hooks.types import UsageEventData + + +@pytest.mark.parametrize("kind", ["image", "video"]) +def test_generation_wrappers_share_usage_context_and_delivery(kind): + from agent_core.core.impl.image_gen.interface import ImageGenInterface + from agent_core.core.impl.video_gen.interface import VideoGenInterface + + cls = ImageGenInterface if kind == "image" else VideoGenInterface + iface = cls.__new__(cls) + session = ContextVar("generation_session") + events = [] + + async def hook(event): + events.append((session.get(), event)) + + iface._report_usage = hook + + def generate(**kwargs): + iface._report_usage_async( + provider="test", model=kind, input_tokens=4, output_tokens=5 + ) + return ["synthetic"] + + setattr(iface, "generate_" + kind, generate) + + async def run(): + session.set("owner") + result = await getattr(iface, "generate_" + kind + "_async")() + assert result == ["synthetic"] + + asyncio.run(run()) + assert len(events) == 1 + assert events[0][0] == "owner" + assert events[0][1].input_tokens == 4 + + +def test_workers_deliver_once_in_their_request_context_on_the_owner_loop(): + session = ContextVar("test_session") + events = [] + + async def run(): + owner = asyncio.get_running_loop() + + async def hook(value): + assert asyncio.get_running_loop() is owner + await asyncio.sleep(0) + events.append((session.get(), value)) + + async def request(key): + token = session.set(key) + try: + await to_thread_with_usage(dispatch_usage, hook, key) + assert (key, key) in events # delivery precedes successful return + finally: + session.reset(token) + + await asyncio.gather(*(request(str(i)) for i in range(20))) + await drain_usage() + + asyncio.run(run()) + assert sorted(events) == sorted((str(i), str(i)) for i in range(20)) + + +def test_cancelled_request_worker_and_usage_are_drained_before_shutdown(): + entered, release = threading.Event(), threading.Event() + events = [] + + async def hook(value): + events.append(value) + + def worker(): + entered.set() + assert release.wait(5) + dispatch_usage(hook, "completed") + + async def run(): + task = asyncio.create_task(to_thread_with_usage(worker)) + await asyncio.to_thread(entered.wait, 5) + task.cancel() + try: + await task + except asyncio.CancelledError: + pass + release.set() + await drain_usage() + + asyncio.run(run()) + assert events == ["completed"] + + +def test_failed_hook_is_observable_and_is_not_retried(caplog): + calls = [] + + async def hook(event): + calls.append(event) + raise RuntimeError("storage unavailable") + + async def run(): + await to_thread_with_usage(dispatch_usage, hook, "one") + await drain_usage() + + asyncio.run(run()) + assert calls == ["one"] + assert "Usage delivery failed" in caplog.text + + +def test_sync_and_async_reporter_calls_across_threads_flush_without_loss(): + from app.usage.reporter import UsageReporter + + saved = [] + reporter = UsageReporter( + SimpleNamespace( + insert_events_batch=lambda batch: saved.extend(batch) or len(batch) + ) + ) + + def report(i): + dispatch_usage(reporter.report, UsageEventData("vlm", "test", str(i), i, 1)) + + async def run(): + await asyncio.gather(*(asyncio.to_thread(report, i) for i in range(30))) + await reporter.shutdown() + + asyncio.run(run()) + assert sorted(event.model for event in saved) == sorted(str(i) for i in range(30)) + assert reporter.get_stats()["buffered"] == 0 + + +def test_shutdown_can_drain_usage_owned_by_another_running_loop(): + entered, release = threading.Event(), threading.Event() + delivered, errors = [], [] + + async def hook(event): + delivered.append(event) + + def worker(): + entered.set() + assert release.wait(5) + dispatch_usage(hook, "foreign") + + def other_loop(): + try: + asyncio.run(to_thread_with_usage(worker)) + except BaseException as exc: + errors.append(exc) + + thread = threading.Thread(target=other_loop) + thread.start() + try: + assert entered.wait(5) + + async def run(): + drain = asyncio.create_task(drain_usage()) + await asyncio.sleep(0) # let shutdown observe the in-flight foreign task + release.set() + await asyncio.wait_for(drain, 2) + + asyncio.run(run()) + finally: + release.set() + thread.join(5) + assert not thread.is_alive() + assert not errors + assert delivered == ["foreign"] diff --git a/tests/test_vlm_regressions.py b/tests/test_vlm_regressions.py new file mode 100644 index 000000000..526fdd552 --- /dev/null +++ b/tests/test_vlm_regressions.py @@ -0,0 +1,292 @@ +"""Vision transport, error, OCR and async regressions without provider traffic.""" + +import asyncio +import base64 +import io +import threading +from types import SimpleNamespace +from unittest.mock import Mock + +import httpx +import pytest +from PIL import Image + +from agent_core.core.errors import ClassifiedError, ErrorCategory +from agent_core.core.impl.vlm.interface import VLMInterface + + +@pytest.fixture +def image_bytes(): + def make(fmt): + output = io.BytesIO() + Image.new("RGB", (32, 24), "red").save(output, format=fmt) + return output.getvalue() + + return make + + +@pytest.fixture +def vision(): + """Use the public constructor but defer clients so no credentials are read.""" + iface = VLMInterface(provider="openai", model="gpt-4o-mini", deferred=True) + iface._report_usage_async = Mock() + iface.client = SimpleNamespace( + chat=SimpleNamespace( + completions=SimpleNamespace( + create=Mock( + return_value=SimpleNamespace( + choices=[ + SimpleNamespace( + message=SimpleNamespace(content='{"shape":"square"}') + ) + ], + usage=SimpleNamespace( + prompt_tokens=100, + completion_tokens=20, + prompt_tokens_details=SimpleNamespace(cached_tokens=80), + ), + ) + ) + ) + ) + ) + return iface + + +@pytest.mark.parametrize( + "fmt,mime", + [ + ("PNG", "image/png"), + ("JPEG", "image/jpeg"), + ("GIF", "image/gif"), + ("WEBP", "image/webp"), + ], +) +def test_real_image_bytes_reach_sdk_with_matching_media_type( + vision, image_bytes, fmt, mime +): + raw = image_bytes(fmt) + answer = vision.describe_image_bytes( + raw, user_prompt="Return JSON describing the shape", log_response=False + ) + assert answer == '{"shape":"square"}' + call = vision.client.chat.completions.create.call_args.kwargs + parts = call["messages"][-1]["content"] + data_url = parts[1]["image_url"]["url"] + prefix, encoded = data_url.split(",", 1) + assert prefix == f"data:{mime};base64" + assert base64.b64decode(encoded) == raw + + +@pytest.mark.parametrize( + "provider", + ["openai", "openrouter", "deepseek", "groq", "together", "lmstudio", "vllm"], +) +def test_openai_compatible_vision_routing_and_cache_accounting( + vision, image_bytes, provider +): + vision.provider = provider + counter = [7] + vision._get_token_count = lambda: counter[0] + vision._set_token_count = lambda value: counter.__setitem__(0, value) + vision.describe_image_bytes(image_bytes("PNG"), log_response=False) + assert counter[0] == 47 # 7 + 100 prompt + 20 output - 80 cached + vision._report_usage_async.assert_called_once_with( + f"vlm_{provider}", provider, vision.model, 100, 20, 80 + ) + + +def test_ocr_preserves_plain_text_and_does_not_force_json( + vision, image_bytes, tmp_path +): + image = tmp_path / "receipt.png" + image.write_bytes(image_bytes("PNG")) + vision.client.chat.completions.create.return_value.choices[ + 0 + ].message.content = "TOTAL: 42.00\nPAID" + assert vision.describe_image_ocr(str(image)) == "TOTAL: 42.00\nPAID" + call = vision.client.chat.completions.create.call_args.kwargs + assert "response_format" not in call + assert "precise OCR engine" in call["messages"][0]["content"] + + +def test_default_description_satisfies_openai_json_mode_contract(vision, image_bytes): + vision.describe_image_bytes(image_bytes("PNG"), log_response=False) + call = vision.client.chat.completions.create.call_args.kwargs + assert "response_format" not in call + + +@pytest.mark.parametrize("provider", ["openai", "groq", "lmstudio", "openrouter"]) +def test_explicit_json_intent_supplies_instructions_and_validates_schema( + vision, image_bytes, provider +): + from agent_core.core.models.output import OutputIntent + + vision.provider = provider + intent = OutputIntent( + "json", + { + "type": "object", + "required": ["shape"], + "properties": {"shape": {"type": "string"}}, + }, + ) + assert vision.describe_image_bytes( + image_bytes("PNG"), user_prompt="Identify it", output_intent=intent + ) + call = vision.client.chat.completions.create.call_args.kwargs + assert "JSON" in call["messages"][0]["content"] + vision.client.chat.completions.create.return_value.choices[ + 0 + ].message.content = '{"other": 1}' + with pytest.raises(ClassifiedError): + vision.describe_image_bytes(image_bytes("PNG"), output_intent=intent) + + +def test_legacy_explicit_json_mode_is_supported(vision, image_bytes): + vision.describe_image_bytes( + image_bytes("PNG"), user_prompt="Identify it", json_mode=True + ) + call = vision.client.chat.completions.create.call_args.kwargs + assert call["response_format"] == {"type": "json_object"} + assert "JSON" in call["messages"][0]["content"] + + +@pytest.mark.parametrize("provider", ["remote", "byteplus"]) +def test_http_vision_providers_report_normalized_usage( + vision, image_bytes, monkeypatch, provider +): + import agent_core.core.impl.vlm.interface as module + + vision.provider = provider + vision.remote_url = vision.byteplus_base_url = "http://provider.invalid" + vision.api_key = "synthetic" + payload = ( + {"response": "Description", "prompt_eval_count": 50, "eval_count": 10} + if provider == "remote" + else { + "choices": [{"message": {"content": "Description"}}], + "usage": {"prompt_tokens": 50, "completion_tokens": 10, "total_tokens": 60}, + } + ) + monkeypatch.setattr( + module.requests, + "post", + Mock( + return_value=SimpleNamespace( + raise_for_status=lambda: None, json=lambda: payload + ) + ), + ) + assert ( + vision.describe_image_bytes(image_bytes("PNG"), log_response=False) + == "Description" + ) + assert vision._report_usage_async.call_count == 1 + assert vision._report_usage_async.call_args.args[:5] == ( + f"vlm_{provider}", + provider, + vision.model, + 50, + 10, + ) + + +@pytest.mark.parametrize("method", ["describe_image", "describe_image_ocr"]) +def test_missing_image_fails_before_provider_call(vision, tmp_path, method): + with pytest.raises(FileNotFoundError): + getattr(vision, method)(str(tmp_path / "missing.png")) + vision.client.chat.completions.create.assert_not_called() + + +@pytest.mark.parametrize( + "status,category", + [ + (401, ErrorCategory.AUTH), + (403, ErrorCategory.AUTH), + (429, ErrorCategory.RATE_LIMIT), + (500, ErrorCategory.SERVER), + ], +) +def test_provider_http_errors_keep_actionable_classification( + vision, image_bytes, status, category +): + from openai import APIStatusError + + response = httpx.Response( + status, request=httpx.Request("POST", "https://api.example.test/vision") + ) + error = APIStatusError( + "provider rejected request", + response=response, + body={"error": {"message": "provider rejected request"}}, + ) + vision.client.chat.completions.create.side_effect = error + with pytest.raises(ClassifiedError) as caught: + vision.describe_image_bytes(image_bytes("PNG"), log_response=False) + assert caught.value.info.category == category + vision._report_usage_async.assert_not_called() + + +def test_empty_choices_raise_a_classified_error(vision, image_bytes): + vision.client.chat.completions.create.return_value.choices = [] + with pytest.raises(ClassifiedError): + vision.describe_image_bytes(image_bytes("PNG"), log_response=False) + + +def test_async_vision_runs_sdk_off_the_event_loop(vision, image_bytes): + caller_thread = threading.get_ident() + sdk_threads = [] + response = vision.client.chat.completions.create.return_value + + def provider(**kwargs): + sdk_threads.append(threading.get_ident()) + return response + + vision.client.chat.completions.create.side_effect = provider + result = asyncio.run( + vision.generate_response_async( + image_bytes("PNG"), user_prompt="Describe the square", log_response=False + ) + ) + assert result == '{"shape":"square"}' + assert sdk_threads and sdk_threads[0] != caller_thread + + +def test_async_vision_delivers_usage_to_reporting_hook(vision, image_bytes): + events = [] + + async def report(event): + events.append(event) + + vision._report_usage = report + vision._report_usage_async = VLMInterface._report_usage_async.__get__(vision) + + async def run(): + await vision.generate_response_async( + image_bytes("PNG"), user_prompt="Describe", log_response=False + ) + await asyncio.sleep(0) + + asyncio.run(run()) + assert len(events) == 1 + assert events[0].input_tokens == 100 + assert events[0].cached_tokens == 80 + + +def test_failed_reinitialization_returns_false_without_crashing(vision, monkeypatch): + from app.models.factory import ModelFactory + + monkeypatch.setattr( + ModelFactory, "create", Mock(side_effect=EnvironmentError("missing API key")) + ) + assert vision.reinitialize(provider="openai", api_key="", base_url="") is False + + +def test_video_with_no_readable_frames_fails_before_provider(vision, tmp_path): + pytest.importorskip("cv2") + video = tmp_path / "empty.mp4" + video.write_bytes(b"") + with pytest.raises(ValueError, match="frames|read"): + vision.describe_video_frames(str(video)) + vision.client.chat.completions.create.assert_not_called()