From 303e12abc7a8464c287612d67f346d9c1246dfcd Mon Sep 17 00:00:00 2001 From: IceRhymers Date: Thu, 23 Jul 2026 13:08:50 -0700 Subject: [PATCH 1/4] db: raw reference-edge schema with lifecycle-safe grants (#83) Adds the reference_edges table (migration 0005) for raw, unresolved call/import edges extracted per file. Deliberately no FK to symbols -- resolution to a concrete symbol happens at query time by name-join in a later child of epic #82. FKs to repos/files only, both ON DELETE CASCADE, so the existing sweep/reconcile cascade paths in indexer/store.py cover the new table without any behavior change; their docstrings are updated to say so truthfully. Grant builders in app/db/grants.py are schema-wide and need no code change to cover the new table. --- app/alembic/versions/0005_reference_edges.py | 101 +++++++++++++++++++ app/db/models.py | 57 ++++++++++- indexer/store.py | 16 +-- 3 files changed, 164 insertions(+), 10 deletions(-) create mode 100644 app/alembic/versions/0005_reference_edges.py diff --git a/app/alembic/versions/0005_reference_edges.py b/app/alembic/versions/0005_reference_edges.py new file mode 100644 index 0000000..0d0b756 --- /dev/null +++ b/app/alembic/versions/0005_reference_edges.py @@ -0,0 +1,101 @@ +"""reference edges (raw, unresolved call/import edges) + +Revision ID: 0005 +Revises: 0004 +Create Date: 2026-07-23 00:00:00.000000 + +Adds ``reference_edges``: one row per raw (unresolved) call/import site found by +the extractor (epic #82). Deliberately NO FK to ``symbols`` -- symbol ids churn +on every per-file delete-and-reinsert, and an FK would couple the two rewrite +orders inside the indexing transaction for no query benefit. Resolution from +``target_name`` to a concrete symbol happens at query time by name-join (a +later child of #82), not here. The enclosing symbol is denormalized onto the +row instead (``enclosing_*``, all nullable -- NULL means module/top-level +scope). No ``branches`` column: branch membership rides ``files.branches`` at +query time, exactly as ``symbols`` does. + +``pg_trgm`` already exists (created by 0001, database-wide) so no +``CREATE EXTENSION`` is needed here; extension-before-index ordering is +satisfied by the chain itself. + +**Grant coupling (not schema-only for an already-deployed target):** the +schema-wide grant builders in ``app/db/grants.py`` (``GRANT ... ON ALL +TABLES IN SCHEMA`` + ``ALTER DEFAULT PRIVILEGES``) cover this new table +automatically ONLY when the same identity that ran the original grants also +runs this migration (Postgres ADP binds to the executing role). A different +identity running a schema-only ``make migrate`` needs an explicit re-grant. +See ``docs/runbooks/reference-edges.md`` for the verification query and the +re-grant command. +""" + +from __future__ import annotations + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "0005" +down_revision: str | None = "0004" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + op.create_table( + "reference_edges", + sa.Column("id", sa.BigInteger(), nullable=False), + sa.Column("repo_id", sa.Integer(), nullable=False), + sa.Column("file_id", sa.Integer(), nullable=False), + sa.Column("edge_kind", sa.Text(), nullable=False), + sa.Column("target_name", sa.Text(), nullable=False), + sa.Column("line", sa.Integer(), nullable=False), + sa.Column("enclosing_name", sa.Text(), nullable=True), + sa.Column("enclosing_kind", sa.Text(), nullable=True), + sa.Column("enclosing_start_line", sa.Integer(), nullable=True), + sa.Column("enclosing_end_line", sa.Integer(), nullable=True), + sa.CheckConstraint("edge_kind IN ('call', 'import')", name="ck_reference_edges_edge_kind"), + sa.ForeignKeyConstraint(["repo_id"], ["repos.id"], ondelete="CASCADE"), + sa.ForeignKeyConstraint(["file_id"], ["files.id"], ondelete="CASCADE"), + sa.PrimaryKeyConstraint("id"), + ) + op.create_index( + "ix_reference_edges_target_name", + "reference_edges", + ["target_name"], + unique=False, + ) + op.create_index( + "ix_reference_edges_target_trgm", + "reference_edges", + ["target_name"], + unique=False, + postgresql_using="gin", + postgresql_ops={"target_name": "gin_trgm_ops"}, + ) + # Load-bearing for write performance, not just integrity: Postgres does NOT + # auto-index a foreign key, and file_id is the hot lookup for both the + # per-file delete-and-reinsert writer and the ON DELETE CASCADE fired by + # store.py's mark-and-sweep -- same rationale as ix_symbols' analog on + # symbols.file_id (there via the implicit FK) and uq_chunks_file_id_chunk_index. + op.create_index( + "ix_reference_edges_file_id", + "reference_edges", + ["file_id"], + unique=False, + ) + op.create_index( + "ix_reference_edges_repo_kind", + "reference_edges", + ["repo_id", "edge_kind"], + unique=False, + ) + + +def downgrade() -> None: + op.drop_index("ix_reference_edges_repo_kind", table_name="reference_edges") + op.drop_index("ix_reference_edges_file_id", table_name="reference_edges") + op.drop_index("ix_reference_edges_target_trgm", table_name="reference_edges") + op.drop_index("ix_reference_edges_target_name", table_name="reference_edges") + op.drop_table("reference_edges") diff --git a/app/db/models.py b/app/db/models.py index d07910c..37133e6 100644 --- a/app/db/models.py +++ b/app/db/models.py @@ -1,7 +1,7 @@ """SQLAlchemy 2.0 models for the durable code search core. -Covers ``repos`` / ``files`` / ``symbols`` / ``repo_branches``. No ``chunks`` / -``VECTOR`` / ``tsvector`` (a separate version table). ``Base.metadata`` is +Covers ``repos`` / ``files`` / ``symbols`` / ``repo_branches`` / ``reference_edges``. +No ``chunks`` / ``VECTOR`` / ``tsvector`` (a separate version table). ``Base.metadata`` is the authoritative desired-state: it also declares the pg_trgm and ``files.branches`` GIN indexes so Alembic autogenerate emits them and there is no future drift. The 0001 migration owns the single ``CREATE EXTENSION IF NOT @@ -13,7 +13,16 @@ from datetime import datetime -from sqlalchemy import DateTime, ForeignKey, Index, Integer, Text, UniqueConstraint +from sqlalchemy import ( + BigInteger, + CheckConstraint, + DateTime, + ForeignKey, + Index, + Integer, + Text, + UniqueConstraint, +) from sqlalchemy.dialects.postgresql import ARRAY from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column, relationship @@ -98,6 +107,9 @@ class File(Base): symbols: Mapped[list[Symbol]] = relationship( back_populates="file", cascade="all, delete-orphan" ) + reference_edges: Mapped[list[ReferenceEdge]] = relationship( + back_populates="file", cascade="all, delete-orphan" + ) class Symbol(Base): @@ -122,6 +134,45 @@ class Symbol(Base): file: Mapped[File] = relationship(back_populates="symbols") +class ReferenceEdge(Base): + """Raw (unresolved) call/import edge for one file content-version. + + Deliberately NO FK to symbols: symbol ids churn on every per-file + delete-and-reinsert, and an FK would couple the two rewrite orders inside + the indexing transaction for no query benefit (epic #82 rule: resolution + happens at query time by name-join). Enclosing symbol is denormalized. + NULL enclosing_* means module/top-level scope. Branch scoping rides + files.branches at query time, exactly as symbols does -- no branch column. + """ + + __tablename__ = "reference_edges" + __table_args__ = ( + CheckConstraint("edge_kind IN ('call', 'import')", name="ck_reference_edges_edge_kind"), + Index("ix_reference_edges_target_name", "target_name"), + Index( + "ix_reference_edges_target_trgm", + "target_name", + postgresql_using="gin", + postgresql_ops={"target_name": "gin_trgm_ops"}, + ), + Index("ix_reference_edges_file_id", "file_id"), + Index("ix_reference_edges_repo_kind", "repo_id", "edge_kind"), + ) + + id: Mapped[int] = mapped_column(BigInteger, primary_key=True) + repo_id: Mapped[int] = mapped_column(ForeignKey("repos.id", ondelete="CASCADE")) + file_id: Mapped[int] = mapped_column(ForeignKey("files.id", ondelete="CASCADE")) + edge_kind: Mapped[str] = mapped_column(Text) + target_name: Mapped[str] = mapped_column(Text) + line: Mapped[int] = mapped_column(Integer) + enclosing_name: Mapped[str | None] = mapped_column(Text) + enclosing_kind: Mapped[str | None] = mapped_column(Text) + enclosing_start_line: Mapped[int | None] = mapped_column(Integer) + enclosing_end_line: Mapped[int | None] = mapped_column(Integer) + + file: Mapped[File] = relationship(back_populates="reference_edges") + + class RepoBranch(Base): """Per-(repo, branch) index registry: the authoritative CAS stamp (0003+). diff --git a/indexer/store.py b/indexer/store.py index 1646d52..4919f0c 100644 --- a/indexer/store.py +++ b/indexer/store.py @@ -105,7 +105,7 @@ def index_repo( 4. Membership sweep, keyed on THIS branch's seen-set (never on ``commit``, which is ambiguous under dedup): strip ``branch`` from any row's ``branches`` array that is not in the seen-set, then delete any row left - with an empty array (cascades ``symbols``/``chunks``). Pure DML, no + with an empty array (cascades ``symbols``/``chunks``/``reference_edges``). Pure DML, no ``TEMP TABLE`` (the job role has no guaranteed database-level TEMP privilege on Lakebase). **Skipped (with a WARNING) when the parsed file set is empty** -- an empty seen-set would otherwise strip ``branch`` from @@ -380,9 +380,9 @@ def reconcile_retired_branches( emptied array, which would leave a zombie row the next step's cardinality check can't see. 3. Delete rows left with zero membership (``cardinality(branches) = 0``); - a strict subset of step 2's rowcount. ``symbols`` and ``chunks`` are - removed by FK cascade, the same invariant ``_sweep_membership`` relies - on (see its docstring, indexer/store.py). + a strict subset of step 2's rowcount. ``symbols``, ``chunks``, and + ``reference_edges`` are removed by FK cascade, the same invariant + ``_sweep_membership`` relies on (see its docstring, indexer/store.py). 4. Delete the matching ``repo_branches`` registry rows. Invariants: repo-scoped on every statement; membership subtraction only, @@ -467,11 +467,13 @@ def reconcile_removed_repos(conn: Connection, *, desired_repos: Collection[str]) RETURNING name`` -- one atomic statement, no prior ``SELECT``. Every victim row's cascade is proven at the database level, not the ORM: ``repos`` -> ``files`` and ``repos`` -> ``symbols`` and ``repos`` -> - ``repo_branches`` are direct ``ON DELETE CASCADE`` foreign keys - (``app/db/models.py``), ``files`` -> ``symbols`` is the same, and + ``repo_branches`` and ``repos`` -> ``reference_edges`` are direct + ``ON DELETE CASCADE`` foreign keys (``app/db/models.py``), ``files`` -> + ``symbols`` and ``files`` -> ``reference_edges`` are the same, and ``files`` -> ``chunks`` cascades via the raw DDL in ``app/alembic/versions/0004_semantic_chunks.py`` -- so a two-hop - ``repos`` -> ``files`` -> ``chunks`` delete fires as one statement. + ``repos`` -> ``files`` -> ``chunks``/``reference_edges`` delete fires as + one statement. ``RETURNING name`` reads back only ``repos`` rows, i.e. exactly the purged repo names, with no separate count query needed. The job role already holds ``DELETE`` on every table in this schema From a9949f63662b5bfbb54cc4d6816468b351505a3b Mon Sep 17 00:00:00 2001 From: IceRhymers Date: Thu, 23 Jul 2026 13:09:00 -0700 Subject: [PATCH 2/4] test: cover reference_edges schema, cascades, and grant lifecycle (#83) Unit: source-level tripwires for migration 0005 (revision chain, no symbols FK, no app import, no stray CREATE EXTENSION) plus an ORM-metadata tripwire (test_reference_edge_model.py) so a future models.py edit can't silently reintroduce a symbols FK or loosen a NOT NULL column. Extends the durable-core-tables tripwire in test_db_client.py. Integration: adds migrated_edges_capable, a fixture that reaches migration head on stock dev Postgres too (by pre-seeding a stub chunks table before 0004, the same idempotency guard test_0004_guard_preserves_preexisting_chunks already exercises) so the reference_edges shape/cascade/EXPLAIN/downgrade tests and both ADP same-role/different-role grant-lifecycle proofs run without a live Lakebase branch. Extends test_reconcile.py and test_store.py to seed reference_edges rows and assert they cascade through the existing sweep/reconcile paths. --- tests/integration/test_migrations.py | 406 +++++++++++++++++++++++- tests/integration/test_reconcile.py | 34 +- tests/integration/test_store.py | 14 + tests/unit/test_db_client.py | 8 +- tests/unit/test_migration_source.py | 73 ++++- tests/unit/test_reference_edge_model.py | 89 ++++++ 6 files changed, 616 insertions(+), 8 deletions(-) create mode 100644 tests/unit/test_reference_edge_model.py diff --git a/tests/integration/test_migrations.py b/tests/integration/test_migrations.py index 98139b8..6cae61a 100644 --- a/tests/integration/test_migrations.py +++ b/tests/integration/test_migrations.py @@ -4,7 +4,9 @@ connection (the same path ``scripts/migrate.py`` uses), with the Alembic version table pinned to that schema. Requires a Lakebase branch whose project preloads ``lakebase_vector,lakebase_text`` (``upgrade head`` includes the 0004 semantic -revision; see docs/runbooks/ci-lakebase.md). +revision; see docs/runbooks/ci-lakebase.md). Exception: the ``reference_edges`` +(0005) tests use the ``migrated_edges_capable`` fixture, which reaches ``head`` +on a stock dev Postgres too -- see ``_upgrade_edges_capable``'s docstring. The enforcement test exercises the least-privilege grants on the *same* superuser connection via ``SET ROLE`` to a NOLOGIN role; opening a second engine would @@ -27,7 +29,7 @@ from alembic.config import Config from alembic.migration import MigrationContext from sqlalchemy import Connection, text -from sqlalchemy.exc import ProgrammingError +from sqlalchemy.exc import IntegrityError, ProgrammingError from app.db.client import create_db_engine from app.db.grants import build_app_grants, build_job_grants @@ -85,6 +87,74 @@ def migrated() -> Iterator[Migrated]: engine.dispose() +def _lakebase_extensions_available(conn: Connection) -> bool: + return bool( + conn.execute( + text("SELECT 1 FROM pg_available_extensions WHERE name = 'lakebase_tokenizer'") + ).scalar() + ) + + +_STUB_CHUNKS_DDL = ( + "CREATE TABLE chunks (" + "id bigserial PRIMARY KEY, " + "file_id integer NOT NULL REFERENCES files(id) ON DELETE CASCADE, " + "chunk_index integer NOT NULL, " + "content text NOT NULL)" +) + + +def _upgrade_edges_capable(config: Config, conn: Connection, target: str) -> None: + """Upgrade to ``target`` ("0004" or "head"), working on both a real Lakebase + branch and a stock dev Postgres. + + On real Lakebase, 0004 runs natively (the ``lakebase_*`` extensions exist). + On stock Postgres those extensions are absent, so this pre-seeds a stub + ``chunks`` table before reaching 0004 -- the exact idempotency guard + ``test_0004_guard_preserves_preexisting_chunks`` exercises -- which makes + 0004 skip its extension/index DDL and land cleanly. 0005's ``reference_edges`` + DDL is pure ``pg_trgm`` (no Lakebase dependency), so it then applies on + either environment. + """ + if _lakebase_extensions_available(conn): + command.upgrade(config, target) + return + command.upgrade(config, "0003") + conn.execute(text(_STUB_CHUNKS_DDL)) + command.upgrade(config, target) + + +@pytest.fixture +def migrated_edges_capable() -> Iterator[Migrated]: + """Like ``migrated``, but reaches ``head`` on stock Postgres too (see + ``_upgrade_edges_capable``) so the ``reference_edges`` tests run locally + without a live Lakebase branch.""" + schema = _unique("test_edges") + engine = create_db_engine() + conn = engine.connect() + try: + conn.execute(text(f"DROP SCHEMA IF EXISTS {schema} CASCADE")) + conn.execute(text(f"CREATE SCHEMA {schema}")) + conn.execute(text(f"SET search_path TO {schema}, public")) + conn.commit() + + config = Config("alembic.ini") + config.attributes["connection"] = conn + config.attributes["version_table_schema"] = schema + + _upgrade_edges_capable(config, conn, "head") + conn.commit() + + yield Migrated(conn=conn, schema=schema, config=config) + finally: + conn.rollback() + conn.execute(text("RESET ROLE")) + conn.execute(text(f"DROP SCHEMA IF EXISTS {schema} CASCADE")) + conn.commit() + conn.close() + engine.dispose() + + @pytest.mark.integration def test_schema_fidelity(migrated: Migrated) -> None: conn, schema = migrated.conn, migrated.schema @@ -97,7 +167,9 @@ def test_schema_fidelity(migrated: Migrated) -> None: .scalars() .all() ) - assert {"repos", "files", "symbols", "repo_branches", "chunks"} <= set(tables) + assert {"repos", "files", "symbols", "repo_branches", "chunks", "reference_edges"} <= set( + tables + ) rows = conn.execute( text("SELECT indexname, indexdef FROM pg_indexes WHERE schemaname = :s"), @@ -111,6 +183,17 @@ def test_schema_fidelity(migrated: Migrated) -> None: assert "ix_files_branches_gin" in by_name assert "USING gin" in by_name["ix_files_branches_gin"] + for idx in ( + "ix_reference_edges_target_name", + "ix_reference_edges_file_id", + "ix_reference_edges_repo_kind", + ): + assert idx in by_name, f"missing index {idx}" + assert "USING gin" not in by_name[idx], f"{idx} must be a plain btree index" + assert "ix_reference_edges_target_trgm" in by_name + assert "USING gin" in by_name["ix_reference_edges_target_trgm"] + assert "gin_trgm_ops" in by_name["ix_reference_edges_target_trgm"] + fk_count = conn.execute( text( "SELECT count(*) FROM information_schema.table_constraints tc " @@ -214,7 +297,8 @@ def test_downgrade_drops_tables_but_keeps_extension(migrated: Migrated) -> None: conn.execute( text( "SELECT table_name FROM information_schema.tables " - "WHERE table_schema = :s AND table_name IN ('repos', 'files', 'symbols')" + "WHERE table_schema = :s AND table_name IN " + "('repos', 'files', 'symbols', 'reference_edges')" ), {"s": schema}, ) @@ -730,3 +814,317 @@ def test_0003_downgrade_blocked_by_multi_branch_data(migrated: Migrated) -> None with pytest.raises(RuntimeError, match="multi-branch data present"): command.downgrade(migrated.config, "0002") conn.rollback() + + +def _seed_repo_and_file( + conn: Connection, *, repo_name: str, path: str, sha: str +) -> tuple[int, int]: + conn.execute(text("INSERT INTO repos (name) VALUES (:n)"), {"n": repo_name}) + repo_id = conn.execute(text("SELECT id FROM repos WHERE name = :n"), {"n": repo_name}).scalar() + conn.execute( + text( + "INSERT INTO files (repo_id, path, content_sha, branches) " + "VALUES (:r, :p, :sha, ARRAY['main'])" + ), + {"r": repo_id, "p": path, "sha": sha}, + ) + file_id = conn.execute( + text("SELECT id FROM files WHERE repo_id = :r AND path = :p"), + {"r": repo_id, "p": path}, + ).scalar() + return repo_id, file_id + + +@pytest.mark.integration +def test_reference_edges_shape_and_constraints(migrated_edges_capable: Migrated) -> None: + conn = migrated_edges_capable.conn + repo_id, file_id = _seed_repo_and_file(conn, repo_name="shape_repo", path="a.py", sha="sha1") + + conn.execute( + text( + "INSERT INTO reference_edges " + "(repo_id, file_id, edge_kind, target_name, line, enclosing_name, enclosing_kind, " + "enclosing_start_line, enclosing_end_line) " + "VALUES (:r, :f, 'call', 'target_fn', 5, 'caller_fn', 'function', 1, 20)" + ), + {"r": repo_id, "f": file_id}, + ) + conn.commit() + assert conn.execute(text("SELECT count(*) FROM reference_edges")).scalar() == 1 + + with pytest.raises(IntegrityError) as excinfo: + conn.execute( + text( + "INSERT INTO reference_edges (repo_id, file_id, edge_kind, target_name, line) " + "VALUES (:r, :f, 'definition', 'x', 1)" + ), + {"r": repo_id, "f": file_id}, + ) + assert isinstance(excinfo.value.orig, psycopg.errors.CheckViolation) + conn.rollback() + + with pytest.raises(IntegrityError) as excinfo: + conn.execute( + text( + "INSERT INTO reference_edges (repo_id, file_id, edge_kind, target_name, line) " + "VALUES (:r, :f, 'call', NULL, 1)" + ), + {"r": repo_id, "f": file_id}, + ) + assert isinstance(excinfo.value.orig, psycopg.errors.NotNullViolation) + conn.rollback() + + +@pytest.mark.integration +def test_reference_edges_cascade_on_file_and_repo_delete(migrated_edges_capable: Migrated) -> None: + conn = migrated_edges_capable.conn + repo_id, file_id = _seed_repo_and_file(conn, repo_name="cascade_repo", path="a.py", sha="sha1") + conn.execute( + text( + "INSERT INTO reference_edges (repo_id, file_id, edge_kind, target_name, line) " + "VALUES (:r, :f, 'call', 'target_fn', 5)" + ), + {"r": repo_id, "f": file_id}, + ) + conn.commit() + assert conn.execute(text("SELECT count(*) FROM reference_edges")).scalar() == 1 + + conn.execute(text("DELETE FROM files WHERE id = :f"), {"f": file_id}) + conn.commit() + assert conn.execute(text("SELECT count(*) FROM reference_edges")).scalar() == 0 + + _, file_id2 = _seed_repo_and_file(conn, repo_name="cascade_repo2", path="b.py", sha="sha2") + repo_id2 = conn.execute( + text("SELECT repo_id FROM files WHERE id = :f"), {"f": file_id2} + ).scalar() + conn.execute( + text( + "INSERT INTO reference_edges (repo_id, file_id, edge_kind, target_name, line) " + "VALUES (:r, :f, 'import', 'os', 1)" + ), + {"r": repo_id2, "f": file_id2}, + ) + conn.commit() + assert conn.execute(text("SELECT count(*) FROM reference_edges")).scalar() == 1 + + conn.execute(text("DELETE FROM repos WHERE id = :r"), {"r": repo_id2}) + conn.commit() + assert conn.execute(text("SELECT count(*) FROM reference_edges")).scalar() == 0 + + +@pytest.mark.integration +def test_reference_edges_adp_same_role_covers_new_table() -> None: + """Positive proof of the runbook's lifecycle-safe-grants claim: when the SAME + role runs ``ALTER DEFAULT PRIVILEGES`` and later creates ``reference_edges`` + via a schema-only migrate, the app/job grants apply automatically -- no + re-grant between ``upgrade head`` and the privilege assertions below.""" + schema = _unique("test_adp_same") + app_ro = _unique("app_ro") + job_rw = _unique("job_rw") + engine = create_db_engine() + conn = engine.connect() + try: + conn.execute(text(f"DROP SCHEMA IF EXISTS {schema} CASCADE")) + conn.execute(text(f"CREATE SCHEMA {schema}")) + conn.execute(text(f"SET search_path TO {schema}, public")) + conn.execute(text(f"CREATE ROLE {app_ro} NOLOGIN")) + conn.execute(text(f"CREATE ROLE {job_rw} NOLOGIN")) + conn.commit() + + config = Config("alembic.ini") + config.attributes["connection"] = conn + config.attributes["version_table_schema"] = schema + + _upgrade_edges_capable(config, conn, "0004") + conn.commit() + + for stmt in build_app_grants(schema, app_ro): + conn.execute(text(stmt)) + for stmt in build_job_grants(schema, job_rw): + conn.execute(text(stmt)) + conn.commit() + + # Same identity (this connection) now creates reference_edges via 0005 -- + # no re-grant runs between this upgrade and the assertions below. + command.upgrade(config, "head") + conn.commit() + + conn.execute(text(f"SET ROLE {app_ro}")) + conn.execute(text(f"SET search_path TO {schema}, public")) + conn.execute(text("SELECT * FROM reference_edges")).all() + with pytest.raises(ProgrammingError) as excinfo: + conn.execute( + text( + "INSERT INTO reference_edges (repo_id, file_id, edge_kind, target_name, line) " + "VALUES (1, 1, 'call', 'x', 1)" + ) + ) + assert isinstance(excinfo.value.orig, psycopg.errors.InsufficientPrivilege) + conn.rollback() + + conn.execute(text(f"SET ROLE {job_rw}")) + conn.execute(text(f"SET search_path TO {schema}, public")) + repo_id, file_id = _seed_repo_and_file(conn, repo_name="adp_repo", path="a.py", sha="sha1") + conn.execute( + text( + "INSERT INTO reference_edges (repo_id, file_id, edge_kind, target_name, line) " + "VALUES (:r, :f, 'call', 'target_fn', 10)" + ), + {"r": repo_id, "f": file_id}, + ) + conn.execute(text("DELETE FROM reference_edges")) + conn.execute(text("RESET ROLE")) + conn.rollback() + finally: + conn.rollback() + conn.execute(text("RESET ROLE")) + conn.execute(text(f"DROP SCHEMA IF EXISTS {schema} CASCADE")) + for role in (app_ro, job_rw): + conn.execute(text(f"DROP OWNED BY {role} CASCADE")) + conn.execute(text(f"DROP ROLE IF EXISTS {role}")) + conn.commit() + conn.close() + engine.dispose() + + +@pytest.mark.integration +def test_reference_edges_adp_different_role_does_not_cover_new_table() -> None: + """Negative proof, motivating the runbook's re-grant command: Postgres ADP + binds to the EXECUTING role, not the schema -- a table created by a + different identity than the one that ran ``ALTER DEFAULT PRIVILEGES`` gets + no automatic grant.""" + schema = _unique("test_adp_diff") + app_ro = _unique("app_ro") + other_creator = _unique("other_creator") + engine = create_db_engine() + conn = engine.connect() + try: + conn.execute(text(f"DROP SCHEMA IF EXISTS {schema} CASCADE")) + conn.execute(text(f"CREATE SCHEMA {schema}")) + conn.execute(text(f"CREATE ROLE {app_ro} NOLOGIN")) + conn.execute(text(f"CREATE ROLE {other_creator} NOLOGIN")) + conn.execute(text(f"GRANT CREATE, USAGE ON SCHEMA {schema} TO {other_creator}")) + conn.commit() + + for stmt in build_app_grants(schema, app_ro): + conn.execute(text(stmt)) + conn.commit() + + conn.execute(text(f"SET ROLE {other_creator}")) + conn.execute(text(f"SET search_path TO {schema}, public")) + conn.execute(text("CREATE TABLE shadow_table (id serial PRIMARY KEY)")) + conn.execute(text("RESET ROLE")) + conn.commit() + + has_select = conn.execute( + text(f"SELECT has_table_privilege('{app_ro}', '{schema}.shadow_table', 'SELECT')") + ).scalar() + assert has_select is False, ( + "a table created by a different identity than the one that ran ADP " + "must NOT automatically receive the app role's SELECT grant" + ) + finally: + conn.rollback() + conn.execute(text("RESET ROLE")) + conn.execute(text(f"DROP SCHEMA IF EXISTS {schema} CASCADE")) + for role in (app_ro, other_creator): + conn.execute(text(f"DROP OWNED BY {role} CASCADE")) + conn.execute(text(f"DROP ROLE IF EXISTS {role}")) + conn.commit() + conn.close() + engine.dispose() + + +def _seed_explain_fixture(conn: Connection) -> None: + repo_id, file_id = _seed_repo_and_file(conn, repo_name="explain_repo", path="a.py", sha="sha1") + conn.execute( + text( + "INSERT INTO symbols (file_id, repo_id, name, kind) " + "VALUES (:f, :r, 'handle_request', 'function')" + ), + {"f": file_id, "r": repo_id}, + ) + names = ["handle_request", "handle_response", "parse_args", "other_fn"] * 5 + conn.execute( + text( + "INSERT INTO reference_edges (repo_id, file_id, edge_kind, target_name, line) " + "VALUES (:r, :f, 'call', :name, :line)" + ), + [{"r": repo_id, "f": file_id, "name": name, "line": i + 1} for i, name in enumerate(names)], + ) + conn.commit() + + +@pytest.mark.integration +def test_reference_edges_explain_resolver_join_uses_target_name_index( + migrated_edges_capable: Migrated, +) -> None: + """EXPLAIN on the resolver's join shape (equality + name-join against + symbols) must reference the btree ``ix_reference_edges_target_name``.""" + conn = migrated_edges_capable.conn + _seed_explain_fixture(conn) + + with conn.begin(): + conn.execute(text("SET LOCAL enable_seqscan = off")) + plan = ( + conn.execute( + text( + "EXPLAIN SELECT re.id FROM reference_edges re " + "JOIN symbols s ON s.name = re.target_name " + "WHERE re.target_name = 'handle_request'" + ) + ) + .scalars() + .all() + ) + plan_text = "\n".join(plan) + assert "ix_reference_edges_target_name" in plan_text, plan_text + + +@pytest.mark.integration +def test_reference_edges_explain_ilike_uses_trgm_gin_index( + migrated_edges_capable: Migrated, +) -> None: + conn = migrated_edges_capable.conn + _seed_explain_fixture(conn) + + with conn.begin(): + conn.execute(text("SET LOCAL enable_seqscan = off")) + plan = ( + conn.execute( + text("EXPLAIN SELECT * FROM reference_edges WHERE target_name ILIKE '%handle%'") + ) + .scalars() + .all() + ) + plan_text = "\n".join(plan) + assert "ix_reference_edges_target_trgm" in plan_text, plan_text + + +@pytest.mark.integration +def test_reference_edges_downgrade_to_0004_removes_table_and_indexes( + migrated_edges_capable: Migrated, +) -> None: + conn, schema, config = migrated_edges_capable + + command.downgrade(config, "0004") + conn.commit() + + assert conn.execute(text("SELECT to_regclass('reference_edges')")).scalar() is None + remaining_indexes = ( + conn.execute( + text( + "SELECT indexname FROM pg_indexes " + "WHERE schemaname = :s AND indexname LIKE 'ix_reference_edges%'" + ), + {"s": schema}, + ) + .scalars() + .all() + ) + assert remaining_indexes == [] + + # The chain re-upgrades cleanly. + command.upgrade(config, "head") + conn.commit() + assert conn.execute(text("SELECT to_regclass('reference_edges')")).scalar() is not None diff --git a/tests/integration/test_reconcile.py b/tests/integration/test_reconcile.py index 6a1b85c..d377e43 100644 --- a/tests/integration/test_reconcile.py +++ b/tests/integration/test_reconcile.py @@ -145,6 +145,23 @@ def _cfg() -> Settings: ) +def _seed_reference_edge( + conn: Connection, *, repo_id: int, file_id: int, target_name: str = "target_fn" +) -> None: + """Seed one raw reference edge row (indexer.store has no writer yet -- #84).""" + conn.execute( + text( + "INSERT INTO reference_edges (repo_id, file_id, edge_kind, target_name, line) " + "VALUES (:r, :f, 'call', :name, 1)" + ), + {"r": repo_id, "f": file_id, "name": target_name}, + ) + + +def _repo_id(conn: Connection, name: str) -> int: + return int(conn.execute(text("SELECT id FROM repos WHERE name = :n"), {"n": name}).scalar_one()) + + def _repo_branch_names(conn: Connection, name: str) -> set[str]: rows = ( conn.execute( @@ -278,9 +295,15 @@ def test_divergent_branch_only_file_is_deleted_with_symbols_and_chunks_cascade( ) conn.rollback() + repo_id = _repo_id(conn, "acme/widgets") feature_file_id = _file_id(conn, "only_feature.py") + main_file_id = _file_id(conn, "main.py") + _seed_reference_edge(conn, repo_id=repo_id, file_id=feature_file_id, target_name="retired_fn") + _seed_reference_edge(conn, repo_id=repo_id, file_id=main_file_id, target_name="surviving_fn") + conn.commit() assert _count(conn, "symbols", f"file_id = {feature_file_id}") == 1 assert _count(conn, "chunks", f"file_id = {feature_file_id}") == 1 + assert _count(conn, "reference_edges", f"file_id = {feature_file_id}") == 1 conn.rollback() # clear the reads' autobegun txn before reconcile's own conn.begin() counts = reconcile_retired_branches(conn, name="acme/widgets", retired_branches=["feature"]) @@ -289,10 +312,12 @@ def test_divergent_branch_only_file_is_deleted_with_symbols_and_chunks_cascade( assert _count(conn, "files", "path = 'only_feature.py'") == 0 assert _count(conn, "symbols", f"file_id = {feature_file_id}") == 0 # cascade assert _count(conn, "chunks", f"file_id = {feature_file_id}") == 0 # cascade + assert _count(conn, "reference_edges", f"file_id = {feature_file_id}") == 0 # cascade - # main.py's branch ('main') was never retired -> untouched. + # main.py's branch ('main') was never retired -> untouched, including its edge row. assert _count(conn, "files", "path = 'main.py'") == 1 assert _branches_of(conn, "main.py") == ["main"] + assert _count(conn, "reference_edges", f"file_id = {main_file_id}") == 1 @pytest.mark.integration @@ -482,7 +507,7 @@ def test_reconcile_runs_under_the_actual_job_role(conn: Connection) -> None: @pytest.mark.integration -def test_reconcile_removed_repos_purges_repo_and_cascades_all_five_tables( +def test_reconcile_removed_repos_purges_repo_and_cascades_all_six_tables( conn: Connection, ) -> None: index_repo( @@ -508,9 +533,13 @@ def test_reconcile_removed_repos_purges_repo_and_cascades_all_five_tables( # Capture the victim's file_id(s) BEFORE the purge -- once the repo row is gone, # post-hoc reconstruction via a repo-name JOIN is impossible. + removed_repo_id = _repo_id(conn, "acme/removed") removed_file_id = _file_id(conn, "only_feature.py", repo="acme/removed") + _seed_reference_edge(conn, repo_id=removed_repo_id, file_id=removed_file_id) + conn.commit() assert _count(conn, "symbols", f"file_id = {removed_file_id}") == 1 assert _count(conn, "chunks", f"file_id = {removed_file_id}") == 1 + assert _count(conn, "reference_edges", f"file_id = {removed_file_id}") == 1 conn.rollback() # clear the reads' autobegun txn before reconcile's own conn.begin() deleted = reconcile_removed_repos(conn, desired_repos=["acme/kept"]) @@ -521,6 +550,7 @@ def test_reconcile_removed_repos_purges_repo_and_cascades_all_five_tables( assert _count(conn, "files", "path = 'only_feature.py'") == 0 assert _count(conn, "symbols", f"file_id = {removed_file_id}") == 0 # cascade assert _count(conn, "chunks", f"file_id = {removed_file_id}") == 0 # cascade + assert _count(conn, "reference_edges", f"file_id = {removed_file_id}") == 0 # cascade # acme/kept is completely untouched. assert _count(conn, "repos", "name = 'acme/kept'") == 1 diff --git a/tests/integration/test_store.py b/tests/integration/test_store.py index c0db450..7aff396 100644 --- a/tests/integration/test_store.py +++ b/tests/integration/test_store.py @@ -131,9 +131,22 @@ def test_rerun_is_idempotent(conn: Connection) -> None: @pytest.mark.integration def test_mark_and_sweep_removes_deleted_file(conn: Connection) -> None: _index_default(conn, name="acme/widgets", head_sha="sha_first", items=_items(MAIN, UTIL)) + repo_id = conn.execute(text("SELECT id FROM repos WHERE name = 'acme/widgets'")).scalar_one() removed_file_id = conn.execute(text("SELECT id FROM files WHERE path = 'util.py'")).scalar_one() assert _count(conn, "symbols", f"file_id = {removed_file_id}") == 1 + # index_repo has no reference_edges writer yet (#84); seed one directly to + # prove the sweep's FK cascade reaches this table too. + conn.execute( + text( + "INSERT INTO reference_edges (repo_id, file_id, edge_kind, target_name, line) " + "VALUES (:r, :f, 'call', 'target_fn', 1)" + ), + {"r": repo_id, "f": removed_file_id}, + ) + conn.commit() + assert _count(conn, "reference_edges", f"file_id = {removed_file_id}") == 1 + # Reads above autobegan a txn; clear it so index_repo gets a clean connection # (production hands a fresh engine.connect() per repo). conn.rollback() @@ -144,6 +157,7 @@ def test_mark_and_sweep_removes_deleted_file(conn: Connection) -> None: assert _count(conn, "files", "path = 'util.py'") == 0 assert _count(conn, "symbols", f"file_id = {removed_file_id}") == 0 # cascade + assert _count(conn, "reference_edges", f"file_id = {removed_file_id}") == 0 # cascade assert _count(conn, "files") == 1 assert _count(conn, "files", "commit = 'sha_second'") == 1 diff --git a/tests/unit/test_db_client.py b/tests/unit/test_db_client.py index d25acd4..7c5ed35 100644 --- a/tests/unit/test_db_client.py +++ b/tests/unit/test_db_client.py @@ -58,7 +58,13 @@ def _boom(*args: object, **kwargs: object) -> object: @pytest.mark.unit def test_metadata_has_exactly_durable_core_tables() -> None: - assert set(Base.metadata.tables) == {"repos", "files", "symbols", "repo_branches"} + assert set(Base.metadata.tables) == { + "repos", + "files", + "symbols", + "repo_branches", + "reference_edges", + } @pytest.mark.unit diff --git a/tests/unit/test_migration_source.py b/tests/unit/test_migration_source.py index 751cd7b..75cca62 100644 --- a/tests/unit/test_migration_source.py +++ b/tests/unit/test_migration_source.py @@ -18,7 +18,7 @@ import pytest _VERSIONS_DIR = Path(__file__).resolve().parents[2] / "app" / "alembic" / "versions" -_EXPECTED_REVISIONS = {"0001", "0002", "0003", "0004"} +_EXPECTED_REVISIONS = {"0001", "0002", "0003", "0004", "0005"} @pytest.fixture @@ -48,6 +48,11 @@ def source_0003(sources: dict[str, str]) -> str: return sources["0003"] +@pytest.fixture +def source_0005(sources: dict[str, str]) -> str: + return sources["0005"] + + @pytest.mark.unit def test_revision_identifiers(source: str) -> None: assert 'revision: str = "0001"' in source @@ -145,3 +150,69 @@ def test_0003_does_not_import_app_constant(source_0003: str) -> None: assert not line.startswith(("import app", "from app")), ( f"0003 must not import from the app package: {line!r}" ) + + +@pytest.mark.unit +def test_0005_revision_identifiers(source_0005: str) -> None: + assert 'revision: str = "0005"' in source_0005 + assert 'down_revision: str | None = "0004"' in source_0005 + + +@pytest.mark.unit +def test_0005_does_not_import_app_constant(source_0005: str) -> None: + """A migration is a historical fact; it must not depend on a mutable app constant.""" + code_lines = [ + line + for line in source_0005.splitlines() + if line.strip() and not line.lstrip().startswith("#") + ] + for line in code_lines: + if line.startswith(("import ", "from ")): + assert not line.startswith(("import app", "from app")), ( + f"0005 must not import from the app package: {line!r}" + ) + + +@pytest.mark.unit +def test_0005_downgrade_drops_indexes_then_table(source_0005: str) -> None: + downgrade_pos = source_0005.find("def downgrade()") + assert downgrade_pos != -1 + downgrade_body = source_0005[downgrade_pos:] + for index_name in ( + "ix_reference_edges_target_name", + "ix_reference_edges_target_trgm", + "ix_reference_edges_file_id", + "ix_reference_edges_repo_kind", + ): + assert f'op.drop_index("{index_name}"' in downgrade_body, ( + f"0005 downgrade must drop {index_name!r}" + ) + table_pos = downgrade_body.find('op.drop_table("reference_edges")') + assert table_pos != -1, "0005 downgrade must drop the reference_edges table" + last_index_pos = max( + downgrade_body.find(f'op.drop_index("{index_name}"') + for index_name in ( + "ix_reference_edges_target_name", + "ix_reference_edges_target_trgm", + "ix_reference_edges_file_id", + "ix_reference_edges_repo_kind", + ) + ) + assert last_index_pos < table_pos, "all indexes must drop before the table" + + +@pytest.mark.unit +def test_0005_no_symbol_fk(source_0005: str) -> None: + """Epic #82 rule: reference_edges must never gain a symbols FK at the source level.""" + for forbidden in ('ForeignKey("symbols', "REFERENCES symbols", '"symbols.id"'): + assert forbidden not in source_0005, f"0005 migration must not reference {forbidden!r}" + + +@pytest.mark.unit +def test_0005_does_not_create_extension(source_0005: str) -> None: + """pg_trgm already exists since 0001 (database-wide); 0005 must not re-create it.""" + upgrade_pos = source_0005.find("def upgrade()") + downgrade_pos = source_0005.find("def downgrade()") + assert upgrade_pos != -1 and downgrade_pos != -1 + body = source_0005[upgrade_pos:downgrade_pos] + assert "CREATE EXTENSION" not in body diff --git a/tests/unit/test_reference_edge_model.py b/tests/unit/test_reference_edge_model.py new file mode 100644 index 0000000..d77b4ba --- /dev/null +++ b/tests/unit/test_reference_edge_model.py @@ -0,0 +1,89 @@ +"""Model-level tripwires for ``reference_edges`` (no database required). + +Introspects ``Base.metadata`` directly, mirroring ``test_migration_source.py``'s +source-level checks but at the ORM-metadata layer: a future edit to +``app/db/models.py`` that accidentally introduces a ``symbols`` FK, drops the +CHECK constraint, or loosens a NOT NULL column fails here, not in production. +""" + +from __future__ import annotations + +import pytest +from sqlalchemy import BigInteger, CheckConstraint, ForeignKeyConstraint + +from app.db.models import Base, File + +_TABLE = Base.metadata.tables["reference_edges"] + + +@pytest.mark.unit +def test_reference_edges_fk_targets_only_repos_and_files() -> None: + fk_targets = {fk.target_fullname for fk in _TABLE.foreign_keys} + assert fk_targets == {"repos.id", "files.id"}, ( + "reference_edges must never gain a symbols FK (epic #82 rule): " + f"found FK targets {fk_targets}" + ) + + +@pytest.mark.unit +def test_reference_edges_fks_cascade_on_delete() -> None: + for constraint in _TABLE.constraints: + if isinstance(constraint, ForeignKeyConstraint): + for element in constraint.elements: + assert element.ondelete == "CASCADE", ( + f"FK to {element.target_fullname!r} must be ON DELETE CASCADE" + ) + + +@pytest.mark.unit +def test_reference_edges_id_is_bigint() -> None: + assert isinstance(_TABLE.c.id.type, BigInteger) + + +@pytest.mark.unit +def test_reference_edges_not_null_columns() -> None: + required = {"repo_id", "file_id", "edge_kind", "target_name", "line"} + nullable = {"enclosing_name", "enclosing_kind", "enclosing_start_line", "enclosing_end_line"} + for name in required: + assert not _TABLE.c[name].nullable, f"{name} must be NOT NULL" + for name in nullable: + assert _TABLE.c[name].nullable, f"{name} must be nullable (module/top-level scope)" + + +@pytest.mark.unit +def test_reference_edges_edge_kind_check_constraint() -> None: + checks = [c for c in _TABLE.constraints if isinstance(c, CheckConstraint)] + assert len(checks) == 1 + check = checks[0] + assert check.name == "ck_reference_edges_edge_kind" + assert "call" in str(check.sqltext) and "import" in str(check.sqltext) + + +@pytest.mark.unit +def test_reference_edges_indexes() -> None: + indexes = {ix.name: ix for ix in _TABLE.indexes} + expected_names = { + "ix_reference_edges_target_name", + "ix_reference_edges_target_trgm", + "ix_reference_edges_file_id", + "ix_reference_edges_repo_kind", + } + assert set(indexes) == expected_names + + assert [c.name for c in indexes["ix_reference_edges_target_name"].columns] == ["target_name"] + assert [c.name for c in indexes["ix_reference_edges_file_id"].columns] == ["file_id"] + assert [c.name for c in indexes["ix_reference_edges_repo_kind"].columns] == [ + "repo_id", + "edge_kind", + ] + + trgm = indexes["ix_reference_edges_target_trgm"] + assert [c.name for c in trgm.columns] == ["target_name"] + assert trgm.dialect_options["postgresql"]["using"] == "gin" + assert trgm.dialect_options["postgresql"]["ops"] == {"target_name": "gin_trgm_ops"} + + +@pytest.mark.unit +def test_file_reference_edges_relationship_cascades() -> None: + rel = File.reference_edges + assert rel.property.cascade.delete_orphan From e645c274a9f9758655405e4d350a3ffead64071a Mon Sep 17 00:00:00 2001 From: IceRhymers Date: Thu, 23 Jul 2026 13:09:06 -0700 Subject: [PATCH 3/4] docs: add reference-edges runbook and update key-file tables (#83) New docs/runbooks/reference-edges.md: schema summary, index-to-consumer mapping, and the deploy/grant-coupling section (same shape as multi-branch.md and semantic-enablement.md) with the ADP same-role/ different-role rule and the has_table_privilege verification query. Links it from README's further-reading list. Updates app/db/AGENTS.md and app/alembic/AGENTS.md key-file tables and the 0001->0005 chain. --- README.md | 3 + app/alembic/AGENTS.md | 9 +-- app/db/AGENTS.md | 5 +- docs/runbooks/reference-edges.md | 96 ++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 6 deletions(-) create mode 100644 docs/runbooks/reference-edges.md diff --git a/README.md b/README.md index b9c2cff..9b2da8b 100644 --- a/README.md +++ b/README.md @@ -596,6 +596,9 @@ Run the server locally with `make run` (binds `DATABRICKS_APP_PORT`, else 8000). semantic search (default-on): the preload assumption, opt-out, embeddings, memory notes - [`docs/runbooks/indexing-parallelism.md`](docs/runbooks/indexing-parallelism.md) — parallel indexing: worker sizing, skip-if-unchanged, compare-and-set stamping +- [`docs/runbooks/reference-edges.md`](docs/runbooks/reference-edges.md) — the raw + call/import edge schema (`reference_edges`, migration `0005`): what it stores, the + no-symbol-FK design, and the grant-coupling this migration introduces - [`docs/runbooks/ci-lakebase.md`](docs/runbooks/ci-lakebase.md) — the integration CI gate: ephemeral Lakebase branches, prerequisites - [`docs/runbooks/webui.md`](docs/runbooks/webui.md) — the web UI app: auth, grants, diff --git a/app/alembic/AGENTS.md b/app/alembic/AGENTS.md index d15cd8c..0d6ecc5 100644 --- a/app/alembic/AGENTS.md +++ b/app/alembic/AGENTS.md @@ -4,7 +4,7 @@ # app/alembic ## Purpose -The Alembic migration environment and the single linear core revision chain (0001 → 0004) for the code-search schema on Lakebase Postgres. `env.py` resolves its connection in strict priority order: an injected connection from `scripts/migrate.py` (which owns the Lakebase OAuth engine) wins; else `LAKEBASE_ENDPOINT`/`PGHOST` builds one via `create_db_engine()` (the `make migration` autogenerate path against a disposable Lakebase branch); else it raises — there is no implicit default. Autogenerate diffs against `app.db.models.Base.metadata`, with the semantic `chunks` surface filtered out entirely. `alembic.ini` at the repo root points `script_location` here. +The Alembic migration environment and the single linear core revision chain (0001 → 0005) for the code-search schema on Lakebase Postgres. `env.py` resolves its connection in strict priority order: an injected connection from `scripts/migrate.py` (which owns the Lakebase OAuth engine) wins; else `LAKEBASE_ENDPOINT`/`PGHOST` builds one via `create_db_engine()` (the `make migration` autogenerate path against a disposable Lakebase branch); else it raises — there is no implicit default. Autogenerate diffs against `app.db.models.Base.metadata`, with the semantic `chunks` surface filtered out entirely. `alembic.ini` at the repo root points `script_location` here. ## Key Files | File | Description | @@ -20,11 +20,12 @@ The Alembic migration environment and the single linear core revision chain (000 | `0002_index_semantics_version.py` | Adds + backfills `repos.index_semantics_version`. Backfill value is FROZEN as `_BACKFILL_VERSION = 1` — deliberately never imports the live `INDEX_SEMANTICS_VERSION`. Backfills by cadence (rows touched in the last 48h); untouched rows stay NULL → re-index once | | `0003_multi_branch.py` | Multi-branch content dedup: adds `files.content_sha` (backfilled in-DB via `pgcrypto` `digest(...,'sha256')`, proven byte-identical to `indexer.hashing.content_sha`) and GIN-indexed `files.branches`; swaps `uq_files_repo_id_path` → `uq_files_repo_path_sha`; creates `repo_branches` seeded from the legacy `repos` stamp. Downgrade guards FIRST: refuses if any path has multiple content versions | | `0004_semantic_chunks.py` | The semantic surface in the core chain (supersedes the retired gated `0002sem`/`versions_semantic`): extensions `lakebase_tokenizer` → `lakebase_vector` → `lakebase_text` with `CASCADE` (load-bearing: `lakebase_vector` declares a dependency on base `vector`), the `chunks` table (embedding dim from `app.config.SEMANTIC_EMBEDDING_DIM`, generated `ts` tsvector, `uq_chunks_file_id_chunk_index` — also the write-path index for per-file DELETE and CASCADE), `ix_chunks_embedding_ann` (`lakebase_ann` with explicit non-default `vector_cosine_ops`; rejects hnsw-style WITH params) and `ix_chunks_ts_bm25`. Idempotency guard: if `to_regclass('chunks')` exists (old gated path), only add `start_line`/`end_line` and drop the orphaned `alembic_version_semantic` | +| `0005_reference_edges.py` | Adds `reference_edges` (epic #82): raw unresolved call/import edges, `edge_kind IN ('call','import')` CHECK, FKs to `repos`/`files` only (both `ON DELETE CASCADE`) — deliberately NO FK to `symbols` (query-time name-join resolution instead). Autogenerate-shaped (`op.create_table`/`op.create_index`, no raw SQL, no new extension — `pg_trgm` already exists since 0001). Four indexes: btree `target_name`, GIN trgm `target_name`, btree `file_id` (write-path/cascade), btree `(repo_id, edge_kind)`. Grant-coupled like 0003/0004 before it — see `docs/runbooks/reference-edges.md` | ## For AI Agents ### Working In This Directory -- **Chain ordering is strictly linear**: `0001 <- 0002 <- 0003 <- 0004`. A new revision sets `down_revision` to the current head; never branch the chain. +- **Chain ordering is strictly linear**: `0001 <- 0002 <- 0003 <- 0004 <- 0005`. A new revision sets `down_revision` to the current head; never branch the chain. - **Migrations are historical facts.** They must never import mutable app constants — `0002` freezes its backfill value locally, and `models.py` explicitly forbids migrations importing `INDEX_SEMANTICS_VERSION`. The one sanctioned exception is `0004`'s use of `SEMANTIC_EMBEDDING_DIM`, which exists precisely so DDL and the `app/db/semantic.py` table can never drift. - **Keep the `chunks` blindness intact.** `include_object` in `env.py` plus `chunks` living outside `Base.metadata` are two halves of one protection; removing either makes `make migration` emit destructive drops. - **Extension-before-index ordering** is load-bearing in 0001 and 0004; downgrades never drop extensions (database-wide, potentially shared, and the 0004 preload prerequisite is irreversible). @@ -33,8 +34,8 @@ The Alembic migration environment and the single linear core revision chain (000 - Run migrations via `make migrate` (`scripts/migrate.py`, injected connection, optional `ARGS=--apply-grants`); autogenerate via `make migration MSG="..."` against a disposable Lakebase branch (`scripts/ci_branch.py up`) — never against production. `env.py` deliberately raises rather than guessing a target. ### Testing Requirements -- `make test`: `tests/unit/test_migration_source.py` / `test_migration_source_semantic.py` (source-level revision-chain and no-app-import checks), `test_semantics_version_tripwire.py`. -- `make test-integration`: `tests/integration/test_migrations.py` (upgrade/downgrade against real Postgres; models ↔ chain parity), `test_content_sha_parity.py` (pgcrypto digest ≡ Python `content_sha`). +- `make test`: `tests/unit/test_migration_source.py` / `test_migration_source_semantic.py` (source-level revision-chain and no-app-import checks, including 0005's no-symbol-FK check), `test_semantics_version_tripwire.py`, `test_reference_edge_model.py` (ORM-metadata tripwires for `reference_edges`). +- `make test-integration`: `tests/integration/test_migrations.py` (upgrade/downgrade against real Postgres; models ↔ chain parity; the `reference_edges` shape/cascade/ADP/EXPLAIN tests run on stock Postgres too via `migrated_edges_capable`), `test_content_sha_parity.py` (pgcrypto digest ≡ Python `content_sha`). ### Common Patterns - Raw `op.execute()` for anything SQLAlchemy can't declare portably (extensions, generated columns, lakebase index access methods, backfill UPDATEs); `op.create_table`/`op.create_index` for the declarable rest. diff --git a/app/db/AGENTS.md b/app/db/AGENTS.md index 9d46ebf..b0258dd 100644 --- a/app/db/AGENTS.md +++ b/app/db/AGENTS.md @@ -4,13 +4,13 @@ # app/db ## Purpose -Database connectivity and schema truth for the code-search corpus. `client.py` is the one and only place that knows how to connect to Lakebase (Autoscaling API, per-connection OAuth token injection) or to plain local Postgres (CI/tests via `PGHOST`). `models.py` holds the SQLAlchemy 2.0 declarative models for the durable core (`repos` / `files` / `symbols` / `repo_branches`) whose `Base.metadata` drives Alembic autogenerate; `semantic.py` declares the `chunks` table in a deliberately separate `MetaData` so autogenerate never sees it; `grants.py` builds least-privilege GRANT SQL strings executed by `scripts/migrate.py`. +Database connectivity and schema truth for the code-search corpus. `client.py` is the one and only place that knows how to connect to Lakebase (Autoscaling API, per-connection OAuth token injection) or to plain local Postgres (CI/tests via `PGHOST`). `models.py` holds the SQLAlchemy 2.0 declarative models for the durable core (`repos` / `files` / `symbols` / `repo_branches` / `reference_edges`) whose `Base.metadata` drives Alembic autogenerate; `semantic.py` declares the `chunks` table in a deliberately separate `MetaData` so autogenerate never sees it; `grants.py` builds least-privilege GRANT SQL strings executed by `scripts/migrate.py`. ## Key Files | File | Description | |------|-------------| | `client.py` | `create_db_engine()`: Lakebase-endpoint-wins-over-`PGHOST` dual-mode selection; Lakebase mode closes one `WorkspaceClient` over a `do_connect` handler that mints a fresh OAuth token as the password on every physical connect (never logged); server defaults `pool_size=5`, `pool_recycle=2700` (45 min, under the ~1h token TTL), `pool_pre_ping=True`; SDK import is lazy so the local path never touches it | -| `models.py` | ORM models + `INDEX_SEMANTICS_VERSION` (currently 2; bump on any indexing-meaning change — CI tripwires it). `File` is content-deduped per (repo_id, path, content_sha) with a `branches ARRAY(Text)` membership column; `RepoBranch` is the authoritative per-(repo, branch) CAS stamp; `repos`' own stamp columns and `files.commit` are deprecated/ambiguous — never add readers. Declares the trgm + branches GIN indexes so autogenerate can't drift | +| `models.py` | ORM models + `INDEX_SEMANTICS_VERSION` (currently 2; bump on any indexing-meaning change — CI tripwires it). `File` is content-deduped per (repo_id, path, content_sha) with a `branches ARRAY(Text)` membership column; `RepoBranch` is the authoritative per-(repo, branch) CAS stamp; `repos`' own stamp columns and `files.commit` are deprecated/ambiguous — never add readers. `ReferenceEdge` (0005, epic #82) is a raw unresolved call/import edge, deliberately with NO FK to `symbols` (resolution happens at query time by name-join in a later child); FKs to `repos`/`files` only, both `ON DELETE CASCADE`. Declares the trgm + branches GIN indexes so autogenerate can't drift | | `semantic.py` | Standalone Core `Table` for `chunks` (BigInteger PK, `Vector(SEMANTIC_EMBEDDING_DIM)` embedding, generated `ts` tsvector, nullable `start_line`/`end_line`) in its own `semantic_metadata` — a typed description only; the real DDL is owned by migration `0004` | | `grants.py` | Pure SQL-string builders `build_app_grants` (read-only) / `build_job_grants` (CRUD + sequences, no DDL); identifiers validated against `^[A-Za-z0-9_-]+$` (1..63 chars) then psycopg-quoted. Execution lives in `scripts/migrate.py` | | `__init__.py` | Re-exports `Base`, `File`, `Repo`, `Symbol`, `create_db_engine` | @@ -23,6 +23,7 @@ Database connectivity and schema truth for the code-search corpus. `client.py` i - **Do not add `chunks` to `Base.metadata`.** It lives in `semantic.py`'s own `MetaData`; pairing with the `include_object` filter in `app/alembic/env.py`, this is what stops autogenerate emitting `drop_table('chunks')` or spurious diffs of the hand-written vector/tsvector DDL. - **`INDEX_SEMANTICS_VERSION` must never be imported by a migration** (migrations freeze their own constants — see `0002`). Bump it whenever `indexer/symbols.py`, `indexer/parse.py` chunking, or `indexer/languages.py` extraction changes meaning. - `RepoBranch.last_indexed_commit` is the ONLY commit truth-source; `files.commit` is write-only and ambiguous under multi-branch dedup. +- **`reference_edges` must never gain a FK to `symbols`** (epic #82 rule) — resolution from `target_name` to a concrete symbol happens at query time by name-join, not via a stored FK. `tests/unit/test_reference_edge_model.py` and `tests/unit/test_migration_source.py::test_0005_no_symbol_fk` are standing tripwires; do not weaken them. - The server's pool default (5) is paired with `app/main.py`'s `CapacityLimiter(5)`; the indexer passes `pool_size=` explicitly from its worker count. Changing one side without the other breaks the oversubscription guarantee. - Grant changes are deploy-coupled: adding a privilege needs a `deploy.sh --apply-grants` re-run, not just a schema migrate. diff --git a/docs/runbooks/reference-edges.md b/docs/runbooks/reference-edges.md new file mode 100644 index 0000000..3d7876f --- /dev/null +++ b/docs/runbooks/reference-edges.md @@ -0,0 +1,96 @@ +# Runbook: reference-edge schema (0005) + +What an operator needs to know about the `reference_edges` table added in migration +`0005`: what it stores (and doesn't yet), why it's grant-coupled like `repo_branches` +(`0003`) and `chunks` (`0004`) before it, and how to verify the app/job grants actually +landed on a given deploy target. + +--- + +## 1. What this table is + +`reference_edges` is a **raw, unresolved** call/import edge extracted from one file's +content-version: `(edge_kind, target_name, line, enclosing_*)` per site tree-sitter finds, +with `edge_kind IN ('call', 'import')` enforced by a CHECK constraint. It is part of the +knowledge-graph epic (#82): a later child (#86) resolves `target_name` to a concrete +`symbols` row at **query time**, by name-join — this table deliberately carries **no +foreign key to `symbols`**. Symbol ids churn on every per-file delete-and-reinsert, and an +FK would couple the two rewrite orders inside the indexing transaction for no query +benefit. + +The enclosing symbol (the function/class a call or import site sits inside, if any) is +denormalized onto the row as `enclosing_name` / `enclosing_kind` / `enclosing_start_line` +/ `enclosing_end_line`, all nullable — `NULL` means module/top-level scope, exactly the +same convention `symbols` and `chunks` use elsewhere. There is no `branches` column and no +`commit` column: branch membership rides `files.branches` at query time (the resolver +joins through `files` with the same `coalesce(default_branch,'HEAD')` conjunct used +everywhere else), and `files.commit` is documented-ambiguous under multi-branch dedup and +must gain no new readers. + +**This table is dormant until #84 ships a writer.** `0005` only creates the schema; no +code path inserts rows yet. `indexer/store.py`'s cascade-owning functions (`index_repo`'s +membership sweep, `reconcile_retired_branches`, `reconcile_removed_repos`) already +enumerate `reference_edges` alongside `symbols`/`chunks` in their docstrings and rely on +the same FK-cascade mechanism proven in §7.2 of the design doc and in +`tests/integration/test_reconcile.py` / `test_store.py` — no behavior change was needed to +make the cascade correct, because both `repos -> reference_edges` and +`files -> reference_edges` are `ON DELETE CASCADE` foreign keys. + +## 2. Indexes + +| Index | Serves | +|---|---| +| `ix_reference_edges_target_name` (btree) | The resolver's name-equality join (`symbols.name = reference_edges.target_name`) once #86 ships | +| `ix_reference_edges_target_trgm` (GIN, `gin_trgm_ops`) | Partial/substring reference lookups, parity with `ix_symbols_name_trgm` | +| `ix_reference_edges_file_id` (btree) | The per-file delete-and-reinsert writer (#84) and the `ON DELETE CASCADE` fired by the sweep/reconcile paths — Postgres does not auto-index a foreign key, and both are hot paths | +| `ix_reference_edges_repo_kind` (btree, `(repo_id, edge_kind)`) | Per-repo kind scans (e.g. a future `list_imports` MCP tool) | + +## 3. Deploy coupling — this migration is NOT schema-only + +Same shape as `repo_branches` (0003) and `chunks` (0004) before it: `reference_edges` is a +new table, and the app/job grant builders in `app/db/grants.py` are schema-wide +(`GRANT ... ON ALL TABLES IN SCHEMA` + `ALTER DEFAULT PRIVILEGES`), so no code change was +needed for them to cover it. Whether a grant re-run is actually **required** after `0005` +depends on Postgres's `ALTER DEFAULT PRIVILEGES` (ADP) semantics: + +- **ADP binds to the role that executed it**, not to the schema. `scripts/deploy.sh full` + (`make deploy`) runs both the migrate step and the grants step as the same deploying + identity, so on a fresh deploy the app/job roles get `SELECT` / `INSERT,UPDATE,DELETE` + on `reference_edges` automatically the moment it's created — **no re-grant needed**. +- **A schema-only `make migrate TARGET=` run by that SAME identity** against an + already-deployed target is also covered automatically, for the same reason. +- **A schema-only migrate run by a DIFFERENT identity** than the one that originally ran + `ALTER DEFAULT PRIVILEGES` is **not** covered — ADP simply never fires for that role, so + the app/job roles get nothing on the new table. + +This is proven in CI (`tests/integration/test_migrations.py`: +`test_reference_edges_adp_same_role_covers_new_table` and +`test_reference_edges_adp_different_role_does_not_cover_new_table`), not assumed. + +**Always deploy this with `scripts/deploy.sh full` (i.e. `make deploy`) or, for an +already-deployed target migrated by a different identity, re-run the grants step +explicitly:** + +``` +APP_SP_ROLE= JOB_WRITER_ROLE= \ + make migrate TARGET= ARGS=--apply-grants +``` + +### Verifying the grant landed + +Run as the deploying identity (or any role with `SELECT` on `pg_catalog`) against the +target: + +```sql +SELECT has_table_privilege('', 'reference_edges', 'SELECT'), + has_table_privilege('', 'reference_edges', 'INSERT'); +``` + +Both must return `true`. If either is `false`, run the re-grant command above — it is +idempotent, safe to run against a target that's already current. + +## Reference + +- [multi-branch.md §3](multi-branch.md#3-deploy-coupling--this-migration-is-not-schema-only) — + the same grant-coupling pattern for `repo_branches` (0003). +- [semantic-enablement.md](semantic-enablement.md) — the same pattern for `chunks` (0004). From dd19a2ce490203a83ffff9d18a2a6cc19b3faa1b Mon Sep 17 00:00:00 2001 From: IceRhymers Date: Thu, 23 Jul 2026 13:17:21 -0700 Subject: [PATCH 4/4] Address review pass 1: quote-agnostic FK tripwire, re-export ReferenceEdge, assert job role has no DDL Fresh code-reviewer and security-reviewer passes both returned APPROVE with no blockers/majors; three low-cost nits/informational findings addressed: - test_0005_no_symbol_fk now matches symbols.id via regex instead of a quote-literal substring, so it survives formatter drift. - app/db/__init__.py re-exports ReferenceEdge alongside the other models. - The ADP same-role grant test now also asserts the job role's grants are DML-only (TRUNCATE raises InsufficientPrivilege), matching build_job_grants' least-privilege intent. --- app/db/__init__.py | 4 ++-- tests/integration/test_migrations.py | 10 +++++++++- tests/unit/test_migration_source.py | 15 ++++++++++++--- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/app/db/__init__.py b/app/db/__init__.py index 7bb1b80..d03f760 100644 --- a/app/db/__init__.py +++ b/app/db/__init__.py @@ -1,6 +1,6 @@ """Database connectivity and ORM models for the code search service.""" from app.db.client import create_db_engine -from app.db.models import Base, File, Repo, Symbol +from app.db.models import Base, File, ReferenceEdge, Repo, Symbol -__all__ = ["Base", "File", "Repo", "Symbol", "create_db_engine"] +__all__ = ["Base", "File", "ReferenceEdge", "Repo", "Symbol", "create_db_engine"] diff --git a/tests/integration/test_migrations.py b/tests/integration/test_migrations.py index 6cae61a..11a6d63 100644 --- a/tests/integration/test_migrations.py +++ b/tests/integration/test_migrations.py @@ -973,7 +973,15 @@ def test_reference_edges_adp_same_role_covers_new_table() -> None: {"r": repo_id, "f": file_id}, ) conn.execute(text("DELETE FROM reference_edges")) - conn.execute(text("RESET ROLE")) + + # Negative proof: the job role's grants are DML-only (SELECT/INSERT/ + # UPDATE/DELETE) -- no DDL, matching build_job_grants' least-privilege + # intent. A future accidental widening of that builder should fail here. + # (The failed TRUNCATE aborts the tx; rollback below also undoes SET ROLE, + # same as test_grant_enforcement_via_set_role.) + with pytest.raises(ProgrammingError) as excinfo: + conn.execute(text("TRUNCATE reference_edges")) + assert isinstance(excinfo.value.orig, psycopg.errors.InsufficientPrivilege) conn.rollback() finally: conn.rollback() diff --git a/tests/unit/test_migration_source.py b/tests/unit/test_migration_source.py index 75cca62..cf9fd82 100644 --- a/tests/unit/test_migration_source.py +++ b/tests/unit/test_migration_source.py @@ -13,6 +13,7 @@ from __future__ import annotations +import re from pathlib import Path import pytest @@ -203,9 +204,17 @@ def test_0005_downgrade_drops_indexes_then_table(source_0005: str) -> None: @pytest.mark.unit def test_0005_no_symbol_fk(source_0005: str) -> None: - """Epic #82 rule: reference_edges must never gain a symbols FK at the source level.""" - for forbidden in ('ForeignKey("symbols', "REFERENCES symbols", '"symbols.id"'): - assert forbidden not in source_0005, f"0005 migration must not reference {forbidden!r}" + """Epic #82 rule: reference_edges must never gain a symbols FK at the source level. + + Quote-agnostic (matches ``symbols.id`` under either quoting style) so this + tripwire survives formatter drift, unlike a literal-substring check. + """ + assert re.search(r"symbols\.id", source_0005) is None, ( + "0005 migration must not reference a symbols.id FK target" + ) + assert "REFERENCES symbols" not in source_0005, ( + "0005 migration must not reference symbols via raw SQL REFERENCES" + ) @pytest.mark.unit