From e8e4f6f7c70b62051188b2f39421fd114fb2955c Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 2 Jul 2026 03:27:02 +0000 Subject: [PATCH] =?UTF-8?q?refactor:=20address=20PR=20review=20=E2=80=94?= =?UTF-8?q?=20rename,=20move=20helpers,=20add=20migration=20runner?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review #320 comments: - Rename _sv() → get_supervise_mod() in both store files (review 206/211) - Move _audit_entry_from_row onto AuditStore as _row_to_entry static method (review 208); move _proposal/_response_from_row onto QueueStore (review 211) - Remove _host_db_path() free function; inline into __init__ (review 209/211) - Add stdlib migration runner using a shared schema_versions table; each store tracks its own version under a module key so they can coexist in the same DB without clobbering a shared PRAGMA user_version (reviews 210/212/213) - PRD: add goal 6 (migration runner), narrow non-goal to third-party ORM only --- bot_bottle/audit_store.py | 75 ++++++++----- bot_bottle/queue_store.py | 126 +++++++++++++--------- docs/prds/prd-new-sqlite-local-storage.md | 5 +- 3 files changed, 126 insertions(+), 80 deletions(-) diff --git a/bot_bottle/audit_store.py b/bot_bottle/audit_store.py index 2682217..9d5518d 100644 --- a/bot_bottle/audit_store.py +++ b/bot_bottle/audit_store.py @@ -10,7 +10,7 @@ if TYPE_CHECKING: from .supervise import AuditEntry -def _sv() -> object: +def get_supervise_mod() -> object: """Lazy import of supervise to avoid a circular-import at module init time. Mirrors our own module identity so patches on supervise.bot_bottle_root propagate correctly in both flat (sidecar / sys.path-injection tests) and @@ -26,28 +26,31 @@ def _sv() -> object: return _m -def _audit_entry_from_row(row: sqlite3.Row) -> AuditEntry: - m = _sv() - return m.AuditEntry( # type: ignore[attr-defined] - timestamp=row["timestamp"], - bottle_slug=row["bottle_slug"], - component=row["component"], - operator_action=row["operator_action"], - operator_notes=row["operator_notes"], - justification=row["justification"], - diff=row["diff"], +# One entry per schema version: _MIGRATIONS[0] brings a fresh DB (user_version=0) +# to version 1, _MIGRATIONS[1] to version 2, and so on. Add new migrations at +# the end; never edit existing ones. +_MIGRATIONS: list[str] = [ + # v1 — initial schema + """ + CREATE TABLE IF NOT EXISTS supervise_audit_entries ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + timestamp TEXT NOT NULL, + bottle_slug TEXT NOT NULL, + component TEXT NOT NULL, + operator_action TEXT NOT NULL, + operator_notes TEXT NOT NULL, + justification TEXT NOT NULL, + diff TEXT NOT NULL ) - - -def _host_db_path() -> Path: - return _sv().host_db_path() # type: ignore[attr-defined,no-any-return] + """, +] class AuditStore: """SQLite-backed persistent store for supervise audit entries.""" def __init__(self, db_path: Path | None = None) -> None: - self.db_path = db_path or _host_db_path() + self.db_path = db_path or get_supervise_mod().host_db_path() # type: ignore[attr-defined] self.db_path.parent.mkdir(parents=True, exist_ok=True) self._init() @@ -85,29 +88,49 @@ class AuditStore: """, (component, slug), ).fetchall() - return [_audit_entry_from_row(row) for row in rows] + return [self._row_to_entry(row) for row in rows] + + @staticmethod + def _row_to_entry(row: sqlite3.Row) -> AuditEntry: + m = get_supervise_mod() + return m.AuditEntry( # type: ignore[attr-defined] + timestamp=row["timestamp"], + bottle_slug=row["bottle_slug"], + component=row["component"], + operator_action=row["operator_action"], + operator_notes=row["operator_notes"], + justification=row["justification"], + diff=row["diff"], + ) def _connect(self) -> sqlite3.Connection: conn = sqlite3.connect(self.db_path) conn.row_factory = sqlite3.Row return conn + _SCHEMA_KEY = "audit_store" + def _init(self) -> None: with self._connect() as conn: conn.execute( """ - CREATE TABLE IF NOT EXISTS supervise_audit_entries ( - id INTEGER PRIMARY KEY AUTOINCREMENT, - timestamp TEXT NOT NULL, - bottle_slug TEXT NOT NULL, - component TEXT NOT NULL, - operator_action TEXT NOT NULL, - operator_notes TEXT NOT NULL, - justification TEXT NOT NULL, - diff TEXT NOT NULL + CREATE TABLE IF NOT EXISTS schema_versions ( + module TEXT PRIMARY KEY, + version INTEGER NOT NULL DEFAULT 0 ) """ ) + row = conn.execute( + "SELECT version FROM schema_versions WHERE module = ?", + (self._SCHEMA_KEY,), + ).fetchone() + version = row[0] if row else 0 + for i, sql in enumerate(_MIGRATIONS[version:], start=version + 1): + conn.execute(sql) + conn.execute( + "INSERT OR REPLACE INTO schema_versions (module, version) VALUES (?, ?)", + (self._SCHEMA_KEY, i), + ) self._chmod() def _chmod(self) -> None: diff --git a/bot_bottle/queue_store.py b/bot_bottle/queue_store.py index 1ede262..6ef5dcb 100644 --- a/bot_bottle/queue_store.py +++ b/bot_bottle/queue_store.py @@ -11,7 +11,7 @@ if TYPE_CHECKING: from .supervise import Proposal, Response -def _sv() -> object: +def get_supervise_mod() -> object: """Lazy import of supervise to avoid a circular-import at module init time. By the time any QueueStore method is called, both modules are fully loaded. @@ -30,31 +30,38 @@ def _sv() -> object: return _m -def _proposal_from_row(row: sqlite3.Row) -> Proposal: - m = _sv() - return m.Proposal( # type: ignore[attr-defined] - id=row["id"], - bottle_slug=row["bottle_slug"], - tool=row["tool"], - proposed_file=row["proposed_file"], - justification=row["justification"], - arrival_timestamp=row["arrival_timestamp"], - current_file_hash=row["current_file_hash"], +# One entry per schema version: _MIGRATIONS[0] brings a fresh DB (user_version=0) +# to version 1, _MIGRATIONS[1] to version 2, and so on. Add new migrations at +# the end; never edit existing ones. +_MIGRATIONS: list[str] = [ + # v1 — proposals table + """ + CREATE TABLE IF NOT EXISTS supervise_proposals ( + queue_key TEXT NOT NULL, + id TEXT NOT NULL, + bottle_slug TEXT NOT NULL, + tool TEXT NOT NULL, + proposed_file TEXT NOT NULL, + justification TEXT NOT NULL, + arrival_timestamp TEXT NOT NULL, + current_file_hash TEXT NOT NULL, + archived INTEGER NOT NULL DEFAULT 0, + PRIMARY KEY (queue_key, id) ) - - -def _response_from_row(row: sqlite3.Row) -> Response: - m = _sv() - return m.Response( # type: ignore[attr-defined] - proposal_id=row["proposal_id"], - status=row["status"], - notes=row["notes"], - final_file=row["final_file"], + """, + # v2 — responses table + """ + CREATE TABLE IF NOT EXISTS supervise_responses ( + queue_key TEXT NOT NULL, + proposal_id TEXT NOT NULL, + status TEXT NOT NULL, + notes TEXT NOT NULL, + final_file TEXT, + archived INTEGER NOT NULL DEFAULT 0, + PRIMARY KEY (queue_key, proposal_id) ) - - -def _host_db_path() -> Path: - return _sv().host_db_path() # type: ignore[attr-defined,no-any-return] + """, +] class QueueStore: @@ -69,7 +76,7 @@ class QueueStore: # bind-mounted host DB. On the host this env var is never set, # so we always fall through to host_db_path(). env_path = os.environ.get("SUPERVISE_DB_PATH", "").strip() - self.db_path = Path(env_path) if env_path else _host_db_path() + self.db_path = Path(env_path) if env_path else get_supervise_mod().host_db_path() # type: ignore[attr-defined] self.db_path.parent.mkdir(parents=True, exist_ok=True) self._init() @@ -107,7 +114,7 @@ class QueueStore: ).fetchone() if row is None: raise FileNotFoundError(proposal_id) - return _proposal_from_row(row) + return self._row_to_proposal(row) def list_pending_proposals(self) -> list[Proposal]: if not self.db_path.is_file(): @@ -128,7 +135,7 @@ class QueueStore: """, (self.queue_key,), ).fetchall() - return [_proposal_from_row(row) for row in rows] + return [self._row_to_proposal(row) for row in rows] def list_all_pending_proposals(self) -> list[Proposal]: if not self.db_path.is_file(): @@ -147,7 +154,7 @@ class QueueStore: ORDER BY p.arrival_timestamp, p.id """ ).fetchall() - return [_proposal_from_row(row) for row in rows] + return [self._row_to_proposal(row) for row in rows] def write_response(self, response: Response) -> Path: with self._connect() as conn: @@ -179,7 +186,7 @@ class QueueStore: ).fetchone() if row is None: raise FileNotFoundError(proposal_id) - return _response_from_row(row) + return self._row_to_response(row) def archive_proposal(self, proposal_id: str) -> None: if not self.db_path.is_file(): @@ -200,42 +207,57 @@ class QueueStore: (self.queue_key, proposal_id), ) + @staticmethod + def _row_to_proposal(row: sqlite3.Row) -> Proposal: + m = get_supervise_mod() + return m.Proposal( # type: ignore[attr-defined] + id=row["id"], + bottle_slug=row["bottle_slug"], + tool=row["tool"], + proposed_file=row["proposed_file"], + justification=row["justification"], + arrival_timestamp=row["arrival_timestamp"], + current_file_hash=row["current_file_hash"], + ) + + @staticmethod + def _row_to_response(row: sqlite3.Row) -> Response: + m = get_supervise_mod() + return m.Response( # type: ignore[attr-defined] + proposal_id=row["proposal_id"], + status=row["status"], + notes=row["notes"], + final_file=row["final_file"], + ) + def _connect(self) -> sqlite3.Connection: conn = sqlite3.connect(self.db_path) conn.row_factory = sqlite3.Row return conn + _SCHEMA_KEY = "queue_store" + def _init(self) -> None: with self._connect() as conn: conn.execute( """ - CREATE TABLE IF NOT EXISTS supervise_proposals ( - queue_key TEXT NOT NULL, - id TEXT NOT NULL, - bottle_slug TEXT NOT NULL, - tool TEXT NOT NULL, - proposed_file TEXT NOT NULL, - justification TEXT NOT NULL, - arrival_timestamp TEXT NOT NULL, - current_file_hash TEXT NOT NULL, - archived INTEGER NOT NULL DEFAULT 0, - PRIMARY KEY (queue_key, id) + CREATE TABLE IF NOT EXISTS schema_versions ( + module TEXT PRIMARY KEY, + version INTEGER NOT NULL DEFAULT 0 ) """ ) - conn.execute( - """ - CREATE TABLE IF NOT EXISTS supervise_responses ( - queue_key TEXT NOT NULL, - proposal_id TEXT NOT NULL, - status TEXT NOT NULL, - notes TEXT NOT NULL, - final_file TEXT, - archived INTEGER NOT NULL DEFAULT 0, - PRIMARY KEY (queue_key, proposal_id) + row = conn.execute( + "SELECT version FROM schema_versions WHERE module = ?", + (self._SCHEMA_KEY,), + ).fetchone() + version = row[0] if row else 0 + for i, sql in enumerate(_MIGRATIONS[version:], start=version + 1): + conn.execute(sql) + conn.execute( + "INSERT OR REPLACE INTO schema_versions (module, version) VALUES (?, ?)", + (self._SCHEMA_KEY, i), ) - """ - ) self._chmod() def _chmod(self) -> None: diff --git a/docs/prds/prd-new-sqlite-local-storage.md b/docs/prds/prd-new-sqlite-local-storage.md index df5981e..1055a8a 100644 --- a/docs/prds/prd-new-sqlite-local-storage.md +++ b/docs/prds/prd-new-sqlite-local-storage.md @@ -32,7 +32,8 @@ one-off persistence. 4. The sidecar receives the host database mount across docker, smolmachines, and macOS-container backends. 5. The implementation stays stdlib-only. -6. Unit tests cover queue round-trips, pending discovery, response waits, +6. Schema migrations use a `PRAGMA user_version` runner — no third-party deps. +7. Unit tests cover queue round-trips, pending discovery, response waits, archive semantics, audit round-trips, and path creation. ## Non-goals @@ -41,7 +42,7 @@ one-off persistence. - Adding forge orchestration state tables. - Adding egress metering or budget tables. - Changing the supervise TUI workflow or remediation behavior. -- Introducing a third-party ORM or migration framework. +- Introducing a third-party ORM or migration library. ## Design