diff --git a/DOCS-add-photo.md b/DOCS-add-photo.md new file mode 100644 index 0000000..d3c4dc6 --- /dev/null +++ b/DOCS-add-photo.md @@ -0,0 +1,92 @@ +# Adding a photo from the chat: `!add_photo` + +How a member of an album's group puts a photo into it by sending one, and +what keeps that path from leaking anything. The album itself — where it +lives, what its manifests look like, how several albums coexist — is +`DOCS-photo-pack.md`. + +Send a photo captioned `!add_photo alfa wakacje2019` in one of the album's +groups. `alfa` resolves to that person's key; `wakacje2019` matches nobody and +is minted as a custom tag. The photo is retrievable through `!alfa` and +`!random wakacje2019` **immediately**, with no restart. + +What holds: + +- **No typed label travels.** Labels become keys in the registry's + `arg_resolver`, at the dispatcher, *before* the line that logs the command + name. The reply is counts and pseudonyms — `dodano · p1: 4 · t1: 1`. +- **No cap.** No cooldown, no size limit, no daily limit. `!random` takes the + ordinary pack cooldown; `!add_photo` takes no decorator at all. +- **At most six labels**, the same ceiling `!alfa beta …` has. +- **The attachment must be on the same message**, never a quoted one: a reply + carries the *quoted* message's media in `quote.attachments`, and taking it + would let anyone re-file somebody else's photo. +- **A duplicate changes nothing.** The stored name is a content hash, so a + photo already in the album is reported and skipped; a mixed batch stores the + new files and says how many were already there. +- **Nothing is written until it works.** Media, `tags.json`, `custom.json` and + `adds.json` are written, the album is reloaded, and only a load that + succeeds is published. A load that fails rolls all four back and keeps + serving the album that was already working. +- **The second copy goes.** signal-cli keeps its own copy of every inbound + attachment outside the album's `0700`. `!add_photo` deletes it once the file + is stored, and also when the upload was refused. + +### Stripping: exiftool, with a positive check + +`ffmpeg -map_metadata -1 -c copy` was tried first and **fails silently on +photos**: given a JPEG with a real APP1 Exif segment it returned a +byte-identical file, GPS marker intact, and exited 0. `-c copy` passes the +compressed frame through as an opaque packet and a JPEG's Exif lives inside +it. Do not reintroduce ffmpeg for this. + +So the gate is a **positive check on the output**, never an exit code: + +```sh +exiftool -all= -o # never --overwrite_original on the temp +exiftool -G0 -s -all # then read it back +``` + +and every surviving tag must be **structural**. Measured against exiftool +13.55, the plan's original "the surviving set must be empty" would have +rejected every format except JPEG — a valid PNG always reports its IHDR under +`PNG`, a GIF its screen descriptor under `GIF`, an MP4 some fifty `QuickTime` +tags that *are* the container. So the allowlist is per **tag name** +(`signal_bot/media_tags.py`), which keeps the property that matters: an atom +nobody anticipated arrives as a name that is not on the list, and the upload +is refused. + +Allowlist, not blocklist, for the reason a naive gate misses: an MP4 geotag is +a QuickTime `udta`/`©xyz` atom reported under **`QuickTime`**, not `GPS`. + +The list includes the **codec's own configuration** — `CompressorName`, +`VendorID`, and the HEVC parameter set (`HEVCConfigurationVersion`, +`GeneralProfileIDC`, `BitDepthLuma`, …). That was forced by measurement, not +chosen for convenience: with them off the list exiftool 13.55 could produce no +acceptable video *and* no acceptable HEIC, because `CompressorName` lives in +the `avc1` sample description and exiftool reports it as not writable at all +(`Sorry, QuickTime:CompressorName doesn't exist or isn't writable`), and a real +HEIC reports sixteen HEVC config tags. Since HEIC is the iPhone default, the +choice was codec parameters or no phone photos. They describe how the picture +is encoded, never where it was taken or who took it; every location and +identity tag is still refused, and `scripts/e2e.sh` proves a QuickTime GPS atom +does not survive on a file the album actually stored. + +Formats are decided by **magic bytes**, never by the filename: jpg, png, gif, +webp, mp4, mov, m4v, heic, heif, avif, webm and svg. Every one of them must +also be in `pack.MEDIA_TYPES`, or a file that uploaded cleanly is refused by +the *next* load and takes startup down. + +### ⚠️ .webm and .svg are stored unstripped + +exiftool 13.55 can write neither (`exiftool -listwf` lists no `WEBM` and no +`SVG`; a strip attempt returns `Can't write SVG files`). Its SVG *reader* does +not surface `` content either, so there is no detection to fall back +on. + +kuhy's decision of 2026-08-31 is to keep both on the allowlist and store them +**as they arrive, metadata included**. The consequence, stated plainly: an +`.svg` carrying `` or an embedded +`` keeps whatever is in it, and +every later `!alfa` re-sends that file verbatim. Every other format on the +list is stripped and verified. diff --git a/DOCS-commands.md b/DOCS-commands.md index d6572f2..43535db 100644 --- a/DOCS-commands.md +++ b/DOCS-commands.md @@ -36,7 +36,7 @@ data: adding one should never mean touching Python. only on an installation that has an album at all — everywhere else they are not commands, not refused and never logged. Both take arguments, and those arguments are resolved to pseudonym keys by the registry before any handler, -log line or `command_uses` row sees them. See `DOCS-photo-pack.md`. +log line or `command_uses` row sees them. See `DOCS-photo-pack.md` and `DOCS-add-photo.md`. ## Why `!pomoc` shows the first alias and not a trigger diff --git a/DOCS-photo-pack.md b/DOCS-photo-pack.md index c074e1d..6bd023f 100644 --- a/DOCS-photo-pack.md +++ b/DOCS-photo-pack.md @@ -15,6 +15,8 @@ gamma — so the mechanism can be public while the album stays private. ~/.local/share/signal-bot/pack/ people.json # the commands, and the only place names exist tags.json # which file shows whom + custom.json # labels !add_photo minted (optional) + adds.json # who added what, read by nothing (optional) media/ # the photos and videos themselves ``` @@ -35,6 +37,32 @@ Two absences, deliberately different: be honoured, **fails startup**. `load_config` raising is an answer, and a bot that came up healthy and said "no such photo" forever would not be. +### More than one album + +`PHOTO_PACK_DIR` takes a **comma-separated list**, the same spelling +`GROUP_IDS` and `TRUSTED_INVITERS` use: + +```sh +PHOTO_PACK_DIR=~/.local/share/signal-bot/pack,~/.local/share/signal-bot/pack-testbot +``` + +Each album carries its own group scope in its own `people.json`, and the two +share nothing but the process: separate people, separate tags, separate +custom labels. That is what lets a test group have working `!add_photo` and +`!losowe` over invented people while the real album does not exist there at +all — which one album cannot do, because its `groups` list *is* the privacy +guarantee. + +Trigger collisions between albums are therefore legal. Two albums are two +sets of people and both may call somebody `!alfa`; no message can ever mean +both, because a command a group is not scoped to is not a command there. +`!dodaj` and `!losowe` are registered once per album for exactly this reason. + +**Two albums whose groups overlap are refused at startup** with +`DuplicateTriggerError`. Overlapping scopes would make "which album does +`!dodaj` write to?" a coin toss, and a coin toss about which private album +receives a photo is the outcome this whole design exists to prevent. + ## people.json ```json @@ -113,91 +141,11 @@ is handled exactly like the photos. ## Adding from the chat: !add_photo -Send a photo captioned `!add_photo alfa wakacje2019` in one of the album's -groups. `alfa` resolves to that person's key; `wakacje2019` matches nobody and -is minted as a custom tag. The photo is retrievable through `!alfa` and -`!random wakacje2019` **immediately**, with no restart. - -What holds: - -- **No typed label travels.** Labels become keys in the registry's - `arg_resolver`, at the dispatcher, *before* the line that logs the command - name. The reply is counts and pseudonyms — `dodano · p1: 4 · t1: 1`. -- **No cap.** No cooldown, no size limit, no daily limit. `!random` takes the - ordinary pack cooldown; `!add_photo` takes no decorator at all. -- **At most six labels**, the same ceiling `!alfa beta …` has. -- **The attachment must be on the same message**, never a quoted one: a reply - carries the *quoted* message's media in `quote.attachments`, and taking it - would let anyone re-file somebody else's photo. -- **A duplicate changes nothing.** The stored name is a content hash, so a - photo already in the album is reported and skipped; a mixed batch stores the - new files and says how many were already there. -- **Nothing is written until it works.** Media, `tags.json`, `custom.json` and - `adds.json` are written, the album is reloaded, and only a load that - succeeds is published. A load that fails rolls all four back and keeps - serving the album that was already working. -- **The second copy goes.** signal-cli keeps its own copy of every inbound - attachment outside the album's `0700`. `!add_photo` deletes it once the file - is stored, and also when the upload was refused. - -### Stripping: exiftool, with a positive check - -`ffmpeg -map_metadata -1 -c copy` was tried first and **fails silently on -photos**: given a JPEG with a real APP1 Exif segment it returned a -byte-identical file, GPS marker intact, and exited 0. `-c copy` passes the -compressed frame through as an opaque packet and a JPEG's Exif lives inside -it. Do not reintroduce ffmpeg for this. - -So the gate is a **positive check on the output**, never an exit code: - -```sh -exiftool -all= -o # never --overwrite_original on the temp -exiftool -G0 -s -all # then read it back -``` - -and every surviving tag must be **structural**. Measured against exiftool -13.55, the plan's original "the surviving set must be empty" would have -rejected every format except JPEG — a valid PNG always reports its IHDR under -`PNG`, a GIF its screen descriptor under `GIF`, an MP4 some fifty `QuickTime` -tags that *are* the container. So the allowlist is per **tag name** -(`signal_bot/media_tags.py`), which keeps the property that matters: an atom -nobody anticipated arrives as a name that is not on the list, and the upload -is refused. - -Allowlist, not blocklist, for the reason a naive gate misses: an MP4 geotag is -a QuickTime `udta`/`©xyz` atom reported under **`QuickTime`**, not `GPS`. - -The list includes the **codec's own configuration** — `CompressorName`, -`VendorID`, and the HEVC parameter set (`HEVCConfigurationVersion`, -`GeneralProfileIDC`, `BitDepthLuma`, …). That was forced by measurement, not -chosen for convenience: with them off the list exiftool 13.55 could produce no -acceptable video *and* no acceptable HEIC, because `CompressorName` lives in -the `avc1` sample description and exiftool reports it as not writable at all -(`Sorry, QuickTime:CompressorName doesn't exist or isn't writable`), and a real -HEIC reports sixteen HEVC config tags. Since HEIC is the iPhone default, the -choice was codec parameters or no phone photos. They describe how the picture -is encoded, never where it was taken or who took it; every location and -identity tag is still refused, and `scripts/e2e.sh` proves a QuickTime GPS atom -does not survive on a file the album actually stored. - -Formats are decided by **magic bytes**, never by the filename: jpg, png, gif, -webp, mp4, mov, m4v, heic, heif, avif, webm and svg. Every one of them must -also be in `pack.MEDIA_TYPES`, or a file that uploaded cleanly is refused by -the *next* load and takes startup down. - -### ⚠️ .webm and .svg are stored unstripped - -exiftool 13.55 can write neither (`exiftool -listwf` lists no `WEBM` and no -`SVG`; a strip attempt returns `Can't write SVG files`). Its SVG *reader* does -not surface `` content either, so there is no detection to fall back -on. - -kuhy's decision of 2026-08-31 is to keep both on the allowlist and store them -**as they arrive, metadata included**. The consequence, stated plainly: an -`.svg` carrying `` or an embedded -`` keeps whatever is in it, and -every later `!alfa` re-sends that file verbatim. Every other format on the -list is stripped and verified. +A member of one of the album's groups can send a photo captioned +`!add_photo alfa wakacje` and have it land in that album, tagged, stripped +and retrievable immediately. That path has its own document, because the +metadata handling and the privacy invariants it has to keep are longer than +the album format itself: **`DOCS-add-photo.md`**. ## Filling it in: tool/tagger.py diff --git a/signal_bot/animals.py b/signal_bot/animals.py index 204b8e3..b396a83 100644 --- a/signal_bot/animals.py +++ b/signal_bot/animals.py @@ -14,9 +14,10 @@ from signal_bot.config import DEFAULT_CAT_API_URL, DEFAULT_DOG_API_URL, DEFAULT_ if TYPE_CHECKING: from collections.abc import Sequence + from signal_bot.context import CommandContext from signal_bot.decorators import Decorator from signal_bot.http_client import ImageFetcher - from signal_bot.registry import CommandContext, CommandRegistry, Handler + from signal_bot.registry import CommandRegistry, Handler ALIAS_FILE: Final = Path(__file__).with_name("aliases.json") diff --git a/signal_bot/bot.py b/signal_bot/bot.py index 2d5999e..adda38c 100644 --- a/signal_bot/bot.py +++ b/signal_bot/bot.py @@ -201,8 +201,10 @@ def build_bot( # Absent unless this installation has an album, and scoped to that # album's own groups when it does -- so on every other machine, and in # every other group, these commands simply do not exist. - if config.photo_pack_dir is not None: - _register_album(registry, config.photo_pack_dir, rate_limit) + # "album", not "directory": build_bot's own `directory` is the group + # directory, and shadowing it here silently passed the wrong object. + for album in config.photo_pack_dirs: + _register_album(registry, album, rate_limit) # Last, so !pomoc trails the list it prints; it reads the registry # when it runs, so the order is presentation only. register_help_command(registry, decorate=rate_limit) diff --git a/signal_bot/config.py b/signal_bot/config.py index f198924..f331a2e 100644 --- a/signal_bot/config.py +++ b/signal_bot/config.py @@ -96,10 +96,14 @@ class Config: # Lowered in the e2e run so the deafness gate can be proven against the # real process rather than only in unit tests. silence_seconds: int = DEFAULT_SILENCE_SECONDS - # The private photo album, or None for an installation without one. Only - # the path is config; everything inside it -- who the people are, what - # fires their photos -- is data this repository never sees. - photo_pack_dir: Path | None = None + # The private photo albums, empty for an installation without any. Only + # the paths are config; everything inside them -- who the people are, + # what fires their photos -- is data this repository never sees. + # + # Several, because each album carries its own group scope: a test group + # can have invented people without the real album existing there at all. + # Two albums whose groups overlap are refused at startup, by the registry. + photo_pack_dirs: tuple[Path, ...] = () @property def is_pinned(self) -> bool: diff --git a/signal_bot/config_env.py b/signal_bot/config_env.py index 1251d46..c3ca05e 100644 --- a/signal_bot/config_env.py +++ b/signal_bot/config_env.py @@ -41,7 +41,7 @@ from signal_bot.endpoints import ( DEFAULT_GECKO_API_URL, ) from signal_bot.groups import ConfigError, parse_group_ids -from signal_bot.pack import PEOPLE_FILE, default_dir +from signal_bot.pack import PEOPLE_FILE, default_dirs from signal_bot.pack_load import load as load_pack from signal_bot.trust import parse_trusted_inviters @@ -105,24 +105,32 @@ def _bind_port(env: Mapping[str, str]) -> int: return bind_port -def _photo_pack_dir() -> Path | None: - """The album directory, loaded once here to prove it can be served. +def _photo_pack_dirs() -> tuple[Path, ...]: + """Every album directory, each loaded once here to prove it can be served. Two different absences, on purpose. A directory the operator *named* and that is not there is a typo, and startup says so. A directory nobody named and that is not there is simply an installation with no album, and the commands do not exist -- which is also what every test gets. - The pack is parsed and thrown away rather than trusted: a manifest that + Each pack is parsed and thrown away rather than trusted: a manifest that cannot be honoured should fail here, where load_config raising is an answer, and not later inside a retry loop that would hide it. """ - directory, explicit = default_dir() + return tuple( + directory + for directory, explicit in default_dirs() + if _servable(directory, explicit=explicit) + ) + + +def _servable(directory: Path, *, explicit: bool) -> bool: + """Whether one album can be served, raising when a named one cannot.""" if not directory.is_dir(): if explicit: msg = f"PHOTO_PACK_DIR={directory} is not a directory" raise ConfigError(msg) - return None + return False if not (directory / PEOPLE_FILE).is_file(): # A directory the tagger has created but not filled in yet. Only an # explicitly named pack may fail here: the implicit path is a place a @@ -131,9 +139,9 @@ def _photo_pack_dir() -> Path | None: if explicit: msg = f"PHOTO_PACK_DIR={directory} has no {PEOPLE_FILE}" raise ConfigError(msg) - return None + return False load_pack(directory) - return directory + return True def load_config(env: dict[str, str] | None = None, *, use_dotenv: bool = True) -> Config: @@ -169,5 +177,5 @@ def load_config(env: dict[str, str] | None = None, *, use_dotenv: bool = True) - log_level=_log_level(env), trusted_inviters=parse_trusted_inviters(env), silence_seconds=_positive_int(env, "SILENCE_SECONDS", DEFAULT_SILENCE_SECONDS), - photo_pack_dir=_photo_pack_dir(), + photo_pack_dirs=_photo_pack_dirs(), ) diff --git a/signal_bot/context.py b/signal_bot/context.py new file mode 100644 index 0000000..08cca9d --- /dev/null +++ b/signal_bot/context.py @@ -0,0 +1,69 @@ +# Copyright (c) 2026 Krzysztof Rudnicki. MIT licence. +"""What a command handler is allowed to touch, and how it answers. + +Split out of registry.py at the 250-line cap. The division is real rather +than clerical: that file is "which word runs what", this one is "what the +thing that runs may do" -- and the reply helpers are the only place in the +codebase that decides *where* an answer goes. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import TYPE_CHECKING + +from signal_bot.quoting import quote_of + +if TYPE_CHECKING: + from signal_bot.config import Config + from signal_bot.models import IncomingMessage + from signal_bot.store import Store + from signal_bot.transport_base import Transport + + +@dataclass(frozen=True, slots=True) +class CommandContext: + """Everything a command handler is allowed to touch.""" + + message: IncomingMessage + group_id: str + transport: Transport + store: Store + config: Config + command_name: str + now: float + day: str + # Whatever followed the trigger, split on whitespace. Empty for every + # command that does not opt into taking any -- see Command.takes_args. + args: tuple[str, ...] = () + + async def reply(self, text: str) -> None: + """Answer the message that triggered this command, in its group. + + There is no configurable reply target. A command answers where it + was asked, which is the only routing that stays correct once the + bot watches a test group and a production group at once -- and it + quotes the asker, so a busy group can tell which "!cat" this is. + """ + await self.transport.send_text(text, self.group_id, quote_of(self.message)) + + async def reply_image(self, image_base64: str, caption: str = "") -> None: + """Answer the triggering message with an image, quoting it.""" + await self.transport.send_image( + image_base64, self.group_id, quote_of(self.message), caption + ) + + async def reply_media(self, payload_base64: str, media_type: str, filename: str) -> None: + """Answer with an attachment that is not necessarily a photo. + + Separate from reply_image because Signal decides how to render an + attachment from what it is told it is, and "a jpeg" is a lie that + makes a video arrive as a file nobody can play. + """ + await self.transport.send_media( + payload_base64, + self.group_id, + quote_of(self.message), + media_type=media_type, + filename=filename, + ) diff --git a/signal_bot/decorators.py b/signal_bot/decorators.py index 49434b4..ea1431d 100644 --- a/signal_bot/decorators.py +++ b/signal_bot/decorators.py @@ -17,10 +17,14 @@ import functools import logging import math from collections.abc import Callable +from typing import TYPE_CHECKING -from signal_bot.registry import CommandContext, CommandRefusedError, Handler +from signal_bot.registry import CommandRefusedError, Handler from signal_bot.stats.text import plural +if TYPE_CHECKING: + from signal_bot.context import CommandContext + logger = logging.getLogger(__name__) Decorator = Callable[[Handler], Handler] diff --git a/signal_bot/dispatcher.py b/signal_bot/dispatcher.py index 0e6b2a1..e71257d 100644 --- a/signal_bot/dispatcher.py +++ b/signal_bot/dispatcher.py @@ -9,10 +9,11 @@ from collections.abc import Callable from dataclasses import dataclass from typing import TYPE_CHECKING +from signal_bot.context import CommandContext from signal_bot.counting import should_count, watched_group from signal_bot.envelope_shape import shape_of from signal_bot.models import parse_envelope -from signal_bot.registry import CommandContext, CommandRefusedError, CommandRegistry +from signal_bot.registry import CommandRefusedError, CommandRegistry from signal_bot.stats.recorder import record_message if TYPE_CHECKING: diff --git a/signal_bot/help_command.py b/signal_bot/help_command.py index cd61dc1..f60b6cd 100644 --- a/signal_bot/help_command.py +++ b/signal_bot/help_command.py @@ -25,8 +25,9 @@ from signal_bot.pack_command import NAME_PREFIX if TYPE_CHECKING: from collections.abc import Sequence + from signal_bot.context import CommandContext from signal_bot.decorators import Decorator - from signal_bot.registry import Command, CommandContext, CommandRegistry + from signal_bot.registry import Command, CommandRegistry COMMAND_NAME: Final = "help" DESCRIPTION: Final = "ta lista" diff --git a/signal_bot/media_sniff.py b/signal_bot/media_sniff.py index 5d2b018..9768539 100644 --- a/signal_bot/media_sniff.py +++ b/signal_bot/media_sniff.py @@ -44,7 +44,7 @@ class MediaKind: media_type: str # Whether exiftool 13.55 can write this container at all. False means the # file is stored exactly as it arrived -- see media_strip.strip_bytes and - # the warning in DOCS-photo-pack.md. + # the warning in DOCS-add-photo.md. strippable: bool = True diff --git a/signal_bot/media_strip.py b/signal_bot/media_strip.py index 71fa24b..712d27d 100644 --- a/signal_bot/media_strip.py +++ b/signal_bot/media_strip.py @@ -111,7 +111,7 @@ class ExiftoolStripper: # kuhy's decision of 2026-08-31: .webm and .svg stay on the # allowlist even though exiftool 13.55 can write neither, so they # are stored exactly as they arrived, metadata included. Flagged - # at the time and recorded in DOCS-photo-pack.md. + # at the time and recorded in DOCS-add-photo.md. logger.warning("storing %s unstripped: exiftool cannot write it", kind.suffix) return payload if self._exiftool is None: diff --git a/signal_bot/pack.py b/signal_bot/pack.py index 55e219c..75cee02 100644 --- a/signal_bot/pack.py +++ b/signal_bot/pack.py @@ -115,8 +115,23 @@ def media_type(filename: str) -> str: return MEDIA_TYPES[Path(filename).suffix.lower()] -def default_dir() -> tuple[Path, bool]: - """Where the pack lives, and whether the operator named it explicitly. +def default_dirs() -> tuple[tuple[Path, bool], ...]: + """Every album directory, and whether the operator named it explicitly. + + A list, because one bot can serve several albums -- each scoped to its + own groups, which is what lets a test group have its own invented people + without the real album being readable there. PHOTO_PACK_DIR takes a + comma-separated list, the same spelling GROUP_IDS and TRUSTED_INVITERS + already use. + """ + explicit = os.environ.get("PHOTO_PACK_DIR", "").strip() + if explicit: + return tuple((Path(part.strip()), True) for part in explicit.split(",") if part.strip()) + return (_implicit_dir(),) + + +def _implicit_dir() -> tuple[Path, bool]: + """Where the pack lives when nobody said, and whether that was explicit. Explicit means a missing directory is an error worth failing startup for. The implicit paths are the opposite: they are where a pack *would* live, @@ -129,9 +144,6 @@ def default_dir() -> tuple[Path, bool]: anything under $HOME because that is the directory systemd creates and exposes regardless of ProtectHome. """ - explicit = os.environ.get("PHOTO_PACK_DIR", "").strip() - if explicit: - return Path(explicit), True state = os.environ.get("STATE_DIRECTORY", "").strip() if state: return Path(state) / "pack", False diff --git a/signal_bot/pack_add.py b/signal_bot/pack_add.py index 473d61e..ad2f525 100644 --- a/signal_bot/pack_add.py +++ b/signal_bot/pack_add.py @@ -40,11 +40,12 @@ from signal_bot.pack_replies import ( ) if TYPE_CHECKING: + from signal_bot.context import CommandContext from signal_bot.media_strip import Stripper from signal_bot.pack import Pack from signal_bot.pack_custom import CustomTags from signal_bot.pack_holder import PackHolder - from signal_bot.registry import ArgResolver, CommandContext, CommandRegistry, Handler + from signal_bot.registry import ArgResolver, CommandRegistry, Handler logger = logging.getLogger(__name__) diff --git a/signal_bot/pack_command.py b/signal_bot/pack_command.py index 2f5357b..6a7795a 100644 --- a/signal_bot/pack_command.py +++ b/signal_bot/pack_command.py @@ -25,10 +25,11 @@ from signal_bot.pack import candidate_keysets, media_type if TYPE_CHECKING: from collections.abc import Sequence + from signal_bot.context import CommandContext from signal_bot.decorators import Decorator from signal_bot.pack import Entry from signal_bot.pack_holder import PackHolder - from signal_bot.registry import CommandContext, CommandRegistry, Handler + from signal_bot.registry import CommandRegistry, Handler # The prefix every pack command's name carries, so one glance at a log line # or a command_uses row says where it came from without saying who. diff --git a/signal_bot/pack_ingest.py b/signal_bot/pack_ingest.py index b5fc799..9f80031 100644 --- a/signal_bot/pack_ingest.py +++ b/signal_bot/pack_ingest.py @@ -30,10 +30,10 @@ from signal_bot.pack_store import ( ) if TYPE_CHECKING: + from signal_bot.context import CommandContext from signal_bot.media_strip import Stripper from signal_bot.pack_add import AddResolver from signal_bot.pack_holder import PackHolder - from signal_bot.registry import CommandContext logger = logging.getLogger(__name__) diff --git a/signal_bot/pack_random.py b/signal_bot/pack_random.py index 6f1d59a..0198163 100644 --- a/signal_bot/pack_random.py +++ b/signal_bot/pack_random.py @@ -24,9 +24,10 @@ from signal_bot.pack import MAX_NAMES, media_type from signal_bot.pack_command import NAME_PREFIX, NO_PHOTO, UNKNOWN, pick if TYPE_CHECKING: + from signal_bot.context import CommandContext from signal_bot.decorators import Decorator from signal_bot.pack_holder import PackHolder - from signal_bot.registry import ArgResolver, CommandContext, CommandRegistry, Handler + from signal_bot.registry import ArgResolver, CommandRegistry, Handler COMMAND_NAME: Final = f"{NAME_PREFIX}random" diff --git a/signal_bot/popes_command.py b/signal_bot/popes_command.py index a9be5e5..f4fe1a6 100644 --- a/signal_bot/popes_command.py +++ b/signal_bot/popes_command.py @@ -14,9 +14,10 @@ from signal_bot.animals import load_aliases from signal_bot.versioning import POPES, article_url, pope_line if TYPE_CHECKING: + from signal_bot.context import CommandContext from signal_bot.decorators import Decorator from signal_bot.portraits import PortraitCache - from signal_bot.registry import CommandContext, CommandRegistry, Handler + from signal_bot.registry import CommandRegistry, Handler from signal_bot.versioning import Pope COMMAND_NAME: Final = "pope" diff --git a/signal_bot/registry.py b/signal_bot/registry.py index 5a12c41..ebc229b 100644 --- a/signal_bot/registry.py +++ b/signal_bot/registry.py @@ -10,68 +10,13 @@ from __future__ import annotations from collections.abc import Awaitable, Callable, Iterable from dataclasses import dataclass -from typing import TYPE_CHECKING +from signal_bot.context import CommandContext from signal_bot.folding import fold -from signal_bot.quoting import quote_of - -if TYPE_CHECKING: - from signal_bot.config import Config - from signal_bot.models import IncomingMessage - from signal_bot.store import Store - from signal_bot.transport_base import Transport COMMAND_PREFIX = "!" -@dataclass(frozen=True, slots=True) -class CommandContext: - """Everything a command handler is allowed to touch.""" - - message: IncomingMessage - group_id: str - transport: Transport - store: Store - config: Config - command_name: str - now: float - day: str - # Whatever followed the trigger, split on whitespace. Empty for every - # command that does not opt into taking any -- see Command.takes_args. - args: tuple[str, ...] = () - - async def reply(self, text: str) -> None: - """Answer the message that triggered this command, in its group. - - There is no configurable reply target. A command answers where it - was asked, which is the only routing that stays correct once the - bot watches a test group and a production group at once -- and it - quotes the asker, so a busy group can tell which "!cat" this is. - """ - await self.transport.send_text(text, self.group_id, quote_of(self.message)) - - async def reply_image(self, image_base64: str, caption: str = "") -> None: - """Answer the triggering message with an image, quoting it.""" - await self.transport.send_image( - image_base64, self.group_id, quote_of(self.message), caption - ) - - async def reply_media(self, payload_base64: str, media_type: str, filename: str) -> None: - """Answer with an attachment that is not necessarily a photo. - - Separate from reply_image because Signal decides how to render an - attachment from what it is told it is, and "a jpeg" is a lie that - makes a video arrive as a file nobody can play. - """ - await self.transport.send_media( - payload_base64, - self.group_id, - quote_of(self.message), - media_type=media_type, - filename=filename, - ) - - Handler = Callable[[CommandContext], Awaitable[None]] # Turns the words after a trigger into whatever the handler is allowed to see. @@ -118,7 +63,14 @@ class Command: class DuplicateTriggerError(ValueError): - """Raised when two commands claim the same trigger word.""" + """Raised when two commands that can both run claim the same trigger word. + + "Can both run" is the whole test. Two albums scoped to different groups + may each define "!alfa" -- they are different people, and no message can + ever mean both, because a command a group is not scoped to is not a + command there at all. A collision is only a collision where the group + sets overlap, and then it is fatal at startup rather than a coin toss. + """ class CommandRefusedError(Exception): @@ -144,7 +96,9 @@ class CommandRegistry: def __init__(self) -> None: """Start with no commands registered.""" - self._by_trigger: dict[str, Command] = {} + # One trigger can be claimed by several commands, as long as no two + # of them are ever live in the same group. See DuplicateTriggerError. + self._by_trigger: dict[str, list[Command]] = {} self._commands: list[Command] = [] def register( @@ -166,8 +120,12 @@ class CommandRegistry: raw = list(triggers) normalised = frozenset(fold(t) for t in raw) for trigger in sorted(normalised): - if trigger in self._by_trigger: - msg = f"trigger {trigger!r} is already registered" + clash = next( + (c for c in self._by_trigger.get(trigger, ()) if _overlap(c.groups, groups)), + None, + ) + if clash is not None: + msg = f"trigger {trigger!r} is already registered by {clash.name!r}" raise DuplicateTriggerError(msg) command = Command( name=name, @@ -180,12 +138,28 @@ class CommandRegistry: arg_resolver=arg_resolver, ) for trigger in normalised: - self._by_trigger[trigger] = command + self._by_trigger.setdefault(trigger, []).append(command) self._commands.append(command) return command - def resolve(self, text: str | None) -> Command | None: - """Return the command a message body triggers, or None.""" + def _claimant(self, trigger: str, group_id: str | None) -> Command | None: + """The command that owns one folded trigger, in one group. + + ``group_id`` None means "anywhere", which is what a caller asking + "is this word a command at all?" wants -- pack_add uses it to refuse + minting a label that already means something. + """ + claimants = self._by_trigger.get(trigger, []) + if group_id is None: + return claimants[0] if claimants else None + return next((c for c in claimants if c.allowed_in(group_id)), None) + + def resolve(self, text: str | None, group_id: str | None = None) -> Command | None: + """Return the command a message body triggers in one group, or None. + + Without a group this answers "does anything claim this word", which + is a different and deliberately broader question. + """ if not text: return None # Folded, not just lowercased: a Cyrillic "o" is a different @@ -193,7 +167,7 @@ class CommandRegistry: candidate = fold(text) if not candidate.startswith(COMMAND_PREFIX): return None - return self._by_trigger.get(candidate) + return self._claimant(candidate, group_id) def resolve_with_args(self, text: str | None, group_id: str) -> Resolved | None: """Resolve a message in one group, with anything that followed it. @@ -208,14 +182,14 @@ class CommandRegistry: """ if not text: return None - command = self.resolve(text) + command = self.resolve(text, group_id) if command is not None: - return Resolved(command, ()) if command.allowed_in(group_id) else None + return Resolved(command, ()) words = text.split() if not words: return None - head = self._by_trigger.get(fold(words[0])) - if head is None or not head.takes_args or not head.allowed_in(group_id): + head = self._claimant(fold(words[0]), group_id) + if head is None or not head.takes_args: return None tail = tuple(words[1:]) # Applied here rather than in the handler, which is the whole point: @@ -229,3 +203,14 @@ class CommandRegistry: def commands(self) -> list[Command]: """Every registered command, in registration order.""" return list(self._commands) + + +def _overlap(one: frozenset[str] | None, other: frozenset[str] | None) -> bool: + """Whether two group scopes can ever both be live for one message. + + None means "every group", so it overlaps with anything -- including + another None. + """ + if one is None or other is None: + return True + return not one.isdisjoint(other) diff --git a/signal_bot/stats/commands.py b/signal_bot/stats/commands.py index b054fdb..19e51b8 100644 --- a/signal_bot/stats/commands.py +++ b/signal_bot/stats/commands.py @@ -42,8 +42,9 @@ from signal_bot.stats.render_extra import ( from signal_bot.stats.render_period import render_period if TYPE_CHECKING: + from signal_bot.context import CommandContext from signal_bot.decorators import Decorator - from signal_bot.registry import CommandContext, CommandRegistry, Handler + from signal_bot.registry import CommandRegistry, Handler from signal_bot.stats.periods import Period diff --git a/tests/gate_helpers.py b/tests/gate_helpers.py index 96f7200..beaf8e6 100644 --- a/tests/gate_helpers.py +++ b/tests/gate_helpers.py @@ -7,8 +7,9 @@ from typing import TYPE_CHECKING import pytest +from signal_bot.context import CommandContext from signal_bot.models import parse_envelope -from signal_bot.registry import CommandContext, CommandRefusedError, Handler +from signal_bot.registry import CommandRefusedError, Handler from tests.conftest import WATCHED_GROUP, envelope if TYPE_CHECKING: diff --git a/tests/pack_add_fixture.py b/tests/pack_add_fixture.py index 0f14f46..84d9748 100644 --- a/tests/pack_add_fixture.py +++ b/tests/pack_add_fixture.py @@ -16,11 +16,12 @@ from typing import TYPE_CHECKING import pytest +from signal_bot.context import CommandContext from signal_bot.models import parse_envelope from signal_bot.pack_add import AddResolver, register_add_command from signal_bot.pack_holder import PackHolder from signal_bot.pack_load import load -from signal_bot.registry import CommandContext, CommandRegistry +from signal_bot.registry import CommandRegistry from signal_bot.transport_fake import FakeTransport from tests import pack_fixture from tests.conftest import WATCHED_GROUP, envelope diff --git a/tests/test_animals.py b/tests/test_animals.py index 70b0996..ca892e6 100644 --- a/tests/test_animals.py +++ b/tests/test_animals.py @@ -16,10 +16,11 @@ from signal_bot.animals import ( paged_url, register_animal_commands, ) +from signal_bot.context import CommandContext from signal_bot.decorators import DISPLAY_MARGIN_SECONDS, cooldown from signal_bot.http_client import ImageFetcher from signal_bot.models import parse_envelope -from signal_bot.registry import CommandContext, CommandRefusedError, CommandRegistry +from signal_bot.registry import CommandRefusedError, CommandRegistry from tests.conftest import WATCHED_GROUP, envelope if TYPE_CHECKING: diff --git a/tests/test_bot.py b/tests/test_bot.py index 9dc191f..ba7c712 100644 --- a/tests/test_bot.py +++ b/tests/test_bot.py @@ -100,7 +100,7 @@ def test_an_album_is_wired_up_only_where_there_is_one( root = pack_fixture.build(tmp_path / "pack") with_album = build_bot( - replace(config, photo_pack_dir=root), + replace(config, photo_pack_dirs=(root,)), client, store, transport, diff --git a/tests/test_config_pack.py b/tests/test_config_pack.py index 9636456..dbf2873 100644 --- a/tests/test_config_pack.py +++ b/tests/test_config_pack.py @@ -21,13 +21,13 @@ if TYPE_CHECKING: def test_an_installation_with_no_album_has_no_pack(tmp_path: Path) -> None: """Which is every machine but one, and every test in this suite.""" - assert load_config(dict(BASE_ENV)).photo_pack_dir is None + assert load_config(dict(BASE_ENV)).photo_pack_dirs == () def test_a_named_album_is_loaded_and_kept(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: root = pack_fixture.build(tmp_path / "pack") monkeypatch.setenv("PHOTO_PACK_DIR", str(root)) - assert load_config(dict(BASE_ENV)).photo_pack_dir == root + assert load_config(dict(BASE_ENV)).photo_pack_dirs == (root,) def test_a_named_album_that_is_not_there_fails_startup( @@ -55,7 +55,7 @@ def test_the_album_may_also_be_found_where_systemd_puts_it( ) -> None: pack_fixture.build(tmp_path / "state" / "pack") monkeypatch.setenv("STATE_DIRECTORY", str(tmp_path / "state")) - assert load_config(dict(BASE_ENV)).photo_pack_dir == tmp_path / "state" / "pack" + assert load_config(dict(BASE_ENV)).photo_pack_dirs == (tmp_path / "state" / "pack",) def test_an_empty_album_directory_is_not_a_crash_loop( @@ -69,7 +69,7 @@ def test_an_empty_album_directory_is_not_a_crash_loop( """ (tmp_path / "share" / "signal-bot" / "pack" / "media").mkdir(parents=True) monkeypatch.setenv("XDG_DATA_HOME", str(tmp_path / "share")) - assert load_config(dict(BASE_ENV)).photo_pack_dir is None + assert load_config(dict(BASE_ENV)).photo_pack_dirs == () def test_a_named_album_with_no_manifest_fails_startup( diff --git a/tests/test_cooldown_gate.py b/tests/test_cooldown_gate.py index 4e02f42..427dc1b 100644 --- a/tests/test_cooldown_gate.py +++ b/tests/test_cooldown_gate.py @@ -13,7 +13,8 @@ from tests.gate_helpers import DAY, gate_context, refused if TYPE_CHECKING: from signal_bot.config import Config - from signal_bot.registry import CommandContext, Handler + from signal_bot.context import CommandContext + from signal_bot.registry import Handler from signal_bot.store import Store from signal_bot.transport_fake import FakeTransport diff --git a/tests/test_decorators.py b/tests/test_decorators.py index 8470f8f..90ad32a 100644 --- a/tests/test_decorators.py +++ b/tests/test_decorators.py @@ -14,7 +14,8 @@ from tests.gate_helpers import DAY, gate_context, refused if TYPE_CHECKING: from signal_bot.config import Config - from signal_bot.registry import CommandContext, Handler + from signal_bot.context import CommandContext + from signal_bot.registry import Handler from signal_bot.store import Store from signal_bot.transport_fake import FakeTransport diff --git a/tests/test_dispatcher.py b/tests/test_dispatcher.py index cd0d2f0..89b90fe 100644 --- a/tests/test_dispatcher.py +++ b/tests/test_dispatcher.py @@ -9,11 +9,12 @@ from typing import TYPE_CHECKING import pytest from signal_bot.dispatcher import Dispatcher -from signal_bot.registry import CommandContext, CommandRegistry +from signal_bot.registry import CommandRegistry from tests.conftest import OTHER_GROUP, SECOND_GROUP, WATCHED_GROUP, envelope if TYPE_CHECKING: from signal_bot.config import Config + from signal_bot.context import CommandContext from signal_bot.discovery import GroupDirectory from signal_bot.store import Store from signal_bot.transport_fake import FakeTransport diff --git a/tests/test_dispatcher_gates.py b/tests/test_dispatcher_gates.py index 0d31266..a3063b0 100644 --- a/tests/test_dispatcher_gates.py +++ b/tests/test_dispatcher_gates.py @@ -8,12 +8,13 @@ from typing import TYPE_CHECKING from signal_bot.decorators import cooldown from signal_bot.dispatcher import Dispatcher -from signal_bot.registry import CommandContext, CommandRegistry +from signal_bot.registry import CommandRegistry from signal_bot.stats.queries_day import day_rows from tests.conftest import SECOND_GROUP, WATCHED_GROUP, envelope if TYPE_CHECKING: from signal_bot.config import Config + from signal_bot.context import CommandContext from signal_bot.discovery import GroupDirectory from signal_bot.store import Store from signal_bot.transport_fake import FakeTransport diff --git a/tests/test_help_command.py b/tests/test_help_command.py index bc093c3..d6ed2d4 100644 --- a/tests/test_help_command.py +++ b/tests/test_help_command.py @@ -10,6 +10,7 @@ import pytest from signal_bot.animals import register_animal_commands from signal_bot.config import DEFAULT_COMMONS_API_URL +from signal_bot.context import CommandContext from signal_bot.help_command import ( ALBUM_HEADING, COMMAND_NAME, @@ -29,7 +30,7 @@ from signal_bot.pack_load import load as load_pack from signal_bot.pack_random import register_random_command from signal_bot.popes_command import register_pope_command from signal_bot.portraits import PortraitCache -from signal_bot.registry import CommandContext, CommandRegistry +from signal_bot.registry import CommandRegistry from signal_bot.stats.commands import register_stats_commands from tests import pack_fixture from tests.conftest import SECOND_GROUP, WATCHED_GROUP, envelope diff --git a/tests/test_multiple_albums.py b/tests/test_multiple_albums.py new file mode 100644 index 0000000..20e3bb4 --- /dev/null +++ b/tests/test_multiple_albums.py @@ -0,0 +1,187 @@ +# Copyright (c) 2026 Krzysztof Rudnicki. MIT licence. +"""Two albums at once, each scoped to its own groups. + +The reason this exists: a test group wants working `!add_photo` and `!losowe` +without the *real* album being readable there. One album cannot do that -- its +`groups` list is the whole privacy guarantee -- so the bot loads several, and +a command from one album simply does not exist in the other's groups. + +Which makes trigger collisions legal for the first time. Two albums are two +different sets of people, and both may reasonably call somebody "!alfa"; no +message can ever mean both, because a command a group is not scoped to is not +a command there. A collision is only fatal where the group sets *overlap*. +""" + +from __future__ import annotations + +import json +from dataclasses import replace +from typing import TYPE_CHECKING + +import httpx +import pytest + +from signal_bot.bot import build_bot +from signal_bot.liveness import Liveness +from signal_bot.registry import DuplicateTriggerError +from tests import pack_fixture +from tests.conftest import SECOND_GROUP, WATCHED_GROUP + +if TYPE_CHECKING: + from pathlib import Path + + from signal_bot.config import Config + from signal_bot.discovery import GroupDirectory + from signal_bot.registry import Command, CommandRegistry + from signal_bot.store import Store + from signal_bot.transport_fake import FakeTransport + + +def _album(root: Path, group: str, *, key: str = "p1", trigger: str = "!alfa") -> Path: + """A one-person album scoped to exactly one group.""" + return pack_fixture.build( + root, + people=[{"key": key, "triggers": [trigger]}], + combos=[], + tags={"alfa.png": [key]}, + groups=[group], + ) + + +def _registry( + config: Config, + store: Store, + transport: FakeTransport, + directory: GroupDirectory, + albums: tuple[Path, ...], +) -> CommandRegistry: + """Build the production object graph over these albums.""" + client = httpx.AsyncClient(transport=httpx.MockTransport(lambda _: httpx.Response(200))) + bot = build_bot( + replace(config, photo_pack_dirs=albums), + client, + store, + transport, + directory, + Liveness(config, client), + ) + return bot.dispatcher.registry + + +def _claim(registry: CommandRegistry, text: str, group: str) -> Command | None: + resolved = registry.resolve_with_args(text, group) + return resolved.command if resolved is not None else None + + +def test_each_album_answers_only_in_its_own_group( + tmp_path: Path, + config: Config, + store: Store, + transport: FakeTransport, + directory: GroupDirectory, +) -> None: + real = _album(tmp_path / "real", WATCHED_GROUP, key="p1", trigger="!alfa") + test = _album(tmp_path / "test", SECOND_GROUP, key="p9", trigger="!zeta") + registry = _registry(config, store, transport, directory, (real, test)) + + assert _claim(registry, "!alfa", WATCHED_GROUP) is not None + assert _claim(registry, "!alfa", SECOND_GROUP) is None + assert _claim(registry, "!zeta", SECOND_GROUP) is not None + assert _claim(registry, "!zeta", WATCHED_GROUP) is None + + +def test_two_albums_may_share_a_trigger_when_their_groups_do_not_overlap( + tmp_path: Path, + config: Config, + store: Store, + transport: FakeTransport, + directory: GroupDirectory, +) -> None: + """Two different people, both called "!alfa". No message can mean both.""" + real = _album(tmp_path / "real", WATCHED_GROUP, key="p1", trigger="!alfa") + test = _album(tmp_path / "test", SECOND_GROUP, key="p1", trigger="!alfa") + registry = _registry(config, store, transport, directory, (real, test)) + + first = _claim(registry, "!alfa", WATCHED_GROUP) + second = _claim(registry, "!alfa", SECOND_GROUP) + assert first is not None + assert second is not None + assert first is not second + + +def test_add_photo_and_random_exist_separately_in_each_album( + tmp_path: Path, + config: Config, + store: Store, + transport: FakeTransport, + directory: GroupDirectory, +) -> None: + """!dodaj and !losowe carry the same triggers in both, by construction.""" + real = _album(tmp_path / "real", WATCHED_GROUP) + test = _album(tmp_path / "test", SECOND_GROUP, key="p9", trigger="!zeta") + registry = _registry(config, store, transport, directory, (real, test)) + + for group in (WATCHED_GROUP, SECOND_GROUP): + assert _claim(registry, "!dodaj alfa", group) is not None + assert _claim(registry, "!losowe", group) is not None + + +def test_two_albums_sharing_a_group_are_refused_at_startup( + tmp_path: Path, + config: Config, + store: Store, + transport: FakeTransport, + directory: GroupDirectory, +) -> None: + """Fail loudly rather than let one message mean two things. + + Overlapping scopes make "which album does !dodaj write to?" a coin toss, + and a coin toss about which private album gets a photo is the one outcome + this whole feature exists to prevent. + """ + one = _album(tmp_path / "one", WATCHED_GROUP) + two = _album(tmp_path / "two", WATCHED_GROUP, key="p9", trigger="!zeta") + with pytest.raises(DuplicateTriggerError): + _registry(config, store, transport, directory, (one, two)) + + +def test_an_album_added_second_does_not_disturb_the_first( + tmp_path: Path, + config: Config, + store: Store, + transport: FakeTransport, + directory: GroupDirectory, +) -> None: + """The real group's photos must answer exactly as they did before.""" + real = _album(tmp_path / "real", WATCHED_GROUP) + alone = _registry(config, store, transport, directory, (real,)) + before = {c.name for c in alone.commands if c.allowed_in(WATCHED_GROUP)} + + test = _album(tmp_path / "test", SECOND_GROUP, key="p9", trigger="!zeta") + both = _registry(config, store, transport, directory, (real, test)) + after = {c.name for c in both.commands if c.allowed_in(WATCHED_GROUP)} + + assert before == after + + +def test_a_custom_tag_minted_in_one_album_is_invisible_in_the_other( + tmp_path: Path, + config: Config, + store: Store, + transport: FakeTransport, + directory: GroupDirectory, +) -> None: + """Albums share no data at all, only the process they run in.""" + real = _album(tmp_path / "real", WATCHED_GROUP) + test = _album(tmp_path / "test", SECOND_GROUP, key="p9", trigger="!zeta") + (test / "custom.json").write_text(json.dumps({"t1": ["wakacje"]}), encoding="utf-8") + registry = _registry(config, store, transport, directory, (real, test)) + + # Resolved to a key in the album that has it, and to the "no such thing" + # sentinel in the one that does not. + in_test = registry.resolve_with_args("!losowe wakacje", SECOND_GROUP) + in_real = registry.resolve_with_args("!losowe wakacje", WATCHED_GROUP) + assert in_test is not None + assert in_real is not None + assert in_test.args == ("t1",) + assert in_real.args == ("?",) diff --git a/tests/test_pack.py b/tests/test_pack.py index 40a1203..5fa8210 100644 --- a/tests/test_pack.py +++ b/tests/test_pack.py @@ -8,7 +8,13 @@ from typing import TYPE_CHECKING import pytest -from signal_bot.pack import MAX_NAMES, candidate_keysets, default_dir, media_type, options_for +from signal_bot.pack import ( + MAX_NAMES, + candidate_keysets, + default_dirs, + media_type, + options_for, +) from signal_bot.pack_load import load from tests import pack_fixture @@ -43,19 +49,34 @@ def test_media_path_is_inside_the_album(pack: Pack) -> None: def test_an_explicit_directory_is_taken_at_its_word(monkeypatch: pytest.MonkeyPatch) -> None: """And is reported as explicit, which is what makes its absence an error.""" monkeypatch.setenv("PHOTO_PACK_DIR", "/srv/album") - assert default_dir() == (Path("/srv/album"), True) + assert default_dirs() == ((Path("/srv/album"), True),) + + +def test_several_albums_can_be_named_at_once(monkeypatch: pytest.MonkeyPatch) -> None: + """One bot, several albums, each carrying its own group scope. + + Comma-separated, the same spelling GROUP_IDS and TRUSTED_INVITERS use. + """ + monkeypatch.setenv("PHOTO_PACK_DIR", "/srv/album, /srv/testalbum ") + assert default_dirs() == ((Path("/srv/album"), True), (Path("/srv/testalbum"), True)) + + +def test_blank_entries_in_the_list_are_dropped(monkeypatch: pytest.MonkeyPatch) -> None: + """A trailing comma is a typo, not a request to load the current directory.""" + monkeypatch.setenv("PHOTO_PACK_DIR", "/srv/album,,") + assert default_dirs() == ((Path("/srv/album"), True),) def test_systemd_beats_anything_under_home(monkeypatch: pytest.MonkeyPatch) -> None: """ProtectHome=read-only is why: StateDirectory is exposed regardless.""" monkeypatch.setenv("STATE_DIRECTORY", "/var/lib/signal-bot") - assert default_dir() == (Path("/var/lib/signal-bot/pack"), False) + assert default_dirs() == ((Path("/var/lib/signal-bot/pack"), False),) def test_the_xdg_fallback_needs_no_environment(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.delenv("XDG_DATA_HOME", raising=False) monkeypatch.setattr(Path, "home", lambda: Path("/home/somebody")) - assert default_dir() == (Path("/home/somebody/.local/share/signal-bot/pack"), False) + assert default_dirs() == ((Path("/home/somebody/.local/share/signal-bot/pack"), False),) def test_a_name_resolves_with_or_without_the_bang(pack: Pack) -> None: diff --git a/tests/test_pack_command.py b/tests/test_pack_command.py index b8cbc0c..94aa6b6 100644 --- a/tests/test_pack_command.py +++ b/tests/test_pack_command.py @@ -8,6 +8,7 @@ from typing import TYPE_CHECKING import pytest +from signal_bot.context import CommandContext from signal_bot.models import parse_envelope from signal_bot.pack_command import ( EITHER, @@ -19,7 +20,7 @@ from signal_bot.pack_command import ( ) from signal_bot.pack_holder import PackHolder from signal_bot.pack_load import load -from signal_bot.registry import CommandContext, CommandRegistry +from signal_bot.registry import CommandRegistry from tests import pack_fixture from tests.conftest import WATCHED_GROUP, envelope diff --git a/tests/test_pack_random.py b/tests/test_pack_random.py index 33d0a42..eb173de 100644 --- a/tests/test_pack_random.py +++ b/tests/test_pack_random.py @@ -8,13 +8,14 @@ from typing import TYPE_CHECKING import pytest +from signal_bot.context import CommandContext from signal_bot.models import parse_envelope from signal_bot.pack_command import NO_PHOTO from signal_bot.pack_custom import CUSTOM_FILE from signal_bot.pack_holder import PackHolder from signal_bot.pack_load import load from signal_bot.pack_random import COMMAND_NAME, register_random_command -from signal_bot.registry import CommandContext, CommandRegistry +from signal_bot.registry import CommandRegistry from tests import pack_fixture from tests.conftest import SECOND_GROUP, WATCHED_GROUP, envelope diff --git a/tests/test_popes_command.py b/tests/test_popes_command.py index d6c353e..759c676 100644 --- a/tests/test_popes_command.py +++ b/tests/test_popes_command.py @@ -10,6 +10,7 @@ from typing import TYPE_CHECKING import httpx import pytest +from signal_bot.context import CommandContext from signal_bot.decorators import DISPLAY_MARGIN_SECONDS, cooldown from signal_bot.http_client import ImageFetcher from signal_bot.models import parse_envelope @@ -21,7 +22,7 @@ from signal_bot.popes_command import ( register_pope_command, ) from signal_bot.portraits import PortraitCache -from signal_bot.registry import CommandContext, CommandRefusedError, CommandRegistry +from signal_bot.registry import CommandRefusedError, CommandRegistry from signal_bot.versioning import POPES from tests.conftest import WATCHED_GROUP, envelope diff --git a/tests/test_registry.py b/tests/test_registry.py index f810dbf..f12c4d4 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -7,8 +7,9 @@ from typing import TYPE_CHECKING import pytest +from signal_bot.context import CommandContext from signal_bot.models import parse_envelope -from signal_bot.registry import CommandContext, CommandRegistry, DuplicateTriggerError +from signal_bot.registry import CommandRegistry, DuplicateTriggerError from signal_bot.transport_fake import FakeTransport from tests.conftest import WATCHED_GROUP, envelope diff --git a/tests/test_selftest_attachments.py b/tests/test_selftest_attachments.py index fc42b30..82f79ca 100644 --- a/tests/test_selftest_attachments.py +++ b/tests/test_selftest_attachments.py @@ -25,8 +25,8 @@ from tests.conftest import WATCHED_GROUP if TYPE_CHECKING: from signal_bot.bot import Bot from signal_bot.config import Config + from signal_bot.context import CommandContext from signal_bot.discovery import GroupDirectory - from signal_bot.registry import CommandContext from signal_bot.store import Store from signal_bot.transport_fake import FakeTransport diff --git a/tool/e2e_fixtures.py b/tool/e2e_fixtures.py index 3d8a561..0a6db9b 100755 --- a/tool/e2e_fixtures.py +++ b/tool/e2e_fixtures.py @@ -25,7 +25,7 @@ PROBE_CREATOR: Final = "A Real Person Name" # An SVG carrying both things exiftool cannot help with: a metadata block and # a whole embedded raster. Stored as-is, per the decision recorded in -# DOCS-photo-pack.md -- this exists so the e2e run *demonstrates* that hole +# DOCS-add-photo.md -- this exists so the e2e run *demonstrates* that hole # rather than leaving it to the prose. SVG_DOCUMENT: Final = ( ''