From e070fd542557badec9690a8de5709820258a791e Mon Sep 17 00:00:00 2001
From: Krzysztof kuhy Rudnicki <krzysztofrudnicki0@gmail.com>
Date: Mon, 31 Aug 2026 08:13:48 +0200
Subject: [PATCH] docs: plan the !add_photo command for a later session

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 TODO-add-photo.md | 207 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 207 insertions(+)
 create mode 100644 TODO-add-photo.md

diff --git a/TODO-add-photo.md b/TODO-add-photo.md
new file mode 100644
index 0000000..0cd2afa
--- /dev/null
+++ b/TODO-add-photo.md
@@ -0,0 +1,207 @@
+# TODO: `!add_photo` — add an attachment to the pack, tagged
+
+REMOVE ME AFTER FINISH
+
+Planned 2026-08-30, not implemented. Every decision below was settled with kuhy
+during planning; they are answers, not suggestions. Do not re-litigate them.
+
+## Context
+
+signal-bot has a private photo album (the "pack") outside the repo at
+`~/.local/share/signal-bot/pack/`. Today the only way to put a photo into it is
+`tool/tagger.py`, a browser tool run by hand on the PC — so every new photo means
+leaving Signal, moving the file to the machine, and running a tagger session.
+
+This change lets a member of the pack's group add a photo *from the chat*: send a
+photo captioned `!add_photo alfa beta` and it lands in the album tagged with
+both, immediately retrievable through each of their existing commands. A label
+matching nobody becomes a **custom tag**, reachable through a new `!random <label>`.
+
+The constraint is unchanged and absolute: **no name and no photo may reach the
+repository, the journal, the SQLite database, a reply body, or any group outside
+the pack's own.** `DOCS-photo-pack.md` and the personal-data pre-commit gate exist
+because that invariant was broken once already.
+
+## Decisions
+
+| | |
+|---|---|
+| New people | `!add_photo` never writes a person into `people.json`. An unmatched label becomes a custom tag. |
+| Photo source | Attachment on the **same** message. Never a quote/reply — `quote` carries its own `attachments` array. |
+| Who may run it | Anyone in the pack's groups. No operator concept. |
+| Scrubbing | Labels resolve to keys **at the dispatcher**; the raw remainder never travels further. |
+| Availability | Immediate: atomic write + validated hot-swap. No restart. |
+| Formats | Explicit allowlist, **magic-byte sniffed**: jpg, jpeg, png, gif, mp4 + heic, heif, webp, mov, webm, m4v, avif, svg. |
+| Stripping | `exiftool`, with a **positive** post-check. See below. |
+| Filename | Content hash. The original filename is discarded. |
+| Reply | Counts and pseudonyms only: `dodano · p1: 4 · t3: 1`. On a first custom-tag mint the reply also explains, generically, that the new tag is reached via `!random` — without echoing the label. |
+| Limits | **None** on `!add_photo` — no size cap, no daily cap, no cooldown. |
+| Undo | Out of scope. See "Deferred" below. |
+| Unit | `ReadWritePaths=%h/.local/share/signal-bot` added. The pack does not move. |
+
+### Why exiftool and not ffmpeg
+
+The first choice was `ffmpeg -map_metadata -1 -c copy`. It was tested and **fails
+silently on photos**: given a JPEG carrying a real APP1 Exif segment it produced a
+byte-identical file (292 → 292 bytes, GPS marker intact) and exited 0. `-c copy`
+copies the compressed frame as an opaque packet and JPEG's Exif lives inside it.
+That ffmpeg also has no HEIF/HEIC demuxer (`ffmpeg -demuxers | grep -ci heif` → 0).
+**Do not reintroduce ffmpeg for this.**
+
+Use `perl-image-exiftool` (Arch `extra`, 13.55-1), which covers the whole allowlist
+losslessly. The gate is a **positive check on the output**, not an exit code — an
+exit code is exactly what lied:
+
+```
+exiftool -all= -o <dst> <src>          # never --overwrite_original on the temp
+exiftool -G0 -s -all <dst>
+reject unless the surviving tag set is empty apart from the computed/filesystem
+pseudo-groups File, Composite, ExifTool
+```
+
+**Allowlist, not blocklist.** The obvious gate — grepping `-Exif:all -GPS:all
+-XMP:all` — is blind to exactly the case that matters most: an MP4/MOV geotag lives
+in the QuickTime `udta`/`©xyz` atom and is reported under group **`QuickTime`**, not
+`GPS`. Enumerating groups to look for is how the next one gets missed; requiring the
+set to be empty cannot be. Verify at implementation time that exiftool's MP4 writer
+honours `-all=` on this version — if it does not, video is rejected rather than
+stored.
+
+SVG has no magic bytes, so sniffing special-cases a leading `<?xml` / `<svg` prefix.
+It is **not** exempt from the check: SVG can carry `<metadata>` RDF and can embed a
+whole raster (`<image xlink:href="data:image/jpeg;base64,…">`) with its EXIF intact.
+exiftool has an SVG reader, so SVG runs the same strip-and-verify as everything else.
+
+**Temp files go in `/tmp`**, which the unit already makes per-service via
+`PrivateTmp=true`. Staging next to `media/` would let a crash mid-strip leave an
+unstripped original inside the album permanently, where nothing in `pack_load` would
+ever flag it.
+
+### The second copy
+
+signal-cli keeps its own copy of every inbound attachment in the container's
+attachment store, outside the album's `0700` permissions. That is pre-existing for
+every photo the group sends, but this feature is the first path that ingests one
+deliberately, so it is in scope for "inaccessible from outside":
+`Transport.delete_attachment` already exists and is called nowhere in production —
+`!add_photo` calls it once the file is safely stored, whether the upload was accepted
+or rejected.
+
+## Implementation
+
+### New modules
+
+| File | Contents |
+|---|---|
+| `signal_bot/pack_custom.py` | `custom.json` load/mint/write. Labels folded like person triggers; keys `t1`, `t2`, …; atomic temp-then-`Path.replace` as `tool/tagger.py::Album.write` already does. |
+| `signal_bot/pack_add.py` | The `!add_photo` handler: fetch → sniff → strip → verify → hash-name → write `media/` → merge `tags.json` → hot-swap → reply. |
+| `signal_bot/pack_random.py` | The `!random` handler and its non-minting resolver. |
+| `signal_bot/media_sniff.py` | Magic-byte detection over the allowlist + the SVG prefix case; maps to suffix and MIME. |
+| `signal_bot/media_strip.py` | `Stripper` protocol and the real exiftool implementation. |
+| `signal_bot/pack_holder.py` | The mutable holder (below). |
+
+`pack_load.py` is at 243 of the 250-line cap, so its custom-tag validation moves
+into `pack_custom.py` rather than growing it. `pack.py` is at 155.
+
+### The scrubbing seam (load-bearing)
+
+`registry.resolve_with_args` currently returns `Resolved(head, tuple(words[1:]))` —
+raw words straight into `CommandContext.args`. Add an optional per-command
+`arg_resolver` set at registration; `resolve_with_args` applies it and yields keys
+only, so no handler, log line or `command_uses` row ever sees a typed label.
+
+Two resolvers, deliberately different:
+
+- **`pack:add`** — minting. Known label → the person's key; unknown label → a *pending*
+  `t`-key held in memory only. Nothing is written until the upload succeeds; a
+  rejected upload or a missing attachment discards the pending mint and leaves
+  `custom.json` untouched. `!add_photo dog` with no attachment must change nothing
+  on disk.
+- **`pack:random`** — non-minting. Unknown label → `None` → the existing `NO_PHOTO`
+  line. If it shared the minting resolver every typo on `!random` would mint a key.
+
+### `!random` semantics
+
+No arguments: a random photo from the whole album. Two or more labels: **intersection**
+(reuse `Pack.matching`, which is already a superset test), ceiling `MAX_NAMES = 6`.
+`!random` resolves **custom tags only, never people** — `!alfa` already exists for
+that, and a second entrance with an unscoped argument list is one too many.
+
+### Hot-swap
+
+`pack_command.pack_handler` closes over the frozen `Pack` (`pack_command.py:62-78`),
+so a newly-tagged photo would stay invisible to the existing `pack:p1` handler.
+Introduce a `PackHolder` with a mutable `.pack` that **every** pack handler reads at
+call time. `bot.py::build_bot` constructs the holder once and passes it where it
+passes `pack` today.
+
+Swap order: write media atomically → write `tags.json`/`custom.json` atomically →
+re-run `pack_load.load` → on success assign `holder.pack`; **on failure roll back the
+files and keep serving the old pack.** Never raise: `Restart=always` would loop.
+
+Custom tags never become commands, so a swap only ever replaces *Pack data* — it
+never re-registers commands or recomputes `Command.groups`. That is what makes
+immediate availability cheap, and it is the property to preserve.
+
+### Gaps that must be filled
+
+- `models.py::_attachments` keeps only `contentType` and discards the attachment
+  `id`. It must carry the id without breaking `counting.media_kind` / `should_count`.
+- `Transport` (`transport_base.py:58-62`) has `list_attachments` / `delete_attachment`
+  but no fetch. Add `fetch_attachment(attachment_id) -> bytes` to the Protocol,
+  `RestTransport` (`GET /v1/attachments/{attachment}`, verified present on the live
+  container's `/swagger/doc.json`) and `FakeTransport`. `tests/test_transport_base.py`
+  asserts the Protocol's member names and must be updated.
+- `fake_signal` cannot inject an inbound attachment at all: no GET-by-id route,
+  `state.py` stores no bytes, and `scenario.py::Injection` has no attachment field
+  (`_build` rejects unknown keys). All three need extending or `scripts/e2e.sh`
+  cannot prove any of this offline.
+- `.svg` → `image/svg+xml` must join `pack.MEDIA_TYPES`, because
+  `pack_load._media_file` validates the suffix against it and a stored SVG would
+  otherwise fail the *next* load and take startup down.
+- `scripts/personal_data_sources.py::_pack_names` must read `custom.json` labels
+  alongside `people.json` triggers — a label can be a name (`!add_photo alfa_wedding`).
+- Any new SQLite table uses `CREATE TABLE IF NOT EXISTS` and must **not** bump
+  `SCHEMA_VERSION` (a bump DROPs the ephemeral tables).
+- `scripts/install_service.sh` gains the `perl-image-exiftool` dependency (there is
+  no `install.sh`; that script is the installer).
+- `DOCS-photo-pack.md` and `DOCS-commands.md` both need updating.
+
+### Assumption to confirm with kuhy before coding
+
+`!random` is a new command and inherits the standard pack cooldown decorator;
+`!add_photo` gets no decorator at all, per "NO CAP, NO LIMITS".
+
+## Verification
+
+1. **Unit** — `pytest`, which enforces 100% branch coverage with no omits and no
+   suppressions. The stripper **must** be an injected `Stripper` seam so unit tests
+   use a fake: `tests/pack_fixture.py` builds packs from a 1×1 PNG and a *fake* mp4
+   (`CLIP`) that real exiftool would reject. New tests assert, explicitly, that a
+   typed label appears in **no log record, no `command_uses` row and no reply body**.
+2. **End-to-end** — new `scripts/e2e.sh` scenarios against the extended `fake_signal`:
+   add tags an existing person and `!<name>` returns it; add mints a custom tag and
+   `!random <label>` returns it; two labels tag both; `!add_photo` with no attachment
+   changes nothing on disk; a rejected file is not stored; the same commands in another
+   group produce **zero** sends.
+3. **Real exiftool** — exercised only in `e2e.sh`, against generated media, covering
+   all three regressions this plan came from:
+   - a JPEG with a real APP1 Exif segment comes out with the marker gone;
+   - an MP4 written with `exiftool -GPSCoordinates=…` comes out with **no** surviving
+     `QuickTime` group tag (the case a naive `-GPS:all` gate would pass);
+   - an SVG with `<metadata>` and an embedded base64 JPEG is rejected or cleaned.
+4. **The second copy** — after an add, assert the attachment is gone from the fake's
+   store, so "inaccessible from outside" is tested and not just asserted.
+5. **Live** — `pre-commit run --all-files`, restart the unit, then in the real group
+   send one photo captioned `!add_photo <existing person>` and confirm the reply is
+   counts-only and the person's own command returns the new photo without a restart.
+   Confirm with `journalctl --user -u signal-bot` that no label appears in the log.
+
+## Deferred (kuhy's Q15: "no functionality like this please — store it in todo")
+
+`!undo_photo` — removing the most recent photo added by the calling user, within a
+short window. Not being built. It matters because anyone in the group may add and
+there is no removal path, so a bad upload is permanent until `tags.json` is edited by
+hand; `tool/tagger.py` is the escape hatch meanwhile. Implementing it later needs the
+adder's id recorded at add time, so consider recording it now even though nothing
+reads it yet.