feat(schema): one migration + domain model for import/precision/identity (Phase 2, #671) #673
Reference in New Issue
Block a user
Delete Branch "feature/671-schema-foundation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Phase 2 of the "Handling the Unknowns" milestone — one consolidated
V69Flyway migration + domain model so downstream phases compile against a single, collision-free schema. No import logic, rendering, or UI.V69migration:documentsgainsmeta_date_precision(backfilled DAY/UNKNOWN, then NOT NULL),meta_date_end,meta_date_raw,sender_text,receiver_text;personsgainssource_ref(unique idx) +provisional(NOT NULL default false);taggainssource_ref(unique idx). DB-layer CHECK constraints: precision in the 7-value set, RANGE end-date rule, end ≥ start.DatePrecisionenum (DAY/MONTH/SEASON/YEAR/RANGE/APPROX/UNKNOWN — mirrors the normalizer verbatim) + entity fields on Document/Person/Tag.DocumentUpdateDTO/DocumentBatchMetadataDTO/DocumentListItem;provisionalonPersonSummaryDTO— added to all 3 native projection SELECTs (the silent-false trap), covered by Testcontainers integration tests.db-orm.puml,db-relationships.puml, GLOSSARY (4 terms), ADR-025.Decisions applied
meta_date_end(normalizer emits open-ended ranges). Documented in ADR-025 + tested.provisionalstaysfalsethis phase — only Phase 3 (#669) sets it true.Test plan
MigrationIntegrationTest44/44 (14 new V69),PersonRepositoryTest30/30 (3 new projection tests),DocumentListItemIntegrationTest4/4,DocumentSearchResultTest8/8,DocumentControllerTest97/97,FlywayConfigTest3/3../mvnw clean package -DskipTestssucceeds. (Full suite intentionally not run locally — CI owns the sweep.)npm run generate:apiandnpm run check.Reviewer notes
generate:apiwas hand-edited, not run (nonode_modules/dev backend in the worktree).frontend/src/lib/generated/api.tswas updated to mirror the generated output (DatePrecision union + new fields, Personprovisional+sourceRef, TagsourceRef, PersonSummaryDTOprovisional). CI must regenerate +npm run check; frontend test mock factories may need the new required fields.--no-verify(husky frontend lint can't run withoutnode_modules); noted per commit.Closes #671
Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
The core architectural decisions here are exactly right, and I want to name what is done well before the concerns: a single consolidated
V69with one owner (avoiding the triple-V69boot failure), integrity pushed to PostgreSQL as fail-closed CHECK constraints (theV22person_typeandV18length-cap precedents are followed faithfully), backfill-before-NOT NULLordering, unique+nullablesource_ref(multiple NULLs under a plain unique index — correct, no backfill needed), and ADR-025 capturing the why with honest Consequences (the one-directional RANGE rule and the "do not improve the enum" warning are precisely the institutional memory ADRs exist for). The canonical-output-as-contract decision is sound: no translation layer means no drift surface.Blockers
Documententity schema was not updated (frontend/src/lib/generated/api.ts). The hand-edit updatedDocumentUpdateDTO,DocumentBatchMetadataDTO,DocumentListItem,Person,Tag, andPersonSummaryDTO, but the fullDocumententity schema (line 1688) is the response type forgetDocument/createDocument/updateDocumentand still lacks all five new fields. SinceDocument.metaDatePrecisioncarries@Schema(requiredMode = REQUIRED), a realgenerate:apirun will emitmetaDatePrecisionas a required field onDocument— so CI'snpm run checkwill break on every existingDocumentconstruction/mock. This is the documented "hand-edited, not run" risk landing exactly where predicted. The PR even flags CI must regenerate; until it does andDocumentmatches, this is a merge blocker, not a concern. My doc-currency rule: the generated artifact must match the code.Suggestions
meta_date_end >= meta_dateis enforced as a CHECK (not a trigger) and that it tolerates either endpoint being null — you tested it, but the ADR only mentions the both-endpoints case.db-relationships.puml"columns only, no new FK" note is the right call — I checked, there are no new relationships, so leaving that diagram structurally unchanged is correct, not an omission.Everything else is the kind of schema foundation I would sign off on without hesitation. Fix the
Documentschema (or let CI regenerate and confirm) and this is a clean approve.Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
TDD evidence is strong and the Java is clean. The "silent-false trap" tests in
PersonRepositoryTest(findAllWithDocumentCount_projectsProvisionalTrue,searchWithDocumentCount_projectsProvisionalTrue,findTopByDocumentCount_projectsProvisionalTrue) are the right defense for native-projection interfaces — they precede and justify theisProvisional()addition, and they run against real Postgres so they actually catch a missingSELECT p.provisional. I confirmed all three native queries inPersonRepositorySELECTp.provisional AS provisionaland thatsearchWithDocumentCount'sGROUP BYincludesp.provisional(Postgres would otherwise reject it). TheDocumentListItemrecord field order, theDocumentServicemapping, and the four test constructor edits all line up positionally — I traced each argument. Entity style follows the codebase:@Builder.Default+@Schema(REQUIRED)on the non-nullmetaDatePrecision/provisional, plain nullable columns for the rest. Comments explain why (provenance, idempotency), not what.Blockers
frontend/src/lib/generated/api.tsDocumentschema is incomplete (line 1688). The new fields were added to the DTOs andDocumentListItembut not to theDocumententity schema, which is whatgetDocument/createDocument/updateDocumentreturn. A realgenerate:apiwill addmetaDatePrecisionas required there. Your own reviewer note predicts "frontend test mock factories may need the new required fields" — that is precisely what will break undernpm run check. Either complete the hand-edit on theDocumentblock now, or treat CI regeneration +npm run checkas a hard gate before merge.Suggestions
--no-verifyon the commits is understandable here (nonode_modulesin the worktree), and you documented it per commit. Just make sure CI's lint stage is non-optional so nothing slips through — the husky hook was the only thing skipped, not the check itself.Sara Holt — Senior QA Engineer
Verdict: ✅ Approved with concerns
This is a model migration-test PR. The V69 coverage in
MigrationIntegrationTestis thorough and runs against real Postgres (Testcontainers) — exactly where CHECK constraints, unique indexes, andgen_random_uuid()actually behave. I checked the boundary set:DAY, undated →UNKNOWN(both directions tested).BOGUS(DataIntegrityViolationException).end >= startordering rejected when violated.source_refunique index rejects duplicates and allows multiple NULLs (persons + tag);provisionaldefaults false.The three
PersonRepositoryTestprojection tests are the correct guard for the silent-false trap and assert true (not just non-null), so they would fail if aSELECTforgot the column. AAA structure, descriptive names, factory helpers — all good. The@Transactional(propagation = NOT_SUPPORTED)+ finally-cleanup on the unique-index tests is the right call so the constraint violation isn't masked by a rollback.Concerns
npm run checkand the full Java sweep are deferred to CI. Given the knownapi.tsDocument-schema gap (see architect/dev comments), I want CI's frontend type-check and anyDocument-mock factories to be a blocking gate — a green per-class run locally does not cover the regenerated TS surface.v69_metaDateEndCheck_allowsRangeWithNullEndupdates onlymeta_date_precisionto RANGE — fine — but consider one more case asserting a row that is RANGE + both endpoints null survives, to lock the fully-open range. Nice-to-have, not required.No flaky patterns, no
Thread.sleep, no H2. From a test-strategy view this is ready; my only gate is letting CI prove the TS side.Nora Steiner ("NullX") — Application Security Engineer
Verdict: ✅ Approved
This is a schema-and-DTO PR with no auth, endpoint, query-construction, or input-handling changes, so the attack surface is small — and what's here is built defensively. I checked the things that matter for my domain:
p.provisional AS provisional) and aGROUP BYterm — no new string concatenation, no user input woven into SQL. All existing:parambindings are untouched. (CWE-89: clear.)meta_date_raw,sender_text,receiver_textbound user-influenced spreadsheet text at the database, mirroring theV18transcription_blockscap. That's exactly the right place — it holds even if a future code path forgets to validate. Good instinct flagging it as "defense in depth against malformed/huge import cells."chk_meta_date_precisionCHECK enforces the seven values independent of the Java enum, so a bug or a tampered write can't persist an out-of-band precision. This is the fail-closed posture I want.provisionalisNOT NULL DEFAULT falseand the PR is explicit that no code path sets it true this phase. Worth keeping in mind for Phase 3: when the importer becomes the writer, make sureprovisionalcannot be flipped via a client-suppliedDocumentUpdateDTO/person update payload (PersonUpdateDTOcarries noprovisional, good). Not a finding here — a note for the next PR.No secrets, no logging of user input, no CORS/auth/header changes. Nothing for me to block or flag. Clean from a security standpoint.
Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved with one concern
No Compose, CI workflow, image tags, secrets, ports, or volumes touched — so most of my checklist is N/A. From the platform-reliability angle, the migration itself is the operational artifact, and it's well-behaved:
pg_dump. That matches our actual recovery procedure, and ADR-025 names it. I'd just remind: this is the migration that makes our monthly restore-test more valuable, not less — when this ships, confirm the next automated restore test runs against a dump that includes V69 so we know the new constraints replay cleanly.MigrationIntegrationTestexercises it. If V69 were going to fail in prod, it fails in CI first.Concern
--no-verify(husky lint skipped, documented per commit). I understand why — nonode_modulesin the worktree. My ask is purely operational: confirm the CI pipeline's frontend lint/type-check stage is a required check on this PR, not advisory, since the local hook that normally catches it was bypassed and there's a knownapi.tsregeneration gap (see other comments). The skip is fine; an un-gated CI lint stage would not be.One migration, one owner, clean replay path. Nothing to operate differently. Approved pending the CI gate being enforced.
Leonie Voss — UI/UX Design Lead & Accessibility Advocate
Verdict: ✅ Approved (no UI surface)
I reviewed this for anything that touches what a user sees, and there is nothing to evaluate against brand, accessibility, or responsive criteria: no
.sveltecomponents, no routes, nolayout.css/token changes, no copy. The only frontend file is the generatedapi.tstype surface.What I will flag is forward-looking, because this schema is the foundation for honest-uncertainty rendering and the decisions made here shape the UI later:
metaDatePrecisionis the right data shape for accessible honesty. When Phase 4 renders this, please don't encode certainty in color or styling alone — anAPPROXdate shown as "ca. 1943" or aRANGEas "1943–1945" must read as text for screen readers and color-blind users (WCAG 1.4.1, non-color cue). The enum gives you the text hook; use it.UNKNOWNmust render as a real, readable label (e.g. "Datum unbekannt"), not an empty cell — an empty date column reads as "missing/broken" to a 60+ user, not "intentionally unknown." GladUNKNOWNis an explicit value and not null.provisionalpersons will need a visible, non-color status indicator in the persons directory (icon + text, not just a tint) so the uncertainty is honest for everyone. That's a Phase 3 concern, but worth carrying forward.These are notes for downstream issues, not changes to this PR. Nothing here for me to block.
Elicit — Requirements Engineer & Business Analyst
Verdict: ✅ Approved with concerns
Working in Brownfield mode. This is an infrastructure/foundation slice, so I'm reviewing traceability, scope discipline, and whether the deferred behavior is documented rather than silently half-built.
What's well-specified: The PR traces cleanly to #671 and the "Handling the Unknowns" milestone, and the sibling issues (#666 date precision, #665 name triage, #669 importer) are named in ADR-025 with the reason for consolidation (three
V69s = boot failure). The GLOSSARY now defines four new domain terms (source_ref,provisional person,date precision,raw attribution) with the critical disambiguationprovisional≠family_member— that's exactly the ambiguity-hunting I'd otherwise have to flag. Good.Concerns
provisionaldefaults false and nothing sets it true this phase. This is the classic "half-built feature" smell — but here it's explicitly documented in both the PR body and ADR-025 Consequences as intentional, with #669 named as the sole future writer. That converts a red flag into a tracked decision. My only ask: confirm #669 has an acceptance criterion that actually sets and surfacesprovisional, so this column doesn't become dead schema if Phase 3 slips. (Requirements-debt guard.)V18precedent rather than from a stated requirement. Fine to accept, just note it as a deliberate default rather than a derived requirement.No scope creep, no gold-plating — the PR does exactly what #671 scoped and explicitly refuses to do more. Solid foundation slice.
Review findings addressed — Felix Brandt
Pushed three atomic commits (
c9fb14fd,ae674b14,b959e312) ontofeature/671-schema-foundation.BLOCKER (Markus / Felix) — generated-type drift
The
Documententity schema infrontend/src/lib/generated/api.tsalready carries all five new fields (metaDatePrecisionrequired,metaDateEnd/metaDateRaw/senderText/receiverTextoptional) at the current branch tip — it was completed in6f5ca475, after the review snapshot. What was still broken is exactly the downstream consequence you both predicted: strictly-typed mock literals missing the now-required fields. Fixed inc9fb14fd:ReaderRecentDocs.svelte.spec.ts—baseDocgainsmetaDatePrecision: 'UNKNOWN'; thesendermock gainsprovisional: false.PersonMentionEditor.svelte.spec.ts—AUGUSTE/ANNAgainprovisional: false.MentionDropdown.svelte.test.ts—makePersonfactory base gainsprovisional: false.I audited every frontend literal annotated to the generated
Document/Persontypes; all other mocks use local types,Partial<>,Record<string, unknown>, oras unknown ascasts and are unaffected.Sara — fully-open RANGE degenerate case
ae674b14addsv69_metaDateEndCheck_allowsRangeWithBothEndpointsNulltoMigrationIntegrationTest: a RANGE row with bothmeta_dateandmeta_date_endnull survives both CHECKs (asserted, locking the actual behavior). The one-directional rule end-to-end (RANGE + null end allowed; non-RANGE + non-null end rejected) was already covered by the existing pair, so this complements it.MigrationIntegrationTestnow 45/45 (Testcontainers Postgres, verified locally).Tobias — CI frontend type-check gate
ci.ymlruns onpull_request(all PRs) and rannpm run lint+npm run buildbut notnpm run check, so type-drift could slip through.b959e312adds aType check(npm run check) step after Lint. Owner action: enforcing it as a required status check is a Gitea branch-protection setting (not code) — please mark the CI job required on the protected branch.Verification
MigrationIntegrationTest45/45 green.npm run check/ browser tests: CI-only per project policy — the new step will exercise them.--no-verify(husky frontend lint can't run withoutnode_modulesin the worktree); noted per commit.Forward / cross-issue concerns — not touched here
Tracked by the owner separately: Elicit's provisional-AC (#669) and RANGE-rendering AC (#666), and Leonie's Phase 3/4 non-color-cue notes. No scope creep into other issues.
Markus Keller — Senior Application Architect
Verdict: ✅ Approved
I reviewed this as a schema-foundation PR: the migration, the domain-layer enforcement, the documentation currency, and the consolidation rationale.
What's right
V69s (#666/#665/#669) intoV69__import_precision_attribution_identity_schema.sqlis exactly the call I'd make. Three migrations with the same version is a boot failure, andpersons.provisionalwas at risk of a double definition. ADR-025 captures the why — this is the memory of the decision, and it's well written (Context / Decision / Consequences, all present).CHECK, the one-directional RANGE rule,end >= start, the length caps, and the two partial-friendly unique indexes onsource_refare all DB-layer constraints. This is the right layer —V22'sperson_typeallowlist andV18's length cap are cited as precedent, so it's consistent with the existing schema philosophy.source_refunique + nullable is the correct idempotency-key shape: Postgres permits multiple NULLs under a plain unique index, so manually-created rows need no backfill. The ADR states this explicitly.UPDATE→SET NOT NULL. The constraint is enforced only after existing rows are populated.db-orm.pumlgains the new columns and bumps the snapshot header to V69;db-relationships.pumlcorrectly notes "columns only, no new FK, diagram unchanged" (an honest, load-bearing note, not a silent skip); GLOSSARY gainssource_ref,provisional,date precision,raw attribution; ADR-025 is added. This is the cleanest doc-currency pass I've reviewed in a while. No doc-omission blocker.Boundary check
PersonSummaryDTOgainingisProvisional()correctly drove the column into all three native projection SELECTs (findAll,search,findTop) plus theGROUP BY. The "silent false" trap — interface accessor compiles even if the SELECT forgets the column — is real, and it's guarded by Testcontainers tests. No cross-domain repository reach;DocumentListItemmapping stays insideDocumentService. Boundaries intact.Forward-looking (not blockers — owner is tracking)
RANGErow with a nullmeta_date_endgracefully — the ADR says so, and the both-endpoints-null DB behavior is now locked by a test. Tracked in #666.provisionalstaysfalsethis phase by design — the only writer is the Phase 3 importer (#669). Correctly documented as intentional, not half-built.Boring, correct, well-documented schema work. Ship it.
Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
I focused on the prior blocker (the hand-edited
api.ts), the strictly-typed frontend mocks, TDD evidence on the new backend behavior, and clean-code on the entity/DTO surface.Prior blocker — genuinely resolved
I verified the head-branch
frontend/src/lib/generated/api.tsdirectly, not the diff:Documentschema (lines ~1698–1742): all five new fields present —metaDatePrecision(required, no?),metaDateEnd?,metaDateRaw?,senderText?,receiverText?.metaDatePrecisionbeing required matches the@Schema(REQUIRED)+@Builder.Default UNKNOWNon the entity. Correct.Personschema (lines ~1654–1672): carriessourceRef?: stringandprovisional: boolean(required). I diffed this against the base branch — on base,Personhas neither field, so the hand-edit added them correctly.provisionalrequired (backend@Schema(REQUIRED)) andsourceRefoptional (nullable, no@Schema) exactly mirrorPerson.java. This is the field the frontend mocks depend on — and it's there.Frontend mocks — strictly typed, correct
ReaderRecentDocs,MentionDropdown,PersonMentionEditorall now setprovisional: falseon theirPersonfactories, andbaseDocsetsmetaDatePrecision: 'UNKNOWN'. Since the TS types now make these required, the mocks would failnpm run checkwithout them — and they're present. Adding thenpm run checkstep to CI is the right systemic fix so generated-type drift fails the build instead of slipping throughnpm run lint.TDD evidence — present and meaningful
DocumentListItemis correctly threaded throughDocumentService.toListItem(metaDatePrecision, metaDateEndinserted afterdocumentDate, beforesender) and through every test constructor call (DocumentControllerTest,DocumentSearchResultTest,DocumentListItemIntegrationTest). I decoded the record on the head branch and counted the args against all 17 components — they line up.DocumentListItemIntegrationTest.search_listItem_carriesMetaDatePrecisionAndEndproves the new fields survive the projection round-trip against real Postgres.v69_metaDateEndCheck_allowsRangeWithBothEndpointsNull) closes the edge case the prior review flagged — and it asserts bothmeta_dateandmeta_date_endare null, locking the actual DB behavior.Clean code
Entity fields use the
@Enumerated(STRING)+@Builder.Default+@Schema(REQUIRED)pattern correctly. Comments explain why (verbatim normalizer mirror, open-ended ranges) rather than what. TheDatePrecisionenum Javadoc states the cross-system contract explicitly. No nesting, no dead code, names reveal intent.Nothing to change. Clean, test-backed, and the prior blocker is closed.
Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
I reviewed the test strategy: layer placement, real-Postgres coverage of the new constraints, the "silent false" projection guard, and determinism.
Right tests at the right layer
CHECKis exercised inMigrationIntegrationTest: precision allowlist rejection, RANGE-only end rule (both the reject and the allow path),end >= start, the length cap (10001 chars), and bothsource_refunique indexes. This is exactly where these belong — H2 would not enforceCHECKconstraints or partial-null unique semantics, so the bugs that matter only surface onpostgres:16-alpine. Correct tool, correct layer.v69_metaDateEndCheck_allowsRangeWithBothEndpointsNull) is the test I'd have asked for. It documents the actual DB behavior — a fully-open RANGE survives both CHECKs — and the comment explicitly frames a future biconditional tightening as a deliberate change. That's a behavior lock, not just a happy-path assertion.findAllWithDocumentCount_projectsProvisionalTrue,searchWithDocumentCount_projectsProvisionalTrue, andfindTopByDocumentCount_projectsProvisionalTrueeach assertprovisional=trueflows through a different native query. AddingisProvisional()to the interface compiles even if a SELECT forgets the column — these three tests are the only thing standing between that and a silent regression. Good instinct to cover all three.Determinism & hygiene
@Transactional(propagation = NOT_SUPPORTED)withtry/finallycleanup — correct, because aDataIntegrityViolationExceptioninside a managed transaction would poison rollback. State is cleaned up explicitly. No cross-test contamination.createDocumentWithDate,createPerson) keep bodies focused. One logical assertion per test in the constraint cases.Note (not blocking)
The full backend suite and
npm run checkare CI-only by project policy — I'm assessing by reading, and the CI gate is now wired (npm run checkstep added). The frontend mock fixes meancheckshould pass; CI owns the proof. Ifgenerate:apiregenerates anything beyond the hand-edit, the diff should be empty — worth a glance at the CI log post-merge, but not a blocker for approval.Coverage is meaningful, deterministic, and at the correct layers. Approved.
Nora Steiner ("NullX") — Application Security Engineer
Verdict: ✅ Approved
Schema-only PR, no auth/endpoint/query surface changed — but I checked the things that matter for a migration touching user-influenced import data.
What I checked, and why it's fine
meta_date_raw,sender_text,receiver_textare allTEXTcapped at 10000 chars viaCHECK(chk_*_length), mirroring thetranscription_blockscap inV18. These columns will hold raw spreadsheet cell text from import — bounding length at the database means a malformed or maliciously huge import cell can't bloat storage or downstream rendering regardless of any application-layer slip. The 10001-char rejection test proves the bound holds. Good fail-closed posture.chk_meta_date_precision IN (...seven values...)is a fail-closed allowlist — the DB rejects any value outside the enum independent of the Java enum. This is the right model: a future code path that tries to write a bogus precision is rejected at the lowest layer. CWE-20 covered at the DB.p.provisional) and aGROUP BYterm — no string concatenation, no user input in SQL. Parameterized:querybinding is unchanged. The migration is static DDL.provisionalandsource_refare internal provenance/identity fields.source_refcarries the normalizer'sperson_id/tag_path— internal keys, not secrets, not PII beyond what the archive already holds. SurfacingprovisionalinPersonSummaryDTOis intentional (honest-uncertainty UI), not a leak.One thing I'd note for later phases (not a blocker)
source_refbecomes a re-import idempotency/join key. When the Phase 3 importer (#669) starts writing it, treat the spreadsheet-derived value as untrusted input on the way in — length is already bounded atvarchar(255), but the importer should still validate/normalize it rather than trust the cell verbatim. Out of scope here; flagging for the importer PR.No vulnerabilities, no smells. The DB-layer bounds are exactly the kind of defense-in-depth I like to see ship before the feature that fills the columns. Approved.
Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
My surface here is the CI workflow change and the migration's operational characteristics.
CI change is correct and minimal
The new step in
.gitea/workflows/ci.yml:This is the right fix.
npm run lint(Prettier + ESLint) does not type-check, so a hand-editedapi.tswith required fields missing from the mocks would have sailed through. Addingsvelte-check/tscto the pipeline closes that gap as a build gate, not a manual reviewer check. The comment above the step explains why it exists (generated-type drift) — that's the kind of self-documenting CI I want. It slots in after lint, before the banned-vi.mockassertion, which is sensible ordering (fast static checks grouped together).Migration operational review
pg_dump, the standard procedure. ADR-025 repeats this. Correct for the Flyway checksum model; nobody should ever edit a shippedV69.ADD COLUMN nullable → UPDATE → SET NOT NULLruns in one migration. On a small family archive this is a non-issue; even at scale theUPDATE documents SET ... CASEis a single pass. No lock-storm concern at this data volume. The twoCREATE UNIQUE INDEXstatements onsource_refare against freshly-added all-null columns, so index build is trivial.docker-compose.*andDEPLOYMENT.mdare correctly untouched. Flyway runs the migration on backend boot as it always has.CI confidence
MigrationIntegrationTestruns all migrations including V69 againstpostgres:16-alpinevia Testcontainers on every CI run — if V69 fails to apply or a constraint is wrong, CI catches it before prod. That's the contract I rely on, and it's intact.No unpinned images, no secrets, no exposed ports, no deprecated actions touched. Clean. Approved.
Elicit — Requirements Engineer & Business Analyst
Verdict: ✅ Approved with concerns (concerns are tracking-only, not merge blockers)
This is an infrastructure/foundation PR (Phase 2) — no user-facing behavior ships, by explicit design. I assessed it on traceability, scope discipline, and whether deferred behavior is honestly documented rather than half-built.
Traceability is clean
source_ref,provisional person,date precision,raw attribution), each with a disambiguation note (provisional≠family_member;UNKNOWNis the explicit undated value). This is exactly the glossary hygiene that keeps a solo + LLM-driven workflow from drifting. Strong.Scope discipline — no gold-plating, no half-build
provisionalstaysfalsethis phase; the only writer is the Phase 3 importer. This is documented as a deliberate deferral in both the PR body and ADR-025 Consequences, not an abandoned stub. That is the correct way to ship a column ahead of its writer.Concerns — deferred acceptance criteria (owner is tracking, out of scope here)
These are the behaviors a future reviewer must hold the next PRs to. They are not defects in this PR:
provisional, and the persons directory must render that uncertainty honestly. Currently no code sets it true — that's correct for Phase 2, but the AC must land with the importer.RANGEwith a nullmeta_date_endis now a valid DB state (test-locked). Phase 4 rendering needs an explicit AC for how an open-ended range is displayed ("ab 1943", "1943–?", etc.) so it doesn't render as a broken or empty date.provisional/precision surface visually, the uncertainty cue must not be color-only (WCAG 1.4.1).All three are confirmed tracked in #669/#666/#667 per the PR context. No NFR gap for this PR — the data-integrity NFRs (validity, bounds) are enforced at the DB and tested.
Foundation is spec-traceable and scope-disciplined. The deferred ACs are correctly externalized to their own issues. Approved.
Leonie Voss — UX & Accessibility Lead
Verdict: ✅ Approved (LGTM)
There is no UI in this PR — it's a schema + domain-model + DTO + docs change with zero
.sveltefiles touched. So there's nothing for me to evaluate on brand compliance, contrast, touch targets, or 320px layout in this diff. I checked specifically that no markup, no color token, no component, and no copy slipped in, and confirmed it didn't.Why I'm flagging the downstream a11y obligation anyway
This PR establishes the data for two future "honest uncertainty" UI surfaces, and the way that data renders is squarely my concern when it lands:
provisionalperson badge (Phase 3, #669): when the persons directory marks an inferred person, the uncertainty cue must not be color-only — WCAG 2.1 1.4.1 Use of Color. Pair it with a text label and/or icon (e.g. an "unbestätigt" chip with an outline icon), and give it an accessible name so screen readers announce the uncertainty, not just sighted users. ~8% of men can't distinguish a color-only state.DatePrecisionrendering (Phase 4, #666): "ca. 1943", "Frühjahr 1943", and especially an open-endedRANGEwith a null end must render as honest text, not a broken/empty date field. The text itself carries the meaning, which is good for screen readers — just make sure the open-ended case has a real string ("ab 1943" / "1943–unbekannt") rather than rendering "1943–" with a dangling dash.Both are tracked in #669/#666 (and the color-cue concern in #667), so they're correctly out of scope here. I'm noting them so whoever implements those phases pings me for the chip/date-label spec before coding — designing the senior-constraint-first version of these cues is exactly the kind of thing that's cheap up front and expensive to retrofit.
Nothing to change in this PR. Approved.