Add reviewable person identities that survive re-indexing (#85) - #149
Add reviewable person identities that survive re-indexing (#85)#149ik020 wants to merge 2 commits into
Conversation
- PersonRecord/PersonAlias/PersonReference/PersonClusterLink domain models - CRUD layer in sql_catalog.py, schema version bumped to 5 - Alembic migration 20260803_01 for existing/Postgres catalogs - Regression tests covering reindex-survival and non-destructive delete - Update test_database_cli.py and test_media_catalog.py for new head/schema version - Fix database_cli.py to ensure local directories exist before migrating
Fresh environments hit 'unable to open database file' because upgrade_database() assumes the target directory tree already exists.
tulayha
left a comment
There was a problem hiding this comment.
Thanks for taking on a broad issue. This PR establishes a useful persistence foundation with the person models, database tables, migration, and catalog operations.
Before merging, could we either narrow this to a foundational PR or complete the application-level workflow? Issue #85 includes management through Desktop, CLI, HTTP, and MCP, plus integration with search and evidence. Currently the behavior is only available by calling LocalCatalog directly.
The contributor guide keeps shared behavior in the application or control plane so every adapter uses the same validation and workflow rules. A people service or application contract would give the remaining surfaces a common boundary. If those pieces will follow separately, this PR should not close #85 yet.
| "person_id", | ||
| String(32), | ||
| ForeignKey("people.person_id", ondelete="CASCADE"), | ||
| primary_key=True, |
There was a problem hiding this comment.
Could we enforce one reviewed person per cluster identity?
Because person_id is part of this primary key, the same (cluster_id, media_id, generation_id) can be linked to two people. person_for_cluster() expects exactly one result, so that state would make lookup fail with MultipleResultsFound.
A unique constraint on the cluster, media, and generation combination, together with an explicit reassignment operation, would keep the schema and lookup behavior consistent.
| alias=alias, | ||
| ) | ||
| ) | ||
| except IntegrityError: |
There was a problem hiding this comment.
Can we avoid treating every IntegrityError as a successful duplicate?
For example, adding an alias for a nonexistent person fails the foreign-key constraint, but this method returns normally even though nothing was saved. The same pattern appears in link_cluster().
It would be safer to confirm that the existing row represents the intended idempotent request and re-raise other failures. This would also be a good place to use PersonAlias, or otherwise validate the raw inputs, so SQLite and PostgreSQL enforce the same rules.
| with self._write_transaction() as connection: | ||
| existing = self._person_by_id(connection, record.person_id) | ||
| if existing is not None: | ||
| if existing != record: |
There was a problem hiding this comment.
One scope clarification: updating an existing person display name, notes, or biography is optional for this PR because #85 does not explicitly require an update operation.
If you would like to include editing here, an explicit update or replace operation would be appropriate. Otherwise, we will track it separately so this PR can stay focused.
Please still make the list ordering deterministic. list_people() paginates using only created_at, while the alias, reference, and cluster lists have no explicit ordering.
| alias: str = Field(min_length=1, max_length=255) | ||
|
|
||
|
|
||
| class StagedPersonReference(_PersonModel): |
There was a problem hiding this comment.
PersonAlias, StagedPersonReference, and StoredPersonReference are not used by the implementation. The catalog accepts raw alias values, and no reference-image store produces the staged or stored reference types. This leaves two representations of the same data without a clear authority.
Could we either wire these models through the catalog and a reference-image storage workflow in this PR, or remove them until that workflow is implemented? The storage workflow can reuse LocalObjectStore to publish, verify, resolve, and delete the image bytes rather than only recording caller-provided metadata.
| ) | ||
| catalog.link_cluster(link_before_reindex) | ||
|
|
||
| # Simulate re-indexing: a new generation produces a |
There was a problem hiding this comment.
Could we strengthen these tests to cover the real workflows?
This test manually inserts a second link, so it proves that two links can coexist but does not exercise reindexing. The deletion test also uses random media and generation IDs without creating underlying records, so it cannot verify that those records remain.
It would help to add a real reindex or generation-replacement test, create real related data for the deletion assertion, and cover the conflicting case where the same cluster identity is assigned to two people. Migration assertions for the important uniqueness and foreign-key constraints would also protect the duplicated schema definitions from drifting.
| mode=ApplicationMode.server, | ||
| runtime_backend="cpu", | ||
| ) | ||
| settings.layout.ensure_local_directories() |
There was a problem hiding this comment.
Could we remove this change from the PR or add a focused test showing the failure it fixes?
main() selects server mode and therefore uses the bundled PostgreSQL URL, so creating local repository directories does not appear to address an SQLite parent-directory error. Keeping this separate would make the people change easier to review and avoid an unrelated filesystem side effect.
Closes #85
What this does
Adds a repository-level "reviewed person" identity that is independent of
any single index generation. A
PersonRecordis a durable, user-approvedidentity; a
PersonClusterLinkis evidence tying that person to a specificanonymous actor cluster produced by one media item + one index generation.
This is the core guarantee the issue asked for: when a video is re-indexed
and produces a brand-new anonymous cluster ID under a new generation ID,
the person, their aliases, and their reference images all survive untouched,
and lookup by the new cluster still resolves to the same person.
What's included
core/people.py,core/identifiers.py):PersonRecord,PersonAlias,PersonReference,PersonClusterLink, plusPersonId/PersonReferenceId.people,person_aliases,person_references,person_cluster_linkstables, registered inlocal_catalog.py. Schemaversion bumped 4 → 5.
infrastructure/sql_catalog.py): put/get/list/delete for people,add/remove/list for aliases and reference images, link/unlink/query for
cluster links.
20260803_01_reviewed_people.py): brings anexisting SQLite catalog or a PostgreSQL deployment up to the same schema,
for anyone not creating a catalog fresh via
LocalCatalog.create_all().Verified column-for-column against the actual read/write code in
sql_catalog.pybefore merging, not just against the tableCREATEstatements.
tests/test_people_catalog.py, 12 cases), including:test_cluster_links_survive_reindexing_with_a_new_generation— thescenario from the issue: link a person to a cluster, re-index (new
generation, new cluster ID), confirm the person and old link survive,
confirm the new link resolves correctly.
test_deleting_a_person_cascades_aliases_references_and_links_only—confirms removing a person cleans up only their own aliases/references/
links, and never touches the underlying video/media/index data.
test_unlink_cluster_corrects_an_accidental_merge_without_touching_media— confirms a bad cluster link can be corrected without side effects.
tests/test_database_cli.pyandtests/test_media_catalog.py,which hardcoded the previous migration head and schema version.
database_cli.py:upgrade_database()previously assumed itstarget directory already existed and failed with
unable to open database fileon a fresh environment. It now callssettings.layout.ensure_local_directories()first.Verification
pytest tests/test_people_catalog.py tests/test_database_cli.py tests/test_media_catalog.py— all green.20260803_01end-to-end against a real local SQLite catalog(not just an in-memory test DB) and confirmed the resulting tables and
alembic_versionby inspection.pytestsuite run for regressions outside the touched area: theonly failures are pre-existing, unrelated missing optional dependencies
in this dev environment (
chromadb,uvicorn,PIL,av,mcp,fastapi,streamlit,srt,cryptography) — confirmed by reproducingthe same failures with this branch's changes stashed out.
Not yet verified - needs reviewer follow-up
This PR was validated entirely against the local SQLite path
(
ApplicationMode.local). The PostgreSQL /ApplicationMode.serverpath has not been exercised - I don't have network access to the
bundled Postgres service from this environment. Someone with access to
that deployment should run the migration there and confirm it applies
cleanly before this is considered fully verified end-to-end.