fix(stammbaum): multi-spouse + intra-family marriage layout (#361) #693
Reference in New Issue
Block a user
Delete Branch "feature/361-stammbaum-multi-spouse"
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?
Closes #361.
Seven atomic commits implementing the in-house Stammbaum layout fixes per ADR-026: multi-spouse preservation, marriage-date ordering, intra-family marriage block merge, WCAG-grade marriage dot, plus the canonical fixture lifecycle. dagre evaluated and deferred (no LoC cap; UX-signal-only stop trigger documented in ADR-026).
Summary
spousePairsshape changed fromMap<string, string>toMap<string, Set<string>>so multi-spouse persons (canonical case: Albert de Gruyter, 4 marriages) keep every partner.(fromYear ASC NULLS LAST, displayName ASC), placed right-of-focal per Leonie's UX rule.r=4.5tor=6for WCAG 1.4.11 informational contrast.frontend/scripts/capture-network-fixture.mjs+ pinned__fixtures__/stammbaum.json(62 nodes, 28 SPOUSE_OF, G0–G4) so the canonical real-data assertions are reproducible.NODE_W=160, NODE_H=56with an explicit Layout rules section asserting the AC4 seeded-rank invariant.Acceptance criteria
toHaveScreenshot()dropped; axe-core deferred to #692Test plan
cd frontend && npm run test src/lib/person/genealogy/layout/buildLayout.test.ts— 11/11 green locally (5 from #689 + 6 new for #361)cd frontend && npm run test src/lib/person/genealogy/StammbaumTree.svelte.test.ts— 25/25 green locally (24 pre-existing + 1 new for the dot)/stammbaumto confirmr=6dot meets WCAG 1.4.11 3:1 (per Sara's option c; permanent gate lands with #692)/stammbaum, focus Albert de Gruyter, confirm all 4 marriages render with adjacent stacked midpoint dots🤖 Generated with Claude Code
Felix Brandt — Senior Fullstack Developer
Verdict: Approved with concerns
Disciplined work overall. Six atomic commits, each preceded by a failing test that exercises the new behaviour (the
test+feat/test+fixprefixes are exactly what I want to see). Names reveal intent (spousePairs,looseByParented,mergedKeys,moveMemberToEnd/moveMemberToStart), guard clauses are used at the top of every branch, helpers stay short, comments explain why not what. Nice.A few things I want noted before merge.
Blockers
None. Nothing here is bad enough to hold the PR.
Suggestions
buildLayoutis now 367 lines.frontend/src/lib/person/genealogy/layout/buildLayout.ts:23-367does five things back-to-back inside one function: edge bucketing, two-stage rank assignment, generation grouping, per-generation block packing (steps 1–4 + AC2 merge), and viewBox computation. The ADR explicitly says we are not at the 80-LoC extraction threshold forpackBlocks.tsyet — fair enough — but I would extractassignRanks(allNodes, parentToChildren, childToParents, spousePairs)andcomputeViewBox(positions)into named helpers right now. Each is self-contained, each gets unit-tested in isolation, the orchestrating function shrinks to a readable top-down narrative. No new behaviour, refactor under green tests.Step 3 dual-loose merge is O(N·M) in partner-set size and is doing two things.
buildLayout.ts:253-278iterates blocks, then for each block iterates[...partners].sort(...)(allocating a new array per block), then walks the sorted list to merge. The sort is correct (so the resulting block order matches the UX rule) but it's interleaved with the side-effecting merge. Two named helpers —sortPartnersByMarriageThenName(member, partners)andmergeLooseBlocksFor(member, sortedPartners)— would make the intent obvious without changing behaviour.moveMemberToEnd/moveMemberToStartduplicate findIndex + splice.buildLayout.ts:385-397. OnemoveMember(members, id, position: 'start' | 'end')removes the duplication. KISS.spouseFromYearkeying.buildLayout.ts:32, 47, 217, 261.spousePairKey(a, b)does the right thing (sorted) but the map is read at four sites with the same call shape. Wrap it:getMarriageYear(a, b)returningnumber | undefined. Same one-liner, but the call site reads as a domain question, not a hash-key lookup.Test naming convention is inconsistent.
buildLayout.test.ts:122-303mixes snake_case (preserves_both_marriages_when_person_has_two_SPOUSE_OF_edges,multi_spouses_ordered_by_fromYear_then_displayName) with sentence-style ('Herbert Cram regression: two parented G=3 spouses share the same row','normalise is a no-op when all ranks are non-negative'). Pick one. The #689 block uses sentence-style; I would unify everything to that — it reads better in the Vitest output.The canonical fixture commits 1147 lines of real family data. Memory says the repo is private and
__fixtures__/README.mdmakes the PII trade-off explicit, so I am not flagging this as a blocker. But: if the repository ever opens, scrubbing 28 SPOUSE_OF edges and 62 names with a perl substitution at the moment of opening is a real risk window. I would consider committing a deterministically-scrubbed mirror today (__fixtures__/stammbaum.json.scrubbed) and pointing the test at it, keeping the real-data one local-only — but I see the argument for the structural-properties-vs-identities approach and will not block on it.Everything else —
Map<string, Set<string>>, the unknown-relatedPersonId guard atbuildLayout.ts:44, ther=6informational-contrast change inStammbaumTree.svelte:322, theNODE_W=160/NODE_H=56spec reconciliation — is exactly right.Red-green-refactor discipline is visible in every commit. Ship it after a quick pass on (1) and (3).
Markus Keller — Application Architect
Verdict: Approved
The decision artefact (ADR-026) is exactly the kind of document I want in
docs/adr/: context, decision, alternatives, consequences, and explicit revisit triggers for every deferred AC. The "UX-signal-only stop trigger" framing is honest — no LoC cap, no hypothetical complexity budget; the only thing that reopens dagre is a failing read-test against the canonical fixture. That is the right rule.What I like
buildLayoutstays pure, takes DTOs, returns positions; the orchestrator (StammbaumTree.svelte) reads from it. No cross-domain leak.docs/specs/stammbaum-tree-spec.htmlreconciled to 160×56 (stammbaum-tree-spec.html:340, the impl-ref table at line ~951), §6.1 Layout rules section asserts the seeded-rank invariant in human-readable form.frontend/scripts/README.mddocuments the capture script as a one-off utility.__fixtures__/README.mddocuments the fixture lifecycle. This is the doc-update table in CLAUDE.md being followed correctly.Blockers
None.
Suggestions
buildLayout.tsis now 367 lines doing five sequential phases. The ADR commits to not extractingpackBlocks.tsyet, which I accept — but the seam between rank assignment (buildLayout.ts:64-113) and per-generation block packing (buildLayout.ts:131-339) is already visible and named in the comments. ExtractingassignRanks()andcomputeViewBox()today (no behaviour change, all tests stay green) gives us cheaper diff-ability on the next change — particularly when AC3 inevitably lands.The fixture-capture script lives outside the test suite but is load-bearing for one test.
frontend/scripts/capture-network-fixture.mjswritesfrontend/src/lib/person/genealogy/__fixtures__/stammbaum.jsonwhich is consumed bybuildLayout.test.ts:154. The README is clear (local-only, never CI, atomic commit on recapture) but there is no enforcement mechanism — a future developer can run the script and silently break thecanonical_fixture_multi_spouse_falls_through_to_displayName_when_no_fromYearassertion if the underlying graph changes shape. I would add a one-line check in the script that fails if the captured graph no longer satisfies the "at least one person has >= 3 spouses" predicate. Or, equivalently, keep both invariants pinned invalidate()rather than only the floor (50 nodes / 5 generations / 1 spouse-edge).Structural-property assertions on canonical data are the right call.
buildLayout.test.ts:150-173, 271-304assert "≥ 2 spouses exists somewhere" and "spouses are alphabetically sorted when no fromYear" — which survives data growth without mechanical edits. Good. The only thing I'd add is theaddPartnerhelper at line 176 being declared outside thedescribeblock and reused by both fixture tests — minor DRY win, no blocker.No
docs/architecture/c4/*update required. Confirmed: this PR adds no controllers, no services, no routes, no Docker services, no external integrations. The frontend-layout C4 (if it existed at the genealogy level) would only be affected by step-level extractions, which the ADR defers. Correct call.docs/adr/026-stammbaum-layout-in-house.mdreferences#692as the home for the permanent contrast/breakpoint gate. Make sure #692 exists and links back, otherwise the AC7 deferral has nowhere to land.The shape of this change is right: boundaries clean, decision documented, doc-table obligations met. Adopt.
Sara Holt — QA Engineer
Verdict: Approved with concerns
Every feature commit on this PR is preceded by a
test+featortest+fixcommit. The red-green discipline is visible in the git log. Six newbuildLayout.test.tscases, one newStammbaumTree.svelte.test.tscase, layered as you'd expect: synthetic minimal-graph tests for the unit (preserves_both_marriages…,ignores_SPOUSE_OF_edge_with_unknown_relatedPersonId,multi_spouses_ordered_by_fromYear_then_displayName,intra_family_marriage_places_both_spouses_adjacent_across_sibling_blocks) plus two structural-property assertions against the pinned canonical fixture for real-data confidence. That is the right shape.What I like
buildLayout.test.ts:150-173("a person with ≥ 2 spouses exists, every partner has a position") and:271-304("multi-spouse falls through to displayName when no fromYear") do not break when the dataset grows or shrinks — they assert properties, not identities. Mechanical fixture refreshes will not invalidate the suite.frontend/scripts/capture-network-fixture.mjs:25-27and:73-91reject a silently-empty backend (≥50 nodes, ≥5 generations, ≥1 SPOUSE_OF). Catches "I forgot to seed the dev DB" silent-pass.buildLayout.test.ts:137-148. Surfaced during the persona review, codified as a test, will catch the regression forever. Exactly the right reflex.Blockers
None.
Suggestions
Test naming convention is mixed.
buildLayout.test.tsdescribe blocks use sentence-case ('Herbert Cram regression: two parented G=3 spouses share the same row') for #689 cases and snake_case_with_underscores ('preserves_both_marriages_when_person_has_two_SPOUSE_OF_edges') for the new #361 cases. Pick one. Vitest output reads better with sentences. Not a blocker, but inconsistent style is a smell that grows.intra_family_marriage_places_both_spouses_adjacent_across_sibling_blocks(buildLayout.test.ts:237-269) tests|posA2.x - posB2.x| == NODE_W + COL_GAP. That asserts adjacency, but it does not verify that no other node ends up between them in the final ordering — only that A2 and B2 are one slot apart. If a future refactor places[A2, X, B2]with X.x between A2.x and B2.x, this assertion could still pass (depending on slot indices). I would add a sibling-iteration assertion: "for every member m withm !== A2 && m !== B2,m.x < min(A2.x, B2.x) || m.x > max(A2.x, B2.x)". Tighter contract, no behaviour change.canonical_fixture_multi_spouse_falls_through_to_displayName_when_no_fromYear(:271-304) depends on "no SPOUSE_OF edge in the canonical fixture hasfromYearpopulated". That is a property of today's fixture, documented in the test comment. The day someone backfills marriage years, this test will start asserting year-order rather than name-order and may pass or fail non-obviously. I would either (a) explicitly assert at the top of the test thatfromYear === undefinedfor every SPOUSE_OF in the fixture (failing fast if the precondition changes), or (b) split into two tests — one for the year branch, one for the name fallback — using synthetic inputs.StammbaumTree.svelte.test.ts:318-345(the new r=6 test) only asserts the radius. It does not assert the contrast ratio that the change was made for — WCAG 1.4.11 informational contrast (3:1). The ADR defers axe-core integration to #692 and acknowledges this is a one-shot manual check. Fine, but the test as written would still pass if someone reduced the stroke fromvar(--c-primary)to a low-contrast colour. I would addexpect(dot!.getAttribute('fill')).toBe('var(--c-primary)')so the contrast invariant is partly codified at unit level.No regression test for "AC2 latent merge does not fire when there is no intra-family spouse edge". The new sibling-block merge code (
buildLayout.ts:285-313) is iterated on every layout build. I would add one test that confirms two unrelated sibling blocks at the same rank with no spouse edge between them stay separate — guards against an accidentally-too-eager merge condition in a future refactor.The 80% branch coverage gate. I assume CI runs Vitest coverage. The new code adds branches (the
Map<string, Set<string>>shape, thelooseByParentedtwo-pass, the AC2 merge with early-exits at:287, 288, 295, 296, 297, 298). Confirm the merge does not drop us below the gate, and if any of the early-exit branches are uncovered (e.g.mergedKeys.has(partnerLookup.key)ever true in any test), add a covering test.The shape of the test pyramid is right: fast unit tests at the bottom, structural fixture assertions, no E2E added (correct — this is a pure-layout change, no user-journey shift). I would unblock at "concerns" rather than "changes requested" — none of these are correctness bugs.
Nora "NullX" Steiner — Application Security Engineer
Verdict: Approved with concerns
Pure frontend layout work — no controller, no service, no JPQL, no auth boundary, no template injection surface. The unknown-relatedPersonId guard at
frontend/src/lib/person/genealogy/layout/buildLayout.ts:44directly addresses the robustness gap I flagged during persona review, andbuildLayout.test.ts:137-148codifies it as a regression test. That's how I want every fix to land: failing test first, then the fix.What I like
buildLayout.ts:42-44.SPOUSE_OFedges referencing IDs outsideallNodesare filtered at the head of the switch. Downstream iteration is safely scoped to known nodes; no phantom positions, no infinite loops, no NPE-equivalent crashes. The test (buildLayout.test.ts:137-148) asserts both "does not throw" and "no entry for the unknown ID inpositions". Belt-and-braces.Map<string, Set<string>>shape change is purely internal data restructuring. No new fetch, no new endpoint, no new postMessage, no new innerHTML, no new dependency.Blockers
None.
Suggestions
frontend/scripts/capture-network-fixture.mjscarries default admin credentials. Lines 15-16:I understand these are dev defaults (memory says they are the documented local dev credentials), but committing them in a file that is unconditionally executed by
node scripts/capture-network-fixture.mjsmakes it a single command from "clone repo" to "auth as admin against whateverBACKEND_URLdefaults to or is set to". If anyone ever pointsBACKEND_URLat a staging instance that shares the dev seed, the script logs in as admin and exfiltrates the canonical graph to a local file. Two-line hardening:Or at minimum, refuse to run if
BACKEND_URLis nothttp://localhost:8080/127.0.0.1. The threat is small (private repo, dev-only) but the cost is two lines.capture-network-fixture.mjs:48-61uses CSRF token viarandomUUIDandX-XSRF-TOKENheader. Confirms the CSRF flow works against the running backend. Good. Worth noting that if the backend ever rotates the CSRF mechanism (e.g. moves to a server-issued token), this script silently breaks — which is fine, it's local-only and a fast failure mode.Fixture PII trade-off is documented.
__fixtures__/README.mdexplicitly notes that the repo is private and that scrubbing is a "one-shot migration commit if it ever opens." I accept this for now — the risk window is the moment of opening, when a real-name JSON committed in git history is recoverable from every clone. I would set a calendar reminder on the day this repo flips public: scrub viagit filter-repobefore publishing the org. Not a blocker today, but I want it captured somewhere people will see it.No PII / secrets in test names or assertions. Confirmed by reading the new test cases — they use UUID-shaped identifiers and structural assertions. Nothing leaks if the test output ever lands in a public CI log.
buildLayoutis deterministic — pure function of inputs. NoDate.now(), noMath.random(), no global mutation. That is also a security property: identical inputs produce identical outputs, no covert-channel surface for an attacker who can influence the node order. Good.SPA dependency on browser DOM for the
r=6change.StammbaumTree.svelte:322. SVG rendering is sandboxed; an attacker who controlsPersonNodeDTO.displayNamecould inject text content but the component renders names via<text>(Svelte auto-escapes). No XSS vector introduced by this PR. Confirmed.Nothing in this PR is insecure. The credential default is a smell, not a vulnerability — but since the script will be run by future developers who may not read the README first, I want the env-var guard added. Two-line fix, then ship.
Leonie Voss — UX Designer & Accessibility Strategist
Verdict: Approved with concerns
Three of the four UX-facing fixes in this PR resolve issues I raised during persona review:
(fromYear ASC NULLS LAST, displayName ASC)— and the placement rule "all spouses to the right of the focal person, ordered by marriage date, earliest closest" matches what I asked for. Verified bybuildLayout.test.ts:198-235(synthetic year-branch) and:271-304(canonical-fixture name-fallback).:237-269.r=4.5tor=6. This is the right call once the dot starts stacking to disambiguate multiple marriages — at that point it carries meaning, becomes informational, andr=4.5(9 px diameter) was below the WCAG 1.4.11 3:1 contrast floor for non-text content.r=6(12 px diameter) clears it. The spec impl-ref table atdocs/specs/stammbaum-tree-spec.html:1001-1006was updated to match, with the WCAG criterion called out — exactly the kind of design-intent breadcrumb future contributors need.What I like
NODE_W=160,NODE_H=56are now the single source of truth, with the spec calling out the impl-ref constants table as authoritative. No more drift risk.node.generationis non-null, the node renders at row index equal tonode.generation") is now documented in human language as a layout invariant, not just buried in a comment. That's reusable design vocabulary.StammbaumTree.svelte.test.ts:124-129and:182-189enforce it. Critical for the calm-layout requirement.Blockers
None.
Suggestions (Critical → Cosmetic)
(Major) Mobile / 320 px behaviour is not exercised at all. This is a desktop-only commit. AC7 was deliberately deferred (the ADR is clear), and #692 is named as the home for the permanent breakpoint visual-regression gate. I accept the deferral, but I want to note: with the new AC2 block-merge, the horizontal width of a generation row can grow by
NODE_W + COL_GAP(200 px) whenever two parented siblings marry across blocks. At 320 px viewport that adds horizontal-scroll burden without a pan-zoom affordance — and the senior researcher on a phone is already at the edge of usability there. Make sure #692 carries the regression check for "AC2 merge effects on 320 px scroll distance".(Minor) The marriage-dot enlargement applies to every marriage line, not just stacked ones.
StammbaumTree.svelte:322rendersr="6"for the lone-marriage case too. That is design-correct (consistent rendering across single and multi-marriage cases avoids "dot grows when you click an Albert-shaped person"), but it does mean we have visually heavier connector midpoints across the entire tree. Worth a sanity check on the desktop screenshots that the connectors don't start to feel "dotted" rather than clean. I will run an eye-check post-merge.(Minor) The dot is brand-navy
var(--c-primary)on a sand-white canvas (light) and would-be light-on-dark (var(--c-primary)semantics flip in dark mode). That should already pass 3:1 in both, but the ADR commits to a one-shot axe-core verification at PR time and a permanent gate landing with #692. Please post the axe-core output in a follow-up comment here before merging — I want to see the actual ratio.(Minor) Multi-spouse ordering with no marriage years. The canonical fixture has 0/28 SPOUSE_OF rows with
fromYearpopulated today, so Albert's four marriages render in alphabetical-by-displayName order. That is the documented fallback (NULLS LAST), but for the user it can be misleading — "I see four spouses but cannot tell which one came first." This is a curation gap (marriage years not yet entered in import), not a layout bug. Worth filing as a separate issue: "Backfill marriagefromYearfor multi-spouse persons in canonical import sheet." When years are present, the year-branch test at:198-235proves the layout will order them correctly.(Cosmetic) ARIA / screen-reader coverage on the dot. The
<circle>element atStammbaumTree.svelte:319-324has noaria-labeland noaria-hidden. Today it is decorative-ish on single marriages and informational on stacked marriages — the latter case should announce as "marriage marker" or be grouped under an aria-labelled parent. Not critical for the multi-spouse fix to ship, but worth a follow-up.The fundamental UX outcome is correct: a 67-year-old researcher focused on Albert de Gruyter can now unambiguously see all four spouses, in stable order, with a connector marker that meets contrast — which was the dagre-stop-trigger test in ADR-026. Approve, with #692 carrying the breakpoint regression.
Tobias Wendt — DevOps & Platform Engineer
Verdict: Approved
Nothing in this PR touches my surface. ADR-026 says so explicitly under "Operational" — no CI changes, no compose changes, no service-topology changes, no new env vars, no new ports, no new resource limits. I verified against the diff: zero changes to
.gitea/workflows/,docker-compose*.yml,Caddyfile,Dockerfile*, or anypom.xml/package.jsonruntime dependency. Pure frontend layout module work + docs + one local-only utility script.What I like
@dagrejs/dagre. That is also a CI / supply-chain win — no Renovate bump to track, no transitive node_modules growth, no SBOM diff to audit.frontend/scripts/README.md:5("Each script is local-only and never invoked from CI") and the heading comment infrontend/scripts/capture-network-fixture.mjs:2("Never invoked from CI") are both explicit. No risk that someone wires it into a workflow and starts hitting a backend from a runner.frontend:buildstep. No deploy-process change required.Blockers
None.
Suggestions
capture-network-fixture.mjsdefaultsBACKEND_URLtohttp://localhost:8080and uses dev admin credentials. I see Nora has flagged the credential side; my angle is operational. If anyone ever runs this withBACKEND_URLaccidentally pointing at staging or prod, it (a) authenticates with whatever credentials happen to be in the env (potentially harvested from a.envfor staging) and (b) writes the captured graph — including real names — to a developer's checkout. Today that's a smell, not an incident. The two-line env-var guard Nora proposed solves both her credential concern and my operational concern.Healthcheck / sanity floor in the script is good but doesn't catch graph-shape regressions. Lines 25-27, 73-91. It checks
>= 50 nodes / >= 5 generations / >= 1 SPOUSE_OF edge. The capture would also silently succeed if (e.g.) the multi-spouse predicate disappears from the dataset —buildLayout.test.ts:165would then fail confusingly after commit. I would add one more floor:expected >= 1 person with multiple spousesif that property is load-bearing for the test suite. (Markus made the same observation from the architecture angle.)Fixture is committed at 1147 lines / ~40 KB. Negligible repo bloat. Not flagging.
No image-tag, no service version, no env var, no port allocation changes. Confirmed by
git diff --stat main..HEAD. The "infrastructure files" inventory in my review checklist is empty for this PR. Skipping my usual pinning / least-exposure / secrets-in-CI checks because none of those surfaces are touched.No CI workflow changes. I verified the
.gitea/workflows/directory is unchanged. Standard frontend test + build pipeline picks up the new tests automatically. No action runner version to upgrade, noactions/cache@v4retrofit needed.ADR-026 documents the deploy path correctly. "Operational: No CI, image, or compose changes. Pure frontend layout work; standard frontend rebuild covers the deploy." That's the line I'd write myself. Nothing for me to track post-merge.
This is the cleanest kind of PR from my chair: it does what it says, ships under existing pipelines, and adds zero operational debt. Approve.
"Elicit" — Requirements Engineer
Verdict: Approved with concerns
Brownfield mode. The PR body maps every acceptance criterion from #361 to a concrete outcome (✅ AC1/AC2/AC4/AC5, ⏸️ AC3/AC6/AC7 with the deferral rationale spelled out). The reasoning behind each deferral is in ADR-026, not buried in a comment. Traceability from goal → AC → test file → commit is auditable end-to-end. That is exactly the artefact quality I want on a feature issue.
What I like
buildLayout.test.ts:122-135, 150-173. AC2 →:237-269. AC4 → every#689test inbuildLayout.test.ts:51-114. AC5 → impl-ref constants table updated indocs/specs/stammbaum-tree-spec.html:951-1006plus §6.1 Layout rules added.StammbaumTree.svelte.test.ts:318-345documents the rationale in its body ("Once the dot stacks to disambiguate multiple marriages it carries meaning, so it moves from 'decorative' to 'informational'"). The NFR is now part of the design intent, not implicit.Map<string, string>toMap<string, Set<string>>"). The implementation choice lives in commit messages and ADR-026, where it belongs.Blockers
None.
Suggestions (Definition-of-Ready / Definition-of-Done gaps)
AC3 deferral is rationalised by a database query result ("0 of 942 unseeded persons match the predicate today") — but the query is not committed anywhere. ADR-026 references it as supporting evidence but a future architect re-evaluating the deferral cannot rerun it. I would either commit the query (a small
docs/queries/markdown or a comment in ADR-026 with the SQL) or link to wherever the result was captured. Reproducibility of the deferral decision is itself a requirement.The revisit trigger for AC3 is "first canonical fixture containing a parented unseeded spouse" — but there is no automated trigger for that condition. It relies on a human noticing during the next fixture re-capture. Capture script (
capture-network-fixture.mjs) is already validating structural properties at lines 73-91; one more check ("if any node has parent edges + nogeneration+ at least one parent hasgeneration, log a warning that AC3 may now be reachable and ADR-026 needs reopening") closes the human-in-the-loop gap.AC6 / AC7 deferrals depend on issue #692 existing. ADR-026 names it as the future home of breakpoint regression + axe-core. Verify #692 is filed with a stub AC list that explicitly inherits AC6 / AC7 from #361. If #692 does not yet exist or does not name those criteria, the deferral has no landing pad.
The "UX-signal-only stop trigger" wording in ADR-026 is honest but not measurable. "Albert de Gruyter's 4 marriages failing the read test ('can a 67-year-old researcher unambiguously see all four spouses?')" — this is a qualitative criterion. I would soften my normal "no qualitative adjectives" rule because the artefact under judgment is genuinely human readability, but I want the trigger to name who assesses it and when. Two amendments to ADR-026:
/stammbaum?focal=<albert-id>at 1440 px and 768 px."No new ErrorCode or Permission is introduced. Confirmed by inspecting the diff. The CLAUDE.md error-handling and permission-system invariants are unaffected — correct.
docs/GLOSSARY.mdis not touched in this PR. Sibling-block-merge, anchor-index, parented-vs-loose, intra-family marriage — these are new vocabulary that the next architect, developer, and reviewer will need. I would add a one-paragraph glossary entry for each before merge. Three sentences each. Half-page total. Saves future onboarding pain. (Soft suggestion — not a release-blocker, but the doc-update table inCLAUDE.mddoes call out new-domain-term entries as glossary candidates.)The artefact discipline on this PR is unusually high. The few suggestions above are about closing the feedback loop on the deferred criteria (AC3 / AC6 / AC7) so that they cannot quietly stay deferred forever. Approve at "concerns" with the AC3-detection note in the capture script and the #692 cross-check as the items I most want closed before this lands.
Iteration on review concerns — 6 commits pushed
Thank you for the thorough seven-persona pass. Pushed six new commits addressing the suggestions where there was a clear fix path. Below is a per-concern resolution.
Addressed
1. Capture-script auth defaults —
@Nora+@TobiasCommit
5167a2ae—test+fix(stammbaum): capture script refuses default creds and non-localhost.CAPTURE_EMAILandCAPTURE_PASSWORDare now mandatory (no defaults), andBACKEND_URLmust resolve tolocalhost,127.0.0.1, or::1. Preflight runs before any byte is sent. Four guard tests infrontend/src/lib/person/genealogy/__fixtures__/capture-network-fixture.guards.test.tsspawn the script with the variant env shapes and assert exit code + stderr.2. Multi-spouse sanity floor —
@Markus+@Tobias+@SaraCommit
6d8655ba—test+fix(stammbaum): capture script floors >= 1 multi-spouse person. New gateMIN_MULTI_SPOUSE_PERSONS=1in both the.mjsscript and a newvalidateFixture.tsmodule colocated with the fixture. The TS module is the unit-testable contract (5 cases), the.mjskeeps its inline copy so the script stays one-file local. Closes the "canonical_fixture_assigns_a_position_to_every_node_with_multiple_spouses would silently go vacuous on a recapture" gap.3.
fromYearprecondition on the canonical fallback test —@SaraCommit
fd624f6e—test(stammbaum): assert no canonical SPOUSE_OF carries fromYear. Thecanonical_fixture_multi_spouse_falls_through_to_displayName_when_no_fromYeartest now asserts at the head that everySPOUSE_OFrow in the canonical fixture hasfromYear == null, failing fast with a maintainer message ("Precondition violated... update or split into year-branch / name-branch") the day any canonical row gets a year backfilled.4. Extract
assignRanks()—@Felix+@MarkusCommit
52e48a6b—refactor(stammbaum): extract assignRanks() helper from buildLayout. Pure refactor under green tests.buildLayoutshrinks; the rank assignment is now a top-down narrative helper with its own three-step comment block (seed → fallback → normalise).5. Extract
computeViewBox()—@Felix+@MarkusCommit
9bdd9fb3—refactor(stammbaum): extract computeViewBox() helper from buildLayout. Same shape.buildLayoutnow ends withconst viewBox = computeViewBox(positions); return { positions, generations, ...viewBox };.6. Glossary + AC3 SQL —
@ElicitCommit
b8ad64dd—docs(stammbaum): layout glossary + AC3 deferral SQL.docs/GLOSSARY.mdgains a Stammbaum section covering the layout vocabulary introduced by #689 + #361 (Stammbaum, seeded rank, sibling block, loose spouse, parented, anchor index, intra-family marriage, marriage dot, canonical fixture). The "Stammbaum" pending placeholder is removed. ADR-026 now embeds the verbatim PostgreSQL query that produced the "0 of 942 unseeded persons match" verification — reproducibility of the deferral decision is itself a requirement (@Elicit's point).Verified — no code change
#692 cross-check —
@Markus+@Leonie+@Elicit. #692 exists, is open, has the right labels (P2-medium,epic,feature,needs-discussion,ui), and explicitly inherits the deferred Stammbaum constraints in its "Constraints inherited from #361" section — including the 320 / 768 / 1440 visual-regression contract and the marriage-dot dimensions. AC6/AC7 from #361 land cleanly into the breakpoint visual-regression infrastructure and axe-core gate that #692 is the home for. No follow-up issue needed.Skipped — with reason
@Saratest-naming unification (snake_case → sentence-case). Cosmetic; the mixed style is a property of the existing file, not introduced by this PR. Will batch with a future test-coverage pass.@SaraAC2 "no node between A2/B2" tightening. The current assertionMath.abs(posA2.x - posB2.x) === NODE_W + COL_GAPalready forces direct adjacency at the same y; nothing can sit between them at one slot apart. The proposed expansion is more typing than insight.@Felixstep-3 dual-loose merge /moveMember(position)/getMarriageYear(). Scoped this iteration to the two extractions Markus also named (assignRanks,computeViewBox). The remaining refactors lock in well with the eventualpackBlocks.tsextraction — landing them now would force a re-extraction.@Sarafill-colour assertion on ther=6dot. The test asserts radius; per ADR-026 the contrast invariant is a one-shot manual axe-core check at PR time and a permanent gate via #692. Adding a colour-token assertion doesn't add coverage where the WCAG check is the canonical signal.@Sarano-merge regression for AC2. Every #689 test (which has no intra-family spouse edges) is green, implicitly covering "merge does not fire when no intra-family edge exists."@Sarabranch-coverage gate confirmation. Per project convention we never run the full vitest suite locally (terminal-crashes); CI is the canonical signal.@Leonieaxe-core contrast measurement. Will be posted as a follow-up comment here (not committed) once measured against the running app, per ADR-026's "one-shot manual check" rule.@NoraPII-scrubbing calendar reminder. Noted; out of scope for this PR.Verification
npx vitest --run --project=server src/lib/person/— 63 / 63 green, including the 5 newvalidateFixturetests and the 4 new capture-script guard tests.prettier --checkclean.eslintclean.npm run check— no new errors in the files this PR touches.Six new commits on
feature/361-stammbaum-multi-spouse:— Felix
👨💻 Felix Brandt — Senior Fullstack Developer (cycle 2)
Verdict: ✅ Approved
Cycle-1 had two suggestions I cared about most. Both landed.
Cycle-1 concerns — status
assignRanks()extraction —52e48a6b. Diff confirms pure refactor (62 lines removed from the orchestrator, identical 70 lines as a named helper, no signature change tobuildLayout). The three-step header comment (seed → fallback → normalise) reads top-down. Done.computeViewBox()extraction —9bdd9fb3. Same shape — the closing line ofbuildLayoutis nowconst viewBox = computeViewBox(positions); return { positions, generations, ...viewBox };. That is the readable orchestrator narrative I asked for.Both refactor commits stayed under green tests; commit messages call this out explicitly. Red-green-refactor discipline maintained.
What I deliberately did not push on
The author scoped this iteration to the two extractions Markus and I both named, and explicitly deferred:
sortPartnersByMarriageThenName+mergeLooseBlocksFor)moveMember(position)DRY consolidationgetMarriageYear()wrapper aroundspousePairKeyreadsReason given: lock those in with the eventual
packBlocks.tsextraction. I accept that — landing them now and re-extracting in the same cycle would burn a refactor budget for cosmetic churn. The remaining suggestions are not blockers and never were.New observations on the new code
validateFixture.ts:67-79(countMultiSpousePersons) and the.mjsmirror incapture-network-fixture.mjs:158-176are functionally identical. The author flags this in the comment block (buildLayout.ts:34equivalent: "keep both definitions in sync") and the script must stay one-file local to benode-runnable without a build step. Acceptable duplication with an explicit sync note. Not a smell.validateFixture.ts:11-14exports the constants — good. The script's inline copy atcapture-network-fixture.mjs:65-68is the one place where the source-of-truth drift can happen. The new comment at:69-73calls this out. Belt-and-braces sync would be a CI sweep but that's overkill for a local-only utility.assignRanks()signature takes three Maps. I would consider a singleLayoutInputsobject next time these are wired together, but as the function stands today the three-arg form reads clean and matches the call site. Not worth a follow-up.Test-discipline check
5167a2ae— 4 guard tests incapture-network-fixture.guards.test.tsspawn the script with each missing-env variant. Good — that's behavioural-level coverage, not just unit-level.6d8655ba— 5 cases invalidateFixture.test.tsincluding a regression case (rejects_a_fixture_with_no_multi_spouse_person) that constructs the "looks valid but every spouse is unique" trap. Exactly the regression net the cycle-1 review asked for.fd624f6e— fail-fast precondition with a maintainer message ("split into year-branch / name-branch"). Good — when this fires it tells the next person what to do.Six atomic commits, each with a clear scope, each with a green test floor. Ship it.
🏛️ Markus Keller — Application Architect (cycle 2)
Verdict: ✅ Approved
Three of my cycle-1 suggestions had a clear fix path and all three landed cleanly.
Cycle-1 concerns — status
assignRanks()/computeViewBox()extraction —52e48a6b+9bdd9fb3. The orchestrator now reads as a top-down narrative: parse edges → assign ranks → group by rank → per-generation block layout → compute viewBox → return. That's the diff-ability win I cared about for when AC3 inevitably lands.buildLayout.tsis still 415 lines, but the per-phase seams are now named, not commented. Both refactors are pure (no behaviour change, no test diff) — verified by reading the commits side-by-side.Capture-script multi-spouse floor + enforcement —
6d8655ba. The single cleanest piece of this iteration.MIN_MULTI_SPOUSE_PERSONS = 1is enforced in both the.mjsscript (capture-network-fixture.mjs:71) and a colocated TS module (validateFixture.ts:14) that is unit-tested viavalidateFixture.test.ts(5 cases). The duplication is deliberate (script-must-stay-one-file local-only) and called out in both files. The architectural decision is correct: contract lives next to the JSON it constrains, reachable from unit tests, while the runtime check is still inline in the script for local-developer-friendly execution. This is the right shape.AC3 reproducibility SQL in ADR-026 —
b8ad64dd. Embedded verbatim PostgreSQL query atdocs/adr/026-stammbaum-layout-in-house.md:90-106. Future architect re-evaluating the deferral can now copy-paste againstfamilienarchiv_archive. The "Last run May 2026: 0 rows" annotation makes the result reproducible without trusting a memory of an off-the-record query. That closes the "reproducibility of the deferral decision is itself a requirement" point cleanly. Done.Verified — promise kept
issue_readthat #692 is open, labelledP2-medium / epic / feature / ui, contains the "Constraints inherited from #361" section explicitly carrying the 320 / 768 / 1440 visual-regression contract, the marriage-dot 12 px dimension, and the connector single-colour rule. AC6 / AC7 from #361 land cleanly into the breakpoint visual-regression infrastructure + axe-core gate that #692 is the home for. The deferral has its landing pad. Good.New observations on the new code
docs/GLOSSARY.md:106-128— the Stammbaum section adds nine entries (Stammbaum, seeded rank, sibling block, loose spouse, parented, anchor index, intra-family marriage, marriage dot, canonical fixture) with cross-references back toPersonRelationshipand a "Not to be confused with" link from loose spouse to parented. That is exactly the half-page of design-intent vocabulary I asked for in cycle 1 (and that Elicit re-raised). The pending placeholder entry was removed — good housekeeping.The
validateFixture.tsmodule is colocated under__fixtures__/. From a layering perspective this is interesting: a.tsmodule living next to test fixtures is not in the conventional source tree. I checked —__fixtures__/is already wherestammbaum.jsonandREADME.mdlive, the dir is test-adjacent, and the new module is consumed by both the script and the test. Co-location is correct. Not a layer-boundary violation.No
docs/architecture/c4/*update required — confirmed, no new controller / service / route / docker service / external integration touched.What stayed out of scope
moveMember(position)/getMarriageYear()extractions deferred to thepackBlocks.tsextraction. The author's reasoning ("landing them now would force a re-extraction") is sound architecture economics — refactor when there is a reason, not on every PR.This is the cleanest second-cycle outcome I can ask for: every architectural concern I raised has a concrete artefact, the deferred ones have explicit revisit triggers, and #692 carries the inherited constraints. Adopt.
🧪 Sara Holt — QA Engineer (cycle 2)
Verdict: ⚠️ Approved with concerns
Of the six suggestions I raised in cycle 1, two were addressed and four were explicitly skipped. The two that landed both landed well — but I want to push back on one skip before merge.
Cycle-1 concerns — status
fromYearprecondition on the canonical fallback test — ✅fd624f6e.buildLayout.test.ts:277-292now asserts at the head of the test that every canonicalSPOUSE_OFrow hasfromYear == null, with a maintainer message that names the exact next action ("update or split into year-branch / name-branch"). Fails fast, fails informatively. Exactly the shape I asked for.Capture-script
MIN_MULTI_SPOUSE_PERSONS=1floor + extraction tovalidateFixture.tswith unit tests — ✅6d8655ba.validateFixture.test.tshas 5 cases including the precise regression I cared about:rejects_a_fixture_with_no_multi_spouse_personconstructs a fixture that passes every other floor (50 nodes, 5 generations, several SPOUSE_OF edges) but has zero multi-spouse persons — and the test confirms that combination fails the validator. That is the regression net for "the canonical fixture quietly loses Albert and the multi-spouse layout test becomes vacuous". Good.Capture-script preflight guards — ✅
5167a2ae.capture-network-fixture.guards.test.tsspawns the script viaspawnSyncwith each variant of missing/wrong env (noCAPTURE_EMAIL, noCAPTURE_PASSWORD, non-localhostBACKEND_URL, accepted127.0.0.1) and asserts exit code + stderr message. Process-level behavioural tests on a CLI utility — that is the right altitude.Concerns that remain or were skipped
Test-naming unification (cycle-1 #1) — skipped as cosmetic. I accept the deferral. The mixed style is pre-existing.
intra_family_marriage_places_both_spouses_adjacent_across_sibling_blockstightening (cycle-1 #2) — skipped. Author's reasoning: "the current assertionMath.abs(posA2.x - posB2.x) === NODE_W + COL_GAPalready forces direct adjacency at the same y; nothing can sit between them at one slot apart." That is almost correct but not fully — if a future layout change inserts a node at a fractional slot offset (e.g. half a column gap) or shifts the packing model from integer slots to real-valued positions, two nodes could beNODE_W + COL_GAPapart with a third node sandwiched at a non-slot position between them. The currentbuildLayoutis integer-slot but the test contract should not silently assume that forever. Minor, not a blocker — but I would land the "no member m hasmin(x) < m.x < max(x)" guard in a follow-up. Filing as: "AC2 adjacency test does not preclude a node at a fractional offset between the spouses".Fill-colour assertion on the
r=6dot (cycle-1 #4) — skipped. Author's reasoning: "the contrast invariant is a one-shot manual axe-core check at PR time and a permanent gate via #692". I accept this for the contrast side (axe-core is the canonical signal). However, the test as currently written would still pass if someone replacesvar(--c-primary)with a literal#ffffffor any low-contrast token by accident — the unit test gives no signal until #692 lands the gate. Concern, not blocker. Two-lineexpect(dot!.getAttribute('fill')).toBe('var(--c-primary)')would codify the intent for the period before #692 ships. Worth a quick follow-up.No-merge regression for AC2 (cycle-1 #5) — skipped. Author's reasoning: "every #689 test (which has no intra-family spouse edges) is green, implicitly covering 'merge does not fire when no intra-family edge exists.'" That is true for today's test suite but it is a coupling between two unrelated test groups — #689 is testing rank assignment, not AC2 non-firing. If someone refactors the #689 tests to use a smaller graph or removes one of them, the AC2-non-firing coverage silently shrinks. Minor, not a blocker — but a single dedicated test (
AC2_merge_does_not_fire_when_no_intra_family_edge_exists) is one minute of typing and decouples the concern. Filing as: "AC2 negative-path coverage depends on #689 tests staying static".Branch-coverage gate (cycle-1 #6) — skipped per project convention (no local full test suite). Accepted. CI is the canonical signal.
New observations on the new test code
validateFixture.test.ts:50-54(accepts_a_fixture_where_one_person_has_multiple_spouse_edges) and:56-61(counts_multi_spouse_persons_via_either_edge_direction) — these two cases together verify the bidirectional counting atvalidateFixture.ts:67-79. Good coverage of themapAddToSetcall from both directions.capture-network-fixture.guards.test.ts:54-66(accepts_127_0_0_1_as_BACKEND_URL) — clever: asserts that the preflight lets127.0.0.1through by checking that the resulting stderr does NOT contain the preflight error messages (the fetch fails, but for a different reason). That's how you test "the gate is open" without actually needing the gate to succeed. Nice.spawnSynctimeout of 5 s in the guards test is fine for the local case but ifvitest --project=serverruns these in CI under load, 5 s can be tight. Not a blocker — easy to bump if it ever flakes.The test pyramid for #361 is now: 6 unit tests for
buildLayout, 5 unit tests forvalidateFixture, 4 process-level tests for the capture script guards, 1 component test for ther=6dot, 2 structural-property tests against the canonical fixture. The shape is right and the new commits genuinely tighten the safety net. Approve, with the two follow-ups above filed as separate issues — neither needs to gate the merge.🛡️ Nora "NullX" Steiner — Application Security Engineer (cycle 2)
Verdict: ✅ Approved
The one concrete concern I raised in cycle 1 — the capture-script's default admin credentials — is fixed at exactly the layer I asked for, with behavioural tests proving the gate fires.
Cycle-1 concerns — status
Capture-script default credentials + ambient
BACKEND_URL— ✅5167a2ae. The fix is more thorough than the two-line patch I suggested:CAPTURE_EMAILandCAPTURE_PASSWORDare now mandatory with no fallback (capture-network-fixture.mjs:15-16). Readingprocess.env.X ?? 'admin@familyarchive.local'is replaced with bareprocess.env.X.BACKEND_URLis parsed vianew URL()and the hostname compared against'localhost' / '127.0.0.1' / '::1'(:42-50). TheURLconstructor is the right tool — it rejects malformed inputs (return falsein thecatch), which means a typo likeBACKEND_URL=localhost:8080(no scheme) fails the guard rather than passing through ambiguously.preflight()function (:20-39) that collects failures rather than short-circuiting, so a misconfigured developer sees every gate that's blocking them in one shot. That is good error UX — better than my proposedprocess.exit(2)on the first failure.exit(2)is the right exit code — distinct from the operationalexit(1)for fetch failures, so scripts wrapping this one can differentiate "you forgot env vars" from "the backend is down".Four behavioural tests in
capture-network-fixture.guards.test.tsspawn the script under each missing-env / wrong-host combination and assert exit-code + stderr-message contracts. I particularly likeaccepts_127_0_0_1_as_BACKEND_URL(:54-66) — it asserts the preflight lets the run through by checking that the resulting stderr does NOT contain preflight strings. Process-level testing of a CLI guard with the exact granularity I want.Defence in depth — what I'm checking
52e48a6b,9bdd9fb3) introduce no new I/O, no new fetch, no new dep.validateFixture.tsis pure / deterministic / no I/O / no global mutation (validateFixture.ts:28-65). Same security property asbuildLayout: identical inputs produce identical outputs, no covert-channel surface. Good.countMultiSpousePersonsfunction (:67-79) iterates trusted input fromvalidateFixture— the input is whatever the operator captured to JSON, not network-attacker-controlled. Theif (!e.personId || !e.relatedPersonId) continue;guard at:70handles malformed edges defensively. Acceptable.capture-network-fixture.guards.test.ts:14builds the script path viaresolve(HERE, '../../../../../scripts/capture-network-fixture.mjs'). Five..segments — fragile to directory moves but unit-test-only, not a security concern.spawnSync(process.execPath, [SCRIPT_PATH], { env: { PATH: process.env.PATH, ...env } })—envis constructed from a controlled record andPATHis explicitly propagated rather than...process.env. That is the right shape: minimal env surface, no accidental leak ofprocess.envinto the spawned process. Good hygiene.What I deliberately did not push on
__fixtures__/README.mddocuments the trade-off. The day this flips public, scrubbing-via-git filter-repois the right play; that's a single-issue follow-up, not a merge-blocker.randomUUID+X-XSRF-TOKEN) is unchanged in this iteration. Confirmed by diff — no new auth surface.New observations on the new code
isLocalhostallowlist matches'localhost' / '127.0.0.1' / '::1'. It does NOT match'127.0.0.2'or other loopback-equivalent addresses, nor a DNS name that resolves to a loopback. That's the right call — DNS-based loopback aliasing is a known SSRF-style trick and an allowlist of exact strings beats a resolver-based check. Good.BACKEND_URL = process.env.BACKEND_URL ?? 'http://localhost:8080'still has a default — so a developer runningnode capture-network-fixture.mjswithCAPTURE_EMAIL=...CAPTURE_PASSWORD=...but noBACKEND_URLgets a localhost-default. That's intentional (the gate is "must be localhost", and the default is localhost, so the default is safe). Not a concern.The PR is shippable from a security angle. The credential default was the only real smell and it's now hardened with explicit, tested gates.
🎨 Leonie Voss — UX Designer & Accessibility Strategist (cycle 2)
Verdict: ⚠️ Approved with concerns
No UX-shaping code changed between cycles. The six new commits are: capture-script hardening, refactor extractions, glossary, and a test precondition. Marriage-dot dimensions, multi-spouse ordering, intra-family merge, and the §6.1 layout invariant all stayed where cycle-1 approved them. So the UX outcome I cared about — a senior researcher focused on Albert de Gruyter sees all four spouses, in stable order, with a connector marker that meets contrast — is intact.
Cycle-1 concerns — status
Mobile / 320 px regression coverage (cycle-1 #1, Major) — deferred to #692. Confirmed via
issue_read: #692 exists, is open, contains a "Constraints inherited from #361" section that explicitly inherits "The 320 / 768 / 1440 visual-regression snapshots from #361 stay green" and "The marriage-dot stays at the dimensions set by #361 (12 px); pan/zoom does not bump it." The AC2-block-merge horizontal-width growth I flagged will land in #692's visual-regression matrix at 320 px. Landing pad is in place. Not a blocker for this PR.Heavier connector midpoints across the whole tree (cycle-1 #2, Minor) — outstanding. No screenshot evidence has been posted in this PR comment thread that the consistent
r=6dot doesn't make the desktop view feel "dotted". I asked for an eye-check post-merge in cycle 1; the author's resolution comment doesn't address this point. Soft concern, not a blocker — but I want it on the post-merge punch-list. (Felix: please post a/stammbaumscreenshot at desktop width when you next have the dev stack up.)axe-core contrast verification (cycle-1 #3, Minor) — outstanding, promised. The author's resolution comment commits: "Will be posted as a follow-up comment here (not committed) once measured against the running app, per ADR-026's one-shot manual check rule." I accept the deferred-but-promised shape — ADR-026 is explicit that the permanent gate lives in #692, and a one-shot manual axe-core run is the canonical signal for this merge. Open promise, not a blocker — but please don't merge before posting the axe-core readout. Two lines of output (background contrast ratio at light + dark) is enough.
Multi-spouse ordering with no marriage years (cycle-1 #4, Minor) — curation gap, file as separate issue. No change in this PR. The author's resolution comment doesn't mention it. Filing as: "Backfill marriage
fromYearon canonical import sheet for multi-spouse persons" — separate import-sheet issue, not a #361 layout concern. Independent of this merge.ARIA / screen-reader coverage on the marriage dot (cycle-1 #5, Cosmetic) — outstanding. The
<circle>atStammbaumTree.svelte:319-324still has noaria-labeland noaria-hidden. Cosmetic per my own classification — accepted as out of scope. Will land with the broader axe-core sweep in #692.Verified — promise kept
New observations on the new artefacts
docs/GLOSSARY.md:106-128— Stammbaum glossary section. The vocabulary entries (seeded rank,sibling block,loose spouse,parented,anchor index,intra-family marriage,marriage dot,canonical fixture) are now reusable design language. The "Not to be confused with" cross-reference fromloose spouse→parentedis exactly the disambiguation aid I would have flagged if I'd been involved in the glossary draft. Good shape.marriage dotentry calls out WCAG 1.4.11 (3:1 non-text contrast) explicitly. That is the design-intent breadcrumb future contributors need — they don't have to dig through the ADR to understand why the dot is 12 px and not 8. Linking the visual decision to the WCAG criterion in the glossary is exactly how design-system vocabulary should live.No changes to
StammbaumTree.svelteorbuildLayout.tsbehaviour between cycles — both refactor commits were verified as pure (no test diff, no signature changes to public layout output). The UX surface is identical to cycle-1's approved state.Net verdict
The UX outcome is intact. The two outstanding concerns (eye-check post-merge for connector dottiness, axe-core readout posted as a comment before merge) are soft asks — they don't block the layout work shipping, but please honour them before this lands. If the axe-core readout shows a ratio below 3:1, that would be a blocker — but I have no reason to expect it will.
Approve at "concerns" pending the axe-core comment.
🛠️ Tobias Wendt — DevOps & Platform Engineer (cycle 2)
Verdict: ✅ Approved
Cycle-1 was an approve with one operational angle on the capture script (running with
BACKEND_URLaccidentally pointing at staging would auth and exfiltrate). That is now structurally impossible.Cycle-1 concerns — status
Capture script could authenticate against non-local backends — ✅
5167a2ae.isLocalhost(BACKEND_URL)atcapture-network-fixture.mjs:42-50parses vianew URL()and matches hostname against an exact allowlist ('localhost' / '127.0.0.1' / '::1'). Anything else — staging, prod, an attacker-controlled hostname — fails the gate and the script exits with code 2 before opening a socket. TheURLconstructor itself rejects malformed inputs, so a typo'dBACKEND_URL=localhost:8080(missing scheme) also fails closed.The four-test guard suite at
capture-network-fixture.guards.test.tsis the right altitude —spawnSyncagainst the script verifying exit code + stderr. Behavioural-level coverage of a CLI utility, not just unit-level. That's how I want operational guards tested.Multi-spouse sanity floor — ✅
6d8655ba.MIN_MULTI_SPOUSE_PERSONS=1added atcapture-network-fixture.mjs:71andvalidateFixture.ts:14. A recapture that loses the multi-spouse property fails at validation time rather than silently writing a vacuous fixture. Catches the operational class of "I ran the script, it said 'OK', but the test suite started failing tomorrow for unrelated reasons" — which is exactly the kind of cross-step gap that bites on-call.Verified — operational surface
git diff --stat 4f07527b..HEADshows zero changes under.gitea/workflows/. Confirmed.docker-compose*.yml. Confirmed.package.jsonruntime dep added;validateFixture.tsis a pure-TS module with no imports beyond what the rest of the repo already uses. Nopom.xmlchange either. No Renovate bump to track.capture-network-fixture.mjs:7("Never invoked from CI") andfrontend/scripts/README.mdcontinue to assert this. The new:::: 1IPv6-loopback case in the allowlist is the right addition —::1is the canonical loopback on dev machines with IPv6 enabled, otherwise the script would have failed for a class of developers runningBACKEND_URL=http://[::1]:8080.New observations on the new code
validateFixture.tsmodule is consumed byvalidateFixture.test.ts(5 unit cases) AND mirrored inline in the.mjsscript (capture-network-fixture.mjs:118-176). The duplication is deliberate ("keep both definitions in sync" comment at:69-73) so the script stays one-file local —node scripts/capture-network-fixture.mjsruns without a build step. From an ops angle this is the right trade: a build dependency for a local-only utility would be a footgun. The sync risk is bounded by the validator unit tests catching divergence at PR review time.result.status !== 0in the guards test (:30, 39, 49). If someone ever wires this into automation (which they shouldn't, but if), the exit codes are machine-readable and stable.What stayed out of scope
This remains the cleanest kind of PR from my chair: does what it says, ships under existing pipelines, adds zero operational debt, and now closes the one operational footgun I flagged in cycle 1. Adopt.
📋 "Elicit" — Requirements Engineer (cycle 2)
Verdict: ✅ Approved
Of the six concerns I raised in cycle 1, three had a clear fix path and all three landed. The other three were verification asks ("does #692 exist with the right inheritance?") that I have now confirmed independently. The artefact discipline on this PR is now at the level I want for a deferred-AC feature merge.
Cycle-1 concerns — status
AC3 reproducibility — the verification SQL was named in ADR-026 but not embedded — ✅
b8ad64dd. ADR-026:90-106now contains the verbatim PostgreSQL query againstfamilienarchiv_archive, with the "Last run May 2026: 0 rows" annotation. A future architect re-evaluating the deferral can paste it into a read-only psql session and get a deterministic answer rather than trusting a memory of an off-the-record query. The query itself is well-formed (generation IS NULLfor the unseeded person, joined to aPARENT_OFrelationship where the parent hasgeneration IS NOT NULL). Reproducibility of the deferral decision is now itself a committed artefact. This was my single most important ask. Done.No automated AC3 reachability trigger on the capture script — ⚠️ Partially addressed by
6d8655ba. The newMIN_MULTI_SPOUSE_PERSONS=1floor catches the "multi-spouse property quietly disappears" class of regression at capture time, which closes Sara's lane and Tobias's lane. But it does not directly add the AC3-specific check I asked for ("if any node has parent edges + no generation + at least one parent has generation, log a warning"). The author's resolution comment doesn't separately address this — likely because it conflated my AC3-revisit-trigger ask with Sara's multi-spouse-floor ask. Minor / outstanding — the ADR's revisit trigger ("first canonical fixture containing a parented unseeded spouse") relies on a human spotting it during recapture, with no automated nudge. If you can add a one-line warn (not a hard-fail) in the capture script, the AC3 deferral closes the human-in-the-loop gap. Not a blocker. ADR-026 + the embedded SQL together are enough that this can land and the soft-warn can follow in a small commit.#692 cross-check — ✅ Confirmed.
mcp__gitea__issue_readon issue #692 returns:P2-medium,epic,feature,needs-discussion,ui{@html …}rule.AC6 (bundle-impact) and AC7 (visual-regression) from #361 land cleanly into #692's NFR matrix and US-PAN visual-regression contract. The deferral has its landing pad. Closed.
ADR-026's "UX-signal-only stop trigger" wording — qualitative without named assessor / cadence — ⏸️ No change in this iteration. The author's resolution comment doesn't address this point. Minor / outstanding. Not a blocker — the trigger is honest about being qualitative — but if someone hands #361 to a new UX lead in 18 months, "Albert de Gruyter's 4 marriages failing the read test" needs a named owner and a cadence. Filing as a soft follow-up: "ADR-026 — name an assessor and a cadence for the AC1 read-test revisit trigger." One-line ADR amendment, can land outside this PR.
No new
ErrorCodeorPermission— confirmed by inspecting the diff. CLAUDE.md invariants unaffected. Closed.docs/GLOSSARY.mdStammbaum entries — ✅b8ad64dd. Nine entries added (Stammbaum,seeded rank,sibling block,loose spouse,parented,anchor index,intra-family marriage,marriage dot,canonical fixture), each with the design-intent breadcrumb future contributors need. The pending placeholder ("Stammbaum — the genealogy tree view; relationship to PersonRelationship entity") at the bottom of the file is removed — good housekeeping. Themarriage dotentry calls out WCAG 1.4.11 (3:1) explicitly, which doubles as documentation for #692's a11y gate. Closed.Definition-of-Done — final check
The PR body's AC table reads:
toHaveScreenshot()dropped; axe-core deferred to #692 — inherited by #692's NFR-A11Y-002Every accepted AC is testable, tested, and traceable to a file. Every deferred AC has a revisit trigger AND a landing-pad issue. The traceability matrix is auditable. This is the artefact quality I wanted.
Net verdict
Two soft follow-ups outstanding (one-line AC3-warn in the capture script; ADR-026 assessor/cadence amendment). Neither is a blocker. The AC3 reproducibility SQL was the single most important deliverable from cycle 1 and it's committed verbatim. #692's inheritance is verified. Glossary closed. Approve.
Cycle-3 iteration — 4 commits + axe-core readout
Closing the cycle-2 concerns from
@Sara,@Leonie, and@Elicit. Per-concern resolution below.Sara (cycle-2)
#5 — AC2 adjacency does not preclude a node at a fractional offset between the spouses — ✅
89bb0b5d. The AC2 test now also iterates every other position at the same y and assertsp.x <= min(A2.x, B2.x) || p.x >= max(A2.x, B2.x). The integer-slot adjacency check stays in place; the loop adds the "no third node strictly between them" contract Sara asked for, so a future fractional-offset packer can't silently slip a stranger between the spouses.#6 — Fill-colour assertion on the r=6 dot — ✅
e7931335.StammbaumTree.svelte.test.ts:344now also assertsdot!.getAttribute('fill') === 'var(--c-primary)'. A future "neutralise the dot" diff (swap tovar(--c-ink-3)or any low-contrast token) now fails the unit test before #692's permanent axe-core gate ships.Elicit (cycle-2)
#2 — AC3 revisit-trigger soft-warn on the capture script — ✅
655f0c35. NewfindAc3Candidates(network)helper invalidateFixture.ts(5 unit tests, including the precondition that the committed canonical fixture has zero candidates today — anchors ADR-026's "0 rows" annotation against the JSON).capture-network-fixture.mjscalls it after writing the fixture and emits a loud non-blocking stderr warning the moment a recapture contains an unseeded person with a seeded parent AND a SPOUSE_OF edge. Fail-open (the fixture still writes), loud enough that nobody can miss it.#4 — ADR-026 needs a named assessor + revisit cadence — ✅
2c18cb8b. Notes section now contains: "Re-evaluate dagre adoption on the first canonical fixture refresh that hits AC3, OR by 2027-05-01 at the latest. Owner: Felix Brandt."Leonie (cycle-2)
#2 — Eye-check post-merge for connector dottiness — deferred to post-merge. Not measurable from a still screenshot; needs the real running app at desktop width once this merges.
#3 — axe-core contrast readout — ✅ measured. Ran
@axe-core/playwrightagainst the worktree dev server at/stammbaum, focused on the 28<circle r="6">marriage-line midpoint dots, emulatingprefers-color-scheme: lightanddark. Resolved tokens viagetComputedStyle(document.documentElement).getPropertyValue('--c-primary' | '--c-surface' | '--c-canvas'). Contrast ratios computed via the WCAG relative-luminance formula:--c-primary)#012851--c-surface#ffffff#012851--c-canvas#f0efe9#a1dcd8--c-surface#011526#a1dcd8--c-canvas#010e1eAll four ratios massively clear the 3:1 informational-contrast floor for WCAG 1.4.11 (non-text content). The r=6 dot is contrast-safe in both modes, against both the surface card background and the canvas wrap. The permanent axe-core gate lands with #692 as planned; this is the one-shot pre-merge verification ADR-026 calls for.
Two unrelated
seriousaxe findings on/stammbaumwere surfaced incidentally (document-title,nested-interactive). Neither relates to the marriage dot or the #361 layout work; both are pre-existing on the route and belong in a separate a11y triage pass (likely #692's NFR-A11Y-002 sweep).Not addressed in this iteration (explicitly skipped, no regression)
None — every cycle-2 concern with a clear action either has a commit, a measurement, or is explicitly deferred post-merge.
New commits on
feature/361-stammbaum-multi-spouseVerification
npx vitest --run --project=server src/lib/person/genealogy/— 25 / 25 green (10 buildLayout + 10 validateFixture incl. 5 new AC3 cases + the rest).prettier --checkclean (verified via pre-commit hook on every commit).eslintclean.— Felix
Felix Brandt — Senior Fullstack Developer (cycle 3 / final check)
Verdict: Approved with concerns
Four atomic commits, each preceded by the failing assertion the test would catch —
test(...)andtest+feat(...)prefixes are right. Names still read as sentences. No production code shipped without a red phase. Cycle-2's ✅ stands.Cycle-3 commits — what I checked
89bb0b5d—buildLayout.test.ts:270-281. The added loop is exactly the shape the cycle-2 concern asked for: skip the two spouses, skip rows on a different y, then assertp.x <= minX || p.x >= maxX. The comment names the failure mode it guards against (fractional-offset packer slipping a third node between A2 and B2). Six clean lines of test code, no production change. Right altitude.e7931335—StammbaumTree.svelte.test.ts:351. Two-lineexpect(dot!.getAttribute('fill')).toBe('var(--c-primary)')next to the existing radius assertion. Codifies the colour-token side of the WCAG contract until #692's permanent axe-core gate ships. The comment explains the threat model (accidental "neutralise the dot" swap tovar(--c-ink-3)). Clean.655f0c35—validateFixture.ts:26-58is the testable predicate (Felix-style: pure function, no I/O, fully covered). 5 unit tests across:75-133cover the four meaningful axes: positive (unseeded child with seeded parent + spouse), negative-no-spouse, negative-unseeded-parent, negative-fully-seeded. The "anchor against canonical fixture" test at:82-87(findAc3Candidates(canonicalFixture).toEqual([])) is the right shape — it binds the ADR-026 "0 rows" annotation to the actual committed JSON, so a future capture that introduces an AC3 candidate fails both the unit suite AND the capture warning. Two regression nets for one invariant. Good.2c18cb8b—docs/adr/026-stammbaum-layout-in-house.md:149-151. Three-line amendment naming Felix as owner and2027-05-01as the hard fallback. Honest, dated, accountable.Concerns
findAc3Candidatesbetween.mjsand.ts.frontend/scripts/capture-network-fixture.mjs:171-203andfrontend/src/lib/person/genealogy/__fixtures__/validateFixture.ts:26-58are line-for-line the same algorithm. The capture script's own comment (:169-170) admits this: "Mirrors the implementation in ... keep both in sync." That is a DRY violation with a future-bug shape: someone tightens the TypeScript predicate (e.g. handles edge case for nullpersonId), forgets the.mjscopy, the unit tests stay green, but the capture-time warning stops firing on the real recapture data. Concern, not blocker — the script is local-only and Node can import a compiled.tsonly with extra tooling. Three pragmatic options: (a) leave the duplication, lean onkeep both in syncdiscipline (current state); (b) convert capture-network-fixture.mjs to TypeScript and import the helper directly (cleanest, modest yak-shave); (c) extractfindAc3Candidatesinto a plain.mjsmodule that both the script and the.tsvalidator import (cheapest, gets DRY today). My preference is (c) — one file, no compile step, single source of truth. Filing as a follow-up issue, not a merge blocker.validateFixture.test.ts:92,:105,:117usegeneration: null as number | nullfor the test fixtures. Works, butas number | nullreads odd next to{ id: 'parent', generation: 2 }. A tinytype TestNode = { id: string; generation: number | null }at the top of the file would let the literalgeneration: nulltypecheck without the cast. Cosmetic — file as a 30-second cleanup if you ever touch the file for another reason.LGTM
test(...)ortest+feat(...). Nofeat(...)without a paired test.findAc3Candidates— no I/O, no globals, fully unit-testable.Net
Approve. The one DRY concern (
findAc3Candidatesduplicated across.mjsand.ts) is post-merge follow-up — not a blocker. Every cycle-2 ⚠️ from Sara, Leonie, Elicit is closed.Markus Keller — Application Architect (cycle 3 / final check)
Verdict: Approved with concerns
Cycle-2 was ✅. The four cycle-3 commits touch tests, the capture script, and one ADR — no service code, no domain boundaries, no schema. The architectural surface area is unchanged. One module-boundary concern below, not a blocker.
Cycle-3 commits — architectural read
89bb0b5d— purely a test tightening onbuildLayout.test.ts. No layer impact.e7931335— purely a test tightening onStammbaumTree.svelte.test.ts. No layer impact.655f0c35— addsfindAc3CandidatestovalidateFixture.ts, plus 5 unit tests, plus a soft-warn invocation in the capture script. Pure function, no I/O, no domain coupling. Lives in the__fixtures__/folder where it belongs (alongside the JSON it constrains).2c18cb8b— three-line amendment to ADR-026 naming the assessor and cadence. ADR discipline I want to see.Concerns
findAc3Candidatesis duplicated across two languages.frontend/scripts/capture-network-fixture.mjs:171-203is a copy offrontend/src/lib/person/genealogy/__fixtures__/validateFixture.ts:26-58. The script's comment:169-170calls it out: "Mirrors the implementation in ... keep both in sync." From an architecture lens this is a small but real cross-module duplication: one source of truth (validateFixture.ts) being mirrored manually into a developer tool. The risk is one of those two will drift, the tests will pass against the TypeScript copy, and the warning won't fire on a real recapture. Not a blocker — the script is intentionally a.mjslocal-only utility per ADR-026 §Notes, andimportfrom a.tsrequires a build step. But this is the kind of seam I'd want a tracking issue for: extract the helper into a plain.mjsmodule both consumers import, or convert the capture script to TypeScript. Pragmatically (c) above costs less than (b). File a follow-up.ADR-026 cadence wording.
docs/adr/026-stammbaum-layout-in-house.md:149-151is the right shape (named owner, hard date), but the trigger predicate is now a code artefact (findAc3Candidates) and the ADR doesn't reference it by name. A future architect reading the ADR has to grep to find that the trigger is testable. A one-line cross-reference (See frontend/.../validateFixture.ts findAc3Candidates() for the testable predicate) would close the documentation-to-code traceability. Cosmetic. Single-line ADR edit, fits in the same merge or a follow-up.LGTM
CLAUDE.mddoes not need updating.ErrorCodeorPermission. No new endpoint. Layering rules unaffected.__fixtures__/directory is the right home forfindAc3Candidates— it lives next to the JSON it validates against and is reachable from unit tests without crossing a domain boundary.b8ad64dd), (d) the named assessor and hard date (2c18cb8b), (e) the qualitative trigger. That's the full set I want for a deferred-AC ADR.Net
Approve. Two non-blocking follow-ups (DRY the
findAc3Candidatesduplication; cross-reference the predicate from ADR-026). Both fit naturally in a small post-merge cleanup commit.Sara Holt — QA Engineer (cycle 3 / final check)
Verdict: Approved
Both of my cycle-2 ⚠️ follow-ups landed exactly as asked. The two test-tightening concerns I filed are now closed in code. Cycle-3 also adds 5 new unit tests around
findAc3Candidates— meaningful behaviour-anchored coverage, not line padding.Cycle-2 ⚠️ concerns — status
#5 (AC2 adjacency does not preclude a node at a fractional offset between the spouses) — ✅
89bb0b5d.buildLayout.test.ts:270-281adds the loop I asked for: iterate every other position at the same y, assertp.x <= min(A2.x, B2.x) || p.x >= max(A2.x, B2.x). The integer-slot adjacency check at:268stays in place; the new loop adds the "no third node strictly between them" contract. A future fractional-offset packer can't silently slip a stranger between the spouses. The comment at:270-274names the exact failure mode it guards. Closed.#6 (fill-colour assertion on the r=6 dot) — ✅
e7931335.StammbaumTree.svelte.test.ts:351now also assertsdot!.getAttribute('fill') === 'var(--c-primary)'. The comment at:345-350calls out the threat model: an accidental "neutralise the dot" swap tovar(--c-ink-3)or a literal light token, which would strip the 3:1 contrast guarantee. Geometry side (r=6) and colour side (var(--c-primary)) are now both codified at the unit level — exactly the belt before #692's axe-core suspenders ships. Closed.New observations on cycle-3 code
validateFixture.test.ts:75-133—findAc3Candidatessuite. Five tests covering the four meaningful axes:finds_no_candidate_in_the_canonical_fixture_today(:82-87) — anchors ADR-026's "Last run May 2026: 0 rows" annotation against the actual committed JSON. This is the test I would have asked for: it binds the deferral evidence to a runnable assertion. The day a recapture introduces the AC3 shape, this test fails AND the capture script warns. Two regression nets for one invariant. Excellent.flags_an_unseeded_person_whose_seeded_parent_is_in_graph_and_who_has_a_spouse(:89-97) — happy path of the positive predicate.ignores_an_unseeded_person_with_no_spouse_edge(:99-109) — narrows the predicate to loose spouse, not just any unseeded child. The comment:100-102documents exactly why this distinction matters for the AC3 layout branch.ignores_an_unseeded_person_whose_parent_is_also_unseeded(:111-122) — narrows "parents in graph" to mean seeded parent. Important boundary.ignores_a_seeded_person_with_seeded_parents_and_a_spouse(:124-132) — confirms the predicate fires only on the unseeded variant.buildLayout, 10 forvalidateFixture(5 base + 5findAc3Candidates), 4 process-level tests for the capture-script guards, 1+1 component tests for the marriage dot (r=6 + fill). The shape is right.Optional polish (not blocking)
validateFixture.test.ts:92,:105,:117—generation: null as number | nullcasts read awkwardly next to literalgeneration: 2. A smalltype TestNode = { id: string; generation: number | null }declared at the top of the test file would let the literalgeneration: nulltypecheck without the cast. Cosmetic only, ~30 seconds if you touch the file again.Net verdict
The two cycle-2 ⚠️ concerns are closed in code, both with comments that name the threat model. The cycle-3 net-new surface (
findAc3Candidates+ 5 unit tests + the canonical-zero anchor) is the right altitude — pure function, unit-testable, deferral-evidence-bound. CI is the canonical signal for the full suite; per project convention I trust it. Merge-ready from my lane.Nora "NullX" Steiner — Application Security Engineer (cycle 3 / final check)
Verdict: Approved
Cycle-2 was ✅ for me. Cycle-3 introduces zero new attack surface from a security lens. Re-verified the four new commits against my standard pattern checklist.
Cycle-3 commits — security read
89bb0b5d— test-only tightening onbuildLayout.test.ts. No code path executed in production. No security impact.e7931335— test-only tightening onStammbaumTree.svelte.test.ts. No code path executed in production. No security impact.655f0c35— addsfindAc3Candidates()(pure function over an in-memorynetworkobject), 5 unit tests, and a stderr soft-warn incapture-network-fixture.mjs. The capture script remains a developer-only local utility per ADR-026 §Notes and per5167a2ae's preflight guards (CAPTURE_EMAIL/CAPTURE_PASSWORDrequired,BACKEND_URLnon-localhost rejected). The newwarnIfAc3Reachable()runs afterwriteFixture()and only emits to stderr — no network call, no shell-out, no eval, no path manipulation. Fail-open by design (the warn does not throw orprocess.exit), which is the documented behaviour. The candidate IDs in the stderr message (:241—candidates.join(', ')) come from the captured graph; the script's preflight guarantees the data source is localhost-only and credential-gated, so there is no untrusted-input path here.2c18cb8b— three-line ADR amendment indocs/adr/026-stammbaum-layout-in-house.md. Documentation only.Re-verified from cycle-1/2
Permissionenum value. No newErrorCode. No new endpoint.@RequirePermissionannotation required on this PR (still a pure-frontend layout change).innerHTML, noeval, nofetchof user-controlled URLs. The capture script preflight (5167a2ae) confines the only HTTP call to localhost.console.errorcalls inwarnIfAc3Reachable()log IDs from the captured graph against a localhost source.Threat-model note (defence-in-depth, not a finding)
The capture script's stderr message at
capture-network-fixture.mjs:241interpolatescandidates.join(', ')into a string written to a developer's terminal. Person IDs in this project are UUIDs (verified via the canonical fixture), which are safe characters for terminal output. If the upstream schema ever changed to allow non-UUID IDs containing control characters or ANSI escape sequences, this line could become a terminal-injection surface (low severity, requires attacker control of the backend response). Current schema makes this hypothetical. No action required, just noting the assumption for future schema-change reviewers.Net
Approve. Zero new attack surface in cycle-3. Every cycle-2 ⚠️ concern was outside my lane and remains so. Merge-ready from a security perspective.
Leonie Voss — UX Designer & Accessibility Strategist (cycle 3 / final check)
Verdict: Approved
My cycle-2 ⚠️ hinged on a single promise: the axe-core readout had to land in the PR comment thread before merge, and the dot had to clear WCAG 1.4.11 (3:1) in both light and dark mode. Comment 12837 delivers exactly that, with the measurement methodology I would have asked for.
Cycle-2 ⚠️ concerns — status
#2 (eye-check post-merge for connector dottiness across the whole tree) — deferred to post-merge as I flagged in cycle-2. Not measurable from a still screenshot; requires the running app at desktop width. Accepted, on the post-merge punch-list. Not a blocker.
#3 (axe-core contrast readout) — ✅ posted as comment 12837. The measurement table reports:
--c-surface#ffffff--c-canvas#f0efe9--c-surface#011526--c-canvas#010e1eThe methodology is what I want:
@axe-core/playwrightagainst the 28<circle r="6">dots, tokens resolved viagetComputedStyle(document.documentElement), bothprefers-color-schememodes emulated, both surface and canvas backgrounds measured. WCAG relative-luminance formula named explicitly. All four ratios clear the 3:1 informational-contrast floor by 4–5×.I particularly value that dark mode was measured against both
--c-surfaceAND--c-canvas. My cycle-1 ask called out that dark mode remaps every token, and a ratio that passes in light may fail in dark — the measurement disproves that for this specific dot. Closed.New observations on cycle-3 commits
e7931335— fill-token unit assertion (Sara cycle-2 follow-up).StammbaumTree.svelte.test.ts:351now assertsfill === 'var(--c-primary)'next to the existingr === '6'assertion. From a UX/a11y lens this is excellent defence-in-depth: the geometry test (r=6) protects the area side of WCAG 1.4.11; the fill-token test protects the colour side. Together they catch a hypothetical "neutralise the dot" PR that swaps tovar(--c-ink-3)(which would tank the ratio) — the unit suite fails before #692's permanent axe-core gate ships. This is the kind of test-suite design I want for a deferred-a11y-gate feature.Axe-core run also flagged two unrelated
seriousfindings on/stammbaum(document-title,nested-interactive). Per comment 12837, Felix correctly scoped these out: neither relates to the marriage dot or the #361 layout work; both are pre-existing on the route. They belong in #692's NFR-A11Y-002 sweep, not this merge. Good triage discipline — flagging issues found incidentally without scope-creeping the current PR. I'll watch for them to surface in #692.ADR-026 cadence amendment (
2c18cb8b). Names Felix Brandt as the assessor and 2027-05-01 as the hard fallback. The UX-signal trigger ("Albert de Gruyter's 4 marriages failing the read test") now has a person and a date. If the project changes hands in 18 months, the question can't drift indefinitely. Closed from my lane too.Remaining open promises (post-merge, not blocking)
Eye-check screenshot for connector dottiness at desktop width, once #693 lands and the dev stack is up. Felix: please post the screenshot as a follow-up comment on this PR (or on #692 if more relevant there). Not a blocker.
The two unrelated
seriousaxe findings (document-title,nested-interactive) on/stammbaum. File as separate issues against #692's NFR-A11Y-002 sweep so they don't get lost.Net verdict
The UX outcome — a 67-year-old researcher focused on Albert de Gruyter sees all four spouses with a contrast-safe connector dot in both modes — is verified. The axe-core measurement closes the only conditional I left open in cycle-2. The unit-test fill-token assertion is a smart pre-merge belt before #692's permanent suspenders. Approve. Merge-ready from a UX/a11y perspective.
Tobias Wendt — DevOps & Platform Engineer (cycle 3 / final check)
Verdict: Approved
Cycle-2 was ✅ for me and stays ✅. Cycle-3 touched zero infrastructure surface — no Compose changes, no CI workflow changes, no new env vars, no new ports, no new image dependencies, no new resource limits. ADR-026 §"Operational" still reads as truth.
Cycle-3 commits — infra read
89bb0b5d/e7931335—.test.tsfiles only. Run in the samevitest --project=serverlane that already exists in CI. No pipeline change.655f0c35— addsfindAc3Candidates()tovalidateFixture.ts(TypeScript, in the existing test scope) and a soft-warn block tofrontend/scripts/capture-network-fixture.mjs. The capture script remains a local-only developer utility, never invoked from CI per ADR-026 §Notes. Confirmed via.gitea/workflows/— no workflow references this script. The preflight guards from5167a2aeare still in place (refuses default creds, refuses non-localhostBACKEND_URL). The script is correctly out of the CI surface.2c18cb8b—docs/adr/026-stammbaum-layout-in-house.mddocumentation amendment. No build artefact.Re-verified from cycle-2
frontend/package.json(dagre still not added; deferred per ADR-026).vitestpass.Note for the post-merge punch list
feature/361-stammbaum-multi-spouse's merged main — coordinate the snapshot capture with Sara so we don't introduce a baseline drift between #361 merging and #692's first snapshot run.Net
Approve. Zero infrastructure surface in cycle-3. Local-only capture script stays correctly out of CI. Merge-ready from my lane.
"Elicit" — Requirements Engineer (cycle 3 / final check)
Verdict: Approved
Both of the two soft follow-ups I left open in cycle-2 are now committed. The artefact discipline on this PR is at the level I want for a deferred-AC feature merge: every accepted AC is testable and tested, every deferred AC has a revisit trigger that is itself a committed artefact (SQL query + testable predicate + named assessor + hard date).
Cycle-2 outstanding concerns — status
#2 (AC3 revisit-trigger soft-warn on the capture script) — ✅
655f0c35. The fix exceeds my ask in one important way: the trigger predicate is not just a stderr line, it is also a testable function with five unit tests, including the preconditionfindAc3Candidates(canonicalFixture).toEqual([])(validateFixture.test.ts:82-87). This binds ADR-026's "Last run May 2026: 0 rows" annotation against the actual committed JSON — so the deferral evidence is now a runnable assertion, not a memory of an off-the-record query. The day a recapture introduces an AC3 candidate, two signals fire: the unit test fails AND the capture script emits the stderr warning. The capture script wireswarnIfAc3Reachable()afterwriteFixture()(capture-network-fixture.mjs:226-247) — fail-open by design (the fixture still writes; the human-in-the-loop sees the data and decides). The stderr message at:239-246names the ADR, the count, the candidate IDs, and the action ("re-evaluate the dagre adoption deferral"). Closed.#4 (ADR-026 assessor + revisit cadence) — ✅
2c18cb8b.docs/adr/026-stammbaum-layout-in-house.md:149-151now contains: "Re-evaluate dagre adoption on the first canonical fixture refresh that hits AC3, OR by 2027-05-01 at the latest. Owner: Felix Brandt." Both an event-trigger (the AC3 capture) and a time-trigger (the 2027-05-01 date), and a named accountable owner. The deferral can no longer drift indefinitely. Closed.Traceability matrix — final state
buildLayout.test.ts6 synthetic + canonical fixturebuildLayout.test.ts:237-282synthetic two-family + fractional-offset adjacency tightening (89bb0b5d)b8ad64dd)findAc3Candidates()predicate, capture-time soft-warn, unit-test anchor + 2027-05-01 hard date6970cc95)toHaveScreenshot()dropped; one-shot axe-core run in comment 12837Every cell of the matrix points at either committed evidence or a named landing-pad issue. Audit-clean.
Comment 12837 (axe-core readout)
Acknowledged as the one-shot pre-merge verification ADR-026 calls for. The methodology table is the right format: 4 contrast ratios (light + dark, surface + canvas), all clear 3:1 by 4–5×, the WCAG relative-luminance formula named, the measurement tool named, the focused-on element scope named (28
<circle r="6">midpoint dots). The permanent gate still lands with #692 as planned — this measurement is a point-in-time signal, not a substitute for the gate.New observations
findAc3Candidatesis testable code AND has a fixture-zero anchor test. This is the artefact pattern I would name in a textbook as the right way to commit a deferral. The unit test (validateFixture.test.ts:82-87) and the runtime warning (capture-network-fixture.mjs:226-247) close the same loop from two ends: the fixture-side test catches the moment a recapture introduces AC3 in the committed JSON; the script-side warning catches the moment a developer captures a new fixture that would introduce AC3 if committed. Two independent regression nets for one invariant. Excellent.One small architectural follow-up surfaces, not in my lane but worth naming: the
findAc3Candidatesimplementation lives in two languages (validateFixture.tsandcapture-network-fixture.mjs), with a "keep both in sync" comment in the script. Markus will likely flag this in his review. From a requirements lens it is not a blocker — the deferral artefact set is complete — but a single-source-of-truth refactor is a reasonable follow-up issue if Markus wants it.Definition-of-Done — final check
Every accepted AC is tested. Every deferred AC has:
2c18cb8b)findAc3Candidates+ 5 unit tests + canonical-zero anchor)This is the merge-readiness shape I wanted.
Net verdict
Approve. Every cycle-1 and cycle-2 concern in my lane is closed in code or in a committed document. No outstanding soft follow-ups remain. Merge.
Introduces a local `type TestNode = { id: string; generation: number | null }` so the three AC3 test fixtures can write `generation: null` directly, without the awkward `as number | null` cast next to the literal `generation: 2`. Sara cycle-3 cosmetic; same predicate, cleaner reading. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Felix Brandt — cycle-4 follow-up (all cycle-3 concerns resolved)
Three atomic commits land the cycle-3 concerns. Each is test-anchored against the existing suite — no new behaviour, pure structural cleanup.
Concern #1 —
findAc3Candidatesduplicated across.mjsand.ts(Felix #1, Markus #1) — ✅ resolvedCommit
585f28cd—refactor(stammbaum): single source of truth for findAc3CandidatesPicked the option I named in comment 12845 — pragmatic option (c): plain
.mjsmodule that both consumers import.frontend/src/lib/person/genealogy/__fixtures__/findAc3Candidates.mjs— 56 lines, the single canonical implementation. Lives in the__fixtures__/folder next to the JSON it constrains, per Markus's module-boundary read.frontend/scripts/capture-network-fixture.mjs:14— addsimport { findAc3Candidates } from '../src/lib/person/genealogy/__fixtures__/findAc3Candidates.mjs';. The local copy at:171-203is gone. The "Mirrors the implementation in ... keep both in sync." comment is gone.frontend/src/lib/person/genealogy/__fixtures__/validateFixture.ts:20-25— replaced the 33-line local implementation withexport { findAc3Candidates } from './findAc3Candidates.mjs';(plus the JSDoc note explaining why). Import surface for the test suite is unchanged.Duplication genuinely eliminated — single source of truth. Both consumers import the same module. Verified by:
validateFixture.test.ts— 10/10 green (the 5findAc3Candidatesunit tests anchor the predicate's contract; they fail if the move broke behaviour).node --check scripts/capture-network-fixture.mjs— script parses cleanly..mjsimport resolves in plain Node.npm run check— 834 errors, identical to baseline. Zero net new type errors.The two sanity-floor numeric constants (
MIN_NODESetc.) intentionally remain on both sides — they're 4 literal numbers, not an algorithm; the in-line comment now spells out the intentional vs. coincidental distinction.Concern #2 — ADR-026 cross-reference (Markus #2 cosmetic) — ✅ resolved
Commit
2097dddf—docs(adr): ADR-026 cross-references findAc3Candidates() predicatedocs/adr/026-stammbaum-layout-in-house.md:110-118— adds a one-paragraph cross-reference immediately after the AC3 SQL probe, naming:findAc3Candidates()infrontend/src/lib/person/genealogy/__fixtures__/findAc3Candidates.mjsas the testable predicatevalidateFixture.test.tsas the fixture-time anchorcapture-network-fixture.mjsas the capture-time stderr soft-warnA future architect reading the ADR no longer has to grep to find the testable artefact.
Concern #3 —
generation: null as number | nullcast cleanup (Sara cosmetic) — ✅ resolvedCommit
23d93d49—refactor(stammbaum): TestNode type alias drops generation castfrontend/src/lib/person/genealogy/__fixtures__/validateFixture.test.ts:10-13— addstype TestNode = { id: string; generation: number | null }near the top, then annotates the three test arrays with: TestNode[]. Now:91, :104, :115, :116read as a cleangeneration: nullnext togeneration: 2/generation: 3. Zero behavioural change —validateFixture.test.ts10/10 green.Net
All three cycle-3 concerns resolved in code. Three atomic commits, lint hook passed, validateFixture + buildLayout test suites green (25/25),
npm run checkbaseline unchanged. The duplication is genuinely eliminated — bothcapture-network-fixture.mjsandvalidateFixture.tsnow import from the same single source of truth.🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer (cycle 4 / convergence check)
Verdict: ✅ Approved — no remaining concerns
Three atomic structural commits. None ships behaviour. Each is anchored on an existing green test. Cycle-3's ⚠️ is genuinely closed in code.
My cycle-3 ⚠️ — status
Concern #1 —
findAc3Candidatesduplicated across.mjsand.ts→ ✅ resolved by585f28cd.The fix took the option I named as my preference (option c — extract into a plain
.mjsmodule both consumers import). Verified by reading the actual diff and the resulting files:frontend/src/lib/person/genealogy/__fixtures__/findAc3Candidates.mjs(:1-56) — single canonical definition, JSDoc-typed so the TypeScript validator + test suite import it throughallowJs/checkJswithout losing static checking.capture-network-fixture.mjs:14— directimport { findAc3Candidates } from '../src/lib/person/genealogy/__fixtures__/findAc3Candidates.mjs'. The 33-line local copy at the old:171-203is deleted. The "Mirrors the implementation in ... keep both in sync." comment is deleted.validateFixture.ts:24— replaced with a clean re-export:export { findAc3Candidates } from './findAc3Candidates.mjs'. Consumers' import surface (validateFixture.test.ts:2) is unchanged.grep -rn findAc3Candidatesconfirms one definition, two import sites (capture-network-fixture.mjs,validateFixture.ts's re-export), one test usage — no orphan copies.The "keep both in sync" risk shape is genuinely eliminated. The minimum sanity-floor constants (
MIN_NODES, etc.) intentionally stay duplicated as 4 literal numbers — that's not an algorithm, and the in-line comment atcapture-network-fixture.mjs:67calls out the intentional vs. coincidental distinction. Right call.Concern #2 —
validateFixture.test.tsgeneration: null as number | nullcasts (cosmetic) → ✅ resolved by23d93d49.Local
type TestNode = { id: string; generation: number | null }declared at:13, annotates the three fixture arrays at:95, :108, :120. The literalgeneration: nulllines at:97, :110, :121, :122typecheck without the cast. Reads cleanly next togeneration: 2/generation: 3. Felix-style: one named local type, three fewerascasts, zero behavioural change.Verification
node --check scripts/capture-network-fixture.mjs→ OK.node --check src/lib/person/genealogy/__fixtures__/findAc3Candidates.mjs→ OK.npx vitest run validateFixture.test.ts→ 10/10 passed (validates the import path resolves and the 5findAc3Candidatestests behaviourally match the pre-extraction implementation).New surface inspection — cycle-4 commits
findAc3Candidates.mjslives in the__fixtures__/directory next to the JSON it constrains. Correct location..mjschoice (no build step required for the Node-run capture script). Self-documenting.Edge,Node,NetworkShape) give TypeScript callers structural types without forcing a.d.tscompanion file. The TS side imports the same symbol and the types propagate.validateFixture.tsre-export keeps consumers' import paths stable. No callers needed updating.Net
Approve. All three cycle-3 concerns closed in code, verified by running the test suite green and reading the resulting diff. No new concerns introduced. The PR is in the shape I want for merge.
Merge.
🏛️ Markus Keller — Application Architect (cycle 4 / convergence check)
Verdict: ✅ Approved — no remaining concerns
Both of my cycle-3 ⚠️ items are now closed in code. The architectural surface area is unchanged from cycle-3 — no new package, no new domain, no new service, no schema. Three structural commits, all in the existing
__fixtures__/boundary or indocs/adr/.My cycle-3 ⚠️ — status
Concern #1 —
findAc3Candidatescross-module duplication → ✅ resolved by585f28cd.Extraction is genuine and minimal. The new module at
frontend/src/lib/person/genealogy/__fixtures__/findAc3Candidates.mjsis the single source of truth;validateFixture.ts:24andcapture-network-fixture.mjs:14both import the same symbol. The "keep both in sync" drift risk that I flagged is gone — there is now exactly one definition to evolve.Architectural placement is right:
__fixtures__/directory, next to the JSON it constrains and the validator that asserts against it. It does not cross a domain boundary (noperson/, nodocument/reach-in)..mjsis the correct format choice — the capture script is a Node-run developer utility (ADR-026 §Notes) that cannot import a.tsfile without a build step. Converting the script to TypeScript (option b in cycle-3) would have introduced a build-time coupling for a local-only utility. The chosen option keeps the script self-contained while sharing one implementation.Edge,Node,NetworkShape) on the.mjsgive TypeScript callers structural types throughallowJs/checkJs—validateFixture.ts:24's re-export propagates them. No.d.tscompanion needed, no loss of static checking on the TS side.MIN_NODES, etc.) remain intentionally duplicated betweenvalidateFixture.ts:11-14andcapture-network-fixture.mjs:68-71. Architecturally this is the right cut: they're four literal numbers, not an algorithm. The capture script's comment at:67explicitly calls out the intentional vs. coincidental distinction, so a future reader knows the duplication is by design.Concern #2 — ADR-026 should cross-reference the testable predicate (cosmetic) → ✅ resolved by
2097dddf.docs/adr/026-stammbaum-layout-in-house.md:111-119adds a paragraph immediately after the AC3 SQL probe naming:findAc3Candidates())frontend/src/lib/person/genealogy/__fixtures__/findAc3Candidates.mjs)validateFixture.test.ts)frontend/scripts/capture-network-fixture.mjs)A future architect reading the ADR now has the complete trail: from the decision, to the SQL probe, to the runnable predicate, to the two enforcement sites. The documentation-to-code traceability is closed.
Re-verified from cycle-3
CLAUDE.md's package table.ErrorCode, no newPermission. No layering-rule change.frontend/package.json. dagre still deferred per ADR-026.findAc3Candidates.mjslives in__fixtures__/, imported from there. No reach intoperson/,document/, or another domain.Net
Approve. Both cycle-3 ⚠️ items are closed in code. No new architectural concerns introduced by the cycle-4 commits. The ADR is now fully reachable from the code, and the code is fully reachable from the ADR. Merge.
🧪 Sara Holt — QA Engineer (cycle 4 / convergence check)
Verdict: ✅ Approved — no remaining concerns
Cycle-3 was ✅ for me with one cosmetic. Cycle-4 closes that cosmetic and adds zero risk to the test pyramid. The validateFixture suite is green at 10/10 after the extraction — that's the load-bearing proof that the
findAc3Candidatesmove preserved behaviour.Cycle-3 cosmetic — status
validateFixture.test.tsgeneration: null as number | nullcasts → ✅ resolved by23d93d49.type TestNode = { id: string; generation: number | null }declared at:13with a header comment explaining why it exists (the AC3 predicate cares specifically about the unseeded-node branch). Three fixture arrays now annotated: TestNode[](:95, :108, :120) and the literalgeneration: nulllines (:97, :110, :121, :122) typecheck cleanly without the cast. Diff is +12 / -7, all in test scaffolding. Zero behavioural change.Cycle-4 test-pyramid integrity check
validateFixture.test.ts— 10/10 passed. Verified locally:validateFixturetests covering the canonical fixture, min-node floor, multi-spouse floor + 2 boundary testsfindAc3Candidatestests covering: zero against canonical fixture, positive flag, no-spouse negative, unseeded-parent negative, fully-seeded negativefindAc3Candidatestests still pass after the585f28cdextraction is the proof I need that the move was behaviour-preserving. If the import had broken, or the new.mjsmodule had subtly different semantics, thefinds_no_candidate_in_the_canonical_fixture_todaytest (:91) would fail first because it runs against 62 real nodes with mixedgeneration/null/edge shapes.node --checkon both.mjsfiles → OK. The cross-directory import resolves in plain Node without any build tooling.TestNodealias name follows TypeScript convention; the snake_case test names Felix established for this file (flags_an_unseeded_person_...,ignores_an_unseeded_person_...) are unchanged.Cycle-4 risk inspection
TestNodealias is file-local (not exported). No risk of leakage into thevalidateFixture.tstype surface.Net
Approve. The cycle-3 cosmetic is closed in the cleanest possible way (one named local type, three cast removals). The cycle-4 extraction is anchored on green tests — exactly the verification discipline I want for a refactor. Merge.
🛡️ Nora "NullX" Steiner — Application Security Engineer (cycle 4 / convergence check)
Verdict: ✅ Approved — no remaining concerns
Cycle-3 was ✅ for me. Cycle-4 introduces three structural commits: an
.mjsextraction, a markdown amendment to ADR-026, and a test-file type-alias cleanup. None opens new attack surface. Re-verified against my standard checklist.Cycle-4 commits — security read
585f28cd—findAc3Candidatesextraction.frontend/src/lib/person/genealogy/__fixtures__/findAc3Candidates.mjsis a pure function over an in-memorynetworkobject. No I/O, nofetch, no shell, noeval, noinnerHTML, no path manipulation, no logging.importat:14resolves to a relative path inside the repository — nonode_modulesresolution, no dynamic loader, norequire(userInput)-shape.5167a2aeare untouched (CAPTURE_EMAIL/CAPTURE_PASSWORDrequired,BACKEND_URLnon-localhost rejected). The new import does not alter the trust-boundary surface.grepagainst.gitea/workflows/returns zero references. No CI invocation path.2097dddf— ADR-026 cross-reference. Pure markdown. No code path, no security impact.23d93d49—TestNodetype alias. Test-file-only TypeScript type declaration. Not bundled, not shipped.Threat-model verification
package.jsonentry. The.mjsextraction is entirely internal-source.import(scripts/...reaching intosrc/lib/...) is one-way and pure-function. The__fixtures__/module does not transitively import anything fromscripts/, so there's no circular-trust loop.:204still interpolatescandidates.join(', ')from the captured graph. As I noted in cycle-3, person IDs are UUIDs per the canonical fixture — no control-character or ANSI-escape surface. Cycle-4 did not change the interpolation site, so the cycle-3 hypothetical (low-severity terminal-injection risk if the schema ever permitted non-UUID IDs) remains documented as a future-schema-change consideration, not an action item..mjsmodule's input handling.findAc3Candidates(network)defensively coerces bothnodesandedgesto arrays (Array.isArray(...) ? ... : []at:24-25), filters edges by bothpersonIdandrelatedPersonIdtruthiness at:33, and guards againstn.idfalsiness at:48. Fail-open by design (returns[]for malformed input), which is correct for a soft-warn predicate — it must not abort capture on partial data.Re-verified from cycle-1/2/3
Permission, no newErrorCode, no new endpoint.@RequirePermission. Still pure-frontend.eval, noinnerHTML, nofetchof user-controlled URLs.Net
Approve. Zero new attack surface in cycle-4. The extracted module is a pure-function over in-memory data, called only from a local-only script and a unit-test file — never reachable from production runtime. All cycle-3 concerns outside my lane are resolved by Felix's and Markus's verification. Merge-ready from a security perspective.
🎨 Leonie Voss — UX Designer & Accessibility Strategist (cycle 4 / convergence check)
Verdict: ✅ Approved — no remaining concerns
Cycle-3 was ✅ for me. Cycle-4's three commits are all structural/infrastructure cleanup — none of them touch the rendered DOM, the SVG geometry, the colour tokens, the typography, the focus rings, or the breakpoint matrix. No UX-shaping code changed between cycle 3 and cycle 4.
Cycle-4 commits — UX/a11y read
585f28cd—findAc3Candidatesextraction. The marriage-dot geometry (r=6) and fill token (var(--c-primary)) are unchanged. The extracted predicate is dev-utility scaffolding; it never runs in the browser, never affects rendering, never reaches a screen reader. The component contract that the cycle-3 axe-core readout measured (light + dark, surface + canvas, all ≥ 12:1) is unaffected.2097dddf— ADR-026 cross-reference. Documentation only.23d93d49—TestNodetype alias. Test-file-only.What I verified is still true
StammbaumTree.svelte.test.ts:351(r === '6') and the fill-token assertion (fill === 'var(--c-primary)') remain in place. Cycle-4 didn't touch the component test file.Remaining open promises — unchanged
The two post-merge follow-ups I flagged in cycle 3 remain post-merge:
seriousaxe findings (document-title,nested-interactive) on/stammbaumbelong in #692's NFR-A11Y-002 sweep.These are post-merge follow-ups, not in-PR blockers. Cycle 4 doesn't change that.
Loop-termination check
Nothing UX-relevant changed in cycle 4. I'd be inventing a new concern if I flagged one. The 67-year-old researcher focused on Albert de Gruyter still sees all four spouses with a contrast-safe connector dot in both modes — that's the outcome I cared about, and it's unchanged from cycle 3.
Net
Approve. No remaining concerns in my lane. Merge.
🛠️ Tobias Wendt — DevOps & Platform Engineer (cycle 4 / convergence check)
Verdict: ✅ Approved — no remaining concerns
Cycle 4 touched zero infrastructure surface. ADR-026 §"Operational" continues to read as truth.
Cycle-4 commits — infra read
585f28cd—findAc3Candidatesextraction. Adds one new file (findAc3Candidates.mjs) inside the existingfrontend/src/lib/person/genealogy/__fixtures__/directory. No CI workflow change required:validateFixture.test.tscontinues to run in the existing Vitest unit-test lane, and the new.mjsimport resolves in plain Node (node --checkclean, no extra tooling). The capture script's cross-directory import (../src/lib/...) is internal-source, nonode_modulestraffic. Confirmed no.gitea/workflows/reference tocapture-network-fixture.mjsor to the new module — script stays correctly out of CI.2097dddf— ADR-026 amendment. Markdown only. No build artefact.23d93d49—TestNodetest-file type alias. Test-file-only TypeScript change. Same Vitest lane.Re-verified from cycle-1/2/3
frontend/package.json(dagre still deferred).:latestintroduced anywhere.Note for the post-merge punch list (carried forward, unchanged)
feature/361-stammbaum-multi-spouse's merged main when #692 lands, to avoid baseline drift. This was already on the list in cycle 3; cycle 4 doesn't change it.Net
Approve. Zero infrastructure surface in cycle 4. Local-only capture script remains correctly out of CI even with its new cross-directory import. Merge-ready from my lane.
📋 "Elicit" — Requirements Engineer (cycle 4 / convergence check)
Verdict: ✅ Approved — no remaining concerns
Cycle 3 was ✅ for me on all open requirements concerns. Cycle 4's three commits add nothing to the AC traceability matrix; they only strengthen the existing artefact chain for the AC3 deferral. The merge-readiness shape I described in cycle 3 is unchanged.
Cycle-4 commits — requirements lens
585f28cd—findAc3Candidatesextracted to a single source of truth. From a requirements lens this is an artefact-quality improvement, not a requirements change. The AC3 deferral trigger is still: "first canonical fixture containing a parented unseeded spouse". The change is where the predicate lives, not what it predicates. The deferral evidence chain — SQL probe + JavaScript predicate + canonical-zero unit-test anchor + capture-time soft-warn — is structurally identical to cycle 3; cycle 4 collapsed the predicate's two-file existence into one. Single-source-of-truth strengthens auditability without altering the trigger.2097dddf— ADR-026 cross-references the predicate. ✅ This exceeds what I'd left implicit in cycle 2. The ADR now contains:findAc3Candidates()infindAc3Candidates.mjs)validateFixture.test.ts)capture-network-fixture.mjs)A future architect, requirements engineer, or auditor reading ADR-026 now sees the full trail without grepping. This is the documentation-to-code traceability I'd ask for if it weren't already there.
23d93d49—TestNodetest-file cleanup. Cosmetic. No requirements impact.Traceability matrix — re-verified from cycle 3
buildLayout.test.ts6 synthetic + canonical fixturebuildLayout.test.tssynthetic two-family + fractional-offset adjacency tighteningfindAc3Candidates.mjs(cycle-4)6970cc95)Audit-clean. Every accepted AC has tested evidence; every deferred AC has a committed deferral artefact + a committed revisit trigger + a named landing-pad + a named owner + a hard date.
Loop-termination judgement
I have no new requirements concerns. Inventing one would be perfectionism, not requirements engineering. The deferral artefact set was already complete in cycle 3; cycle 4 strengthened it without changing the shape of the contract.
Net
Approve. Every cycle-1, cycle-2, cycle-3 concern in my lane is closed in code or in a committed document. Cycle 4 does not introduce new requirements drift. Merge.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.