feat: manual face review mode — audit any person's faces without a scan - #838
Merged
Conversation
Adds a second entry path to the admin face cleanup console: pick any person and audit all of their faces, with no scan required. Reuses the existing tile grid, bulk-select-then-apply interaction, and verdict writes, so a manual decision is indistinguishable from a guided one. Server work is narrow: relax guard E15 for lock/unknown/detach (keeping stay scan-only, since it needs a suspected owner), add the eligibility check that snapshot membership was implicitly providing, and add an admin person-metadata endpoint for the name/ownerId the review page currently reads off the scan.
Three independent verification passes against the code disproved the first draft's central premise and five other claims. The big one: manual mode cannot reuse the guided review page behind a mode flag. FlaggedFace.suspectedOwnerId is required, every face defaults to the 'owner' state, and buildResolveRequest dereferences it - so an untouched manual review would POST moveToPerson with an undefined destination for every face. There is also no neutral state (six terminal states with a tested tally invariant), and the model is $derived over the face array, so paging would wipe staged decisions. The design now forks the review UI: a separate manual page under /people/[personId] with its own view-model (keep default, stable across appends, never emits stay), leaving all nine guided web specs untouched. Also corrected: lock is a PRIMARY KEY upsert with DO UPDATE that can steal a face from another identity (not an idempotent no-op via a plain unique index - that claim came from a stale comment referencing the long-gone insertLocks); FaceRepairRepository has no @GenerateSql so no mise sql regen; the navbar needs no repointing; resolveFaces has no unit coverage, only medium; and relaxing unknown converts one 400 into a success with unknown:0.
…ning states Pierre's review of the chooser design: a first-time admin has never scanned, so the page needs a distinct first-visit presentation, and the two modes should carry equal weight since some admins will live in manual review rather than triaging scans. Equal-weight cards with no recommended badge. First visit gets an explanatory header and advertises that manual review needs no scan — it is fully usable on a brand-new instance, which the returning status-board layout would otherwise obscure. Adds the full card state matrix (never-scanned / running / flagged / zero-flagged / failed), including the manual card being disabled during a scan, which is the UI half of the resolveFaces 409 guard and prevents staging dozens of decisions only to lose them to a conflict. Slices renumbered 1-26 to cover the two chooser presentations and the remaining card states separately.
Folds the manual review page design into 6.4. The key point is a visual inversion that falls out of the data model: guided stamps every tile because every face holds one of six terminal states, while manual defaults to keep, so tiles start clean and colour appears only where the admin acted. keep needs no colour token - absence signals it, which satisfies the existing rule that state is never encoded in colour alone. Two interaction problems get resolved. Selection cannot claim the whole cluster over server paging, so it always means loaded faces (Select all loaded (N), showing N of M) and whole-cluster work goes through the server-enumerated entireCluster with its own destination picker. And a keep default needs an undo, so Unmark returns a selection to keep - guided never needed one because every face is already stamped. Slices consolidated 26 -> 12 on one test: can it ship alone without leaving the tree broken or dead? The route move now carries its own e2e repair (the move alone leaves CI red), the endpoint carries its SDK regen (no caller otherwise), the eligibility read joins the lock slice it exists for, and the chooser's three slices collapse into one component with one state machine. The pure view-model stays separate despite being small - highest logic risk on the branch.
…lagged ones Manual review needs to lock a face that no scan flagged, so lock is dropped from the E15 flagged-snapshot gate. stay keeps that gate: it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion, so without a snapshot row there is no owner to record against. Lifting the gate for lock is not merely permissive - it removes a live guard. getScanFlaggedFacesForPersons INNER JOINs asset_face and re-validates personId/deletedAt/isVisible at snapshot-read time, so today a foreign or deleted id can never reach the write path. Dropping the gate without a replacement was verified to be exploitable: with lock removed and no eligibility check, resolveFaces returned locked:1 for a face on another person, for another USER's person, and for a soft-deleted face - silently re-pointing them onto this person's identity through replaceFaceIdentities' unscoped ON CONFLICT DO UPDATE. A nonexistent id crashed with an unhandled FK violation rather than a 400. getEligibleFaceIdsForPerson replaces that guard explicitly, mirroring getClusterFacePage's predicate so lockable == listed on the manual review page. It is advisory - the write-time guards remain authoritative - and exists so an inapplicable lock is a 400 rather than a silent no-op or a crash. Tests: 66/66 in the resolve medium spec, incl. the cross-tenant arm.
…nly flagged ones Completes the E15 relaxation: stay is now the ONLY snapshot-gated bucket. It keeps that gate because it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion - with no snapshot row that yields undefined and a 500/FK violation. Unlike lock, detach and unknown need no eligibility read: both are already person-scoped at the write layer (detachFaces filters personId and keys its identity-strip on the RETURNING output; reattributeFaces filters still-on-source). That claim is now pinned by test rather than trusted - a foreign face passed to detach is an inert no-op, left completely untouched. Behaviour change (spec 5.4): a stale unknown id - a face that left this person since the scan - previously 400'd via the gate. It now returns success with unknown: 0, truthfully reporting zero parks, and the freshly created cluster is cleaned up rather than orphaned. Tests: 72/72 in the resolve medium spec, 6/6 cross-flow.
GET admin/face-repair/person/:personId returns
{id,name,ownerId,faceCount,thumbnailFaceId}, admin-gated, 404 on
unknown.
The manual review page cannot get these from anywhere else. The guided
page reads personName and ownerId off getLatestScan(), and ownerId is
what scopes the move-picker - so with no scan the picker cannot open at
all. The user-scoped GET /people/:id does not admin-bypass for a person
the admin does not own, the same gap already fixed once for face
thumbnails.
faceCount reuses searchOwnerPeople's exact join conditions so the count
agrees between the browser grid and the review page header; a mismatch
there would read as a bug. The name is returned raw - an unnamed person
yields an empty name and the client applies its own fallback rather than
the server inventing a display string.
Ships with the client regen because the endpoint has no caller until
then. No @GenerateSql was added (this repository carries none) so no
mise sql regeneration; mise.lock verified unchanged.
Tests: controller 76/76, metadata medium 6/6, tsc clean.
…cleanup/scan Frees /admin/face-cleanup for the mode chooser. Pure relocation of the ten-file dashboard cluster via git mv - no behaviour change - plus a temporary 307 redirect at the old path so this lands without a dead entry point. The chooser replaces that redirect next. The navbar is deliberately NOT repointed: NavbarItem highlights via pathname.startsWith(href), so pointing at /admin/face-cleanup keeps it active across /scan, /people and the chooser. Repointing it would be the regression, not the fix. Four e2e assertions broke on the move. Three are repoints; Playwright globs match the whole URL, so '**/admin/face-cleanup' does not match /scan. The fourth needed a real fix rather than a repoint: the post-apply drain check asserts a person's name is ABSENT, which passes vacuously on any page that never lists people - or on one that simply has not finished loading. It now waits for the Eligible faces stat, which renders unconditionally once a scan is completed and is therefore independent of the drained-vs-not condition under test. Tests: 117/117 across the nine guided specs from their new locations, tsc clean.
Replaces slice 4's temporary redirect. Two EQUAL-WEIGHT cards with no recommended badge on either: we do not know which mode a given admin lives in, and privileging guided would penalise anyone who spends their time in manual review. Two distinct presentations rather than one degraded into the other. First visit reframes the difference as readiness, not importance - guided says it needs a scan first, manual advertises that it needs no scan and can start right away. That line matters: manual review is fully functional on a brand-new instance, and the returning status-board layout would otherwise hide that behind an empty guided card, leaving a first-time admin thinking the feature does nothing until a scan finishes. The manual card is disabled while a scan runs. This is the UI half of resolveFaces' 409 guard: without it an admin can stage dozens of decisions across a server-paged cluster and lose them all on Apply. Disabled means genuinely unactivatable - no href and a real <button disabled>, so keyboard cannot reach it either - not just opacity. Only pending/running block it; failed and zero-flagged leave manual reachable. Visual vocabulary is reused verbatim from the dashboard - card shell, dot+label+tabular-nums stat rhythm, semantic colours - so this reads as the same product rather than a bolted-on page. Tests: 13 new chooser cases, 130/130 across the face-cleanup suite, tsc clean.
Builds the destination the chooser's manual card links to. Zero new server endpoints: the owner list reuses searchUsersAdmin (the same call the guided dashboard already makes) and the people grid reuses getFaceRepairOwnerPeople, whose query parameter is optional - so omitting it lists all of an owner's people, paginated. Face crops go through the admin-gated, join-free face-thumbnail route, NOT the user-scoped /people/:id/thumbnail, which 404s for people the admin does not own. A null thumbnailFaceId renders a real placeholder rather than a broken image. Switching owner resets the page and clears the list, with a request token guarding against a stale response clobbering a newer selection - otherwise rows from two owners interleave. Pagination appends rather than replaces. Four states are kept distinct: this owner has no people, no results for this query, load failed, and no users at all. Conflating empty with error was defect D17 on the guided page. No client-side filtering is applied to what the endpoint returns; that passthrough is pinned by test so a later server-side change surfaces here rather than being silently masked. Tests: 14 new cases, 144/144 across the face-cleanup suite, tsc clean.
A separate pure model rather than a reuse of the guided one, because the guided model cannot express manual review's semantics and two of its properties are actively dangerous here. No neutral state: guided's FaceState is six TERMINAL states and every face initialises to 'owner', with buildResolveRequest dereferencing suspectedOwnerId for it. Pointed at a scan-free cluster an untouched review would POST a move for every face to an undefined destination. Manual defaults to keep, which produces no bucket entry at all, and an all-keep review builds to null rather than an empty request - so a disabled Apply cannot be bypassed and the server never sees the empty resolve it would 400. State survives paging: guided rebuilds its model from a $derived over the face array, so appending a page discards staged decisions. This model owns its list and exposes appendFaces, which preserves both states and selection and is idempotent on assetFaceId. Manual is server-paged, so that defect would have been hit on the first Load more. stay is hardcoded [] on every request - manual can never emit it, since it needs a suspected owner. Two @ts-expect-error guards make owner/stay a compile error rather than a convention; they are load-bearing, since tsc reports an unused directive if the type ever stops rejecting them. Colour/icon tokens are imported from the guided model so one glyph means one thing across both pages; keep deliberately has no token - it is signalled by absence. Tests: 26 cases, 170/170 across the face-cleanup suite, tsc clean.
…faces Opens any person with no scan in existence and lists ALL of their faces, via the scan-free cluster endpoint with an empty excludeFaceIds. Implements the visual inversion this page is built around: guided stamps every tile because every face holds one of six terminal states, so its grid is a wall of colour to audit. Manual defaults to keep, so a tile renders a clean crop with no badge and no ribbon, and colour appears only where the admin acted. keep needs no colour token - it is signalled by absence, which keeps the existing rule that state is never encoded in colour alone without inventing a seventh swatch. Selection is honest about server paging: it always means LOADED faces, labelled Select all loaded (N) with showing N of M in the header. An unqualified select-all would either lie about a cluster of thousands or force loading all of it; whole-cluster work goes through the server-enumerated entireCluster instead (slice 10). Load more appends through the model rather than re-creating it, so staged marks and selection survive - the defect that made reusing the guided model impossible, and the first thing a paged cluster would have hit. Person name and ownerId resolve from the URL param through the metadata endpoint rather than navigation state, so refresh and deep links work. Empty and load-error are kept distinct (conflating them was D17 on the guided page), and a person the scan flagged shows no flagged badging - manual ignores scan state entirely. Tests: 15 new cases, 185/185 across the face-cleanup suite, tsc clean.
The footer dock: Move to person (via the reused PersonPicker, scoped by the ownerId the metadata endpoint supplies), Lock, Unknown, Not a face, and Unmark - plus the staged-work tally and Apply. Unmark exists because manual defaults to keep. Guided never needed one: every face there is already stamped, so the admin only ever swaps one stamp for another. Here marking is a deliberate act, so mis-marking needs reversing without discarding every other staged decision via Reset. stay and owner are not offered and can never be emitted - both need a suspected owner. The payload assertion pins stay: [] explicitly across mixed buckets. Apply is disabled while everything is keep, because buildResolveRequest returns null and an all-keep POST would be the empty resolve the server 400s. Not a face keeps guided's destructive confirm, and declining it posts nothing. A 409 - a scan starting mid-review - surfaces without discarding staged work. That conflict is what the chooser's disabled-manual card exists to prevent; if it happens anyway the page must not compound it by throwing away decisions staged across a paged cluster. Tests drive the real UI rather than the model. 16 new cases, 201/201 across the face-cleanup suite, guided's 73 specs untouched, tsc clean.
Move entire cluster uses the server-enumerated entireCluster rather than selection, which is the whole reason it belongs here: on a server-paged page selection can only ever cover LOADED faces, so whole-cluster work must not route through it. Unlike guided - where entireCluster rides the scan's suspected owner - manual has none, so it requires an explicit destination via PersonPicker, and it posts with no per-face buckets since the server rejects combining them. It carries its own confirm because it moves faces the admin has never seen, which is both the point and the risk. Manual gets its own help modal rather than extending guided's, whose names all six actions test is load-bearing and stays untouched. The modal leads with the thing most likely to confuse someone arriving from guided: keep writes nothing. It also states that Not a face is the irreversible one and sits beside Unknown, which means the opposite - bin this crop versus this is a real person I cannot name. Swatches are asserted against the tile tokens so an explanation ties back to the button and the tile it describes, and keep renders no swatch at all - absence is how the default is signalled. Tests: 21 new cases, 222/222 across the face-cleanup suite, guided's specs untouched, tsc clean.
…ariant Two tests. The first drives the whole manual path against a person with NO scan in existence: chooser to people browser to review page, listing all faces rather than a flagged subset, then move, lock and not-a-face through the real UI, asserting durable DB rows rather than UI text. The second is the point of the feature: a later scan must honour a manual decision exactly as it honours a guided one - the locked face is not re-flagged, the detached face stays gone, the moved face is not re-proposed. Two seeding traps handled. utils.createFace links faces with source='manual', which the verdict layer excludes from flagging, so every face here is downgraded to 'ml' or it would not behave like a machine-clustered one; and face_search rows are required because both the cluster listing and lock eligibility inner-join it. The cross-engine test seeds ONE multi-person scan rather than two sequential ones: two scans stamped the same now() make getLatestScan nondeterministic, which would let the invariant assertions pass vacuously instead of for the right reason.
Adds a Two ways to clean up comparison and a Manual review section to the admin face cleanup page. The two points worth documenting explicitly are the ones an admin cannot infer: that faces you leave alone are deliberately NOT recorded (marking everything you glanced at as human-verified would stop future scans flagging it and hide real mistakes, so Lock is the opt-in), and that Select all covers the faces currently loaded rather than the whole cluster - with Move entire cluster as the server-resolved alternative. Also notes that manual review is unavailable during a scan, so the behaviour reads as deliberate rather than broken.
github-actions
Bot
added
documentation
A Record<..., string> annotation widened each value to string, and $t only accepts known translation keys, so the tally lookup stopped type-checking. satisfies keeps the exhaustiveness check on the keys while preserving the literal value types. Caught only by CI's check-svelte: that task reports 0 FILES locally in this worktree - even when invoked through mise exactly as CI does - so neither tsc nor the local svelte-check surfaced it.
satisfies was not enough: indexing an object of whole translation keys
still yields string at the call site, and $t accepts a template-literal
type but not string. Matches the guided dock's idiom exactly -
$t(`admin.face_cleanup_review_tally_${...}`) - by mapping only the key
SUFFIX, with move reusing guided's existing 'other' key rather than
adding a near-duplicate.
Only CI's check-svelte catches this; that task scans 0 files locally
even when invoked through mise as CI does.
Rework the two-door chooser at /admin/face-cleanup into a considered design: a 48px icon tile anchors each mode, guided keeps the primary accent while manual gets a distinct teal identity (neither marked recommended), and each card carries a tinted status well that recolours across all five scan states with the live count as a hero number. Actions become full-width and foot-aligned, with a soft corner glow, inset ring, and hover lift for depth. Logic, copy, i18n keys, data-testids and the genuinely-disabled manual guard (§7) are unchanged; all 13 chooser page-spec tests still pass, eslint/prettier/svelte-check clean.
Replace the "Load more" buttons on the manual people browser and the selected-person review grid with scroll-driven pagination, so both lists grow as the admin scrolls instead of dead-ending on a button. Adds a reusable InfiniteScrollSentinel primitive (a sentinel-only port of people-grid's IntersectionObserver + visibility-fallback logic; people-grid itself isn't a drop-in where items aren't keyed by `id`). The review grid keeps appending through vm.appendFaces, so staged marks and selection still survive each page load.
…ection UI Compose ConfidentLane (approve-all + spot-check exclude) and ReviewFirstLane (clickable rows) in the scan page, driven by the exclusion-set triage model. Removes the 3-step checklist, 5 stat cards, filter tabs, and the selection bar; the two non-actionable totals become quiet footnotes. Deletes ScanChecklist, FaceCleanupTable, and the old face-cleanup selection model, and prunes their now-dead i18n keys.
…moved stat card
The two-lane scan redesign removed the "Eligible faces" stat card the X1 test
used as its "completed snapshot loaded" signal; use the header summary line
("… flagged faces across … people"), which renders on any completed scan.
…rflow Collapse the wrapping multi-pill reason column to a single primary reason (bad-target wins, else the first) plus a "+N", narrow the flagged column, and share one absolute right-slot between the chevron (default) and the hover dismiss so the row can no longer widen past the container.
The routing buttons were text-xs with 13px icons on the dark dock and read as small/hidden. Enlarge them (text-sm, 16px icons, roomier padding, a defining inset ring, rounded-lg), tint the destructive "not a face" red to set it apart from the routine routes, and bump the selected-count and Clear.
…s, review-lane alignment)
Deeds67
added
the
rc
Deeds67
added a commit
that referenced
this pull request
Jul 26, 2026
…an (#838) * docs: design spec for manual face review mode Adds a second entry path to the admin face cleanup console: pick any person and audit all of their faces, with no scan required. Reuses the existing tile grid, bulk-select-then-apply interaction, and verdict writes, so a manual decision is indistinguishable from a guided one. Server work is narrow: relax guard E15 for lock/unknown/detach (keeping stay scan-only, since it needs a suspected owner), add the eligibility check that snapshot membership was implicitly providing, and add an admin person-metadata endpoint for the name/ownerId the review page currently reads off the scan. * docs: revise manual face review spec after verification pass Three independent verification passes against the code disproved the first draft's central premise and five other claims. The big one: manual mode cannot reuse the guided review page behind a mode flag. FlaggedFace.suspectedOwnerId is required, every face defaults to the 'owner' state, and buildResolveRequest dereferences it - so an untouched manual review would POST moveToPerson with an undefined destination for every face. There is also no neutral state (six terminal states with a tested tally invariant), and the model is $derived over the face array, so paging would wipe staged decisions. The design now forks the review UI: a separate manual page under /people/[personId] with its own view-model (keep default, stable across appends, never emits stay), leaving all nine guided web specs untouched. Also corrected: lock is a PRIMARY KEY upsert with DO UPDATE that can steal a face from another identity (not an idempotent no-op via a plain unique index - that claim came from a stale comment referencing the long-gone insertLocks); FaceRepairRepository has no @GenerateSql so no mise sql regen; the navbar needs no repointing; resolveFaces has no unit coverage, only medium; and relaxing unknown converts one 400 into a success with unknown:0. * docs: chooser page design — equal-weight cards, first-visit and returning states Pierre's review of the chooser design: a first-time admin has never scanned, so the page needs a distinct first-visit presentation, and the two modes should carry equal weight since some admins will live in manual review rather than triaging scans. Equal-weight cards with no recommended badge. First visit gets an explanatory header and advertises that manual review needs no scan — it is fully usable on a brand-new instance, which the returning status-board layout would otherwise obscure. Adds the full card state matrix (never-scanned / running / flagged / zero-flagged / failed), including the manual card being disabled during a scan, which is the UI half of the resolveFaces 409 guard and prevents staging dozens of decisions only to lose them to a conflict. Slices renumbered 1-26 to cover the two chooser presentations and the remaining card states separately. * docs: manual review page design; consolidate 26 slices into 12 Folds the manual review page design into 6.4. The key point is a visual inversion that falls out of the data model: guided stamps every tile because every face holds one of six terminal states, while manual defaults to keep, so tiles start clean and colour appears only where the admin acted. keep needs no colour token - absence signals it, which satisfies the existing rule that state is never encoded in colour alone. Two interaction problems get resolved. Selection cannot claim the whole cluster over server paging, so it always means loaded faces (Select all loaded (N), showing N of M) and whole-cluster work goes through the server-enumerated entireCluster with its own destination picker. And a keep default needs an undo, so Unmark returns a selection to keep - guided never needed one because every face is already stamped. Slices consolidated 26 -> 12 on one test: can it ship alone without leaving the tree broken or dead? The route move now carries its own e2e repair (the move alone leaves CI red), the endpoint carries its SDK regen (no caller otherwise), the eligibility read joins the lock slice it exists for, and the chooser's three slices collapse into one component with one state machine. The pure view-model stays separate despite being small - highest logic risk on the branch. * feat(server): allow locking any eligible face on a person, not only flagged ones Manual review needs to lock a face that no scan flagged, so lock is dropped from the E15 flagged-snapshot gate. stay keeps that gate: it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion, so without a snapshot row there is no owner to record against. Lifting the gate for lock is not merely permissive - it removes a live guard. getScanFlaggedFacesForPersons INNER JOINs asset_face and re-validates personId/deletedAt/isVisible at snapshot-read time, so today a foreign or deleted id can never reach the write path. Dropping the gate without a replacement was verified to be exploitable: with lock removed and no eligibility check, resolveFaces returned locked:1 for a face on another person, for another USER's person, and for a soft-deleted face - silently re-pointing them onto this person's identity through replaceFaceIdentities' unscoped ON CONFLICT DO UPDATE. A nonexistent id crashed with an unhandled FK violation rather than a 400. getEligibleFaceIdsForPerson replaces that guard explicitly, mirroring getClusterFacePage's predicate so lockable == listed on the manual review page. It is advisory - the write-time guards remain authoritative - and exists so an inapplicable lock is a 400 rather than a silent no-op or a crash. Tests: 66/66 in the resolve medium spec, incl. the cross-tenant arm. * feat(server): allow detach and unknown on any face of a person, not only flagged ones Completes the E15 relaxation: stay is now the ONLY snapshot-gated bucket. It keeps that gate because it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion - with no snapshot row that yields undefined and a 500/FK violation. Unlike lock, detach and unknown need no eligibility read: both are already person-scoped at the write layer (detachFaces filters personId and keys its identity-strip on the RETURNING output; reattributeFaces filters still-on-source). That claim is now pinned by test rather than trusted - a foreign face passed to detach is an inert no-op, left completely untouched. Behaviour change (spec 5.4): a stale unknown id - a face that left this person since the scan - previously 400'd via the gate. It now returns success with unknown: 0, truthfully reporting zero parks, and the freshly created cluster is cleaned up rather than orphaned. Tests: 72/72 in the resolve medium spec, 6/6 cross-flow. * feat(server): add admin person metadata endpoint for manual face review GET admin/face-repair/person/:personId returns {id,name,ownerId,faceCount,thumbnailFaceId}, admin-gated, 404 on unknown. The manual review page cannot get these from anywhere else. The guided page reads personName and ownerId off getLatestScan(), and ownerId is what scopes the move-picker - so with no scan the picker cannot open at all. The user-scoped GET /people/:id does not admin-bypass for a person the admin does not own, the same gap already fixed once for face thumbnails. faceCount reuses searchOwnerPeople's exact join conditions so the count agrees between the browser grid and the review page header; a mismatch there would read as a bug. The name is returned raw - an unnamed person yields an empty name and the client applies its own fallback rather than the server inventing a display string. Ships with the client regen because the endpoint has no caller until then. No @GenerateSql was added (this repository carries none) so no mise sql regeneration; mise.lock verified unchanged. Tests: controller 76/76, metadata medium 6/6, tsc clean. * refactor(web): move the guided face-cleanup dashboard to /admin/face-cleanup/scan Frees /admin/face-cleanup for the mode chooser. Pure relocation of the ten-file dashboard cluster via git mv - no behaviour change - plus a temporary 307 redirect at the old path so this lands without a dead entry point. The chooser replaces that redirect next. The navbar is deliberately NOT repointed: NavbarItem highlights via pathname.startsWith(href), so pointing at /admin/face-cleanup keeps it active across /scan, /people and the chooser. Repointing it would be the regression, not the fix. Four e2e assertions broke on the move. Three are repoints; Playwright globs match the whole URL, so '**/admin/face-cleanup' does not match /scan. The fourth needed a real fix rather than a repoint: the post-apply drain check asserts a person's name is ABSENT, which passes vacuously on any page that never lists people - or on one that simply has not finished loading. It now waits for the Eligible faces stat, which renders unconditionally once a scan is completed and is therefore independent of the drained-vs-not condition under test. Tests: 117/117 across the nine guided specs from their new locations, tsc clean. * feat(web): add the face-cleanup mode chooser Replaces slice 4's temporary redirect. Two EQUAL-WEIGHT cards with no recommended badge on either: we do not know which mode a given admin lives in, and privileging guided would penalise anyone who spends their time in manual review. Two distinct presentations rather than one degraded into the other. First visit reframes the difference as readiness, not importance - guided says it needs a scan first, manual advertises that it needs no scan and can start right away. That line matters: manual review is fully functional on a brand-new instance, and the returning status-board layout would otherwise hide that behind an empty guided card, leaving a first-time admin thinking the feature does nothing until a scan finishes. The manual card is disabled while a scan runs. This is the UI half of resolveFaces' 409 guard: without it an admin can stage dozens of decisions across a server-paged cluster and lose them all on Apply. Disabled means genuinely unactivatable - no href and a real <button disabled>, so keyboard cannot reach it either - not just opacity. Only pending/running block it; failed and zero-flagged leave manual reachable. Visual vocabulary is reused verbatim from the dashboard - card shell, dot+label+tabular-nums stat rhythm, semantic colours - so this reads as the same product rather than a bolted-on page. Tests: 13 new chooser cases, 130/130 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-cleanup people browser Builds the destination the chooser's manual card links to. Zero new server endpoints: the owner list reuses searchUsersAdmin (the same call the guided dashboard already makes) and the people grid reuses getFaceRepairOwnerPeople, whose query parameter is optional - so omitting it lists all of an owner's people, paginated. Face crops go through the admin-gated, join-free face-thumbnail route, NOT the user-scoped /people/:id/thumbnail, which 404s for people the admin does not own. A null thumbnailFaceId renders a real placeholder rather than a broken image. Switching owner resets the page and clears the list, with a request token guarding against a stale response clobbering a newer selection - otherwise rows from two owners interleave. Pagination appends rather than replaces. Four states are kept distinct: this owner has no people, no results for this query, load failed, and no users at all. Conflating empty with error was defect D17 on the guided page. No client-side filtering is applied to what the endpoint returns; that passthrough is pinned by test so a later server-side change surfaces here rather than being silently masked. Tests: 14 new cases, 144/144 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review view-model A separate pure model rather than a reuse of the guided one, because the guided model cannot express manual review's semantics and two of its properties are actively dangerous here. No neutral state: guided's FaceState is six TERMINAL states and every face initialises to 'owner', with buildResolveRequest dereferencing suspectedOwnerId for it. Pointed at a scan-free cluster an untouched review would POST a move for every face to an undefined destination. Manual defaults to keep, which produces no bucket entry at all, and an all-keep review builds to null rather than an empty request - so a disabled Apply cannot be bypassed and the server never sees the empty resolve it would 400. State survives paging: guided rebuilds its model from a $derived over the face array, so appending a page discards staged decisions. This model owns its list and exposes appendFaces, which preserves both states and selection and is idempotent on assetFaceId. Manual is server-paged, so that defect would have been hit on the first Load more. stay is hardcoded [] on every request - manual can never emit it, since it needs a suspected owner. Two @ts-expect-error guards make owner/stay a compile error rather than a convention; they are load-bearing, since tsc reports an unused directive if the type ever stops rejecting them. Colour/icon tokens are imported from the guided model so one glyph means one thing across both pages; keep deliberately has no token - it is signalled by absence. Tests: 26 cases, 170/170 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review page with server-paged cluster faces Opens any person with no scan in existence and lists ALL of their faces, via the scan-free cluster endpoint with an empty excludeFaceIds. Implements the visual inversion this page is built around: guided stamps every tile because every face holds one of six terminal states, so its grid is a wall of colour to audit. Manual defaults to keep, so a tile renders a clean crop with no badge and no ribbon, and colour appears only where the admin acted. keep needs no colour token - it is signalled by absence, which keeps the existing rule that state is never encoded in colour alone without inventing a seventh swatch. Selection is honest about server paging: it always means LOADED faces, labelled Select all loaded (N) with showing N of M in the header. An unqualified select-all would either lie about a cluster of thousands or force loading all of it; whole-cluster work goes through the server-enumerated entireCluster instead (slice 10). Load more appends through the model rather than re-creating it, so staged marks and selection survive - the defect that made reusing the guided model impossible, and the first thing a paged cluster would have hit. Person name and ownerId resolve from the URL param through the metadata endpoint rather than navigation state, so refresh and deep links work. Empty and load-error are kept distinct (conflating them was D17 on the guided page), and a person the scan flagged shows no flagged badging - manual ignores scan state entirely. Tests: 15 new cases, 185/185 across the face-cleanup suite, tsc clean. * feat(web): add manual review bulk actions and apply The footer dock: Move to person (via the reused PersonPicker, scoped by the ownerId the metadata endpoint supplies), Lock, Unknown, Not a face, and Unmark - plus the staged-work tally and Apply. Unmark exists because manual defaults to keep. Guided never needed one: every face there is already stamped, so the admin only ever swaps one stamp for another. Here marking is a deliberate act, so mis-marking needs reversing without discarding every other staged decision via Reset. stay and owner are not offered and can never be emitted - both need a suspected owner. The payload assertion pins stay: [] explicitly across mixed buckets. Apply is disabled while everything is keep, because buildResolveRequest returns null and an all-keep POST would be the empty resolve the server 400s. Not a face keeps guided's destructive confirm, and declining it posts nothing. A 409 - a scan starting mid-review - surfaces without discarding staged work. That conflict is what the chooser's disabled-manual card exists to prevent; if it happens anyway the page must not compound it by throwing away decisions staged across a paged cluster. Tests drive the real UI rather than the model. 16 new cases, 201/201 across the face-cleanup suite, guided's 73 specs untouched, tsc clean. * feat(web): add entire-cluster move and the manual actions help modal Move entire cluster uses the server-enumerated entireCluster rather than selection, which is the whole reason it belongs here: on a server-paged page selection can only ever cover LOADED faces, so whole-cluster work must not route through it. Unlike guided - where entireCluster rides the scan's suspected owner - manual has none, so it requires an explicit destination via PersonPicker, and it posts with no per-face buckets since the server rejects combining them. It carries its own confirm because it moves faces the admin has never seen, which is both the point and the risk. Manual gets its own help modal rather than extending guided's, whose names all six actions test is load-bearing and stays untouched. The modal leads with the thing most likely to confuse someone arriving from guided: keep writes nothing. It also states that Not a face is the irreversible one and sits beside Unknown, which means the opposite - bin this crop versus this is a real person I cannot name. Swatches are asserted against the tile tokens so an explanation ties back to the button and the tile it describes, and keep renders no swatch at all - absence is how the default is signalled. Tests: 21 new cases, 222/222 across the face-cleanup suite, guided's specs untouched, tsc clean. * test(e2e): cover the manual face-review flow and its cross-engine invariant Two tests. The first drives the whole manual path against a person with NO scan in existence: chooser to people browser to review page, listing all faces rather than a flagged subset, then move, lock and not-a-face through the real UI, asserting durable DB rows rather than UI text. The second is the point of the feature: a later scan must honour a manual decision exactly as it honours a guided one - the locked face is not re-flagged, the detached face stays gone, the moved face is not re-proposed. Two seeding traps handled. utils.createFace links faces with source='manual', which the verdict layer excludes from flagging, so every face here is downgraded to 'ml' or it would not behave like a machine-clustered one; and face_search rows are required because both the cluster listing and lock eligibility inner-join it. The cross-engine test seeds ONE multi-person scan rather than two sequential ones: two scans stamped the same now() make getLatestScan nondeterministic, which would let the invariant assertions pass vacuously instead of for the right reason. * docs: document guided and manual face cleanup modes Adds a Two ways to clean up comparison and a Manual review section to the admin face cleanup page. The two points worth documenting explicitly are the ones an admin cannot infer: that faces you leave alone are deliberately NOT recorded (marking everything you glanced at as human-verified would stop future scans flagging it and hide real mistakes, so Lock is the opt-in), and that Select all covers the faces currently loaded rather than the whole cluster - with Move entire cluster as the server-resolved alternative. Also notes that manual review is unavailable during a scan, so the behaviour reads as deliberate rather than broken. * fix(web): keep translation-key literal types in the manual review tally A Record<..., string> annotation widened each value to string, and $t only accepts known translation keys, so the tally lookup stopped type-checking. satisfies keeps the exhaustiveness check on the keys while preserving the literal value types. Caught only by CI's check-svelte: that task reports 0 FILES locally in this worktree - even when invoked through mise exactly as CI does - so neither tsc nor the local svelte-check surfaced it. * fix(web): build the manual tally translation key with a template literal satisfies was not enough: indexing an object of whole translation keys still yields string at the call site, and $t accepts a template-literal type but not string. Matches the guided dock's idiom exactly - $t(`admin.face_cleanup_review_tally_${...}`) - by mapping only the key SUFFIX, with move reusing guided's existing 'other' key rather than adding a near-duplicate. Only CI's check-svelte catches this; that task scans 0 files locally even when invoked through mise as CI does. * style(web): redesign the face-cleanup mode chooser Rework the two-door chooser at /admin/face-cleanup into a considered design: a 48px icon tile anchors each mode, guided keeps the primary accent while manual gets a distinct teal identity (neither marked recommended), and each card carries a tinted status well that recolours across all five scan states with the live count as a hero number. Actions become full-width and foot-aligned, with a soft corner glow, inset ring, and hover lift for depth. Logic, copy, i18n keys, data-testids and the genuinely-disabled manual guard (§7) are unchanged; all 13 chooser page-spec tests still pass, eslint/prettier/svelte-check clean. * feat(web): infinite-scroll the manual face-review lists Replace the "Load more" buttons on the manual people browser and the selected-person review grid with scroll-driven pagination, so both lists grow as the admin scrolls instead of dead-ending on a button. Adds a reusable InfiniteScrollSentinel primitive (a sentinel-only port of people-grid's IntersectionObserver + visibility-fallback logic; people-grid itself isn't a drop-in where items aren't keyed by `id`). The review grid keeps appending through vm.appendFaces, so staged marks and selection still survive each page load. * docs: spec the face-cleanup scan two-lane triage redesign * docs: sliced impl-loop plan for the scan two-lane redesign * feat(web): add exclusion-set selection model for scan triage * feat(web): add ConfidentLane with approve-all + spot-check exclude * feat(web): add ReviewFirstLane clickable review list * feat(web): two-lane triage scan page; drop checklist/table/filter/selection UI Compose ConfidentLane (approve-all + spot-check exclude) and ReviewFirstLane (clickable rows) in the scan page, driven by the exclusion-set triage model. Removes the 3-step checklist, 5 stat cards, filter tabs, and the selection bar; the two non-actionable totals become quiet footnotes. Deletes ScanChecklist, FaceCleanupTable, and the old face-cleanup selection model, and prunes their now-dead i18n keys. * docs: note the scan-redesign plan corrections applied during impl-loop * test(e2e): key the scan-drain wait off the header summary, not the removed stat card The two-lane scan redesign removed the "Eligible faces" stat card the X1 test used as its "completed snapshot loaded" signal; use the header summary line ("… flagged faces across … people"), which renders on any completed scan. * fix(web): tidy the review-lane rows — one primary reason pill, no overflow Collapse the wrapping multi-pill reason column to a single primary reason (bad-target wins, else the first) plus a "+N", narrow the flagged column, and share one absolute right-slot between the chevron (default) and the hover dismiss so the row can no longer widen past the container. * feat(web): enlarge the guided-review bulk action bar The routing buttons were text-xs with 13px icons on the dark dock and read as small/hidden. Enlarge them (text-sm, 16px icons, roomier padding, a defining inset ring, rounded-lg), tint the destructive "not a face" red to set it apart from the routine routes, and bump the selected-count and Clear. * docs: spec for face-cleanup console UX fixes (clickable auto-fix chips, review-lane alignment) * docs: implementation plan for face-cleanup console UX fixes * docs: add column-content regression guard to the face-cleanup UX plan * fix(web): align the review-lane columns and give them headings * style(web): prettier fallout on the review-lane changes * feat(web): make auto-fix spot-check chips open the per-cluster review page * fix(web): persist confident-lane spot-check exclusions across chip click-through
Deeds67
added a commit
that referenced
this pull request
Jul 30, 2026
…an (#838) * docs: design spec for manual face review mode Adds a second entry path to the admin face cleanup console: pick any person and audit all of their faces, with no scan required. Reuses the existing tile grid, bulk-select-then-apply interaction, and verdict writes, so a manual decision is indistinguishable from a guided one. Server work is narrow: relax guard E15 for lock/unknown/detach (keeping stay scan-only, since it needs a suspected owner), add the eligibility check that snapshot membership was implicitly providing, and add an admin person-metadata endpoint for the name/ownerId the review page currently reads off the scan. * docs: revise manual face review spec after verification pass Three independent verification passes against the code disproved the first draft's central premise and five other claims. The big one: manual mode cannot reuse the guided review page behind a mode flag. FlaggedFace.suspectedOwnerId is required, every face defaults to the 'owner' state, and buildResolveRequest dereferences it - so an untouched manual review would POST moveToPerson with an undefined destination for every face. There is also no neutral state (six terminal states with a tested tally invariant), and the model is $derived over the face array, so paging would wipe staged decisions. The design now forks the review UI: a separate manual page under /people/[personId] with its own view-model (keep default, stable across appends, never emits stay), leaving all nine guided web specs untouched. Also corrected: lock is a PRIMARY KEY upsert with DO UPDATE that can steal a face from another identity (not an idempotent no-op via a plain unique index - that claim came from a stale comment referencing the long-gone insertLocks); FaceRepairRepository has no @GenerateSql so no mise sql regen; the navbar needs no repointing; resolveFaces has no unit coverage, only medium; and relaxing unknown converts one 400 into a success with unknown:0. * docs: chooser page design — equal-weight cards, first-visit and returning states Pierre's review of the chooser design: a first-time admin has never scanned, so the page needs a distinct first-visit presentation, and the two modes should carry equal weight since some admins will live in manual review rather than triaging scans. Equal-weight cards with no recommended badge. First visit gets an explanatory header and advertises that manual review needs no scan — it is fully usable on a brand-new instance, which the returning status-board layout would otherwise obscure. Adds the full card state matrix (never-scanned / running / flagged / zero-flagged / failed), including the manual card being disabled during a scan, which is the UI half of the resolveFaces 409 guard and prevents staging dozens of decisions only to lose them to a conflict. Slices renumbered 1-26 to cover the two chooser presentations and the remaining card states separately. * docs: manual review page design; consolidate 26 slices into 12 Folds the manual review page design into 6.4. The key point is a visual inversion that falls out of the data model: guided stamps every tile because every face holds one of six terminal states, while manual defaults to keep, so tiles start clean and colour appears only where the admin acted. keep needs no colour token - absence signals it, which satisfies the existing rule that state is never encoded in colour alone. Two interaction problems get resolved. Selection cannot claim the whole cluster over server paging, so it always means loaded faces (Select all loaded (N), showing N of M) and whole-cluster work goes through the server-enumerated entireCluster with its own destination picker. And a keep default needs an undo, so Unmark returns a selection to keep - guided never needed one because every face is already stamped. Slices consolidated 26 -> 12 on one test: can it ship alone without leaving the tree broken or dead? The route move now carries its own e2e repair (the move alone leaves CI red), the endpoint carries its SDK regen (no caller otherwise), the eligibility read joins the lock slice it exists for, and the chooser's three slices collapse into one component with one state machine. The pure view-model stays separate despite being small - highest logic risk on the branch. * feat(server): allow locking any eligible face on a person, not only flagged ones Manual review needs to lock a face that no scan flagged, so lock is dropped from the E15 flagged-snapshot gate. stay keeps that gate: it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion, so without a snapshot row there is no owner to record against. Lifting the gate for lock is not merely permissive - it removes a live guard. getScanFlaggedFacesForPersons INNER JOINs asset_face and re-validates personId/deletedAt/isVisible at snapshot-read time, so today a foreign or deleted id can never reach the write path. Dropping the gate without a replacement was verified to be exploitable: with lock removed and no eligibility check, resolveFaces returned locked:1 for a face on another person, for another USER's person, and for a soft-deleted face - silently re-pointing them onto this person's identity through replaceFaceIdentities' unscoped ON CONFLICT DO UPDATE. A nonexistent id crashed with an unhandled FK violation rather than a 400. getEligibleFaceIdsForPerson replaces that guard explicitly, mirroring getClusterFacePage's predicate so lockable == listed on the manual review page. It is advisory - the write-time guards remain authoritative - and exists so an inapplicable lock is a 400 rather than a silent no-op or a crash. Tests: 66/66 in the resolve medium spec, incl. the cross-tenant arm. * feat(server): allow detach and unknown on any face of a person, not only flagged ones Completes the E15 relaxation: stay is now the ONLY snapshot-gated bucket. It keeps that gate because it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion - with no snapshot row that yields undefined and a 500/FK violation. Unlike lock, detach and unknown need no eligibility read: both are already person-scoped at the write layer (detachFaces filters personId and keys its identity-strip on the RETURNING output; reattributeFaces filters still-on-source). That claim is now pinned by test rather than trusted - a foreign face passed to detach is an inert no-op, left completely untouched. Behaviour change (spec 5.4): a stale unknown id - a face that left this person since the scan - previously 400'd via the gate. It now returns success with unknown: 0, truthfully reporting zero parks, and the freshly created cluster is cleaned up rather than orphaned. Tests: 72/72 in the resolve medium spec, 6/6 cross-flow. * feat(server): add admin person metadata endpoint for manual face review GET admin/face-repair/person/:personId returns {id,name,ownerId,faceCount,thumbnailFaceId}, admin-gated, 404 on unknown. The manual review page cannot get these from anywhere else. The guided page reads personName and ownerId off getLatestScan(), and ownerId is what scopes the move-picker - so with no scan the picker cannot open at all. The user-scoped GET /people/:id does not admin-bypass for a person the admin does not own, the same gap already fixed once for face thumbnails. faceCount reuses searchOwnerPeople's exact join conditions so the count agrees between the browser grid and the review page header; a mismatch there would read as a bug. The name is returned raw - an unnamed person yields an empty name and the client applies its own fallback rather than the server inventing a display string. Ships with the client regen because the endpoint has no caller until then. No @GenerateSql was added (this repository carries none) so no mise sql regeneration; mise.lock verified unchanged. Tests: controller 76/76, metadata medium 6/6, tsc clean. * refactor(web): move the guided face-cleanup dashboard to /admin/face-cleanup/scan Frees /admin/face-cleanup for the mode chooser. Pure relocation of the ten-file dashboard cluster via git mv - no behaviour change - plus a temporary 307 redirect at the old path so this lands without a dead entry point. The chooser replaces that redirect next. The navbar is deliberately NOT repointed: NavbarItem highlights via pathname.startsWith(href), so pointing at /admin/face-cleanup keeps it active across /scan, /people and the chooser. Repointing it would be the regression, not the fix. Four e2e assertions broke on the move. Three are repoints; Playwright globs match the whole URL, so '**/admin/face-cleanup' does not match /scan. The fourth needed a real fix rather than a repoint: the post-apply drain check asserts a person's name is ABSENT, which passes vacuously on any page that never lists people - or on one that simply has not finished loading. It now waits for the Eligible faces stat, which renders unconditionally once a scan is completed and is therefore independent of the drained-vs-not condition under test. Tests: 117/117 across the nine guided specs from their new locations, tsc clean. * feat(web): add the face-cleanup mode chooser Replaces slice 4's temporary redirect. Two EQUAL-WEIGHT cards with no recommended badge on either: we do not know which mode a given admin lives in, and privileging guided would penalise anyone who spends their time in manual review. Two distinct presentations rather than one degraded into the other. First visit reframes the difference as readiness, not importance - guided says it needs a scan first, manual advertises that it needs no scan and can start right away. That line matters: manual review is fully functional on a brand-new instance, and the returning status-board layout would otherwise hide that behind an empty guided card, leaving a first-time admin thinking the feature does nothing until a scan finishes. The manual card is disabled while a scan runs. This is the UI half of resolveFaces' 409 guard: without it an admin can stage dozens of decisions across a server-paged cluster and lose them all on Apply. Disabled means genuinely unactivatable - no href and a real <button disabled>, so keyboard cannot reach it either - not just opacity. Only pending/running block it; failed and zero-flagged leave manual reachable. Visual vocabulary is reused verbatim from the dashboard - card shell, dot+label+tabular-nums stat rhythm, semantic colours - so this reads as the same product rather than a bolted-on page. Tests: 13 new chooser cases, 130/130 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-cleanup people browser Builds the destination the chooser's manual card links to. Zero new server endpoints: the owner list reuses searchUsersAdmin (the same call the guided dashboard already makes) and the people grid reuses getFaceRepairOwnerPeople, whose query parameter is optional - so omitting it lists all of an owner's people, paginated. Face crops go through the admin-gated, join-free face-thumbnail route, NOT the user-scoped /people/:id/thumbnail, which 404s for people the admin does not own. A null thumbnailFaceId renders a real placeholder rather than a broken image. Switching owner resets the page and clears the list, with a request token guarding against a stale response clobbering a newer selection - otherwise rows from two owners interleave. Pagination appends rather than replaces. Four states are kept distinct: this owner has no people, no results for this query, load failed, and no users at all. Conflating empty with error was defect D17 on the guided page. No client-side filtering is applied to what the endpoint returns; that passthrough is pinned by test so a later server-side change surfaces here rather than being silently masked. Tests: 14 new cases, 144/144 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review view-model A separate pure model rather than a reuse of the guided one, because the guided model cannot express manual review's semantics and two of its properties are actively dangerous here. No neutral state: guided's FaceState is six TERMINAL states and every face initialises to 'owner', with buildResolveRequest dereferencing suspectedOwnerId for it. Pointed at a scan-free cluster an untouched review would POST a move for every face to an undefined destination. Manual defaults to keep, which produces no bucket entry at all, and an all-keep review builds to null rather than an empty request - so a disabled Apply cannot be bypassed and the server never sees the empty resolve it would 400. State survives paging: guided rebuilds its model from a $derived over the face array, so appending a page discards staged decisions. This model owns its list and exposes appendFaces, which preserves both states and selection and is idempotent on assetFaceId. Manual is server-paged, so that defect would have been hit on the first Load more. stay is hardcoded [] on every request - manual can never emit it, since it needs a suspected owner. Two @ts-expect-error guards make owner/stay a compile error rather than a convention; they are load-bearing, since tsc reports an unused directive if the type ever stops rejecting them. Colour/icon tokens are imported from the guided model so one glyph means one thing across both pages; keep deliberately has no token - it is signalled by absence. Tests: 26 cases, 170/170 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review page with server-paged cluster faces Opens any person with no scan in existence and lists ALL of their faces, via the scan-free cluster endpoint with an empty excludeFaceIds. Implements the visual inversion this page is built around: guided stamps every tile because every face holds one of six terminal states, so its grid is a wall of colour to audit. Manual defaults to keep, so a tile renders a clean crop with no badge and no ribbon, and colour appears only where the admin acted. keep needs no colour token - it is signalled by absence, which keeps the existing rule that state is never encoded in colour alone without inventing a seventh swatch. Selection is honest about server paging: it always means LOADED faces, labelled Select all loaded (N) with showing N of M in the header. An unqualified select-all would either lie about a cluster of thousands or force loading all of it; whole-cluster work goes through the server-enumerated entireCluster instead (slice 10). Load more appends through the model rather than re-creating it, so staged marks and selection survive - the defect that made reusing the guided model impossible, and the first thing a paged cluster would have hit. Person name and ownerId resolve from the URL param through the metadata endpoint rather than navigation state, so refresh and deep links work. Empty and load-error are kept distinct (conflating them was D17 on the guided page), and a person the scan flagged shows no flagged badging - manual ignores scan state entirely. Tests: 15 new cases, 185/185 across the face-cleanup suite, tsc clean. * feat(web): add manual review bulk actions and apply The footer dock: Move to person (via the reused PersonPicker, scoped by the ownerId the metadata endpoint supplies), Lock, Unknown, Not a face, and Unmark - plus the staged-work tally and Apply. Unmark exists because manual defaults to keep. Guided never needed one: every face there is already stamped, so the admin only ever swaps one stamp for another. Here marking is a deliberate act, so mis-marking needs reversing without discarding every other staged decision via Reset. stay and owner are not offered and can never be emitted - both need a suspected owner. The payload assertion pins stay: [] explicitly across mixed buckets. Apply is disabled while everything is keep, because buildResolveRequest returns null and an all-keep POST would be the empty resolve the server 400s. Not a face keeps guided's destructive confirm, and declining it posts nothing. A 409 - a scan starting mid-review - surfaces without discarding staged work. That conflict is what the chooser's disabled-manual card exists to prevent; if it happens anyway the page must not compound it by throwing away decisions staged across a paged cluster. Tests drive the real UI rather than the model. 16 new cases, 201/201 across the face-cleanup suite, guided's 73 specs untouched, tsc clean. * feat(web): add entire-cluster move and the manual actions help modal Move entire cluster uses the server-enumerated entireCluster rather than selection, which is the whole reason it belongs here: on a server-paged page selection can only ever cover LOADED faces, so whole-cluster work must not route through it. Unlike guided - where entireCluster rides the scan's suspected owner - manual has none, so it requires an explicit destination via PersonPicker, and it posts with no per-face buckets since the server rejects combining them. It carries its own confirm because it moves faces the admin has never seen, which is both the point and the risk. Manual gets its own help modal rather than extending guided's, whose names all six actions test is load-bearing and stays untouched. The modal leads with the thing most likely to confuse someone arriving from guided: keep writes nothing. It also states that Not a face is the irreversible one and sits beside Unknown, which means the opposite - bin this crop versus this is a real person I cannot name. Swatches are asserted against the tile tokens so an explanation ties back to the button and the tile it describes, and keep renders no swatch at all - absence is how the default is signalled. Tests: 21 new cases, 222/222 across the face-cleanup suite, guided's specs untouched, tsc clean. * test(e2e): cover the manual face-review flow and its cross-engine invariant Two tests. The first drives the whole manual path against a person with NO scan in existence: chooser to people browser to review page, listing all faces rather than a flagged subset, then move, lock and not-a-face through the real UI, asserting durable DB rows rather than UI text. The second is the point of the feature: a later scan must honour a manual decision exactly as it honours a guided one - the locked face is not re-flagged, the detached face stays gone, the moved face is not re-proposed. Two seeding traps handled. utils.createFace links faces with source='manual', which the verdict layer excludes from flagging, so every face here is downgraded to 'ml' or it would not behave like a machine-clustered one; and face_search rows are required because both the cluster listing and lock eligibility inner-join it. The cross-engine test seeds ONE multi-person scan rather than two sequential ones: two scans stamped the same now() make getLatestScan nondeterministic, which would let the invariant assertions pass vacuously instead of for the right reason. * docs: document guided and manual face cleanup modes Adds a Two ways to clean up comparison and a Manual review section to the admin face cleanup page. The two points worth documenting explicitly are the ones an admin cannot infer: that faces you leave alone are deliberately NOT recorded (marking everything you glanced at as human-verified would stop future scans flagging it and hide real mistakes, so Lock is the opt-in), and that Select all covers the faces currently loaded rather than the whole cluster - with Move entire cluster as the server-resolved alternative. Also notes that manual review is unavailable during a scan, so the behaviour reads as deliberate rather than broken. * fix(web): keep translation-key literal types in the manual review tally A Record<..., string> annotation widened each value to string, and $t only accepts known translation keys, so the tally lookup stopped type-checking. satisfies keeps the exhaustiveness check on the keys while preserving the literal value types. Caught only by CI's check-svelte: that task reports 0 FILES locally in this worktree - even when invoked through mise exactly as CI does - so neither tsc nor the local svelte-check surfaced it. * fix(web): build the manual tally translation key with a template literal satisfies was not enough: indexing an object of whole translation keys still yields string at the call site, and $t accepts a template-literal type but not string. Matches the guided dock's idiom exactly - $t(`admin.face_cleanup_review_tally_${...}`) - by mapping only the key SUFFIX, with move reusing guided's existing 'other' key rather than adding a near-duplicate. Only CI's check-svelte catches this; that task scans 0 files locally even when invoked through mise as CI does. * style(web): redesign the face-cleanup mode chooser Rework the two-door chooser at /admin/face-cleanup into a considered design: a 48px icon tile anchors each mode, guided keeps the primary accent while manual gets a distinct teal identity (neither marked recommended), and each card carries a tinted status well that recolours across all five scan states with the live count as a hero number. Actions become full-width and foot-aligned, with a soft corner glow, inset ring, and hover lift for depth. Logic, copy, i18n keys, data-testids and the genuinely-disabled manual guard (§7) are unchanged; all 13 chooser page-spec tests still pass, eslint/prettier/svelte-check clean. * feat(web): infinite-scroll the manual face-review lists Replace the "Load more" buttons on the manual people browser and the selected-person review grid with scroll-driven pagination, so both lists grow as the admin scrolls instead of dead-ending on a button. Adds a reusable InfiniteScrollSentinel primitive (a sentinel-only port of people-grid's IntersectionObserver + visibility-fallback logic; people-grid itself isn't a drop-in where items aren't keyed by `id`). The review grid keeps appending through vm.appendFaces, so staged marks and selection still survive each page load. * docs: spec the face-cleanup scan two-lane triage redesign * docs: sliced impl-loop plan for the scan two-lane redesign * feat(web): add exclusion-set selection model for scan triage * feat(web): add ConfidentLane with approve-all + spot-check exclude * feat(web): add ReviewFirstLane clickable review list * feat(web): two-lane triage scan page; drop checklist/table/filter/selection UI Compose ConfidentLane (approve-all + spot-check exclude) and ReviewFirstLane (clickable rows) in the scan page, driven by the exclusion-set triage model. Removes the 3-step checklist, 5 stat cards, filter tabs, and the selection bar; the two non-actionable totals become quiet footnotes. Deletes ScanChecklist, FaceCleanupTable, and the old face-cleanup selection model, and prunes their now-dead i18n keys. * docs: note the scan-redesign plan corrections applied during impl-loop * test(e2e): key the scan-drain wait off the header summary, not the removed stat card The two-lane scan redesign removed the "Eligible faces" stat card the X1 test used as its "completed snapshot loaded" signal; use the header summary line ("… flagged faces across … people"), which renders on any completed scan. * fix(web): tidy the review-lane rows — one primary reason pill, no overflow Collapse the wrapping multi-pill reason column to a single primary reason (bad-target wins, else the first) plus a "+N", narrow the flagged column, and share one absolute right-slot between the chevron (default) and the hover dismiss so the row can no longer widen past the container. * feat(web): enlarge the guided-review bulk action bar The routing buttons were text-xs with 13px icons on the dark dock and read as small/hidden. Enlarge them (text-sm, 16px icons, roomier padding, a defining inset ring, rounded-lg), tint the destructive "not a face" red to set it apart from the routine routes, and bump the selected-count and Clear. * docs: spec for face-cleanup console UX fixes (clickable auto-fix chips, review-lane alignment) * docs: implementation plan for face-cleanup console UX fixes * docs: add column-content regression guard to the face-cleanup UX plan * fix(web): align the review-lane columns and give them headings * style(web): prettier fallout on the review-lane changes * feat(web): make auto-fix spot-check chips open the per-cluster review page * fix(web): persist confident-lane spot-check exclusions across chip click-through
Deeds67
added a commit
that referenced
this pull request
Aug 2, 2026
…an (#838) * docs: design spec for manual face review mode Adds a second entry path to the admin face cleanup console: pick any person and audit all of their faces, with no scan required. Reuses the existing tile grid, bulk-select-then-apply interaction, and verdict writes, so a manual decision is indistinguishable from a guided one. Server work is narrow: relax guard E15 for lock/unknown/detach (keeping stay scan-only, since it needs a suspected owner), add the eligibility check that snapshot membership was implicitly providing, and add an admin person-metadata endpoint for the name/ownerId the review page currently reads off the scan. * docs: revise manual face review spec after verification pass Three independent verification passes against the code disproved the first draft's central premise and five other claims. The big one: manual mode cannot reuse the guided review page behind a mode flag. FlaggedFace.suspectedOwnerId is required, every face defaults to the 'owner' state, and buildResolveRequest dereferences it - so an untouched manual review would POST moveToPerson with an undefined destination for every face. There is also no neutral state (six terminal states with a tested tally invariant), and the model is $derived over the face array, so paging would wipe staged decisions. The design now forks the review UI: a separate manual page under /people/[personId] with its own view-model (keep default, stable across appends, never emits stay), leaving all nine guided web specs untouched. Also corrected: lock is a PRIMARY KEY upsert with DO UPDATE that can steal a face from another identity (not an idempotent no-op via a plain unique index - that claim came from a stale comment referencing the long-gone insertLocks); FaceRepairRepository has no @GenerateSql so no mise sql regen; the navbar needs no repointing; resolveFaces has no unit coverage, only medium; and relaxing unknown converts one 400 into a success with unknown:0. * docs: chooser page design — equal-weight cards, first-visit and returning states Pierre's review of the chooser design: a first-time admin has never scanned, so the page needs a distinct first-visit presentation, and the two modes should carry equal weight since some admins will live in manual review rather than triaging scans. Equal-weight cards with no recommended badge. First visit gets an explanatory header and advertises that manual review needs no scan — it is fully usable on a brand-new instance, which the returning status-board layout would otherwise obscure. Adds the full card state matrix (never-scanned / running / flagged / zero-flagged / failed), including the manual card being disabled during a scan, which is the UI half of the resolveFaces 409 guard and prevents staging dozens of decisions only to lose them to a conflict. Slices renumbered 1-26 to cover the two chooser presentations and the remaining card states separately. * docs: manual review page design; consolidate 26 slices into 12 Folds the manual review page design into 6.4. The key point is a visual inversion that falls out of the data model: guided stamps every tile because every face holds one of six terminal states, while manual defaults to keep, so tiles start clean and colour appears only where the admin acted. keep needs no colour token - absence signals it, which satisfies the existing rule that state is never encoded in colour alone. Two interaction problems get resolved. Selection cannot claim the whole cluster over server paging, so it always means loaded faces (Select all loaded (N), showing N of M) and whole-cluster work goes through the server-enumerated entireCluster with its own destination picker. And a keep default needs an undo, so Unmark returns a selection to keep - guided never needed one because every face is already stamped. Slices consolidated 26 -> 12 on one test: can it ship alone without leaving the tree broken or dead? The route move now carries its own e2e repair (the move alone leaves CI red), the endpoint carries its SDK regen (no caller otherwise), the eligibility read joins the lock slice it exists for, and the chooser's three slices collapse into one component with one state machine. The pure view-model stays separate despite being small - highest logic risk on the branch. * feat(server): allow locking any eligible face on a person, not only flagged ones Manual review needs to lock a face that no scan flagged, so lock is dropped from the E15 flagged-snapshot gate. stay keeps that gate: it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion, so without a snapshot row there is no owner to record against. Lifting the gate for lock is not merely permissive - it removes a live guard. getScanFlaggedFacesForPersons INNER JOINs asset_face and re-validates personId/deletedAt/isVisible at snapshot-read time, so today a foreign or deleted id can never reach the write path. Dropping the gate without a replacement was verified to be exploitable: with lock removed and no eligibility check, resolveFaces returned locked:1 for a face on another person, for another USER's person, and for a soft-deleted face - silently re-pointing them onto this person's identity through replaceFaceIdentities' unscoped ON CONFLICT DO UPDATE. A nonexistent id crashed with an unhandled FK violation rather than a 400. getEligibleFaceIdsForPerson replaces that guard explicitly, mirroring getClusterFacePage's predicate so lockable == listed on the manual review page. It is advisory - the write-time guards remain authoritative - and exists so an inapplicable lock is a 400 rather than a silent no-op or a crash. Tests: 66/66 in the resolve medium spec, incl. the cross-tenant arm. * feat(server): allow detach and unknown on any face of a person, not only flagged ones Completes the E15 relaxation: stay is now the ONLY snapshot-gated bucket. It keeps that gate because it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion - with no snapshot row that yields undefined and a 500/FK violation. Unlike lock, detach and unknown need no eligibility read: both are already person-scoped at the write layer (detachFaces filters personId and keys its identity-strip on the RETURNING output; reattributeFaces filters still-on-source). That claim is now pinned by test rather than trusted - a foreign face passed to detach is an inert no-op, left completely untouched. Behaviour change (spec 5.4): a stale unknown id - a face that left this person since the scan - previously 400'd via the gate. It now returns success with unknown: 0, truthfully reporting zero parks, and the freshly created cluster is cleaned up rather than orphaned. Tests: 72/72 in the resolve medium spec, 6/6 cross-flow. * feat(server): add admin person metadata endpoint for manual face review GET admin/face-repair/person/:personId returns {id,name,ownerId,faceCount,thumbnailFaceId}, admin-gated, 404 on unknown. The manual review page cannot get these from anywhere else. The guided page reads personName and ownerId off getLatestScan(), and ownerId is what scopes the move-picker - so with no scan the picker cannot open at all. The user-scoped GET /people/:id does not admin-bypass for a person the admin does not own, the same gap already fixed once for face thumbnails. faceCount reuses searchOwnerPeople's exact join conditions so the count agrees between the browser grid and the review page header; a mismatch there would read as a bug. The name is returned raw - an unnamed person yields an empty name and the client applies its own fallback rather than the server inventing a display string. Ships with the client regen because the endpoint has no caller until then. No @GenerateSql was added (this repository carries none) so no mise sql regeneration; mise.lock verified unchanged. Tests: controller 76/76, metadata medium 6/6, tsc clean. * refactor(web): move the guided face-cleanup dashboard to /admin/face-cleanup/scan Frees /admin/face-cleanup for the mode chooser. Pure relocation of the ten-file dashboard cluster via git mv - no behaviour change - plus a temporary 307 redirect at the old path so this lands without a dead entry point. The chooser replaces that redirect next. The navbar is deliberately NOT repointed: NavbarItem highlights via pathname.startsWith(href), so pointing at /admin/face-cleanup keeps it active across /scan, /people and the chooser. Repointing it would be the regression, not the fix. Four e2e assertions broke on the move. Three are repoints; Playwright globs match the whole URL, so '**/admin/face-cleanup' does not match /scan. The fourth needed a real fix rather than a repoint: the post-apply drain check asserts a person's name is ABSENT, which passes vacuously on any page that never lists people - or on one that simply has not finished loading. It now waits for the Eligible faces stat, which renders unconditionally once a scan is completed and is therefore independent of the drained-vs-not condition under test. Tests: 117/117 across the nine guided specs from their new locations, tsc clean. * feat(web): add the face-cleanup mode chooser Replaces slice 4's temporary redirect. Two EQUAL-WEIGHT cards with no recommended badge on either: we do not know which mode a given admin lives in, and privileging guided would penalise anyone who spends their time in manual review. Two distinct presentations rather than one degraded into the other. First visit reframes the difference as readiness, not importance - guided says it needs a scan first, manual advertises that it needs no scan and can start right away. That line matters: manual review is fully functional on a brand-new instance, and the returning status-board layout would otherwise hide that behind an empty guided card, leaving a first-time admin thinking the feature does nothing until a scan finishes. The manual card is disabled while a scan runs. This is the UI half of resolveFaces' 409 guard: without it an admin can stage dozens of decisions across a server-paged cluster and lose them all on Apply. Disabled means genuinely unactivatable - no href and a real <button disabled>, so keyboard cannot reach it either - not just opacity. Only pending/running block it; failed and zero-flagged leave manual reachable. Visual vocabulary is reused verbatim from the dashboard - card shell, dot+label+tabular-nums stat rhythm, semantic colours - so this reads as the same product rather than a bolted-on page. Tests: 13 new chooser cases, 130/130 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-cleanup people browser Builds the destination the chooser's manual card links to. Zero new server endpoints: the owner list reuses searchUsersAdmin (the same call the guided dashboard already makes) and the people grid reuses getFaceRepairOwnerPeople, whose query parameter is optional - so omitting it lists all of an owner's people, paginated. Face crops go through the admin-gated, join-free face-thumbnail route, NOT the user-scoped /people/:id/thumbnail, which 404s for people the admin does not own. A null thumbnailFaceId renders a real placeholder rather than a broken image. Switching owner resets the page and clears the list, with a request token guarding against a stale response clobbering a newer selection - otherwise rows from two owners interleave. Pagination appends rather than replaces. Four states are kept distinct: this owner has no people, no results for this query, load failed, and no users at all. Conflating empty with error was defect D17 on the guided page. No client-side filtering is applied to what the endpoint returns; that passthrough is pinned by test so a later server-side change surfaces here rather than being silently masked. Tests: 14 new cases, 144/144 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review view-model A separate pure model rather than a reuse of the guided one, because the guided model cannot express manual review's semantics and two of its properties are actively dangerous here. No neutral state: guided's FaceState is six TERMINAL states and every face initialises to 'owner', with buildResolveRequest dereferencing suspectedOwnerId for it. Pointed at a scan-free cluster an untouched review would POST a move for every face to an undefined destination. Manual defaults to keep, which produces no bucket entry at all, and an all-keep review builds to null rather than an empty request - so a disabled Apply cannot be bypassed and the server never sees the empty resolve it would 400. State survives paging: guided rebuilds its model from a $derived over the face array, so appending a page discards staged decisions. This model owns its list and exposes appendFaces, which preserves both states and selection and is idempotent on assetFaceId. Manual is server-paged, so that defect would have been hit on the first Load more. stay is hardcoded [] on every request - manual can never emit it, since it needs a suspected owner. Two @ts-expect-error guards make owner/stay a compile error rather than a convention; they are load-bearing, since tsc reports an unused directive if the type ever stops rejecting them. Colour/icon tokens are imported from the guided model so one glyph means one thing across both pages; keep deliberately has no token - it is signalled by absence. Tests: 26 cases, 170/170 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review page with server-paged cluster faces Opens any person with no scan in existence and lists ALL of their faces, via the scan-free cluster endpoint with an empty excludeFaceIds. Implements the visual inversion this page is built around: guided stamps every tile because every face holds one of six terminal states, so its grid is a wall of colour to audit. Manual defaults to keep, so a tile renders a clean crop with no badge and no ribbon, and colour appears only where the admin acted. keep needs no colour token - it is signalled by absence, which keeps the existing rule that state is never encoded in colour alone without inventing a seventh swatch. Selection is honest about server paging: it always means LOADED faces, labelled Select all loaded (N) with showing N of M in the header. An unqualified select-all would either lie about a cluster of thousands or force loading all of it; whole-cluster work goes through the server-enumerated entireCluster instead (slice 10). Load more appends through the model rather than re-creating it, so staged marks and selection survive - the defect that made reusing the guided model impossible, and the first thing a paged cluster would have hit. Person name and ownerId resolve from the URL param through the metadata endpoint rather than navigation state, so refresh and deep links work. Empty and load-error are kept distinct (conflating them was D17 on the guided page), and a person the scan flagged shows no flagged badging - manual ignores scan state entirely. Tests: 15 new cases, 185/185 across the face-cleanup suite, tsc clean. * feat(web): add manual review bulk actions and apply The footer dock: Move to person (via the reused PersonPicker, scoped by the ownerId the metadata endpoint supplies), Lock, Unknown, Not a face, and Unmark - plus the staged-work tally and Apply. Unmark exists because manual defaults to keep. Guided never needed one: every face there is already stamped, so the admin only ever swaps one stamp for another. Here marking is a deliberate act, so mis-marking needs reversing without discarding every other staged decision via Reset. stay and owner are not offered and can never be emitted - both need a suspected owner. The payload assertion pins stay: [] explicitly across mixed buckets. Apply is disabled while everything is keep, because buildResolveRequest returns null and an all-keep POST would be the empty resolve the server 400s. Not a face keeps guided's destructive confirm, and declining it posts nothing. A 409 - a scan starting mid-review - surfaces without discarding staged work. That conflict is what the chooser's disabled-manual card exists to prevent; if it happens anyway the page must not compound it by throwing away decisions staged across a paged cluster. Tests drive the real UI rather than the model. 16 new cases, 201/201 across the face-cleanup suite, guided's 73 specs untouched, tsc clean. * feat(web): add entire-cluster move and the manual actions help modal Move entire cluster uses the server-enumerated entireCluster rather than selection, which is the whole reason it belongs here: on a server-paged page selection can only ever cover LOADED faces, so whole-cluster work must not route through it. Unlike guided - where entireCluster rides the scan's suspected owner - manual has none, so it requires an explicit destination via PersonPicker, and it posts with no per-face buckets since the server rejects combining them. It carries its own confirm because it moves faces the admin has never seen, which is both the point and the risk. Manual gets its own help modal rather than extending guided's, whose names all six actions test is load-bearing and stays untouched. The modal leads with the thing most likely to confuse someone arriving from guided: keep writes nothing. It also states that Not a face is the irreversible one and sits beside Unknown, which means the opposite - bin this crop versus this is a real person I cannot name. Swatches are asserted against the tile tokens so an explanation ties back to the button and the tile it describes, and keep renders no swatch at all - absence is how the default is signalled. Tests: 21 new cases, 222/222 across the face-cleanup suite, guided's specs untouched, tsc clean. * test(e2e): cover the manual face-review flow and its cross-engine invariant Two tests. The first drives the whole manual path against a person with NO scan in existence: chooser to people browser to review page, listing all faces rather than a flagged subset, then move, lock and not-a-face through the real UI, asserting durable DB rows rather than UI text. The second is the point of the feature: a later scan must honour a manual decision exactly as it honours a guided one - the locked face is not re-flagged, the detached face stays gone, the moved face is not re-proposed. Two seeding traps handled. utils.createFace links faces with source='manual', which the verdict layer excludes from flagging, so every face here is downgraded to 'ml' or it would not behave like a machine-clustered one; and face_search rows are required because both the cluster listing and lock eligibility inner-join it. The cross-engine test seeds ONE multi-person scan rather than two sequential ones: two scans stamped the same now() make getLatestScan nondeterministic, which would let the invariant assertions pass vacuously instead of for the right reason. * docs: document guided and manual face cleanup modes Adds a Two ways to clean up comparison and a Manual review section to the admin face cleanup page. The two points worth documenting explicitly are the ones an admin cannot infer: that faces you leave alone are deliberately NOT recorded (marking everything you glanced at as human-verified would stop future scans flagging it and hide real mistakes, so Lock is the opt-in), and that Select all covers the faces currently loaded rather than the whole cluster - with Move entire cluster as the server-resolved alternative. Also notes that manual review is unavailable during a scan, so the behaviour reads as deliberate rather than broken. * fix(web): keep translation-key literal types in the manual review tally A Record<..., string> annotation widened each value to string, and $t only accepts known translation keys, so the tally lookup stopped type-checking. satisfies keeps the exhaustiveness check on the keys while preserving the literal value types. Caught only by CI's check-svelte: that task reports 0 FILES locally in this worktree - even when invoked through mise exactly as CI does - so neither tsc nor the local svelte-check surfaced it. * fix(web): build the manual tally translation key with a template literal satisfies was not enough: indexing an object of whole translation keys still yields string at the call site, and $t accepts a template-literal type but not string. Matches the guided dock's idiom exactly - $t(`admin.face_cleanup_review_tally_${...}`) - by mapping only the key SUFFIX, with move reusing guided's existing 'other' key rather than adding a near-duplicate. Only CI's check-svelte catches this; that task scans 0 files locally even when invoked through mise as CI does. * style(web): redesign the face-cleanup mode chooser Rework the two-door chooser at /admin/face-cleanup into a considered design: a 48px icon tile anchors each mode, guided keeps the primary accent while manual gets a distinct teal identity (neither marked recommended), and each card carries a tinted status well that recolours across all five scan states with the live count as a hero number. Actions become full-width and foot-aligned, with a soft corner glow, inset ring, and hover lift for depth. Logic, copy, i18n keys, data-testids and the genuinely-disabled manual guard (§7) are unchanged; all 13 chooser page-spec tests still pass, eslint/prettier/svelte-check clean. * feat(web): infinite-scroll the manual face-review lists Replace the "Load more" buttons on the manual people browser and the selected-person review grid with scroll-driven pagination, so both lists grow as the admin scrolls instead of dead-ending on a button. Adds a reusable InfiniteScrollSentinel primitive (a sentinel-only port of people-grid's IntersectionObserver + visibility-fallback logic; people-grid itself isn't a drop-in where items aren't keyed by `id`). The review grid keeps appending through vm.appendFaces, so staged marks and selection still survive each page load. * docs: spec the face-cleanup scan two-lane triage redesign * docs: sliced impl-loop plan for the scan two-lane redesign * feat(web): add exclusion-set selection model for scan triage * feat(web): add ConfidentLane with approve-all + spot-check exclude * feat(web): add ReviewFirstLane clickable review list * feat(web): two-lane triage scan page; drop checklist/table/filter/selection UI Compose ConfidentLane (approve-all + spot-check exclude) and ReviewFirstLane (clickable rows) in the scan page, driven by the exclusion-set triage model. Removes the 3-step checklist, 5 stat cards, filter tabs, and the selection bar; the two non-actionable totals become quiet footnotes. Deletes ScanChecklist, FaceCleanupTable, and the old face-cleanup selection model, and prunes their now-dead i18n keys. * docs: note the scan-redesign plan corrections applied during impl-loop * test(e2e): key the scan-drain wait off the header summary, not the removed stat card The two-lane scan redesign removed the "Eligible faces" stat card the X1 test used as its "completed snapshot loaded" signal; use the header summary line ("… flagged faces across … people"), which renders on any completed scan. * fix(web): tidy the review-lane rows — one primary reason pill, no overflow Collapse the wrapping multi-pill reason column to a single primary reason (bad-target wins, else the first) plus a "+N", narrow the flagged column, and share one absolute right-slot between the chevron (default) and the hover dismiss so the row can no longer widen past the container. * feat(web): enlarge the guided-review bulk action bar The routing buttons were text-xs with 13px icons on the dark dock and read as small/hidden. Enlarge them (text-sm, 16px icons, roomier padding, a defining inset ring, rounded-lg), tint the destructive "not a face" red to set it apart from the routine routes, and bump the selected-count and Clear. * docs: spec for face-cleanup console UX fixes (clickable auto-fix chips, review-lane alignment) * docs: implementation plan for face-cleanup console UX fixes * docs: add column-content regression guard to the face-cleanup UX plan * fix(web): align the review-lane columns and give them headings * style(web): prettier fallout on the review-lane changes * feat(web): make auto-fix spot-check chips open the per-cluster review page * fix(web): persist confident-lane spot-check exclusions across chip click-through
Deeds67
added a commit
that referenced
this pull request
Aug 5, 2026
…an (#838) * docs: design spec for manual face review mode Adds a second entry path to the admin face cleanup console: pick any person and audit all of their faces, with no scan required. Reuses the existing tile grid, bulk-select-then-apply interaction, and verdict writes, so a manual decision is indistinguishable from a guided one. Server work is narrow: relax guard E15 for lock/unknown/detach (keeping stay scan-only, since it needs a suspected owner), add the eligibility check that snapshot membership was implicitly providing, and add an admin person-metadata endpoint for the name/ownerId the review page currently reads off the scan. * docs: revise manual face review spec after verification pass Three independent verification passes against the code disproved the first draft's central premise and five other claims. The big one: manual mode cannot reuse the guided review page behind a mode flag. FlaggedFace.suspectedOwnerId is required, every face defaults to the 'owner' state, and buildResolveRequest dereferences it - so an untouched manual review would POST moveToPerson with an undefined destination for every face. There is also no neutral state (six terminal states with a tested tally invariant), and the model is $derived over the face array, so paging would wipe staged decisions. The design now forks the review UI: a separate manual page under /people/[personId] with its own view-model (keep default, stable across appends, never emits stay), leaving all nine guided web specs untouched. Also corrected: lock is a PRIMARY KEY upsert with DO UPDATE that can steal a face from another identity (not an idempotent no-op via a plain unique index - that claim came from a stale comment referencing the long-gone insertLocks); FaceRepairRepository has no @GenerateSql so no mise sql regen; the navbar needs no repointing; resolveFaces has no unit coverage, only medium; and relaxing unknown converts one 400 into a success with unknown:0. * docs: chooser page design — equal-weight cards, first-visit and returning states Pierre's review of the chooser design: a first-time admin has never scanned, so the page needs a distinct first-visit presentation, and the two modes should carry equal weight since some admins will live in manual review rather than triaging scans. Equal-weight cards with no recommended badge. First visit gets an explanatory header and advertises that manual review needs no scan — it is fully usable on a brand-new instance, which the returning status-board layout would otherwise obscure. Adds the full card state matrix (never-scanned / running / flagged / zero-flagged / failed), including the manual card being disabled during a scan, which is the UI half of the resolveFaces 409 guard and prevents staging dozens of decisions only to lose them to a conflict. Slices renumbered 1-26 to cover the two chooser presentations and the remaining card states separately. * docs: manual review page design; consolidate 26 slices into 12 Folds the manual review page design into 6.4. The key point is a visual inversion that falls out of the data model: guided stamps every tile because every face holds one of six terminal states, while manual defaults to keep, so tiles start clean and colour appears only where the admin acted. keep needs no colour token - absence signals it, which satisfies the existing rule that state is never encoded in colour alone. Two interaction problems get resolved. Selection cannot claim the whole cluster over server paging, so it always means loaded faces (Select all loaded (N), showing N of M) and whole-cluster work goes through the server-enumerated entireCluster with its own destination picker. And a keep default needs an undo, so Unmark returns a selection to keep - guided never needed one because every face is already stamped. Slices consolidated 26 -> 12 on one test: can it ship alone without leaving the tree broken or dead? The route move now carries its own e2e repair (the move alone leaves CI red), the endpoint carries its SDK regen (no caller otherwise), the eligibility read joins the lock slice it exists for, and the chooser's three slices collapse into one component with one state machine. The pure view-model stays separate despite being small - highest logic risk on the branch. * feat(server): allow locking any eligible face on a person, not only flagged ones Manual review needs to lock a face that no scan flagged, so lock is dropped from the E15 flagged-snapshot gate. stay keeps that gate: it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion, so without a snapshot row there is no owner to record against. Lifting the gate for lock is not merely permissive - it removes a live guard. getScanFlaggedFacesForPersons INNER JOINs asset_face and re-validates personId/deletedAt/isVisible at snapshot-read time, so today a foreign or deleted id can never reach the write path. Dropping the gate without a replacement was verified to be exploitable: with lock removed and no eligibility check, resolveFaces returned locked:1 for a face on another person, for another USER's person, and for a soft-deleted face - silently re-pointing them onto this person's identity through replaceFaceIdentities' unscoped ON CONFLICT DO UPDATE. A nonexistent id crashed with an unhandled FK violation rather than a 400. getEligibleFaceIdsForPerson replaces that guard explicitly, mirroring getClusterFacePage's predicate so lockable == listed on the manual review page. It is advisory - the write-time guards remain authoritative - and exists so an inapplicable lock is a 400 rather than a silent no-op or a crash. Tests: 66/66 in the resolve medium spec, incl. the cross-tenant arm. * feat(server): allow detach and unknown on any face of a person, not only flagged ones Completes the E15 relaxation: stay is now the ONLY snapshot-gated bucket. It keeps that gate because it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion - with no snapshot row that yields undefined and a 500/FK violation. Unlike lock, detach and unknown need no eligibility read: both are already person-scoped at the write layer (detachFaces filters personId and keys its identity-strip on the RETURNING output; reattributeFaces filters still-on-source). That claim is now pinned by test rather than trusted - a foreign face passed to detach is an inert no-op, left completely untouched. Behaviour change (spec 5.4): a stale unknown id - a face that left this person since the scan - previously 400'd via the gate. It now returns success with unknown: 0, truthfully reporting zero parks, and the freshly created cluster is cleaned up rather than orphaned. Tests: 72/72 in the resolve medium spec, 6/6 cross-flow. * feat(server): add admin person metadata endpoint for manual face review GET admin/face-repair/person/:personId returns {id,name,ownerId,faceCount,thumbnailFaceId}, admin-gated, 404 on unknown. The manual review page cannot get these from anywhere else. The guided page reads personName and ownerId off getLatestScan(), and ownerId is what scopes the move-picker - so with no scan the picker cannot open at all. The user-scoped GET /people/:id does not admin-bypass for a person the admin does not own, the same gap already fixed once for face thumbnails. faceCount reuses searchOwnerPeople's exact join conditions so the count agrees between the browser grid and the review page header; a mismatch there would read as a bug. The name is returned raw - an unnamed person yields an empty name and the client applies its own fallback rather than the server inventing a display string. Ships with the client regen because the endpoint has no caller until then. No @GenerateSql was added (this repository carries none) so no mise sql regeneration; mise.lock verified unchanged. Tests: controller 76/76, metadata medium 6/6, tsc clean. * refactor(web): move the guided face-cleanup dashboard to /admin/face-cleanup/scan Frees /admin/face-cleanup for the mode chooser. Pure relocation of the ten-file dashboard cluster via git mv - no behaviour change - plus a temporary 307 redirect at the old path so this lands without a dead entry point. The chooser replaces that redirect next. The navbar is deliberately NOT repointed: NavbarItem highlights via pathname.startsWith(href), so pointing at /admin/face-cleanup keeps it active across /scan, /people and the chooser. Repointing it would be the regression, not the fix. Four e2e assertions broke on the move. Three are repoints; Playwright globs match the whole URL, so '**/admin/face-cleanup' does not match /scan. The fourth needed a real fix rather than a repoint: the post-apply drain check asserts a person's name is ABSENT, which passes vacuously on any page that never lists people - or on one that simply has not finished loading. It now waits for the Eligible faces stat, which renders unconditionally once a scan is completed and is therefore independent of the drained-vs-not condition under test. Tests: 117/117 across the nine guided specs from their new locations, tsc clean. * feat(web): add the face-cleanup mode chooser Replaces slice 4's temporary redirect. Two EQUAL-WEIGHT cards with no recommended badge on either: we do not know which mode a given admin lives in, and privileging guided would penalise anyone who spends their time in manual review. Two distinct presentations rather than one degraded into the other. First visit reframes the difference as readiness, not importance - guided says it needs a scan first, manual advertises that it needs no scan and can start right away. That line matters: manual review is fully functional on a brand-new instance, and the returning status-board layout would otherwise hide that behind an empty guided card, leaving a first-time admin thinking the feature does nothing until a scan finishes. The manual card is disabled while a scan runs. This is the UI half of resolveFaces' 409 guard: without it an admin can stage dozens of decisions across a server-paged cluster and lose them all on Apply. Disabled means genuinely unactivatable - no href and a real <button disabled>, so keyboard cannot reach it either - not just opacity. Only pending/running block it; failed and zero-flagged leave manual reachable. Visual vocabulary is reused verbatim from the dashboard - card shell, dot+label+tabular-nums stat rhythm, semantic colours - so this reads as the same product rather than a bolted-on page. Tests: 13 new chooser cases, 130/130 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-cleanup people browser Builds the destination the chooser's manual card links to. Zero new server endpoints: the owner list reuses searchUsersAdmin (the same call the guided dashboard already makes) and the people grid reuses getFaceRepairOwnerPeople, whose query parameter is optional - so omitting it lists all of an owner's people, paginated. Face crops go through the admin-gated, join-free face-thumbnail route, NOT the user-scoped /people/:id/thumbnail, which 404s for people the admin does not own. A null thumbnailFaceId renders a real placeholder rather than a broken image. Switching owner resets the page and clears the list, with a request token guarding against a stale response clobbering a newer selection - otherwise rows from two owners interleave. Pagination appends rather than replaces. Four states are kept distinct: this owner has no people, no results for this query, load failed, and no users at all. Conflating empty with error was defect D17 on the guided page. No client-side filtering is applied to what the endpoint returns; that passthrough is pinned by test so a later server-side change surfaces here rather than being silently masked. Tests: 14 new cases, 144/144 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review view-model A separate pure model rather than a reuse of the guided one, because the guided model cannot express manual review's semantics and two of its properties are actively dangerous here. No neutral state: guided's FaceState is six TERMINAL states and every face initialises to 'owner', with buildResolveRequest dereferencing suspectedOwnerId for it. Pointed at a scan-free cluster an untouched review would POST a move for every face to an undefined destination. Manual defaults to keep, which produces no bucket entry at all, and an all-keep review builds to null rather than an empty request - so a disabled Apply cannot be bypassed and the server never sees the empty resolve it would 400. State survives paging: guided rebuilds its model from a $derived over the face array, so appending a page discards staged decisions. This model owns its list and exposes appendFaces, which preserves both states and selection and is idempotent on assetFaceId. Manual is server-paged, so that defect would have been hit on the first Load more. stay is hardcoded [] on every request - manual can never emit it, since it needs a suspected owner. Two @ts-expect-error guards make owner/stay a compile error rather than a convention; they are load-bearing, since tsc reports an unused directive if the type ever stops rejecting them. Colour/icon tokens are imported from the guided model so one glyph means one thing across both pages; keep deliberately has no token - it is signalled by absence. Tests: 26 cases, 170/170 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review page with server-paged cluster faces Opens any person with no scan in existence and lists ALL of their faces, via the scan-free cluster endpoint with an empty excludeFaceIds. Implements the visual inversion this page is built around: guided stamps every tile because every face holds one of six terminal states, so its grid is a wall of colour to audit. Manual defaults to keep, so a tile renders a clean crop with no badge and no ribbon, and colour appears only where the admin acted. keep needs no colour token - it is signalled by absence, which keeps the existing rule that state is never encoded in colour alone without inventing a seventh swatch. Selection is honest about server paging: it always means LOADED faces, labelled Select all loaded (N) with showing N of M in the header. An unqualified select-all would either lie about a cluster of thousands or force loading all of it; whole-cluster work goes through the server-enumerated entireCluster instead (slice 10). Load more appends through the model rather than re-creating it, so staged marks and selection survive - the defect that made reusing the guided model impossible, and the first thing a paged cluster would have hit. Person name and ownerId resolve from the URL param through the metadata endpoint rather than navigation state, so refresh and deep links work. Empty and load-error are kept distinct (conflating them was D17 on the guided page), and a person the scan flagged shows no flagged badging - manual ignores scan state entirely. Tests: 15 new cases, 185/185 across the face-cleanup suite, tsc clean. * feat(web): add manual review bulk actions and apply The footer dock: Move to person (via the reused PersonPicker, scoped by the ownerId the metadata endpoint supplies), Lock, Unknown, Not a face, and Unmark - plus the staged-work tally and Apply. Unmark exists because manual defaults to keep. Guided never needed one: every face there is already stamped, so the admin only ever swaps one stamp for another. Here marking is a deliberate act, so mis-marking needs reversing without discarding every other staged decision via Reset. stay and owner are not offered and can never be emitted - both need a suspected owner. The payload assertion pins stay: [] explicitly across mixed buckets. Apply is disabled while everything is keep, because buildResolveRequest returns null and an all-keep POST would be the empty resolve the server 400s. Not a face keeps guided's destructive confirm, and declining it posts nothing. A 409 - a scan starting mid-review - surfaces without discarding staged work. That conflict is what the chooser's disabled-manual card exists to prevent; if it happens anyway the page must not compound it by throwing away decisions staged across a paged cluster. Tests drive the real UI rather than the model. 16 new cases, 201/201 across the face-cleanup suite, guided's 73 specs untouched, tsc clean. * feat(web): add entire-cluster move and the manual actions help modal Move entire cluster uses the server-enumerated entireCluster rather than selection, which is the whole reason it belongs here: on a server-paged page selection can only ever cover LOADED faces, so whole-cluster work must not route through it. Unlike guided - where entireCluster rides the scan's suspected owner - manual has none, so it requires an explicit destination via PersonPicker, and it posts with no per-face buckets since the server rejects combining them. It carries its own confirm because it moves faces the admin has never seen, which is both the point and the risk. Manual gets its own help modal rather than extending guided's, whose names all six actions test is load-bearing and stays untouched. The modal leads with the thing most likely to confuse someone arriving from guided: keep writes nothing. It also states that Not a face is the irreversible one and sits beside Unknown, which means the opposite - bin this crop versus this is a real person I cannot name. Swatches are asserted against the tile tokens so an explanation ties back to the button and the tile it describes, and keep renders no swatch at all - absence is how the default is signalled. Tests: 21 new cases, 222/222 across the face-cleanup suite, guided's specs untouched, tsc clean. * test(e2e): cover the manual face-review flow and its cross-engine invariant Two tests. The first drives the whole manual path against a person with NO scan in existence: chooser to people browser to review page, listing all faces rather than a flagged subset, then move, lock and not-a-face through the real UI, asserting durable DB rows rather than UI text. The second is the point of the feature: a later scan must honour a manual decision exactly as it honours a guided one - the locked face is not re-flagged, the detached face stays gone, the moved face is not re-proposed. Two seeding traps handled. utils.createFace links faces with source='manual', which the verdict layer excludes from flagging, so every face here is downgraded to 'ml' or it would not behave like a machine-clustered one; and face_search rows are required because both the cluster listing and lock eligibility inner-join it. The cross-engine test seeds ONE multi-person scan rather than two sequential ones: two scans stamped the same now() make getLatestScan nondeterministic, which would let the invariant assertions pass vacuously instead of for the right reason. * docs: document guided and manual face cleanup modes Adds a Two ways to clean up comparison and a Manual review section to the admin face cleanup page. The two points worth documenting explicitly are the ones an admin cannot infer: that faces you leave alone are deliberately NOT recorded (marking everything you glanced at as human-verified would stop future scans flagging it and hide real mistakes, so Lock is the opt-in), and that Select all covers the faces currently loaded rather than the whole cluster - with Move entire cluster as the server-resolved alternative. Also notes that manual review is unavailable during a scan, so the behaviour reads as deliberate rather than broken. * fix(web): keep translation-key literal types in the manual review tally A Record<..., string> annotation widened each value to string, and $t only accepts known translation keys, so the tally lookup stopped type-checking. satisfies keeps the exhaustiveness check on the keys while preserving the literal value types. Caught only by CI's check-svelte: that task reports 0 FILES locally in this worktree - even when invoked through mise exactly as CI does - so neither tsc nor the local svelte-check surfaced it. * fix(web): build the manual tally translation key with a template literal satisfies was not enough: indexing an object of whole translation keys still yields string at the call site, and $t accepts a template-literal type but not string. Matches the guided dock's idiom exactly - $t(`admin.face_cleanup_review_tally_${...}`) - by mapping only the key SUFFIX, with move reusing guided's existing 'other' key rather than adding a near-duplicate. Only CI's check-svelte catches this; that task scans 0 files locally even when invoked through mise as CI does. * style(web): redesign the face-cleanup mode chooser Rework the two-door chooser at /admin/face-cleanup into a considered design: a 48px icon tile anchors each mode, guided keeps the primary accent while manual gets a distinct teal identity (neither marked recommended), and each card carries a tinted status well that recolours across all five scan states with the live count as a hero number. Actions become full-width and foot-aligned, with a soft corner glow, inset ring, and hover lift for depth. Logic, copy, i18n keys, data-testids and the genuinely-disabled manual guard (§7) are unchanged; all 13 chooser page-spec tests still pass, eslint/prettier/svelte-check clean. * feat(web): infinite-scroll the manual face-review lists Replace the "Load more" buttons on the manual people browser and the selected-person review grid with scroll-driven pagination, so both lists grow as the admin scrolls instead of dead-ending on a button. Adds a reusable InfiniteScrollSentinel primitive (a sentinel-only port of people-grid's IntersectionObserver + visibility-fallback logic; people-grid itself isn't a drop-in where items aren't keyed by `id`). The review grid keeps appending through vm.appendFaces, so staged marks and selection still survive each page load. * docs: spec the face-cleanup scan two-lane triage redesign * docs: sliced impl-loop plan for the scan two-lane redesign * feat(web): add exclusion-set selection model for scan triage * feat(web): add ConfidentLane with approve-all + spot-check exclude * feat(web): add ReviewFirstLane clickable review list * feat(web): two-lane triage scan page; drop checklist/table/filter/selection UI Compose ConfidentLane (approve-all + spot-check exclude) and ReviewFirstLane (clickable rows) in the scan page, driven by the exclusion-set triage model. Removes the 3-step checklist, 5 stat cards, filter tabs, and the selection bar; the two non-actionable totals become quiet footnotes. Deletes ScanChecklist, FaceCleanupTable, and the old face-cleanup selection model, and prunes their now-dead i18n keys. * docs: note the scan-redesign plan corrections applied during impl-loop * test(e2e): key the scan-drain wait off the header summary, not the removed stat card The two-lane scan redesign removed the "Eligible faces" stat card the X1 test used as its "completed snapshot loaded" signal; use the header summary line ("… flagged faces across … people"), which renders on any completed scan. * fix(web): tidy the review-lane rows — one primary reason pill, no overflow Collapse the wrapping multi-pill reason column to a single primary reason (bad-target wins, else the first) plus a "+N", narrow the flagged column, and share one absolute right-slot between the chevron (default) and the hover dismiss so the row can no longer widen past the container. * feat(web): enlarge the guided-review bulk action bar The routing buttons were text-xs with 13px icons on the dark dock and read as small/hidden. Enlarge them (text-sm, 16px icons, roomier padding, a defining inset ring, rounded-lg), tint the destructive "not a face" red to set it apart from the routine routes, and bump the selected-count and Clear. * docs: spec for face-cleanup console UX fixes (clickable auto-fix chips, review-lane alignment) * docs: implementation plan for face-cleanup console UX fixes * docs: add column-content regression guard to the face-cleanup UX plan * fix(web): align the review-lane columns and give them headings * style(web): prettier fallout on the review-lane changes * feat(web): make auto-fix spot-check chips open the per-cluster review page * fix(web): persist confident-lane spot-check exclusions across chip click-through
Deeds67
added a commit
that referenced
this pull request
Aug 10, 2026
…an (#838) * docs: design spec for manual face review mode Adds a second entry path to the admin face cleanup console: pick any person and audit all of their faces, with no scan required. Reuses the existing tile grid, bulk-select-then-apply interaction, and verdict writes, so a manual decision is indistinguishable from a guided one. Server work is narrow: relax guard E15 for lock/unknown/detach (keeping stay scan-only, since it needs a suspected owner), add the eligibility check that snapshot membership was implicitly providing, and add an admin person-metadata endpoint for the name/ownerId the review page currently reads off the scan. * docs: revise manual face review spec after verification pass Three independent verification passes against the code disproved the first draft's central premise and five other claims. The big one: manual mode cannot reuse the guided review page behind a mode flag. FlaggedFace.suspectedOwnerId is required, every face defaults to the 'owner' state, and buildResolveRequest dereferences it - so an untouched manual review would POST moveToPerson with an undefined destination for every face. There is also no neutral state (six terminal states with a tested tally invariant), and the model is $derived over the face array, so paging would wipe staged decisions. The design now forks the review UI: a separate manual page under /people/[personId] with its own view-model (keep default, stable across appends, never emits stay), leaving all nine guided web specs untouched. Also corrected: lock is a PRIMARY KEY upsert with DO UPDATE that can steal a face from another identity (not an idempotent no-op via a plain unique index - that claim came from a stale comment referencing the long-gone insertLocks); FaceRepairRepository has no @GenerateSql so no mise sql regen; the navbar needs no repointing; resolveFaces has no unit coverage, only medium; and relaxing unknown converts one 400 into a success with unknown:0. * docs: chooser page design — equal-weight cards, first-visit and returning states Pierre's review of the chooser design: a first-time admin has never scanned, so the page needs a distinct first-visit presentation, and the two modes should carry equal weight since some admins will live in manual review rather than triaging scans. Equal-weight cards with no recommended badge. First visit gets an explanatory header and advertises that manual review needs no scan — it is fully usable on a brand-new instance, which the returning status-board layout would otherwise obscure. Adds the full card state matrix (never-scanned / running / flagged / zero-flagged / failed), including the manual card being disabled during a scan, which is the UI half of the resolveFaces 409 guard and prevents staging dozens of decisions only to lose them to a conflict. Slices renumbered 1-26 to cover the two chooser presentations and the remaining card states separately. * docs: manual review page design; consolidate 26 slices into 12 Folds the manual review page design into 6.4. The key point is a visual inversion that falls out of the data model: guided stamps every tile because every face holds one of six terminal states, while manual defaults to keep, so tiles start clean and colour appears only where the admin acted. keep needs no colour token - absence signals it, which satisfies the existing rule that state is never encoded in colour alone. Two interaction problems get resolved. Selection cannot claim the whole cluster over server paging, so it always means loaded faces (Select all loaded (N), showing N of M) and whole-cluster work goes through the server-enumerated entireCluster with its own destination picker. And a keep default needs an undo, so Unmark returns a selection to keep - guided never needed one because every face is already stamped. Slices consolidated 26 -> 12 on one test: can it ship alone without leaving the tree broken or dead? The route move now carries its own e2e repair (the move alone leaves CI red), the endpoint carries its SDK regen (no caller otherwise), the eligibility read joins the lock slice it exists for, and the chooser's three slices collapse into one component with one state machine. The pure view-model stays separate despite being small - highest logic risk on the branch. * feat(server): allow locking any eligible face on a person, not only flagged ones Manual review needs to lock a face that no scan flagged, so lock is dropped from the E15 flagged-snapshot gate. stay keeps that gate: it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion, so without a snapshot row there is no owner to record against. Lifting the gate for lock is not merely permissive - it removes a live guard. getScanFlaggedFacesForPersons INNER JOINs asset_face and re-validates personId/deletedAt/isVisible at snapshot-read time, so today a foreign or deleted id can never reach the write path. Dropping the gate without a replacement was verified to be exploitable: with lock removed and no eligibility check, resolveFaces returned locked:1 for a face on another person, for another USER's person, and for a soft-deleted face - silently re-pointing them onto this person's identity through replaceFaceIdentities' unscoped ON CONFLICT DO UPDATE. A nonexistent id crashed with an unhandled FK violation rather than a 400. getEligibleFaceIdsForPerson replaces that guard explicitly, mirroring getClusterFacePage's predicate so lockable == listed on the manual review page. It is advisory - the write-time guards remain authoritative - and exists so an inapplicable lock is a 400 rather than a silent no-op or a crash. Tests: 66/66 in the resolve medium spec, incl. the cross-tenant arm. * feat(server): allow detach and unknown on any face of a person, not only flagged ones Completes the E15 relaxation: stay is now the ONLY snapshot-gated bucket. It keeps that gate because it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion - with no snapshot row that yields undefined and a 500/FK violation. Unlike lock, detach and unknown need no eligibility read: both are already person-scoped at the write layer (detachFaces filters personId and keys its identity-strip on the RETURNING output; reattributeFaces filters still-on-source). That claim is now pinned by test rather than trusted - a foreign face passed to detach is an inert no-op, left completely untouched. Behaviour change (spec 5.4): a stale unknown id - a face that left this person since the scan - previously 400'd via the gate. It now returns success with unknown: 0, truthfully reporting zero parks, and the freshly created cluster is cleaned up rather than orphaned. Tests: 72/72 in the resolve medium spec, 6/6 cross-flow. * feat(server): add admin person metadata endpoint for manual face review GET admin/face-repair/person/:personId returns {id,name,ownerId,faceCount,thumbnailFaceId}, admin-gated, 404 on unknown. The manual review page cannot get these from anywhere else. The guided page reads personName and ownerId off getLatestScan(), and ownerId is what scopes the move-picker - so with no scan the picker cannot open at all. The user-scoped GET /people/:id does not admin-bypass for a person the admin does not own, the same gap already fixed once for face thumbnails. faceCount reuses searchOwnerPeople's exact join conditions so the count agrees between the browser grid and the review page header; a mismatch there would read as a bug. The name is returned raw - an unnamed person yields an empty name and the client applies its own fallback rather than the server inventing a display string. Ships with the client regen because the endpoint has no caller until then. No @GenerateSql was added (this repository carries none) so no mise sql regeneration; mise.lock verified unchanged. Tests: controller 76/76, metadata medium 6/6, tsc clean. * refactor(web): move the guided face-cleanup dashboard to /admin/face-cleanup/scan Frees /admin/face-cleanup for the mode chooser. Pure relocation of the ten-file dashboard cluster via git mv - no behaviour change - plus a temporary 307 redirect at the old path so this lands without a dead entry point. The chooser replaces that redirect next. The navbar is deliberately NOT repointed: NavbarItem highlights via pathname.startsWith(href), so pointing at /admin/face-cleanup keeps it active across /scan, /people and the chooser. Repointing it would be the regression, not the fix. Four e2e assertions broke on the move. Three are repoints; Playwright globs match the whole URL, so '**/admin/face-cleanup' does not match /scan. The fourth needed a real fix rather than a repoint: the post-apply drain check asserts a person's name is ABSENT, which passes vacuously on any page that never lists people - or on one that simply has not finished loading. It now waits for the Eligible faces stat, which renders unconditionally once a scan is completed and is therefore independent of the drained-vs-not condition under test. Tests: 117/117 across the nine guided specs from their new locations, tsc clean. * feat(web): add the face-cleanup mode chooser Replaces slice 4's temporary redirect. Two EQUAL-WEIGHT cards with no recommended badge on either: we do not know which mode a given admin lives in, and privileging guided would penalise anyone who spends their time in manual review. Two distinct presentations rather than one degraded into the other. First visit reframes the difference as readiness, not importance - guided says it needs a scan first, manual advertises that it needs no scan and can start right away. That line matters: manual review is fully functional on a brand-new instance, and the returning status-board layout would otherwise hide that behind an empty guided card, leaving a first-time admin thinking the feature does nothing until a scan finishes. The manual card is disabled while a scan runs. This is the UI half of resolveFaces' 409 guard: without it an admin can stage dozens of decisions across a server-paged cluster and lose them all on Apply. Disabled means genuinely unactivatable - no href and a real <button disabled>, so keyboard cannot reach it either - not just opacity. Only pending/running block it; failed and zero-flagged leave manual reachable. Visual vocabulary is reused verbatim from the dashboard - card shell, dot+label+tabular-nums stat rhythm, semantic colours - so this reads as the same product rather than a bolted-on page. Tests: 13 new chooser cases, 130/130 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-cleanup people browser Builds the destination the chooser's manual card links to. Zero new server endpoints: the owner list reuses searchUsersAdmin (the same call the guided dashboard already makes) and the people grid reuses getFaceRepairOwnerPeople, whose query parameter is optional - so omitting it lists all of an owner's people, paginated. Face crops go through the admin-gated, join-free face-thumbnail route, NOT the user-scoped /people/:id/thumbnail, which 404s for people the admin does not own. A null thumbnailFaceId renders a real placeholder rather than a broken image. Switching owner resets the page and clears the list, with a request token guarding against a stale response clobbering a newer selection - otherwise rows from two owners interleave. Pagination appends rather than replaces. Four states are kept distinct: this owner has no people, no results for this query, load failed, and no users at all. Conflating empty with error was defect D17 on the guided page. No client-side filtering is applied to what the endpoint returns; that passthrough is pinned by test so a later server-side change surfaces here rather than being silently masked. Tests: 14 new cases, 144/144 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review view-model A separate pure model rather than a reuse of the guided one, because the guided model cannot express manual review's semantics and two of its properties are actively dangerous here. No neutral state: guided's FaceState is six TERMINAL states and every face initialises to 'owner', with buildResolveRequest dereferencing suspectedOwnerId for it. Pointed at a scan-free cluster an untouched review would POST a move for every face to an undefined destination. Manual defaults to keep, which produces no bucket entry at all, and an all-keep review builds to null rather than an empty request - so a disabled Apply cannot be bypassed and the server never sees the empty resolve it would 400. State survives paging: guided rebuilds its model from a $derived over the face array, so appending a page discards staged decisions. This model owns its list and exposes appendFaces, which preserves both states and selection and is idempotent on assetFaceId. Manual is server-paged, so that defect would have been hit on the first Load more. stay is hardcoded [] on every request - manual can never emit it, since it needs a suspected owner. Two @ts-expect-error guards make owner/stay a compile error rather than a convention; they are load-bearing, since tsc reports an unused directive if the type ever stops rejecting them. Colour/icon tokens are imported from the guided model so one glyph means one thing across both pages; keep deliberately has no token - it is signalled by absence. Tests: 26 cases, 170/170 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review page with server-paged cluster faces Opens any person with no scan in existence and lists ALL of their faces, via the scan-free cluster endpoint with an empty excludeFaceIds. Implements the visual inversion this page is built around: guided stamps every tile because every face holds one of six terminal states, so its grid is a wall of colour to audit. Manual defaults to keep, so a tile renders a clean crop with no badge and no ribbon, and colour appears only where the admin acted. keep needs no colour token - it is signalled by absence, which keeps the existing rule that state is never encoded in colour alone without inventing a seventh swatch. Selection is honest about server paging: it always means LOADED faces, labelled Select all loaded (N) with showing N of M in the header. An unqualified select-all would either lie about a cluster of thousands or force loading all of it; whole-cluster work goes through the server-enumerated entireCluster instead (slice 10). Load more appends through the model rather than re-creating it, so staged marks and selection survive - the defect that made reusing the guided model impossible, and the first thing a paged cluster would have hit. Person name and ownerId resolve from the URL param through the metadata endpoint rather than navigation state, so refresh and deep links work. Empty and load-error are kept distinct (conflating them was D17 on the guided page), and a person the scan flagged shows no flagged badging - manual ignores scan state entirely. Tests: 15 new cases, 185/185 across the face-cleanup suite, tsc clean. * feat(web): add manual review bulk actions and apply The footer dock: Move to person (via the reused PersonPicker, scoped by the ownerId the metadata endpoint supplies), Lock, Unknown, Not a face, and Unmark - plus the staged-work tally and Apply. Unmark exists because manual defaults to keep. Guided never needed one: every face there is already stamped, so the admin only ever swaps one stamp for another. Here marking is a deliberate act, so mis-marking needs reversing without discarding every other staged decision via Reset. stay and owner are not offered and can never be emitted - both need a suspected owner. The payload assertion pins stay: [] explicitly across mixed buckets. Apply is disabled while everything is keep, because buildResolveRequest returns null and an all-keep POST would be the empty resolve the server 400s. Not a face keeps guided's destructive confirm, and declining it posts nothing. A 409 - a scan starting mid-review - surfaces without discarding staged work. That conflict is what the chooser's disabled-manual card exists to prevent; if it happens anyway the page must not compound it by throwing away decisions staged across a paged cluster. Tests drive the real UI rather than the model. 16 new cases, 201/201 across the face-cleanup suite, guided's 73 specs untouched, tsc clean. * feat(web): add entire-cluster move and the manual actions help modal Move entire cluster uses the server-enumerated entireCluster rather than selection, which is the whole reason it belongs here: on a server-paged page selection can only ever cover LOADED faces, so whole-cluster work must not route through it. Unlike guided - where entireCluster rides the scan's suspected owner - manual has none, so it requires an explicit destination via PersonPicker, and it posts with no per-face buckets since the server rejects combining them. It carries its own confirm because it moves faces the admin has never seen, which is both the point and the risk. Manual gets its own help modal rather than extending guided's, whose names all six actions test is load-bearing and stays untouched. The modal leads with the thing most likely to confuse someone arriving from guided: keep writes nothing. It also states that Not a face is the irreversible one and sits beside Unknown, which means the opposite - bin this crop versus this is a real person I cannot name. Swatches are asserted against the tile tokens so an explanation ties back to the button and the tile it describes, and keep renders no swatch at all - absence is how the default is signalled. Tests: 21 new cases, 222/222 across the face-cleanup suite, guided's specs untouched, tsc clean. * test(e2e): cover the manual face-review flow and its cross-engine invariant Two tests. The first drives the whole manual path against a person with NO scan in existence: chooser to people browser to review page, listing all faces rather than a flagged subset, then move, lock and not-a-face through the real UI, asserting durable DB rows rather than UI text. The second is the point of the feature: a later scan must honour a manual decision exactly as it honours a guided one - the locked face is not re-flagged, the detached face stays gone, the moved face is not re-proposed. Two seeding traps handled. utils.createFace links faces with source='manual', which the verdict layer excludes from flagging, so every face here is downgraded to 'ml' or it would not behave like a machine-clustered one; and face_search rows are required because both the cluster listing and lock eligibility inner-join it. The cross-engine test seeds ONE multi-person scan rather than two sequential ones: two scans stamped the same now() make getLatestScan nondeterministic, which would let the invariant assertions pass vacuously instead of for the right reason. * docs: document guided and manual face cleanup modes Adds a Two ways to clean up comparison and a Manual review section to the admin face cleanup page. The two points worth documenting explicitly are the ones an admin cannot infer: that faces you leave alone are deliberately NOT recorded (marking everything you glanced at as human-verified would stop future scans flagging it and hide real mistakes, so Lock is the opt-in), and that Select all covers the faces currently loaded rather than the whole cluster - with Move entire cluster as the server-resolved alternative. Also notes that manual review is unavailable during a scan, so the behaviour reads as deliberate rather than broken. * fix(web): keep translation-key literal types in the manual review tally A Record<..., string> annotation widened each value to string, and $t only accepts known translation keys, so the tally lookup stopped type-checking. satisfies keeps the exhaustiveness check on the keys while preserving the literal value types. Caught only by CI's check-svelte: that task reports 0 FILES locally in this worktree - even when invoked through mise exactly as CI does - so neither tsc nor the local svelte-check surfaced it. * fix(web): build the manual tally translation key with a template literal satisfies was not enough: indexing an object of whole translation keys still yields string at the call site, and $t accepts a template-literal type but not string. Matches the guided dock's idiom exactly - $t(`admin.face_cleanup_review_tally_${...}`) - by mapping only the key SUFFIX, with move reusing guided's existing 'other' key rather than adding a near-duplicate. Only CI's check-svelte catches this; that task scans 0 files locally even when invoked through mise as CI does. * style(web): redesign the face-cleanup mode chooser Rework the two-door chooser at /admin/face-cleanup into a considered design: a 48px icon tile anchors each mode, guided keeps the primary accent while manual gets a distinct teal identity (neither marked recommended), and each card carries a tinted status well that recolours across all five scan states with the live count as a hero number. Actions become full-width and foot-aligned, with a soft corner glow, inset ring, and hover lift for depth. Logic, copy, i18n keys, data-testids and the genuinely-disabled manual guard (§7) are unchanged; all 13 chooser page-spec tests still pass, eslint/prettier/svelte-check clean. * feat(web): infinite-scroll the manual face-review lists Replace the "Load more" buttons on the manual people browser and the selected-person review grid with scroll-driven pagination, so both lists grow as the admin scrolls instead of dead-ending on a button. Adds a reusable InfiniteScrollSentinel primitive (a sentinel-only port of people-grid's IntersectionObserver + visibility-fallback logic; people-grid itself isn't a drop-in where items aren't keyed by `id`). The review grid keeps appending through vm.appendFaces, so staged marks and selection still survive each page load. * docs: spec the face-cleanup scan two-lane triage redesign * docs: sliced impl-loop plan for the scan two-lane redesign * feat(web): add exclusion-set selection model for scan triage * feat(web): add ConfidentLane with approve-all + spot-check exclude * feat(web): add ReviewFirstLane clickable review list * feat(web): two-lane triage scan page; drop checklist/table/filter/selection UI Compose ConfidentLane (approve-all + spot-check exclude) and ReviewFirstLane (clickable rows) in the scan page, driven by the exclusion-set triage model. Removes the 3-step checklist, 5 stat cards, filter tabs, and the selection bar; the two non-actionable totals become quiet footnotes. Deletes ScanChecklist, FaceCleanupTable, and the old face-cleanup selection model, and prunes their now-dead i18n keys. * docs: note the scan-redesign plan corrections applied during impl-loop * test(e2e): key the scan-drain wait off the header summary, not the removed stat card The two-lane scan redesign removed the "Eligible faces" stat card the X1 test used as its "completed snapshot loaded" signal; use the header summary line ("… flagged faces across … people"), which renders on any completed scan. * fix(web): tidy the review-lane rows — one primary reason pill, no overflow Collapse the wrapping multi-pill reason column to a single primary reason (bad-target wins, else the first) plus a "+N", narrow the flagged column, and share one absolute right-slot between the chevron (default) and the hover dismiss so the row can no longer widen past the container. * feat(web): enlarge the guided-review bulk action bar The routing buttons were text-xs with 13px icons on the dark dock and read as small/hidden. Enlarge them (text-sm, 16px icons, roomier padding, a defining inset ring, rounded-lg), tint the destructive "not a face" red to set it apart from the routine routes, and bump the selected-count and Clear. * docs: spec for face-cleanup console UX fixes (clickable auto-fix chips, review-lane alignment) * docs: implementation plan for face-cleanup console UX fixes * docs: add column-content regression guard to the face-cleanup UX plan * fix(web): align the review-lane columns and give them headings * style(web): prettier fallout on the review-lane changes * feat(web): make auto-fix spot-check chips open the per-cluster review page * fix(web): persist confident-lane spot-check exclusions across chip click-through
Deeds67
added a commit
that referenced
this pull request
Aug 12, 2026
…ict layer (#834) * feat(server): add person face-suggestion endpoints * feat: regenerate OpenAPI + SQL and add face-suggestion API e2e Regenerated TypeScript SDK and Dart client from the three new face-suggestion endpoints (GET /face-suggestions, POST /confirm, POST /dismiss). Added SQL query docs for markConfirmed and markDismissed. Added API e2e spec with 401/400/200/idempotency coverage for all three endpoints plus the read-gate (hidden person). * docs: add Phase 4 implementation plan for face-suggestion web UI * feat(web): add i18n strings for face-suggestion banner and review modal * phase 3 docs * feat(web): add getFaceCropTransform crop-math helper * feat(web): add localStorage snooze for face suggestions * feat(web): add client-side FaceCrop component for unassigned candidate faces * feat(web): add PersonSuggestionBanner with snooze * feat(web): add PersonSuggestionReviewModal guided review queue * feat(web): mount face-suggestion banner + review modal on person page * test(e2e): web E2E for face-suggestion banner, review confirm and snooze - Add PLAYWRIGHT_DB_PORT env var to playwright.config.ts and utils.ts so the e2e-web-dev Makefile targets can connect to the dev stack database on port 5432 instead of the e2e stack's 5435 - Seed person_face_suggestion rows via raw SQL (no ML required) - Verify banner renders count, review modal opens and confirms a face, and snooze persists the hidden state across reloads * docs: document face-suggestion review and the dismiss/auto-assign behavior * fix(build): enable deleteOutDir to prevent stale dist migration files * revert: restore deleteOutDir false * feat(web): add suggestionMaxDistance to facial recognition admin settings * docs: add Phase 5 (shared spaces) face-suggestion design * feat(server): support space-person face suggestion rows * feat(server): add space-scoped face embedding search * feat(server): add space-person suggestion scan queries * feat(server): add space-person suggestion repository methods * feat(server): add space-person suggestion scan jobs * feat(server): chain space face-suggestion scans after identity maintenance * feat(server): resolve space suggestions when space people merge * chore: added plan * docs: add phase 5b face suggestion plan * feat(server): add shared-space face suggestion DTOs * feat(server): guard space face suggestion actions * test(server): cover unshared space suggestion guard * fix(server): refine space face suggestion DTOs * test(server): add space face suggestion review edges * feat(server): read space face suggestions * fix(server): validate space suggestion person route * feat(server): confirm space face suggestions * test(server): fix space suggestion medium fixture * feat(server): add space face suggestion routes * test(server): cover space suggestion person param validation * chore(api): regenerate space face suggestion clients * fix(ci): cover space suggestion revert cleanup * docs: format face suggestion phase plans * fix(ci): align space suggestion schema artifacts * fix(ci): clean server formatting and schema override * fix(ci): clean web face suggestion checks * fix(ci): avoid unused banner dependency expression * feat(web): add shared-space face suggestion review * docs: format face suggestion phase 5c plan * fix(server): queue shared-space suggestion scans on naming * feat(server): add face suggestion maintenance job * fix(openapi): regenerate face suggestion job enums * fix(server): stabilize face suggestion embedding samples * fix(server): make face suggestion migration overrides idempotent * fix(server): update generated face embedding SQL * fix(server): clean dangling active queue entries * ci: allow test pre-job to read PR files * ci: allow docker metadata to read PRs * test: ignore paused jobs when waiting for queues * test(web): scope tag-disabled fetch assertion * test(e2e): use real video fixture for live photos * chore(server): log face identity backfill progress * fix(server): converge personal identity backfill * chore(server): demote face identity backfill progress logs * chore(server): satisfy face identity lint * docs: design face suggestion ignore action * docs: harden face suggestion ignore spec * docs: plan face suggestion ignore action * docs: tighten face suggestion ignore implementation plan * feat(server): add face suggestion intent statuses * feat(server): persist face suggestion rejection and ignore intents * chore(server): leave face suggestion SQL regen to CI * test(server): keep face suggestion migration legacy status literal typed * feat(server): add personal face suggestion reject and ignore APIs * feat(server): add shared-space face suggestion reject and ignore APIs * test(server): expect shared-space dismiss to reject suggestions * chore: regenerate face suggestion API clients * feat(web): add ignore action to face suggestion review * chore: cover face suggestion intent migration in revert script * chore(docs): format face suggestion planning docs * chore: refresh generated face suggestion artifacts * feat(i18n): add German and French face-suggestion translations Adds de/fr translations for the 19 face-suggestion strings the feature introduced in en.json (review modal, banner, admin job + ML setting, error toast), keeping en/de/fr at parity. # Conflicts: # i18n/de.json # i18n/fr.json * test(server): wire DatabaseRepository for space merge suggestion test mergeSpacePeople now delegates to IdentityMergePropagationService (rebased onto main), which needs databaseRepository.transaction; add DatabaseRepository to the medium test's real services so the merge-resolves-suggestions case runs. * fix(server): re-timestamp face-suggestion migrations after rebase 1779000000000 and 1779100000000 now collide with main's AddSharedSpaceAlbumUserTables / AddSharedSpaceAlbumCreateSideTriggers. Move the two space/intent face-suggestion migrations to 1784000000000 and 1784100000000, which are free above main's highest fork migration (1783700000000). Ordering relative to 1778900000000-AddPersonFaceSuggestion (which creates the table) is preserved. * fix(server): repair face-suggestion wiring after rebase + regenerate Dart client - base.service.ts: add personFaceSuggestionRepository to the static create() positional list (main introduced create() after this branch forked; the import and constructor sites were already present, the third was missing and would have shifted every later repository). - person.service.ts: main split asDateString into asDateTimeString (ISO datetime) and asDateString (yyyy-mm-dd). The suggestion fileCreatedAt is a date-time, so it now uses asDateTimeString. - mobile/openapi: regenerate the Dart client (the OpenAPI spec and the TypeScript SDK regenerate to a zero diff). * style(web): re-run prettier on face-suggestion specs after rebase * test(server): match main's validation error shape in face-suggestion controller specs Main replaced the flat `badRequest(['[field] message'])` body with `{ message: 'Validation failed', errors: [...] }` and the `errorDto.validationError([{ path, message }])` helper. Update the eight face-suggestion route assertions to the new shape. * fix(server): route space face-suggestion reads through the 3-path scope helper main's shared-space-album-scope guard (a static CI test) requires that every shared_space_library scoping arm carries the linked-album arm, and that every space asset read carries the visibility gate. Both landed after this branch forked, so the branch's space-scoped face reads were 2-path and ungated: - search.repository.ts searchFaces (space): now unions all three paths via spaceAssetPathBranches and applies spaceVisibilityGate, so album-linked and cross-owner contributed assets are candidates and other members' Hidden/Locked assets are not. - person-face-suggestion.repository.ts getPendingForSpacePerson / hasPendingForSpacePerson: same 3-path helper (these already gated visibility). - shared-space.repository.ts getScannableSpacePeopleWithUnassignedFaces: 3-path helper plus the visibility gate. Generated SQL regenerated (only these two query files change). Also: - web PersonSuggestionReviewModal: Faces moved to managers/asset-viewer-manager. - e2e face-suggestion spec: main's 400 body has no 'error' field; use errorDto. - medium person.service spec: SystemMetadataRepository was registered in BOTH real and mock after the merge, so getMock returned the real repository. * fix(migrations): renumber AddSpacePersonFaceSuggestion off the #770 collision at 1784000000000 * chore: regenerate OpenAPI clients for the unified face-review branch * fix(server): preserve manual face-identity links across backfill realign Slice 1 of the face-review unification proves the assumption the unified verdict layer rests on: face_identity_face.source='manual' is the durable record that a human placed a face on a person. It was not durable. realignFacesToPersonIdentity set source='backfill' unconditionally, so any face whose link identity had drifted from its person's identity lost the record - and drift is the normal state right after a people merge. Every human-confirmed face was one backfill pass away from silently becoming eligible for re-flagging again. Realigning which human a face links to is this method's job; erasing that a human placed it is not. Preserve 'manual', realign everything else as before. * feat(server): face_person_verdict — the shared face-review verdict layer Replaces person_face_suggestion with a table that holds both meanings the two face features need, under one uniqueness constraint per (target, face): status='pending' the QUEUE - a suggestion awaiting review status IN ('rejected','ignored') the VERDICT - a durable 'not that person' Keyed identity-first with the person/space-person target as a fallback, so one rejection answers the question in personal scope AND in every space, and survives a people merge without bespoke re-pointing machinery. There is no 'confirmed' status. The positive verdict lives in face_identity_face.source='manual', which every human reassignment already writes; storing it twice is the duplicate-fact problem this table removes. Confirm now just claims the queue row. Targets are ON DELETE SET NULL (not CASCADE) so an identity-keyed verdict outlives the person it was written against; the single-target check is '<= 1' rather than '= 1' precisely so that SET NULL cannot make a person DELETE fail. Three never-deployed fork migrations collapse into one final-form migration. * feat(server): unify both face engines on the shared verdict layer Slice 3 (shared exclusion predicates) + Slice 5 (cleanup rewiring, lock retired), landed together because they are one change to one seam. - The Face Cleanup in-memory filter (applyDeclineFilters -> applyVerdictFilters) now consults the same three facts both engines share: a human placement (face_identity_face.source='manual'), a negative verdict (face_person_verdict), and a not-a-face tombstone. Verdicts match identity-first with a person/space fallback, via opaque target tokens so an identity-keyed 'not Anna' and a person-keyed suspicion of Anna resolve to each other. - face_repair_lock is deleted. A human placement was already recorded by every reassignment as the manual identity link; the lock table was a second, weaker, non-merge-safe copy of that fact. Confirm/lock now re-affirms the link; an admin move (lock flag or not) settles the face the same way a user's confirmed suggestion does — which is what stops the two engines fighting over a face. - face_repair_decline is narrowed to console-local cluster mutes only; its face-level 'keep here' rows are now shared verdicts the suggestion engine can see. The merge re-pointing machinery both facts needed is gone: identity keys and ON DELETE SET NULL make them merge-safe by construction. - The resolutions page lists negative verdicts from BOTH engines (source + actor); un-confirming a placement is a per-person action that demotes the manual link back to 'ml'. Closes leaks 1, 3, 4, 5, 6 from the design; leak 2 and hazard 8 are Slice 6. * fix(server): exclude soft-deleted faces from search; clear manual links on bulk unassign Slice 6 — the two source-of-truth safety fixes. - searchFaces now filters asset_face.deletedAt (it previously filtered only asset.deletedAt). A 'not a face' tombstone must be honoured by every recognition/suggestion candidate, or the suggestion scan keeps writing pending rows for a crop an admin already declared not-a-face. - unassignFaces ('reset all people') now clears face_identity_face manual links in the same operation. Otherwise a library-wide reset leaves every previously-confirmed face carrying a stale human-placement record with no person behind it, permanently excluding it from both engines. * test(server): cross-flow integration + drain stale suggestions on cleanup move (leak 3) Slice 7 — drives BOTH face engines against one database and proves each of the original leaks is closed end-to-end, using a real embedding KNN cleanup scan so 'not flagged' means the shared verdict layer actually excluded the face: leak 1 confirm a suggestion -> cleanup scan does not re-flag it leak 2 detach 'not a face' -> suggestion candidate search never returns it leak 3 cleanup move -> no stale pending suggestion survives leak 4/5 user rejects F=Bob -> cleanup no longer flags F toward Bob Closing leak 3 required a write-path drain: executeRepair (batch) and resolveFaces (interactive console) now both call facePersonVerdictRepository.drainPendingForFaces for every reassigned/detached face, so the never-reappear guarantee no longer rests on the suggestion read's personId IS NULL filter — which would let a stale row resurface if the face were later unassigned. * feat(web): resolutions page lists negative verdicts from both engines; add un-confirm API Slice 8 (partial) — the admin manage surface for the unified verdict layer. - /admin/face-cleanup/resolutions is now a single list of NEGATIVE verdicts ('this face is not that person') from BOTH engines, with a source filter (admin cleanup vs user review), an actor column, and the space name for space-person verdicts. The locks section is gone: human placements are no longer rows, so listing them globally would be unbounded and meaningless. - New POST /admin/face-repair/unconfirm downgrades a human placement's identity link from 'manual' back to 'ml' so a re-scan may flag the face again. This is the admin's answer to 'why did this face vanish from my queue' and will back the per-person 'skipped: human-confirmed' review-page section (deferred follow-up). Regenerated the TS SDK and Dart client for the new resolution-item shape, the verdictIds/clusterMuteIds remove request, and the un-confirm endpoint. * test(e2e): update face-cleanup specs for the unified verdict model - Resolutions page empty-state text and negative-verdict rows (source filter) replace the declines/locks sections. - Lock-undo test becomes an un-confirm test through POST /admin/face-repair/unconfirm. - Consistency specs assert the human placement as face_identity_face.source='manual' rather than a face_repair_lock row, and its merge-survival via the identity key. - Suggestion e2e SQL seeds target the renamed face_person_verdict table. * style: prettier on the hand-edited controller and e2e seed rename * test(server): drop unused repo locals flagged by --max-warnings 0 * test(e2e): assert verdict/placement recovery via durable DB state, not a re-seeded scan The keep-here and un-confirm e2e tests asserted re-flagging by seeding a SECOND completed scan and reading getPersonFlaggedFaces — but two scans stamped now() make getLatestScan's ordering nondeterministic, so the drained first scan could win and the face would read as still-not-flagged. The re-scan-re-flags semantics are covered deterministically by the medium tests (face-repair.resolutions + face-review-cross-flow); the e2e now asserts the durable state the page's Undo / the un-confirm endpoint actually produced (verdict row deleted; manual link downgraded to 'ml'). * test(e2e): run the new keep-here scan-seeding test last in the serial suite X1 (unmodified from #770) was silently SKIPPED in earlier runs because Playwright serial-skips tests after a failure, so it never actually validated. Once keep-here passed, X1 ran and failed with 0 flagged tiles: my keep-here test seeded a completed scan BEFORE X1, which #770 never did (its pre-X1 test seeded a decline, not a scan). The server handles this correctly (proven by a medium repro), but restoring #770's order — X1 as the first scan-seeder, my new test last — removes the cross-test scan-state coupling in the e2e stack. * test(e2e): seed face-cleanup faces as ML-sourced, not manual ROOT CAUSE of the X1 (and, via serial-skip, every other) face-cleanup e2e failure: utils.createFace links each face with face_identity_face.source='manual' (its shortcut for a full face->identity link). The unified verdict layer correctly treats source='manual' as a HUMAN placement and excludes such faces from cleanup flagging — so getPersonFlaggedFaces returned 0 and the review page showed 'No flagged faces' (confirmed from the Playwright DOM snapshot). A face a scan flags is by definition an ML attribution, never a human placement (production only writes 'manual' on explicit human actions — face-editor reassign, cleanup move/lock, confirm suggestion, merge). seedFlaggedScan now downgrades its faces to source='ml' so they represent what a real scan flags. Post-action assertions ('manual' after lock/move) are unaffected. Verified with a medium repro: manual-linked flagged face -> 0 returned; ml-linked -> returned. * test(e2e): preserve manual source on durability re-seeds The source='ml' downgrade must NOT apply to the Consistency X1/X2 durability re-seeds: those simulate a later scan re-proposing a face that a prior move-and-lock (X1) or lock+merge (X2) legitimately set to source='manual'. The test asserts that placement keeps the face out of the review — downgrading it to 'ml' re-flagged it and broke the assertion. Add a preserveSource opt-out and set it on the two re-seeds (personId=other / mergeTarget, after the human action). * test(e2e): run keep-here test before X1 so it is the only resolution row With source='ml' fixing the real regression, the earlier reorder (keep-here last) is not only unnecessary but harmful: X1's 'stay' action leaves a persistent cleanup verdict, so keep-here — running last — saw 2 rows on the global resolutions page and its 'toHaveCount(0) after undo' found 1 left. Restore keep-here to before X1, where no prior test has created a verdict, so it is the sole resolution row and the undo drains the list to empty. * chore(docs): spec the face verdict layer remediation * fix(server): re-key face verdicts through merges instead of cascading them away * fix(server): user face verdicts carry identity and actor; space rejects never no-op * feat(server): suggestion engine consults the shared verdict layer * fix(server): manual face placements survive merges, races, and backfill sweeps * test(server): stub addPersonFaces in space-confirm unit spec (slice 3 follow-up) * fix(server): byte-match face_person_verdict index overrides; drift gate covers everything * fix(server): owner-only personal suggestion reads; pending queue honours asset state * feat(server+web): admin face thumbnails; cleanup surfaces render non-owned clusters * fix(web): suggestion modal paging/error truthfulness; admin pages fail loudly Closes D8 (the suggestion modal walked fixed page offsets over a server list its own actions drain, closing "complete" with roughly half the queue unseen; act()'s bare catch made 500s indistinguishable from success) and D17 (snooze baseline never rebased and was keyed per-person instead of per-user; modal back-nav re-enabled already-acted rows; a failed initial load on the three admin face-cleanup pages rendered as a reassuring empty state; the console's bulk-apply skipped its refetch on partial failure). The modal now always refetches page 1 (acted rows drain server-side, so the head of the list is the only stable cursor), keeps a client actedFaceIds set to skip just-acted rows the server hasn't settled yet and to render them read-only on back-nav, and closes only once a fresh fetch confirms nothing is left. act() surfaces every error via handleError except the server's one documented already-resolved outcome (HTTP 400), which advances silently without inflating the confirmed count. Snooze entries are now keyed by userId:personId and rebase their baseline down on every read, so a genuinely new suggestion resurfaces even below the original snoozed count. The three admin pages gained a distinct loadError state (existing red-banner idiom, with Retry) ahead of their empty-state branches, and the console's fetchLatestScan() moved into handleApply's finally so the table always reflects what the server actually did. * fix(server): transactional confirm and drains; live dashboard counts * test: close the verdict-layer coverage gaps found in review Slice 10 Tasks 1-3: suite hardening (D15), hygiene (D16), and parent-doc corrections for the face-review-unification remediation. Suite hardening: - Remove the `.catch(() => {})` swallow around confirmFaceSuggestion in the cross-flow spec; it passes green against the real DB unaided. - Pin the getPendingForPerson band predicate at its exact boundaries: distance == maxDistance (excluded) and distance == suggestionMaxDistance (included). - Assert a cleanup keep-here verdict stores both identityId and actorId when the suspected owner already has an established identity. - Add space-person and fully-orphaned (SET-NULL-target) fixtures to the resolutions listing at both the medium-service and web-page layers. - Fix the FK test that claimed CASCADE but asserted nothing distinguishing it from SET NULL; it now deletes the identity and asserts the verdict row survives, queried by its own id, with personId nulled. - Scope the face-cleanup e2e keep-here assertion to the row the test itself created so `.serial` order no longer matters. Hygiene: - Retitle two stale test names that referenced the retired face_repair_lock/face_repair_decline tables. - Fix the admin resolutions endpoint summary to describe what it actually lists. - Fix the four mark*/mark*ForSpacePerson @GenerateSql param objects, which were single malformed object args instead of positional (id, id, opts) triples — the malformed shape silently dropped assetFaceId from the generated SQL docs. - Delete 19 orphaned admin.face_cleanup_* keys from de.json/fr.json, absent from en.json and unreferenced in web/src, mobile/lib, e2e/src, server/src. - Delete the dead private findOrCreateSpacePersonForFace in shared-space.service.ts (zero callers; only forwarded to findOrCreateCompatibleSpacePersonForIdentity). Parent-doc corrections: append a "2026-07-23 corrections" note to the design doc covering the unshipped face_repair_cluster_mute rename, the omitted sixth fork migration, the two never-written coverage-matrix rows, and the false merge-safety claim fixed by this remediation's own Slice 1. * chore: regenerate openapi clients + sql docs for the verdict layer; prettier slice plans * style(server): prettier the slice-3 follow-up + slice-10 hardening specs * test(e2e): poll job queues with the admin token in the second-user face-cleanup test * feat: manual face review mode — audit any person's faces without a scan (#838) * docs: design spec for manual face review mode Adds a second entry path to the admin face cleanup console: pick any person and audit all of their faces, with no scan required. Reuses the existing tile grid, bulk-select-then-apply interaction, and verdict writes, so a manual decision is indistinguishable from a guided one. Server work is narrow: relax guard E15 for lock/unknown/detach (keeping stay scan-only, since it needs a suspected owner), add the eligibility check that snapshot membership was implicitly providing, and add an admin person-metadata endpoint for the name/ownerId the review page currently reads off the scan. * docs: revise manual face review spec after verification pass Three independent verification passes against the code disproved the first draft's central premise and five other claims. The big one: manual mode cannot reuse the guided review page behind a mode flag. FlaggedFace.suspectedOwnerId is required, every face defaults to the 'owner' state, and buildResolveRequest dereferences it - so an untouched manual review would POST moveToPerson with an undefined destination for every face. There is also no neutral state (six terminal states with a tested tally invariant), and the model is $derived over the face array, so paging would wipe staged decisions. The design now forks the review UI: a separate manual page under /people/[personId] with its own view-model (keep default, stable across appends, never emits stay), leaving all nine guided web specs untouched. Also corrected: lock is a PRIMARY KEY upsert with DO UPDATE that can steal a face from another identity (not an idempotent no-op via a plain unique index - that claim came from a stale comment referencing the long-gone insertLocks); FaceRepairRepository has no @GenerateSql so no mise sql regen; the navbar needs no repointing; resolveFaces has no unit coverage, only medium; and relaxing unknown converts one 400 into a success with unknown:0. * docs: chooser page design — equal-weight cards, first-visit and returning states Pierre's review of the chooser design: a first-time admin has never scanned, so the page needs a distinct first-visit presentation, and the two modes should carry equal weight since some admins will live in manual review rather than triaging scans. Equal-weight cards with no recommended badge. First visit gets an explanatory header and advertises that manual review needs no scan — it is fully usable on a brand-new instance, which the returning status-board layout would otherwise obscure. Adds the full card state matrix (never-scanned / running / flagged / zero-flagged / failed), including the manual card being disabled during a scan, which is the UI half of the resolveFaces 409 guard and prevents staging dozens of decisions only to lose them to a conflict. Slices renumbered 1-26 to cover the two chooser presentations and the remaining card states separately. * docs: manual review page design; consolidate 26 slices into 12 Folds the manual review page design into 6.4. The key point is a visual inversion that falls out of the data model: guided stamps every tile because every face holds one of six terminal states, while manual defaults to keep, so tiles start clean and colour appears only where the admin acted. keep needs no colour token - absence signals it, which satisfies the existing rule that state is never encoded in colour alone. Two interaction problems get resolved. Selection cannot claim the whole cluster over server paging, so it always means loaded faces (Select all loaded (N), showing N of M) and whole-cluster work goes through the server-enumerated entireCluster with its own destination picker. And a keep default needs an undo, so Unmark returns a selection to keep - guided never needed one because every face is already stamped. Slices consolidated 26 -> 12 on one test: can it ship alone without leaving the tree broken or dead? The route move now carries its own e2e repair (the move alone leaves CI red), the endpoint carries its SDK regen (no caller otherwise), the eligibility read joins the lock slice it exists for, and the chooser's three slices collapse into one component with one state machine. The pure view-model stays separate despite being small - highest logic risk on the branch. * feat(server): allow locking any eligible face on a person, not only flagged ones Manual review needs to lock a face that no scan flagged, so lock is dropped from the E15 flagged-snapshot gate. stay keeps that gate: it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion, so without a snapshot row there is no owner to record against. Lifting the gate for lock is not merely permissive - it removes a live guard. getScanFlaggedFacesForPersons INNER JOINs asset_face and re-validates personId/deletedAt/isVisible at snapshot-read time, so today a foreign or deleted id can never reach the write path. Dropping the gate without a replacement was verified to be exploitable: with lock removed and no eligibility check, resolveFaces returned locked:1 for a face on another person, for another USER's person, and for a soft-deleted face - silently re-pointing them onto this person's identity through replaceFaceIdentities' unscoped ON CONFLICT DO UPDATE. A nonexistent id crashed with an unhandled FK violation rather than a 400. getEligibleFaceIdsForPerson replaces that guard explicitly, mirroring getClusterFacePage's predicate so lockable == listed on the manual review page. It is advisory - the write-time guards remain authoritative - and exists so an inapplicable lock is a 400 rather than a silent no-op or a crash. Tests: 66/66 in the resolve medium spec, incl. the cross-tenant arm. * feat(server): allow detach and unknown on any face of a person, not only flagged ones Completes the E15 relaxation: stay is now the ONLY snapshot-gated bucket. It keeps that gate because it writes a negative verdict against the face's SUSPECTED owner, read from the snapshot via a non-null assertion - with no snapshot row that yields undefined and a 500/FK violation. Unlike lock, detach and unknown need no eligibility read: both are already person-scoped at the write layer (detachFaces filters personId and keys its identity-strip on the RETURNING output; reattributeFaces filters still-on-source). That claim is now pinned by test rather than trusted - a foreign face passed to detach is an inert no-op, left completely untouched. Behaviour change (spec 5.4): a stale unknown id - a face that left this person since the scan - previously 400'd via the gate. It now returns success with unknown: 0, truthfully reporting zero parks, and the freshly created cluster is cleaned up rather than orphaned. Tests: 72/72 in the resolve medium spec, 6/6 cross-flow. * feat(server): add admin person metadata endpoint for manual face review GET admin/face-repair/person/:personId returns {id,name,ownerId,faceCount,thumbnailFaceId}, admin-gated, 404 on unknown. The manual review page cannot get these from anywhere else. The guided page reads personName and ownerId off getLatestScan(), and ownerId is what scopes the move-picker - so with no scan the picker cannot open at all. The user-scoped GET /people/:id does not admin-bypass for a person the admin does not own, the same gap already fixed once for face thumbnails. faceCount reuses searchOwnerPeople's exact join conditions so the count agrees between the browser grid and the review page header; a mismatch there would read as a bug. The name is returned raw - an unnamed person yields an empty name and the client applies its own fallback rather than the server inventing a display string. Ships with the client regen because the endpoint has no caller until then. No @GenerateSql was added (this repository carries none) so no mise sql regeneration; mise.lock verified unchanged. Tests: controller 76/76, metadata medium 6/6, tsc clean. * refactor(web): move the guided face-cleanup dashboard to /admin/face-cleanup/scan Frees /admin/face-cleanup for the mode chooser. Pure relocation of the ten-file dashboard cluster via git mv - no behaviour change - plus a temporary 307 redirect at the old path so this lands without a dead entry point. The chooser replaces that redirect next. The navbar is deliberately NOT repointed: NavbarItem highlights via pathname.startsWith(href), so pointing at /admin/face-cleanup keeps it active across /scan, /people and the chooser. Repointing it would be the regression, not the fix. Four e2e assertions broke on the move. Three are repoints; Playwright globs match the whole URL, so '**/admin/face-cleanup' does not match /scan. The fourth needed a real fix rather than a repoint: the post-apply drain check asserts a person's name is ABSENT, which passes vacuously on any page that never lists people - or on one that simply has not finished loading. It now waits for the Eligible faces stat, which renders unconditionally once a scan is completed and is therefore independent of the drained-vs-not condition under test. Tests: 117/117 across the nine guided specs from their new locations, tsc clean. * feat(web): add the face-cleanup mode chooser Replaces slice 4's temporary redirect. Two EQUAL-WEIGHT cards with no recommended badge on either: we do not know which mode a given admin lives in, and privileging guided would penalise anyone who spends their time in manual review. Two distinct presentations rather than one degraded into the other. First visit reframes the difference as readiness, not importance - guided says it needs a scan first, manual advertises that it needs no scan and can start right away. That line matters: manual review is fully functional on a brand-new instance, and the returning status-board layout would otherwise hide that behind an empty guided card, leaving a first-time admin thinking the feature does nothing until a scan finishes. The manual card is disabled while a scan runs. This is the UI half of resolveFaces' 409 guard: without it an admin can stage dozens of decisions across a server-paged cluster and lose them all on Apply. Disabled means genuinely unactivatable - no href and a real <button disabled>, so keyboard cannot reach it either - not just opacity. Only pending/running block it; failed and zero-flagged leave manual reachable. Visual vocabulary is reused verbatim from the dashboard - card shell, dot+label+tabular-nums stat rhythm, semantic colours - so this reads as the same product rather than a bolted-on page. Tests: 13 new chooser cases, 130/130 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-cleanup people browser Builds the destination the chooser's manual card links to. Zero new server endpoints: the owner list reuses searchUsersAdmin (the same call the guided dashboard already makes) and the people grid reuses getFaceRepairOwnerPeople, whose query parameter is optional - so omitting it lists all of an owner's people, paginated. Face crops go through the admin-gated, join-free face-thumbnail route, NOT the user-scoped /people/:id/thumbnail, which 404s for people the admin does not own. A null thumbnailFaceId renders a real placeholder rather than a broken image. Switching owner resets the page and clears the list, with a request token guarding against a stale response clobbering a newer selection - otherwise rows from two owners interleave. Pagination appends rather than replaces. Four states are kept distinct: this owner has no people, no results for this query, load failed, and no users at all. Conflating empty with error was defect D17 on the guided page. No client-side filtering is applied to what the endpoint returns; that passthrough is pinned by test so a later server-side change surfaces here rather than being silently masked. Tests: 14 new cases, 144/144 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review view-model A separate pure model rather than a reuse of the guided one, because the guided model cannot express manual review's semantics and two of its properties are actively dangerous here. No neutral state: guided's FaceState is six TERMINAL states and every face initialises to 'owner', with buildResolveRequest dereferencing suspectedOwnerId for it. Pointed at a scan-free cluster an untouched review would POST a move for every face to an undefined destination. Manual defaults to keep, which produces no bucket entry at all, and an all-keep review builds to null rather than an empty request - so a disabled Apply cannot be bypassed and the server never sees the empty resolve it would 400. State survives paging: guided rebuilds its model from a $derived over the face array, so appending a page discards staged decisions. This model owns its list and exposes appendFaces, which preserves both states and selection and is idempotent on assetFaceId. Manual is server-paged, so that defect would have been hit on the first Load more. stay is hardcoded [] on every request - manual can never emit it, since it needs a suspected owner. Two @ts-expect-error guards make owner/stay a compile error rather than a convention; they are load-bearing, since tsc reports an unused directive if the type ever stops rejecting them. Colour/icon tokens are imported from the guided model so one glyph means one thing across both pages; keep deliberately has no token - it is signalled by absence. Tests: 26 cases, 170/170 across the face-cleanup suite, tsc clean. * feat(web): add the manual face-review page with server-paged cluster faces Opens any person with no scan in existence and lists ALL of their faces, via the scan-free cluster endpoint with an empty excludeFaceIds. Implements the visual inversion this page is built around: guided stamps every tile because every face holds one of six terminal states, so its grid is a wall of colour to audit. Manual defaults to keep, so a tile renders a clean crop with no badge and no ribbon, and colour appears only where the admin acted. keep needs no colour token - it is signalled by absence, which keeps the existing rule that state is never encoded in colour alone without inventing a seventh swatch. Selection is honest about server paging: it always means LOADED faces, labelled Select all loaded (N) with showing N of M in the header. An unqualified select-all would either lie about a cluster of thousands or force loading all of it; whole-cluster work goes through the server-enumerated entireCluster instead (slice 10). Load more appends through the model rather than re-creating it, so staged marks and selection survive - the defect that made reusing the guided model impossible, and the first thing a paged cluster would have hit. Person name and ownerId resolve from the URL param through the metadata endpoint rather than navigation state, so refresh and deep links work. Empty and load-error are kept distinct (conflating them was D17 on the guided page), and a person the scan flagged shows no flagged badging - manual ignores scan state entirely. Tests: 15 new cases, 185/185 across the face-cleanup suite, tsc clean. * feat(web): add manual review bulk actions and apply The footer dock: Move to person (via the reused PersonPicker, scoped by the ownerId the metadata endpoint supplies), Lock, Unknown, Not a face, and Unmark - plus the staged-work tally and Apply. Unmark exists because manual defaults to keep. Guided never needed one: every face there is already stamped, so the admin only ever swaps one stamp for another. Here marking is a deliberate act, so mis-marking needs reversing without discarding every other staged decision via Reset. stay and owner are not offered and can never be emitted - both need a suspected owner. The payload assertion pins stay: [] explicitly across mixed buckets. Apply is disabled while everything is keep, because buildResolveRequest returns null and an all-keep POST would be the empty resolve the server 400s. Not a face keeps guided's destructive confirm, and declining it posts nothing. A 409 - a scan starting mid-review - surfaces without discarding staged work. That conflict is what the chooser's disabled-manual card exists to prevent; if it happens anyway the page must not compound it by throwing away decisions staged across a paged cluster. Tests drive the real UI rather than the model. 16 new cases, 201/201 across the face-cleanup suite, guided's 73 specs untouched, tsc clean. * feat(web): add entire-cluster move and the manual actions help modal Move entire cluster uses the server-enumerated entireCluster rather than selection, which is the whole reason it belongs here: on a server-paged page selection can only ever cover LOADED faces, so whole-cluster work must not route through it. Unlike guided - where entireCluster rides the scan's suspected owner - manual has none, so it requires an explicit destination via PersonPicker, and it posts with no per-face buckets since the server rejects combining them. It carries its own confirm because it moves faces the admin has never seen, which is both the point and the risk. Manual gets its own help modal rather than extending guided's, whose names all six actions test is load-bearing and stays untouched. The modal leads with the thing most likely to confuse someone arriving from guided: keep writes nothing. It also states that Not a face is the irreversible one and sits beside Unknown, which means the opposite - bin this crop versus this is a real person I cannot name. Swatches are asserted against the tile tokens so an explanation ties back to the button and the tile it describes, and keep renders no swatch at all - absence is how the default is signalled. Tests: 21 new cases, 222/222 across the face-cleanup suite, guided's specs untouched, tsc clean. * test(e2e): cover the manual face-review flow and its cross-engine invariant Two tests. The first drives the whole manual path against a person with NO scan in existence: chooser to people browser to review page, listing all faces rather than a flagged subset, then move, lock and not-a-face through the real UI, asserting durable DB rows rather than UI text. The second is the point of the feature: a later scan must honour a manual decision exactly as it honours a guided one - the locked face is not re-flagged, the detached face stays gone, the moved face is not re-proposed. Two seeding traps handled. utils.createFace links faces with source='manual', which the verdict layer excludes from flagging, so every face here is downgraded to 'ml' or it would not behave like a machine-clustered one; and face_search rows are required because both the cluster listing and lock eligibility inner-join it. The cross-engine test seeds ONE multi-person scan rather than two sequential ones: two scans stamped the same now() make getLatestScan nondeterministic, which would let the invariant assertions pass vacuously instead of for the right reason. * docs: document guided and manual face cleanup modes Adds a Two ways to clean up comparison and a Manual review section to the admin face cleanup page. The two points worth documenting explicitly are the ones an admin cannot infer: that faces you leave alone are deliberately NOT recorded (marking everything you glanced at as human-verified would stop future scans flagging it and hide real mistakes, so Lock is the opt-in), and that Select all covers the faces currently loaded rather than the whole cluster - with Move entire cluster as the server-resolved alternative. Also notes that manual review is unavailable during a scan, so the behaviour reads as deliberate rather than broken. * fix(web): keep translation-key literal types in the manual review tally A Record<..., string> annotation widened each value to string, and $t only accepts known translation keys, so the tally lookup stopped type-checking. satisfies keeps the exhaustiveness check on the keys while preserving the literal value types. Caught only by CI's check-svelte: that task reports 0 FILES locally in this worktree - even when invoked through mise exactly as CI does - so neither tsc nor the local svelte-check surfaced it. * fix(web): build the manual tally translation key with a template literal satisfies was not enough: indexing an object of whole translation keys still yields string at the call site, and $t accepts a template-literal type but not string. Matches the guided dock's idiom exactly - $t(`admin.face_cleanup_review_tally_${...}`) - by mapping only the key SUFFIX, with move reusing guided's existing 'other' key rather than adding a near-duplicate. Only CI's check-svelte catches this; that task scans 0 files locally even when invoked through mise as CI does. * style(web): redesign the face-cleanup mode chooser Rework the two-door chooser at /admin/face-cleanup into a considered design: a 48px icon tile anchors each mode, guided keeps the primary accent while manual gets a distinct teal identity (neither marked recommended), and each card carries a tinted status well that recolours across all five scan states with the live count as a hero number. Actions become full-width and foot-aligned, with a soft corner glow, inset ring, and hover lift for depth. Logic, copy, i18n keys, data-testids and the genuinely-disabled manual guard (§7) are unchanged; all 13 chooser page-spec tests still pass, eslint/prettier/svelte-check clean. * feat(web): infinite-scroll the manual face-review lists Replace the "Load more" buttons on the manual people browser and the selected-person review grid with scroll-driven pagination, so both lists grow as the admin scrolls instead of dead-ending on a button. Adds a reusable InfiniteScrollSentinel primitive (a sentinel-only port of people-grid's IntersectionObserver + visibility-fallback logic; people-grid itself isn't a drop-in where items aren't keyed by `id`). The review grid keeps appending through vm.appendFaces, so staged marks and selection still survive each page load. * docs: spec the face-cleanup scan two-lane triage redesign * docs: sliced impl-loop plan for the scan two-lane redesign * feat(web): add exclusion-set selection model for scan triage * feat(web): add ConfidentLane with approve-all + spot-check exclude * feat(web): add ReviewFirstLane clickable review list * feat(web): two-lane triage scan page; drop checklist/table/filter/selection UI Compose ConfidentLane (approve-all + spot-check exclude) and ReviewFirstLane (clickable rows) in the scan page, driven by the exclusion-set triage model. Removes the 3-step checklist, 5 stat cards, filter tabs, and the selection bar; the two non-actionable totals become quiet footnotes. Deletes ScanChecklist, FaceCleanupTable, and the old face-cleanup selection model, and prunes their now-dead i18n keys. * docs: note the scan-redesign plan corrections applied during impl-loop * test(e2e): key the scan-drain wait off the header summary, not the removed stat card The two-lane scan redesign removed the "Eligible faces" stat card the X1 test used as its "completed snapshot loaded" signal; use the header summary line ("… flagged faces across … people"), which renders on any completed scan. * fix(web): tidy the review-lane rows — one primary reason pill, no overflow Collapse the wrapping multi-pill reason column to a single primary reason (bad-target wins, else the first) plus a "+N", narrow the flagged column, and share one absolute right-slot between the chevron (default) and the hover dismiss so the row can no longer widen past the container. * feat(web): enlarge the guided-review bulk action bar The routing buttons were text-xs with 13px icons on the dark dock and read as small/hidden. Enlarge them (text-sm, 16px icons, roomier padding, a defining inset ring, rounded-lg), tint the destructive "not a face" red to set it apart from the routine routes, and bump the selected-count and Clear. * docs: spec for face-cleanup console UX fixes (clickable auto-fix chips, review-lane alignment) * docs: implementation plan for face-cleanup console UX fixes * docs: add column-content regression guard to the face-cleanup UX plan * fix(web): align the review-lane columns and give them headings * style(web): prettier fallout on the review-lane changes * feat(web): make auto-fix spot-check chips open the per-cluster review page * fix(web): persist confident-lane spot-check exclusions across chip click-through * fix(web): default manual-review owner picker to the admin's own account and persist the selection Browse People defaulted to whichever owner sorted first alphabetically and reset to that default every time the page remounted (e.g. after reviewing a person and navigating back), instead of the admin's own account and the last owner picked. * fix(web): leave the manual review page once its cluster is empty Moving/parking/detaching the last faces out of a cluster left the admin on a dead page, and for an unnamed cluster it stacked a 404 error toast directly on top of the success toast: resolveFaces DELETES an emptied, never-named person ("Empty-unnamed cleanup" in face-repair.service.ts), so the unconditional post-apply refresh chased a person that no longer existed. commitResolve now returns to the manual review list whenever the refresh comes back missing (deleted) or with zero faces (a named cluster survives the same emptying) — success toast intact. loadPersonData grew an `allowMissing` flag so only that caller treats a 404 as "the cluster is gone"; mount and Retry still surface it as a load failure. Also fixes the refresh itself: it went through appendFaces, which is idempotent by assetFaceId, so after a PARTIAL apply the grid kept rendering the faces the resolve had just moved away. The model grew clear(), and a refresh now replaces what the page holds instead of merging into it — only the scroll sentinel appends. * feat(i18n): translate the face review UI into all nine supported locales The unified face review branch added 241 keys to en.json, but only German (168) and French (101) had any coverage — every other supported locale had just the 12 space-album keys. That is why the admin console rendered half in German and half in English. Fills the gap for all nine locales the fork translates (de, es, fr, it, nl, pl, ru, zh_Hans, zh_Hant), so each is now at 241/241: - de +73, fr +140, and +229 each for es/it/nl/pl/ru/zh_Hans/zh_Hant - reuses the terminology already established per locale (Gesichtsbereinigung / Nettoyage des visages / cluster→Cluster, groupe, grupo, gruppo, 聚类, 叢集) - Polish and Russian use one/few/many/other plural forms; Chinese mirrors the existing one/other convention - corrects one German string that drifted from its English source (face_cleanup_resolutions_empty) ICU placeholder parity with en.json verified for every key in every locale. * docs(spec): design the face-suggestion opt-in toggle Face suggestions are currently enabled by a sentinel (suggestionMaxDistance above maxDistance) and still need a manual job run, so every step of turning them on fails silently. Specs an explicit nested suggestions.{enabled,maxDistance} config, one isFaceSuggestionEnabled helper in place of eight scattered guards, save-time validation of the band, an auto-queued scan on the enable transition, and a load-time fold so instances already running the feature survive the key rename whether configured via the database or a config file. * docs(plan): implementation plan for the face-suggestion opt-in Nine tasks off the approved spec. Also amends the spec with a defect found while planning: because disabling now retains a valid distance band, the repository band short-circuits no longer imply the feature is off, so all three suggestion read paths need their own enablement guard. * feat(server): explicit suggestions.enabled config with one enablement helper Replaces the suggestionMaxDistance sentinel with a nested suggestions.{enabled,maxDistance} object and routes all eight guards plus the three read paths through isFaceSuggestionEnabled. The read-path guards are load-bearing: disabling now retains a valid band, so the repository short-circuits no longer imply the feature is off. * test(server): pin the reachable suggestions.enabled=false toggle, not just an invalid band Five "feature disabled" fixtures used suggestions.maxDistance: 0, a config state the DTO's min(0.1) bound makes unreachable in production. Flips them to suggestions.enabled: false with a valid band so the tests exercise the actual toggle four guard sites are meant to honour, and adds an enabled:false row to the two updateSpacePerson/backfillSpacePersonMetadata table-driven tests. Retitles a test whose name still referenced the removed suggestionMaxDistance field. * fix(server): fold the legacy suggestionMaxDistance key into suggestions Unknown config keys only warn and are then dropped, so the rename would have silently disabled face suggestions on every already-configured instance. The fold runs on the partial before it merges over defaults, covering both database and IMMICH_CONFIG_FILE sources. * test(server): cover the fold's 0.1-minimum and equality boundaries Review flagged missing coverage for the two load-bearing boundaries in foldLegacyFaceSuggestionConfig: the >= 0.1 schema-minimum threshold, and the > comparison's equality point (both against the default and an overridden maxDistance). No production code changes; the implementation was already correct at both boundaries. * feat(server): reject a face-suggestion band that can never match An enabled band at or below the recognition distance selects nothing. Refusing the save replaces the old failure mode where the setting saved cleanly and did nothing. * feat(server): queue the face-suggestion scan when the feature is enabled Enabling the toggle now starts the first scan itself instead of silently requiring a second trip to the Jobs page. * test(server): expand onConfigUpdate coverage to exercise all transition paths Added 4 new test cases and generalized onConfigUpdateTestConfig to vary all relevant config parameters (recognition distance, suggestions distance, flags). Tests now verify: - Band widening while enabled does not re-queue (transition gate works). - Enabling suggestions with ML or FR disabled does not queue (multi-flag logic). - Invalid band becoming valid does trigger a queue (full helper logic tested). * chore(open-api): regenerate clients for the nested suggestions config * feat(web): explicit face-suggestions toggle in admin settings Enabling auto-fills the distance from the recognition distance so the number field stops being the hidden on/off switch. * fix(web): gate face-suggestions auto-fill effect on config-file mode The auto-fill effect had no `disabled` guard, so opening the settings page in read-only config-file mode could silently rewrite the displayed suggestion distance if a config-file-sourced config already violated the invariant (config-file configs are only schema-validated on boot, not cross-field validated). Also tones down "one-tap review" to "quick review" for a desktop admin page, and adds a unit test for the auto-fill effect since check:svelte has no signal on this machine. * feat(i18n): translate the face-suggestions toggle into all nine locales * test(e2e): drive face suggestions through the explicit toggle Adds the enabled-false-with-valid-band case, which the repository band short-circuits cannot catch. * docs(facial-recognition): document the face-suggestions toggle * fix(i18n): align face-suggestion terminology with each locale's established term * test(server): split the table-driven suggestion-scan specs into it.each should not queue suggestion scans for non-name edits, ... looped 8 cases inside one shared 5s test budget; under full-suite parallelism this intermittently exceeded vitest's default testTimeout, and a failure couldn't say which case broke. Splits it (and its backfillSpacePersonMetadata sibling) into one independent it.each test per case, each with its own name and its own timeout. Same cases, same assertions, no behavior change. * fix(server): don't let the ML master switch disable face suggestions Face suggestions are a pure vector query over embeddings that already exist and never call the machine learning service, so gating them on isFacialRecognitionEnabled (ML master switch AND facial recognition) disabled a feature that still works fine with the ML container off. Turning it off to reclaim resources after a library is scanned is a supported configuration — the e2e stack itself runs that way, which is how this surfaced. Gate on facialRecognition.enabled alone. * fix(face-cleanup): apply a resolve on a cluster with more than 1000 flagged faces The resolve DTO capped every face array at 1000 on the assumption that a selection that large always goes through `entireCluster`. A real 2952-face cluster flagged 2382 faces toward one owner — a SUBSET, which `entireCluster` cannot express — and the client emits one move group per (destination, lock), so Apply 400'd with no payload the admin could ever make valid. Raise the face bound to 25 000 (~1 MB against a 10 MB body limit) and give group/person/owner arrays their own 1000 bound, so a higher face ceiling does not also permit 25 000 destinations. Two paths would have turned that 400 into a timeout instead of a fix: `drainPendingForFaces` built an unchunked IN-list, and the "keep here" bucket wrote one round-trip per face — both are now chunked set-at-a-time writes. The banner also said only "could not be applied", so a permanent failure read exactly like a transient one and sent admins into retry loops. Resolve failures an admin can actually hit now carry a stable reason code, and the page renders a translated explanation for each (all nine locales) — falling back to the server's own text rather than to silence. * fix(web): type the resolve reason-code map as Translations `$t` is narrowed to the key union derived from en.json, so the map's `Record<string, string>` widened its values out of that union and svelte-check rejected `$t(reasonKey)`. Typing the values `Translations` also turns a typo in one of these keys into a build error instead of a raw key rendered at an admin. Caught only in CI: `check:svelte` scans 0 files locally in this worktree, so it is effectively a push-only gate. Verified instead by asserting the four keys against the union directly, with a deliberately wrong key as the control. * docs(face-cleanup): design for giving the review destination an identity The review page receives thumbnailFaceId per suspected owner and renders a bare name, so a suggestion pointing at "Unbenannter Cluster" is unactionable: no thumbnail, no size, no way to go and look at it. Three defects share the root cause — the page reads one field off suspectedOwners[0]. Design: two overlay-only fields (ownerFaceCount, ownerMissing) filled by the read-time overlay that already refreshes names/thumbnails; a destination card per suspected owner with a new-tab link to the manual review page; a real destination chooser for the two bulk actions that hardcode [0] today and silently mis-route faces attributed to a secondary owner. * docs(face-cleanup): settle C2-C5 and restructure the spec around TDD slices Review of the first draft found a runtime blocker and four undecided behaviors, all now resolved: - the overlay sketch dropped the bigint->number conversion count() needs (getPersonMetadata already does it); ownerFaceCount would have failed z.number() at runtime - C2: the reviewed cluster's faceCount goes live too, from the same aggregate. flaggedFraction is denominated on frozen `eligible`, so nothing desyncs - C3: picking the reviewed cluster as its own destination disables both bulk actions with a reason, rather than filtering the paginated picker or failing server-side after Apply - C4: destinations whose person row is gone are omitted from the chooser and skipped when defaulting; the card still explains why - C5: staged rest faces follow a destination change, and the dock chip now names where they are going Also records the load-bearing ordering between withCurrentNames and withLiveFlaggedCounts (the latter's spread is what carries the new fields through), and replaces the flat test list with five red-first slices in the behavior register page.spec.ts already uses. * docs(face-cleanup): implementation plan for the review destination identity * feat(face-cleanup): report a suspected owner's own face count and whether it still exists * test(face-cleanup): pin the overlay ordering the destination fields depend on withLiveFlaggedCounts rebuilds every suspectedOwner from withCurrentNames' output; its `{ ...owner }` spread is the only reason ownerFaceCount/ ownerMissing survive to the client. Add a regression test plus comments at both call sites so a future refactor can't drop this silently. * fix(face-cleanup): correct two stale comments from the review pin The test's inline comment still claimed reordering the two overlay passes drops the fields — disproven by hand in task-2 review. Also fix a repository comment referencing getLatestScan (repository method) where the chaining actually happens in the service's getLatestScanStatus. * feat(face-cleanup): render each suggested destination as an identifiable cluster * fix(face-cleanup): give the no-thumbnail destination test a real assertion * feat(face-cleanup): let the admin choose where whole-cluster moves send faces * fix(face-cleanup): stop the destination chooser from misrepresenting state - Move "Choose someone else…" out of the <select> into a sibling button: an <option>'s value commits to the select natively before any handler runs, so a dismissed picker left the control reading the placeholder forever and re-selecting the same option fired no further change event in a real browser. A button never touches the select's value, removing the bug and the double-open risk by construction. - Render an extra <option> for a destination chosen outside the scan's own suggestions, so picking an unlisted person no longer renders the select blank. - Gate rest-tile staging on canBulkMove, and clear any already-staged rest faces the instant the destination becomes unusable, so the ribbon/dock chip can never name a destination Apply will not honour. - PersonPicker: report lock:false when the re-flag toggle is hidden, rather than echoing its unreachable default. - Rename a handler parameter that shadowed the module-level ownerPersonId. * fix(face-cleanup): keep staged rest faces on an unusable destination, block Apply instead The project decided earlier that changing destination must never discard staged rest faces (a mis-click should not destroy real work). The prior auto-clear effect only fired on a valid->invalid switch, but the realistic trigger for that is exactly the mis-click case the decision covers, and the discard was silent. - Remove the clearing effect; staged rest faces survive an unusable destination unconditionally. - Rest-tile staging is now add-only gated: deselecting a face always works, only newly staging one is blocked while canBulkMove is false. - Block Apply (disabled + inline reason) while faces are staged for a destination that can't be used, rather than quietly dropping them. - Stop naming a destination that won't be used: the dock chip, the selected rest-tile ribbon, and the rest-section hint all fall back to an unnamed variant while canBulkMove is false. - Disable + dim rest tiles that can't be newly staged, and explain why in the same slot as the self-move warning (a distinct "pick a destination first" message for the no-destination case). * fix(face-cleanup): show the destination's own size in the dashboard, not its routing share The dashboard's review-first lane printed dest.count -- the flagged faces routing to a destination -- under its name, so a destination holding thousands of faces displayed as "1 faces". Swap in ownerFaceCount for the size, move the routing share into the row's tooltip, and mirror the new suspected-owner fields (ownerFaceCount, ownerMissing) into every hand-written fixture/type that shadows the server's FaceRepairScan suspected-owner shape. * test(face-cleanup): tighten the destination tooltip assertion so a size/share swap turns it red The tooltip test used expect.stringContaining('20'), and (1204).toLocaleString() is "1,204" -- which contains "20" as a substring. So the exact regression this task exists to prevent (rendering ownerFaceCount where count belongs), relocated to the tooltip, still passed. Give count and ownerFaceCount non-overlapping digits (7 vs 1204) and assert both the literal routing-share substring and the absence of the destination-size number. * feat(i18n): translate the face-cleanup destination card into all nine supported locales Tasks 1-5 rebuilt the destination card in English only. The admin who reported the original problem reads this console in German, so shipping the English strings alone would leave the reported bug half-fixed for the reporter. Adds the 16 new admin.face_cleanup_review_* keys to de, fr, es, it, nl, pl, ru, zh_Hans and zh_Hant -- the nine locales that already carry this console -- and updates the two edited ones: - banner_body drops its leading "Default is -> {ownerName}." sentence, which no longer has a placeholder to substitute. Leaving it made placeholders.spec.ts red for all nine locales; that was the red phase for this change. - tally_ad…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a second entry path to the admin face cleanup console: pick any person and audit all of their faces, with no scan required. Builds on #834's verdict layer and is based on
feat/face-review-unified(that PR is untouched).Spec:
docs/superpowers/specs/2026-07-23-manual-face-review-mode-design.mdWhy
The console is scan-driven end to end. Open a person the scan did not flag and you get "no flagged faces" — there is no way to say "I know this person's cluster is dirty, show me everything."
The model
Manual mode introduces no new table, column, or status. It writes exactly the rows guided writes, so a decision made either way is durable and honoured by both engines:
asset_face.personId+face_identity_face.source='manual'+ drainsource='manual'+ draindeletedAt+personId=NULL+ link deleted + drainstayis deliberately absent: it writes a negative verdict against the suspected owner, which does not exist without a scan.Server
Relaxes guard E15 so
lock/unknown/detachaccept any face on the person;staykeeps the gate.Lifting it for
lockwas not merely permissive — it removed a live guard.getScanFlaggedFacesForPersonsre-validatespersonId/deletedAt/isVisibleat snapshot-read time, so a foreign id could never previously reach the write path. With the gate dropped and no replacement,resolveFaceswas verified to returnlocked: 1for a face on another person, for another user's person, and for a soft-deleted face — silently re-pointing them viareplaceFaceIdentities' unscopedON CONFLICT DO UPDATE; a nonexistent id crashed with an unhandled FK violation.getEligibleFaceIdsForPersonreplaces that guard explicitly.Also adds
GET admin/face-repair/person/:personId— the manual page has no scan to readpersonName/ownerIdfrom, andownerIdis what scopes the move-picker.Web
A separate manual review page rather than a mode flag on the guided one. The guided view-model cannot serve manual mode:
suspectedOwnerIdis required, every face initialises to'owner', andbuildResolveRequestdereferences it — so an untouched manual review would POST a move for every face to an undefined destination. It also has no neutral state (six terminal states with a tested tally invariant) and rebuilds from a$derived, which would wipe staged decisions on every page load. Forking leaves all nine guided specs untouched.New surfaces: mode chooser (equal-weight cards, five scan states, manual card disabled mid-scan because
resolveFaces409s), people browser, manual review page, and its own view-model + help modal.Verification
sync-album-user, fails in isolation too, unrelated).source='ml', i.e. not mass-stamped.Known issue for CI to arbitrate
The pre-existing e2e test "admin renders face crops for a cluster owned by a different user" fails locally in the full serial run only when the two manual-review tests are declared — despite them running strictly after it. It passes in isolation, with
--grep-invert, and with two trivial filler tests in the same position. No collection-time side effects exist and no route shadowing; the admin thumbnail path is untouched. Flagging rather than hiding it.