Skip to content

fix(face-review): GA-readiness fixes from the #834 correctness review - #977

Merged
Deeds67 merged 21 commits into
feat/face-review-unifiedfrom
fix/834-ga-readiness
Aug 12, 2026
Merged

fix(face-review): GA-readiness fixes from the #834 correctness review#977
Deeds67 merged 21 commits into
feat/face-review-unifiedfrom
fix/834-ga-readiness

Conversation

@Deeds67

@Deeds67 Deeds67 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Implements the GA-readiness spec produced by the #834 correctness review. Based on feat/face-review-unified and targets it, so the fixes can be reviewed separately from the 321-commit feature branch.

What's fixed

Blockers

  • B1 — upgraded installs failed to boot. Defaults injected suggestions.maxDistance: 0.7, which inverts against any facialRecognition.maxDistance >= 0.7. Config-file installs threw at boot; database installs returned 400 from every settings page, since the whole config is PUT on each save. deriveSuggestionBand now raises an unset band to maxDistance + 0.2, clamped to the schema ceiling of 2, and warns when no valid band exists. An explicitly set inverted band still surfaces.
  • B2 — lock: false was not honoured. Every move wrote source: 'manual', the strongest verdict in the system: both engines exclude such a face from every scan and every suggestion queue thereafter. Accepting a scan suggestion on a large cluster made all of it unreviewable while the response reported locked: 0. The relink still always happens (skipping it leaves the torn state FaceIdentityBackfill reverts); only its strength is now conditional.
  • B3 — destructive confirms rendered "NaN". Five call sites passed .toLocaleString() into an ICU plural arg, so # computed "2,952" - 0. The whole-cluster confirm read "This moves all NaN faces to Anna" — only above 1 000 faces, i.e. where the action is least reversible.

High severity

  • H5 — confirm/lock had no write-time guard, so a concurrent reassign could leave personId on one person with the identity pointing at another. replaceFaceIdentities now re-checks placement inside the transaction and returns what it actually wrote, which also corrects an inflated locked tally.
  • H6 — four verdict reads bound one parameter per id, unchunked, sized by the whole library. Reproduced at 70 000 ids: MAX_PARAMETERS_EXCEEDED: Max number of parameters (65534) exceeded.
  • H7 — the in-flight scan index was unique on the value of status, so one pending and one running row could coexist and two full scans could race, deleting each other's snapshot. Now a constant-expression index; Postgres rejects the second with Key ((true))=(t) already exists.
  • H8 — a failed per-person suggestion scan held its stable dedup jobId, so that person's queue never refilled. Added removeOnFail, plus a sweep that actually runs (the existing one is gated behind a flag already set on every booted instance).
  • H9 — 18 count-bearing keys shipped bare {count} in nine locales ("1 Gesichter", "1 лиц"). The guard written to prevent this registered only en.
  • H10face_repair_decline.personId is ON DELETE CASCADE and merge deletes the source person, so merging discarded the console's cluster mute.

Test hardening

Every item below was confirmed by applying the mutation and watching the suite stay green before writing the assertion:

  • Deleting confirmFaceSuggestion's face-ownership check survived 349/349.
  • Removing visibility: spaceVisibleAssetVisibilities survived — that mutation makes faces on Locked and Hidden assets suggestable.
  • identityId, the cross-scope key the design rests on, was expect.any(String) at 9 sites.
  • All 6 clearNegativeForTarget call sites were deletable with zero failures.
  • Hardcoding all seven Advanced-Scan params to defaults — making the modal decorative — survived.
  • The C6 cross-owner guard in executeRepair could never fire: one getById mock served both source and destination.

Also: clearMocks: true enabled globally; three assertions targeting testids that exist in no component removed; page-load.spec.ts added for all seven face-cleanup routes; medium coverage for the deletion cascades (asset_face, user, and shared space, which had no coverage anywhere). Two tests that could not fail were deleted.

Fork isolation

  • The suggestion engine moved out of upstream-owned person.service.ts / person.controller.ts into fork-owned face-suggestion.service.ts / face-suggestion.controller.ts. person.controller.ts returns to zero diff against origin/main. Proof of no behaviour change: git status --porcelain open-api packages/sdk mobile/openapi is empty after a full regenerate.
  • getJobCounts restored to upstream's pure delegate form; the Redis repair moved to getJobCountsWithRepair for the fork's own call path.
  • FacePersonVerdictRepository moved beside its four siblings in every positional DI list; placeholders.spec.ts scoped to the fork's nine locales (so Weblate can't turn the suite red on a future rebase) and the mr.json / ms.json hand-patches reverted; two upstream-owned e2e files restored.

Verification

gate result
server unit 5 656 passed, 14 skipped
web unit 5 190 passed, 347 files
check:svelte 606 files, 0 errors, 0 warnings
tsc --noEmit (server + web) clean
eslint / prettier clean on all 42 changed server + web files
OpenAPI / SDK / Dart zero-diff empty
medium specs touched pass individually

Also validated on a real library (58 206 faces / 2 348 people): clean boot and migration, and the H7 index applied as the constant-expression form.

Known limits

  • The full medium suite is flaky under full-parallel load (Postgres connection exhaustion — a different set of files fails each run, including files this branch never touched, e.g. memory.service). Every file this work touched passes in isolation. CI shards differently; treating CI as the arbiter.
  • Slice 7's migration demotion path (self-healing an instance that already has one pending and one running scan) is verified by source inspection, not live execution — the global-setup DB already has the fixed index, so the pre-fix state can't be seeded without dropping it first.
  • The spec committed here (docs/superpowers/specs/2026-08-11-...) was fact-checked after writing and had ~25 errors corrected during implementation. Individual slice line numbers are stale by design — the commits are the source of truth.

Deeds67 added 20 commits August 11, 2026 22:54
…s 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.
…very 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.
…top 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.
…ss 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.
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`.
…n suppress another user's queue"

This reverts commit b51afc0.
… 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.
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.
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.
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.
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.
…ck 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.
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.
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.
…etion 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.
…d 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/.
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.
…, 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).
@Deeds67 Deeds67 added the changelog:fix Bug fix for changelog label Aug 11, 2026
@github-actions github-actions Bot added documentation Improvements or additions to documentation 🗄️server 🖥️web labels Aug 11, 2026
…st 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
Deeds67 merged commit ac99717 into feat/face-review-unified Aug 12, 2026
69 of 73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:fix Bug fix for changelog documentation Improvements or additions to documentation 🗄️server 🖥️web

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant