diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a16749e..47d0a3c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,6 +32,42 @@ jobs: - name: Run offline test suite run: conda run -n agenticcli python -m pytest -m 'not llm and not docker' -q + wheel-acceptance: + name: Built-wheel acceptance (console smoke) + runs-on: ubuntu-latest + defaults: + run: + shell: bash -el {0} + steps: + - uses: actions/checkout@v4 + + - name: Set up conda env (agenticcli) + uses: conda-incubator/setup-miniconda@v3 + with: + miniforge-version: latest + environment-file: environment.yml + activate-environment: agenticcli + + # pexpect drives the console over a pty. Installed explicitly via the dev + # extra rather than relied on transitively. + - name: Install dev extra (pexpect) + run: conda run -n agenticcli pip install -e '.[dev]' + + # Its own step rather than part of the offline suite: it builds a wheel + # and installs that wheel's dependencies into a throwaway virtualenv, so + # it is slow and needs the network. The opt-in env var is what the tests + # gate on, so the offline selector stays fast and offline. + # + # Targets the file rather than `-m wheel` alone: a bare marker selection + # still *collects* the whole suite, which would need the langgraph extra + # installed only to collect tests this job does not run. + - name: Run built-wheel acceptance + env: + AGENTIC_WHEEL_ACCEPTANCE: "1" + run: > + conda run -n agenticcli python -m pytest + tests/examples/test_research_demo_wheel.py -m wheel -v + docker-isolation: name: Docker isolation tests runs-on: ubuntu-latest diff --git a/examples/research_demo/agents.py b/examples/research_demo/agents.py index 765173c..d15fc41 100644 --- a/examples/research_demo/agents.py +++ b/examples/research_demo/agents.py @@ -204,9 +204,35 @@ def report_writer_prompt() -> str: Rule of thumb: concept pages > sidecars > chunks. Concept pages and sidecars are synthesis-first; chunks are evidence-first. +## Tool Names + +Call only the tools that appear in your tool declarations, and call each one by +its exact declared name, exactly as written. If a task needs two tools, make two +separate calls. To hand work to another agent, call +`transfer_to_agent(agent_name="")`. + +## When to plan, and when to just do it + +Match the response to the size of the request: + +- **Explicit, bounded request** — one clear operation with its parameters + already given ("search arXiv for two papers on X", "ingest this note", + "read it back"). **Do it now**: execute or delegate directly. Do not write a + plan and do not ask for confirmation; the user already told you exactly what + they want. +- **Open-ended or substantial multi-step research** — a goal rather than an + operation ("research X and write me a report", anything spanning several + tools or agents). **Plan first**: `save_plan(content)` with markdown + checkboxes, show the plan immediately, and wait for confirmation. +- **The user asks for a plan** — always plan, whatever the size. + +Planning is a workflow courtesy, not a safety gate: what you are allowed to do +is enforced by tool permissions, not by whether you planned first. So never use +"I should plan" as a reason to refuse or defer a small, explicit request. + ## Workflow Guidelines -When the user asks you to research something: +When the user asks you to research something open-ended: 1. **Check `kb_search_concepts(topic)`** — reuse any existing synthesis before deriving a new one. 2. Browse the knowledge base with `kb_list` to see what's already ingested. 3. Run `kb_search` only if you need evidence that isn't already summarized in a concept page. @@ -255,13 +281,14 @@ def report_writer_prompt() -> str: - ALWAYS show the plan after creating it - ALWAYS show progress after completing tasks - Share findings and learnings explicitly in your responses -- Ask for confirmation before starting lengthy work +- Ask for confirmation before starting *lengthy* work — not before a single + explicit operation the user has already spelled out - Be thorough and detailed in your findings and reports """ AGENT_CONFIGS = [ - # Leaf agent: arXiv specialist (must be listed before coordinator) + # Leaf agent: arXiv specialist AgentConfig( name="arxiv_specialist", prompt=ARXIV_SPECIALIST_PROMPT, @@ -279,7 +306,7 @@ def report_writer_prompt() -> str: ], description="arXiv paper research specialist: search, analyze, save, and catalog academic papers", ), - # Leaf agent: data analyst (must be listed before coordinator) + # Leaf agent: data analyst AgentConfig( name="data_analyst", prompt=DATA_ANALYST_PROMPT, @@ -287,7 +314,7 @@ def report_writer_prompt() -> str: tools=[sandbox_execute, read_file, write_file, ask_clarification], description="Stateful data-analysis specialist: loads datasets and runs multi-step pandas/plotting analysis in an isolated executor.", ), - # Leaf agent: report writer (must be listed before coordinator) + # Leaf agent: report writer AgentConfig( name="report_writer", prompt=report_writer_prompt, diff --git a/examples/research_demo/commands.py b/examples/research_demo/commands.py index 90f56e4..7e74bbe 100644 --- a/examples/research_demo/commands.py +++ b/examples/research_demo/commands.py @@ -10,9 +10,26 @@ from agentic_cli.cli.commands import Command, CommandCategory if TYPE_CHECKING: + from agentic_cli.workflow.base_manager import BaseWorkflowManager + from examples.research_demo.app import ResearchDemoApp +def _ready_workflow(app: "ResearchDemoApp") -> "BaseWorkflowManager | None": + """The workflow manager, or None while it is still coming up. + + ``app.workflow`` raises until the controller reports READY, so a command + that touches it during background initialization would otherwise surface as + the generic "Error executing command" from ``BaseCLIApp._handle_command``. + Built-in commands use exactly this shape (see ``SessionsCommand``); the + caller is expected to warn and return. + """ + try: + return app.workflow + except (RuntimeError, AttributeError): + return None + + class MemoryCommand(Command): """Show persistent memory contents.""" @@ -27,7 +44,15 @@ def __init__(self) -> None: ) async def execute(self, args: str, app: "ResearchDemoApp") -> None: - memory_store = app.workflow.memory_manager if app.workflow else None + workflow = _ready_workflow(app) + if workflow is None: + app.session.add_warning( + "Memory is not available yet — the workflow is still " + "initializing. Try /memory again in a moment." + ) + return + + memory_store = workflow.memory_manager table = Table(title="Persistent Memory", show_header=True) table.add_column("ID", style="dim", width=8) @@ -123,9 +148,12 @@ async def execute(self, args: str, app: "ResearchDemoApp") -> None: from agentic_cli.knowledge_base.manager import BackfillAlreadyRunning from agentic_cli.workflow.service_registry import set_service_registry - workflow = app.workflow + workflow = _ready_workflow(app) if workflow is None: - app.session.add_error("Workflow not initialized") + app.session.add_warning( + "The knowledge base is not available yet — the workflow is " + "still initializing. Try /kb-backfill again in a moment." + ) return project_kb = workflow.kb_manager diff --git a/examples/research_demo/settings.py b/examples/research_demo/settings.py index 9f58c4d..a699be8 100644 --- a/examples/research_demo/settings.py +++ b/examples/research_demo/settings.py @@ -12,12 +12,20 @@ class ResearchDemoSettings(BaseSettings): Demonstrates all P0/P1 features with memory, planning, and HITL. - Settings are loaded from (in order of precedence): - 1. Environment variables (RESEARCH_DEMO_* prefix) - 2. Project config (./settings.json) - 3. User config (~/.research_demo/settings.json) - 4. .env file (~/.research_demo/.env) - 5. Default values + Settings are loaded from (highest precedence first): + + 1. Constructor arguments + 2. Environment variables (``RESEARCH_DEMO_*`` prefix) + 3. Project config ``./.research_demo/settings.json`` — **untrusted**: a + cloned repo can ship one, so only an explicit allowlist of benign keys + is honoured and security-sensitive keys are dropped with a warning + 4. User config ``~/.research_demo/settings.json`` (trusted) + 5. ``~/.research_demo/.env`` — trusted because the path is absolute; a + cwd-relative ``.env`` would be filtered like the project config, so put + API keys here or in real environment variables + 6. Field defaults (including the ones set in ``model_post_init`` below) + + JSON sources are only consulted when the file exists. """ model_config = SettingsConfigDict( diff --git a/pyproject.toml b/pyproject.toml index 5a5b742..54bfd75 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -52,6 +52,10 @@ dev = [ "pytest>=8.0.0", "pytest-asyncio>=0.24.0", "pytest-cov>=6.0.0", + # The console smoke tests drive the real prompt_toolkit application over a + # pty; it is available transitively today (via ipykernel), which is not a + # guarantee, so declare it. + "pexpect>=4.9.0", ] kb = [ "torch>=2.2.0", @@ -91,6 +95,7 @@ markers = [ "llm: tests that require real LLM API calls (deselect with -m 'not llm')", "docker: tests that require a real container runtime (select with -m docker; set SANDBOX_REQUIRE_DOCKER=1 to fail instead of skip when absent)", "latex: tests that require a host TeX engine (select with -m latex; set LATEX_REQUIRE=1 to fail instead of skip when absent)", + "wheel: acceptance tests that build and install the wheel (select with -m wheel; excluded from the offline suite because they download dependencies)", ] [tool.ruff] diff --git a/src/agentic_cli/workflow/adk/manager.py b/src/agentic_cli/workflow/adk/manager.py index f84f105..5f6b5df 100644 --- a/src/agentic_cli/workflow/adk/manager.py +++ b/src/agentic_cli/workflow/adk/manager.py @@ -751,9 +751,19 @@ def _init_plugins(self) -> list: List of BasePlugin instances to pass to Runner(plugins=...). """ from agentic_cli.workflow.adk.task_progress_plugin import TaskProgressPlugin + from agentic_cli.workflow.adk.transfer_tool_description import ( + TransferToolDescriptionPlugin, + ) plugins: list = [PermissionPlugin()] + # ADK's generated description for its own transfer tool tells the model + # to call `TransferToAgentTool` — a name that does not exist as a tool. + # Corrected on the prepared request; a no-op once ADK ships a fixed + # docstring. See transfer_tool_description for why this is safe. + self._transfer_description_plugin = TransferToolDescriptionPlugin() + plugins.append(self._transfer_description_plugin) + # Task progress tracking via ToolContext.state self._task_progress_plugin = TaskProgressPlugin() plugins.append(self._task_progress_plugin) diff --git a/src/agentic_cli/workflow/adk/permission_plugin.py b/src/agentic_cli/workflow/adk/permission_plugin.py index 2b4935e..cb02ebd 100644 --- a/src/agentic_cli/workflow/adk/permission_plugin.py +++ b/src/agentic_cli/workflow/adk/permission_plugin.py @@ -61,11 +61,34 @@ class name, and never equality. pass +def _native_transfer_tool_type() -> type | None: + """ADK's exact ``TransferToAgentTool`` class, or None if unavailable.""" + try: + from google.adk.tools.transfer_to_agent_tool import TransferToAgentTool + except ImportError: # pragma: no cover - ADK always ships it today + return None + return TransferToAgentTool + + # ADK's own function-tool types: their documented contract is to call exactly # ``self.func``, so the callable's identity is the tool's identity. Matched by # exact type — a subclass may override ``run_async`` and run something else # while still advertising a genuine ``func``. -_TRUSTED_FUNCTION_TOOL_TYPES = (FunctionTool, LongRunningFunctionTool) +# +# ``TransferToAgentTool`` is in the list for the same reason and on the same +# terms. ADK auto-injects it into any agent with ``sub_agents``, and it is a +# ``FunctionTool`` *subclass*, so an exact-type check on ``FunctionTool`` alone +# denied the built-in routing tool as unregistered — delegation could not work +# at all, even with permissions disabled. It is safe to add because ADK +# constructs it as ``super().__init__(func=transfer_to_agent)`` and overrides +# only ``_get_declaration`` (to add the agent-name enum), never ``run_async``: +# what it invokes is still exactly ``self.func``. Listing the exact class keeps +# every other ``FunctionTool`` subclass denied. +_TRUSTED_FUNCTION_TOOL_TYPES = tuple( + t + for t in (FunctionTool, LongRunningFunctionTool, _native_transfer_tool_type()) + if t is not None +) def _trusted_wrapped_callable(tool: "BaseTool") -> Any | None: diff --git a/src/agentic_cli/workflow/adk/transfer_tool_description.py b/src/agentic_cli/workflow/adk/transfer_tool_description.py new file mode 100644 index 0000000..8aa484c --- /dev/null +++ b/src/agentic_cli/workflow/adk/transfer_tool_description.py @@ -0,0 +1,136 @@ +"""Correct the model-visible description of ADK's ``transfer_to_agent`` tool. + +ADK builds ``TransferToAgentTool``'s declaration from the ``transfer_to_agent`` +function's docstring, which (through 1.37.0, the newest 1.x) contains: + + Note: + For most use cases, you should use TransferToAgentTool instead of this + function directly. + +That paragraph is written for *Python callers*, but it ships to the model as +the tool's description — so the model is told, in the tool it is supposed to +call, to call something else. Gemini 3.1 followed the instruction and emitted +``TransferToAgentTool``, which ADK rejects (``Tool 'TransferToAgentTool' not +found``), and delegation failed outright. + +Upstream fixed the docstring in ADK 2.x, which is a major release outside this +project's ``google-adk>=1.34,<2`` pin. So the description is corrected here, on +the prepared request, under conditions narrow enough that the correction simply +stops applying once the installed ADK no longer needs it: + +- only when the tool object is **exactly** ``TransferToAgentTool`` — an + application tool that happens to be named ``transfer_to_agent`` is left alone; +- only when the misleading sentence is actually present — so it is idempotent, + and an ADK release that fixes the text makes this a no-op; +- only ``description`` is written. The declaration name, parameter schema, + required fields and ADK's enum of valid agent names are untouched, and the + upstream function's ``__doc__`` is never mutated (ADK rebuilds the + declaration per request, so the edit is scoped to one ``LlmRequest``). +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Iterator + +from google.adk.plugins.base_plugin import BasePlugin + +from agentic_cli.logging import Loggers + +if TYPE_CHECKING: + from google.adk.agents.callback_context import CallbackContext + from google.adk.models.llm_request import LlmRequest + from google.genai import types + +logger = Loggers.workflow() + +#: The declaration name ADK uses. Also the name the model must call. +TRANSFER_TOOL_NAME = "transfer_to_agent" + +#: The fragment that makes ADK's generated description actively harmful. Its +#: presence is the trigger; its absence means there is nothing to correct. +_MISLEADING_FRAGMENT = "you should use TransferToAgentTool" + +#: Replacement: says plainly what to call, and defers to the parameter schema +#: for *which* agents are valid rather than restating them (ADK's enum is the +#: authority and stays intact). +ROUTING_DESCRIPTION = ( + "Route the current request to another agent. " + f'Call this function by its exact name: {TRANSFER_TOOL_NAME}' + '(agent_name=""). ' + "Choose agent_name from the enum of valid agent names in this tool's " + "parameter schema — those are the only accepted values. " + "Transfer when another agent's description fits the user's request better " + "than your own." +) + + +def _native_transfer_tool_type() -> type | None: + """ADK's exact ``TransferToAgentTool`` class, or None if unavailable.""" + try: + from google.adk.tools.transfer_to_agent_tool import TransferToAgentTool + except ImportError: # pragma: no cover - ADK always ships it today + return None + return TransferToAgentTool + + +def _function_declarations(llm_request: "LlmRequest") -> Iterator["types.FunctionDeclaration"]: + """Every function declaration attached to the prepared request.""" + config = getattr(llm_request, "config", None) + for tool in getattr(config, "tools", None) or []: + for declaration in getattr(tool, "function_declarations", None) or []: + yield declaration + + +def correct_transfer_declaration(llm_request: "LlmRequest") -> bool: + """Rewrite the transfer tool's description on this request, if warranted. + + Returns: + True if a declaration was corrected (useful for tests and logging). + """ + transfer_type = _native_transfer_tool_type() + if transfer_type is None: + return False + + tool = (getattr(llm_request, "tools_dict", None) or {}).get(TRANSFER_TOOL_NAME) + # Exact type, never isinstance: a subclass may declare anything it likes, + # and an application tool sharing the name is not ADK's routing primitive. + if type(tool) is not transfer_type: + return False + + corrected = False + for declaration in _function_declarations(llm_request): + if declaration.name != TRANSFER_TOOL_NAME: + continue + if _MISLEADING_FRAGMENT not in (declaration.description or ""): + continue # already correct (or fixed upstream) — leave it alone + declaration.description = ROUTING_DESCRIPTION + corrected = True + + return corrected + + +class TransferToolDescriptionPlugin(BasePlugin): + """Applies :func:`correct_transfer_declaration` to every model request.""" + + def __init__(self) -> None: + super().__init__(name="transfer_tool_description") + self._corrections = 0 + + @property + def corrections(self) -> int: + """How many requests have been corrected (diagnostics/tests).""" + return self._corrections + + async def before_model_callback( + self, + *, + callback_context: "CallbackContext", + llm_request: "LlmRequest", + ) -> None: + """Fix the transfer description in place; never blocks the request.""" + try: + if correct_transfer_declaration(llm_request): + self._corrections += 1 + except Exception as exc: # noqa: BLE001 - never break a turn over this + logger.warning("transfer_description_correction_failed", error=str(exc)) + return None diff --git a/tests/demo_isolation.py b/tests/demo_isolation.py new file mode 100644 index 0000000..cb52556 --- /dev/null +++ b/tests/demo_isolation.py @@ -0,0 +1,158 @@ +"""Construct fully-isolated ``ResearchDemoSettings`` for tests. + +Plumbing shared by the headless application tests and the live scenario tests; +it defines no test classes, so pytest does not collect it. + +``ResearchDemoSettings`` reads from three developer-owned places, and they are +*not* isolated by the same mechanism: + +=========================== ========================== ==================== +source resolved isolated by +=========================== ========================== ==================== +``./.research_demo/…json`` ``Path.cwd()`` per call ``monkeypatch.chdir`` +``~/.research_demo/…json`` ``Path.home()`` per call ``monkeypatch.setenv`` +``~/.research_demo/.env`` ``Path.home()`` **at an explicit + class-definition time** ``_env_file=`` +=========================== ========================== ==================== + +The third is the trap: ``model_config["env_file"]`` is evaluated when the +module is imported — which, under pytest, is during collection, long before any +fixture runs. Redirecting ``HOME`` afterwards does nothing to it, so a test that +only patched ``HOME`` was still reading the developer's real dotenv (and any +API key in it). Passing ``_env_file`` explicitly is the only thing that moves +it, so every fixture here does. +""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path +from typing import Any + +# The real home, captured at import (collection time) before any fixture has +# had a chance to redirect HOME. This is what isolation is asserted *against*. +REAL_HOME = Path(os.path.expanduser("~")).resolve() + +_REPO_ROOT = Path(__file__).resolve().parents[1] +if str(_REPO_ROOT) not in sys.path: + sys.path.insert(0, str(_REPO_ROOT)) + +from examples.research_demo.settings import ResearchDemoSettings # noqa: E402 + + +def isolated_env_file(home: Path) -> Path: + """Where an isolated run's dotenv lives (it need not exist).""" + return home / ".research_demo" / ".env" + + +def make_isolated_settings( + *, home: Path, workspace: Path, **kwargs: Any +) -> ResearchDemoSettings: + """``ResearchDemoSettings`` that cannot read the developer's configuration. + + The caller is still responsible for redirecting ``HOME`` and ``cwd`` + (``monkeypatch``) — those cover the two JSON sources. This adds the third: + an explicit ``_env_file`` under the temp home, overriding the class's + import-time default. + """ + kwargs.setdefault("_env_file", str(isolated_env_file(home))) + return ResearchDemoSettings(workspace_dir=workspace, **kwargs) + + +def effective_env_file(**kwargs: Any) -> Path | None: + """The dotenv path ``ResearchDemoSettings`` would *actually* read. + + Captured from the ``dotenv_settings`` source pydantic-settings hands to + ``settings_customise_sources`` — the real value in effect, not the class + default and not what the caller hoped it passed. + + Returns: + The resolved path, or None when the dotenv source is disabled. + """ + captured: list[Any] = [] + + class _Probe(ResearchDemoSettings): + @classmethod + def settings_customise_sources( # type: ignore[override] + cls, + settings_cls, + init_settings, + env_settings, + dotenv_settings, + file_secret_settings, + ): + captured.append(getattr(dotenv_settings, "env_file", None)) + return super().settings_customise_sources( + settings_cls, + init_settings, + env_settings, + dotenv_settings, + file_secret_settings, + ) + + _Probe(**kwargs) + assert captured, "settings_customise_sources was never called" + value = captured[0] + if value is None: + return None + if isinstance(value, (list, tuple)): + assert len(value) == 1, f"expected a single env_file, got {value}" + value = value[0] + return Path(value).resolve() + + +def assert_dotenv_isolated(env_file: Path | None, home: Path) -> None: + """The effective dotenv must be inside the temp home, or disabled. + + Asserting "not under the real home" alone would pass for a path that is + simply somewhere else on disk, so this pins the positive form too. + """ + if env_file is None: + return # dotenv disabled outright — isolated by construction + home = home.resolve() + assert env_file.is_relative_to(home), ( + f"the effective dotenv is {env_file}, which is not under the test's " + f"temporary home {home}" + ) + assert not env_file.is_relative_to(REAL_HOME), ( + f"the effective dotenv resolves under the developer's real home: {env_file}" + ) + + +def suppress_global_logging_config(monkeypatch) -> None: + """Stop app construction from reconfiguring logging process-wide. + + ``BaseCLIApp.__init__`` calls ``configure_logging()``, which reconfigures + structlog globally with ``cache_logger_on_first_use=True``. That cannot be + undone afterwards: by the time the constructor returns, the module-level + loggers have already bound and *cached* a logger, so restoring the previous + configuration leaves those caches in place and every later test using + ``structlog.testing.capture_logs()`` silently captures nothing. + + So it is prevented rather than reverted. These tests assert on command + routing and rendering, never on log output, so the application's logging + configuration is not part of what they cover. + """ + monkeypatch.setattr( + "agentic_cli.cli.app.configure_logging", lambda *a, **k: None + ) + + +def assert_no_global_settings_leak() -> None: + """No test may leave a global settings singleton behind. + + ``set_settings()`` writes a process-wide singleton with no teardown, so one + test's settings would silently answer ``get_settings()`` for every later + test that runs outside a manager's ``SettingsContext``. + """ + from agentic_cli import config as _config + + assert _config._settings_instance is None, ( + "a test left a global settings singleton behind " + f"({_config._settings_instance!r}); use the manager's SettingsContext " + "instead of set_settings()" + ) + assert _config._settings_context.get() is None, ( + "a test left settings in the context variable" + ) diff --git a/tests/examples/console_smoke.py b/tests/examples/console_smoke.py new file mode 100644 index 0000000..233c380 --- /dev/null +++ b/tests/examples/console_smoke.py @@ -0,0 +1,274 @@ +"""Shared driver for the research-demo console smoke tests. + +Plumbing only — no test classes, so pytest does not collect it. Used by both +``test_research_demo_pty.py`` (source checkout, ``python -m research_demo``) +and ``test_research_demo_wheel.py`` (built wheel, the installed +``research-demo`` console script), so the two exercise *the same* interaction +against different installations and different entry points. + +The demo is a full-screen prompt_toolkit application, so it needs a real +terminal: it is driven over a pty with ``pexpect`` and its output is +ANSI-stripped before matching. Matching is on short, stable fragments rather +than a snapshot of raw terminal bytes — the screen is redrawn repeatedly and +line-wrapped to the terminal width, so raw output is not stable. +""" + +from __future__ import annotations + +import io +import json +import os +import re +import subprocess +import sys +import tomllib +from dataclasses import dataclass, field +from pathlib import Path + +_REPO_ROOT = Path(__file__).resolve().parents[2] + + +def project_version() -> str: + """The version declared in the repository's ``pyproject.toml``. + + Derived, never hardcoded: a release-specific literal would pin the + acceptance suite to one release, going red on every branch that predates a + version bump and needing an edit at each bump. Reading the declared version + instead means these assertions validate *whatever* is being released. + """ + with (_REPO_ROOT / "pyproject.toml").open("rb") as fh: + return tomllib.load(fh)["project"]["version"] + +# CSI/OSC escape sequences, charset selects, and bare CRs, so matching sees +# the text a human reads rather than the bytes that drew it. +_ANSI = re.compile( + r"\x1b\[[0-9;?]*[a-zA-Z]" # CSI + r"|\x1b\][^\x07\x1b]*(?:\x07|\x1b\\)" # OSC + r"|\x1b[()][A-Za-z0-9]" # charset select + r"|\x1b[=>]" # keypad mode + r"|\r" +) + +#: The *only* parent variables allowed through to the child. Everything else — +#: provider credentials, cloud authentication, ``RESEARCH_DEMO_*``/``AGENTIC_*`` +#: settings overrides, custom config paths, ``PYTHONPATH`` — is dropped, so the +#: smoke measures the shipped defaults rather than whatever the developer (or +#: CI) happens to export. These are what it takes to *launch* an interpreter and +#: a terminal, nothing about the application. +ALLOWED_PARENT_VARS = ( + "PATH", # find the interpreter/console script and anything it execs + "LANG", # terminal text encoding + "LC_ALL", + "LC_CTYPE", + "TMPDIR", # POSIX temp location (macOS gives every user its own) + "SYSTEMROOT", # Windows needs these to start a process at all + "SystemRoot", + "COMSPEC", +) + +#: Set explicitly on every child, never inherited. +EXPLICIT_CHILD_VARS = ( + "HOME", + "TERM", + "NO_COLOR", + "PYTHONUNBUFFERED", + "PYTHONDONTWRITEBYTECODE", +) + +#: Terminal size. Wide enough that the help table's cells do not wrap, which is +#: what makes short-fragment matching reliable. +TERM_SIZE = (40, 200) + +#: Every wait is bounded. Startup measured ~1s on a warm checkout; 60s is +#: headroom for a cold import on CI, not an invitation to hang. +STARTUP_TIMEOUT = 60 +STEP_TIMEOUT = 30 + + +@dataclass +class SmokeResult: + """What the console session did, for the caller to assert on.""" + + transcript: str + exit_status: int | None + signal_status: int | None + argv: list[str] + steps: list[str] = field(default_factory=list) + + def saw(self, fragment: str) -> bool: + return fragment in self.transcript + + +@dataclass +class ChildImports: + """Where a child process resolved the packages under test.""" + + demo: Path + framework: Path + version: str + executable: Path + + +def child_env(home: Path) -> dict[str, str]: + """A minimal, allowlisted environment for the child process. + + Built up from nothing rather than filtered down from ``os.environ``: a + deny-list only removes the hazards someone thought of, and the ones that + matter here (``RESEARCH_DEMO_*``, ``AGENTIC_*``, ``GOOGLE_APPLICATION_ + CREDENTIALS``, ``VERTEX_*``) are open-ended. + + ``HOME`` is redirected so the demo's user config, user KB and + ``~/.research_demo/.env`` all resolve inside the temp dir; ``PYTHONPATH`` is + absent so the child cannot pick up a checkout the caller did not intend. + """ + env = {k: os.environ[k] for k in ALLOWED_PARENT_VARS if k in os.environ} + env.update( + HOME=str(home), + TERM="xterm-256color", + NO_COLOR="1", + PYTHONUNBUFFERED="1", + PYTHONDONTWRITEBYTECODE="1", + ) + return env + + +def probe_child_imports(python: str, cwd: Path, home: Path) -> ChildImports: + """Ask a child process — same interpreter, cwd and env — what it imports. + + The parent pytest process is *not* evidence: it has the editable install on + ``sys.path`` and whatever the developer's environment supplies. Only the + child can say where the child resolves ``research_demo``. + """ + code = ( + "import json, sys, research_demo, agentic_cli; " + "print(json.dumps({" + "'demo': research_demo.__file__, " + "'framework': agentic_cli.__file__, " + "'version': agentic_cli.__version__, " + "'executable': sys.executable}))" + ) + result = subprocess.run( + [python, "-c", code], + cwd=str(cwd), + env=child_env(home), + capture_output=True, + text=True, + timeout=STEP_TIMEOUT, + ) + if result.returncode != 0: + raise AssertionError( + f"import probe failed ({result.returncode}):\n" + f"stdout:\n{result.stdout}\nstderr:\n{result.stderr}" + ) + payload = json.loads(result.stdout.strip().splitlines()[-1]) + return ChildImports( + demo=Path(payload["demo"]).resolve(), + framework=Path(payload["framework"]).resolve(), + version=payload["version"], + executable=Path(payload["executable"]).resolve(), + ) + + +def run_console_smoke(argv: list[str], cwd: Path, home: Path) -> SmokeResult: + """Drive a console command through startup → /help → /exit. + + Every wait is an ``expect`` on observable output, never a sleep, so the + test is deterministic rather than timing-dependent. + + Args: + argv: The command to launch — ``[python, "-m", "research_demo"]`` for + the source checkout, or ``[".../bin/research-demo"]`` for the + installed console script. + cwd: Working directory for the child — keeps ``./.research_demo/`` + (project KB, permission workdir) out of the repo. + home: Value for ``HOME``. + + Returns: + The full ANSI-stripped transcript plus the process's exit status. + """ + import pexpect + + log = io.StringIO() + child = pexpect.spawn( + argv[0], + list(argv[1:]), + cwd=str(cwd), + env=child_env(home), + encoding="utf-8", + codec_errors="replace", + timeout=STARTUP_TIMEOUT, + dimensions=TERM_SIZE, + ) + child.logfile_read = log + steps: list[str] = [] + try: + # 1. Startup: the welcome panel is drawn and the prompt appears. + child.expect(r">>>", timeout=STARTUP_TIMEOUT) + steps.append("prompt") + + # 2. /help renders the command table. + child.sendline("/help") + child.expect(r"Exit the application", timeout=STEP_TIMEOUT) + steps.append("help") + + # 3. The prompt comes back, so the app is still interactive. + child.expect(r">>>", timeout=STEP_TIMEOUT) + steps.append("prompt-after-help") + + # 4. /exit shuts down cleanly. + child.sendline("/exit") + child.expect(r"Goodbye!", timeout=STEP_TIMEOUT) + steps.append("goodbye") + child.expect(pexpect.EOF, timeout=STEP_TIMEOUT) + steps.append("eof") + finally: + child.close() + + return SmokeResult( + transcript=_ANSI.sub("", log.getvalue()), + exit_status=child.exitstatus, + signal_status=child.signalstatus, + argv=list(argv), + steps=steps, + ) + + +def assert_clean_session(result: SmokeResult) -> None: + """Shared assertions: the demo started, responded, and exited cleanly.""" + assert result.steps == [ + "prompt", + "help", + "prompt-after-help", + "goodbye", + "eof", + ], f"console session did not complete: reached {result.steps} via {result.argv}" + + assert result.saw("Exit the application"), "the /help table was not rendered" + assert result.saw("Goodbye!"), "the app did not say goodbye on /exit" + + for marker in ("Traceback (most recent call last)", "Unhandled exception"): + assert marker not in result.transcript, ( + f"{marker!r} in console output:\n{_tail(result.transcript)}" + ) + + assert result.signal_status is None, ( + f"the console process died on signal {result.signal_status}" + ) + assert result.exit_status == 0, ( + f"console exited with status {result.exit_status}:\n{_tail(result.transcript)}" + ) + + +def _tail(text: str, lines: int = 40) -> str: + return "\n".join(text.splitlines()[-lines:]) + + +def platform_skip_reason() -> str | None: + """Why this platform cannot run a pty smoke, or None if it can.""" + if sys.platform.startswith("win"): + return ( + "the console smoke needs a POSIX pty: Python's `pty` module and " + "`pexpect.spawn` are POSIX-only, and prompt_toolkit's full-screen " + "app cannot be driven through a pipe" + ) + return None diff --git a/tests/examples/test_research_demo_app.py b/tests/examples/test_research_demo_app.py new file mode 100644 index 0000000..5124432 --- /dev/null +++ b/tests/examples/test_research_demo_app.py @@ -0,0 +1,313 @@ +"""Headless acceptance tests for the *composed* research-demo application. + +The workflow, the renderer and the individual commands all have component +tests. What none of them covers is the wiring: a real ``ResearchDemoApp`` — its +own command registry, ``BaseCLIApp.process_input``, the real +``MessageProcessor`` — reacting to real input. + +So these build the actual app and drive ``process_input()``. The only things +substituted are the two nondeterministic ones: + +- the **workflow manager**, replaced by a scripted ``WorkflowEvent`` stream + (no LLM, no network) — ``WorkflowEvent`` is the UI-independent boundary the + framework already defines, so scripting it is not a new seam; +- the **UI session**, replaced by ``RecordingSession`` (the same stand-in + ``test_message_processor_render.py`` uses) so assertions are on semantic + render calls rather than terminal bytes. + +Assertions are deliberately about *shape* — which kind of call happened, in +what order, was it a warning or an error — never about generated prose. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +from agentic_cli.workflow.events import WorkflowEvent +from tests.demo_isolation import ( + assert_dotenv_isolated, + assert_no_global_settings_leak, + effective_env_file, + isolated_env_file, + make_isolated_settings, + suppress_global_logging_config, +) +from tests.event_replay import RecordingSession, ReplayController + +# Make the `examples` namespace package importable (no __init__.py). +_REPO_ROOT = Path(__file__).resolve().parents[2] +if str(_REPO_ROOT) not in sys.path: + sys.path.insert(0, str(_REPO_ROOT)) + +from examples.research_demo.app import ResearchDemoApp # noqa: E402 + + +class ScriptedWorkflow: + """Workflow-manager stand-in driving one scripted turn per call. + + Extends what ``ReplayWorkflow`` offers with the two things these tests + need: several distinct turns in sequence, and a turn that fails — so + "the app is still usable afterwards" can actually be asserted. + """ + + def __init__(self, turns: list[list[WorkflowEvent] | Exception]) -> None: + self._turns = list(turns) + self.messages: list[str] = [] + self.session_ids: list[str | None] = [] + self.input_callback = None + + def set_input_callback(self, callback) -> None: # noqa: ANN001 + self.input_callback = callback + + def clear_input_callback(self) -> None: + self.input_callback = None + + async def process(self, message: str, user_id: str, session_id: str | None = None): + self.messages.append(message) + self.session_ids.append(session_id) + turn = self._turns.pop(0) if self._turns else [] + if isinstance(turn, Exception): + raise turn + for event in turn: + yield event + + +@pytest.fixture +def demo_app(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> ResearchDemoApp: + """A real ResearchDemoApp on an isolated HOME/cwd, with a recording UI. + + HOME and cwd are both redirected: the demo derives its user config, user + KB and ``.env`` from ``~/.research_demo`` and its project KB and permission + workdir from ``./.research_demo``, and neither may touch the developer's + real config or the repo. + + The dotenv needs the third, explicit step (``make_isolated_settings``): + ``model_config["env_file"]`` was frozen to the developer's real + ``~/.research_demo/.env`` when this module was imported, so redirecting + HOME cannot move it. + + The workflow controller is left as the **real** one, uninitialized — that + is what makes ``app.workflow`` raise, which is exactly the state the + readiness tests are about. Tests that need a turn install a scripted + controller themselves. + """ + home = tmp_path / "home" + project = tmp_path / "project" + workspace = tmp_path / "ws" + for d in (home, project, workspace): + d.mkdir(parents=True) + monkeypatch.setenv("HOME", str(home)) + monkeypatch.chdir(project) + + settings = make_isolated_settings( + home=home, + workspace=workspace, + permissions_enabled=False, + ) + # Building a real app would otherwise reconfigure structlog globally and + # break every later test that captures logs (see the helper's docstring). + suppress_global_logging_config(monkeypatch) + + app = ResearchDemoApp(settings=settings) + app.session = RecordingSession() + app._test_home = home # for the isolation assertions below + return app + + +def _script(app: ResearchDemoApp, *turns) -> ScriptedWorkflow: + """Install a scripted workflow behind the app's controller seam.""" + workflow = ScriptedWorkflow(list(turns)) + app._workflow_controller = ReplayController(workflow) + return workflow + + +class TestFixtureIsolation: + """The fixture must not be able to read the developer's configuration. + + Worth its own tests because the failure is silent: a headless test that + quietly picked up a real API key from ``~/.research_demo/.env`` would still + pass, while proving something different from what it claims. + """ + + def test_effective_dotenv_is_under_the_temporary_home(self, demo_app) -> None: + home = demo_app._test_home + env_file = effective_env_file( + workspace_dir=home / "ws", + _env_file=str(isolated_env_file(home)), + ) + assert_dotenv_isolated(env_file, home) + + def test_the_class_default_would_not_have_been_isolated(self) -> None: + """Pin the hazard this fixture works around. + + If the class ever stops resolving its dotenv at import time, this test + fails and the ``_env_file`` plumbing can be simplified away. + """ + from tests.demo_isolation import REAL_HOME, ResearchDemoSettings + + class_default = Path(str(ResearchDemoSettings.model_config["env_file"])) + assert class_default.is_relative_to(REAL_HOME), ( + "ResearchDemoSettings no longer freezes its env_file under the real " + f"home ({class_default}) — re-check whether _env_file is still needed" + ) + + def test_settings_do_not_leak_globally(self, demo_app) -> None: + """Building the app must not install a process-wide settings singleton.""" + assert_no_global_settings_leak() + + +class TestCommandsBeforeReadiness: + """Demo commands must degrade to a warning while the workflow comes up. + + ``app.workflow`` raises until the controller reports READY. A command that + touched it during background init was caught by + ``BaseCLIApp._handle_command`` and surfaced as the generic + "Error executing command: Workflow not initialized yet" — which reads like + a bug rather than "not yet". + """ + + async def test_memory_warns_and_does_not_error(self, demo_app): + with pytest.raises(RuntimeError): + _ = demo_app.workflow # precondition: genuinely not ready + + await demo_app.process_input("/memory") + + assert demo_app.session.errors() == [], ( + f"/memory errored before readiness: {demo_app.session.errors()}" + ) + assert len(demo_app.session.warnings()) == 1 + assert "initializing" in demo_app.session.warnings()[0].lower() + + async def test_kb_backfill_warns_and_does_not_error(self, demo_app): + await demo_app.process_input("/kb-backfill") + + assert demo_app.session.errors() == [], ( + f"/kb-backfill errored before readiness: {demo_app.session.errors()}" + ) + assert len(demo_app.session.warnings()) == 1 + assert "initializing" in demo_app.session.warnings()[0].lower() + + async def test_the_command_is_still_registered_and_echoed(self, demo_app): + """A warning must come from the command, not from it being unknown.""" + await demo_app.process_input("/memory") + + echoed = [c for c in demo_app.session.calls if c[0] == "message"] + assert ("message", "user", "/memory") in echoed + assert not any("Unknown command" in e for e in demo_app.session.errors()) + + +class TestMessageRouting: + """A plain message goes through MessageProcessor and renders its events.""" + + async def test_scripted_turn_is_rendered_in_order(self, demo_app): + workflow = _script( + demo_app, + [ + WorkflowEvent.thinking("Considering the question."), + WorkflowEvent.tool_call("kb_search", {"query": "decoding"}), + WorkflowEvent.tool_result( + "kb_search", {"success": True}, success=True, duration_ms=5 + ), + WorkflowEvent.text("Here is what I found."), + ], + ) + + await demo_app.process_input("what do you know about decoding?") + + assert workflow.messages == ["what do you know about decoding?"] + + kinds = demo_app.session.kinds() + # The user's message is echoed before anything is rendered for the turn. + assert kinds[0] == "message" + assert demo_app.session.calls[0][1] == "user" + # The scripted text reached the UI as a response, after the echo. + assert "Here is what I found." in demo_app.session.responses() + assert kinds.index("response") > 0 + # A tool call opened the events box before the response was written. + assert "start_thinking" in kinds + assert kinds.index("start_thinking") < kinds.index("response") + assert demo_app.session.errors() == [] + + async def test_turn_receives_the_application_session_id(self, demo_app): + workflow = _script(demo_app, [WorkflowEvent.text("ok")]) + + await demo_app.process_input("hello") + + assert workflow.session_ids == [demo_app.session_id] + assert demo_app.session_id, "the app must carry a durable session id" + + async def test_blank_input_is_ignored(self, demo_app): + workflow = _script(demo_app, [WorkflowEvent.text("should not run")]) + + await demo_app.process_input(" ") + + assert workflow.messages == [] + assert demo_app.session.calls == [] + + +class TestUnknownCommand: + async def test_unknown_command_errors_with_a_hint(self, demo_app): + await demo_app.process_input("/definitely-not-a-command") + + errors = demo_app.session.errors() + assert len(errors) == 1 + assert "definitely-not-a-command" in errors[0] + # And the user is pointed somewhere useful. + assert any( + "/help" in c[2] for c in demo_app.session.calls if c[0] == "message" + ) + + async def test_unknown_command_does_not_reach_the_workflow(self, demo_app): + workflow = _script(demo_app, [WorkflowEvent.text("should not run")]) + + await demo_app.process_input("/nope") + + assert workflow.messages == [] + + +class TestRecoveryAfterFailure: + """A failed turn must not wedge the app: the next turn still works.""" + + async def test_failed_turn_is_reported_then_the_next_turn_succeeds(self, demo_app): + workflow = _script( + demo_app, + RuntimeError("backend exploded"), + [WorkflowEvent.text("second turn is fine")], + ) + + await demo_app.process_input("first") + assert demo_app.session.errors(), "a failing turn reported nothing" + first_errors = len(demo_app.session.errors()) + + await demo_app.process_input("second") + + assert workflow.messages == ["first", "second"] + assert "second turn is fine" in demo_app.session.responses() + assert len(demo_app.session.errors()) == first_errors, ( + "the recovery turn produced a new error" + ) + + async def test_a_command_error_does_not_wedge_the_next_turn(self, demo_app): + """An exception inside a command is contained by _handle_command.""" + workflow = _script(demo_app, [WorkflowEvent.text("still working")]) + + boom = demo_app.command_registry.get("memory") + + async def _raise(args, app): # noqa: ANN001 + raise RuntimeError("command exploded") + + monkey = boom.execute + boom.execute = _raise + try: + await demo_app.process_input("/memory") + finally: + boom.execute = monkey + + assert any("command exploded" in e for e in demo_app.session.errors()) + + await demo_app.process_input("carry on") + assert workflow.messages == ["carry on"] + assert "still working" in demo_app.session.responses() diff --git a/tests/examples/test_research_demo_pty.py b/tests/examples/test_research_demo_pty.py new file mode 100644 index 0000000..0eeee06 --- /dev/null +++ b/tests/examples/test_research_demo_pty.py @@ -0,0 +1,219 @@ +"""Real-terminal smoke test for the research-demo console process. + +Everything else in the suite drives Python objects — ``process_input()``, +``MessageProcessor``, the workflow manager. None of it proves the thing a user +actually runs starts up, draws a prompt, answers a command and exits. + +This launches the **actual console process** over a pty and interacts with it: +startup → prompt → ``/help`` → prompt → ``/exit``. It needs no API key, no +network, no Docker and no LLM — the workflow's background initialization is +allowed to fail (there are no credentials), which is itself part of what is +being asserted: a demo with no keys must still come up, answer ``/help`` and +exit 0 rather than crash. + +This module covers the **source checkout** (via ``python -m research_demo`` on +the editable install). ``test_research_demo_wheel.py`` runs the identical +session against the installed console script from a built wheel. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +from tests.examples.console_smoke import ( + ALLOWED_PARENT_VARS, + EXPLICIT_CHILD_VARS, + assert_clean_session, + child_env, + platform_skip_reason, + probe_child_imports, + project_version, + run_console_smoke, +) + +_SKIP = platform_skip_reason() +pytestmark = pytest.mark.skipif(_SKIP is not None, reason=_SKIP or "") + +_REPO_ROOT = Path(__file__).resolve().parents[2] + + +@pytest.fixture +def sandbox(tmp_path: Path) -> tuple[Path, Path]: + """An isolated (home, cwd) pair for one console run.""" + home = tmp_path / "home" + cwd = tmp_path / "cwd" + home.mkdir() + cwd.mkdir() + return home, cwd + + +def _argv() -> list[str]: + """How the source checkout is launched.""" + return [sys.executable, "-m", "research_demo"] + + +class TestChildImportOrigin: + """Pin *which* installation the spawned child resolves. + + The parent pytest process is not evidence: it has the editable install on + ``sys.path`` already. Only a child launched exactly the way the smoke + launches one — same interpreter, cwd, HOME and stripped environment — can + say where the console under test comes from. Without this, a wheel-only + failure would look like a checkout failure, and vice versa. + """ + + def test_child_resolves_the_checkout(self, sandbox) -> None: + home, cwd = sandbox + + imports = probe_child_imports(sys.executable, cwd=cwd, home=home) + + assert imports.demo.is_relative_to(_REPO_ROOT), ( + f"the child resolved research_demo to {imports.demo}, " + "which is not the checkout this module is meant to cover" + ) + assert imports.framework.is_relative_to(_REPO_ROOT), ( + f"the child resolved agentic_cli to {imports.framework}" + ) + assert imports.version == project_version() + + def test_the_probe_uses_the_same_interpreter_as_the_smoke(self, sandbox) -> None: + home, cwd = sandbox + imports = probe_child_imports(sys.executable, cwd=cwd, home=home) + assert imports.executable == Path(sys.executable).resolve() + assert _argv()[0] == sys.executable + + +class TestChildEnvironmentIsHermetic: + """Nothing from the developer's shell may change what the smoke measures.""" + + def test_only_allowlisted_variables_reach_the_child(self, tmp_path) -> None: + env = child_env(tmp_path) + unexpected = set(env) - set(ALLOWED_PARENT_VARS) - set(EXPLICIT_CHILD_VARS) + assert unexpected == set(), f"unexpected variables in the child env: {unexpected}" + + def test_explicit_variables_are_set(self, tmp_path) -> None: + env = child_env(tmp_path) + assert env["HOME"] == str(tmp_path) + for name in EXPLICIT_CHILD_VARS: + assert name in env, f"{name} was not set on the child" + + @pytest.mark.parametrize( + "name, value", + [ + # Provider credentials + ("GOOGLE_API_KEY", "sk-should-not-leak"), + ("ANTHROPIC_API_KEY", "sk-should-not-leak"), + ("TAVILY_API_KEY", "sk-should-not-leak"), + # Cloud authentication + ("GOOGLE_APPLICATION_CREDENTIALS", "/tmp/creds.json"), + ("GOOGLE_CLOUD_PROJECT", "some-project"), + ("GOOGLE_GENAI_USE_VERTEXAI", "true"), + ("VERTEX_LOCATION", "us-central1"), + # Application settings overrides + ("RESEARCH_DEMO_DEFAULT_MODEL", "some-other-model"), + ("RESEARCH_DEMO_WORKSPACE_DIR", "/tmp/elsewhere"), + ("RESEARCH_DEMO_PERMISSIONS_ENABLED", "false"), + ("AGENTIC_ORCHESTRATOR", "langgraph"), + ("AGENTIC_CLI_LOG_LEVEL", "debug"), + # Import path + ("PYTHONPATH", "/somewhere/else"), + ("PYTHONSTARTUP", "/tmp/startup.py"), + ], + ) + def test_hazardous_variables_never_reach_the_child( + self, tmp_path, monkeypatch, name, value + ) -> None: + """Set it in the parent; it must still be absent from the child env.""" + monkeypatch.setenv(name, value) + + env = child_env(tmp_path) + + assert name not in env, ( + f"{name} leaked into the console child and could change the smoke" + ) + + def test_a_leaked_setting_would_actually_change_the_demo( + self, tmp_path, monkeypatch + ) -> None: + """The pin above is only worth having if such a variable would matter. + + Proves ``RESEARCH_DEMO_DEFAULT_MODEL`` really is load-bearing, so the + allowlist is protecting against something real rather than asserting a + vacuous property. + """ + # `tests.demo_isolation` already puts the repo root on sys.path at + # import; scoped here too so this test does not depend on that and does + # not leave an entry behind if it ever becomes the first importer. + monkeypatch.syspath_prepend(str(_REPO_ROOT)) + from examples.research_demo.settings import ResearchDemoSettings + + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("RESEARCH_DEMO_DEFAULT_MODEL", "sentinel-model") + + settings = ResearchDemoSettings( + workspace_dir=tmp_path / "ws", _env_file=None + ) + assert settings.default_model == "sentinel-model" + + +class TestConsoleSmoke: + """The shipped console entry point, driven as a user would drive it.""" + + def test_startup_help_and_exit(self, sandbox) -> None: + home, cwd = sandbox + + result = run_console_smoke(_argv(), cwd=cwd, home=home) + + assert_clean_session(result) + + def test_a_help_and_exit_session_is_side_effect_free(self, sandbox) -> None: + """Starting up, asking for /help and exiting must write nothing at all. + + The demo resolves its project KB and permission workdir from ``cwd`` + and its user config, user KB and session store from ``HOME``. A session + that never runs a turn should touch none of them — so this asserts both + that the repository is untouched (a leaked path would litter the + checkout) *and* that the temp HOME/cwd are still empty, which is the + stronger statement and the one that would catch a stray write landing + somewhere the repo check does not look. + """ + home, cwd = sandbox + + before = {p.name for p in _REPO_ROOT.iterdir()} + + result = run_console_smoke(_argv(), cwd=cwd, home=home) + assert_clean_session(result) + + assert {p.name for p in _REPO_ROOT.iterdir()} == before, ( + "the console run created entries in the repository root" + ) + assert list(home.rglob("*")) == [], ( + f"the run wrote into HOME: {[str(p) for p in home.rglob('*')][:10]}" + ) + assert list(cwd.rglob("*")) == [], ( + f"the run wrote into cwd: {[str(p) for p in cwd.rglob('*')][:10]}" + ) + + def test_runs_without_any_credentials_in_the_environment( + self, sandbox, monkeypatch + ) -> None: + """Even with keys exported in the parent, the child runs without them. + + ``monkeypatch.setenv`` rather than writing ``os.environ`` directly: a + developer running this with a real ``GOOGLE_API_KEY`` exported would + otherwise have it deleted by the cleanup, because ``os.environ.pop`` + cannot restore a value it never saved. + """ + home, cwd = sandbox + monkeypatch.setenv("GOOGLE_API_KEY", "sk-not-a-real-key") + + env = child_env(home) + assert "GOOGLE_API_KEY" not in env + + result = run_console_smoke(_argv(), cwd=cwd, home=home) + + assert_clean_session(result) diff --git a/tests/examples/test_research_demo_wheel.py b/tests/examples/test_research_demo_wheel.py new file mode 100644 index 0000000..cb40484 --- /dev/null +++ b/tests/examples/test_research_demo_wheel.py @@ -0,0 +1,270 @@ +"""Acceptance smoke against the **built wheel**, not the checkout. + +Every other test in the suite imports the source tree. That cannot catch a +packaging defect: a module or data file left out of +``[tool.hatch.build.targets.wheel]`` is invisible until someone installs the +artifact. This builds the wheel, installs it into an isolated virtualenv, and +runs the same console session ``test_research_demo_pty.py`` runs — through the +**installed ``research-demo`` console script**, which is what a user actually +types, not ``python -m``. + +Building a wheel and installing its dependencies takes minutes and needs the +network, which is exactly what the offline suite is not. So this is opt-in on +two axes: it carries the ``wheel`` marker *and* requires +``AGENTIC_WHEEL_ACCEPTANCE=1``. That keeps the offline selector +(``-m 'not llm and not docker'``) fast and network-free without redefining it, +and CI runs the acceptance as its own step: + + AGENTIC_WHEEL_ACCEPTANCE=1 conda run -n agenticcli python -m pytest \ + tests/examples/test_research_demo_wheel.py -m wheel -v +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import venv +from pathlib import Path + +import pytest + +from tests.examples.console_smoke import ( + assert_clean_session, + platform_skip_reason, + probe_child_imports, + project_version, + run_console_smoke, +) + +_PLATFORM_SKIP = platform_skip_reason() +_OPT_IN = os.environ.get("AGENTIC_WHEEL_ACCEPTANCE") == "1" + +pytestmark = [ + pytest.mark.wheel, + pytest.mark.skipif(_PLATFORM_SKIP is not None, reason=_PLATFORM_SKIP or ""), + pytest.mark.skipif( + not _OPT_IN, + reason=( + "wheel acceptance builds a wheel and installs its dependencies " + "(slow + needs network); set AGENTIC_WHEEL_ACCEPTANCE=1 to run it" + ), + ), +] + +_REPO_ROOT = Path(__file__).resolve().parents[2] + +#: Non-Python files the demo cannot work without. They live under +#: ``examples/research_demo`` and are only in the wheel because hatchling +#: packages that directory — exactly the kind of thing that silently vanishes. +REQUIRED_PACKAGE_DATA = ( + Path("data") / "benchmarks.csv", + Path("skills") / "report-writer" / "SKILL.md", + Path("skills") / "report-writer" / "assets" / "report_template.tex", +) + + +def _run(cmd: list[str], **kwargs) -> subprocess.CompletedProcess: + result = subprocess.run(cmd, capture_output=True, text=True, **kwargs) + if result.returncode != 0: + raise AssertionError( + f"command failed ({result.returncode}): {' '.join(cmd[:4])}…\n" + f"stdout:\n{result.stdout[-2000:]}\nstderr:\n{result.stderr[-2000:]}" + ) + return result + + +@pytest.fixture(scope="module") +def installed_wheel(tmp_path_factory: pytest.TempPathFactory) -> dict: + """Build the wheel and install it into a throwaway virtualenv. + + Module-scoped: building and installing once is the expensive part, and the + tests below only read from the result. + """ + root = tmp_path_factory.mktemp("wheel-acceptance") + dist = root / "dist" + env_dir = root / "venv" + + _run( + [sys.executable, "-m", "pip", "wheel", "--no-deps", "-w", str(dist), + str(_REPO_ROOT)], + cwd=str(root), # outside the repository + ) + wheels = sorted(dist.glob("agentic_cli-*.whl")) + assert len(wheels) == 1, f"expected exactly one wheel, got {wheels}" + + venv.EnvBuilder(with_pip=True, symlinks=True).create(env_dir) + python = env_dir / "bin" / "python" + console = env_dir / "bin" / "research-demo" + assert python.exists(), f"virtualenv has no interpreter at {python}" + + _run([str(python), "-m", "pip", "install", "--quiet", str(wheels[0])], + cwd=str(root)) + + return { + "python": python, + "console": console, + "wheel": wheels[0], + "root": root, + "env_dir": env_dir, + } + + +@pytest.fixture +def sandbox(tmp_path: Path) -> tuple[Path, Path]: + home = tmp_path / "home" + cwd = tmp_path / "cwd" + home.mkdir() + cwd.mkdir() + return home, cwd + + +class TestWheelIsSelfContained: + def test_child_resolves_from_the_isolated_install(self, installed_wheel, sandbox): + """A child launched as the smoke launches one must not see the checkout. + + Probed in a subprocess with the same interpreter, cwd, HOME and + stripped environment the console gets — the parent pytest process has + the editable install on ``sys.path``, so its own imports prove nothing + about the child. + """ + home, cwd = sandbox + + imports = probe_child_imports( + str(installed_wheel["python"]), cwd=cwd, home=home + ) + + env_dir = installed_wheel["env_dir"].resolve() + for label, path in (("demo", imports.demo), ("framework", imports.framework)): + assert path.is_relative_to(env_dir), ( + f"{label} resolved to {path}, outside the isolated venv" + ) + assert not path.is_relative_to(_REPO_ROOT), ( + f"{label} resolved into the repository checkout: {path}" + ) + assert imports.version == project_version() + + def test_package_data_is_present(self, installed_wheel, sandbox): + home, cwd = sandbox + imports = probe_child_imports( + str(installed_wheel["python"]), cwd=cwd, home=home + ) + pkg_dir = imports.demo.parent + + missing = [str(rel) for rel in REQUIRED_PACKAGE_DATA + if not (pkg_dir / rel).is_file()] + assert missing == [], ( + f"the installed wheel is missing package data: {missing}\n" + f"(looked under {pkg_dir})" + ) + + def test_console_script_is_installed_and_executable(self, installed_wheel): + console = installed_wheel["console"] + assert console.exists(), ( + "the `research-demo` console script was not installed by the wheel" + ) + assert os.access(console, os.X_OK), f"{console} is not executable" + + def test_console_script_runs_the_venv_interpreter(self, installed_wheel): + """Its shebang must point into the venv, not at the checkout's python. + + This is what makes running the script equivalent to running the venv's + interpreter — and therefore what lets the import probe above stand for + the console process. + + The shebang path is compared **unresolved**: ``venv/bin/python`` is a + symlink to the base interpreter (``EnvBuilder(symlinks=True)``), so + resolving it lands on the conda binary and says nothing. What makes an + interpreter a venv interpreter is its ``sys.prefix``, which is asserted + separately below. + """ + env_dir = installed_wheel["env_dir"] + shebang = installed_wheel["console"].read_text(errors="replace").splitlines()[0] + assert shebang.startswith("#!"), f"no shebang in the console script: {shebang!r}" + + interpreter = Path(shebang[2:].strip().split()[0]) + assert interpreter.is_relative_to(env_dir), ( + f"console script runs {interpreter}, outside the venv {env_dir}" + ) + + # The property that actually confers isolation: this interpreter's + # site-packages is the venv's, not the base environment's. + prefix = subprocess.run( + [str(interpreter), "-c", "import sys; print(sys.prefix)"], + capture_output=True, text=True, check=True, + ).stdout.strip() + assert Path(prefix) == env_dir, ( + f"the console script's interpreter has sys.prefix={prefix}, " + f"expected the venv at {env_dir}" + ) + + +class TestWheelConsoleSmoke: + def test_installed_console_script_startup_help_and_exit( + self, installed_wheel, sandbox + ): + """The user-facing path: run `research-demo`, not `python -m`.""" + home, cwd = sandbox + + result = run_console_smoke( + [str(installed_wheel["console"])], cwd=cwd, home=home + ) + + assert_clean_session(result) + assert result.argv == [str(installed_wheel["console"])], ( + "the smoke did not execute the installed console script" + ) + + def test_module_invocation_also_works(self, installed_wheel, sandbox): + """`python -m research_demo` from the installed wheel, for parity.""" + home, cwd = sandbox + + result = run_console_smoke( + [str(installed_wheel["python"]), "-m", "research_demo"], + cwd=cwd, + home=home, + ) + + assert_clean_session(result) + + +class TestArtifactCarriesTheDeclaredVersion: + """The built and installed artifact must match ``pyproject.toml``. + + All three surfaces are checked because they can disagree: the wheel + filename comes from the build, the distribution metadata from the install, + and ``__version__`` is a separate literal in the package that a bump can + forget. + """ + + def test_wheel_filename_carries_the_declared_version(self, installed_wheel): + expected = project_version() + assert installed_wheel["wheel"].name.startswith(f"agentic_cli-{expected}-"), ( + f"wheel {installed_wheel['wheel'].name} does not carry the declared " + f"version {expected}" + ) + + def test_installed_metadata_and_module_version_agree(self, installed_wheel): + """Distribution metadata and ``agentic_cli.__version__``, from the venv.""" + expected = project_version() + probe = ( + "import json, importlib.metadata as md, agentic_cli; " + "print(json.dumps({" + "'distribution': md.version('agentic-cli'), " + "'module': agentic_cli.__version__}))" + ) + result = _run( + [str(installed_wheel["python"]), "-c", probe], + cwd=str(installed_wheel["root"]), + ) + found = json.loads(result.stdout.strip().splitlines()[-1]) + + assert found["distribution"] == expected, ( + f"installed distribution metadata says {found['distribution']}, " + f"pyproject.toml declares {expected}" + ) + assert found["module"] == expected, ( + f"agentic_cli.__version__ is {found['module']}, " + f"pyproject.toml declares {expected}" + ) diff --git a/tests/integration/test_research_scenarios.py b/tests/integration/test_research_scenarios.py index 525e334..b913645 100644 --- a/tests/integration/test_research_scenarios.py +++ b/tests/integration/test_research_scenarios.py @@ -12,14 +12,17 @@ conda run -n agenticcli python -m pytest tests/integration/test_research_scenarios.py -v -m llm -Requires GOOGLE_API_KEY (Gemini/ADK, the demo default) or ANTHROPIC_API_KEY. -For Anthropic, also set AGENTIC_SCENARIO_MODEL=claude-... so the factory routes -to LangGraph. Set AGENTIC_RECORD_EVENTS= to dump each run's event stream -to JSON (replayable by the deterministic render tests). +Requires GOOGLE_API_KEY (Gemini, the demo default) or ANTHROPIC_API_KEY. These +scenarios are pinned to the **ADK** orchestrator, which is what the demo ships +with; ADK runs Claude natively via ``AnthropicLlm``, so setting +``AGENTIC_SCENARIO_MODEL=claude-...`` just changes the model, not the backend. +Set AGENTIC_RECORD_EVENTS= to dump each run's event stream to JSON +(replayable by the deterministic render tests). """ from __future__ import annotations +import contextlib import json import os import sys @@ -27,10 +30,18 @@ import pytest -from agentic_cli.config import BaseSettings, set_settings +from agentic_cli.workflow.adk.transfer_tool_description import TRANSFER_TOOL_NAME from agentic_cli.workflow.events import EventType from agentic_cli.workflow.factory import create_workflow_manager_from_settings - +from agentic_cli.workflow.settings import OrchestratorType + +from tests.demo_isolation import ( + assert_dotenv_isolated, + assert_no_global_settings_leak, + effective_env_file, + isolated_env_file, + make_isolated_settings, +) from tests.event_replay import events_to_dicts from tests.integration.helpers import ( find_events, @@ -43,6 +54,7 @@ if str(_REPO_ROOT) not in sys.path: sys.path.insert(0, str(_REPO_ROOT)) from examples.research_demo.agents import AGENT_CONFIGS # noqa: E402 +from examples.research_demo.settings import ResearchDemoSettings # noqa: E402 _has_any_key = bool( @@ -56,22 +68,73 @@ @pytest.fixture -def research_settings(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> BaseSettings: - """Settings for live runs: real keys, temp workspace, permission gate off. +def research_settings( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> ResearchDemoSettings: + """The demo's own settings, fully isolated: temp HOME, temp cwd, no gate. + + Uses ``ResearchDemoSettings`` rather than a bare ``BaseSettings`` so the + scenarios exercise what the demo actually runs with — its ``skills_dirs``, + its sandbox data mount and artifacts dir, and its ``research_demo`` + app_name (which is what ``./.{app_name}/`` and ``~/.{app_name}/`` are + derived from). + + Isolation is on both axes, because the demo reads from both: + + - ``HOME`` is redirected, so the user config and user KB resolve into + tmp_path and a developer's real config cannot change what these assert. + - ``cwd`` is redirected, so the project KB and permission workdir + (``./.research_demo/...``) are written under tmp_path, not into the repo. + - the dotenv is passed explicitly (``make_isolated_settings``), because + ``model_config["env_file"]`` was frozen to the real ``~/.research_demo/ + .env`` at import and ``HOME`` cannot move it. + + Provider credentials still arrive the way the live-test framework supplies + them (real environment / the integration conftest) — that is the point of a + live test. What must *not* happen is ``ResearchDemoSettings`` re-reading the + developer's dotenv or JSON config and changing the model, orchestrator or + workspace out from under the scenario. + + No ``set_settings()``: the manager scopes settings to itself for the whole + turn (``BaseWorkflowManager._workflow_context`` → ``set_context_settings``, + and ADK agent construction under ``SettingsContext``), so the global + singleton is unnecessary — and it has no teardown, so setting it would leak + this fixture's settings into every later test in the session. The permission gate is disabled so tool calls run headlessly without a prompt UI — these tests exercise agent behavior, not the permission UX. - - The project knowledge base and the permission workdir are resolved relative - to ``cwd`` (``./.{app_name}/...``), so we chdir into the temp workspace to - keep those writes out of the repo. """ + home = tmp_path / "home" workspace = tmp_path / "research_ws" - workspace.mkdir(parents=True) - monkeypatch.chdir(workspace) - settings = BaseSettings(workspace_dir=workspace, permissions_enabled=False) - set_settings(settings) - return settings + project = tmp_path / "project" + for d in (home, workspace, project): + d.mkdir(parents=True) + monkeypatch.setenv("HOME", str(home)) + monkeypatch.chdir(project) + + settings = make_isolated_settings( + home=home, + workspace=workspace, + permissions_enabled=False, + # The demo ships ADK; pin it so a stray orchestrator setting in the + # environment cannot silently move these scenarios to another backend. + orchestrator=OrchestratorType.ADK, + ) + + # Prove the isolation rather than assume it: a live run that silently read + # the real dotenv could pick up a different model or workspace. + assert_dotenv_isolated( + effective_env_file( + workspace_dir=workspace, + _env_file=str(isolated_env_file(home)), + ), + home, + ) + assert settings.orchestrator is OrchestratorType.ADK + + yield settings + + assert_no_global_settings_leak() def _maybe_record(events: list, name: str) -> None: @@ -85,14 +148,26 @@ def _maybe_record(events: list, name: str) -> None: ) -async def run_agent( - settings: BaseSettings, - message: str, +@contextlib.asynccontextmanager +async def conversation( + settings: ResearchDemoSettings, *, - session_id: str | None = None, - record_name: str | None = None, -) -> list: - """Run one turn through the real demo agents and return the event stream.""" + user_id: str = "tester", + session_id: str = "scenario-session", +): + """One manager and one session, held open across several turns. + + The coordinator's policy is size-based: an explicit, bounded request ("search + arXiv for two papers", "ingest this note") is executed or delegated + immediately, while an open-ended goal — or an explicit request for a plan — + is planned, shown and held for confirmation. The scenarios therefore ask for + one or the other deliberately, and each asserts only its own contract. + + Multiple turns matter where state carries across them: the KB scenario + ingests in one turn and reads back in the next, which only works because the + manager is initialized and cleaned up exactly once and every turn reuses the + same ``user_id`` and ``session_id``. + """ manager = create_workflow_manager_from_settings( agent_configs=AGENT_CONFIGS, settings=settings, @@ -104,23 +179,59 @@ async def _auto_input(request) -> str: # noqa: ANN001 return "yes, proceed" manager.set_input_callback(_auto_input) - try: + turns: list[list] = [] + + async def say(message: str) -> list: + """Send one user message; return that turn's events. + + A workflow exception propagates: these are behavioural contracts, and a + turn that died is a failure, not something to inspect around. Only + ``Exception`` is involved anywhere here — cancellation, Ctrl+C and + SystemExit must keep unwinding an interrupted live run. + """ events: list = [] async for event in manager.process( - message=message, user_id="tester", session_id=session_id + message=message, user_id=user_id, session_id=session_id ): events.append(event) + turns.append(events) + return events + + say.turns = turns # type: ignore[attr-defined] + say.all_events = lambda: [e for t in turns for e in t] # type: ignore[attr-defined] + + try: + yield say finally: manager.clear_input_callback() await manager.cleanup() + +async def run_agent( + settings: ResearchDemoSettings, + message: str, + *, + session_id: str | None = None, + record_name: str | None = None, +) -> list: + """Run one turn through the real demo agents and return the event stream.""" + async with conversation( + settings, session_id=session_id or "scenario-session" + ) as say: + events = await say(message) + if record_name: _maybe_record(events, record_name) return events class TestPlanningScenario: - """Planning: the coordinator should save and show a plan.""" + """Planning, on its own: an explicit plan request produces a saved plan. + + Deliberately does **not** also test delegation. Planning, delegation and KB + behaviour are three independent properties, and bundling them meant one + stochastic policy choice failed all three at once. + """ async def test_creates_and_saves_a_plan(self, research_settings): events = await run_agent( @@ -139,23 +250,87 @@ async def test_creates_and_saves_a_plan(self, research_settings): assert find_events(events, EventType.TEXT), "no text response produced" +def _tool_names(events: list) -> list[str]: + return [c.metadata.get("tool_name") for c in find_tool_calls(events)] + + +#: A bounded, fully-specified arXiv request. Per the coordinator prompt this is +#: an "explicit, bounded operation": delegate and run it, no plan, no +#: confirmation. Shared so the delegation diagnostic and the behavioural +#: scenario exercise the same path. +BOUNDED_ARXIV_REQUEST = ( + "Search arXiv for 2 recent papers on 'speculative decoding' and list their " + "titles. This is a small, explicit request — do it now via the " + "arxiv_specialist; no plan and no confirmation needed." +) + + +#: The class name ADK's own tool description used to recommend. Asserted +#: against here (never put in the agent's prompt, where it would prime the +#: model toward the very mistake being guarded). +_TRANSFER_CLASS_NAME = "TransferToAgentTool" + + +def _assert_no_class_name_transfer(events: list) -> None: + """Delegation must go through the function, never the class name.""" + called = _tool_names(events) + assert _TRANSFER_CLASS_NAME not in called, ( + f"the model called the transfer tool by its class name: {called}" + ) + offending = [ + str(e.content) + for e in find_events(events, EventType.ERROR) + if _TRANSFER_CLASS_NAME in str(e.content) + ] + assert not offending, f"transfer-by-class-name error surfaced: {offending}" + + +def _assert_no_unknown_tool_errors(events: list) -> None: + """Every tool the model called must be one that actually exists.""" + unknown = [ + str(e.content) + for e in find_events(events, EventType.ERROR) + if "not found" in str(e.content) + ] + assert not unknown, ( + f"the model called a tool that does not exist: {unknown}; " + f"tools actually called: {_tool_names(events)}" + ) + + class TestArxivScenario: - """arXiv: the specialist should search arXiv and get results back.""" + """Delegation: a bounded request reaches the specialist and runs. - async def test_searches_arxiv(self, research_settings): - events = await run_agent( - research_settings, - "Search arXiv for 2 recent papers on 'speculative decoding' using " - "search_arxiv and list their titles. Do not ask for confirmation.", - record_name="arxiv_search", - ) + One turn, because the coordinator's prompt says an explicit bounded + operation should just be done. No plan is required or asserted here — that + is :class:`TestPlanningScenario`'s job. + + This is also the delegation regression: the transfer defect is asserted + inside the behavioural run rather than as a separate paid diagnostic, so + the class-name check can never be satisfied vacuously by a run in which no + delegation was attempted — the same test requires the correct + ``transfer_to_agent`` call. + """ - calls = find_tool_calls(events, "search_arxiv") - assert calls, "agent did not call search_arxiv" + async def test_bounded_request_delegates_and_searches(self, research_settings): + async with conversation(research_settings) as say: + events = await say(BOUNDED_ARXIV_REQUEST) + _maybe_record(events, "arxiv_search") + # Delegation happened, by the correct function name. + assert find_tool_calls(events, TRANSFER_TOOL_NAME), ( + f"the coordinator never delegated; tools called: {_tool_names(events)}" + ) + _assert_no_class_name_transfer(events) + _assert_no_unknown_tool_errors(events) + + # And the specialist actually did the work. + assert find_tool_calls(events, "search_arxiv"), ( + f"the specialist never searched arXiv; tools called: {_tool_names(events)}" + ) results = find_tool_results(events, "search_arxiv") assert any(r.metadata.get("success", True) for r in results), ( - "search_arxiv never returned a successful result: " + f"search_arxiv never returned a successful result: " f"{[r.content for r in results]}" ) @@ -172,35 +347,57 @@ def _require_embeddings(self): pytest.importorskip("faiss") pytest.importorskip("sentence_transformers") - async def test_ingest_text_then_search(self, research_settings): + async def test_bounded_ingest_then_bounded_readback(self, research_settings): + """Two bounded operations in one persistent conversation. + + Each turn is explicit and small, so per the coordinator's prompt both + should just run — no plan-policy assertions here. The two turns share a + manager and a session so the read-back sees what the ingest wrote. + + The ingest turn asks for the *outcome* and points at the specialist, + rather than naming a writer tool: the coordinator holds only the KB + readers (``kb_search``/``kb_read``/``kb_list``/``kb_search_concepts``), + and ``kb_ingest_*`` belongs to ``arxiv_specialist``. Telling the + coordinator to call a tool it does not have makes a correct refusal + look like a failure. + """ note = ( "Speculative decoding uses a small draft model to propose tokens that a " "larger target model verifies in parallel, cutting latency without " "changing the output distribution." ) - events = await run_agent( - research_settings, - "Ingest the following note into the knowledge base (use the " - "arxiv_specialist, which has KB write access), then search the knowledge " - "base for 'speculative decoding' to confirm it is retrievable. Do not ask " - f"for confirmation.\n\nNote: {note}", - record_name="kb_ingest_search", - ) + async with conversation(research_settings) as say: + ingest_turn = await say( + "Store this exact note in the knowledge base now. The " + "arxiv_specialist holds the knowledge-base write tools, so hand " + "it over. This is a small explicit request — no plan and no " + f"confirmation needed.\n\nNote: {note}" + ) + readback_turn = await say( + "Now search the knowledge base for 'speculative decoding' and " + "show me what you find. Again, just do it." + ) + _maybe_record(say.all_events(), "kb_ingest_search") ingest_results = [ r - for r in find_events(events, EventType.TOOL_RESULT) + for r in find_events(ingest_turn, EventType.TOOL_RESULT) if str(r.metadata.get("tool_name", "")).startswith("kb_ingest") ] assert ingest_results, ( - "no kb_ingest_* tool result observed; tool calls were: " - f"{[c.metadata.get('tool_name') for c in find_tool_calls(events)]}" + f"no kb_ingest_* tool result; turn-1 tools: {_tool_names(ingest_turn)}" ) assert any(r.metadata.get("success", True) for r in ingest_results), ( - "kb_ingest never succeeded: " - f"{[r.content for r in ingest_results]}" + f"kb_ingest never succeeded: {[r.content for r in ingest_results]}" ) - # And the KB was queried afterward. - assert find_tool_calls(events, "kb_search") or find_tool_calls( - events, "kb_list" - ), "agent did not query the KB after ingesting" + + assert find_tool_calls(readback_turn, "kb_search") or find_tool_calls( + readback_turn, "kb_list" + ), f"the KB was never queried back; turn-2 tools: {_tool_names(readback_turn)}" + + # Both turns must have used real tool names. Asserted here rather than + # as a separate paid run: a spliced name (two real tools joined into + # one) shows up as an unknown-tool error on exactly this path. + both_turns = ingest_turn + readback_turn + _assert_no_unknown_tool_errors(both_turns) + _assert_no_class_name_transfer(both_turns) diff --git a/tests/workflow/test_adk_transfer_tool.py b/tests/workflow/test_adk_transfer_tool.py new file mode 100644 index 0000000..6b76754 --- /dev/null +++ b/tests/workflow/test_adk_transfer_tool.py @@ -0,0 +1,403 @@ +"""ADK's built-in agent-transfer tool: permission identity and description. + +Two verified defects blocked multi-agent delegation, and both are covered here. + +1. **Permission identity.** ADK auto-injects an exact + ``TransferToAgentTool`` into any agent with ``sub_agents``. It is a + ``FunctionTool`` *subclass*, and the plugin only unwrapped exact + ``FunctionTool``/``LongRunningFunctionTool``, so the routing primitive + resolved to ``_UNVERIFIED`` and was denied as unregistered — even with + permissions disabled. Delegation could not work at all. + +2. **Model-visible description.** ADK builds the declaration from the + ``transfer_to_agent`` docstring, which advises callers to "use + TransferToAgentTool instead of this function directly". The model followed + that advice and emitted ``TransferToAgentTool``, which ADK rejects. + +The identity fix must not become a hole: adding a type to the trusted list is +exactly the kind of change that can quietly re-admit forged tools, so the +negative cases are asserted alongside the positive one. +""" + +from __future__ import annotations + +from types import SimpleNamespace +from typing import Any + +import pytest + +pytest.importorskip("google.adk") + +from google.adk.tools import FunctionTool, LongRunningFunctionTool # noqa: E402 +from google.adk.tools.transfer_to_agent_tool import ( # noqa: E402 + TransferToAgentTool, + transfer_to_agent, +) +from google.genai import types # noqa: E402 + +from agentic_cli.workflow.adk.permission_plugin import ( # noqa: E402 + _UNVERIFIED, + PermissionPlugin, +) +from agentic_cli.workflow.permissions import EXEMPT # noqa: E402 +from agentic_cli.workflow.adk.transfer_tool_description import ( # noqa: E402 + ROUTING_DESCRIPTION, + TRANSFER_TOOL_NAME, + TransferToolDescriptionPlugin, + correct_transfer_declaration, +) + +AGENT_NAMES = ["arxiv_specialist", "data_analyst", "report_writer"] + + +def _tool() -> TransferToAgentTool: + return TransferToAgentTool(agent_names=list(AGENT_NAMES)) + + +def _request(tool: Any, declaration: types.FunctionDeclaration | None = None): + """A minimal LlmRequest carrying one tool and its declaration.""" + from google.adk.models.llm_request import LlmRequest + + if declaration is None: + declaration = tool._get_declaration() + return LlmRequest( + config=types.GenerateContentConfig( + tools=[types.Tool(function_declarations=[declaration])] + ), + tools_dict={declaration.name: tool}, + ) + + +#: A description in the shape ADK 1.x generates. The correction-path tests use +#: *this*, not whatever the installed ADK happens to produce: the production +#: workaround is deliberately a no-op once upstream fixes its docstring (2.x +#: already has), so tests that required the installed declaration to be broken +#: would start failing the day the pinned range picks up the fix. What must keep +#: working is the correction itself, given input that needs correcting. +MISLEADING_DESCRIPTION = ( + "Transfer the question to another agent.\n\n" + "This tool hands off control to another agent when it's more suitable to\n" + "answer the user's question according to the agent's description.\n\n" + "Note:\n" + " For most use cases, you should use TransferToAgentTool instead of this\n" + " function directly. TransferToAgentTool provides additional enum " + "constraints\n" + " that prevent LLMs from hallucinating invalid agent names.\n\n" + "Args:\n" + " agent_name: the agent name to transfer to." +) + +SAFE_DESCRIPTION = "Transfer the query to another agent." + + +def _declaration_with(description: str) -> types.FunctionDeclaration: + """The native tool's real declaration, with the description swapped. + + Name, parameters, required fields and ADK's agent-name enum all come from + the genuine tool, so schema assertions stay meaningful. + """ + declaration = _tool()._get_declaration() + declaration.description = description + return declaration + + +def _misleading_request(): + """An exact native tool whose declaration needs correcting.""" + return _request(_tool(), _declaration_with(MISLEADING_DESCRIPTION)) + + +def _description_is_safe(description: str) -> bool: + """No advice to call the class, under either ADK's wording or ours.""" + return ( + "TransferToAgentTool" not in description + and "instead of this function" not in description + ) + + +# --------------------------------------------------------------------------- +# 1. Permission identity +# --------------------------------------------------------------------------- + + +class TestTransferToolResolvesToItsRegisteredDefinition: + def test_exact_transfer_tool_resolves_to_the_exempt_definition(self): + from agentic_cli.tools.registry import identify_tool + + defn = PermissionPlugin._resolve_definition(_tool()) + + assert defn is not _UNVERIFIED, ( + "ADK's built-in transfer tool was denied as unregistered" + ) + assert defn is identify_tool(transfer_to_agent), ( + "resolved to something other than the registered transfer_to_agent" + ) + assert defn.name == TRANSFER_TOOL_NAME + assert defn.capabilities is EXEMPT, ( + "the routing primitive must stay EXEMPT, not acquire capabilities" + ) + + async def test_before_tool_callback_allows_it(self): + result = await PermissionPlugin().before_tool_callback( + tool=_tool(), + tool_args={"agent_name": "arxiv_specialist"}, + tool_context=None, + ) + assert result is None, f"the transfer tool was gated: {result}" + + +class TestIdentityGuaranteesSurvive: + """Widening the trusted-type list must not admit anything else.""" + + async def _denied(self, tool: Any) -> None: + assert PermissionPlugin._resolve_definition(tool) is _UNVERIFIED + result = await PermissionPlugin().before_tool_callback( + tool=tool, tool_args={}, tool_context=None + ) + assert result is not None and result.get("success") is False + assert "not registered" in result.get("error", "") + + async def test_arbitrary_function_tool_subclass_is_denied(self): + class SneakyTool(FunctionTool): + """A subclass may override run_async and do anything.""" + + await self._denied(SneakyTool(func=transfer_to_agent)) + + async def test_transfer_tool_subclass_is_denied(self): + class SubclassedTransfer(TransferToAgentTool): + pass + + await self._denied(SubclassedTransfer(agent_names=list(AGENT_NAMES))) + + async def test_forged_object_named_transfer_to_agent_is_denied(self): + forged = SimpleNamespace(name=TRANSFER_TOOL_NAME) + await self._denied(forged) + + async def test_forged_object_named_after_the_class_is_denied(self): + class TransferToAgentTool: # shadows the real name deliberately + name = TRANSFER_TOOL_NAME + + await self._denied(TransferToAgentTool()) + + async def test_forged_object_carrying_the_real_func_is_denied(self): + """A copied ``.func`` is an attribute, not evidence of behaviour.""" + forged = SimpleNamespace(name=TRANSFER_TOOL_NAME, func=transfer_to_agent) + await self._denied(forged) + + async def test_long_running_function_tool_still_resolves(self): + """The pre-existing trusted types keep working.""" + from agentic_cli.tools.registry import identify_tool + + defn = PermissionPlugin._resolve_definition( + LongRunningFunctionTool(func=transfer_to_agent) + ) + assert defn is identify_tool(transfer_to_agent) + + +# --------------------------------------------------------------------------- +# 2. Model-visible description +# --------------------------------------------------------------------------- + + +class TestTransferDeclarationIsCorrected: + """The correction path, driven by a synthetic misleading declaration. + + These never depend on the installed ADK still being broken — only on the + correction doing the right thing when handed input that needs it. + """ + + def test_exactly_one_transfer_declaration_with_the_right_name(self): + request = _misleading_request() + correct_transfer_declaration(request) + + decls = [ + d + for tool in request.config.tools + for d in tool.function_declarations + if d.name == TRANSFER_TOOL_NAME + ] + assert len(decls) == 1 + assert decls[0].name == TRANSFER_TOOL_NAME + + def test_description_no_longer_advises_calling_the_class(self): + request = _misleading_request() + assert correct_transfer_declaration(request) is True + + description = request.config.tools[0].function_declarations[0].description + assert _description_is_safe(description) + + def test_description_states_the_callable_name(self): + request = _misleading_request() + correct_transfer_declaration(request) + + description = request.config.tools[0].function_declarations[0].description + assert f'{TRANSFER_TOOL_NAME}(agent_name="' in description + + def test_agent_name_enum_and_schema_are_preserved(self): + before = _tool()._get_declaration().parameters.model_dump(exclude_none=True) + + request = _misleading_request() + correct_transfer_declaration(request) + after = request.config.tools[0].function_declarations[0].parameters + + assert after.properties["agent_name"].enum == AGENT_NAMES + assert after.required == ["agent_name"] + assert after.model_dump(exclude_none=True) == before + + def test_correction_is_idempotent(self): + request = _misleading_request() + assert correct_transfer_declaration(request) is True + first = request.config.tools[0].function_declarations[0].description + + # A second pass has nothing left to do. + assert correct_transfer_declaration(request) is False + assert request.config.tools[0].function_declarations[0].description == first + + def test_already_correct_description_is_left_alone(self): + """An ADK release that ships a sane docstring needs no correction.""" + request = _request(_tool(), _declaration_with(SAFE_DESCRIPTION)) + + assert correct_transfer_declaration(request) is False + assert ( + request.config.tools[0].function_declarations[0].description + == SAFE_DESCRIPTION + ) + + def test_the_upstream_function_doc_is_never_mutated(self): + before = transfer_to_agent.__doc__ + correct_transfer_declaration(_misleading_request()) + assert transfer_to_agent.__doc__ == before + + +class TestAgainstTheInstalledAdk: + """One integration check that works whichever docstring ADK ships. + + Either the installed declaration is misleading and the correction fixes it, + or it is already safe and no correction is needed. Both are acceptable; a + declaration that is still misleading *after* the plugin has run is not. + """ + + def test_installed_declaration_ends_up_safe(self): + request = _request(_tool()) + initial = request.config.tools[0].function_declarations[0].description or "" + + corrected = correct_transfer_declaration(request) + final = request.config.tools[0].function_declarations[0].description or "" + + if _description_is_safe(initial): + assert corrected is False, ( + "an already-safe ADK description was rewritten anyway" + ) + else: + assert corrected is True, ( + "the installed ADK description advises calling the class and " + "was not corrected" + ) + + assert _description_is_safe(final), ( + f"the model would still be told to call the class: {final!r}" + ) + # Whichever branch ran, the contract the model needs is intact. + declaration = request.config.tools[0].function_declarations[0] + assert declaration.name == TRANSFER_TOOL_NAME + assert declaration.parameters.properties["agent_name"].enum == AGENT_NAMES + assert declaration.parameters.required == ["agent_name"] + + +class TestOnlyTheNativeToolIsRewritten: + def test_application_tool_with_the_same_name_is_untouched(self): + """A same-named application tool is not ADK's routing primitive.""" + + def transfer_to_agent(agent_name: str) -> dict: # noqa: A001 - deliberate + """You should use TransferToAgentTool instead of this function.""" + return {"success": True} + + app_tool = FunctionTool(func=transfer_to_agent) + declaration = app_tool._get_declaration() + original = declaration.description + + request = _request(app_tool, declaration) + assert correct_transfer_declaration(request) is False + assert request.config.tools[0].function_declarations[0].description == original + + def test_impostor_base_tool_named_transfer_to_agent_is_untouched(self): + """A real BaseTool that merely takes the name is not ADK's tool. + + ``LlmRequest.tools_dict`` is typed to ``BaseTool``, so the realistic + impostor is a genuine tool object with the right name — which is the + case the exact-type check has to reject. + """ + from google.adk.tools import BaseTool + + class ImpostorTool(BaseTool): + def __init__(self) -> None: + super().__init__(name=TRANSFER_TOOL_NAME, description="impostor") + + declaration = types.FunctionDeclaration( + name=TRANSFER_TOOL_NAME, + description="you should use TransferToAgentTool instead", + ) + + request = _request(ImpostorTool(), declaration) + assert correct_transfer_declaration(request) is False + assert ( + request.config.tools[0].function_declarations[0].description + == "you should use TransferToAgentTool instead" + ) + + def test_subclass_of_the_native_tool_is_untouched(self): + class SubclassedTransfer(TransferToAgentTool): + pass + + tool = SubclassedTransfer(agent_names=list(AGENT_NAMES)) + request = _request(tool) + assert correct_transfer_declaration(request) is False + + +class TestPluginWiring: + async def test_plugin_corrects_the_request_and_counts_it(self): + """Deterministic: driven by a synthetic misleading declaration.""" + plugin = TransferToolDescriptionPlugin() + request = _misleading_request() + + await plugin.before_model_callback( + callback_context=SimpleNamespace(invocation_id="i1"), + llm_request=request, + ) + + assert plugin.corrections == 1 + description = request.config.tools[0].function_declarations[0].description + assert description == ROUTING_DESCRIPTION + + async def test_plugin_does_not_count_an_already_safe_request(self): + plugin = TransferToolDescriptionPlugin() + request = _request(_tool(), _declaration_with(SAFE_DESCRIPTION)) + + await plugin.before_model_callback( + callback_context=SimpleNamespace(invocation_id="i1"), + llm_request=request, + ) + + assert plugin.corrections == 0 + + async def test_plugin_never_blocks_a_request(self): + """A malformed request must not fail the turn.""" + plugin = TransferToolDescriptionPlugin() + broken = SimpleNamespace(tools_dict={TRANSFER_TOOL_NAME: _tool()}, config=None) + + result = await plugin.before_model_callback( + callback_context=SimpleNamespace(invocation_id="i1"), llm_request=broken + ) + assert result is None + + def test_manager_registers_the_plugin(self): + """The correction must actually be wired into the runner.""" + from agentic_cli.workflow.adk.manager import GoogleADKWorkflowManager + + manager = GoogleADKWorkflowManager.__new__(GoogleADKWorkflowManager) + manager._settings = SimpleNamespace( + raw_llm_logging=False, app_name="t", verbose_thinking=False + ) + + plugins = manager._init_plugins() + + assert any(isinstance(p, TransferToolDescriptionPlugin) for p in plugins)