From 092167ed4863561369e14228eea8ba3c6840e6dd Mon Sep 17 00:00:00 2001 From: ghinks Date: Mon, 15 Jun 2026 08:33:15 -0400 Subject: [PATCH 1/2] fix(cli): handle missing database gracefully during classify Running classify in a directory with no review_classification.db produced a raw SQLAlchemy traceback (OperationalError: no such table: pullrequest) because the org path queries the DB before any table is created. - Add database_exists() helper and an upfront guard in the classify command that prints a clean error and exits 1 when no DB is present. - Harden get_repos_for_org() to return [] on OperationalError, mirroring get_latest_pr_date(), for present-but-uninitialized databases. Closes #62 Co-Authored-By: Claude Opus 4.8 --- src/review_classification/cli/app.py | 10 +++ src/review_classification/sqlite/database.py | 26 +++++-- tests/cli/test_classify.py | 72 ++++++++++++++++++++ tests/sqlite/test_database.py | 27 ++++++++ 4 files changed, 128 insertions(+), 7 deletions(-) create mode 100644 tests/cli/test_classify.py diff --git a/src/review_classification/cli/app.py b/src/review_classification/cli/app.py index 6f5d4bb..aa1945a 100644 --- a/src/review_classification/cli/app.py +++ b/src/review_classification/cli/app.py @@ -570,6 +570,16 @@ def classify( ) raise typer.Exit(code=1) + from ..sqlite.database import database_exists + + if not database_exists(): + typer.echo( + "Error: No database found. Run 'review-classify fetch' first " + "to collect PR data.", + err=True, + ) + raise typer.Exit(code=1) + try: targets = _resolve_targets( repos=repo_list, diff --git a/src/review_classification/sqlite/database.py b/src/review_classification/sqlite/database.py index 8317d75..4631d55 100644 --- a/src/review_classification/sqlite/database.py +++ b/src/review_classification/sqlite/database.py @@ -1,3 +1,4 @@ +import os from datetime import datetime from sqlalchemy import text @@ -12,6 +13,11 @@ engine = create_engine(sqlite_url) +def database_exists() -> bool: + """Return True if the SQLite database file exists on disk.""" + return os.path.exists(sqlite_file_name) + + def init_db() -> None: """Initialize the database tables.""" SQLModel.metadata.create_all(engine) @@ -123,14 +129,20 @@ def get_repos_for_org(org_name: str) -> list[str]: """Return distinct repository names stored in the DB for the given org/owner. Avoids any network call — resolves org repos entirely from fetched data. + Returns an empty list if the database has not been initialized yet. """ - with Session(engine) as session: - statement = ( - select(col(PullRequest.repository_name)) - .where(col(PullRequest.repository_name).like(f"{org_name}/%")) - .distinct() - ) - return sorted(session.exec(statement).all()) + from sqlalchemy.exc import OperationalError + + try: + with Session(engine) as session: + statement = ( + select(col(PullRequest.repository_name)) + .where(col(PullRequest.repository_name).like(f"{org_name}/%")) + .distinct() + ) + return sorted(session.exec(statement).all()) + except OperationalError: + return [] def get_outlier_scores( diff --git a/tests/cli/test_classify.py b/tests/cli/test_classify.py new file mode 100644 index 0000000..21aa389 --- /dev/null +++ b/tests/cli/test_classify.py @@ -0,0 +1,72 @@ +"""Tests for classify command behavior.""" + +from importlib import import_module +from pathlib import Path + +import pytest +import typer + +cli_app = import_module("review_classification.cli.app") + + +def test_classify_missing_database_shows_clean_error( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """classify in a dir with no DB exits 1 with a clean message, no traceback.""" + monkeypatch.chdir(tmp_path) + + with pytest.raises(typer.Exit) as exc_info: + cli_app.classify( + repo=["my-org/repo"], + org=None, + config=None, + threshold=2.0, + min_samples=30, + output_format="table", + verbose=False, + start=None, + end=None, + exclude_primary_merged=False, + ) + + assert exc_info.value.exit_code == 1 + + captured = capsys.readouterr() + assert "No database found" in captured.err + assert "fetch" in captured.err + # The raw SQLAlchemy error must never reach the user. + assert "OperationalError" not in captured.err + assert "no such table" not in captured.err + # No stray empty database file should be created. + assert not (tmp_path / "review_classification.db").exists() + + +def test_classify_missing_database_for_org_shows_clean_error( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """The org path (which reads the DB to resolve repos) also fails gracefully.""" + monkeypatch.chdir(tmp_path) + + with pytest.raises(typer.Exit) as exc_info: + cli_app.classify( + repo=None, + org=["my-org"], + config=None, + threshold=2.0, + min_samples=30, + output_format="table", + verbose=False, + start=None, + end=None, + exclude_primary_merged=False, + ) + + assert exc_info.value.exit_code == 1 + + captured = capsys.readouterr() + assert "No database found" in captured.err + assert "OperationalError" not in captured.err diff --git a/tests/sqlite/test_database.py b/tests/sqlite/test_database.py index f4e74da..66b8dd0 100644 --- a/tests/sqlite/test_database.py +++ b/tests/sqlite/test_database.py @@ -1,6 +1,7 @@ """Tests for sqlite.database helper functions.""" from datetime import UTC, datetime +from pathlib import Path import pytest from sqlalchemy.engine import Engine @@ -83,3 +84,29 @@ def test_get_repos_for_org_deduplicates(patched_engine: Engine) -> None: session.commit() assert get_repos_for_org("my-org") == ["my-org/repo-a"] + + +def test_get_repos_for_org_handles_missing_table( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Returns [] instead of raising when the database has no tables yet.""" + import review_classification.sqlite.database as db_module + + # Engine pointing at an in-memory DB with no tables created. + empty_engine = create_engine("sqlite:///:memory:") + monkeypatch.setattr(db_module, "engine", empty_engine) + + assert db_module.get_repos_for_org("any-org") == [] + + +def test_database_exists_reflects_file_presence( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """database_exists() tracks whether the SQLite file is on disk.""" + from review_classification.sqlite.database import database_exists, sqlite_file_name + + monkeypatch.chdir(tmp_path) + assert database_exists() is False + + (tmp_path / sqlite_file_name).touch() + assert database_exists() is True From 2cde314af88342702b38f134cbb768b59df4a136 Mon Sep 17 00:00:00 2001 From: ghinks Date: Mon, 15 Jun 2026 08:41:28 -0400 Subject: [PATCH 2/2] fix(cli): treat uninitialized database as not present in classify guard An empty review_classification.db left behind by a failed run passed the file-only existence check, so classify --org returned [] from get_repos_for_org and exited 0 with an empty report instead of prompting the user to run fetch. Replace database_exists() with database_is_initialized(), which keeps the on-disk short-circuit (no stray file creation) and additionally verifies the pullrequest table exists via the SQLAlchemy inspector. Co-Authored-By: Claude Opus 4.8 --- src/review_classification/cli/app.py | 4 +- src/review_classification/sqlite/database.py | 21 ++++++++-- tests/cli/test_classify.py | 33 ++++++++++++++++ tests/sqlite/test_database.py | 40 +++++++++++++++++--- 4 files changed, 87 insertions(+), 11 deletions(-) diff --git a/src/review_classification/cli/app.py b/src/review_classification/cli/app.py index aa1945a..d51c580 100644 --- a/src/review_classification/cli/app.py +++ b/src/review_classification/cli/app.py @@ -570,9 +570,9 @@ def classify( ) raise typer.Exit(code=1) - from ..sqlite.database import database_exists + from ..sqlite.database import database_is_initialized - if not database_exists(): + if not database_is_initialized(): typer.echo( "Error: No database found. Run 'review-classify fetch' first " "to collect PR data.", diff --git a/src/review_classification/sqlite/database.py b/src/review_classification/sqlite/database.py index 4631d55..3fdd4bd 100644 --- a/src/review_classification/sqlite/database.py +++ b/src/review_classification/sqlite/database.py @@ -13,9 +13,24 @@ engine = create_engine(sqlite_url) -def database_exists() -> bool: - """Return True if the SQLite database file exists on disk.""" - return os.path.exists(sqlite_file_name) +def database_is_initialized() -> bool: + """Return True if the database file exists and contains the PR table. + + A bare file with no tables (e.g. left behind by an earlier failed run) + counts as *not* initialized, so callers can prompt the user to run + ``fetch`` first instead of producing an empty report. The on-disk check + runs first to avoid lazily creating an empty database file. + """ + from sqlalchemy import inspect + from sqlalchemy.exc import OperationalError + + if not os.path.exists(sqlite_file_name): + return False + + try: + return inspect(engine).has_table(str(PullRequest.__tablename__)) + except OperationalError: + return False def init_db() -> None: diff --git a/tests/cli/test_classify.py b/tests/cli/test_classify.py index 21aa389..fb815d3 100644 --- a/tests/cli/test_classify.py +++ b/tests/cli/test_classify.py @@ -5,6 +5,7 @@ import pytest import typer +from sqlmodel import create_engine cli_app = import_module("review_classification.cli.app") @@ -70,3 +71,35 @@ def test_classify_missing_database_for_org_shows_clean_error( captured = capsys.readouterr() assert "No database found" in captured.err assert "OperationalError" not in captured.err + + +def test_classify_empty_database_for_org_shows_clean_error( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """A leftover empty DB file (no tables) must not yield a silent empty report.""" + import review_classification.sqlite.database as db_module + + monkeypatch.chdir(tmp_path) + db_path = tmp_path / db_module.sqlite_file_name + db_path.touch() # file exists but has no tables, e.g. from a failed run + monkeypatch.setattr(db_module, "engine", create_engine(f"sqlite:///{db_path}")) + + with pytest.raises(typer.Exit) as exc_info: + cli_app.classify( + repo=None, + org=["my-org"], + config=None, + threshold=2.0, + min_samples=30, + output_format="table", + verbose=False, + start=None, + end=None, + exclude_primary_merged=False, + ) + + assert exc_info.value.exit_code == 1 + captured = capsys.readouterr() + assert "No database found" in captured.err diff --git a/tests/sqlite/test_database.py b/tests/sqlite/test_database.py index 66b8dd0..a58675c 100644 --- a/tests/sqlite/test_database.py +++ b/tests/sqlite/test_database.py @@ -99,14 +99,42 @@ def test_get_repos_for_org_handles_missing_table( assert db_module.get_repos_for_org("any-org") == [] -def test_database_exists_reflects_file_presence( +def test_database_is_initialized_false_when_file_missing( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: - """database_exists() tracks whether the SQLite file is on disk.""" - from review_classification.sqlite.database import database_exists, sqlite_file_name + """No database file on disk → not initialized (and no file is created).""" + import review_classification.sqlite.database as db_module + + monkeypatch.chdir(tmp_path) + assert db_module.database_is_initialized() is False + # The check must not lazily create an empty database file. + assert not (tmp_path / db_module.sqlite_file_name).exists() + + +def test_database_is_initialized_false_for_empty_file( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A bare file with no tables (failed earlier run) counts as not initialized.""" + import review_classification.sqlite.database as db_module + + monkeypatch.chdir(tmp_path) + db_path = tmp_path / db_module.sqlite_file_name + db_path.touch() # empty SQLite file, no tables + monkeypatch.setattr(db_module, "engine", create_engine(f"sqlite:///{db_path}")) + + assert db_module.database_is_initialized() is False + + +def test_database_is_initialized_true_with_tables( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A file containing the expected tables counts as initialized.""" + import review_classification.sqlite.database as db_module monkeypatch.chdir(tmp_path) - assert database_exists() is False + db_path = tmp_path / db_module.sqlite_file_name + engine = create_engine(f"sqlite:///{db_path}") + SQLModel.metadata.create_all(engine) + monkeypatch.setattr(db_module, "engine", engine) - (tmp_path / sqlite_file_name).touch() - assert database_exists() is True + assert db_module.database_is_initialized() is True