feat: unify face suggestions (#592) + face cleanup (#770) on one verdict layer - #834
Merged
Conversation
github-actions
Bot
added
documentation
Deeds67
force-pushed
the
feat/face-review-unified
branch
from
July 26, 2026 10:01
1009885 to
c8a4afe
Compare
Deeds67
added
the
rc
🧪 Release candidate buildLatest RC Images published
How to run this RCIn the directory containing your services:
immich-server:
image: ghcr.io/open-noodle/gallery-server:pr-834-rc.27Then pull and restart: docker compose pull immich-server
docker compose up -dEach push publishes a new numbered tag, so update the image line to move to a newer RC. To roll back, point it at an earlier Previous builds (19)
9 older build(s) omitted. Their tags are still in GHCR until this PR closes. Last updated Wed, 12 Aug 2026 16:21:29 GMT — every push while the |
Deeds67
force-pushed
the
feat/face-review-unified
branch
3 times, most recently
from
August 5, 2026 17:49
821bbe6 to
fc911ed
Compare
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).
- 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
…-blank person crumb
…ps found in execution
…breadcrumb labels
Face suggestions were opt-in, behind a toggle an admin had to find. Default them on so a fresh install surfaces near-miss faces without any configuration. The default alone would only reach fresh installs. `onConfigUpdate`'s false -> true transition — the hook that starts the initial scan — cannot fire for an instance that upgrades *into* a new default, and `FaceSuggestionMaintenance` has no cron, so an upgraded library would show the toggle on over a permanently empty queue until someone renamed a person or ran the job by hand. `PersonService.onBootstrap` therefore queues one catch-up sweep, guarded by a system-metadata marker so it runs at most once per instance (the same one-shot pattern as SharedSpaceService.onBootstrap). The marker is burnt even when an admin has the feature switched off, since a later opt-in is already served by the ConfigUpdate transition. The sweep is safe to re-run: upsertPending is a conditional upsert that cannot resurrect a resolved row, and the scan excludes manually-placed and negatively-verdicted faces before writing, so an instance that was already using suggestions gets no duplicates and no re-asked questions. Existing configuration is preserved by the legacy fold: an admin who had explicitly set a suggestion distance keeps that decision, including one too low to ever match, which still reads as off. Only instances that never configured the setting pick up the new default. Docs: rewrite the Face Suggestions section around the new default, document the upgrade sweep, and add a section on what turning the feature off and back on does (nothing is deleted; pending rows are re-validated on read, decisions are durable). The shared-space spec gains a `getById` default: with suggestions on, the rename and metadata-backfill paths now get past `areSpacePersonSuggestionsEnabled` and ask whether the space has face recognition enabled.
The page still described the pre-unification console. Corrected against the current UI: - the chooser's mode is "Guided cleanup", not "Guided review" - the scan page's two lanes are "Ready to auto-fix" (Approve all / Approve / Review them / Exclude from this batch) and "Needs your review" — the old "Confident — auto-selected" pre-selection and the "Re-attribute selected (N)" batch button are gone, and the lanes are applied independently - the review page's per-face verdicts were undocumented; added the six actions with the Keep here vs. Confirm & lock distinction, which is the one worth learning - "View declined" no longer exists. Decisions are logged on the Resolutions page, which also covers the face suggestions users review on their own People pages, filterable by source, with Undo per entry - manual review's action label is "Confirm & lock", and the reason badge is "also flagged", not "bad-target"
Separating a profile no longer marks its faces human-confirmed. detachScopedProfile stamped source='manual' on every backing face. Under the unified verdict layer that means "a human attested to this face" and excludes it, owner-agnostically, from both the cleanup console and the suggestion scan — permanently. Separating a contaminated cluster therefore buried every ML mistake inside it, with no UI to undo. Separation is a grouping decision, not a per-face attestation, so the source label is left alone; the identityId rewrite is what makes the separation stick. The one-shot suggestion sweep marker now records that a sweep ran, not that one was queued. It was burnt at queue time even when the feature resolved off, on the assumption that a later opt-in re-triggers via ConfigUpdate's false -> true transition. That cannot happen under IMMICH_CONFIG_FILE, where updateSystemConfig throws and a YAML edit plus restart emits only ConfigInit — leaving an admin with a toggle reading "on" over a queue nothing fills. The same write also outlived a failed sweep, which runs attempts:1 with removeOnFail:true. Writing it from handleFaceSuggestionMaintenance's success path covers both, and stops the ConfigUpdate opt-in from queueing a redundant second sweep on the next boot. The resolutions page reported both target states backwards. person.name and shared_space_person.name are NOT NULL DEFAULT '', and both target FKs are ON DELETE SET NULL, so a live unnamed cluster arrives as an empty string beside an intact id, and a deleted target as a null id. Branching on the name first labelled the live cluster "Deleted target" and the genuinely deleted target "Unnamed cluster". Branch on the id instead. The spec pinned the inversion with a personName:null fixture the SQL cannot emit.
Deeds67
force-pushed
the
feat/face-review-unified
branch
from
August 10, 2026 17:24
c8128c5 to
1e27f03
Compare
Nine user-facing keys added by this branch shipped in en.json only, so the face-cleanup confirm dialog, the resolutions filter/pagination empty states, the deleted-target label and the two partial-load error banners fell back to English in every locale. face_cleanup_resolutions_load_more reuses the already-translated face_cleanup_review_load_more values verbatim — the two English strings are identical. The rest follow each file's existing face_cleanup vocabulary (cluster -> Cluster/groupe/gruppo/grupa/grupo/группа/聚类/叢集, target -> Ziel/destination/destinazione/bestemming/cel/destino/целевой человек) and register, with Polish and Russian carrying the few/many plural categories the neighbouring count strings already use.
…line width The banner is a block-level flex column, so it spanned the whole timeline regardless of content. Its content is at most five 56px face crops and two small buttons — roughly 400px — which left a metre of empty panel on a desktop screen with "N faces found" stranded against the far edge by the header's justify-between. w-fit sizes the card to its content instead. The only row that can grow is the title, so it keeps two caps: max-w-full holds it inside a narrow viewport, where the title's existing min-w-0 + truncate then engage, and sm:max-w-2xl stops a very long person name from dragging the card back across a wide screen. No behaviour change and no change at mobile widths, where the card already filled the available space.
An instance that has never scanned was shown a primary button labelled "Re-scan" wearing a refresh icon, and an empty state reading "Click Re-scan to detect contaminated face clusters" — pointing at a control in the opposite corner from the sentence naming it. The chooser one click earlier already says "Run first scan", so the wording regressed mid-flow. scan === null is exactly "this instance has never scanned" (loading and loadError are separate branches), so the action now names itself for what it is, reusing the chooser's own face_cleanup_mode_run_first_scan so the button an admin was just told to click is called the same thing on both pages, with a radar icon instead of the refresh arrow. The CTA also appears inside the empty state, so the instruction and the action are one object. The empty-state copy no longer names the button at all — naming a control in body copy is what let it rot, and all nine locales had translated the stale name into their own copy. It now describes what a scan does and that it changes nothing, reusing each file's existing face_cleanup_intro_scan_body vocabulary and register. The i18n coverage spec gains a guard, in the same shape as its other I7 checks, asserting no locale names the re-scan button in that copy again. Advanced (maxDistance / minFaces / maxFlaggedFraction) drops to ghost before the first scan: the defaults are what run one wants and an admin who has never seen a scan's output has no basis for tuning them. Recessed, not hidden, so it stays reachable. The existing "clicking Re-scan" test mocked getLatestScan to null while asserting the re-scan label — the precise state this fixes — so it now seeds a completed scan. Its timeout was also cascading into the dismiss test that follows, whose queued mockResolvedValueOnce values the failed test's un-unmounted page was consuming.
…confirm race Two web e2e failures, only one of them caused by the first-run change. face-cleanup.e2e-spec.ts asserted a "Re-scan" button on a stack that has never run a scan — the same mistake the unit test made, and precisely the wording this branch fixes. It was asserting the bug. It now expects the first-run action, asserts "Re-scan" is absent, and covers the CTA that now lives inside the empty state. The file header's summary is corrected to match. person-face-suggestions.e2e-spec.ts is unrelated and was a latent race: the confirm click fires a POST it does not await, and the only statement between the click and the API read asserted that suggestion-progress was visible — which it already was before the click, so it passes instantly and synchronises nothing. The API read could therefore beat the write and see a still-pending total of 3. It now arms a waitForResponse on the confirm POST before clicking (album.e2e-spec.ts's idiom); the pending drain shares the confirm's transaction, so the response is a sufficient gate.
…ces claiming a filter it never had Two things an admin had no way to learn from the UI. Resolutions lists NEGATIVE verdicts only, and the sole writer of a cleanup-sourced one is the "keep here" bucket — cleaning a person up by moving or confirming faces records nothing here at all. That scope lived only in a source comment, so an admin who had just processed several people arrived to an empty list that read as lost work. A subtitle now says it on the page, outside every loading/empty/error branch, because the empty states are exactly when it is needed. "Minimum faces per person / Skip people with fewer faces than this" was plainly false, and all nine locales plus the docs had faithfully translated it. No per-person face-count filter exists anywhere in the scan pipeline. decideReattribution uses minFaces to require that a suspected owner hold at least that many of the face's near neighbours, and to treat a person holding fewer than that as not claiming its own faces — which flags them without their having to lose the vote. Raising the value therefore flags MORE small clusters, not fewer, which is the opposite of what the control promised.
…ng, instead of just that nothing matched "No decisions match this filter" is true but useless: it reads as lost history, which is precisely the wrong conclusion when the rows are sitting one chip away. The state now names the source that is empty, counts what the filter is hiding, and offers one click back to all sources. The precise wording is gated on everything being loaded. `filtered` derives from `resolutions`, which holds only the pages fetched so far, so "no admin cleanup decisions yet" is a claim about the whole list that only the fully-loaded case can support — with pages outstanding, a match may simply not have been fetched. That case keeps the neutral wording and grows its own Load more button, which otherwise lives in the rows branch and is unreachable exactly when the filter matches nothing.
# Conflicts: # server/src/repositories/face-identity.repository.ts
…#977) * docs(face-review): spec the GA-readiness fixes from the #834 correctness review * docs(face-review): correct six factual errors in the GA-readiness spec and add BDD conventions * fix(face-suggestions): derive the suggestion band so upgraded installs still boot An install that raised facialRecognition.maxDistance before `suggestions` existed carries no suggestions block, so the merge supplied the 0.7 default — inverting the band against any recognition distance >= 0.7. In config-file mode buildConfig threw and the server crash-looped on upgrade; on a database source every settings page 400'd, because the whole config is PUT on each save. deriveSuggestionBand raises an UNSET band to maxDistance + 0.2 (clamped to the schema ceiling of 2, disabling the feature if no valid band exists). A band the admin set explicitly is left alone, so a real misconfiguration still surfaces. * fix(face-cleanup): honour lock:false instead of permanently locking every moved face executeRepair wrote source='manual' for every move. A manual identity link is the strongest verdict in the system — getManualLinkedFaceIds and the pending eligibility anti-join both exclude such a face from every scan and every suggestion queue, permanently. So accepting a scan suggestion on a large cluster made every one of those faces unreviewable forever, while the response reported locked: 0 and the DTO promised 'plain moves stay undurable unless the caller opts in'. The relink still always happens — leaving the face on the destination while it carries the source person's identity is the torn state FaceIdentityBackfill silently reverts. Only the strength is now conditional: 'manual' when the caller asked to lock, 'owner-person' otherwise. Route keys include the flag so a mixed batch cannot collapse into one write. entireCluster carries no lock field by design and keeps today's durable default. Four medium tests asserted the old behaviour, one of them named 'records a human placement for a moved face even when lock: false' with a comment stating the flag no longer changes durability. That was a deliberate decision that contradicted the DTO and the picker's lock checkbox; this commit resolves the contradiction the other way and the tests now pin the new behaviour, with a lock:true positive control alongside. * fix(face-cleanup): pass raw counts to ICU plurals so large clusters stop rendering NaN Five call sites passed .toLocaleString() into a {count, plural} argument. ICU computes # as value - offset, so "2,952" - 0 is NaN: the whole-cluster confirm read "This moves all NaN faces to Anna" and its button "Move all NaN". Only above a thousand, which is exactly where a whole-cluster move is least reversible. Adds a render test at 2952 with a discriminating control (the formatted string still yields NaN) plus a source scan so a new call site cannot reintroduce it. One existing test asserted values.count === '5' as a string, which was only true because of the formatting; it now pins the number. * fix(spaces): require face ownership before a space verdict can suppress another user's queue Every verdict row is stamped with the target's identity, and face_identity.id is a cross-owner key whose anti-join in getPendingForPerson has no ownership filter. So a row written against a face the caller does not own suppresses the ASSET OWNER's personal suggestion queue for their own face — and a confirm also runs clearNegativeForTarget, whose identityId arm can delete a rejection that owner recorded, leaving no trace it existed. The personal twin has guarded this since S11, with a comment naming the exact hazard. Its space counterpart checked only Editor role and reachability. Space membership grants rights inside the space; it does not grant the right to record a durable verdict about another user's face. Guard sits before the reachability check so an unowned face 400s rather than returning a silent false. dismiss delegates to reject and inherits it; both are pinned, alongside a positive control that the same call succeeds when the editor does own the face. * fix(face-cleanup): re-check face ownership inside the lock transaction The lock bucket's eligibility read (getEligibleFaceIdsForPerson) runs outside the write transaction and its own comment calls itself "advisory only". reattributeFaces and detachFaces both re-check placement at write time; replaceFaceIdentities did not, so a concurrent reassign landing between the read and the write could leave asset_face.personId pointing at the new person while this call still re-pointed the old person's identity onto the face. replaceFaceIdentities now accepts an optional requirePersonId: when set, each chunk is filtered (inside the caller's transaction) to faces still assigned to that person before writing, and returns the asset-face ids actually written. The lock bucket passes requirePersonId, scopes drainPendingForFaces/clearNegativeForTarget to the written ids, and counts locked from what was actually written rather than what was requested. The move path (executeRepair) keeps calling replaceFaceIdentities without requirePersonId, since reattributeFaces has already re-pointed personId inside the same transaction by the time it runs. Added medium coverage for the guard: a raced face is excluded from the write and the returned ids, a face still on the required person writes normally, requirePersonId omitted keeps writing unconditionally, and an empty input returns []. Updated two pre-existing F12/F13 tests whose duplicate-lock-id assertions hard-coded the old count-what-was-requested semantics for `locked`. * Revert "fix(spaces): require face ownership before a space verdict can suppress another user's queue" This reverts commit b51afc0. * docs(face-review): record why the B4 guard was reverted and what must be decided The space face-suggestion flow is cross-owner by design — the canonical fixture has an Editor acting on a third user's asset, and the pending read deliberately spans cross-owner contributions. Requiring face ownership denied the feature's primary flow and shipped a red medium suite. The underlying leak (a non-owner's space verdict suppressing the asset owner's personal queue via the cross-owner identity arm, and a confirm deleting that owner's rejection) is still open. Slice 4 now records the three candidate fixes and their trade-offs instead of a fix that cannot ship. * fix(face-cleanup): chunk the four library-sized verdict reads face-verdict.service.ts fans out to four reads that bind one parameter per id and were never chunked: getManualLinkedFaceIds and getPersonVerdictTokens (face-identity.repository.ts), getNegativeVerdictTokens (face-person-verdict.repository.ts), and getClusterMuteMap (face-repair-decline.repository.ts). minFaces is admin-settable, so a full-library scan can pass every flagged face (or suspected owner) in the instance and exceed Postgres's 65,535 bind-parameter ceiling, permanently failing scans at high minFaces. Three of the four return a Map, which the existing @ChunkedArray/ @ChunkedSet decorators cannot merge, so all four are chunked with an explicit loop of 1000 instead, matching every sibling write path in this feature. No decorator changed, so the generated server/src/queries/*.sql docs are unchanged. * fix(face-review): close the review findings on slices 1-3 Slice 1 (B1): - Add the equality-boundary case. An install sitting exactly at the 0.7 default band is the most common upgrade shape, and mutating the guard from > to >= left the whole file green while reintroducing the crash for those users. - Round the derived value to two decimals: 0.7 + 0.2 is 0.8999999999999999, and updateConfig persists it and the admin number input renders it. - Warn when the ceiling forces suggestions off. The invariant check in buildConfig is gated on suggestions.enabled, so it cannot fire afterwards — without this the admin gets no diagnostic at all. - Document the derivation in the admin docs (the slice's own step 5, missed). Slice 3 (B3): - Tighten the source guard from toLocaleString\(\) to toLocaleString\(. A locale-aware call such as toLocaleString('de-DE') formats identically and evaded the narrower pattern; verified by mutation that it now fails. Slice 2 (B2): - Correct two comments that contradicted the code after the behaviour change. * fix(face-cleanup): make the in-flight scan index enforce a single scan face_repair_scan_in_flight_uq was UNIQUE (status) WHERE status IN ('pending','running'), which is unique on the VALUE of status — so one 'pending' row and one 'running' row could coexist. Two admins crossing the pending -> running transition at the same time both succeeded, and pruneSupersededScans then deleted the loser's flagged-face rows mid-flight. Fixed with a constant-expression unique index (ON (true) WHERE ...) so every in-flight row evaluates to the same key and at most one can exist regardless of which in-flight status it carries. The migration first demotes every in-flight row but the newest to 'failed', so an instance that already hit the old race can still apply this migration instead of crash-looping on index creation. Table decorator and migration_overrides payload are updated to match (schema-drift.spec.ts stays green), and the new migration name is appended to revert-to-immich.sql. * fix(face-suggestions): let a failed per-person scan be re-enqueued PersonSuggestionScan and SpacePersonSuggestionScan set removeOnComplete but not removeOnFail, unlike every sibling job option in getJobOptions. A job that fails while removeOnFail is unset permanently occupies its stable per-person dedup jobId, and BullMQ silently ignores every later add() with the same id — one transient failure and that person's suggestion queue never refills again, with no log and no admin-visible symptom. removeOnFail protects new failures only; it does nothing for jobIds already stuck in the failed set. The existing prefix sweep (removeFailedJobsByJobIdPrefix) has one production caller, SharedSpaceService.onBootstrap, gated by SystemMetadataKey.SharedSpaceFaceJobCleanupState — already true on every instance that has ever booted, so folding the new prefixes into that gate would make the sweep for them a permanent no-op exactly where it is needed. Added a second, independent one-time sweep under its own SystemMetadataKey.PersonSuggestionScanJobCleanupState so it actually runs and clears the already-stuck jobIds. * fix(i18n): restore ICU plurals in nine locales and make the guard check them 18 count-bearing admin.face_cleanup_* keys had been flattened to a bare {count} interpolation in de/fr/it/nl/pl/es/ru/zh_Hans/zh_Hant, so they rendered ungrammatical text like "1 Gesichter" / "1 visages" / "1 лиц" for an admin whose queue was down to exactly one item. The guard written to catch this (face-cleanup-plurals.spec.ts) only ever registered `en` and read en.json, so it stayed green while the bug shipped in all nine translations. - Extract the fork's nine-locale list into web/src/lib/i18n/fork-locales.ts (FORK_LOCALES) and have fork-string-parity.spec.ts and face-cleanup-i18n-coverage.spec.ts import it instead of each keeping their own copy, which could silently drift apart. - face-cleanup-plurals.spec.ts now derives the plural-bearing key list from en.json (any admin.face_cleanup_* whose English value carries `{count, plural`) and asserts every fork locale's translation of it also carries an ICU plural clause. A key missing entirely from a locale falls back to English and is fork-string-parity's concern, not this guard's. - zh_Hans / zh_Hant are not valid BCP47 tags: constructing IntlMessageFormat with the underscore code throws, and svelte-i18n silently falls back to the raw, unparsed message — so a render assertion registered under 'zh_Hans' would pass no matter what the translated content says. The locales are now registered under their hyphenated convertBCP47 form (exactly how the app's own initLanguage does it) while still reading the underscore-named source files, with tests proving that registration is load-bearing rather than decorative. - Rewrote the 18 keys in all nine locales using each language's own CLDR plural categories rather than a mechanical copy of English's two branches: one/other for de/es/fr/it/nl, one/few/many/other for pl/ru, and other-only for zh_Hans/zh_Hant (Chinese has no plural inflection). - Fixed i18n/nl.json's face_cleanup_review_picker_create: a bare apostrophe before `{query}` opens an ICU quoted-literal section, so the placeholder printed literally instead of being substituted. Replaced the apostrophes with a plain double quote (matching English's own `"{query}"`), which is not ICU-significant, and added a render assertion plus a positive control proving the old shape really did leak the placeholder. No apostrophe guard was added for the other locales — a naive /'\{/ regex produces false positives across existing correct ICU escapes (e.g. oauth and delete_group keys), and this was the only genuine occurrence. * fix(face-cleanup): keep cluster mutes when their person is merged away face_repair_decline.personId is ON DELETE CASCADE (unlike face_person_verdict.personId, which is SET NULL and already re-pointed by retargetVerdictPersonId), so mergePersonProfile deleting the source person silently destroyed any cluster mute recorded against it. The console's "stop showing me this cluster" decision vanished and the cluster resurfaced on the very next scan, even though face-repair.merge-consistency.spec.ts's own header claims both durable Face Cleanup facts survive a merge — they didn't, this one just had no test. Added retargetDeclinePersonId (server/src/utils/face-decline-merge.ts, mirroring face-verdict-merge.ts's style) and call it in mergePersonProfile immediately after retargetVerdictPersonId, so it commits or rolls back with the rest of the merge. A person carries at most one type='person' decline row (createClusterMutes deletes-then-inserts), so there are three cases: only the source has a mute (move it), only the survivor has one (or neither does — no-op), or both have one, in which case the survivor's row absorbs the union of both suspected-owner sets and the source row is dropped. Leaving two rows for the survivor would be worse than the bug: getClusterMuteMap does a plain Map.set() per row it reads, so which row "wins" would depend on read order. Edge case: suspectedOwnerIds carries no FK, so if the survivor's mute already lists the source person as a suspected owner, the union keeps that now-dangling id. Left as-is — nothing upstream of this function enforces referential integrity on that column either, and pruning it is out of scope for a merge-time re-pointing step. * test(face-review): pin the seven assertions that survived mutation All seven gaps were confirmed by watching the named mutation survive the existing suite (green), then adding the assertion and watching it fail (red), then reverting (green again): - person.service.ts confirmFaceSuggestion: deleting the face-ownership requireAccess call (Permission.PersonCreate) survived unit tests. - person.service.ts handlePersonSuggestionScan: removing `visibility: spaceVisibleAssetVisibilities` from the personal searchFaces call survived (would let Locked/Hidden assets become suggestable). The space-scan call's copy of the same arg is confirmed redundant by the search repository's unconditional spaceVisibilityGate on that branch, so it is deliberately left unpinned. - identityId was expect.any(String) at every verdict-write assertion in person.service.spec.ts and shared-space.service.spec.ts. Pinned to the concrete id the mocked ensure*Identity call returns, and added an argument-pin on the shared-space reject/ignore call site (the OTHER ensureSpacePersonIdentity call site, inside confirm, was already pinned). - clearNegativeForTarget: all 6 production call sites (3 in person.service.ts, 1 in shared-space.service.ts, 2 in face-repair.service.ts) were deletable with zero failures. Added an argument assertion at each, matching that site's real target shape (personId vs spacePersonId). - face-repair.service.ts executeRepair: the C6 cross-owner guard could never fire because every existing test resolves both people to the same owner. Added arrangeDifferentOwnerMove and a test that gives getById differing ownerIds per id, asserting the route is skipped and nothing is written. - face-repair.service.ts triggerScan: hardcoding all seven scan-override params to their defaults survived — the Advanced Scan modal was decorative. Added an assertion pinning all seven fields to non-default values. - Deleted two tests that could never fail: the "admin guard" test in face-repair.scan.spec.ts asserted only that a QueueName constant is defined (the real guard is already proven by face-repair-admin.controller.spec.ts's authenticated-route tests), and face-repair.scan-defaults.spec.ts, which echoes its own mocked config values back at itself — verified vacuous by hardcoding the service to ignore its config input entirely and watching the test still pass. * test(face-review): clear mocks, fix vacuous assertions, cover the deletion cascades - web/vite.config.ts: enable clearMocks: true globally. Ran the whole web unit suite twice after enabling it (5255/5255, identical to the pre-change baseline both times) — nothing relied on cross-test mock-call leakage, so no fixes were needed. - Three testid assertions targeted testids that exist in no component (locks-section, manual-review-load-more, move-rest-selection-btn) and so could never fail. Deleted each: none has a real counterpart to point at (resolutions has no locks concept, the manual-review page uses a scroll sentinel with no load-more button, and the rest section was deliberately built with no separate commit button — its real guard, `resolveFaces).not.toHaveBeenCalled()`, already covers the behaviour). Per the brief's correction, no generic testid-guard test was added. - scan/+page.svelte's polling test never advanced past the completed-scan poll. Rewrote it to advance to the poll that returns a completed scan, then advance another 20s (past POLL_MAX_MS) and assert getLatestScan's call count is unchanged — this fails if stopPolling() is skipped in the !isActive branch. - [personId]/page.spec.ts only ever asserted `goto` was NOT called. Added a positive assertion on the Apply-success path (goto called with Route.faceCleanupScan()) and on the empty-state Cancel button (the other goto call site on this page) — both currently unexercised. The manual sibling (people/[personId]) already had positive coverage for its own target, Route.faceCleanupPeople(); left as-is. - Added page-load.spec.ts for all 7 face-cleanup +page.ts files (the repo's existing convention), covering the authenticate(url, { admin: true }) gate on each and the declined/+page.ts 307 redirect to the unified resolutions page. - Added medium tests for three FK-cascade paths nothing exercised against a real database: deleting an asset_face cascades its face_person_verdict row (ON DELETE CASCADE); deleting the actor user degrades actorId to NULL (and a scan's requestedBy the same way) while listNegativeVerdicts still renders the row via its LEFT JOIN; deleting a shared space cascades its shared_space_person row, which degrades the verdict's spacePersonId to NULL while identityId (an independent FK straight to face_identity) survives untouched. Full web unit suite green (347 files / 5269 tests) with check:svelte and check:typescript clean. Server: ran the full medium suite twice; the resulting failures are pre-existing, environmental (Postgres connection exhaustion under full-suite parallelism — a different set fails each run) and reproduce on files this commit never touches; every touched/added medium file passes cleanly in isolation. * refactor(face-suggestions): move the suggestion engine into fork-owned files person.service.ts carried the fork's entire face-suggestion engine inline — the four suggestion-scan job handlers and the five confirm/reject/ignore/ dismiss/get endpoints — none of which interleave with upstream PersonService logic. Moves that code, verbatim, into a new FaceSuggestionService and FaceSuggestionController, matching the isolation already applied to face-repair.service.ts, classification.service.ts and shared-space.service.ts. person.controller.ts returns to its origin/main content; person.service.ts keeps only the genuine in-place hooks (verdict clearing in reassignFaces/ reassignFacesById, the re-scan queue in update(), the backfill-completion queue, and the bootstrap sweep). Medium tests that exercised the moved methods through PersonService now use a second service instance sharing the same MediumTestContext dependencies (MediumTestContext.getService), added to test/medium.factory.ts for this purpose. Behaviour is unchanged: every route path, DTO and job name is identical, the unit suite's pass count is identical before and after (5655 passed / 14 skipped), and rebuilding + regenerating the OpenAPI spec and both generated clients produces a zero diff in open-api/, packages/sdk/ and mobile/openapi/. * refactor(jobs): take the Redis repair out of upstream's getJobCounts getJobCounts is a pure delegate upstream. The fork made it async and prepended removeDanglingActiveJobs (cleaning up BullMQ "active" list entries with no backing job hash or lock, first found on the fork's own PeopleBackfill queue) directly inside it, so every read — including upstream's own call paths (queue.service.ts's admin queue poll, media.service.ts, person.service.ts's recognition-queue checks, storage-migration.service.ts, and this repository's own getTelemetryMetrics) — silently mutated Redis. Adds getJobCountsWithRepair(name), carrying today's repair+delegate body, and restores getJobCounts to upstream's exact pure form. Every caller listed above turned out to be either byte-identical to origin/main or, in getTelemetryMetrics's case, a passive per-request-shaped telemetry read that should never write on a read either way — verified individually against origin/main rather than taken on the original spec's word, which mislabeled several of them as fork-added. The one caller that genuinely needs the repair is waitForQueueCompletion: it blocks in a loop deciding whether a queue has actually drained, and a dangling active entry would otherwise read as perpetually "still active" — a comparatively rare, blocking wait, not a per-request read, so paying for the repair there does not reintroduce the problem. Adds a test asserting getJobCounts performs no Redis writes even with a dangling active entry present, and renames the existing repair test to target getJobCountsWithRepair directly. * refactor(fork): three isolation fixes for FacePersonVerdictRepository, placeholders.spec.ts and e2e utils 15a. FacePersonVerdictRepository sat inside upstream's alphabetized repository list in base.service.ts (BASE_SERVICE_DEPENDENCIES, the constructor parameter list, and the static create() ctx list) instead of beside its four siblings (FaceIdentityRepository, FaceRepairRepository, FaceRepairScanRepository, FaceRepairDeclineRepository). An upstream insertion into that stretch would conflict on every rebase, and a careless resolve would silently desync the positional lists and inject the wrong repository into every service. Moved it beside its siblings in all four affected files (base.service.ts x3, repositories/index.ts, test/utils.ts x3, test/medium.factory.ts x2) — the full unit suite passing is the proof the positional lists still agree. 15b. web/src/lib/i18n/placeholders.spec.ts iterated every file in i18n/, including the ~80 translator-owned locale files this fork does not maintain — which is why mr.json and ms.json had been hand-patched to keep the suite green. Scoped it to en.json + FORK_LOCALES (the shared module fork-locales.ts already created for this exact purpose) and reverted the mr.json/ms.json patches now that Weblate content is out of scope. The module's own doc comment already claimed this suite was scoped correctly; it wasn't — now it is. 15c. Reverted two upstream-owned e2e files to origin/main: asset.e2e-spec.ts (an upstream live-photo test edited to load a fork video fixture from across the server/e2e directory boundary — no independent justification for the real-video requirement was found, so it reverts to upstream's makeRandomImage() bytes) and utils.ts's isQueueEmpty, whose paused-aware semantics change affects all ~97 callers of the shared waitForQueueFinish helper, including upstream's own jobs.e2e-spec.ts (which deliberately waits on a paused queue). No currently-existing fork spec was found to depend on the changed semantics at its actual call site, so no fork-local isQueueEmptyIgnoringPaused was added — the brief's own instruction gates that addition on such a spec existing. utils.ts's DB URL change to use the fork-added playwrightDbPort export was kept; it isn't part of the isQueueEmpty regression and playwrightDbPort has no upstream equivalent to fall back to. Verified with `cd server && pnpm test` (identical pass/fail shape, only the expected +1 from slice 14's new test), `cd web && pnpm exec vitest run`, `cd web && pnpm run check:svelte && pnpm run check:typescript`, and `cd e2e && pnpm exec tsc --noEmit` (e2e suites intentionally not run — they need a live stack). * test(spaces): account for the new person-suggestion-scan sweep on first bootstrap H8 split onBootstrap into two independent cleanups, each behind its own state key — the shared-space sweep is already marked done on every booted instance, so appending prefixes to it would never have run. That makes three removeFailedJobsByJobIdPrefix calls on a first boot (PeopleBackfill and FacialRecognition for shared-space faces, PeopleBackfill for the suggestion scans), not two. The test's actual intent — sweep on the first bootstrap only — is unchanged and still asserted: the second boot calls nothing, because both state keys are set. Also pins the new sweep's arguments so the prefixes cannot silently drift.
Deeds67
added a commit
that referenced
this pull request
Aug 12, 2026
…p collision #834 landed 1785000000000-AddFaceRepairLock, which shares a timestamp prefix with this branch's 1785000000000-AddPhysicalUsageInBytes. Two migrations on one timestamp is ambiguous and breaks the repo's unique-round-timestamp convention, so move ours to 1791000000000 — above main's highest (1790000000000). The migration has never merged, so no database has recorded the old name and no pre-rename alias entry is needed in the revert script.
Deeds67
added a commit
that referenced
this pull request
Aug 12, 2026
…o upstream semantics (#979) * feat(server): add storageUsage config group for derivative accounting * feat(server): add derivative filename asset-id parser * fix(server): remove dash from derivative filename separator check and add android motion test * feat(server): allow filtering files out of storage size walks * feat(server): add physicalUsageInBytes column and reset quota to upstream semantics Adds user.physicalUsageInBytes (originals + thumbnails + transcodes) and a migration that preserves the current quotaUsageInBytes value into it before resetting quotaUsageInBytes to upstream semantics (asset_exif.fileSizeInByte sum, external libraries excluded). Also updates the test factory for the new column and keeps revert-to-immich.sql / its coverage test in sync with the new fork migration and column. * feat(server): look up a user's external library asset ids * fix(server): restore upstream quota usage and track physical usage separately * docs(plan): add task 12 for config-file support and documentation * fix(server): pin the storage-usage resync to the microservices worker * feat(server): enforce quota against physical usage when opted in * feat(api): expose physical usage and the display toggle Adds physicalUsageInBytes to UserAdminResponseDto and storageUsageIncludesDerivatives to ServerConfigDto so clients can pick which storage-usage number to show, without exposing the separate quota-enforcement toggle. * feat(web): show physical storage usage when the server opts in * style(web): run prettier on rail-storage.spec.ts * fix(web): add physicalUsageInBytes to userAdminFactory to green the typecheck * test(web): describe wiring divergence in the parity comment; cover null physical usage * feat(web): add admin settings for derivative storage accounting * fix(i18n): normalize storage terminology within the storage-usage panel Polish used three different storage nouns (magazynu / no noun / przestrzeni dyskowej) across the panel's four switch strings; normalized to magazyn throughout, keeping kontyngent for the quota concept itself. Italian's display-switch description said spazio di archiviazione while its own title said utilizzo visualizzato; changed the description to echo utilizzo di archiviazione, matching the title and the panel's other keys. * chore(fork): declare storage usage accounting in the ownership manifest * docs: document storageUsage config file options * docs: correct Task 12's rationale for how storageUsage is pinned * chore(server): sync the generated SQL for getExternalAssetIds The @GenerateSql decorator on AssetRepository.getExternalAssetIds emits a block that was never written to src/queries/asset.repository.sql, so the sql-schema-up-to-date job would fail on the diff. * chore(api): regenerate the Dart client for the storage usage options Picks up ServerConfigDto.storageUsageIncludesDerivatives, SystemConfigDto.storageUsage, UserAdminResponseDto.physicalUsageInBytes and the new SystemConfigStorageUsageDto. * test(e2e): assert the new server config flag in the exact-match literal GET /server/config now returns the required storageUsageIncludesDerivatives, so the toEqual literal has to carry it. With both storage usage toggles off the exposed value is false. * fix(server,web): keep the displayed storage figure consistent with enforcement Two independent toggles meant a supported configuration (includeDerivativesInDisplay=false, includeDerivativesInQuota=true) showed a user "10 GB of 20 GB" while rejecting their upload as over quota — the exact misleading number this work set out to remove. - server: expose storageUsageIncludesDerivatives as the OR of the two toggles, so opting quota in implies showing the enforced figure. - web: make the admin user page's storage meter follow the same server config flag instead of always reading quotaUsageInBytes, so an admin investigating a rejected upload sees the number enforcement used. * fix(server): resync cached storage usage at boot on config file installs SystemConfigService.updateSystemConfig rejects outright while IMMICH_CONFIG_FILE is set, so those installs never emit ConfigUpdate and the only resync trigger never fired. An admin who set includeDerivativesInDisplay in a config file saw 0 B on a fresh install, or a drifted migration-time value on an existing one, until the nightly job ran. Resync on ConfigInit instead, gated on the install actually using a config file and on at least one toggle being on. Pinned to the microservices worker like onConfigUpdate, since UserSyncUsage carries no jobId. Also documents how the figure populates and how it stays fresh, including that nightlyTasks.syncQuotaUsage is what keeps it current. * feat(web): explain how the cached storage figure is refreshed The figure is only recalculated when a toggle is first turned on and then by the nightly Sync quota usage task, so an admin who disabled that task would see a permanently frozen number with nothing on screen saying why. Adds the hint in en plus the nine maintained locales. * docs(fork): mark the fork's hunks in upstream files and correct stale comments - Mark every storage-usage hunk sitting in an otherwise upstream file with a Gallery-fork comment, starting with the greatest(0, ...) line inside updateUsage — the one a rebase resolver would most easily drop. - Restore the circular-dependency note on the lazy StorageService import in base.service, matching the other three lazy-import sites in that file. - Name the actual syncUsage call sites instead of "below this method". - Drop the claim that physicalUsageInBytes is not in the userAdmin projection; it has been since database.ts added it. * docs: state that quota accounting also changes what users are shown The two toggles are no longer independent — enabling quota enforcement now switches the displayed figure too — so the admin panel copy for the quota switch and the two code comments that described the flag as display-only were incomplete. Updated in en plus the nine maintained locales. * chore(server): renumber AddPhysicalUsageInBytes off the #834 timestamp collision #834 landed 1785000000000-AddFaceRepairLock, which shares a timestamp prefix with this branch's 1785000000000-AddPhysicalUsageInBytes. Two migrations on one timestamp is ambiguous and breaks the repo's unique-round-timestamp convention, so move ours to 1791000000000 — above main's highest (1790000000000). The migration has never merged, so no database has recorded the old name and no pre-rename alias entry is needed in the revert script. * refactor(server): collapse derivative storage accounting to one toggle The two independent opt-ins (display and quota) could disagree, which meant holding two numbers at once: a physicalUsageInBytes column, a migration, a field on AuthUser/UserAdmin and both user DTOs, a ServerConfigDto flag telling clients which number to render, and a matching read-site change everywhere storage is shown. One key, storageUsage.includeDerivatives, removes all of it. quotaUsageInBytes again holds whatever the admin chose: upstream's originals-only statement when off, the disk/S3 walk when on. Nothing downstream has to know which produced it, so the column, the migration, the DTO fields and the enforcement branch in requireQuota all revert to upstream. UserRepository keeps upstream syncUsage verbatim and updateUsage exactly as upstream has it; setUsage returns as the fork setter writing the same column. BaseService.syncUsage is now either/or - running upstream's statement alongside the walk would overwrite the walked figure with the originals-only one. The resync trigger fires on any change of the flag, not just off->on: switching off leaves a derivative-inclusive figure that also needs recomputing. * test(server): cover the single storage-usage toggle Adapts the handleUserSyncUsage block to setUsage and one flag, keeping the S3 prefix walk and the external-library exclusion, and pins the either/or: with the toggle on, upstream's originals-only statement must not also run. The storage-usage service spec now covers on->off queuing alongside off->on. Tests that only asserted removed behaviour - quota enforcement picking a column, the ServerConfigDto flag, the physicalUsageInBytes column shape - are deleted rather than left asserting nothing. The medium spec is renamed to match what it still covers. * revert(web): return the storage meters to upstream, keep one admin switch With quotaUsageInBytes always holding the figure the admin asked for, no client needs to know the setting exists. StorageSpace, rail-storage and the admin user page go back to reading that column directly, which retires the shared derivation helper, the serverConfigManager.valueOrUndefined accessor and the rail-storage tests covering the removed branch. rail-storage keeps its original "Duplicated from StorageSpace.svelte" comment and parity block. The settings panel keeps its refresh hint and drops to a single switch. * fix(i18n): collapse the storage-usage strings to one toggle Replaces the separate display and quota switch strings with a single pair describing what the one switch now does - it moves the shown figure and the enforced figure together - and rewords the refresh hint, which said "when you first turn one of these on" and now has to cover turning it off as well. en plus the nine maintained locales. * chore(api): regenerate clients for the single storage-usage key Only SystemConfigStorageUsageDto survives, with one field. ServerConfigDto and UserAdminResponseDto return to upstream: no client reads a second usage number or a flag selecting between them. Regenerated from a built server (sync-open-api, oazapfts, generate-dart-sdk), not hand-edited. * docs: describe the single-toggle storage usage design config-file.md documents storageUsage.includeDerivatives and drops the two-flag interaction that no longer exists. The ownership manifest keeps the paths this change still touches and drops the ones that reverted to upstream, along with the migration glob for the deleted column migration. The plan doc described a two-column design the code no longer implements, so it is rewritten as a design record of what shipped, including why the two-toggle draft was abandoned. * fix(server): format system-config.service.spec.ts per prettier CI's Test & Lint Server job runs `prettier --check .` first, and a misformatted storageUsage config-file test was failing it before any test ran. * fix(i18n): stop naming Immich in the storage-usage description, fix the refresh-hint clause The derivatives-setting description named upstream Immich by product name, which the branding pipeline has no override for — substituting "Noodle Gallery" there would turn "matches upstream Immich" into the false claim "matches upstream Noodle Gallery". Reworded to reference the upstream default without naming the product, in en and all nine translated locales. The refresh-hint string also overstated what stops when the nightly sync task is disabled: UserService.onAssetCreate keeps applying live original-size deltas regardless, so only the reconciliation of derivative bytes actually stops. Reworded the same nine locales. * docs(server,web): drop stale plural wording from the single-toggle design Both comments still described the abandoned two-toggle draft ("when the toggles are enabled", "opt-ins for counting..."). There is only one admin toggle now. * test(server): include src/gallery in the coverage report src/gallery/** now holds this PR's core storage-usage logic but was missing from the coverage include list alongside cores/services/utils/ sql-tools, so it never showed up in coverage output. * fix(server): lowercase the extracted derivative asset id getExternalAssetIds returns Postgres-lowercased uuids, but getDerivativeAssetId preserved the filename's own casing, so an uppercase-cased filename would fail the set-membership check and get counted as non-external. Unreachable today since filenames are built from asset.id (already lowercase), but the test was locking in the mismatch by asserting the uppercase value came back. Normalise the extraction instead and flip the test to match.
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.
What this does
Ships the fork's whole face-review story on a single shared truth layer, so a decision made in one surface is never re-asked, silently reverted, or left stale by another:
None of these ship alone. This branch supersedes #592 and #770 (leave them open as review context; do not merge them). #838 is already merged into this branch.
Why unify
Both original features independently recorded the same fact — a human's verdict about a (face → person) pairing — in different private tables that neither engine reads. That produced concrete, verified defects when both are present:
searchFacesfilteredasset.deletedAtbut notasset_face.deletedAt, so the suggestion scan kept writing rows for "not a face" crops.person_face_suggestion.rejectedvsface_repair_decline), neither read by the other.face_repair_lockhad no revocation path outside the admin page.The model — three facts, one home each
face_identity_face.source='manual'— keyed by identity (survives merges), replaced (never stale).face_repair_lockwas a weaker duplicate and is retired.face_person_verdict, identity-first with a person/space-person fallback, so one rejection answers the question in personal scope and every space. There is noconfirmedstatus.asset_face.deletedAt(already canonical;searchFacesnow honours it).face_repair_declineis narrowed to console-local cluster mutes. Both engines read one shared exclusion predicate (applyVerdictFilters) through one sharedFaceVerdictService— write-time scan exclusion, read-time anti-joins, and cross-scope projection.Three decisions about what counts as a human attestation (each reversible, each pinned by tests):
'manual'. A merge is a person-level decision; mass-stamping blinded the cleanup scan to whole merged clusters.sourcealone. TheidentityIdrewrite is what makes the separation stick; stamping'manual'buried every ML mistake inside a contaminated cluster forever.lock: false:'manual'when the caller asks to lock,'owner-person'otherwise. Writing'manual'unconditionally made every face in an accepted cluster permanently unreviewable while the response reportedlocked: 0.Server
face_person_verdicttable + repository (identity-first reads, coalescing upsert that never nulls a stronger key, bulk pending-drain), re-keyed through merges (FKidentityIdisON DELETE SET NULL— degrade, don't destroy).face_repair_locktable/repository/migrations deleted.face_repair_decline) are re-targeted on a merge instead of being CASCADE-deleted with the source person.unassignFacesclears manual links so "reset all people" leaves no phantom placements.POST /admin/face-repair/unconfirmdowngrades a placement so a re-scan may flag it again.machineLearning.facialRecognition.suggestions.{enabled,maxDistance}, defaulting on, with the legacysuggestionMaxDistancefolded in so an admin's existing choice is preserved. The band is derived on upgrade (maxDistance + 0.2, clamped) — the flat default inverted the band for any install that had raised recognition distance, crash-looping config-file installs and 400'ing every admin settings page on DB config.onBootstrapsweep fills the queue for libraries that upgrade into the new default (theConfigUpdatefalse→true hook cannot fire for them), marked done only when the sweep actually ran. Per-person scan jobs setremoveOnFail, plus a one-time sweep for jobIds already stuck in the failed set.UNIQUE (status)let onependingand onerunningrow coexist, and the loser's flagged faces were pruned mid-flight); the migration demotes pre-existing in-flight rows so an already-raced instance can still apply it.requirePersonId) and counts what was actually written.minFacesscan cannot blow Postgres's 65,535 bind-parameter ceiling.person.service.ts/person.controller.tsintoFaceSuggestionService/FaceSuggestionController(person.controller.tsis back toorigin/maincontent); the Redis dangling-active repair moved out of upstream'sgetJobCountsintogetJobCountsWithRepair;FacePersonVerdictRepositorysits beside its four siblings instead of inside upstream's alphabetised list; two upstream-owned e2e files reverted.Web
The admin console is now a coherent surface rather than one page per feature:
/admin/face-cleanup— chooser landing with a permanent explanation, equal-weight guided/manual cards, and the full state matrix (never-scanned / running / flagged / zero-flagged / failed); the manual card is genuinely disabled while a scan runs, which is the UI half of theresolveFaces409 guard./admin/face-cleanup/scan— two-lane triage: a confident lane you can bulk-approve (with clickable per-cluster spot-check chips) and a review-first lane in a fixed-column table./admin/face-cleanup/[personId]— guided per-cluster review;/admin/face-cleanup/people[/[personId]]— manual review, any person, no scan required./admin/face-cleanup/resolutions— negative verdicts from both engines, with a source filter (admin cleanup vs user review), actor and space name, a stated scope, and an empty state that says what the filter is hiding./declined307s here.FaceReviewDock+ one mode-aware actions-help modal + aface-actionsregistry that owns each action's colour, icon and copy, so both review pages behave identically; breadcrumbs built from one builder across the whole console.{count}→NaNbug in destructive confirmations fixed at five call sites, and guards that actually check the fork locales (sharedFORK_LOCALES, plural guard, parity guard, placeholder guard scoped off the ~80 translator-owned files).Review history
This branch has been through three full review-and-remediation rounds since the original write-up, each executed as red-first TDD slices whose tests stay as the regression suite:
2026-07-23-face-verdict-layer-remediation-design.md2026-07-30-face-review-unification-remediation-slices.md2026-08-11-pr834-ga-readiness-implementation-slices.md(merged as #977)Test hardening in the last round was mutation-driven: each gap was confirmed by watching the named mutation survive a green suite, then pinned. Assertions that could never fail (testids present in no component, a spec echoing its own mocked config back at itself) were deleted rather than kept as decoration, and
clearMocksis now enabled globally for the web suite.Testing
face-review-cross-flowdrives both engines against one DB with a real embedding KNN scan.face-verdict.merge-durability,face-identity.manual-durability,face-suggestion-exclusions, plus FK-cascade medium tests against a real database.revert-to-immich.sqland the e2e suites all run in CI on every push to this branch — see the checks on this PR for the current head.For reviewers
Diff against the union of #592, #770 and #838. The genuinely new surface is the unification and everything after it:
face_person_verdict+FaceVerdictService, the lock retirement, the three attestation decisions, the console UX unification, the suggestions-by-default upgrade path, and the fork-isolation refactors. The imported feature commits were reviewed on their own PRs.Known open items
identityId, andgetPendingForPerson's identity anti-join has no ownership filter — so it can suppress the asset owner's personal queue, and a confirm'sclearNegativeForTargetcan delete a rejection that owner recorded. The obvious fix (require face ownership) denies the feature's primary flow — shared-space suggestions are cross-owner by design — so it was implemented, reviewed and reverted. Three candidate fixes and their trade-offs are recorded in slice 4 of the GA spec; this needs a product call.unconfirmendpoint backing it exists and is tested) and a user-facing undo-my-reject affordance.Migrations on this branch were edited in place across iterations (notably
1787…AddFacePersonVerdict), and #592/#770 recorded migration names that no longer exist on disk. Any RC, staging or personal clone that ran an earlier version of this branch, or #592/#770, must be reset — not upgraded in place (Kysely hard-fails on boot when a recorded migration has no file). Fresh installs and instances coming frommainare unaffected.