feat(search): auto-select a single direct person match in smart search (#763) #769
Reference in New Issue
Block a user
Delete Branch "feat/issue-763-nl-search-direct-match"
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 #763.
What & why
In NL ("smart") search,
NlQueryParserService.resolveNames()resolved person names by substring match and treated any result count > 1 as ambiguous — so "Briefe von Clara Cram" forced the disambiguation picker whenever a Clara Cramer/Crammond coexisted, even though exactly one person is literally Clara Cram. For the 60+ audience that's a needless, confusing click.This introduces a token / word-boundary, alias-aware notion of a direct match that lives in the Person domain. One direct match auto-selects; multiple direct or partial-only matches show the picker (with case-appropriate copy); no candidates fold into full-text search.
How
PersonService.resolveByName(String) → NameMatches(direct, partial)— orchestrates tokenize → cap(8) → fetch pool → classify → cap(10) after classification. Empty-token guard first; PII-freelog.debugof the outcome bucket. Read-only transaction keeps lazynameAliasesreachable during classification (ADR-022).firstName,lastName,alias, eachPersonNameAliasfirst+last,title).titleis classification-only.PersonRepository.searchByNameextended witha.firstNameso a candidate matchable only via an alias first name is fetched (reuses the bound:query; no migration).NlQueryParserService.resolveNamesmapsNameMatches: 1 direct → resolved (auto-select), ≥2 direct → ambiguous, partial-only → ambiguous suggestions, 0 → full-text.MAX_CANDIDATESmoved into PersonService (after classification);MAX_NAME_LENGTHguard, resolved-cap overflow, sender/receiver mapping preserved.DisambiguationPickertakesheading+showCueprops; the page derives both. Newsearch_disambiguation_did_you_meankey in de/en/es.NameMatchesentry; person/README public surface. No ADR, no DB-diagram change.Tests (TDD, red/green per criterion)
PersonServiceTest— tokenizer pins +resolveByNameclassification (direct/partial, maiden alias, alias first name, middle name, reordered, cram-vs-cramer, empty-after-tokenizing, direct-sorts-beyond-cap, ≤8 fetches).PersonRepositoryTest(real Postgres slice) —searchByNameby alias first name + ordering.NlQueryParserServiceTest— 23 person mocks re-pointed toresolveByName/NameMatches; new partial-only (1 and 2 candidates) and one-direct cases.DisambiguationPicker.svelte.spec.ts— visible heading + cue suppression.Backend: 179 affected tests green;
mvnw packagesucceeds. Frontend: picker spec 8/8 green;npm run checkadds no new errors over baseline.🤖 Generated with Claude Code
When tagsApplied is true, each resolvedTag renders as a 'Thema: Name' chip with optional inline color style from the tag's resolved color. Clicking × calls onRemoveChip('theme', tag.name). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
I reviewed only the #763 work (the person-domain matching rule, the
searchByNameextension, theNlQueryParserService.resolveNamesmapping, the picker change, and the docs). The #743 tag code is out of scope here.What I checked
PersonService.resolveByName→NameMatches), exactly where the resolved design decision #2 placed it. The search layer mapsNameMatchesinto its own resolved/ambiguous/noMatch vocabulary and never reaches intoPersonRepository.NlQueryParserServicetalks toPersonServiceonly. No boundary leak.NameMatches(direct, partial)uses name-match-strength terms, not the search layer's resolved/ambiguous. The Javadoc onNameMatches.javadocuments this deliberately. This is the right call: it keeps the two layers' concepts from bleeding.MAX_TOKENS/MAX_CANDIDATESare co-located in the layer that owns the fetch loop (PersonService),MAX_NAME_LENGTHstays at the search boundary. No duplicated constants. Good.resolveByNameshape — a short orchestrator (tokenize → cap → fetchPool → classify → cap-after-classify) with each step a private helper. Reads top-to-bottom. This is the structure I'd have asked for.Documentation currency (my hard gate)
docs/GLOSSARY.md—NameMatchesentry added. ✅ New domain concept documented.backend/.../person/README.md—resolveByNameadded to the public-surface table. ✅person_name_aliases.first_namecolumn already exists; thesearchByNamechange is a query-onlyOR a.firstName LIKE. No DB-diagram update needed.resolveByNameis a new method on an existing service andNameMatchesa new value record in an existing package, so per my doc table neither triggers a c4l3-backend-*.pumlchange. (The search-domain diagram was already touched by #743's commits.)nameAliasescorrectly references the existing ADR-022 rather than minting a new one.Nothing for me to block. This is a well-bounded change that respects the layering rule.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Reviewing only the #763 work. This is clean, TDD-evident code — exactly the kind of change I like to read.
TDD evidence
Every behavior has a test that clearly precedes it: tokenizer pins (
Anna-Maria,D'Angelo,Müller, double-space, all-whitespace, null), the direct/partial classification cases, maiden-alias-as-direct, alias-first-name fetch, middle name, reordered tokens, cram-vs-cramer, empty-after-tokenizing (withverify(..., never()).searchByName), direct-sorts-beyond-cap, and theatMost(8)fetch invariant. The frontend picker spec gained heading-visible + cue-suppressed + cue-shown cases. Coverage maps 1:1 to the acceptance criteria.Clean code
resolveByNameorchestrator —tokenize → cap → fetchPool → classify, each helper under 20 lines, guard clause (empty tokens) first. Textbook.Set.containsAll(emptySet)returnstrue, so without it every candidate would be marked direct. Good that there's a test pinning it.tokenizeisstaticand package-private — testable in isolation without a Person, which is why the tokenizer pins are one-liners. Right visibility choice.$derivednot$effectin+page.svelte—disambiguationHeadingandshowDisambiguationCueare both$derived. The comment explaining "a 1-item picker is always a did-you-mean" earns its place (it's a why, not a what).DisambiguationPickerstays dumb —heading+showCueare props; the page decides. The{#each persons as person (person.id)}is keyed. The<ul>now hasaria-labelledby={headingId}pointing at a real visible<p>— better than the oldaria-label.Minor observations (non-blocking, no change requested)
cap()usessubList(0, MAX_CANDIDATES)which returns a view backed by the originalArrayList. It's never mutated afterward and the source list is local, so this is safe — just noting I checked it.personTokens()rebuilds aLinkedHashSetper candidate inside the classify loop. For a pool capped at the fetch level this is fine; no premature optimization warranted.No blockers. Ship it.
🔒 Nora Steiner ("NullX") — Application Security Engineer
Verdict: ✅ Approved
Reviewing only the #763 surface. No vulnerabilities, no security smells.
What I checked
PersonRepository.searchByNameaddsOR LOWER(a.firstName) LIKE LOWER(CONCAT('%', :query, '%'))reusing the same bound:querynamed parameter. No string concatenation, no new parameter, injection-proof by construction. ✅ (CWE-89 N/A.)MAX_TOKENS = 8caps the number of unindexed leading-wildcardLIKEscans per name before the fetch loop, and there's a dedicated test (over8Tokens_issuesAtMost8Fetches) assertingatMost(8)actual invocations — the guarantee is about fetches issued, not list size, which is the correct thing to pin.MAX_NAME_LENGTH = 200remains the first-line guard at the search boundary. Defense in depth, and tested.log.debugrecords only the outcome bucket (direct=1/direct>=2/partial-only/no-match) and the token count, never the raw name or any name component. This is exactly right for a private family archive — no name leaks into Loki. SLF4J parameterized form ("... outcome={} tokens={}") used throughout, so no log-injection risk either.resolveByNameis a read path reached through the already-authenticated/api/search/nlendpoint; no new endpoint, no@RequirePermissiongap introduced.heading/displayNameas text interpolation ({heading},{person.displayName}), neverinnerHTML. No XSS vector. Names originate server-side from the DB, not from raw user input.No failing-test-first security finding to raise — the DoS invariant the issue called out is already codified as a permanent regression test. Clean.
🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
Reviewing only the #763 test work. The coverage is strong and lands at the right pyramid layers — I have one gap and one suggestion, neither a merge blocker.
What's done well
@Mock PersonRepository(no Spring context, milliseconds). The fetch-query change (a.firstName, ordering) is tested inPersonRepositoryTestagainst real Postgres via the existing Testcontainers slice — correct, because H2 differs onLOWER/CONCAT-over-nulls. No new container spun up; you reused the existing slice as the issue asked. The search-layer mapping is unit-tested inNlQueryParserServiceTest. Frontend picker behavior is in thevitest-browser-sveltespec. Textbook pyramid discipline.resolveByName_maidenAliasToken_classifiesAsDirect,searchByName_ordersByLastNameThenFirstName). Easy to read a CI failure.verify(..., atMost(8)).searchByName), not a list-size proxy. That's the meaningful assertion.makeNameMatchesfactory with overloads (empty / direct-only / direct+partial) keeps the re-pointed 23 mocks one-liners. Good factory hygiene.elevenCandidates_capsAtTentotenDirectMatches_allShownAsAmbiguous— it now verifies the cap lives inresolveByName(after classification) and the search layer adds no second cap. The behavior moved, and the test moved with it.Concern (non-blocking)
resolveByName_maidenAliasToken_classifiesAsDirectandaliasFirstNameToken_isFetchedAndClassifiedboth stubsearchByNameto return the candidate, so they verify the classifier, not thatsearchByNameactually finds a person by maidena.lastName+a.firstNameand feeds it into classification.PersonRepositoryTest.searchByName_findsByAliasFirstNamecovers the fetch in isolation, but nothing exercises fetch→classify together for the alias case. The acceptance criterion "a person matchable only via an alias first name is fetched and classifiable" is split across two tests that never meet. A single@DataJpaTest-or-service-level test (real Postgres, realresolveByName) saving a person with a maiden alias and assertingresult.direct()would close the seam. Low effort, high confidence — worth a follow-up if not this PR.Suggestion
resolveByNametest where the same person appears in the fetch pool via two different tokens (dedup by id infetchPool) to pin theputIfAbsentbehavior — currently implied by the single-direct tests but not asserted directly.Quality gate (88% branch) — the empty-token-guard, direct-sorts-beyond-cap, and direct/partial branches are all hit, so I don't expect a JaCoCo regression. Approving with the fetch→classify integration gap noted as a follow-up.
🎨 Leonie Voss — Senior UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
Reviewing only the #763 frontend changes. The core UX win is real and well-executed — but there's one accessibility seam in the trigger that I want on record for our 60+ audience.
What's done well
max-w-[8rem] truncate sm:max-w-[12rem], which would clip "Meintest du Clara Cra…" at 320px and hide the suggestion from a senior on a small phone. Putting the full suggested name in the<p>panel heading at body size guarantees it renders in full when the picker opens. Exactly the constraint-first call I'd make.aria-labelledbyupgrade — the<ul>now references a visible<p id="disambiguation-heading">instead of a hiddenaria-label. The heading is announced AND seen. Better than before.min-h-[44px]. i18n added to all three locales (de/en/es) with sensible copy.$derivedheading on the page; component stays dumb.Concern (High — non-blocking, but please track)
The trigger button's accessible name is wrong for the 1-item case.
DisambiguationPicker.sveltehardcodesaria-label={m.search_disambiguation_trigger_label()}= "Mehrere Personen gefunden — zum Auswählen klicken" / "Several people found — click to choose" on the trigger regardless of person count. So for a single "did you mean Clara Cramer" suggestion, a screen-reader user hears "Several people found" while sighted users see one name and a "Meintest du" heading inside. That's a system-status mismatch (Nielsen #1/#2) specifically for the assistive-tech path — and the picker spec even locates the 1-item trigger viagetByRole('button', { name: /Mehrere Personen gefunden/ }), which confirms the stale label is what's exposed.The issue spec said "trigger stays as-is" to avoid the 320px truncation, and that's right for the visible label — but the accessible label should still switch with count. Concrete fix: pass a
triggerLabelprop (or derive frompersons.length) so the 1-item case announces something like "Meintest du {name}? Zum Auswählen klicken" / "Did you mean {name}? Click to choose", while the visible truncated span is unchanged. This keeps the visual fix intact and gives SR users the correct status.Note
aria-haspopup/expanded/controls, focus-visible, Esc-to-close + focus return) is intact, but the trigger-label mismatch above is exactly the kind of thing a per-state axe assertion or an aria-name assertion would have caught. Worth a small follow-up.Approving — the visible UX is a genuine improvement for the senior audience. Please file the trigger aria-label fix; it's the one thing standing between this and a clean ✅ from me.
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Reviewing only the #763 work. Nothing in my domain to flag — and I checked specifically because the new code path can issue multiple DB queries.
What I checked
MAX_TOKENS = 8caps it at 8searchByNameinvocations per name, each a single parameterized query, and there's a test asserting the cap. TheLIKE '%...%'leading-wildcard scans are unindexed, but at ≤8 per NL search on a family-sizedpersonstable this is a non-issue — and the NL search endpoint is already rate-limited (NlSearchRateLimiter, 5/min/user) upstream, so there's no amplification vector. No need to profile or add an index.log.debugis at DEBUG and structured (outcome bucket + token count), so it's queryable in Loki without flooding INFO and without redeploying to answer "is auto-select firing?". This is the kind of operability hook I like to see added proactively. PII-free, so it's safe to leave on in a troubleshooting session.No changes requested.
📋 "Elicit" — Requirements Engineer & Business Analyst
Verdict: ⚠️ Approved with concerns
I traced the PR against the acceptance criteria of #763 (the source of truth). My job here is requirements coverage, not code — does the implementation satisfy every confirmed criterion, and is anything ambiguous or untraced?
Acceptance-criteria traceability
resolveByName_singleDirectMatch+cramVsCramer_classifiesAsPartial+oneDirect_executesSearchdirect.size() >= 2branch inresolveNames; partial-twoCandidates analog testedresolveByName_middleName_stillDirectresolveByName_maidenAliasToken_classifiesAsDirectsearchByName_findsByAliasFirstName+aliasFirstNameToken_isFetchedAndClassifiedAnna-Maria,D'Angelo,Müller, lowercase, drop empties)tokenize_*testspartialOnly_oneCandidate+ picker spec (heading visible, cue absent)partialOnly_twoCandidates+disambiguationHeadingderivation (≥2 → heading)noMatchFragmentsbranch;search_noMatchName_isFoldedIntoTextemptyAfterTokenizing_returnsNoCandidates(guard clause)MAX_CANDIDATESstill auto-selecteddirectSortsBeyondCap_stillReturnedAsDirectover8Tokens_issuesAtMost8FetchesresolveByNamereorderedTokens_stillDirect+ lowercase tokenizerEvery one of the 14 acceptance criteria is traceable to code + a test. That is excellent requirements discipline — this is the spec-dense, testable style the issue itself modeled.
Flags (non-blocking)
Scope check
No scope creep within #763. (The PR also carries #743's tag work, but that's the unmerged base branch and out of scope for this review per the reviewer's instruction — not a #763 concern.)
Approving on coverage. The two flags are best handled as explicit follow-up acceptance criteria so the assistive-tech and fetch→classify gaps are tracked rather than assumed.
👨💻 Felix Brandt — review concerns addressed
All three review concerns are resolved via red/green TDD, one atomic commit each. Zero blockers were raised; these were the three non-blocking concerns from Leonie and Sara.
aria-labelwrong for the 1-item case (Leonie, High) — a single did-you-mean picker announced "Mehrere Personen gefunden" to screen readers while sighted users saw one name + a "Meintest du …?" heading.DisambiguationPickernow derives the trigger's accessible name frompersons.length: a single suggestion reuses theheadingprop ("Meintest du {name}?"), ≥2 keepssearch_disambiguation_trigger_label. Visible truncated name span unchanged. Spec re-pointed: the two 1-item tests now locate the trigger by the did-you-mean name, plus explicit accessible-name assertions for both the 1-item and ≥2 cases. Validated with the Svelte autofixer (no issues).3f0d37bfPersonRepositoryTest(the fetch) andPersonServiceTest(the classifier with stubs); nothing walkedsearchByName → resolveByNameend-to-end on real Postgres.@DataJpaTest+ Testcontainers slice (no new slice/container) that build a realPersonServiceover the autowired repositories, persist a person with aMAIDEN_NAMEalias "Cram" (displayName "Clara Müller") and one with an aliasfirstName"Wilhelmina", then assertresolveByName(...)classifies each as direct.751ac1c4fetchPooldedup (Sara, suggestion)PersonServiceTestwhere the same person id is returned by two different token fetches and asserts the person appears exactly once (directsize 1,partialempty) — pins theputIfAbsentdedup against a future double-classify regression.79bcd1b3Verification (touched classes only — full suite left to CI per repo convention)
DisambiguationPicker.svelte.spec.ts— 10 passed (10) (was 8; +2 accessible-name assertions)PersonRepositoryTest— Tests run: 56, Failures: 0, Errors: 0 (real Postgres slice, +2 end-to-end)PersonServiceTest— Tests run: 81, Failures: 0, Errors: 0 (+1 dedup)Pre-commit frontend lint (prettier + eslint) passes. Branch pushed to
feat/issue-763-nl-search-direct-match(c0edbd43..79bcd1b3).🤖 Generated with Claude Code
🏛️ Markus Keller — Senior Application Architect · re-review (cycle 2)
Verdict: ✅ Approved
I re-checked the three follow-up commits (
3f0d37bf,751ac1c4,79bcd1b3) against my cycle-1 position. I raised no concerns in cycle 1, and nothing in the new work touches my domain in a way that would change that.What changed since cycle 1 — architectural impact
3f0d37bf(trigger aria-label) — frontend-only.DisambiguationPickergainsheading/showCueprops and derivestriggerLabelfrompersons.length; the page already owned the heading derivation. The component stays presentation-dumb and the page keeps the policy. No new boundary, no coupling shift.751ac1c4(real-Postgres fetch→classify tests) — test-only. Notably it constructsnew PersonService(personRepository, aliasRepository)over the autowired repositories inside the existing@DataJpaTestslice. That respects the layering rule (the test wires the service over its own domain's repositories; it does not reach across domains). No production code touched.79bcd1b3(fetchPool dedup test) — test-only, pinsputIfAbsentbehaviour already present infetchPool.Confirmation
l3-backend-*.pumlor DB-diagram trigger. Docs currency from cycle 1 (GLOSSARYNameMatches, person/READMEresolveByName) remains accurate; nothing in the new commits invalidates it.NameMatches↔ search-layer vocabulary boundary is unchanged.No regressions in my lens. Still a cleanly bounded change. Approved.
👨💻 Felix Brandt — Senior Fullstack Developer · re-review (cycle 2)
Verdict: ✅ Approved
I approved in cycle 1; the three follow-up commits are exactly the disciplined red/green increments I'd expect, one atomic concern each. Re-checked all three.
3f0d37bf— trigger aria-label (Leonie's concern)triggerLabel = $derived(persons.length === 1 ? heading : m.search_disambiguation_trigger_label())— the right place for the branch. The visible truncated<span>{names}</span>is untouched, so the 320px clipping fix is preserved while the accessible name now matches the shown status. This is the surgical fix, not a rewrite.<div>wrapping a visible<p id="disambiguation-heading">+<ul aria-labelledby={headingId}>. The{#each persons as person (person.id)}stays keyed. Clean.Mehrere Personen gefunden). The pre-existing tests were re-pointed via amultiPropsspread — minimal churn, no behavioural drift.751ac1c4— fetch→classify e2e (Sara/Elicit's seam)…_endToEndtests do the right thing: persist a real person + alias, thenentityManager.flush(); entityManager.clear();before callingresolveByName, so the lazynameAliasesare genuinely re-fetched from Postgres rather than served from the persistence context. That's what makes this a real fetch→classify proof and not just a dressed-up unit test. The detach comment earns its place (a why).79bcd1b3— fetchPool dedupsamePersonFromTwoTokens_appearsOnceassertsdirect().hasSize(1)+partial().isEmpty()— pinsputIfAbsentagainst a future double-classify regression. One logical assertion, good name.No new production logic, no regressions. Ship it.
🔒 Nora Steiner ("NullX") — Application Security Engineer · re-review (cycle 2)
Verdict: ✅ Approved
I approved in cycle 1. The three follow-up commits introduce no new attack surface — re-checked specifically for that.
Security review of the new commits
3f0d37bf— the newtriggerLabeland the panel heading both render via Svelte text interpolation (aria-label={triggerLabel},{heading},{person.displayName}). NoinnerHTML, no{@html}. The heading text originates from server-side persondisplayNamevalues, not from raw query input. No XSS / DOM-injection vector. Thearia-labelledby={headingId}references a static id, not user data.751ac1c4/79bcd1b3— test-only, no production code path. The e2e tests exercise the existing parameterizedsearchByNamequery; no new query, no string concatenation.MAX_TOKENS = 8is unchanged, and theover8Tokens_issuesAtMost8FetchesatMost(8)regression test is still present. The new dedup test actually strengthens the bound posture (confirms a flooded pool collapses to one classification). NL search remains rate-limited upstream (5/min/user).Nothing to raise. Clean.
🧪 Sara Holt — Senior QA Engineer · re-review (cycle 2)
Verdict: ✅ Approved (upgraded from ⚠️ Approved with concerns)
Both items I raised in cycle 1 are now closed. Re-verified the new tests.
Cycle-1 concern (fetch→classify seam) — RESOLVED ✅
751ac1c4addsresolveByName_maidenAlias_classifiesAsDirect_endToEndandresolveByName_aliasFirstName_classifiesAsDirect_endToEndto the existing@DataJpaTest+ Testcontainers slice (no new container, as I asked). These build a realPersonServiceover the autowired repositories, persist a person with aMAIDEN_NAME/BIRTHalias, and assertresolveByName(...).direct()contains the saved id. Crucially they callentityManager.flush(); entityManager.clear();before resolving, so the lazynameAliasesare re-read from Postgres — the fetch and the classify steps now genuinely meet in one test, which is exactly the seam AC#4/#5 implied. The supportingsearchByName_findsByAliasFirstNameandsearchByName_ordersByLastNameThenFirstNamerepository tests are also present and correct.Cycle-1 suggestion (fetchPool dedup) — RESOLVED ✅
79bcd1b3addsresolveByName_samePersonFromTwoTokens_appearsOnce: both token fetches return the same id, assertingdirectsize 1 andpartialempty. That pinsputIfAbsentdirectly rather than leaving it implied.Regression check on the new tests
getId()after a real save — deterministic and not dependent on insertion order. Good.searchByName_ordersByLastNameThenFirstNamepins the ORDER BY (Anna, Bernd, Clara) on real Postgres collation — the right place for an ordering assertion (not H2).clear()the test would pass off the persistence context and prove nothing about the fetch. They got it right.Pyramid discipline intact, no JaCoCo regression expected (these are net-new covered branches). Clean approve.
🎨 Leonie Voss — Senior UX Designer & Accessibility Strategist · re-review (cycle 2)
Verdict: ✅ Approved (upgraded from ⚠️ Approved with concerns)
My one High concern from cycle 1 — the trigger's accessible name mismatching the visible status for a single suggestion — is fully resolved in
3f0d37bf.Cycle-1 concern (trigger aria-label) — RESOLVED ✅
DisambiguationPicker.sveltenow derives the trigger's accessible name from count:So a single did-you-mean suggestion now announces "Meintest du {name}?" to a screen-reader user — matching what sighted users see in the panel heading — while ≥2 keeps "Mehrere Personen gefunden". The status the assistive-tech path hears now equals the status shown (Nielsen #1/#2 satisfied for both paths). The visible truncated name span is untouched, so the 320px clipping fix I cared about is preserved. This is exactly the fix I proposed.
Regression check
<div>→ visible<p id="disambiguation-heading">+<ul aria-labelledby={headingId}>) keeps the heading both announced and seen. Per-optionaria-label,min-h-[44px]touch targets,aria-haspopup/expanded/controls, focus-visible rings, and Esc-to-close-with-focus-return are all intact in the diff.getByRole('button', { name: 'Meintest du Clara Cramer?' })and the ≥2 case by/Mehrere Personen gefunden/. That's the per-state aria-name assertion I wanted.(auswählen…), ≥2 keeps it) is verified byshowCuetrue/false specs.The senior-audience UX win is intact and the assistive-tech seam is closed. Clean ✅.
🔧 Tobias Wendt — DevOps & Platform Engineer · re-review (cycle 2)
Verdict: ✅ Approved
I approved in cycle 1. Re-checked the three follow-up commits for any operational/CI impact — there is none.
Operational impact of the new commits
3f0d37bfis frontend code+spec;751ac1c4and79bcd1b3are backend tests. No Compose, env var, CI workflow, image, or port change.new PersonService(...)over the autowired repos rather than spinning anything up). The new service test and the frontend spec are mock/component-level. Marginal runtime delta.MAX_TOKENS = 8cap and itsatMost(8)regression test are intact; the new dedup test confirms a duplicate-heavy pool doesn't amplify classification work. The DEBUG outcome log is unchanged (PII-free, Loki-queryable). No new amplification vector; NL search still rate-limited upstream.No changes requested.
📋 "Elicit" — Requirements Engineer & Business Analyst · re-review (cycle 2)
Verdict: ✅ Approved (upgraded from ⚠️ Approved with concerns)
Both flags I raised in cycle 1 are now closed with traceable coverage. All 14 acceptance criteria of #763 remain satisfied; the two seams are now backed by explicit tests.
Cycle-1 flag 1 — AC#4 / AC#5 fetch→classify seam — RESOLVED ✅
The criteria "matchable only via an alias first name is fetched and classifiable" (AC#5) and the maiden-alias direct case (AC#4) were previously split across two tests that never met.
751ac1c4adds two@DataJpaTestreal-Postgres tests (resolveByName_maidenAlias_classifiesAsDirect_endToEnd,resolveByName_aliasFirstName_classifiesAsDirect_endToEnd) that walksearchByName → resolveByNamecontinuously, with aflush()/clear()detach so the alias is genuinely re-fetched. The criterion as written ("fetched and classifiable" = one continuous behaviour) is now proven as one continuous behaviour, not two halves. Traceability for AC#4/#5 upgraded from split to end-to-end.Cycle-1 flag 2 — assistive-tech requirement gap — RESOLVED ✅
I flagged that the visible-heading criteria (AC#7) were silent on the trigger's accessible name, leaving SR users hearing "Several people found" for a 1-item suggestion.
3f0d37bfmakes the trigger'saria-labelderive from count (1 → did-you-mean heading, ≥2 → multi-people label), and adds explicit accessible-name assertions for both states. The unstated requirement — "announced status must match shown status for a single suggestion" — is now implemented and pinned by test. The implicit requirement is satisfied and verified.Updated traceability
samePersonFromTwoTokens_appearsOnce).Scope check
Still no scope creep within #763. The #743 tag work in the diff remains out of scope per the review instruction.
Every confirmed and implicit requirement is now traceable to code + test. Approving on coverage — no remaining follow-up needed.