diff --git a/features/steps/coverage_boost_steps.py b/features/steps/coverage_boost_steps.py index eeadab04d..389297d51 100644 --- a/features/steps/coverage_boost_steps.py +++ b/features/steps/coverage_boost_steps.py @@ -15,9 +15,13 @@ def step_create_settings(context): # Clear any existing singleton if hasattr(Settings, "_instance"): Settings._instance = None - # Remove database URL env vars so we test the actual pydantic defaults, - # not the per-scenario temp paths injected by environment.py. - for key in ("CLEVERAGENTS_DATABASE_URL", "CLEVERAGENTS_TEST_DATABASE_URL"): + # Remove database URL and home env vars so we test the actual pydantic + # defaults, not the per-scenario temp paths injected by environment.py. + for key in ( + "CLEVERAGENTS_DATABASE_URL", + "CLEVERAGENTS_TEST_DATABASE_URL", + "CLEVERAGENTS_HOME", + ): os.environ.pop(key, None) context.settings = Settings() @@ -50,11 +54,15 @@ def step_verify_production_status(context): @when("I get the database URL from Settings") def step_get_database_url(context): """Get database URL.""" - # Clear singleton and database URL env vars so we test the actual + # Clear singleton and database URL/home env vars so we test the actual # pydantic defaults, not per-scenario temp paths from environment.py. if hasattr(Settings, "_instance"): Settings._instance = None - for key in ("CLEVERAGENTS_DATABASE_URL", "CLEVERAGENTS_TEST_DATABASE_URL"): + for key in ( + "CLEVERAGENTS_DATABASE_URL", + "CLEVERAGENTS_TEST_DATABASE_URL", + "CLEVERAGENTS_HOME", + ): os.environ.pop(key, None) settings = Settings() context.db_url = settings.get_database_url() diff --git a/features/steps/settings_steps.py b/features/steps/settings_steps.py index b6a250604..59210927e 100644 --- a/features/steps/settings_steps.py +++ b/features/steps/settings_steps.py @@ -32,6 +32,14 @@ def step_clear_env_vars(context): "CLEVERAGENTS_DEFAULT_PROVIDER", "CLEVERAGENTS_DEFAULT_MODEL", "CLEVERAGENTS_OPENROUTER_ORGANIZATION", + # Database URL env vars: clear so scenarios testing the default or an + # explicit override start from a clean slate, regardless of what + # before_scenario injected for test isolation. + "CLEVERAGENTS_DATABASE_URL", + "CLEVERAGENTS_TEST_DATABASE_URL", + # CLEVERAGENTS_HOME affects database_url resolution; must be cleared + # alongside the URL vars so Settings() uses the true default path. + "CLEVERAGENTS_HOME", "DATABASE_URL", "OPENAI_API_KEY", "OPENROUTER_API_KEY", diff --git a/features/steps/tdd_sqlite_url_cwd_steps.py b/features/steps/tdd_sqlite_url_cwd_steps.py index 6765dacb0..f6d08f97c 100644 --- a/features/steps/tdd_sqlite_url_cwd_steps.py +++ b/features/steps/tdd_sqlite_url_cwd_steps.py @@ -59,6 +59,10 @@ def _extract_sqlite_path(url: str) -> Path | None: Handles both ``sqlite:///path`` (relative) and ``sqlite:////abs/path`` (absolute) forms. + + Note: intentionally duplicated in robot/helper_tdd_sqlite_url_cwd.py + to keep the Behave and Robot test suites independently runnable + without shared test utility coupling. """ prefix = "sqlite:///" if not url.startswith(prefix): diff --git a/features/tdd_sqlite_url_cwd.feature b/features/tdd_sqlite_url_cwd.feature index cd48d848d..2f98b9af5 100644 --- a/features/tdd_sqlite_url_cwd.feature +++ b/features/tdd_sqlite_url_cwd.feature @@ -15,14 +15,12 @@ Feature: TDD Issue #1024 — SQLite DB URL resolves to CWD instead of CLEVERAGEN Bug: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/1024 TDD: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/1034 - @tdd_issue @tdd_issue_4288 @tdd_expected_fail 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 - @tdd_issue @tdd_issue_4288 @tdd_expected_fail 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 diff --git a/robot/core_cli_commands.robot b/robot/core_cli_commands.robot index 803c3dc72..7862194ad 100644 --- a/robot/core_cli_commands.robot +++ b/robot/core_cli_commands.robot @@ -229,10 +229,10 @@ Setup Test Environment Set Suite Variable ${PROJECT_NAME} test-project-${run_id} Set Environment Variable CLEVERAGENTS_TESTING_USE_MOCK_AI true Create Directory ${TEST_DIR} - ${home}= Set Variable ${TEST_DIR}${/}.cleveragents_home - Run Keyword And Ignore Error Remove Directory ${home} recursive=True - Create Directory ${home} - Set Environment Variable CLEVERAGENTS_HOME ${home} + # Do NOT set CLEVERAGENTS_HOME here — individual test cases use + # separate project directories (cwd=...) and expect database files + # to be created relative to CWD, not a shared home directory. + Remove Environment Variable CLEVERAGENTS_HOME Clean Core CLI Temp Dir Run Keyword And Ignore Error Remove Directory ${TEST_DIR} recursive=True diff --git a/robot/helper_tdd_sqlite_url_cwd.py b/robot/helper_tdd_sqlite_url_cwd.py index b1de58057..e78dcbf11 100644 --- a/robot/helper_tdd_sqlite_url_cwd.py +++ b/robot/helper_tdd_sqlite_url_cwd.py @@ -35,7 +35,12 @@ def _fail(message: str) -> NoReturn: def _extract_sqlite_path(url: str) -> Path | None: - """Extract the file path from a SQLite URL.""" + """Extract the file path from a SQLite URL. + + Note: intentionally duplicated in features/steps/tdd_sqlite_url_cwd_steps.py + to keep the Robot and Behave test suites independently runnable + without shared test utility coupling. + """ prefix = "sqlite:///" if not url.startswith(prefix): return None @@ -113,8 +118,9 @@ 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. + whether any NEW database files ended up in CWD rather than + CLEVERAGENTS_HOME. Records which files exist before the command + runs so pre-existing files from earlier test runs are not flagged. """ from typer.testing import CliRunner @@ -133,23 +139,30 @@ def check_cli_db_location() -> None: 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 + # Record which suspect files already exist before running the command, + # so we only flag files that are newly created by the CLI invocation. 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()] + pre_existing = {str(f) for f in suspect_files if f.exists()} - if found_in_cwd: + runner = CliRunner() + runner.invoke(session_app, ["list"]) + + home = Path(tmpdir).resolve() + + # Check for NEW DB files in CWD that should not be there + newly_created = [ + f for f in suspect_files if f.exists() and str(f) not in pre_existing + ] + + if newly_created: _fail( - f"Database file(s) found in CWD instead of CLEVERAGENTS_HOME:\n" - f" CWD files: {found_in_cwd}\n" + f"Database file(s) newly created in CWD instead of " + f"CLEVERAGENTS_HOME:\n" + f" New CWD files: {newly_created}\n" f" CLEVERAGENTS_HOME: {home}" ) diff --git a/robot/tdd_sqlite_url_cwd.robot b/robot/tdd_sqlite_url_cwd.robot index 8ed1e35e2..2779eac10 100644 --- a/robot/tdd_sqlite_url_cwd.robot +++ b/robot/tdd_sqlite_url_cwd.robot @@ -21,7 +21,6 @@ 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_issue tdd_issue_1024 tdd_issue tdd_issue_4320 tdd_expected_fail ${result}= Run Process ${PYTHON} ${HELPER} check-db-url-resolution cwd=${WORKSPACE} timeout=60s on_timeout=kill Log ${result.stdout} @@ -32,7 +31,6 @@ TDD SQLite DB URL Resolves Inside CLEVERAGENTS_HOME 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_issue tdd_issue_1024 tdd_issue tdd_issue_4320 tdd_expected_fail ${result}= Run Process ${PYTHON} ${HELPER} check-cli-db-location cwd=${WORKSPACE} timeout=60s on_timeout=kill Log ${result.stdout} diff --git a/src/cleveragents/application/container.py b/src/cleveragents/application/container.py index 2afa51536..a206c60f5 100644 --- a/src/cleveragents/application/container.py +++ b/src/cleveragents/application/container.py @@ -170,6 +170,34 @@ class TuiServiceAdapters: plan_service: PlanService +def _ensure_sqlite_parent_dir(database_url: str) -> None: + """Create the parent directory for a SQLite database URL if needed. + + Called by engine-building helpers so that the ``.cleveragents/`` + directory (or whichever parent the resolved URL points at) exists + before SQLAlchemy attempts to create the file. This is separate + from ``get_database_url()`` to avoid eagerly creating the project + marker directory during URL resolution, which would break + ``agents init`` detection. + + Special SQLite URLs (e.g. ``:memory:``) are skipped since they + have no filesystem path. Errors during directory creation are + silently ignored — this is a best-effort convenience; the + downstream engine/session will raise the proper error if the + path is truly unusable. + """ + prefix = "sqlite:///" + if not database_url.startswith(prefix): + return + raw_path = database_url[len(prefix) :] + if raw_path and not raw_path.startswith(":"): + try: + db_file = Path(raw_path) + db_file.parent.mkdir(parents=True, exist_ok=True) + except OSError: + pass + + def get_ai_provider( settings: Settings | None = None, provider_registry: ProviderRegistry | None = None, @@ -218,6 +246,9 @@ def get_database_url() -> str: """Get the database URL with proper path handling. Prefers explicit test overrides to keep CI fast and non-interactive. + When no explicit URL is set, resolves the default database path + relative to ``CLEVERAGENTS_HOME`` (if set) rather than CWD, ensuring + test isolation and correct data placement. Returns: SQLAlchemy database URL with absolute path for SQLite @@ -232,9 +263,20 @@ def get_database_url() -> str: if env_url: return env_url - # Fallback to file-based SQLite in the current working directory - db_path = Path.cwd() / ".cleveragents" / "db.sqlite" - # Don't create directory here - let ProjectService handle it + # Resolve relative to CLEVERAGENTS_HOME when set, otherwise CWD. + # This ensures that database files live inside the project home + # directory rather than wherever the process happens to be invoked, + # which is critical for test isolation and multi-project setups. + home = os.environ.get("CLEVERAGENTS_HOME") + base_dir = Path(home) if home else Path.cwd() + db_path = base_dir / ".cleveragents" / "db.sqlite" + # Do NOT eagerly create the parent directory here. The + # ``.cleveragents/`` directory is the project marker checked by + # ``agents init``; creating it prematurely causes init to report + # "Project already initialized" when CLEVERAGENTS_HOME == CWD. + # Directory creation is handled by ``agents init`` and by the + # engine-building helpers (_build_session_service, etc.) that + # call ``Base.metadata.create_all()`` after ensuring the schema. # SQLite requires absolute path with 4 slashes for file URLs return f"sqlite:///{db_path.absolute()}" @@ -254,6 +296,7 @@ def _build_repo_indexing_service( from sqlalchemy import create_engine from sqlalchemy.orm import sessionmaker + _ensure_sqlite_parent_dir(database_url) engine = create_engine(database_url, echo=False) factory = sessionmaker(bind=engine, expire_on_commit=False) return RepoIndexingService(session_factory=factory) @@ -268,6 +311,7 @@ def _build_session_factory(database_url: str) -> sessionmaker[Session]: from sqlalchemy import create_engine from sqlalchemy.orm import sessionmaker + _ensure_sqlite_parent_dir(database_url) engine = create_engine(database_url, echo=False) return sessionmaker(bind=engine, expire_on_commit=False) @@ -297,6 +341,7 @@ def _build_resource_registry_service( from sqlalchemy import create_engine from sqlalchemy.orm import sessionmaker + _ensure_sqlite_parent_dir(database_url) engine = create_engine(database_url, echo=False) factory = sessionmaker(bind=engine, expire_on_commit=False) return ResourceRegistryService(session_factory=factory) @@ -309,6 +354,7 @@ def _build_namespaced_project_repo( from sqlalchemy import create_engine from sqlalchemy.orm import sessionmaker + _ensure_sqlite_parent_dir(database_url) engine = create_engine(database_url, echo=False) factory = sessionmaker(bind=engine, expire_on_commit=False) return NamespacedProjectRepository(session_factory=factory) @@ -321,6 +367,7 @@ def _build_project_resource_link_repo( from sqlalchemy import create_engine from sqlalchemy.orm import sessionmaker + _ensure_sqlite_parent_dir(database_url) engine = create_engine(database_url, echo=False) factory = sessionmaker(bind=engine, expire_on_commit=False) return ProjectResourceLinkRepository(session_factory=factory) @@ -339,6 +386,7 @@ def _build_checkpoint_service( from sqlalchemy import create_engine from sqlalchemy.orm import sessionmaker + _ensure_sqlite_parent_dir(database_url) engine = create_engine(database_url, echo=False) factory = sessionmaker(bind=engine, expire_on_commit=False) repository = CheckpointRepository(session_factory=factory) @@ -390,6 +438,7 @@ def _build_trace_service( from sqlalchemy import create_engine from sqlalchemy.orm import sessionmaker + _ensure_sqlite_parent_dir(database_url) engine = create_engine(database_url, echo=False) factory = sessionmaker(bind=engine, expire_on_commit=False) repository = LLMTraceRepository(session_factory=factory) @@ -436,6 +485,7 @@ def _build_skill_service( SkillRepository, ) + _ensure_sqlite_parent_dir(database_url) engine = create_engine(database_url, echo=False) # Targeted table creation — only skill tables, never the full @@ -547,6 +597,7 @@ def _build_session_service( SessionModel, ) + _ensure_sqlite_parent_dir(database_url) engine = create_engine(database_url, echo=False) # Targeted table creation — only session tables, never the full schema. diff --git a/src/cleveragents/cli/commands/system.py b/src/cleveragents/cli/commands/system.py index 1e4f02eb6..b1416adf1 100644 --- a/src/cleveragents/cli/commands/system.py +++ b/src/cleveragents/cli/commands/system.py @@ -218,9 +218,15 @@ def _check_database() -> dict[str, Any]: "status": CheckStatus.OK if writable else CheckStatus.ERROR, "details": "writable" if writable else "locked or not writable", } - # DB file doesn't exist yet — that's OK for SQLite (created on first use) - parent = db_path.parent - if parent.exists() and os.access(parent, os.W_OK): + # DB file doesn't exist yet — that's OK for SQLite (created on first use). + # Allow at most one missing intermediate directory (e.g. .cleveragents/ + # before ``agents init`` runs). Do NOT walk all the way to the + # filesystem root — that would return OK for completely invalid paths + # when running as root (e.g. in CI). + ancestor = db_path.parent + if not ancestor.exists(): + ancestor = ancestor.parent + if ancestor.exists() and os.access(ancestor, os.W_OK): return { "name": "Database", "status": CheckStatus.OK, @@ -229,7 +235,7 @@ def _check_database() -> dict[str, Any]: return { "name": "Database", "status": CheckStatus.ERROR, - "details": f"parent dir not writable ({parent})", + "details": f"parent dir not writable ({ancestor})", } return { diff --git a/src/cleveragents/config/settings.py b/src/cleveragents/config/settings.py index a0d5b304f..db91dbdf2 100644 --- a/src/cleveragents/config/settings.py +++ b/src/cleveragents/config/settings.py @@ -29,6 +29,33 @@ class ProviderDefaults: model_source: str +def _resolve_sqlite_url(url: str, base_dir: Path) -> str: + """Resolve a relative SQLite URL to an absolute path under *base_dir*. + + Only modifies ``sqlite:///`` URLs whose path component is relative. + Absolute paths, non-SQLite URLs, and special SQLite connection + strings (e.g. ``:memory:``) are returned unchanged. + + Note: No path containment validation — database_url is admin-configured, + not untrusted user input. Paths with ``../`` could resolve outside + CLEVERAGENTS_HOME. + """ + prefix = "sqlite:///" + if not url.startswith(prefix): + return url + raw_path = url[len(prefix) :] + if not raw_path: + return url + # Preserve special SQLite connection strings like :memory: + if raw_path.startswith(":"): + return url + p = Path(raw_path) + if p.is_absolute(): + return url + resolved = (base_dir / p).resolve() + return f"sqlite:///{resolved}" + + class Settings(BaseSettings): """Application runtime configuration backed by environment variables.""" @@ -330,13 +357,31 @@ class Settings(BaseSettings): # Persistence database_url: str = Field( default_factory=lambda: ( - f"sqlite:///{Path.home() / '.cleveragents' / 'cleveragents.db'}" + "sqlite:///" + + str( + Path( + os.environ.get( + "CLEVERAGENTS_HOME", + str(Path.home() / ".cleveragents"), + ) + ) + / "cleveragents.db" + ) ), validation_alias=AliasChoices("CLEVERAGENTS_DATABASE_URL"), ) test_database_url: str = Field( default_factory=lambda: ( - f"sqlite:///{Path.home() / '.cleveragents' / 'cleveragents_test.db'}" + "sqlite:///" + + str( + Path( + os.environ.get( + "CLEVERAGENTS_HOME", + str(Path.home() / ".cleveragents"), + ) + ) + / "cleveragents_test.db" + ) ), validation_alias=AliasChoices("CLEVERAGENTS_TEST_DATABASE_URL"), ) @@ -673,6 +718,41 @@ class Settings(BaseSettings): ), ) + @model_validator(mode="after") + def _resolve_database_urls(self) -> Settings: + """Resolve relative SQLite database URLs against CLEVERAGENTS_HOME. + + The default ``database_url`` is ``sqlite:///cleveragents.db`` — a + relative path. Without resolution this would create the DB file + relative to CWD, breaking test isolation when + ``CLEVERAGENTS_HOME`` points elsewhere. This validator rewrites + relative SQLite paths to be absolute under ``CLEVERAGENTS_HOME`` + (if set) or CWD (as a fallback). + + Resolution is **skipped** when the corresponding env var + (``CLEVERAGENTS_DATABASE_URL`` / ``CLEVERAGENTS_TEST_DATABASE_URL``) + is explicitly set to a non-empty value. In that case the caller + chose a specific URL intentionally and it must be honoured as-is. + """ + home_env = os.environ.get("CLEVERAGENTS_HOME") + base_dir = Path(home_env) if home_env else Path.cwd() + + for attr, env_key in ( + ("database_url", "CLEVERAGENTS_DATABASE_URL"), + ("test_database_url", "CLEVERAGENTS_TEST_DATABASE_URL"), + ): + # If the caller explicitly provided this URL via env var, use + # it as-is. Only resolve DEFAULT values (factory-generated or + # bare relative paths that the user did not deliberately set). + if os.environ.get(env_key): + continue + url: str = getattr(self, attr) + resolved = _resolve_sqlite_url(url, base_dir) + if resolved != url: + object.__setattr__(self, attr, resolved) + + return self + @model_validator(mode="after") def _max_delay_ge_base_delay(self) -> Settings: """Ensure retry_max_delay >= retry_base_delay."""