diff --git a/CHANGELOG.md b/CHANGELOG.md index 60251d589..28b437b53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,12 @@ configurable singletons in the DI container with provider selection via `override_providers()`. Includes Behave BDD tests (35 scenarios), Robot Framework smoke tests, ASV benchmarks, and reference documentation. (#498) +- Added TDD bug-capture tests for #1024 — SQLite DB URL resolves to CWD + instead of CLEVERAGENTS_HOME. Behave BDD scenarios + (`@tdd_bug @tdd_bug_1024 @tdd_expected_fail`) verify that the default + `database_url` resolves inside `CLEVERAGENTS_HOME`, not the current + working directory. Includes Robot Framework integration tests with a + helper script exercising the same resolution path via subprocess. (#1034) - Added BuiltinAdapter class and MCP automatic resource slot creation. BuiltinAdapter wraps register_file_tools/register_git_tools/register_subplan_tool into a unified adapter interface. McpAdapter.infer_resource_slots() analyzes diff --git a/features/steps/tdd_sqlite_url_cwd_steps.py b/features/steps/tdd_sqlite_url_cwd_steps.py new file mode 100644 index 000000000..0de2692ff --- /dev/null +++ b/features/steps/tdd_sqlite_url_cwd_steps.py @@ -0,0 +1,185 @@ +"""Step definitions for TDD Bug #1024 — SQLite DB URL CWD resolution. + +These steps verify that when ``CLEVERAGENTS_HOME`` is set to a temporary +directory, the SQLite database file is created **inside** that directory +rather than relative to the current working directory. + +The default ``database_url`` in ``Settings`` is ``sqlite:///cleveragents.db`` +— a relative path that resolves against CWD. This breaks test isolation +when ``CLEVERAGENTS_HOME`` points to a different directory. + +Bug: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/1024 +TDD: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/1034 +""" + +from __future__ import annotations + +import logging +import os +import shutil +import tempfile +from pathlib import Path + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.application.container import get_database_url, reset_container +from cleveragents.config.settings import Settings + + +def _suppress_structlog_stdout() -> tuple[int, bool]: + """Prevent structlog debug lines from contaminating CLI stdout.""" + import structlog + + root = logging.getLogger() + prev_level = root.level + root.setLevel(logging.WARNING) + + prev_config = structlog.get_config() + prev_cache: bool = prev_config.get("cache_logger_on_first_use", True) # type: ignore[assignment] + structlog.configure( + logger_factory=structlog.stdlib.LoggerFactory(), + wrapper_class=structlog.stdlib.BoundLogger, + cache_logger_on_first_use=False, + ) + + return prev_level, prev_cache + + +def _restore_structlog(prev_level: int, prev_cache: bool) -> None: + """Undo the suppression applied by :func:`_suppress_structlog_stdout`.""" + import structlog + + logging.getLogger().setLevel(prev_level) + structlog.configure(cache_logger_on_first_use=prev_cache) + + +def _extract_sqlite_path(url: str) -> Path | None: + """Extract the file path from a SQLite URL. + + Handles both ``sqlite:///path`` (relative) and + ``sqlite:////abs/path`` (absolute) forms. + """ + prefix = "sqlite:///" + if not url.startswith(prefix): + return None + raw_path = url[len(prefix) :] + if not raw_path: + return None + return Path(raw_path).resolve() + + +@given("a fresh CLEVERAGENTS_HOME temp directory for DB URL testing") +def step_fresh_home(context: Context) -> None: + """Create an isolated CLEVERAGENTS_HOME and record the original CWD.""" + # Reset singletons to avoid stale state + reset_container() + Settings._instance = None # type: ignore[attr-defined] + + prev_level, prev_cache = _suppress_structlog_stdout() + + context.original_cwd = Path.cwd().resolve() + context.tdd_home = tempfile.mkdtemp(prefix="tdd_sqlite_url_1024_") + + # Remove any env vars that could override database_url + context.saved_db_url = os.environ.pop("CLEVERAGENTS_DATABASE_URL", None) + context.saved_test_db_url = os.environ.pop("CLEVERAGENTS_TEST_DATABASE_URL", None) + + os.environ["CLEVERAGENTS_HOME"] = context.tdd_home + + # Reset singletons so new Settings picks up the environment + reset_container() + Settings._instance = None # type: ignore[attr-defined] + + def _cleanup() -> None: + # Restore env vars + if context.saved_db_url is not None: + os.environ["CLEVERAGENTS_DATABASE_URL"] = context.saved_db_url + else: + os.environ.pop("CLEVERAGENTS_DATABASE_URL", None) + if context.saved_test_db_url is not None: + os.environ["CLEVERAGENTS_TEST_DATABASE_URL"] = context.saved_test_db_url + else: + os.environ.pop("CLEVERAGENTS_TEST_DATABASE_URL", None) + + os.environ.pop("CLEVERAGENTS_HOME", None) + reset_container() + Settings._instance = None # type: ignore[attr-defined] + + shutil.rmtree(context.tdd_home, ignore_errors=True) + _restore_structlog(prev_level, prev_cache) + + context.add_cleanup(_cleanup) + + +@when("I resolve the effective database URL from Settings") +def step_resolve_db_url(context: Context) -> None: + """Resolve the effective database URL via the container helper.""" + # Use the container's get_database_url which is what the app uses + context.resolved_url = get_database_url() + context.resolved_path = _extract_sqlite_path(context.resolved_url) + + +@when("I resolve the Settings database_url default") +def step_resolve_settings_default(context: Context) -> None: + """Resolve the database_url via the Settings model default. + + Constructs a fresh Settings instance (with CLEVERAGENTS_DATABASE_URL + removed) to exercise the default ``database_url`` field, then resolves + the path. This tests the Settings-level default independently of the + container's ``get_database_url()`` helper. + """ + settings = Settings() + context.settings_db_url = settings.get_database_url() + context.settings_db_path = _extract_sqlite_path(context.settings_db_url) + + +@then("the resolved database path should be inside CLEVERAGENTS_HOME") +def step_path_inside_home(context: Context) -> None: + """Assert the database path is under CLEVERAGENTS_HOME.""" + home = Path(context.tdd_home).resolve() + db_path = context.resolved_path + assert db_path is not None, ( + f"Could not extract a file path from database URL: {context.resolved_url}" + ) + assert str(db_path).startswith(str(home)), ( + f"Database path {db_path} is NOT inside CLEVERAGENTS_HOME {home}.\n" + f"Full database URL: {context.resolved_url}" + ) + + +@then("the resolved database path should not be inside the original CWD") +def step_path_not_in_cwd(context: Context) -> None: + """Assert the database path is NOT under the original CWD.""" + cwd = context.original_cwd + db_path = context.resolved_path + assert db_path is not None, ( + f"Could not extract a file path from database URL: {context.resolved_url}" + ) + # The DB should not resolve to a child of the original CWD + # (unless CLEVERAGENTS_HOME happens to be inside CWD, which it isn't + # for our temp directory setup). + home = Path(context.tdd_home).resolve() + if not str(home).startswith(str(cwd)): + # Only check if home is NOT under cwd (normal case) + assert not str(db_path).startswith(str(cwd)), ( + f"Database path {db_path} is inside CWD {cwd} instead of " + f"CLEVERAGENTS_HOME {home}.\n" + f"Full database URL: {context.resolved_url}" + ) + + +@then("the settings database path should be inside CLEVERAGENTS_HOME") +def step_settings_path_inside_home(context: Context) -> None: + """Assert the Settings-level database_url resolves inside CLEVERAGENTS_HOME.""" + home = Path(context.tdd_home).resolve() + db_path = context.settings_db_path + assert db_path is not None, ( + f"Could not extract a file path from Settings database URL: " + f"{context.settings_db_url}" + ) + assert str(db_path).startswith(str(home)), ( + f"Settings database path {db_path} is NOT inside " + f"CLEVERAGENTS_HOME {home}.\n" + f"Full Settings database URL: {context.settings_db_url}" + ) diff --git a/features/tdd_sqlite_url_cwd.feature b/features/tdd_sqlite_url_cwd.feature new file mode 100644 index 000000000..a1f257f82 --- /dev/null +++ b/features/tdd_sqlite_url_cwd.feature @@ -0,0 +1,27 @@ +@tdd_expected_fail @tdd_bug @tdd_bug_1024 +Feature: TDD Bug #1024 — SQLite DB URL resolves to CWD instead of CLEVERAGENTS_HOME + As a developer + I want to verify that the SQLite database file is created inside + CLEVERAGENTS_HOME (not the current working directory) + So that test isolation is maintained across CLI invocations + + The default ``database_url`` in ``Settings`` is ``sqlite:///cleveragents.db`` + — a relative path. When E2E tests (or any CLI invocation) run with + ``CLEVERAGENTS_HOME`` set to a different directory, the DB file is still + created relative to CWD, NOT inside ``CLEVERAGENTS_HOME``. This breaks + test isolation: data persists across runs, causing UNIQUE constraint + failures. + + Bug: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/1024 + TDD: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/1034 + + Scenario: Default database_url resolves inside CLEVERAGENTS_HOME not CWD + Given a fresh CLEVERAGENTS_HOME temp directory for DB URL testing + When I resolve the effective database URL from Settings + Then the resolved database path should be inside CLEVERAGENTS_HOME + And the resolved database path should not be inside the original CWD + + Scenario: Settings database_url default resolves inside CLEVERAGENTS_HOME + Given a fresh CLEVERAGENTS_HOME temp directory for DB URL testing + When I resolve the Settings database_url default + Then the settings database path should be inside CLEVERAGENTS_HOME diff --git a/robot/helper_tdd_sqlite_url_cwd.py b/robot/helper_tdd_sqlite_url_cwd.py new file mode 100644 index 000000000..b1de58057 --- /dev/null +++ b/robot/helper_tdd_sqlite_url_cwd.py @@ -0,0 +1,202 @@ +"""Helper script for tdd_sqlite_url_cwd.robot integration tests. + +Each subcommand exercises the real application path (no mocks) to +reproduce bug #1024. The helper reports the **real** outcome: it exits +0 and prints the sentinel when the database resolves inside +CLEVERAGENTS_HOME (bug is fixed), and exits 1 when it resolves to CWD +(bug still present). The ``tdd_expected_fail_listener`` on the Robot +side handles pass/fail inversion while the bug remains open. + +Bug: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/1024 +TDD: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/1034 +""" + +from __future__ import annotations + +import os +import shutil +import sys +import tempfile +from collections.abc import Callable +from pathlib import Path +from typing import NoReturn + +# Ensure local source tree is importable. +_ROOT = Path(__file__).resolve().parents[1] +_SRC = str(_ROOT / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + + +def _fail(message: str) -> NoReturn: + """Print an error message to stderr and exit with code 1.""" + print(message, file=sys.stderr) + sys.exit(1) + + +def _extract_sqlite_path(url: str) -> Path | None: + """Extract the file path from a SQLite URL.""" + prefix = "sqlite:///" + if not url.startswith(prefix): + return None + raw_path = url[len(prefix) :] + if not raw_path: + return None + return Path(raw_path).resolve() + + +# --------------------------------------------------------------------------- +# Subcommands +# --------------------------------------------------------------------------- + + +def check_db_url_resolution() -> None: + """Verify that get_database_url() resolves inside CLEVERAGENTS_HOME. + + Creates a temporary CLEVERAGENTS_HOME, removes any overriding env + vars for database URLs, resets singletons, and checks whether the + resolved database URL points inside the temporary home directory. + """ + from cleveragents.application.container import ( + get_database_url, + reset_container, + ) + from cleveragents.config.settings import Settings + + original_cwd = Path.cwd().resolve() + tmpdir = tempfile.mkdtemp(prefix="tdd_sqlite_url_1024_robot_") + + # Save and remove any overriding env vars + saved_db_url = os.environ.pop("CLEVERAGENTS_DATABASE_URL", None) + saved_test_db_url = os.environ.pop("CLEVERAGENTS_TEST_DATABASE_URL", None) + + try: + os.environ["CLEVERAGENTS_HOME"] = tmpdir + reset_container() + Settings._instance = None + + resolved_url = get_database_url() + db_path = _extract_sqlite_path(resolved_url) + + home = Path(tmpdir).resolve() + + if db_path is None: + _fail(f"Could not extract file path from database URL: {resolved_url}") + + if not str(db_path).startswith(str(home)): + _fail( + f"Database path {db_path} is NOT inside " + f"CLEVERAGENTS_HOME {home}.\n" + f"Full URL: {resolved_url}\n" + f"CWD was: {original_cwd}" + ) + + print("tdd-sqlite-url-cwd-resolution-ok") + + finally: + # Restore env vars + if saved_db_url is not None: + os.environ["CLEVERAGENTS_DATABASE_URL"] = saved_db_url + else: + os.environ.pop("CLEVERAGENTS_DATABASE_URL", None) + if saved_test_db_url is not None: + os.environ["CLEVERAGENTS_TEST_DATABASE_URL"] = saved_test_db_url + else: + os.environ.pop("CLEVERAGENTS_TEST_DATABASE_URL", None) + os.environ.pop("CLEVERAGENTS_HOME", None) + reset_container() + Settings._instance = None + shutil.rmtree(tmpdir, ignore_errors=True) + + +def check_cli_db_location() -> None: + """Verify that a CLI command creates the DB inside CLEVERAGENTS_HOME. + + Invokes ``session list`` via the Typer CLI runner and then checks + whether any database files ended up inside CLEVERAGENTS_HOME rather + than the original CWD. + """ + from typer.testing import CliRunner + + from cleveragents.application.container import reset_container + from cleveragents.cli.commands.session import app as session_app + from cleveragents.config.settings import Settings + + original_cwd = Path.cwd().resolve() + tmpdir = tempfile.mkdtemp(prefix="tdd_sqlite_url_1024_robot_cli_") + + saved_db_url = os.environ.pop("CLEVERAGENTS_DATABASE_URL", None) + saved_test_db_url = os.environ.pop("CLEVERAGENTS_TEST_DATABASE_URL", None) + + try: + os.environ["CLEVERAGENTS_HOME"] = tmpdir + reset_container() + Settings._instance = None + + runner = CliRunner() + runner.invoke(session_app, ["list"]) + + home = Path(tmpdir).resolve() + + # Check for DB files in CWD that shouldn't be there + suspect_files = [ + original_cwd / "cleveragents.db", + original_cwd / "cleveragents_test.db", + original_cwd / ".cleveragents" / "db.sqlite", + ] + found_in_cwd = [f for f in suspect_files if f.exists()] + + if found_in_cwd: + _fail( + f"Database file(s) found in CWD instead of CLEVERAGENTS_HOME:\n" + f" CWD files: {found_in_cwd}\n" + f" CLEVERAGENTS_HOME: {home}" + ) + + # Verify the DB was actually created inside CLEVERAGENTS_HOME. + # If no DB file exists anywhere, the test isn't exercising the + # database path and the result would be vacuously true. + home_db_files = list(home.rglob("*.db")) + list(home.rglob("*.sqlite")) + if not home_db_files: + _fail( + f"No database file was created inside CLEVERAGENTS_HOME " + f"({home}). The CLI command did not trigger database " + f"creation, so the test result is inconclusive.\n" + f"CWD was: {original_cwd}" + ) + + print("tdd-sqlite-url-cwd-cli-ok") + + finally: + if saved_db_url is not None: + os.environ["CLEVERAGENTS_DATABASE_URL"] = saved_db_url + else: + os.environ.pop("CLEVERAGENTS_DATABASE_URL", None) + if saved_test_db_url is not None: + os.environ["CLEVERAGENTS_TEST_DATABASE_URL"] = saved_test_db_url + else: + os.environ.pop("CLEVERAGENTS_TEST_DATABASE_URL", None) + os.environ.pop("CLEVERAGENTS_HOME", None) + reset_container() + Settings._instance = None + shutil.rmtree(tmpdir, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# Dispatcher +# --------------------------------------------------------------------------- + +_COMMANDS: dict[str, Callable[[], None]] = { + "check-db-url-resolution": check_db_url_resolution, + "check-cli-db-location": check_cli_db_location, +} + +if __name__ == "__main__": + if len(sys.argv) < 2 or sys.argv[1] not in _COMMANDS: + print( + f"Usage: {sys.argv[0]} <{'|'.join(_COMMANDS)}>", + file=sys.stderr, + ) + sys.exit(1) + cmd = _COMMANDS[sys.argv[1]] + cmd() diff --git a/robot/tdd_sqlite_url_cwd.robot b/robot/tdd_sqlite_url_cwd.robot new file mode 100644 index 000000000..f9ea5aae2 --- /dev/null +++ b/robot/tdd_sqlite_url_cwd.robot @@ -0,0 +1,39 @@ +*** Settings *** +Documentation TDD Bug #1024 — SQLite DB URL resolves to CWD instead of CLEVERAGENTS_HOME +... Integration tests verifying that the database URL resolves +... inside CLEVERAGENTS_HOME rather than the current working +... directory. The default ``database_url`` in Settings is +... ``sqlite:///cleveragents.db`` — a relative path that resolves +... against CWD, breaking test isolation when CLEVERAGENTS_HOME +... points to a different directory. +... +... Bug: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/1024 +... TDD: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/1034 +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_tdd_sqlite_url_cwd.py + +*** Test Cases *** +TDD SQLite DB URL Resolves Inside CLEVERAGENTS_HOME + [Documentation] Verify that ``get_database_url()`` resolves the database + ... path inside CLEVERAGENTS_HOME when no explicit database URL + ... is provided via environment variable. + [Tags] tdd_expected_fail tdd_bug tdd_bug_1024 + ${result}= Run Process ${PYTHON} ${HELPER} check-db-url-resolution cwd=${WORKSPACE} timeout=60s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-sqlite-url-cwd-resolution-ok + +TDD CLI Command Creates DB Inside CLEVERAGENTS_HOME + [Documentation] Verify that a CLI command (session list) creates the + ... database file inside CLEVERAGENTS_HOME, not in CWD. + [Tags] tdd_expected_fail tdd_bug tdd_bug_1024 + ${result}= Run Process ${PYTHON} ${HELPER} check-cli-db-location cwd=${WORKSPACE} timeout=60s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-sqlite-url-cwd-cli-ok diff --git a/src/cleveragents/infrastructure/sandbox/strategy_registry.py b/src/cleveragents/infrastructure/sandbox/strategy_registry.py index 04c8c68d0..5d7335f7a 100644 --- a/src/cleveragents/infrastructure/sandbox/strategy_registry.py +++ b/src/cleveragents/infrastructure/sandbox/strategy_registry.py @@ -259,14 +259,14 @@ class SandboxStrategyRegistry: # ------------------------------------------------------------------ @staticmethod - def _validate_protocol(cls: type[Any]) -> None: + def _validate_protocol(klass: type[Any]) -> None: """Validate that a class satisfies :class:`SandboxStrategyProtocol`. Uses structural subtyping: checks that all 9 required methods are present as callable attributes on the class. Args: - cls: The class to validate. + klass: The class to validate. Raises: ProtocolMismatchError: If the class is missing required methods. @@ -286,12 +286,12 @@ class SandboxStrategyRegistry: missing = [ method for method in required_methods - if not callable(getattr(cls, method, None)) + if not callable(getattr(klass, method, None)) ] if missing: msg = ( - f"Class '{cls.__name__}' does not satisfy " + f"Class '{klass.__name__}' does not satisfy " f"SandboxStrategyProtocol. Missing methods: " f"{', '.join(missing)}" )