From 0e4662afd8d0a619f24b7ee25f3c0e24a850f775 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:21:51 -0400 Subject: [PATCH 1/2] fix(demo): harden startup and add release acceptance coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two demo commands broke while the workflow was still coming up. ``app.workflow`` raises until the controller reports READY, so ``/memory`` and ``/kb-backfill`` surfaced the generic "Error executing command: Workflow not initialized yet" during background init — which reads like a bug rather than "not yet". Both now warn and return, using the same readiness shape the built-in commands use. ``ResearchDemoSettings``' documented precedence was also wrong: it omitted constructor arguments, named the wrong project path, and did not say that the project file is untrusted and allowlist-filtered. The gap this closes on the test side is application *composition*. The workflow, the renderer and the individual commands all had component tests; nothing exercised a real ``ResearchDemoApp`` reacting to real input, and nothing exercised the thing a user actually runs. - Headless composition tests drive the real app through ``BaseCLIApp.process_input`` and the real ``MessageProcessor``, substituting only the nondeterministic workflow (scripted ``WorkflowEvent`` streams) and the UI (``RecordingSession``). They cover readiness warnings, message routing and session-id propagation, unknown commands, and recovery after a failed turn. Reverting the command fix turns two of them red with the exact reported error. - A pty smoke drives the real console process: startup → prompt → /help → /exit, asserting no traceback and exit 0, with no API key, network, Docker or LLM. It also pins that the run is side-effect free and that the child resolves its modules from the checkout. - A built-wheel acceptance builds the wheel, installs it into a throwaway virtualenv and runs the same session through the installed ``research-demo`` console script, asserting the child resolves inside the venv and that the package data (benchmarks.csv, the report-writer SKILL.md, report_template.tex) shipped. It is opt-in (``wheel`` marker + ``AGENTIC_WHEEL_ACCEPTANCE=1``) and runs as its own CI job, so the offline suite stays fast and network-free. ``tests/demo_isolation.py`` exists because ``ResearchDemoSettings`` reads three developer-owned sources that are *not* isolated alike: the two JSON files follow ``cwd``/``HOME`` at call time, but ``model_config["env_file"]`` is frozen at class definition — i.e. at collection, before any fixture runs. Redirecting HOME never moved it, so headless tests were reading the developer's real ``~/.research_demo/.env``. An explicit ``_env_file`` is the only thing that moves it, and the effective path is asserted rather than assumed. pexpect is declared in the dev extra rather than relied on transitively. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh --- .github/workflows/ci.yml | 36 +++ examples/research_demo/commands.py | 34 ++- examples/research_demo/settings.py | 20 +- pyproject.toml | 5 + tests/demo_isolation.py | 158 +++++++++++ tests/examples/console_smoke.py | 274 ++++++++++++++++++ tests/examples/test_research_demo_app.py | 313 +++++++++++++++++++++ tests/examples/test_research_demo_pty.py | 219 ++++++++++++++ tests/examples/test_research_demo_wheel.py | 270 ++++++++++++++++++ 9 files changed, 1320 insertions(+), 9 deletions(-) create mode 100644 tests/demo_isolation.py create mode 100644 tests/examples/console_smoke.py create mode 100644 tests/examples/test_research_demo_app.py create mode 100644 tests/examples/test_research_demo_pty.py create mode 100644 tests/examples/test_research_demo_wheel.py 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/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/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}" + ) From d3d55009697be8a45e4601e990c7725a117bb4f1 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:25:17 -0400 Subject: [PATCH 2/2] fix(adk): restore safe multi-agent delegation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects made ADK's built-in agent hand-off unusable, so any demo or app with ``sub_agents`` could not delegate at all. **The routing tool was denied as unregistered.** ``PermissionPlugin`` unwraps ``.func`` only for exact ADK function-tool types, because a subclass may override ``run_async`` and run something other than the callable it advertises. ``TransferToAgentTool`` is a ``FunctionTool`` *subclass* that ADK auto-injects, so it resolved to no registry identity and was refused — even with permissions disabled, since the refusal happens before the engine is consulted. Its exact class now joins the trusted types, on the same terms and for the same reason they are trusted: ADK constructs it as ``super().__init__(func=transfer_to_agent)`` and overrides only ``_get_declaration()`` (to add the agent-name enum), never ``run_async``, so what it invokes is still exactly ``self.func``. Listing the exact class keeps every other subclass out; no name-based authority was reintroduced and no alias was added. **The tool told the model to call something that does not exist.** ADK builds the declaration from ``transfer_to_agent``'s docstring, which through 1.37.0 — the newest release inside our ``<2`` pin, checked against the published wheel — advises callers to "use TransferToAgentTool instead of this function directly". That paragraph is written for Python callers but ships to the model as the tool's description, and Gemini 3.1 followed it, emitting ``TransferToAgentTool`` for ADK to reject with ``Tool 'TransferToAgentTool' not found``. A narrowly scoped before-model plugin rewrites that description on the prepared request. It acts only when the tool object is exactly ``TransferToAgentTool`` (an application tool sharing the name is untouched) and only while the misleading sentence is present, so it is idempotent and becomes a no-op the day the installed ADK ships a corrected docstring — upstream fixed it in 2.x. Only ``description`` is written: the declaration name, parameter schema, required fields and the agent-name enum are preserved, and the upstream function's ``__doc__`` is never mutated. Regression coverage asserts both halves, including that widening the trusted type list opened no hole: an arbitrary ``FunctionTool`` subclass, a ``TransferToAgentTool`` subclass, a forged object named ``transfer_to_agent``, one named after the class, and one carrying a copied ``.func`` all stay denied. The declaration tests drive a synthetic misleading declaration rather than the installed one, so they do not require upstream to remain broken; a single integration test accepts either state and asserts the outcome is safe. The demo's coordinator prompt gains a matching policy: an explicit bounded request is executed or delegated immediately, an open-ended goal (or an explicit request for a plan) is planned and held for confirmation, and planning is stated to be a workflow courtesy rather than the authorization boundary — tool permissions remain responsible for that. The live scenarios are split into three independent contracts (planning, bounded delegation, KB ingest/readback) over a persistent conversation, so one stochastic policy choice can no longer fail all three. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh --- examples/research_demo/agents.py | 37 +- src/agentic_cli/workflow/adk/manager.py | 10 + .../workflow/adk/permission_plugin.py | 25 +- .../workflow/adk/transfer_tool_description.py | 136 ++++++ tests/integration/test_research_scenarios.py | 309 +++++++++++--- tests/workflow/test_adk_transfer_tool.py | 403 ++++++++++++++++++ 6 files changed, 858 insertions(+), 62 deletions(-) create mode 100644 src/agentic_cli/workflow/adk/transfer_tool_description.py create mode 100644 tests/workflow/test_adk_transfer_tool.py 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/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/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)