diff --git a/CHANGELOG.md b/CHANGELOG.md index aa064b2..0261f3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,16 @@ drafts even when they appear in a release. reading arbitrary repository files, so "the producer promises no source text" stopped being a guarantee that could live in one place. +### Fixed + +- **Store schema migration runner** ([#44](https://github.com/trionnemesis/AgentSec/issues/44)). + `SCHEMA_VERSION` moved from 1 to 2 when `run_counter` was added, but the + stored version row was only ever written when absent — a database created + under 1 reported version 1 forever, regardless of its actual tables. Opening + a store now applies pending migrations in order and corrects the stored + version; a version newer than this build supports raises `SchemaVersionError` + rather than being silently opened. + ## [0.2.0] — 2026-08-06 The release that gives AgentSec a first step. In 0.1.0 the entry point was diff --git a/docs/roadmap.md b/docs/roadmap.md index 4d54792..46ded9c 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -31,6 +31,7 @@ agentsec init → agentsec scan → agentsec scan --verify -t → dashboard | Evidence collectors: Wazuh, OTel, tool audit, state diff | ✅ | file backends tested; timeline rebasing for fixtures | | Run provenance (`recorded` / `live` / `mixed`) | ✅ | a fixture-derived `secure` is labelled as such ([#27](https://github.com/trionnemesis/AgentSec/issues/27)) | | SQLite store (runs, findings, audit log) | ✅ | latest-run-per-scenario aggregates | +| Store schema migration runner | ✅ | ordered, idempotent, fails closed on an unrecognised future version ([#44](https://github.com/trionnemesis/AgentSec/issues/44)) | | CLI with meaningful exit codes | ✅ | `0` clean, `1` blocking, `2` could not tell | | Selected-project manifest and discovery | ✅ | `.agentsec/project.yaml`; relative locations only, traversal and symlink escape refused | | Runtime framework fingerprint engine | ✅ | deterministic, read-only detection for LangGraph/LangChain, OpenAI Agents SDK, AutoGen, Semantic Kernel, CrewAI and framework-neutral tool calling; development-agent config stays separate | @@ -58,10 +59,6 @@ agentsec init → agentsec scan → agentsec scan --verify -t → dashboard `environments: [ci, staging]` and `scan --verify` needs a real target. - [ ] Wazuh rule pack for the four original bundled scenarios (`100501`, `100610`, `100720`, `100810`) -- [ ] **Migration runner — overdue.** `SCHEMA_VERSION` is `2`, and - `store/sqlite.py:_init_schema` writes the version row only when absent, so - a database created under version 1 reports version 1 forever and nothing - reads that row to decide anything. --- diff --git a/src/agentsec/store/sqlite.py b/src/agentsec/store/sqlite.py index f4526da..c14e5fd 100644 --- a/src/agentsec/store/sqlite.py +++ b/src/agentsec/store/sqlite.py @@ -19,24 +19,18 @@ from pathlib import Path from typing import Any -from agentsec.errors import FindingNotFound, RunNotFound +from agentsec.errors import AgentSecError, FindingNotFound, RunNotFound from agentsec.models.finding import Finding, FindingStatus from agentsec.models.run import Run SCHEMA_VERSION = 2 +# The version-1 shape: every table that existed before `run_counter`. Applied +# unconditionally and idempotently on every open, including against a legacy +# database, so it must never do more than `CREATE ... IF NOT EXISTS`. _SCHEMA = """ CREATE TABLE IF NOT EXISTS schema_version (version INTEGER NOT NULL); --- One row per day, incremented atomically. Deriving the next run id from --- MAX(run_id) in Python let two processes on one workspace mint the same id, --- and `save_run` upserts, so the second run silently overwrote the first -- --- losing a run without trace, in the component whose job is to be the record. -CREATE TABLE IF NOT EXISTS run_counter ( - day TEXT PRIMARY KEY, - next_n INTEGER NOT NULL -); - CREATE TABLE IF NOT EXISTS runs ( run_id TEXT PRIMARY KEY, scenario_id TEXT NOT NULL, @@ -85,6 +79,53 @@ CREATE INDEX IF NOT EXISTS idx_audit_at ON audit_log (at DESC); """ +# Ordered upgrades applied on top of _SCHEMA, oldest first. Each entry's `sql` +# takes a database already at `version - 1` to `version`; it must be safe to +# re-run (`IF NOT EXISTS` / idempotent DDL only), because a fresh database +# gets every migration applied in sequence rather than starting pre-upgraded. +_MIGRATIONS: list[tuple[int, str]] = [ + ( + 2, + """ + -- One row per day, incremented atomically. Deriving the next run id + -- from MAX(run_id) in Python let two processes on one workspace mint + -- the same id, and `save_run` upserts, so the second run silently + -- overwrote the first -- losing a run without trace, in the + -- component whose job is to be the record. + CREATE TABLE IF NOT EXISTS run_counter ( + day TEXT PRIMARY KEY, + next_n INTEGER NOT NULL + ); + + -- A v1 database predates run_counter, so it may already hold runs for + -- "today" under the old MAX(run_id)-in-Python scheme. Leaving the + -- counter empty would hand out RUN--001 again, and because + -- save_run upserts on run_id, that would silently overwrite the + -- earlier run. Seed each day's counter from the highest suffix + -- already used, so the next claim continues past it. + INSERT INTO run_counter (day, next_n) + SELECT substr(run_id, 5, 8) AS day, + MAX(CAST(substr(run_id, 14, 3) AS INTEGER)) AS next_n + FROM runs + WHERE run_id LIKE 'RUN-________-___' + GROUP BY day + ON CONFLICT(day) DO UPDATE SET + next_n = MAX(run_counter.next_n, excluded.next_n); + """, + ), +] + + +class SchemaVersionError(AgentSecError): + """The database's recorded schema version is newer than this build knows. + + Raised rather than silently proceeding: a store that guessed at an + unrecognised shape could corrupt data or misreport what it read, and + either is worse than refusing to open. + """ + + code = "schema_version_unsupported" + class ResultStore: def __init__(self, path: Path) -> None: @@ -105,10 +146,34 @@ def _conn(self) -> Iterator[sqlite3.Connection]: def _init_schema(self) -> None: with self._conn() as conn: - conn.executescript(_SCHEMA) + # Bootstrap only the version table first, and check it, before + # applying _SCHEMA or any migration: a database from a future + # build may have renamed or dropped a table _SCHEMA still + # expects, and running that script against it could mutate the + # database or raise a raw sqlite3.OperationalError instead of + # the clean refusal this guard promises. + conn.execute("CREATE TABLE IF NOT EXISTS schema_version (version INTEGER NOT NULL)") row = conn.execute("SELECT version FROM schema_version").fetchone() + current = row["version"] if row is not None else 0 + + if current > SCHEMA_VERSION: + raise SchemaVersionError( + f"{self.path} reports schema version {current}, newer than the " + f"{SCHEMA_VERSION} this build supports; refusing to open it", + details={"found_version": current, "supported_version": SCHEMA_VERSION}, + ) + + conn.executescript(_SCHEMA) + + for target_version, migration_sql in _MIGRATIONS: + if current < target_version: + conn.executescript(migration_sql) + current = target_version + if row is None: - conn.execute("INSERT INTO schema_version (version) VALUES (?)", (SCHEMA_VERSION,)) + conn.execute("INSERT INTO schema_version (version) VALUES (?)", (current,)) + elif current != row["version"]: + conn.execute("UPDATE schema_version SET version = ?", (current,)) # -- runs --------------------------------------------------------------- diff --git a/tests/test_store_migrations.py b/tests/test_store_migrations.py new file mode 100644 index 0000000..f01fb52 --- /dev/null +++ b/tests/test_store_migrations.py @@ -0,0 +1,196 @@ +"""Schema migration runner for the SQLite result store. + +`SCHEMA_VERSION` moved from 1 to 2 when `run_counter` was added (#12), but the +version row was only ever written when absent -- so a database created under +1 kept reporting 1 forever, regardless of what its actual tables looked like. +These tests pin the fix (#44): a stale version is upgraded and the row +corrected, migrations are idempotent, and a future version this build does +not recognise is refused rather than silently accepted. +""" + +from __future__ import annotations + +import sqlite3 +from pathlib import Path + +import pytest + +from agentsec.store.sqlite import SCHEMA_VERSION, ResultStore, SchemaVersionError + +#: The tables the version-1 schema shipped with, before `run_counter` existed. +_V1_SCHEMA = """ +CREATE TABLE schema_version (version INTEGER NOT NULL); +INSERT INTO schema_version (version) VALUES (1); + +CREATE TABLE runs ( + run_id TEXT PRIMARY KEY, + scenario_id TEXT NOT NULL, + target_id TEXT NOT NULL, + profile TEXT NOT NULL, + status TEXT NOT NULL, + purple_verdict TEXT, + prevention TEXT, + detection TEXT, + evidence TEXT, + response TEXT, + created_at TEXT NOT NULL, + finished_at TEXT, + scenario_digest TEXT, + payload TEXT NOT NULL +); + +CREATE TABLE findings ( + finding_id TEXT PRIMARY KEY, + scenario_id TEXT NOT NULL, + target_id TEXT NOT NULL, + status TEXT NOT NULL, + severity TEXT NOT NULL, + verdict TEXT NOT NULL, + first_seen_run TEXT NOT NULL, + last_seen_run TEXT NOT NULL, + updated_at TEXT NOT NULL, + payload TEXT NOT NULL +); + +CREATE TABLE audit_log ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + at TEXT NOT NULL, + actor TEXT NOT NULL, + action TEXT NOT NULL, + subject TEXT, + outcome TEXT NOT NULL, + detail TEXT +); +""" + + +def _make_v1_database(path: Path) -> None: + """Build a database in exactly the pre-#12 v1 shape: no `run_counter`.""" + conn = sqlite3.connect(path) + try: + conn.executescript(_V1_SCHEMA) + conn.commit() + finally: + conn.close() + + +def _stored_version(path: Path) -> int: + conn = sqlite3.connect(path) + try: + row = conn.execute("SELECT version FROM schema_version").fetchone() + assert row is not None + return int(row[0]) + finally: + conn.close() + + +def _has_table(path: Path, name: str) -> bool: + conn = sqlite3.connect(path) + try: + row = conn.execute( + "SELECT 1 FROM sqlite_master WHERE type='table' AND name=?", (name,) + ).fetchone() + return row is not None + finally: + conn.close() + + +def test_a_fresh_database_lands_directly_on_the_current_version(tmp_path: Path) -> None: + db_path = tmp_path / "fresh.db" + + ResultStore(db_path) + + assert _stored_version(db_path) == SCHEMA_VERSION + assert _has_table(db_path, "run_counter") + + +def test_a_v1_database_is_upgraded_to_v2_on_open(tmp_path: Path) -> None: + db_path = tmp_path / "legacy.db" + _make_v1_database(db_path) + assert not _has_table(db_path, "run_counter") + assert _stored_version(db_path) == 1 + + ResultStore(db_path) + + assert _stored_version(db_path) == SCHEMA_VERSION + assert _has_table(db_path, "run_counter") + + +def test_reopening_an_already_migrated_database_is_a_no_op(tmp_path: Path) -> None: + db_path = tmp_path / "legacy.db" + _make_v1_database(db_path) + + ResultStore(db_path) + assert _stored_version(db_path) == SCHEMA_VERSION + + # Second open must not fail, duplicate the migration, or move the version + # again -- CREATE TABLE without IF NOT EXISTS would raise here if the + # migration re-ran instead of recognising it already applied. + ResultStore(db_path) + assert _stored_version(db_path) == SCHEMA_VERSION + + +def _insert_run_row(path: Path, run_id: str) -> None: + """Insert a minimal `runs` row directly, bypassing `next_run_id`. + + Mirrors what a real v1 database looks like: rows already exist under the + old MAX(run_id)-in-Python id scheme, with no `run_counter` involved. + """ + conn = sqlite3.connect(path) + try: + conn.execute( + """ + INSERT INTO runs (run_id, scenario_id, target_id, profile, status, + created_at, payload) + VALUES (?, 'AGT-XPIA-001', 'demo-agent-fixture', 'pr', 'completed', + '2026-08-11T00:00:00+00:00', '{}') + """, + (run_id,), + ) + conn.commit() + finally: + conn.close() + + +def test_migrating_a_v1_database_with_existing_runs_seeds_the_counter_past_them( + tmp_path: Path, +) -> None: + """A v1 database may already hold runs for "today" under the old + MAX(run_id)-in-Python scheme. An empty `run_counter` after migration + would hand out `RUN--001` again, and because `save_run` upserts on + `run_id`, that would silently overwrite the pre-existing run (#44, + review comment on #46). The migration must seed each day's counter past + the highest suffix already in use. + """ + db_path = tmp_path / "legacy_with_runs.db" + _make_v1_database(db_path) + _insert_run_row(db_path, "RUN-20260811-001") + _insert_run_row(db_path, "RUN-20260811-002") + _insert_run_row(db_path, "RUN-20260810-005") # a different day, lower number + + store = ResultStore(db_path) + + assert store.next_run_id("20260811") == "RUN-20260811-003" + assert store.next_run_id("20260810") == "RUN-20260810-006" + # A day with no prior runs still starts from scratch. + assert store.next_run_id("20260101") == "RUN-20260101-001" + + +def test_a_future_schema_version_is_refused_rather_than_silently_opened( + tmp_path: Path, +) -> None: + db_path = tmp_path / "from_the_future.db" + _make_v1_database(db_path) + conn = sqlite3.connect(db_path) + try: + conn.execute("UPDATE schema_version SET version = ?", (SCHEMA_VERSION + 1,)) + conn.commit() + finally: + conn.close() + + with pytest.raises(SchemaVersionError): + ResultStore(db_path) + + # Refusal must not have rewritten the version row to make the problem + # disappear on the next attempt. + assert _stored_version(db_path) == SCHEMA_VERSION + 1