diff --git a/.github/workflows/doc-gate.yml b/.github/workflows/doc-gate.yml index 0ad27c28b..7ddbca3d9 100644 --- a/.github/workflows/doc-gate.yml +++ b/.github/workflows/doc-gate.yml @@ -37,6 +37,9 @@ jobs: - name: Retrofit-migration guard (#2188) run: python scripts/check_retrofit_migrations.py + - name: Schema-column guard (tsk-hrzgip) + run: python scripts/check_schema_column_migrations.py + - name: Diff gate (Layer B) env: BASE_REF: ${{ github.base_ref }} diff --git a/changelog.d/tsk-hrzgip-schema-column-guard.md b/changelog.d/tsk-hrzgip-schema-column-guard.md new file mode 100644 index 000000000..13bf18129 --- /dev/null +++ b/changelog.d/tsk-hrzgip-schema-column-guard.md @@ -0,0 +1,2 @@ +### Added +- New `schema-column-guard` static check (`scripts/check_schema_column_migrations.py`) plus matching doc-gate step; flags any column added to a `CREATE TABLE` inside a store's `SCHEMA` with no matching `ALTER TABLE ... ADD COLUMN` in the same file, including the previously-uncovered case of zero migration at all (proven on PR #2416). \ No newline at end of file diff --git a/docs/contributor-pitfalls.md b/docs/contributor-pitfalls.md index b65b265cc..d925673a9 100644 --- a/docs/contributor-pitfalls.md +++ b/docs/contributor-pitfalls.md @@ -119,6 +119,16 @@ upgraded DBs lose the feature at runtime. Use the guarded `_post_init` pattern builds the PRE-change schema first. Fresh-DB tests are structurally blind to this class (#2043: `peer_fingerprint`). +**19. Adding a column straight into SCHEMA bricks every existing install.** +`CREATE TABLE IF NOT EXISTS` is a no-op on existing tables, so a new column +slotted into the `CREATE TABLE` body is silently absent on upgrade; the first +INSERT that touches it crashes with `table has no column named `. The +two existing migration guards cannot see this because neither has a migration +entry to inspect (both clean by vacuity), and CI cannot see it because tests +build fresh databases. The fix is the same guarded `_post_init` pattern as +pitfall 18; the check is `scripts/check_schema_column_migrations.py` (added +tsk-hrzgip after PR #2416 proved both old guards passed on this exact brick). + ## Process **13. A fix and its test belong in one PR.** diff --git a/scripts/check_schema_column_migrations.py b/scripts/check_schema_column_migrations.py new file mode 100644 index 000000000..46d6c9f3d --- /dev/null +++ b/scripts/check_schema_column_migrations.py @@ -0,0 +1,249 @@ +#!/usr/bin/env python3 +"""Static guard for SCHEMA-only column adds with no migration (tsk-hrzgip). + +``BaseStore.init()`` runs a store's ``SCHEMA`` string (CREATE TABLE + CREATE +INDEX) at boot. ``CREATE TABLE IF NOT EXISTS`` is a no-op on existing +databases, so a new column added straight into the ``CREATE TABLE`` body is +silently absent on upgrade. The first INSERT or SELECT that touches it then +crashes with ``table has no column named `` on every existing install. +This is the brick that the two existing migration guards cannot see, because +neither has a migration entry to inspect (taOS PR #2416 proved both clean on +exactly this case). + +The mandated pattern is a guarded ``_post_init`` coroutine: ``PRAGMA +table_info`` check + ``ALTER TABLE ADD COLUMN `` only when absent. + +This script statically inspects every Python file under ``tinyagentos/`` and +flags, for every CREATE TABLE in a SCHEMA string: a column that + + (a) is declared in that CREATE TABLE in the CURRENT file, AND + (b) has NO matching ``ALTER TABLE ADD [COLUMN] `` anywhere in the + SAME file (no _post_init migration runs it), AND + (c) was NOT in the CREATE TABLE column list on ``origin/dev`` (it is newly + added by this change -- so existing columns that have always lived in + SCHEMA do not trip the guard). + +The third condition is what keeps it from flagging every column in the +repo: comparison is done against ``git show origin/dev:`` for the same +file, only NEW columns relative to that snapshot count. + +CI does NOT catch it because tests build fresh databases (which always have +the column); this guard exists to fill that gap. + +Usage: + python scripts/check_schema_column_migrations.py +Invoked by the ``schema-column-guard`` step in +``.github/workflows/doc-gate.yml``. + +Prints ``schema-column-guard: clean`` and exits 0 when no violations, or +prints each violation and exits 1. + +Dependency-light: stdlib only (ast + re + pathlib + subprocess). +""" +from __future__ import annotations + +import re +import subprocess +import sys +from dataclasses import dataclass +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +STORES_ROOT = REPO_ROOT / "tinyagentos" + +# Tables that are not "user data" stores and therefore not subject to this +# guard. The migration runner's bookkeeping table is created on first boot +# before any store init runs, so columns added to it on origin/dev are +# genuinely new for the very DBs that need them and would generate noise. +_EXCLUDED_TABLES = frozenset({"schema_migrations"}) + +# CREATE TABLE [IF NOT EXISTS] ( ... ) +_CREATE_TABLE_RE = re.compile( + r"CREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?(\w+)\s*\((.*?)\)\s*;", + re.IGNORECASE | re.DOTALL, +) + +# ALTER TABLE
ADD [COLUMN] ... +_ADD_COLUMN_RE = re.compile( + r"ALTER\s+TABLE\s+(\w+)\s+ADD\s+(?:COLUMN\s+)?(\w+)", + re.IGNORECASE, +) + + +@dataclass +class Violation: + path: Path + table: str + column: str + detail: str + + def __str__(self) -> str: + fix = ( + "add a guarded _post_init coroutine that ALTERs this column " + "into place after a PRAGMA table_info check" + ) + return ( + f"{self.path}: table '{self.table}', column '{self.column}' " + f"added to SCHEMA with no migration\n" + f" detail: {self.detail}\n" + f" fix: {fix}" + ) + + +def _split_columns(body: str) -> set[str]: + """Extract declared column names from a CREATE TABLE body. + + Splits on commas that are not inside parentheses (so function/default + expressions and inline CHECK(...) are handled), then keeps the first + whitespace-delimited token of each segment as the column name (unless + that segment is an inline table-level constraint, which does not add + a column). + """ + _INLINE_CONSTRAINT_RE = re.compile( + r"^\s*(?:CONSTRAINT\s+\w+\s+)?(?:PRIMARY\s+KEY|UNIQUE|CHECK|FOREIGN\s+KEY|REFERENCES)\b", + re.IGNORECASE, + ) + _COLUMN_NAME_RE = re.compile(r"^\s*(\w+)\s+", re.IGNORECASE) + _NON_COLUMN_KEYWORDS = { + "create", "table", "primary", "key", "unique", "check", "foreign", + "references", "constraint", "default", "not", "null", "integer", + "text", "real", "blob", "numeric", "autoincrement", "if", "exists", + "select", "on", "and", "or", "as", "collate", "generated", "always", + } + + columns: set[str] = set() + depth = 0 + segments: list[str] = [] + current: list[str] = [] + for ch in body: + if ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + if ch == "," and depth == 0: + segments.append("".join(current)) + current = [] + else: + current.append(ch) + segments.append("".join(current)) + + for seg in segments: + if _INLINE_CONSTRAINT_RE.match(seg): + continue + m = _COLUMN_NAME_RE.match(seg) + if not m: + continue + name = m.group(1) + if name.lower() in _NON_COLUMN_KEYWORDS: + continue + columns.add(name) + return columns + + +def _origin_dev_columns(path: Path) -> dict[str, set[str]] | None: + """Return {table: columns} for CREATE TABLE bodies in the file on + ``origin/dev``. Returns None if the file is missing on origin/dev (new + file -- treat every column as new so violations surface naturally).""" + rel = path.relative_to(REPO_ROOT) + try: + result = subprocess.run( + ["git", "show", f"origin/dev:{rel.as_posix()}"], + cwd=REPO_ROOT, + capture_output=True, + text=True, + check=False, + ) + except (OSError, subprocess.SubprocessError): + return None + if result.returncode != 0: + return None + out: dict[str, set[str]] = {} + for tm in _CREATE_TABLE_RE.finditer(result.stdout): + out[tm.group(1)] = _split_columns(tm.group(2)) + return out + + +def _extract_schemas(source: str) -> list[str]: + """Find SCHEMA string constants in a Python source. + + Only triple-quoted strings are scanned: the existing migration guards + already require multi-line SQL blocks (CREATE TABLE + CREATE INDEX), + so single-line string literals in source code (which would otherwise + double-count a block already captured by the triple-quoted regex) are + ignored. Files that don't define a SCHEMA contribute nothing. + """ + out: list[str] = [] + seen: set[int] = set() + for m in re.finditer(r'"""(.*?)"""', source, re.DOTALL): + if "CREATE TABLE" in m.group(1): + out.append(m.group(1)) + seen.add(m.start()) + for m in re.finditer(r"'''(.*?)'''", source, re.DOTALL): + if "CREATE TABLE" in m.group(1) and m.start() not in seen: + out.append(m.group(1)) + return out + + +def find_violations(path: Path) -> list[Violation]: + """Run the static check against a single Python file. Returns violations.""" + try: + source = path.read_text(encoding="utf-8", errors="ignore") + except OSError: + return [] + + schemas = _extract_schemas(source) + if not schemas: + return [] + + origin_cols = _origin_dev_columns(path) + + added_columns: set[tuple[str, str]] = set() + for m in _ADD_COLUMN_RE.finditer(source): + added_columns.add((m.group(1), m.group(2))) + + violations: list[Violation] = [] + for schema in schemas: + for tm in _CREATE_TABLE_RE.finditer(schema): + table = tm.group(1) + if table in _EXCLUDED_TABLES: + continue + current_cols = _split_columns(tm.group(2)) + baseline_cols = ( + origin_cols.get(table, set()) if origin_cols is not None else set() + ) + new_cols = current_cols - baseline_cols + for col in sorted(new_cols): + if (table, col) not in added_columns: + violations.append(Violation( + path=path, + table=table, + column=col, + detail=f"new column '{col}' in CREATE TABLE {table} with no ALTER TABLE {table} ADD COLUMN {col} in this file", + )) + return violations + + +def find_all_violations(root: Path = STORES_ROOT) -> list[Violation]: + """Walk every Python file under the stores root and collect violations.""" + violations: list[Violation] = [] + if not root.is_dir(): + return violations + for py_file in sorted(root.rglob("*.py")): + violations.extend(find_violations(py_file)) + return violations + + +def main(argv: list[str] | None = None) -> int: + args = argv if argv is not None else sys.argv[1:] + root = Path(args[0]) if args else STORES_ROOT + violations = find_all_violations(root) + if not violations: + print("schema-column-guard: clean") + return 0 + for v in violations: + print(f"SCHEMA-COLUMN VIOLATION: {v}") + return 1 + + +if __name__ == "__main__": + sys.exit(main()) \ No newline at end of file diff --git a/tests/scripts/test_check_schema_column_migrations.py b/tests/scripts/test_check_schema_column_migrations.py new file mode 100644 index 000000000..2c95c9167 --- /dev/null +++ b/tests/scripts/test_check_schema_column_migrations.py @@ -0,0 +1,225 @@ +"""Tests for scripts/check_schema_column_migrations.py.""" +from __future__ import annotations + +import importlib.util +from pathlib import Path + +import pytest + +_SCRIPT = Path(__file__).resolve().parent.parent.parent / "scripts" / "check_schema_column_migrations.py" + + +def _load_module(): + import sys + spec = importlib.util.spec_from_file_location( + "check_schema_column_migrations", _SCRIPT + ) + mod = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = mod + spec.loader.exec_module(mod) + return mod + + +@pytest.fixture(scope="module") +def guard_mod(): + return _load_module() + + +def _write_store(tmp_path: Path, name: str, body: str) -> Path: + """Write a synthetic store file under tmp_path/. Returns the path.""" + p = tmp_path / name + p.write_text(body) + return p + + +# Module-level fixtures used by TestMain. Each test builds its own root so +# the passing and failing fixtures stay independent. +@pytest.fixture +def passing_root(tmp_path: Path) -> Path: + """A stores root with one store whose columns are all pre-existing.""" + root = tmp_path / "stores_passing" + root.mkdir() + _write_store( + root, + "good_store.py", + ''' +"""All columns already exist on origin/dev, no new ones -> clean.""" +SCHEMA = """ +CREATE TABLE IF NOT EXISTS widgets ( + id TEXT PRIMARY KEY, + name TEXT NOT NULL DEFAULT '' +); +""" +''', + ) + return root + + +@pytest.fixture +def failing_root(tmp_path: Path) -> Path: + """A stores root with one store that adds SCHEMA columns with no ALTER.""" + root = tmp_path / "stores_failing" + root.mkdir() + _write_store( + root, + "bad_store.py", + ''' +"""Adds two new columns directly into CREATE TABLE with no ALTER -> red.""" +SCHEMA = """ +CREATE TABLE IF NOT EXISTS gadgets ( + id TEXT PRIMARY KEY, + kind TEXT NOT NULL DEFAULT '', + purpose TEXT NOT NULL DEFAULT '' +); +""" +''', + ) + return root + + +# Baseline shape per file (table -> columns) used to simulate +# `git show origin/dev:` so the guard does not shell out. +_BASELINES = { + "good_store.py": {"widgets": {"id", "name"}}, + "bad_store.py": {"gadgets": {"id"}}, +} + + +class TestSplitColumns: + def test_extracts_simple_columns(self, guard_mod) -> None: + body = "id TEXT PRIMARY KEY,\n name TEXT NOT NULL DEFAULT ''\n" + assert guard_mod._split_columns(body) == {"id", "name"} + + def test_skips_inline_constraints(self, guard_mod) -> None: + body = ( + "id INTEGER PRIMARY KEY,\n" + "name TEXT,\n" + "UNIQUE (name),\n" + "CHECK (id > 0)\n" + ) + assert guard_mod._split_columns(body) == {"id", "name"} + + def test_handles_commas_inside_parentheses(self, guard_mod) -> None: + body = ( + "id INTEGER PRIMARY KEY,\n" + "name TEXT NOT NULL DEFAULT func(a, b, c)\n" + ) + assert guard_mod._split_columns(body) == {"id", "name"} + + +class TestExtractSchemas: + def test_finds_triple_double_quoted_block(self, guard_mod) -> None: + src = 'SCHEMA = """\nCREATE TABLE foo (id TEXT);\n"""' + schemas = guard_mod._extract_schemas(src) + assert any("CREATE TABLE foo" in s for s in schemas) + + def test_finds_triple_single_quoted_block(self, guard_mod) -> None: + src = "SCHEMA = '''\nCREATE TABLE foo (id TEXT);\n'''" + schemas = guard_mod._extract_schemas(src) + assert any("CREATE TABLE foo" in s for s in schemas) + + def test_ignores_non_schema_strings(self, guard_mod) -> None: + src = 'NAME = "CREATE TABLE nope (id TEXT);"' + assert guard_mod._extract_schemas(src) == [] + + +class TestFindViolations: + def _patched_baselines(self, guard_mod, monkeypatch, baselines: dict) -> None: + """Force _origin_dev_columns to return a fixed baseline per file.""" + def fake(path: Path): + return baselines.get(path.name, {}) + monkeypatch.setattr(guard_mod, "_origin_dev_columns", fake) + + def test_passing_fixture_no_violations( + self, guard_mod, tmp_path: Path, monkeypatch + ) -> None: + body = ''' +SCHEMA = """ +CREATE TABLE IF NOT EXISTS widgets ( + id TEXT PRIMARY KEY, + name TEXT NOT NULL DEFAULT '' +); +""" +''' + path = _write_store(tmp_path, "ok.py", body) + self._patched_baselines(guard_mod, monkeypatch, {"ok.py": {"widgets": {"id", "name"}}}) + assert guard_mod.find_violations(path) == [] + + def test_failing_fixture_emits_one_violation_per_new_column( + self, guard_mod, tmp_path: Path, monkeypatch + ) -> None: + body = ''' +SCHEMA = """ +CREATE TABLE IF NOT EXISTS gadgets ( + id TEXT PRIMARY KEY, + kind TEXT NOT NULL DEFAULT '', + purpose TEXT NOT NULL DEFAULT '' +); +""" +''' + path = _write_store(tmp_path, "broken.py", body) + self._patched_baselines(guard_mod, monkeypatch, {"broken.py": {"gadgets": {"id"}}}) + violations = guard_mod.find_violations(path) + assert {v.column for v in violations} == {"kind", "purpose"} + assert all(v.table == "gadgets" for v in violations) + + def test_alter_for_new_column_silences_violation( + self, guard_mod, tmp_path: Path, monkeypatch + ) -> None: + body = ''' +SCHEMA = """ +CREATE TABLE IF NOT EXISTS gadgets ( + id TEXT PRIMARY KEY, + kind TEXT NOT NULL DEFAULT '' +); +""" + +async def _post_init(self): + if not await self._has_column("gadgets", "kind"): + await self._db.execute("ALTER TABLE gadgets ADD COLUMN kind TEXT NOT NULL DEFAULT ''") +''' + path = _write_store(tmp_path, "migrated.py", body) + self._patched_baselines(guard_mod, monkeypatch, {"migrated.py": {"gadgets": {"id"}}}) + assert guard_mod.find_violations(path) == [] + + def test_pre_existing_column_with_alter_still_clean( + self, guard_mod, tmp_path: Path, monkeypatch + ) -> None: + body = ''' +SCHEMA = """ +CREATE TABLE IF NOT EXISTS widgets ( + id TEXT PRIMARY KEY, + name TEXT NOT NULL DEFAULT '' +); +""" +''' + path = _write_store(tmp_path, "stable.py", body) + # Baseline already has both columns -> nothing is "new". + self._patched_baselines( + guard_mod, monkeypatch, {"stable.py": {"widgets": {"id", "name"}}} + ) + assert guard_mod.find_violations(path) == [] + + +class TestMain: + def test_passing_fixture_exits_zero( + self, guard_mod, passing_root: Path, monkeypatch, capsys: pytest.CaptureFixture + ) -> None: + monkeypatch.setattr(guard_mod, "_origin_dev_columns", lambda p: _BASELINES.get(p.name, {})) + rc = guard_mod.main([str(passing_root)]) + out = capsys.readouterr().out + assert rc == 0 + assert "schema-column-guard: clean" in out + + def test_failing_fixture_exits_one( + self, guard_mod, failing_root: Path, monkeypatch, capsys: pytest.CaptureFixture + ) -> None: + """The failing fixture is the red-side of the contract: a store that + adds SCHEMA columns with no ALTER must drive main() to exit 1.""" + monkeypatch.setattr(guard_mod, "_origin_dev_columns", lambda p: _BASELINES.get(p.name, {})) + rc = guard_mod.main([str(failing_root)]) + out = capsys.readouterr().out + assert rc == 1 + assert "SCHEMA-COLUMN VIOLATION" in out + assert "kind" in out + assert "purpose" in out \ No newline at end of file