feat: data model and migrations - #2
Merged
Merged
Conversation
…c-settings M1 dependencies for the data layer: - sqlalchemy>=2.0 (typed Mapped[] models) - psycopg[binary]>=3.2 (driver matching the postgresql+psycopg URL in CI) - alembic>=1.13 (schema migrations) - pgvector>=0.3 (Vector column type for embeddings) - pydantic-settings>=2.6 (config loaded from env)
…ing fields Settings is loaded from env vars (and a local .env if present) and exposed via a cached get_settings() accessor. Includes the M1 database URL, the pgvector embedding dimension (1536 by default, matching text-embedding-3-small), and forward-looking fields for embedding/LLM keys and retrieval thresholds consumed from M2+.
Engine and session factory are built lazily so test fixtures can override env vars before construction. The pgvector type adapter is registered on every new connection, guarded by a pg_type probe so the engine works against a fresh DB before migrations have created the vector extension. reset_engine_for_tests() lets fixtures dispose and rebuild the engine between cases. Adds an mypy override for pgvector.* (no py.typed marker upstream).
…low_items, audit_events Five aggregate models with typed Mapped[] columns and a stable constraint-naming convention on Base.metadata so Alembic autogenerate diffs are deterministic. - Document: id, hash (unique), source, title, mime_type, timestamps. - Chunk: id, document_id (FK ON DELETE CASCADE), ord, text, token_count, embedding (pgvector Vector(EMBEDDING_DIM); nullable so M1 can insert chunks without vectors). Unique index on (document_id, ord). - Extraction: id, document_id, schema_name, payload (JSONB), per-field confidence and citations as JSONB sidecars, model_name, created_at. - WorkflowItem: id, extraction_id, status (StrEnum: auto_approved/needs_review/rejected, native SQL enum 'workflow_status'), reason, idempotency_key (unique), timestamps. - AuditEvent: id, ts (indexed), actor, action, target_type/id, before/after JSONB, request_id (indexed). Composite index on (target_type, target_id) for per-target queries. EMBEDDING_DIM is captured at import time from settings.embedding_dim. Future changes to the dimension require a new migration to alter the column type.
alembic.ini sits at the repo root; script_location=backend/alembic and prepend_sys_path=. so env.py can import backend.app.* without packaging tricks. env.py reads the SQLAlchemy URL from the same Settings the app uses (single source of truth for DATABASE_URL) and runs against the engine in backend/app/db.py. The initial migration (0001_initial_schema) creates the vector extension *first*, then the workflow_status enum, then the five M1 tables in FK-dependency order with all naming-convention-compliant indexes and constraints. Verified locally: - alembic upgrade head: applies cleanly on a fresh DB - alembic upgrade head (re-run): no-op - alembic downgrade base: drops tables, enum, and extension - alembic upgrade head (after downgrade): restores everything
One module per aggregate (documents, chunks, extractions, workflow_items, audit_events) exposing thin functions that take an SA Session. Transaction boundaries are owned by the caller (FastAPI dep, ingestion pipeline, etc.). audit_events.py exposes append() and read helpers only — there is intentionally no update or delete path, satisfying the M1 invariant. An introspection test (added next) enforces this surface so it cannot be quietly broken later.
…ma, append-only tests conftest.py supplies a session-scoped engine fixture (with reset_engine_for_tests so DATABASE_URL changes between processes are picked up) and a per-test session that joins the outer connection's transaction via 'create_savepoint', so any commits inside a test are rolled back at the end and tests are fully isolated. - test_schema.py: introspects the migrated DB and asserts five tables, the vector extension, the workflow_status enum's value order, the chunks.embedding column type is vector(1536), and alembic_version is at 0001_initial_schema. This satisfies the test side of DoD #1. - test_models.py: round-trip + constraint coverage (unique hash, unique (doc_id, ord), cascade delete from documents, JSONB round-trip on extractions and audit_events, enum + idempotency_key constraint on workflow_items). - test_repositories.py: exercises every public function in each repo module. - test_audit_events_append_only.py: behavioural tests for append/list ordering plus an introspection test that fails if any future change adds a public name matching forbidden mutator patterns (update*, delete*, remove*, set*, etc.). Also fixes WorkflowStatus persistence: SAEnum now uses values_callable so the DB stores the enum *value* (e.g. 'needs_review'), not the Python *name* ('NEEDS_REVIEW') — the latter was rejected by the workflow_status PG enum type.
Adds 'uv run alembic upgrade head' between mypy and pytest. The Postgres service container starts empty, so this step explicitly verifies M1 DoD #1 ('make migrate applies cleanly on a fresh DB; pgvector extension enabled') in CI on every PR.
Owner
Author
|
@codex review Review this PR strictly as M1 — Data model + migrations. PR: Do not modify files. Read first:
Review against M1 scope only:
Check for:
Output only:
Be strict. No praise. No generic review comments. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 520db844f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # Pin the embedding dimension at import time. Changing EMBEDDING_DIM later requires a | ||
| # migration to alter the column type — the value baked into the M1 migration is canonical. | ||
| EMBEDDING_DIM: int = get_settings().embedding_dim |
There was a problem hiding this comment.
When EMBEDDING_DIM is set to a non-default value such as 1024 for the documented voyage option, Base.metadata is built with Vector(1024) while the M1 migration always creates chunks.embedding as vector(1536). That creates Alembic autogenerate drift and lets M2 produce embeddings that the migrated database will reject, so the ORM dimension needs to be tied to the migration or the config must fail early unless it matches the schema.
Useful? React with 👍 / 👎.
Comment on lines
+75
to
+77
| def get(session: Session, event_id: int) -> AuditEvent | None: | ||
| """Look up a single event by id (read-only).""" | ||
| return session.get(AuditEvent, event_id) |
There was a problem hiding this comment.
This read helper returns a live, session-tracked AuditEvent, so any caller that fetched an event through the repository can mutate fields or call session.delete(event) and commit, which is still an update/delete path despite the absence of mutator-named functions. For the M1 append-only audit invariant, repository reads should not hand out mutable persistent audit rows without additional enforcement.
Useful? React with 👍 / 👎.
Owner
Author
|
@codex review Re-review PR #2 after commit 92a078c. Focus ONLY on the two previously reported findings and their fixes. Previously reported issues:
Verify:
Output only:
Be strict. |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
9 tasks
div0rce
added a commit
that referenced
this pull request
May 28, 2026
* docs(progress): record M1 merged (PR #2) and mark M2 in progress * chore(deps): add tiktoken for token-based chunking tiktoken supplies the cl100k_base BPE encoder that text-embedding-3-* uses, so chunk boundaries align with the embedding tokenizer. * feat(config): add 'fake' embeddings provider and chunking knobs - embeddings_provider Literal extended to {openai, voyage, fake}; CI/tests use 'fake'. - openai_embedding_model exposed for explicit pinning (default text-embedding-3-small). - chunk_size_tokens (default 512) and chunk_overlap_tokens (default 64) configure the sliding-window chunker arriving in this milestone. - .env.example mirrors the additions. * feat(embeddings): provider Protocol, FakeEmbedder, OpenAI provider, factory - base.py: EmbeddingProvider Protocol with .dim and .embed(texts). - fake.py: deterministic SHA-256-stretched-then-L2-normalized vectors of SCHEMA_EMBEDDING_DIM length. Same input → same vector. No API calls. - openai_provider.py: thin httpx POST against /v1/embeddings, no SDK dep. Defensively sorts results by index and validates output dim against the configured dim. - __init__.py: get_embedder(settings) factory dispatches to fake|openai (voyage raises NotImplementedError for now). Raises before any work if runtime EMBEDDING_DIM does not match SCHEMA_EMBEDDING_DIM, so dimension drift is loud and deterministic. httpx promoted from dev to runtime deps for the OpenAI provider; FakeEmbedder itself does not need it. * feat(chunking): tiktoken-based sliding-window chunker chunk_text(text, *, chunk_size_tokens, chunk_overlap_tokens) returns deterministic ChunkOut(ord, text, token_count) records using the cl100k_base encoder (matches text-embedding-3-*). Window advances by (size - overlap); overlap >= size raises. Empty input produces no chunks; short input produces one. The encoder is cached via lru_cache so the BPE table loads once per process. * feat(ingest): hash-keyed idempotent document ingestion + CLI ingest_document(session, text, source, ...) is the unit operation: - Compute SHA-256 of canonical UTF-8 text → content hash. - If a document with that hash exists → return status='skipped', no chunks added. - Else: insert document, chunk via tiktoken, embed via the injected provider, bulk-insert chunks. Returns status='ingested' with chunk_count. ingest_path(path, embedder) walks supported files (.txt, .md) in sorted order, opens a session per file, and commits after each so a partial failure mid-corpus does not lose completed documents. CLI: python -m backend.app.ingest --path <dir|file> reads settings.embeddings_provider to pick fake|openai, prints per-file +/= flags and a totals line, exits 0 on success. Wired by 'make seed'. * feat(corpus): synthetic sample corpus + deterministic generator scripts/gen_synthetic_corpus.py uses a seeded random.Random to produce 14 fictional markdown documents (5 invoices, 5 incident reports, 4 policy memos) under data/sample/. Re-running the script on a clean tree produces byte-identical output, so the committed corpus is reproducible. All content is synthetic — fictional vendors, IDs, dates, and dollar values. data/sample/ README.md and the in-document boilerplate make this explicit. * test(m2): chunking determinism, FakeEmbedder, ingestion idempotency - test_chunking.py: determinism across runs, advance-by-(size-overlap), overlap yields shared content, overlap=0 yields disjoint token coverage (verified by total token count, not by decoded text — repeating inputs can decode identically), invalid config (overlap >= size, size <= 0, overlap < 0) raises, ChunkOut is frozen. - test_embeddings_fake.py: Protocol satisfaction, default dim matches schema, identical inputs yield identical vectors, vectors are L2-normalized, factory dispatch for fake and openai, factory raises early on dim mismatch and missing API key, voyage raises NotImplementedError. - test_ingest.py: SHA-256 hash stability and lack of whitespace normalization; ingest_document creates a doc and chunks with embeddings of SCHEMA_EMBEDDING_DIM length; **re-ingesting the same content is a no-op** (status='skipped', no new chunks) even from a different source path; different content creates separate documents; empty input creates a document with zero chunks. * ci: set EMBEDDINGS_PROVIDER=fake and run `make seed` after pytest EMBEDDINGS_PROVIDER=fake at the job level guarantees no live API calls anywhere in CI (DoD #3 of M2). The `make seed` step runs after pytest against the empty Postgres service container, exercising the full ingest pipeline end-to-end: sample corpus → token chunker → FakeEmbedder → INSERT into chunks with vectors. That satisfies M2 DoD #1 (`make seed` ingests the synthetic corpus; chunks populated with embeddings) on every PR. * docs(progress): mark M2 complete on branch with DoD verification and decision log * fix: preserve unicode provenance in chunking
This was referenced May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Milestone
M1 — Data model + migrations
Summary
Persistent layer for Sentinel: SQLAlchemy 2.x typed
Mapped[]models for the fiveaggregates (
documents,chunks,extractions,workflow_items,audit_events),the
chunks.embeddingcolumn asVector(1536)via pgvector, the workflow status as anative PG enum, an Alembic config + initial migration that creates the
vectorextension and all tables in dependency order, functional repositories with an
intentionally append-only
audit_eventsmodule, and a DB-backed test suite (25 tests)that runs against the CI Postgres + pgvector service container.
Definition of Done
make checkpasses (ruff + ruff-format + mypy strict + pytest)audit-events append-only invariant
PR chore: scaffold project, tooling, and CI #1 link; decision log seeded
M1 DoD verification (from MILESTONES.md)
make migrateapplies cleanly on a fresh DB; pgvector extension enabled.Verified locally against Postgres.app (pgvector 0.8.1) on a dedicated
sentinel_m1_localDB:
alembic upgrade headis clean, idempotent (re-run is a no-op), and fully reversible(
downgrade basedrops tables, theworkflow_statusenum, and thevectorextension;re-
upgrade headrestores everything). CI re-verifies this via an explicituv run alembic upgrade headstep against thepgvector/pgvector:pg16service container,immediately before pytest.
backend/tests/conftest.pyopens a session-scoped engine and yields a per-test sessionusing
join_transaction_mode="create_savepoint"so each test runs inside a SAVEPOINT andis rolled back at the end — even if test code commits. The schema is at head before the
first test runs (CI does so explicitly; local dev does so via
make migrate).audit_eventshas no update/delete path in the repository layer. Two-prongedenforcement:
backend/app/repositories/audit_events.py) exposes onlyappend,list_for_target,list_by_request_id, andget.test_audit_events_append_only.py::test_module_public_surface_has_no_mutatorsfailsif any future change adds a public symbol matching
update*,delete*,remove*,set*,purge*, etc.Notes / decisions
See PROGRESS.md "Decision log" — three entries logged for M1:
vector(1536)(matches OpenAItext-embedding-3-small,the planned M2 default). Switching providers requires a new migration.
WorkflowItem.statuspersists the enum value (needs_review), not the Python name,via
SAEnum(values_callable=…). Caught immediately by the test suite when missing.Session); the caller owns transaction boundaries.Other deliberate choices:
Base.metadatauses a constraint-naming convention so future Alembic autogeneratediffs are deterministic.
connectevent on the engine,guarded by a
pg_typeprobe so the engine remains importable against a fresh DB beforemigrations have created the
vectorextension.pgvector.*(nopy.typedmarker upstream).Test coverage
test_schema.py(5 tests): five tables present;vectorextension installed;workflow_statusenum has values[auto_approved, needs_review, rejected];chunks.embeddingisvector(1536);alembic_versionis at0001_initial_schema.test_models.py(8 tests): Document round-trip + unique hash, Chunk requires document(document_id, ord)+ cascade delete, Extraction JSONB round-trip,WorkflowItem enum + idempotency_key constraint, AuditEvent JSONB before/after round-trip.
test_repositories.py(7 tests): every public function in each repo (documents, chunks,extractions, workflow_items).
test_audit_events_append_only.py(4 tests): append + JSONB round-trip; oldest-firstordering by target and by request_id; introspection test against forbidden mutators.
Reminder
Please squash-merge this PR (Golden Rule #2: one milestone = one squash-merge commit
on
main).