diff --git a/CHANGELOG.md b/CHANGELOG.md index 36cb64469..a790e74d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,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) +- Fixed default SQLite database URL resolving relative to CWD instead of + CLEVERAGENTS_HOME. Added a model validator in Settings that rewrites + relative SQLite paths to be absolute under CLEVERAGENTS_HOME (or CWD + as fallback). Updated get_database_url() in the container to use + CLEVERAGENTS_HOME as the base directory for the default database path. + Removed @tdd_expected_fail from TDD tests confirming the fix. (#1024) - 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 diff --git a/features/steps/coverage_boost_steps.py b/features/steps/coverage_boost_steps.py index fa0c14ddc..d5534787b 100644 --- a/features/steps/coverage_boost_steps.py +++ b/features/steps/coverage_boost_steps.py @@ -26,7 +26,10 @@ def step_check_default_values(context): """Check Settings has default values.""" assert context.settings.server_host == "0.0.0.0" assert context.settings.server_port == 8080 - assert context.settings.database_url == "sqlite:///cleveragents.db" + # After the _resolve_database_urls validator, relative SQLite paths are + # resolved to absolute paths under CLEVERAGENTS_HOME (or CWD). + assert context.settings.database_url.startswith("sqlite:///") + assert context.settings.database_url.endswith("cleveragents.db") assert context.settings.debug_log_level == "INFO" assert context.settings.storage_base_path.name == "data" @@ -62,8 +65,12 @@ def step_get_database_url(context): @then("it should return the configured database URL") def step_check_database_url(context): """Check database URL.""" - assert context.db_url == "sqlite:///cleveragents.db" - assert context.test_db_url == "sqlite:///cleveragents_test.db" + # After the _resolve_database_urls validator, relative SQLite paths are + # resolved to absolute paths under CLEVERAGENTS_HOME (or CWD). + assert context.db_url.startswith("sqlite:///") + assert context.db_url.endswith("cleveragents.db") + assert context.test_db_url.startswith("sqlite:///") + assert context.test_db_url.endswith("cleveragents_test.db") @when("I check if any provider is configured in Settings") diff --git a/features/steps/tdd_sqlite_url_cwd_steps.py b/features/steps/tdd_sqlite_url_cwd_steps.py index 0de2692ff..a3cbecfe2 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 a1f257f82..6cb3fca15 100644 --- a/features/tdd_sqlite_url_cwd.feature +++ b/features/tdd_sqlite_url_cwd.feature @@ -1,4 +1,4 @@ -@tdd_expected_fail @tdd_bug @tdd_bug_1024 +@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 diff --git a/noxfile.py b/noxfile.py index bc9365c4f..9f799e864 100644 --- a/noxfile.py +++ b/noxfile.py @@ -539,7 +539,7 @@ def serve_docs(session: nox.Session): @nox.session(python=SUPPORTED_PYTHONS, reuse_venv=True, venv_backend="uv") def build(session: nox.Session): """Build the wheel distribution.""" - session.install("build") + session.install("pip", "build") session.run("python", "-m", "build", "--wheel") @@ -911,6 +911,7 @@ def coverage_report(session: nox.Session): def pre_commit(session: nox.Session): """Run all pre-commit hooks on all files.""" session.install("-e", ".[dev]") + session.install("setuptools<81") # pkg_resources for semgrep session.run("pre-commit", "run", "--all-files", *session.posargs) @@ -932,6 +933,7 @@ def security_scan(session: nox.Session): - Vulture >=80% confidence: hard fail """ session.install("-e", ".[dev]") + session.install("setuptools<81") # pkg_resources for semgrep # Ensure output directory exists (CI starts with a clean checkout) os.makedirs("build", exist_ok=True) @@ -993,6 +995,7 @@ def security_scan(session: nox.Session): def dead_code(session: nox.Session): """Run vulture to detect dead code.""" session.install("-e", ".[dev]") + session.install("setuptools<81") # pkg_resources for semgrep session.run( "vulture", "src/cleveragents", @@ -1008,6 +1011,7 @@ def dead_code(session: nox.Session): def complexity(session: nox.Session): """Check code complexity using radon.""" session.install("-e", ".[dev]") + session.install("setuptools<81") # pkg_resources for semgrep session.run( "radon", "cc", @@ -1023,6 +1027,7 @@ def complexity(session: nox.Session): def adr_compliance(session: nox.Session): """Check code compliance with Architecture Decision Records.""" session.install("-e", ".[dev]") + session.install("setuptools<81") # pkg_resources for semgrep session.run("python", "scripts/check-adr-compliance.py") diff --git a/robot/core_cli_commands.robot b/robot/core_cli_commands.robot index 9195735d0..c85c94a28 100644 --- a/robot/core_cli_commands.robot +++ b/robot/core_cli_commands.robot @@ -232,10 +232,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 f9ea5aae2..aabf9b217 100644 --- a/robot/tdd_sqlite_url_cwd.robot +++ b/robot/tdd_sqlite_url_cwd.robot @@ -21,7 +21,7 @@ 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 + [Tags] 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} @@ -31,7 +31,7 @@ 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_expected_fail tdd_bug tdd_bug_1024 + [Tags] 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} diff --git a/src/cleveragents/application/container.py b/src/cleveragents/application/container.py index 360c33cb7..cf14563bf 100644 --- a/src/cleveragents/application/container.py +++ b/src/cleveragents/application/container.py @@ -153,6 +153,25 @@ 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. + """ + prefix = "sqlite:///" + if not database_url.startswith(prefix): + return + raw_path = database_url[len(prefix) :] + if raw_path: + db_file = Path(raw_path) + db_file.parent.mkdir(parents=True, exist_ok=True) + + def get_ai_provider( settings: Settings | None = None, provider_registry: ProviderRegistry | None = None, @@ -201,6 +220,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 @@ -215,9 +237,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()}" @@ -237,6 +270,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) @@ -251,6 +285,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) @@ -262,6 +297,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) @@ -274,6 +310,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) @@ -286,6 +323,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) @@ -304,6 +342,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) @@ -355,6 +394,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) @@ -401,6 +441,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 @@ -461,6 +502,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 eeb6a53a9..903bbf1ba 100644 --- a/src/cleveragents/cli/commands/system.py +++ b/src/cleveragents/cli/commands/system.py @@ -205,9 +205,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). + # Walk up the directory tree to find the nearest existing ancestor; + # intermediate directories (e.g. .cleveragents/) are created by + # ``agents init`` or the project bootstrap, so we only require + # that *some* ancestor is writable. + ancestor = db_path.parent + while not ancestor.exists() and ancestor != ancestor.parent: + ancestor = ancestor.parent + if ancestor.exists() and os.access(ancestor, os.W_OK): return { "name": "Database", "status": CheckStatus.OK, @@ -216,7 +222,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 41175ff9b..58cb60f36 100644 --- a/src/cleveragents/config/settings.py +++ b/src/cleveragents/config/settings.py @@ -23,6 +23,29 @@ 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 and non-SQLite URLs 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 + 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.""" @@ -554,6 +577,28 @@ 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). + """ + home_env = os.environ.get("CLEVERAGENTS_HOME") + base_dir = Path(home_env) if home_env else Path.cwd() + + for attr in ("database_url", "test_database_url"): + 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."""