fix(photo-exif): caption pass must preserve face enrichment

[MSIH]

Closes #276

Summary

The two photo-enrichment passes overwrote each other's work, and only one of them knew it. face-worker.js already preserved captions (it reads the caption cache and rebuilds base + caption + "Pictured: …"), but caption-worker.js posted a bare extra: { captioned: true } — and since ingest replaces text_repr and extra wholesale (doc 04 §3, no deep-merge), that destroyed any faces_detected / pictured the face pass had written.

Both workers now build their payload through one shared lib/photo-payload.js, so either pass may run first, or they may interleave, without losing the other's output.

This matters right now because #217 (face pass) runs first and #275 (descriptions) runs second — so this data loss was on the critical path. #217 carried a final "re-run the face pass to restore what #275 overwrote" step purely because of this asymmetry; that step can now be dropped.

Changes

• lib/photo-payload.js (new) — buildPhotoPayload() (the single payload shape), picturedNames() (moved verbatim from face-worker.js), and readFaceEnrichment() (face state + clusters → relPath -> {faces, pictured} lookup).
• face-worker.js — buildPayload now delegates to the shared builder; local picturedNames deleted. Behavior byte-identical (existing tests are the guard).
• caption-worker.js — reads the face state once at startup and carries face data through its upsert.
• README.md — "Wave order matters" rewritten: either order is now safe, with the two remaining properties (absent≠zero, keep state co-located) documented.
• test.mjs — 3 new tests.

Design decisions

• One shared builder, not a second copy. The root cause was two scripts independently constructing the same payload with only one complete. Copying the face-reading logic into the caption worker would have recreated the same drift. This mirrors the existing lib/describe.js convention ("used by every script so they can never drift").
• Absent ≠ zero. With no face data, faces_detected is omitted, never sent as 0 — 0 would assert "detection ran and found nothing," which is false and, given the wholesale replace, permanent. Explicitly asserted in tests.
• The caption worker re-sends entity_hints. Redundant server-side (hints are idempotent), but it makes both workers converge on a byte-identical payload. Without it the face worker's signature check would see a difference after the caption pass and re-post the entire library — a whole extra ingest wave.

Verification

• npm test — 31/32. The 1 failure (face-worker: scan clusters…) is the pre-existing Windows/Node-v26 libuv crash seen in feat(photo-exif): swap face stack from tfjs-node to onnxruntime-node #273/feat(photo-exif): suggest cluster labels from single-name Google Photos sidecars #274; it reproduces identically on unmodified main and is green on CI.
• New coverage: face data preserved through a caption run; absence asserted as absent, not 0; and a face → caption → face round trip asserting both workers converge byte-for-byte (this is the regression guard against future drift).
• All 28 pre-existing tests pass unchanged — the face-worker refactor is behavior-neutral.
• npm run check:boundary — clean (29 connector files, no src/ import).

Test plan

• npm test passes (aside from the pre-existing flake noted above)
• npm run check:boundary passes
• Follow-up after merge: drop the "re-run the face pass" step from chore(photo-exif): tag people in the photo library (face pass) #217

[Copilot]

Pull request overview

This PR fixes a data-loss bug in the photo-exif connector where the caption-enrichment pass could overwrite face-enrichment output due to ingest’s wholesale replacement of text_repr and extra. It centralizes photo-ingest payload construction so both enrichment workers always post the union of known fields, making the two passes order-independent.

Changes:

• Added lib/photo-payload.js to provide a shared buildPhotoPayload() plus helpers to read face enrichment and derive pictured names.
• Refactored face-worker.js and caption-worker.js to use the shared payload builder so they no longer clobber each other’s fields.
• Updated docs and added regression tests to assert preservation of face enrichment and “absent ≠ zero” semantics.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
connectors/photo-exif/lib/photo-payload.js	New shared builder for consistent ingest payloads and face-enrichment carry-through
connectors/photo-exif/face-worker.js	Refactor to delegate payload construction to shared builder
connectors/photo-exif/caption-worker.js	Now reads face state and posts a full union payload via the shared builder
connectors/photo-exif/test.mjs	Adds regression tests covering preservation + round-trip convergence
connectors/photo-exif/README.md	Updates “wave order” guidance to reflect order-independence

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[MSIH]

Resolving Copilot's review (2 comments). Both accepted — fixed in 3263004.

1. lib/photo-payload.js:63 — unguarded clusters read

Accepted, but the stated mechanism was wrong. parseClustersFile already has its own try/catch (face-cluster.js:45-49), so a malformed clusters file body returns {version: 0, clusters: []} rather than throwing.

The finding still lands, for a different reason: the readFileSync itself was unguarded — while the face-state read three lines above it was wrapped. That asymmetry was mine, and it's a real crash path: EBUSY if the face worker is mid-write (likely on Windows), EISDIR, or EACCES.

Fixed — but not by returning an empty pictured, which would have introduced the exact bug this PR exists to prevent. Claiming pictured: [] asserts "nobody named is in this photo," and the wholesale extra replace would make that false claim permanent. So the distinction is now:

• Absent clusters file → legitimate ("face pass ran, nothing named yet") → pictured: [], matching what face-worker.js itself sends in that state.
• Unreadable clusters file → anomaly → log loudly and return an empty lookup, so both faces_detected and pictured are omitted. Partial face data is not sent.

Regression test added using a directory at the clusters path (portable way to force a read error), asserting the run exits 0, logs the anomaly, and omits both fields.

2. README.md:122 — "interleaved" overclaims

Accepted, straightforwardly correct. I wrote "may run in either order, or be interleaved." The second half is false: each worker snapshots the other's state once at startup, so a label applied mid-caption-run isn't seen and gets overwritten by that run's upsert. Corrected to "either order, one at a time," with the snapshot behavior spelled out as the reason.

Good catch — that would have been a documented guarantee the code doesn't make.