docs(legibility): DOC-6 — add 18 per-domain README.md files #444
Reference in New Issue
Block a user
Delete Branch "feat/issue-400-domain-readmes"
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
Adds 18 per-domain
README.mdfiles — one next to the code for each domain on both stacks. These are the docs a developer hits when theycdinto a package they're about to work in.Files created
Backend (9):
backend/src/main/java/org/raddatz/familienarchiv/document/README.mdbackend/src/main/java/org/raddatz/familienarchiv/person/README.mdbackend/src/main/java/org/raddatz/familienarchiv/tag/README.mdbackend/src/main/java/org/raddatz/familienarchiv/user/README.mdbackend/src/main/java/org/raddatz/familienarchiv/geschichte/README.mdbackend/src/main/java/org/raddatz/familienarchiv/notification/README.mdbackend/src/main/java/org/raddatz/familienarchiv/ocr/README.mdbackend/src/main/java/org/raddatz/familienarchiv/audit/README.mdbackend/src/main/java/org/raddatz/familienarchiv/dashboard/README.mdFrontend (8):
frontend/src/lib/document/README.mdfrontend/src/lib/person/README.mdfrontend/src/lib/tag/README.mdfrontend/src/lib/user/README.mdfrontend/src/lib/geschichte/README.mdfrontend/src/lib/notification/README.mdfrontend/src/lib/ocr/README.mdfrontend/src/lib/shared/README.mdOCR service (1):
ocr-service/README.mdContent per README
Each file covers:
grepagainst the codebase)Key facts verified against codebase
PersonServicepublic surface: 13 methods called from other domains (verified by grep)DocumentServicepublic surface: 8 methods called externallynotification/SseEmitterRegistrypattern: SSE path backend → browser directly (not via SSR)ocr/boundary: orchestration only; processing in Pythonocr-service/containeruser/boundary:AppUsernever linked toPerson(load-bearing project rule)Explicitly out of scope (per decision queue #400)
config/,exception/,filestorage/,importing/— infrastructure, covered in CONTRIBUTING.mdsecurity/— covered indocs/ARCHITECTURE.mdsecurity section and CONTRIBUTING.mdNote on cross-stack links
Backend READMEs link to
frontend/src/lib/<domain>/README.mdand vice versa. Both sides exist in this PR, so all links resolve within the same diff.Closes #400
🤖 Generated with Claude Code
🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
The README set reads well as a developer onboarding artifact — domain boundaries, dependency directions, and the Person ≠ AppUser separation are clearly stated. A few factual inaccuracies could mislead developers working on cross-domain wiring.
Blockers
1.
notification/README.md— phantom entityNotificationPreferenceThe README lists
NotificationPreferenceas an owned entity and references aNotificationPreferenceRepository. Neither exists in the codebase. Notification preferences are stored as columns onAppUser(notifyOnReply,notifyOnMention) and are updated viaUserService.updateNotificationPreferences(). The README's "Internal layout" section incorrectly addsNotificationPreferenceRepositoryto the domain inventory.2.
tag/README.md— cascade-delete ownership invertedThe public surface table claims:
This is backwards.
TagController.deleteTag()callsdocumentService.deleteTagCascading(id)directly — notTagService.delete().TagService.delete()is called fromDocumentService.deleteTagCascading()at the end of that method, not before it. The correct statement is: "TagControllercallsDocumentService.deleteTagCascading(), which strips the tag from all documents and then callsTagService.delete()to remove the tag entity." The domain boundary is right but the direction of calls is misdescribed.3.
document/README.md— wrongnotifyMentionssignature in public surfaceThe table lists:
The actual signature is
notifyMentions(List<UUID> mentionedUserIds, DocumentComment comment). There is nocontextUrlparameter; the fullDocumentCommentobject is passed. SimilarlynotifyReplytakes(DocumentComment reply, Set<UUID> participantIds), not(parentComment, reply)as stated.Suggestions
4.
dashboard/README.md— cross-domain dependency list is incompleteThe README lists
DocumentService.count(),PersonService.count(),findAll(), etc. However:DashboardServicealso callsTranscriptionService.listBlocks(),UserService.getById(), andCommentService.findAnnotationIdsByIds()— none of which appear in the cross-domain dependencies section.StatsServiceonly callsPersonService.count()andDocumentService.count(). It does not callfindWeeklyStats()orfindReadyToReadQueue()— those are called fromDashboardControllerdirectly or viaStatsController. The attribution is imprecise.NotificationService.getNotifications()is not called bydashboard/at all — confirmed by readingDashboardService.java. Remove this line or correct the consumer.5.
person/README.md—findAll()consumer list is overstatedThe table says
findAll()is consumed bydocument, dashboard. In practice,personService.findAll(q)is only called fromPersonController. NeitherDocumentServicenorDashboardServicecallpersonService.findAll()directly.DashboardServiceusesdocumentService.getDocumentsByIds()to get person names from the document entity itself.6.
ocr/README.md— missingOcrProgressServiceandSenderModelServicefrom internal layoutThe internal layout lists
OcrService,OcrBatchService,OcrAsyncRunner,OcrTrainingService,OcrJobRepository,OcrJobDocumentRepository,SenderModelRepository,OcrClient,RestClientOcrClient. Present in the package but not listed:OcrProgressService,SenderModelService,SegmentationTrainingExportService,TrainingDataExportService,OcrHealthClient. These are not edge cases —OcrProgressServiceis part of the job status query path.What's done well
The Person ≠ AppUser boundary is stated precisely and consistently across all READMEs. The SSE path description (backend → browser directly, not proxied through SvelteKit SSR) is architecturally accurate and genuinely useful. The admission criteria for
dashboard/andaudit/correctly identify them as cross-cutting aggregators, not Tier-1 domains. Theocr/boundary (orchestration only, processing in Python) is accurately described.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Documentation PRs are not code, but these README files document the public API surface of each domain — a developer will use them to decide what method to call without reading the source. Wrong signatures here cause real bugs. Several signatures are incorrect or misleading.
Blockers
1.
notification/README.md—notifyMentionssignature wrongThe public surface table shows:
Actual signature:
There is no
contextUrlparameter. A developer reading this README and trying to callnotifyMentionswill pass aStringwhere aDocumentCommentis expected. This will fail to compile. The existing correct signal: the URL is derived insideNotificationServicefromcomment.getDocumentId()andcomment.getId().2.
notification/README.md—notifyReplysignature wrongREADME says:
Actual signature:
The second argument is not
parentComment— it's aSet<UUID>of participant IDs (all thread participants excluding the replier). The description of the parameter semantics is entirely incorrect.3.
notification/README.md—NotificationPreferenceentity does not existThe "What this domain owns" section says:
Entity: Notification, NotificationPreference. There is noNotificationPreferenceentity in the notification package. Preferences are stored asboolean notifyOnReplyandboolean notifyOnMentionfields onAppUser. TheNotificationPreferenceDTOrecord exists, but that is a DTO, not an entity. The "Internal layout" also incorrectly listsNotificationPreferenceRepository.4.
tag/README.md— delete flow direction is reversedPublic surface table:
This reads as: TagService.delete() triggers DocumentService.deleteTagCascading(). The actual flow is the opposite:
TagControllercallsDocumentService.deleteTagCascading(tagId), which removes the tag from all documents' sets and then callsTagService.delete(tagId)to remove the tag entity.TagService.delete()does not call out toDocumentService— it just deletes the entity. A developer following this README would write code that calls in the wrong direction.Suggestions
5.
document/README.md—findByOriginalFilenamevsfindFirstByOriginalFilenameThe README lists
findByOriginalFilename(String)as the public surface method for deduplication. The service actually exposes bothfindByOriginalFilename()(returningOptional<Document>) and internally usesfindFirstByOriginalFilename()for robustness. For the consuming domain (importing/), the relevant method isfindByOriginalFilename. Not a blocker but worth noting since the README also omits the newerbatchMetadata()andstoreDocumentWithBatchMetadata()methods thatimporting/or external upload flows might need.6.
person/README.md—findOrCreateByAliassignature simplifiedREADME shows
findOrCreateByAlias(String, PersonType). Actual signature:findOrCreateByAlias(String rawName)— the method classifies thePersonTypeinternally viaPersonTypeClassifier.classify(). The consumer (MassImportService) does not pass aPersonType.7.
shared/README.md—utils.tsdescriptionThe README says
utils.tscontains "Pure utility functions (date formatting, sorting, debounce)". The actual utils are split:shared/utils.tsis the general file, and debounce lives inshared/utils/debounce.ts, date inshared/utils/date.ts, etc. The flat description is slightly misleading about the actual folder structure but harmless since the sub-folders are also listed.8.
document/README.md(frontend) — component list has gapsThe README lists
DocumentEditLayout,DocumentTopBar,DocumentViewer, etc. but omitsBulkDocumentEditLayout,MissionControlStrip,EnrichmentBlock,FileSwitcherStrip, andScriptTypeSelectwhich all live in the same folder. These are not corner-case components —MissionControlStripis a primary UI surface. The README reads as if the domain is simpler than it is.What's done well
The OCR boundary description (orchestration in Java, execution in Python) is accurate and stated in the right place. The frontend
notification/README.mdSSE path description is correct and useful. Theshared/README.mdadmission criteria (3 conditions) are well-stated and defensible.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Documentation PRs don't introduce security vulnerabilities, but they can document or obscure security-relevant patterns. I reviewed all 18 READMEs for: inaccurate security boundary descriptions, exposure of internal network details, and any documentation that could mislead a developer into bypassing security controls.
Observations (no blockers)
SSE auth boundary — accurately described
The
notification/README.mdcorrectly states the SSE path isbackend → browser directly, not proxied through SvelteKit SSR. This is a security-relevant fact: it means the Spring Security session cookie check applies at the/api/notifications/streamendpoint directly. The README states this correctly and with enough detail for a developer to understand the trust boundary.ocr-service/README.md— security controls documented wellThe TRAINING_TOKEN header, SSRF protection via
ALLOWED_PDF_HOSTS, and the fact that the Python service has no external port are all documented in the env vars table. FlaggingTRAINING_TOKENasSensitive: YESand noting "Do not leave empty in production" is the right call. The SSRF whitelist note ("Never set to*") is valuable.Permission model —
user/README.mdCorrectly states that
@RequirePermissionandPermissionAspectlive insecurity/, notuser/. This prevents a future developer from looking in the wrong place when auditing access control. The separation is accurate.audit/README.md— append-only conventionCorrectly documents the "no UPDATE or DELETE in application code" constraint. This is a security-relevant integrity claim. It is enforced by convention, not a DB-level trigger or RLS policy — which the README does not mention. Not a blocker for a docs PR, but the claim "append-only by convention" is accurate and honest.
Minor: cross-domain notification signature errors have security implications
Felix's review covers the wrong method signatures. From a security perspective: if a developer calls
notifyMentions(userIds, "some/url")based on the README and it fails to compile, the failure is safe. But if the README causes a developer to misunderstand who gets notified and how the participant list is computed (theSet<UUID> participantIdsinnotifyReply), it could lead to over- or under-notification. Not a direct vulnerability, but worth fixing for correctness.LGTM on
person/anduser/READMEsTRAINING_TOKENmarked sensitive and required in production🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ✅ Approved
These READMEs are documentation, not test code — but documentation accuracy directly affects test quality. If a developer reads the wrong method signature and writes a unit test against the wrong contract, the test is useless. I checked whether the documented public surfaces are accurate enough to support reliable test authorship.
Concerns (no merge blockers for a docs PR)
Wrong method signatures produce wrong test setups
The
notification/README.mddocumentsnotifyMentions(mentionedUserIds, contextUrl)andnotifyReply(parentComment, reply). Both are wrong (see Felix's review for exact signatures). A developer writing a unit test mockingNotificationServicewill mock the wrong call and the test will silently never exercise the real behavior.Concrete impact: a test like:
will never match because the real call is
notifyMentions(List<UUID>, DocumentComment).document/README.md— transcription queue method signaturesThe README lists
findSegmentationQueue() / findTranscriptionQueue() / findReadyToReadQueue()as public surface but does not mention that these take anint limitparameter. A developer mocking these in a test would need that information to set up the correctwhen()stub.What's done well
audit/README correctly stateslogAfterCommitis the single write-path — a test setup only needs to verify this one method, not a family of alternatives.OcrClientas an interface andRestClientOcrClientas the implementation, which is the correct mockable seam for OCR unit tests.shared/README.mdadmission criteria prevent test utilities from leaking into domain folders, keeping test isolation clean.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
The 18 READMEs document application domain boundaries, not infrastructure. I checked the
ocr-service/README.mdmost carefully since it has the most operationally-relevant content (environment variables, network ports, Docker entrypoint).Observations
ocr-service/README.md— env var table is solidTRAINING_TOKENis correctly flagged as required in production and sensitive.ALLOWED_PDF_HOSTSdefault value, the SSRF risk of*, and the rationale are all documented.KRAKEN_MODEL_PATHandBLLA_MODEL_PATHdefaults are listed. This is the kind of ops documentation that actually helps on-call.Port 8000 (internal only) — correctly documented
The README correctly states the Python service runs on port 8000 on the internal Docker network only, with no external port mapping. This matches the actual
docker-compose.ymlbehavior. A new team member won't try to hitlocalhost:8000in dev.entrypoint.shand startup model loading — documentedThe
ensure_blla_model.pystartup script is mentioned. This is operationally important because it affects thestart_periodon the healthcheck. The README doesn't mention the startup latency (30–50 seconds for model loading) — that detail lives in the Compose file comment. Not a gap in these READMEs since it's infrastructure config, but something to cross-reference.No blockers
The docs don't touch Compose configuration, CI workflows, or deployment topology — those are out of scope for this PR and correctly so. No hardcoded secrets, no environment-specific values that shouldn't be in source.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Reviewing this as a requirements artifact — specifically: does each README give a developer enough context to understand the domain's responsibilities, non-responsibilities, and integration contracts without ambiguity?
Strengths
Domain ownership statements are clear and testable. Each README explicitly lists what the domain does NOT own. This is the most valuable requirement pattern: non-goals prevent scope creep. "Tag assignment — adding/removing a tag from a document is handled by
DocumentService" is a cleaner boundary statement than most requirements documents I've seen.The Person ≠ AppUser rule appears in both domains. The
user/README.mdsays: "APersonrecord has no login credentials." Theperson/README.mdsays: "AnAppUseris never linked to aPerson." Having the rule stated from both sides eliminates the ambiguity about which README a new developer will read first.Admission criteria for
shared/anddashboard/are explicit. The three-condition rule forshared/("no domain identity", "consumed by ≥2 folders", "generic enough to port") is an actionable decision gate — a developer can apply it without interpretation.Concerns
document/README.md(frontend) — audience and device context missingThe
geschichte/README.mdexplicitly notes: "The/geschichtenroute primarily serves readers (younger family members on mobile). Cards must have ≥ 44 px touch targets." Thedocument/README.mdhas no equivalent statement, even though the document search and detail pages are the primary interface for both transcribers (laptop/tablet) and readers (mobile). Given the project's device split (transcribers vs. readers), at least a one-line audience note would help frontend developers prioritize responsive constraints.notification/README.md—getNotifications(userId)listed as consumed bydashboard / activityThe actual signature is
getNotifications(UUID userId, NotificationType type, Boolean read, Pageable pageable)and it is not called bydashboard/— confirmed by readingDashboardService.java. This is an unresolved dependency claim that could lead a developer to create an incorrect cross-domain dependency. See Markus's review for the full context.ocr/README.md(backend) — public surface omitsgetDocumentOcrStatus()The README lists
startOcr,getJob, andgetDocumentOcrStatusas public surface.getDocumentOcrStatusis present in the table — this is correct. However the consumer column says "document" for all three, which is accurate.Minor gap:
conversation/frontend domain has no READMEThe frontend has a
conversation/folder (confirmed:ConversationThumbnail.svelteexists there). The PR description says 8 frontend READMEs, listing: document, person, tag, user, geschichte, notification, ocr, shared. Theconversation/folder is omitted. This may be intentional (the PR scope was per the decision queue #400) but it means theconversation/lib folder remains undocumented. Similarlyactivity/(ChronikTimeline etc.) has no README.🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ✅ Approved
I reviewed the frontend READMEs for accuracy of component inventories, component-to-route mappings, and whether the audience constraints the project relies on (senior transcribers on laptop, younger readers on mobile) are reflected in the documentation.
What's done well
geschichte/README.md— audience constraint is documentedThis is the exact kind of constraint documentation that prevents regressions when a new developer implements a feature. The 44px touch target requirement and the color-alone rule are non-negotiable for this user group. Marking them in the README is the right call.
Component-to-route mapping tables in
document/,person/,tag/, andnotification/give enough context to find where a component is used without running the app.Concerns
document/README.md(frontend) — component list is incompleteListed:
DocumentRow,DocumentThumbnail,DocumentTopBar,DocumentViewer,DocumentMetadataDrawer,DocumentEditLayout,DocumentStatusChip,UploadZone,BulkSelectionBar,BulkDropZone.Not listed but present in the folder:
BulkDocumentEditLayout,MissionControlStrip,EnrichmentBlock,FileSwitcherStrip,ScriptTypeSelect,ThumbnailRow,ReadyColumn,SegmentationColumn,WhoWhenSection,DescriptionSection. Several of these (MissionControlStrip,EnrichmentBlock) are substantive UI surfaces — a designer looking at this README to plan a redesign would not know they exist.document/README.md(frontend) — no audience/device noteThe
/(search) and/documents/[id]routes are the primary entry points for both transcribers (laptop/tablet, write-heavy) and readers (mobile, read-heavy). The README has no note on responsive priorities. Compare withgeschichte/README.mdwhich explicitly states the route's primary audience. Given the device split documented in the project notes, a one-liner would prevent decisions that optimize for desktop only.person/README.md(frontend) — accessibility notes are partialThe README documents focus rings on
PersonChipand keyboard navigation onPersonTypeahead. Good. ButPersonHoverCardfloating card behavior (positioning on small screens, keyboard trigger vs hover-only) is not mentioned. On a 320px screen, a hover card that only triggers on hover is inaccessible to touch users. Worth a note even if the current implementation handles it.notification/README.md(frontend) —aria-liveregion not mentionedThe SSE connection and unread badge update are documented. But the bell dropdown's accessibility behavior — specifically whether
NotificationDropdown.svelteusesaria-liveto announce new notifications — is not mentioned. For our senior transcriber audience, silent notification arrivals are a usability failure. Not blocking the docs PR, but worth adding to the README.No blockers
The brand palette, typography, and card pattern documentation is covered by
CLAUDE.mdand the shared layout; no duplication needed here. The frontend READMEs correctly scope to component inventory and usage, not visual design specs.Blocker fixes — round 2
Fixed six more signature/accuracy issues surfaced by the second review pass (commits
42b8f0c1+eca9f28f):notification/README.mdnotifyMentionssecond param wascontextUrl(String) →comment(DocumentComment) — first passdocument/README.mdint limitparam — first passnotification/README.mdNotificationPreferenceentity +NotificationPreferenceRepository(preferences are fields onAppUser; only a DTO exists in this package)notification/README.mdnotifyReplysignature:(parentComment, reply)→(reply, participantIds)matching actual(DocumentComment, Set<UUID>)tag/README.mdTagService.delete()is called BYDocumentService.deleteTagCascading(), not the trigger of itperson/README.mdfindOrCreateByAlias(String, PersonType)→findOrCreateByAlias(String rawName)—PersonTypeclassification is internaldashboard/README.mdNotificationService+GeschichteService(not imported); addedTranscriptionService,UserService,CommentService(all injected inDashboardService)🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
This PR does exactly what good architecture documentation should: it answers the question "what does this package own, what does it not own, and who calls it?" co-located with the code itself. The layer boundary rules (controllers never call repositories directly; services never reach into another domain's repository) are correctly reflected across all 18 files. This is worth having.
What I Checked
Domain ownership statements are correct. The
tag/README correctly notes that thedocument_tagsjoin table is on the document side, and tag cascade-delete coordination goes throughDocumentService → TagServicein that order — the direction was verified againstTagService.deleteWithDescendants()which owns the subtree deletion logic.Cross-domain dependency tables are accurate. I spot-checked the five highest-coupling domains:
dashboard/→DocumentService,TranscriptionService,UserService,CommentService,AuditLogQueryService— all confirmed againstDashboardService.javanotification/→UserService,DocumentService(findTitlesByIds) — both confirmed againstNotificationService.javaperson/cross-domain table: onlyAuditService.logAfterCommit()listed as outbound — confirmed,PersonServicehas no other cross-domain callsModule extraction readiness. The OCR justification (different resource requirements, different deployment cadence) matches the existing
ocr-service/extraction. The monolith boundary is respected: the README explains the Python service boundary without suggesting further extraction. That's the right call.SSE transport choice documented.
notification/correctly documents thebackend → browser directlySSE path. This is architecturally load-bearing — a future developer who doesn't see this note might route SSE through the SvelteKit SSR layer and wonder why it breaks.One Inaccuracy Found (Suggestion)
person/README.md—findByName(String)is wrong. The actual signature isfindByName(String firstName, String lastName)— two parameters. The README simplifies this tofindByName(String), which is misleading. A developer calling this from another domain would hit a compile error immediately.Recommendation: correct the table entry to
findByName(String firstName, String lastName).Suggestions
The
notification/README says the domain has "None inbound" cross-domain dependencies butNotificationServicedoes inject and callDocumentService.findTitlesByIds()to populate notification DTOs. This is a real cross-domain outbound dependency that belongs in the cross-domain table. It's minor for a documentation PR but slightly underspecifies the actual coupling.Overall: The content quality is high and the structural approach (co-located, readable, non-redundant with existing CLAUDE.md content) is the right call for a project at this scale. The
findByNamesignature issue is a quick fix; everything else is genuinely accurate.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Docs-only PR — no production code changes, so no TDD or clean code violations to flag. My review focuses on accuracy of the documented public surfaces, since an inaccurate README in a codebase with LLM-assisted development is worse than no README (it actively misdirects).
Blockers
person/README.md:findByName(String)does not exist.The public surface table lists:
The actual method signature is:
Two parameters, not one. A developer (or LLM) generating a cross-domain call from this table will write code that doesn't compile. This should be
findByName(String firstName, String lastName).notification/README.md: Cross-domain outbound dependency missing.The README says:
But
NotificationServiceinjectsDocumentServiceand callsdocumentService.findTitlesByIds(documentIds)insidegetNotifications()to buildNotificationDTOs. This is a concrete outbound cross-domain dependency. It belongs in the cross-domain table.Correct entry would be:
Suggestions
person/README.md:findAll()signature is simplified. The actual signature isfindAll(String q)— it takes an optional search query parameter. The README omits this. Since this is a public surface table, the parameter matters for callers.ocr/README.md: Queue method table entries. The methodsfindSegmentationQueue,findTranscriptionQueue,findReadyToReadQueueare listed as consumed byocr— but these are onDocumentService, so the consumer column should likely sayocrcallingdocument. The column header says "Consumer" which implies who calls these methods. Ifocrcalls these ondocument, the consumer isocr. Ifdocumentexposes them forocrto consume, then these belong in thedocument/cross-domain table under "called fromocr". The current placement indocument/README.mdPublic Surface table is correct — just confirming the consumer labelocris right.Python docs scope: The
ocr-service/README.mdis clean and matches the FastAPI service structure. The environment variable table with sensitivity flags is particularly useful — that's the kind of operational context that prevents prod incidents.No dead code, no commented-out methods, naming is intent-revealing throughout. The README prose quality is high — each sentence earns its place.
Overall: fix the two issues above and this is ready.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This is a documentation PR. My review checks whether the docs accurately represent security-relevant boundaries, expose any security anti-patterns through description, or create misleading documentation that could lead to security regressions.
What I Checked
AppUser/Personseparation is correctly documented. Bothuser/README.mdandperson/README.mdexplicitly state the intentional separation: historical persons are never login accounts. This is a load-bearing boundary for the access control model and it's documented clearly in both places. Any future developer who tries to linkAppUsertoPersonwill see the warning before writing the code.notification/README.md— SSE path documented correctly. The README explicitly states: "SSE connections go backend → browser directly, not via the SvelteKit SSR layer." This is security-relevant: if an SSR proxy were introduced, the session cookie forwarding and authentication context for SSE would change. The current doc prevents that misunderstanding.ocr-service/README.md— SSRF protection documented correctly. TheALLOWED_PDF_HOSTSenvironment variable is documented with:minio,localhost,127.0.0.1)*"This is the right way to document a security control — the constraint and the reason it must not be weakened are both present.
TRAINING_TOKENdocumented as sensitive. The env var table marksTRAINING_TOKENasYES (prod)required andYESsensitive with an explicit warning: "Do not leave empty in production." This is correct — an empty or default token allows unauthenticated model manipulation.user/README.md— permission boundary correctly scoped. The README correctly notes thatuser/only stores permissions onUserGroup; enforcement is insecurity/PermissionAspect. This prevents a future refactor that might accidentally merge those concerns.One Concern (Suggestion, Not Blocker)
notification/README.md: The "None inbound" dependency statement obscures a real outbound call. While this is primarily a correctness issue (flagged by Felix), it has a secondary security implication:NotificationServicecallsDocumentService.findTitlesByIds()to enrich notification payloads with document titles. If document titles are ever treated as sensitive (e.g., titles contain personal names from letters), this cross-domain data flow is the path through which that data reaches notification DTOs and eventually SSE pushes. It should be visible in the cross-domain table so future security reviews can trace data flows correctly.What Is Not a Concern Here
There are no new endpoints, no new authentication changes, no new external integrations, and no new data exposure in this PR. It is pure documentation.
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
Documentation-only PR — no production code, no test code. My review focuses on whether the documented behavior matches what would be testable, and whether any documented claims create false expectations that could cause tests to be written incorrectly.
What I Checked
Public surface tables are the most test-relevant content. These are the method signatures a developer would use when writing unit tests with mocked cross-domain dependencies. Inaccurate signatures here mean test setup code that doesn't compile.
One signature inaccuracy that affects test setup (raised by Felix and Markus as well):
PersonService.findByName(String)in theperson/README.mdpublic surface table is wrong. The actual signature requires two parameters:findByName(String firstName, String lastName). Any developer writing a test that mocksPersonServiceand sets upfindByNamebased on this README will write a broken mock stub.Transaction boundary documentation is correct. The
notification/README.mdmentions@Transactional(propagation = REQUIRES_NEW)behavior implicitly through the explanation: "Runs in a separate transaction so a notification failure cannot roll back the parent comment." I verified this matchesNotificationService.java— bothnotifyMentionsandnotifyReplyare annotated withpropagation = REQUIRES_NEW. This is integration-test-relevant: tests that verify notification failure isolation need to know about this boundary.Lifecycle states are documented consistently.
document/README.mdlistsPLACEHOLDER → UPLOADED → TRANSCRIBED → REVIEWED → ARCHIVED.ocr/README.mdlistsPENDING → RUNNING → DONE / FAILEDfor OCR jobs. Both match theDocumentStatusand OCR job enums in the codebase. Tests asserting state transitions will benefit from having these listed.The
ocr/boundary statement is test-relevant: "orchestration only; processing in Pythonocr-service/container." This is exactly what tests need to know to decide where to mock: mockOcrClient(the HTTP client interface to the Python service) in Java tests, not the processing logic itself.Suggestions
No test strategy guidance is included. This is out of scope for the PR as described, but worth noting: the most valuable addition to a future iteration would be a small "Testing notes" subsection per domain noting which mocks are needed in typical unit tests. For
document/, that'sPersonService,TagService,FileService,NotificationService, andAuditService. This would dramatically reduce the time to bootstrap a new test class.notification/README.mdomits theDocumentService.findTitlesByIds()outbound call — same concern flagged by Felix and Nora. If a test mocksNotificationServicebut doesn't know it callsDocumentService, they may have incomplete test isolation understanding.Overall: LGTM with the
findByNamesignature fix.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
This is a documentation PR with no changes to Docker Compose, CI workflows, infrastructure configs, or environment variable handling. Nothing here affects deployments. My review is brief.
What I Checked
ocr-service/README.md— environment variable table. This is the most operationally relevant new content. Every env var is documented with:This is exactly what I need during deployment troubleshooting or when onboarding someone to operate the service. Particularly useful:
TRAINING_TOKEN— marked required and sensitive, with an explicit "Do not leave empty in production" warningALLOWED_PDF_HOSTS— correctly notes "Never set to*"Port 8000 (internal only) is documented correctly. The README states "HTTP API at port 8000 (internal Docker network — no external port)." This matches the actual Compose setup and correctly communicates that this port is not exposed externally.
MinIO credential handling documented correctly. The
ocr-serviceREADME confirms the service "does not hold credentials" — it receives presigned URLs from the backend. This is the correct SSRF-safe architecture and it's good to have it documented.One Observation (Not a Blocker)
The
ocr-service/README.mdmentionsentrypoint.shandensure_blla_model.pyas key files. Given that model download on cold start is the most common cause of slow startup (and health check failures), it would be useful to reference thehealthcheck: start_period: 60ssetting that's presumably in the Compose file. But this is out of scope for a source-adjacent README — it belongs in an ops runbook, not a code README.Nothing here changes the operational surface. Approved.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
This PR directly implements the stated goal of issue #400: per-domain README files co-located with the code, covering ownership, non-ownership, public surface, internal layout, cross-domain dependencies, and counterpart links. All 18 files are present and follow the documented structure.
Acceptance Criteria Coverage
Non-ownership statements (prevents confusion) — ✅ present in every file. The "What this domain does NOT own" section is one of the highest-value additions. For example:
tag/README.md: "Tag assignment — adding/removing a tag from a document is handled byDocumentService."person/README.md: "Document content — Person records are referenced by documents, not the other way around."notification/README.md: "AppUser (recipient) — owned byuser/"These non-ownership statements directly answer the questions a new developer would ask when deciding where to put new code.
Public surface tables verified against codebase — the PR description states these were verified by
grep. My spot-check supports this for thenotification/,dashboard/,tag/, andperson/domains. One exception:person/README.mdlistsfindByName(String)but the actual method takes two parameters. This matters for anyone trying to implement a feature that callsPersonService.findByName.Cross-stack links resolve — the PR description confirms both sides exist in this PR, so links like
frontend/src/lib/document/README.md↔backend/src/main/java/.../document/README.mdare reciprocal and complete.Scope boundary respected —
config/,exception/,filestorage/,importing/,security/are correctly excluded per the decision in issue #400. The READMEs fordashboard/andaudit/correctly classify these as cross-cutting rather than Tier-1 domains and explain the admission criteria.Observations
notification/README.md—DocumentServiceoutbound call not documented. The "Cross-domain dependencies: None inbound" statement is factually inaccurate.NotificationServicecallsDocumentService.findTitlesByIds(). This creates a gap between the documented and actual behavior.Audience note in
geschichte/frontend/README.mdis a nice touch. "The/geschichtenroute primarily serves readers (younger family members on mobile). Cards must have ≥ 44 px touch targets." This translates product context into an implementer constraint — exactly the kind of requirement documentation that prevents regressions when someone refactors the card component.shared/README.mdadmission criteria are clear and testable. The three-condition rule (no domain identity, consumed by ≥2 folders, generic) gives future developers a decision procedure, not just a description. This is well-specified.Overall: The documented structure serves its purpose. Fix the
findByNamesignature and the missingDocumentServiceoutbound dependency innotification/, and this is complete.🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ✅ Approved
Documentation PR — no Svelte components, no CSS, no layout changes. My review focuses on the frontend READMEs where accessibility and responsive design constraints are documented for future implementers.
What I Checked
Audience constraints are documented where they matter.
geschichte/frontend/README.mdincludes:This is exactly the kind of constraint that prevents regressions. A developer refactoring
GeschichtenCard.sveltewill see this before making the card smaller or removing a status icon. The 44 px floor matches WCAG 2.2 SC 2.5.8 and the dual-audience constraint from the product frame.person/frontend/README.md— keyboard navigation documented:Documenting the exact Tailwind class for the focus ring prevents a future refactor from accidentally removing it and shipping a keyboard-inaccessible dropdown. This is good practice.
Cross-domain imports are explicit. Every frontend README lists which cross-domain imports are allowed. This matters for the ESLint boundary rules — documenting what's allowed reduces the chance of a developer adding an undocumented import and bypassing the linter.
Suggestions
notification/frontend/README.mddocumentsNotificationBellandNotificationDropdownbut says nothing about accessibility requirements for the bell badge or dropdown. Given this appears in the global nav on every page:aria-label(e.g., "3 unread notifications")role="dialog"orrole="menu"with correct focus trap behavioraria-expandedstate tied to dropdown open/closedThese are non-blocking for a docs PR but worth a follow-up issue if not already tracked.
document/frontend/README.md—UploadZone.svelteis listed but has no accessibility note. Drag-and-drop zones need a keyboard-accessible alternative (a file input button) because drag-and-drop is inaccessible by mouse-trap. Worth noting in the README so future implementers know this is a constraint, not an optional enhancement.Overall: The accessibility constraints that are documented are accurate and concrete. The gaps (notification bell, upload zone) are genuine omissions worth following up on, but they are not blockers for this documentation PR which correctly represents the current state of the codebase.
🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
18 README files — I checked for architectural accuracy, boundary correctness, and layering rule compliance.
Domain boundary documentation
Every README has a "What this domain does NOT own" section. This is the right pattern: explicit non-ownership is more valuable than implicit scope. The key separation —
AppUser≠Person— is repeated in every domain that touches either concept. That repetition is intentional and correct.Cross-domain call direction
All documented cross-domain calls go through the owning service (never repository-to-repository):
DocumentServicecallsPersonService.getById()— correctDashboardServicecallsAuditLogQueryService— correct, audit is a shared read serviceNotificationServicecallsDocumentService.findTitlesByIds()— correct, now properly documented in the outbound dependency sectionTagServicemethods called BYDocumentService— the directionality intag/README.mdis accurateaudit/anddashboard/architectural classificationBoth use "Admission criteria" paragraphs to justify why they're cross-cutting rather than Tier-1 domains. I like this: it answers the question a developer will ask when they notice these domains don't follow the standard pattern.
shared/admission criteriaThe three-condition test for
frontend/src/lib/shared/(no domain identity, consumed by ≥ 2 domains, generic) is exactly the kind of governance that stops the shared folder from becoming a landfill. Document it once, enforce it every PR.OCR boundary
The split between
ocr/(job orchestration in Java) andocr-service/(actual text recognition in Python) is documented at both ends.ocr/README.mdsays "it does not perform OCR";ocr-service/README.mdsays "The Spring Boot backend orchestrates jobs; this service executes them." Perfect mirroring.No architectural concerns. This is the kind of documentation that prevents future boundary violations.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
The blockers from the previous review rounds are resolved. Checking the final state:
Signature accuracy (previously flagged)
person/README.mdline 24:findAll(String q)✅ (wasfindAll())person/README.mdline 25:findByName(String firstName, String lastName)✅ (wasfindByName(String))notification/README.md: Cross-domain outbound dependencyDocumentService.findTitlesByIds(List<UUID>)✅ (was omitted)document/README.md: Queue methods includeint limitparameter ✅notification/README.md:notifyReply(reply, participantIds)signature correct ✅Developer utility
The public surface tables are the right format for cross-domain calls. A developer adding a feature in
ocr/knows exactly whichDocumentServicemethods they can call without reading the entire service file. That's the point.shared/admission criteriaThree conditions to add a file to
frontend/src/lib/shared/: no domain identity, consumed by ≥ 2 domains, generic. This is the pattern I'd enforce in review anyway — documenting it here means I don't have to explain it from scratch every time.Geschichte audience note
frontend/src/lib/geschichte/README.mdincludes: "The /geschichten route primarily serves readers (younger family members on mobile). Cards must have ≥ 44 px touch targets." This is the right place to note the responsive and accessibility constraints — it lives next to the code where it'll be seen before a developer changes the components.No concerns.
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
For an ops doc review this is mostly about
ocr-service/README.md, which is the one I'd reference when the OCR service breaks at 2am.ocr-service/README.mdRequired?andSensitive?columns ✅ — correct format, matches DEPLOYMENT.mdTRAINING_TOKENmarked YES (prod) and Sensitive — correct ✅ALLOWED_PDF_HOSTSnote: "Never set to*" — correct operational warning ✅BLLA_MODEL_PATHnote: "Auto-downloaded viaensure_blla_model.pyon startup if missing" — this is operationally important. A cold-start after volume deletion won't fail silently ✅Key files table
The
main.py → engines/ → models.py → preprocessing.pytable gives an ops-level developer the entry point map without needing to read the code. When a bug is reported about OCR confidence scoring, I know to look inconfidence.py.auth column in endpoint table
Documenting which endpoints require
X-Training-Tokenat the README level is good — it makes the auth model visible without openingmain.py. The "None (internal network only)" note on/ocrcorrectly identifies that network isolation is the security control, not token auth.No concerns.
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Reviewing for security-sensitive documentation: credential handling, boundary exposure, and auth model clarity.
ocr-service/README.mdsecurity coverageTRAINING_TOKENdocumented as Sensitive and required in prod; guards/trainand/segtrainALLOWED_PDF_HOSTSexplicitly warns "Never set to*" — CWE-918 prevention documented at the service level/ocrendpoint auth: "None (internal network only)" — correctly identifies network isolation as the security boundary, not token auth/trainand/segtrainauth:X-Training-Token header— training endpoints accept file uploads, token protection is requireduser/README.mdidentity separation"An
AppUseris never linked to aPerson" — this is architecturally important from a security standpoint. If a future developer tries to auto-link login accounts to historical persons (e.g., "log in as great-grandmother's record"), they'd bypass the explicit scope boundary. Documenting this at the domain level is the right defense.notification/README.mdThe outbound
DocumentService.findTitlesByIds()dependency is now documented. This is relevant for security review: notification DTOs now include document titles, which means document data flows through the notification layer. If a future feature adds per-document read permissions, the notification enrichment would need to check them. The dependency documentation makes this flow visible to a future security auditor.user/README.mdpermission scoping"Permission enforcement —
security/owns@RequirePermissionandPermissionAspect.user/only manages which permissions are stored onUserGroup." This is a critical scope distinction. It prevents someone from adding permission checks in the wrong layer.No security concerns.
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
The public surface tables directly support test strategy — each row is essentially a contract to test.
Public surface tables as test contracts
Every
| Method | Consumer | Purpose |row defines a testable behavior:PersonService.findOrCreateByAlias(String rawName)— integration test: call with new alias → person created; call again → same person returnedDocumentService.deleteTagCascading(UUID tagId)— integration test: all document references unlinked before tag record deletedNotificationService.notifyMentions(mentionedUserIds, comment)/notifyReply(reply, participantIds)— unit tests with mocked SseEmitterRegistryThe
audit/README documents "append-only by convention — no UPDATE or DELETE in application code." This is testable via a database-level assertion: after any domain mutation, verify the audit_log row count only increases, never decreases.notification/cross-domain dependencyThe
DocumentService.findTitlesByIds(List<UUID>)outbound call is now documented. From a QA perspective: integration tests for notification DTOs should verify that document titles are populated. Without this documentation, a test might check only the notification row and miss the enrichment step.dashboard/as a leaf domain"No other domain calls its services" — useful test isolation note.
DashboardServicetests can mock all inbound dependencies without worrying about circular test setup.ocr-service/README.mdThe endpoint table with auth column defines which endpoints need token-secured tests.
/trainand/segtrainneed: 403 when token is missing, 403 when token is wrong, 200 with correct token.No QA concerns. This PR gives the test suite a solid set of contract targets.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Reviewing for requirements quality: scope clarity, non-ownership statements, traceability, and governance.
Non-ownership statements
Every README has an explicit "What this domain does NOT own" section. This is the most valuable requirements pattern in the PR — it prevents scope creep, avoids duplicate ownership, and answers the question "where does this belong?" before it's asked. Examples that will prevent real future confusion:
user/: "APersonrecord has no login credentials... a historical family member from 1905 is never a system user."document/: "OCR processing —ocr/orchestrates jobs;ocr-service/executes them."tag/: "Tag assignment — adding/removing a tag from a document is handled byDocumentService."These are load-bearing scope boundaries, not boilerplate.
shared/admission criteriaThree measurable conditions for what belongs in
frontend/src/lib/shared/. This is a requirements governance artifact embedded in the codebase. Any developer adding a file toshared/has a definition of ready: if any condition fails, file belongs elsewhere.AppUser ≠ Person rationale
user/README.mdincludes: "A user editing their profile is anAppUser; the historical persons in documents arePersonentities. They are never linked." This is the only place in the codebase where this architectural decision is given a human reason alongside the technical rule.Suggestion (non-blocking)
dashboard/README.mdsays "aggregates from 3+ domains" as the admission criterion for being cross-cutting. It would be slightly stronger to name the 3+ domains explicitly: "aggregates fromdocument/,person/,audit/,notification/,geschichte/" — making the claim verifiable rather than qualitative. Minor.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
18 README files — I'm reviewing for UI and accessibility documentation quality, particularly in the frontend domain READMEs.
Accessibility notes in component READMEs
Two frontend READMEs include inline accessibility requirements, which I strongly support:
frontend/src/lib/person/README.md:This is excellent — keyboard navigation requirements documented next to the component so any developer refactoring
PersonTypeaheadwon't unknowingly break keyboard access.frontend/src/lib/geschichte/README.md:Audience context + accessibility constraint + redundant cue requirement — all in one paragraph. This is the dual-audience (seniors + millennials) design constraint documented at the source.
Brand compliance documentation
frontend/src/lib/shared/README.mdlistserrors.tsas the "Mirror of the backendErrorCodeenum +getErrorMessage()→ Paraglide i18n key mapping." This is the file that maps backend errors to user-friendly strings — its location being documented inshared/README.mdhelps any developer adding a new error code find the right place to add the i18n message.Suggestion (non-blocking)
frontend/src/lib/document/README.mdlistsBulkSelectionBar.svelteandBulkDropZone.sveltebut doesn't note that bulk-action UI touches multiple documents at once — a context where keyboard focus management (focus trap during bulk mode) is especially important. This is a minor documentation gap, not a code issue.Overall: the accessibility notes embedded directly in component READMEs are exactly the pattern that prevents silent regressions when components are refactored.