From 318568ae6af2fbe4766d3a597cb58601d389de8e Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 21 Sep 2026 14:02:20 +0200 Subject: [PATCH 1/4] refactor(archivist): wire the bot's services into one bundle The archivist declared ten collaborators as `X | None = None` in the constructor and assigned them in `start()`. None was never a valid state once the bot ran, but the type said it was: every use was reported as a possible None access, and tests reached into the private attributes to swap in fakes. - `ArchivistServices`, a frozen dataclass, holds them. `classifier` and `mirror` stay optional because they are absent without an AI endpoint or Forgejo; everything else is required. - `_build_services()` is the wiring that used to live inline in `start()`; `_init_mirror()` becomes `_build_mirror()` and returns the mirror instead of setting it. - The constructor takes `services=` for callers that never start the bot. The archivist tests use it instead of assigning privates. No behaviour change. --- stacklets/docs/bot/archivist.py | 189 +++++++++++------- tests/stacklets/test_archivist_corrections.py | 18 +- tests/stacklets/test_archivist_routing.py | 10 +- tests/stacklets/test_archivist_source.py | 38 ++-- 4 files changed, 149 insertions(+), 106 deletions(-) diff --git a/stacklets/docs/bot/archivist.py b/stacklets/docs/bot/archivist.py index e9826da0..ab257f9a 100644 --- a/stacklets/docs/bot/archivist.py +++ b/stacklets/docs/bot/archivist.py @@ -31,6 +31,7 @@ import sys import time from contextlib import contextmanager +from dataclasses import dataclass from pathlib import Path from urllib.parse import urlsplit @@ -242,12 +243,39 @@ def _combine_images_to_pdf(files: list[tuple[str, bytes]]) -> bytes: # ── ArchivistBot ───────────────────────────────────────────────────────────── +@dataclass(frozen=True) +class ArchivistServices: + """Everything the archivist delegates work to. + + Built once by `ArchivistBot.start()`, or handed to the constructor by + a caller that never starts the bot (a test driving one handler). + `classifier` is None without an AI endpoint and `mirror` is None + without Forgejo; every other service is always present. + """ + + paperless: PaperlessAPI + classifier: Classifier | None + url_extractor: UrlExtractor + text_extractor: TextExtractor + capture_tags: CaptureTagCache + mirror: GitMirror | None + vault: VaultContext + pipeline: DocumentPipeline + search: SearchService + capture: CapturePipeline + + class ArchivistBot(MicroBot): """Document filing bot — watches a Matrix room for uploads, classifies them with an LLM, and files them in Paperless-ngx.""" name = "archivist-bot" + # Assigned by the constructor or by start(), never None. Reading it + # before either ran is a bug, and an AttributeError here names it + # where a None would travel on into a handler. + _services: ArchivistServices + # Filing is the slowest thing any famstack bot does, and the framework # default (180s) is a chat-bot budget, not a document-pipeline one. One # archived document costs, in sequence: the Paperless upload + OCR wait @@ -268,8 +296,13 @@ class ArchivistBot(MicroBot): # fails visibly (❌ + a threaded notice) and can be retried with 🔁. HANDLER_TIMEOUT_SECONDS = 1800 - def __init__(self, homeserver, user_id, password, session_dir, **settings): + def __init__( + self, homeserver, user_id, password, session_dir, *, + services: ArchivistServices | None = None, **settings, + ): super().__init__(homeserver, user_id, password, session_dir, **settings) + if services is not None: + self._services = services # Shared config from env vars — rendered by the CLI from stack.toml # URL contract: `*_url` is container-internal (compose service # hostname like `stack-paperless:8000` / `stack-code:3000`) and @@ -353,7 +386,6 @@ def __init__(self, homeserver, user_id, password, session_dir, **settings): self.capture_tag_prompt_size = int( settings.get("capture_tag_prompt_size", 50), ) - self._capture_tags: CaptureTagCache | None = None self._scan_sessions: dict[str, dict] = {} # Rooms known to be welcomed this session — caches the # own-messages history query in `_send_room_welcome_if_needed` @@ -362,16 +394,6 @@ def __init__(self, homeserver, user_id, password, session_dir, **settings): # Topic bindings resolved this session — caches the # own-messages history query in `_topic_binding` the same way. self._topic_bindings: dict[str, TopicBinding] = {} - self._http: aiohttp.ClientSession | None = None - self._paperless: PaperlessAPI | None = None - self._classifier: Classifier | None = None - self._url_extractor: UrlExtractor | None = None - self._text_extractor: TextExtractor | None = None - self._mirror: GitMirror | None = None - self._pipeline: DocumentPipeline | None = None - self._search: SearchService | None = None - self._capture: CapturePipeline | None = None - self._vault: VaultContext | None = None self._paperless_version: str = "" def t(self, key: str, **kwargs) -> str: @@ -397,19 +419,40 @@ async def start(self) -> None: except ValueError as e: logger.warning("[archivist] {}", e) - # Reuse the framework-owned aiohttp session (created here, before - # super().start() blocks in the sync loop) — one pool, closed by + if not hasattr(self, "_services"): + self._services = self._build_services() + # Warm the vision-capability cache on every boot. Previously + # this was kicked from on_first_sync, but MicroBot only runs + # that hook once across the lifetime of the welcome marker, + # so a restart never re-probed. Probe results are cached to + # disk inside Classifier, so this is a no-op if the cache + # is already populated. + classifier = self._services.classifier + if self.classify_enabled and classifier is not None: + asyncio.create_task(classifier.has_vision()) + # The framework's start() owns the session loop and closes the + # http session (via _aclose) on shutdown. + await super().start() + + def _build_services(self) -> ArchivistServices: + """Wire every service from the bot's config. + + Runs inside the event loop, before super().start() blocks in the + sync loop, because the HTTP session has to be created there. + """ + # Reuse the framework-owned aiohttp session — one pool, closed by # MicroBot on shutdown. Paperless / OpenAI / extractors share it. - self._http = self._ensure_http() - self._paperless = PaperlessAPI(self._http, self.paperless_url, self.paperless_token) + http = self._ensure_http() + paperless = PaperlessAPI(http, self.paperless_url, self.paperless_token) # Vision-capability cache lives in the bot's data dir so a probe # done in one container restart isn't repeated by the next one. # When OPENAI_URL is empty, leave the classifier unconfigured — # the bot still does filing / search / URL archiving, and the # framework refuses to construct against a missing endpoint so # we don't silently leak documents to api.openai.com. + classifier: Classifier | None = None if self.openai_url: - self._classifier = Classifier.from_endpoint( + classifier = Classifier.from_endpoint( self.openai_url, self.openai_key, bot_name=self.name, language=self.language, capabilities=ModelCapabilities( @@ -423,27 +466,26 @@ async def start(self) -> None: ) self.classify_enabled = False self.reformat_enabled = False - self._url_extractor = UrlExtractor(self._http) - self._text_extractor = TextExtractor() - self._capture_tags = CaptureTagCache( + url_extractor = UrlExtractor(http) + text_extractor = TextExtractor() + capture_tags = CaptureTagCache( self._session_dir / "capture-tags.json", ) - self._capture_tags.load() + capture_tags.load() logger.info( "[archivist] capture tag cache: {} tags (keep_body={})", - len(self._capture_tags.top(10_000)), + len(capture_tags.top(10_000)), self.capture_keep_body, ) # Always attempt to wire the memory vault writer. If - # CODE_URL / admin creds aren't present, `_init_mirror` - # leaves `self._mirror = None` and logs the reason; writes - # become silent skips. - self._init_mirror() - self._vault = VaultContext(language=self.language, shared_bucket=self.shared_bucket) - self._pipeline = DocumentPipeline( - paperless=self._paperless, - classifier=self._classifier, - mirror=self._mirror, + # CODE_URL / admin creds aren't present, `_build_mirror` + # returns None and logs the reason; writes become silent skips. + mirror = self._build_mirror() + vault = VaultContext(language=self.language, shared_bucket=self.shared_bucket) + pipeline = DocumentPipeline( + paperless=paperless, + classifier=classifier, + mirror=mirror, bot_name=self.name, language=self.language, classify_enabled=self.classify_enabled, @@ -454,47 +496,47 @@ async def start(self) -> None: paperless_public_url=self.paperless_public_url, link_base_url=self.link_base_url, actor=self.user_id, - vault=self._vault, + vault=vault, ) - self._search = SearchService( - classifier=self._classifier, - paperless=self._paperless, + search = SearchService( + classifier=classifier, + paperless=paperless, t=self.t, language=self.language, code_public_url=self.code_public_url, mirror_org=self.mirror_org, link_base_url=self.link_base_url, shared_bucket=self.shared_bucket, - vault=self._vault, + vault=vault, ) - capture_llm = self._classifier.llm if self._classifier is not None else None - self._capture = CapturePipeline( - url_extractor=self._url_extractor, - text_extractor=self._text_extractor, - classifier=self._classifier, - mirror=self._mirror, - capture_tags=self._capture_tags, - paperless=self._paperless, + capture = CapturePipeline( + url_extractor=url_extractor, + text_extractor=text_extractor, + classifier=classifier, + mirror=mirror, + capture_tags=capture_tags, + paperless=paperless, bot_name=self.name, classify_max_chars=self.classify_max_chars, capture_keep_body=self.capture_keep_body, capture_tag_prompt_size=self.capture_tag_prompt_size, vision_max_pdf_pages=self.vision_max_pdf_pages, - llm=capture_llm, + llm=classifier.llm if classifier is not None else None, + ) + return ArchivistServices( + paperless=paperless, + classifier=classifier, + url_extractor=url_extractor, + text_extractor=text_extractor, + capture_tags=capture_tags, + mirror=mirror, + vault=vault, + pipeline=pipeline, + search=search, + capture=capture, ) - # Warm the vision-capability cache on every boot. Previously - # this was kicked from on_first_sync, but MicroBot only runs - # that hook once across the lifetime of the welcome marker, - # so a restart never re-probed. Probe results are cached to - # disk inside Classifier, so this is a no-op if the cache - # is already populated. - if self.classify_enabled and self.openai_url: - asyncio.create_task(self._classifier.has_vision()) - # The framework's start() owns the session loop and closes the - # http session (via _aclose) on shutdown. - await super().start() - def _init_mirror(self) -> None: + def _build_mirror(self) -> GitMirror | None: """Build a GitMirror if all required env is present. Soft-fails: missing env just disables the mirror for this run. @@ -511,7 +553,7 @@ def _init_mirror(self) -> None: "[archivist] Memory vault writer offline — " "CODE_URL or admin creds missing. Bring up `code` to enable." ) - return + return None # @homer:homestead.me → homer admin_usernames = [] @@ -527,7 +569,7 @@ def _init_mirror(self) -> None: # runner mounts (`/data//bot`). Don't read DATA_DIR — # that env var carries the host path and would dump mirror state # outside the container's volume mount. - self._mirror = GitMirror( + mirror = GitMirror( code_url=code_url, admin_user=admin_user, admin_password=admin_password, @@ -538,6 +580,7 @@ def _init_mirror(self) -> None: ) logger.info("[archivist] Memory vault writer: {} org={} (admins: {})", code_url, self.mirror_org, ", ".join(admin_usernames) or "-") + return mirror def _ai_status(self) -> str: if self.openai_url: @@ -1294,7 +1337,7 @@ async def _handle_reply_reprocess( confirmation posted outside the thread would leave every later correction anchored to the original filing. """ - o = await self._pipeline.reprocess( + o = await self._services.pipeline.reprocess( doc_id=doc_id, user_hint=user_hint, date_filed=date_filed, initial_classification=initial_classification, ) @@ -1336,7 +1379,7 @@ async def _handle_reply_capture_reprocess( attach the fresh `capture.reclassified` envelope so the user can chain another correction by replying to THIS message. """ - outcome = await self._capture.reprocess( + outcome = await self._services.capture.reprocess( vault_path=vault_path, user_hint=user_hint, sender_mxid=sender_mxid, initial_classification=initial_classification, @@ -1366,16 +1409,16 @@ async def _process_document( pass the source type and header). Applied best-effort: an enrichment failure never fails a filing that otherwise succeeded. """ - outcome = await self._pipeline.process( + outcome = await self._services.pipeline.process( filename=filename, display_name=display_name, file_data=file_data, date_filed=date_filed, submitter_mxid=submitter_mxid, user_hint=user_hint, ) - if outcome.doc_id and self._paperless is not None: + if outcome.doc_id: for tag in extra_tags or []: - await self._paperless.ensure_doc_tag(outcome.doc_id, tag) + await self._services.paperless.ensure_doc_tag(outcome.doc_id, tag) if note_prefix: - await self._paperless.add_note(outcome.doc_id, note_prefix) + await self._services.paperless.add_note(outcome.doc_id, note_prefix) await self._reply_for_outcome(room_id, outcome, reply_to) if outcome.status == "enriched": processed_parts = [*outcome.resolved_topics, *outcome.resolved_persons] @@ -2029,8 +2072,6 @@ async def _handle_source_message(self, room, event, source: dict) -> None: event.sender, ) return - if self._capture is None: - return raw = source.get("raw_content") or "" if not raw.strip(): return @@ -2065,7 +2106,7 @@ async def _handle_source_message(self, room, event, source: dict) -> None: # No topic: scope by room membership (a DM/private room → its # human; the shared family room → the shared bucket). bucket = self._scope_bucket(room) - outcome = await self._capture.capture_email( + outcome = await self._services.capture.capture_email( subject=source.get("subject"), body=body, message_id=source.get("message_id"), @@ -2211,7 +2252,7 @@ async def _handle_capture( binding = await self._topic_binding( self._room_by_id(room_id), sender_mxid, ) - outcome = await self._capture.capture_url( + outcome = await self._services.capture.capture_url( url=url, sender_mxid=sender_mxid, notifier=self._notifier(room_id, reply_to), capture_id=capture_id, @@ -2237,7 +2278,7 @@ async def _handle_text_capture( binding = await self._topic_binding( self._room_by_id(room_id), sender_mxid, ) - outcome = await self._capture.capture_text( + outcome = await self._services.capture.capture_text( text=text, sender_mxid=sender_mxid, capture_id=capture_id, seed_topics=binding.seed_topics if binding else None, @@ -2336,7 +2377,7 @@ async def _handle_binary_capture( bucket = self._scope_bucket(room) else: bucket = None - outcome = await self._capture.capture_binary( + outcome = await self._services.capture.capture_binary( file_data=file_data, mime=mime, filename=filename, source_uri=source_uri, sender_mxid=sender_mxid, capture_id=capture_id, @@ -2455,7 +2496,7 @@ async def _handle_url( await self._send(room_id, self.t("downloading_url"), reply_to) try: - async with self._http.get(download_url, timeout=aiohttp.ClientTimeout(total=60), allow_redirects=True) as resp: + async with self._ensure_http().get(download_url, timeout=aiohttp.ClientTimeout(total=60), allow_redirects=True) as resp: if resp.status != 200: await self._send(room_id, self.t("url_http_error", status=resp.status), reply_to) return @@ -2523,7 +2564,7 @@ async def _handle_search( if binding is not None: topic_bucket = binding.bucket - reply = await self._search.run( + reply = await self._services.search.run( query=query, sender=sender, notifier=self._notifier(room_id, reply_to), topic_bucket=topic_bucket, @@ -2534,7 +2575,7 @@ async def _handle_search( async def _handle_show(self, room_id: str, doc_id: int, reply_to: str | None = None): """Fetch a document's content from Paperless and return it as Markdown.""" - doc = await self._paperless.get_doc(doc_id) + doc = await self._services.paperless.get_doc(doc_id) if not doc: await self._send(room_id, f"Document #{doc_id} not found.", reply_to) return diff --git a/tests/stacklets/test_archivist_corrections.py b/tests/stacklets/test_archivist_corrections.py index d8b7ab21..03344a53 100644 --- a/tests/stacklets/test_archivist_corrections.py +++ b/tests/stacklets/test_archivist_corrections.py @@ -656,12 +656,17 @@ class TestTheAnswerStaysInTheThread: """ @pytest.fixture - def bot(self, tmp_path): + def pipeline(self): + """The document pipeline stub; each test gives it a `reprocess`.""" + return SimpleNamespace() + + @pytest.fixture + def bot(self, tmp_path, pipeline): """An archivist with the REAL reprocess handler and a stub pipeline, recording every send with its relation.""" bot = ArchivistBot( homeserver="http://homeserver", user_id=BOT_ID, password="x", - session_dir=tmp_path, + session_dir=tmp_path, services=SimpleNamespace(pipeline=pipeline), ) bot._client = FakeMatrix() bot.sent: list[dict] = [] @@ -671,7 +676,6 @@ async def _room_send(room_id, message_type, content, **kw): return SimpleNamespace(event_id="$answer") bot._client.room_send = _room_send - bot._pipeline = SimpleNamespace() return bot def _outcome(self, status="reclassified"): @@ -684,7 +688,7 @@ def _outcome(self, status="reclassified"): ) @pytest.mark.asyncio - async def test_reclassified_confirmation_joins_the_correction_thread(self, bot): + async def test_reclassified_confirmation_joins_the_correction_thread(self, bot, pipeline): """The confirmation carries an `m.thread` relation rooted where the correction was, so the next correction can find it.""" bot._client.add(_message("$upload", HOMER, "policy.pdf")) @@ -697,7 +701,7 @@ async def test_reclassified_confirmation_joins_the_correction_thread(self, bot): async def _reprocess(**kw): return self._outcome() - bot._pipeline.reprocess = _reprocess + pipeline.reprocess = _reprocess await bot._handle_reply_reprocess( "!docs:server", DOC_ID, "this is Marge's", "$correction", ) @@ -711,7 +715,7 @@ async def _reprocess(**kw): assert relation.get("event_id") == "$upload" @pytest.mark.asyncio - async def test_a_failed_reprocess_answers_in_the_thread_too(self, bot): + async def test_a_failed_reprocess_answers_in_the_thread_too(self, bot, pipeline): """An error is a reply to what the user just typed, so it belongs where they typed it. Otherwise a correction that failed looks like nothing happened.""" @@ -725,7 +729,7 @@ async def test_a_failed_reprocess_answers_in_the_thread_too(self, bot): async def _reprocess(**kw): return self._outcome(status="llm_error") - bot._pipeline.reprocess = _reprocess + pipeline.reprocess = _reprocess await bot._handle_reply_reprocess( "!docs:server", DOC_ID, "this is Marge's", "$correction", ) diff --git a/tests/stacklets/test_archivist_routing.py b/tests/stacklets/test_archivist_routing.py index 9163cf10..cdf666f6 100644 --- a/tests/stacklets/test_archivist_routing.py +++ b/tests/stacklets/test_archivist_routing.py @@ -436,7 +436,6 @@ def _bot(self, tmp_path, *, binding, target_body="remember the boiler service"): What this class asserts is what happens *before* the reply -- the acknowledgement and the routing. """ - bot = _build_bot(tmp_path) captures, reactions = [], [] class _RecordingPipeline: @@ -453,7 +452,9 @@ async def _binding(_room, _sender): async def _reply(*_a, **_kw): return None - bot._capture = _RecordingPipeline() + bot = _build_bot( + tmp_path, services=SimpleNamespace(capture=_RecordingPipeline()), + ) bot._react = _react bot._topic_binding = _binding bot._reply_for_capture = _reply @@ -1045,7 +1046,6 @@ class TestAnUploadKeepsItsFile: WHEN = int(datetime(2026, 3, 14, 9, 30, tzinfo=timezone.utc).timestamp() * 1000) def _bot(self, tmp_path): - bot = _build_bot(tmp_path) calls: list[dict] = [] class _RecordingPipeline: @@ -1059,7 +1059,9 @@ async def _binding(_room, _sender): async def _reply(*_a, **_kw): return None - bot._capture = _RecordingPipeline() + bot = _build_bot( + tmp_path, services=SimpleNamespace(capture=_RecordingPipeline()), + ) bot._topic_binding = _binding bot._reply_for_capture = _reply bot._client = SimpleNamespace(rooms={"!r:server": _room()}) diff --git a/tests/stacklets/test_archivist_source.py b/tests/stacklets/test_archivist_source.py index f1aed5f8..e83887f8 100644 --- a/tests/stacklets/test_archivist_source.py +++ b/tests/stacklets/test_archivist_source.py @@ -24,9 +24,15 @@ BOT_ID = "@archivist-bot:server" -def _bot(tmp_path): +def _bot(tmp_path, capture=None): + """An archivist whose only service is the given capture pipeline. + + Anything else the handler under test reaches for raises, so a test + cannot pass by accident on a service it did not mean to exercise. + """ return ArchivistBot( homeserver="http://hs", user_id=BOT_ID, password="x", session_dir=tmp_path, + services=SimpleNamespace(capture=capture), ) @@ -91,9 +97,8 @@ async def _true(): @pytest.mark.asyncio async def test_email_source_folds_through_capture(tmp_path): - bot = _bot(tmp_path) cap = _FakeCapture() - bot._capture = cap + bot = _bot(tmp_path, capture=cap) sends = _wire(bot) await bot._on_text(_room(), _source_event(_EMAIL_BLOCK)) @@ -151,9 +156,8 @@ async def test_email_routes_to_topic_room_bucket(tmp_path): # A topic-bound room scopes the email to that bucket (and seeds its tag), # instead of always the shared bucket — co-located with the room's # attachments. The default (no binding) path is covered above. - bot = _bot(tmp_path) cap = _FakeCapture() - bot._capture = cap + bot = _bot(tmp_path, capture=cap) _wire(bot) async def _binding(room, sender): @@ -173,9 +177,8 @@ async def _binding(room, sender): @pytest.mark.asyncio async def test_email_defaults_to_shared_bucket_without_binding(tmp_path): - bot = _bot(tmp_path) cap = _FakeCapture() - bot._capture = cap + bot = _bot(tmp_path, capture=cap) _wire(bot) async def _no_binding(room, sender): @@ -190,9 +193,8 @@ async def _no_binding(room, sender): async def test_email_in_dm_files_under_the_sole_human(tmp_path): # A DM is just a room with one human + the bots. Email there scopes to # that person's bucket, not the shared one — the bot sender is ignored. - bot = _bot(tmp_path) cap = _FakeCapture() - bot._capture = cap + bot = _bot(tmp_path, capture=cap) _wire(bot) async def _no_binding(room, sender): @@ -208,9 +210,8 @@ async def _no_binding(room, sender): @pytest.mark.asyncio async def test_email_in_multi_human_room_is_shared(tmp_path): - bot = _bot(tmp_path) cap = _FakeCapture() - bot._capture = cap + bot = _bot(tmp_path, capture=cap) _wire(bot) async def _no_binding(room, sender): @@ -226,9 +227,8 @@ async def _no_binding(room, sender): @pytest.mark.asyncio async def test_non_bot_sender_is_rejected(tmp_path): - bot = _bot(tmp_path) cap = _FakeCapture() - bot._capture = cap + bot = _bot(tmp_path, capture=cap) _wire(bot) # A family member cannot spoof an ingest event. await bot._on_text(_room(), _source_event(_EMAIL_BLOCK, sender="@homer:server")) @@ -239,9 +239,8 @@ async def test_non_bot_sender_is_rejected(tmp_path): async def test_plain_bot_chatter_is_ignored(tmp_path): # The mail bot's join welcome (a plain message from a -bot sender, no # source block) must not be treated as a capture or query. - bot = _bot(tmp_path) cap = _FakeCapture() - bot._capture = cap + bot = _bot(tmp_path, capture=cap) sends = _wire(bot) welcome = SimpleNamespace( body="mail bot here. I'll deliver new email into this room", @@ -255,9 +254,8 @@ async def test_plain_bot_chatter_is_ignored(tmp_path): @pytest.mark.asyncio async def test_non_email_source_ignored(tmp_path): - bot = _bot(tmp_path) cap = _FakeCapture() - bot._capture = cap + bot = _bot(tmp_path, capture=cap) _wire(bot) block = {"source": "webhook", "raw_content": "x"} await bot._on_text(_room(), _source_event(block)) @@ -266,9 +264,8 @@ async def test_non_email_source_ignored(tmp_path): @pytest.mark.asyncio async def test_empty_raw_content_skipped(tmp_path): - bot = _bot(tmp_path) cap = _FakeCapture() - bot._capture = cap + bot = _bot(tmp_path, capture=cap) _wire(bot) block = {**_EMAIL_BLOCK, "raw_content": " "} await bot._on_text(_room(), _source_event(block)) @@ -277,9 +274,8 @@ async def test_empty_raw_content_skipped(tmp_path): @pytest.mark.asyncio async def test_no_envelope_skips_timeline_post(tmp_path): - bot = _bot(tmp_path) cap = _FakeCapture(envelope=False) - bot._capture = cap + bot = _bot(tmp_path, capture=cap) sends = _wire(bot) await bot._on_text(_room(), _source_event(_EMAIL_BLOCK)) assert len(cap.calls) == 1 # still filed From 66709513b8fad0ba76d4a4b459fff83fe0f4e49f Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 21 Sep 2026 14:17:31 +0200 Subject: [PATCH 2/4] refactor: import what the code uses and type optional defaults - Call `urllib.parse.quote` instead of `urllib.request.quote`, which only works because `urllib.request` happens to re-export it. - Import `urllib.error` where `docker.py` catches `HTTPError`; it was reachable only as a side effect of importing `urllib.request`. - Declare `env` and `headers` parameters that default to None as `dict | None`. No behaviour change. --- lib/stack/docker.py | 13 +++++++------ stacklets/ai/omlx.py | 9 +++++---- stacklets/core/bot-runner/accounts.py | 5 +++-- stacklets/messages/cli/_matrix.py | 7 ++++--- 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/lib/stack/docker.py b/lib/stack/docker.py index 5670e537..bfffd739 100644 --- a/lib/stack/docker.py +++ b/lib/stack/docker.py @@ -21,6 +21,7 @@ import ssl import subprocess import time +import urllib.error import urllib.request from pathlib import Path @@ -50,7 +51,7 @@ def compose(compose_file: str | Path, *args) -> tuple[int, str, str]: return result.returncode, result.stdout, result.stderr -def compose_up(compose_file: str | Path, env: dict = None) -> tuple[int, str]: +def compose_up(compose_file: str | Path, env: dict | None = None) -> tuple[int, str]: """Start containers, always force-recreating. Returns (exit_code, error_output). `--force-recreate` is unconditional because compose's service-config @@ -90,7 +91,7 @@ def compose_down(compose_file: str | Path) -> tuple[int, str]: return code, (stdout + stderr).strip() -def compose_pull(compose_file: str | Path, env: dict = None) -> None: +def compose_pull(compose_file: str | Path, env: dict | None = None) -> None: """Pull images for a compose file. Streams output.""" full_env = {**__import__("os").environ, **(env or {})} _docker( @@ -99,7 +100,7 @@ def compose_pull(compose_file: str | Path, env: dict = None) -> None: ) -def compose_build(compose_file: str | Path, env: dict = None) -> None: +def compose_build(compose_file: str | Path, env: dict | None = None) -> None: """Build images for a compose file. Streams output.""" full_env = {**__import__("os").environ, **(env or {})} _docker( @@ -206,7 +207,7 @@ def check_docker() -> tuple[str | None, str | None]: return None, "Docker is not installed" -def check_health(url: str, headers: dict = None) -> bool: +def check_health(url: str, headers: dict | None = None) -> bool: """Quick probe — returns True if URL responds with 2xx.""" try: req = urllib.request.Request(url, headers=headers or {}) @@ -216,7 +217,7 @@ def check_health(url: str, headers: dict = None) -> bool: return False -def probe_health(url: str, headers: dict = None, timeout: float = 3) -> str: +def probe_health(url: str, headers: dict | None = None, timeout: float = 3) -> str: """Single-shot health probe. Returns 'ready', 'auth', or 'down'. No retries — the caller loops if they want a wait. Used by both @@ -236,7 +237,7 @@ def probe_health(url: str, headers: dict = None, timeout: float = 3) -> str: def wait_for_health(url: str, timeout: int = 120, interval: int = 3, - headers: dict = None) -> str: + headers: dict | None = None) -> str: """Poll a URL until it responds. Returns 'ready', 'auth', or 'timeout'. Used after compose up to wait for a service to actually be ready, diff --git a/stacklets/ai/omlx.py b/stacklets/ai/omlx.py index 3ddbb663..6d817ce7 100644 --- a/stacklets/ai/omlx.py +++ b/stacklets/ai/omlx.py @@ -12,6 +12,7 @@ import http.cookiejar import json import urllib.error +import urllib.parse import urllib.request from dataclasses import dataclass @@ -91,7 +92,7 @@ def list_downloaded(self) -> list[dict]: def get_model_info(self, repo_id: str) -> ModelInfo | None: """Get model info from HuggingFace. Returns None if not found.""" - encoded = urllib.request.quote(repo_id, safe="") + encoded = urllib.parse.quote(repo_id, safe="") result = self._request("GET", f"/admin/api/hf/model-info?repo_id={encoded}") if result is None or "error" in result: return None @@ -148,13 +149,13 @@ def load_model(self, model_id: str) -> bool: model_id is the folder name (e.g. "Qwen3.5-9B-MLX-8bit"), not the full repo_id. """ - encoded = urllib.request.quote(model_id, safe="") + encoded = urllib.parse.quote(model_id, safe="") result = self._request("POST", f"/admin/api/models/{encoded}/load") return result is not None and result.get("success", False) def get_model_settings(self, model_id: str) -> dict | None: """Get per-model settings. Returns the settings dict or None.""" - encoded = urllib.request.quote(model_id, safe="") + encoded = urllib.parse.quote(model_id, safe="") result = self._request("PUT", f"/admin/api/models/{encoded}/settings", {}) if result is None: return None @@ -162,7 +163,7 @@ def get_model_settings(self, model_id: str) -> dict | None: def update_model_settings(self, model_id: str, **settings) -> bool: """Update per-model settings (chat_template_kwargs, context size, etc.).""" - encoded = urllib.request.quote(model_id, safe="") + encoded = urllib.parse.quote(model_id, safe="") result = self._request("PUT", f"/admin/api/models/{encoded}/settings", settings) return result is not None and result.get("success", False) diff --git a/stacklets/core/bot-runner/accounts.py b/stacklets/core/bot-runner/accounts.py index 3f06fb4e..11ce740d 100644 --- a/stacklets/core/bot-runner/accounts.py +++ b/stacklets/core/bot-runner/accounts.py @@ -8,6 +8,7 @@ import json import ssl import urllib.error +import urllib.parse import urllib.request from loguru import logger @@ -52,7 +53,7 @@ def _admin_login(base, admin_user, admin_password): def _resolve_space(base, token, server_name): """Resolve the Family Space room ID, or None.""" space_alias = f"#family:{server_name}" - encoded_alias = urllib.request.quote(space_alias) + encoded_alias = urllib.parse.quote(space_alias) status, resp = _api("GET", f"{base}/_matrix/client/v3/directory/room/{encoded_alias}", token=token) return resp.get("room_id") if status == 200 else None @@ -133,7 +134,7 @@ def ensure_rooms(bots, homeserver, server_name, admin_user, admin_password, def _ensure_room(base, token, server_name, alias, topic, space_id): """Create a room or resolve an existing one.""" full_alias = f"#{alias}:{server_name}" - encoded = urllib.request.quote(full_alias) + encoded = urllib.parse.quote(full_alias) # Check if room exists status, resp = _api("GET", f"{base}/_matrix/client/v3/directory/room/{encoded}", token=token) diff --git a/stacklets/messages/cli/_matrix.py b/stacklets/messages/cli/_matrix.py index a1b5f485..0795bb8e 100644 --- a/stacklets/messages/cli/_matrix.py +++ b/stacklets/messages/cli/_matrix.py @@ -18,6 +18,7 @@ import time import tomllib import urllib.error +import urllib.parse import urllib.request from pathlib import Path @@ -201,7 +202,7 @@ def logout(self): def resolve_room(self, alias): """Resolve a room alias to a room ID.""" full = self._full_room(alias) - encoded = urllib.request.quote(full) + encoded = urllib.parse.quote(full) status, resp = _get( self._url(f"/_matrix/client/v3/directory/room/{encoded}"), token=self.token, @@ -479,7 +480,7 @@ def join(self, room): """Join a room by alias or ID as the logged-in user. Idempotent — Synapse returns 200 if you are already a member.""" target = room if room.startswith("!") else self._full_room(room) - encoded = urllib.request.quote(target) + encoded = urllib.parse.quote(target) status, _ = _post( self._url(f"/_matrix/client/v3/join/{encoded}"), {}, token=self.token, ) @@ -493,7 +494,7 @@ def upload_media(self, data, content_type, filename): content type. Returns None on failure. """ url = self._url( - f"/_matrix/media/v3/upload?filename={urllib.request.quote(filename)}" + f"/_matrix/media/v3/upload?filename={urllib.parse.quote(filename)}" ) req = urllib.request.Request( url, data=data, method="POST", From 2965061ad14b2c3511383518559c193aff4d97fd Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 21 Sep 2026 14:19:07 +0200 Subject: [PATCH 3/4] refactor: type values that are never None when they are used The type checker reported these as possible None accesses because their declarations allowed a state the code never reaches. - `MicroBot._react` already ignores an empty event id; its parameter is now `str | None`, matching the callers that pass a reply id. - The messages CLI `_api` helper returns `tuple[int, dict[str, Any]]`. Without it the error branch made the body look like `dict[str, str]` and every `.get()` chain on a Matrix response read as `str`. - The archivist's topic welcome parses the room name once and branches on the result, instead of re-parsing after the kind check. - `GitMirror` reads its credentials through `_setup_creds`, which raises if `ensure_setup()` has not succeeded; every caller already returns early in that case. - agent-lab `lab-api` declares `ARGS` as the Namespace `main()` sets. No behaviour change. --- stacklets/core/bot-runner/microbot.py | 2 +- stacklets/docs/bot/archivist.py | 4 ++-- stacklets/docs/bot/git_mirror.py | 22 +++++++++++++++++----- stacklets/messages/cli/_matrix.py | 3 ++- tools/agent-lab/rig/lab-api.py | 3 ++- 5 files changed, 24 insertions(+), 10 deletions(-) diff --git a/stacklets/core/bot-runner/microbot.py b/stacklets/core/bot-runner/microbot.py index c5052183..f528f4be 100644 --- a/stacklets/core/bot-runner/microbot.py +++ b/stacklets/core/bot-runner/microbot.py @@ -638,7 +638,7 @@ async def _room_send( room_id=room_id, message_type=message_type, content=content, ) - async def _react(self, room_id: str, event_id: str, emoji: str) -> None: + async def _react(self, room_id: str, event_id: str | None, emoji: str) -> None: """Annotate an event with an emoji reaction (MSC2677). The bot's way to signal state on a specific message without a diff --git a/stacklets/docs/bot/archivist.py b/stacklets/docs/bot/archivist.py index ab257f9a..9d6f14ba 100644 --- a/stacklets/docs/bot/archivist.py +++ b/stacklets/docs/bot/archivist.py @@ -944,8 +944,8 @@ def _welcome_text_for(self, room, ctx) -> str: """ kind = self._welcome_kind_for(room, ctx) - if kind == "topic": - parsed = parse_topic_name(self._room_display_name(room)) + parsed = parse_topic_name(self._room_display_name(room)) + if kind == "topic" and parsed is not None: scope = scope_from_members(self._count_humans_in_room(room)) if scope == "shared": bucket = f"{self.shared_bucket}/{parsed.slug}" diff --git a/stacklets/docs/bot/git_mirror.py b/stacklets/docs/bot/git_mirror.py index fd95d5b8..36be69f5 100644 --- a/stacklets/docs/bot/git_mirror.py +++ b/stacklets/docs/bot/git_mirror.py @@ -285,8 +285,20 @@ def _load_or_create_creds(self) -> MirrorCreds: self._save_creds(creds) return creds + @property + def _setup_creds(self) -> MirrorCreds: + """The credentials `ensure_setup()` loaded. + + Every caller runs after `ensure_setup()` returned True, which is + what sets them. Reaching this without it is a bug, and it says so + here instead of failing later on a None. + """ + if self._creds is None: + raise RuntimeError("GitMirror used before ensure_setup() succeeded") + return self._creds + def _save_creds(self, creds: MirrorCreds | None = None) -> None: - creds = creds or self._creds + creds = creds or self._setup_creds self.creds_path.parent.mkdir(parents=True, exist_ok=True) self.creds_path.write_text(json.dumps({ "password": creds.password, @@ -626,7 +638,7 @@ async def publish( if not await self.ensure_setup(): return False - client = ForgejoClient(url=self.code_url, token=self._creds.token) + client = ForgejoClient(url=self.code_url, token=self._setup_creds.token) # Title comes from AI classification first, then caller's fallback # (usually the original filename), and only then the generic @@ -826,7 +838,7 @@ async def read_capture(self, path: str) -> str | None: """ if not await self.ensure_setup(): return None - client = ForgejoClient(url=self.code_url, token=self._creds.token) + client = ForgejoClient(url=self.code_url, token=self._setup_creds.token) try: data = await asyncio.to_thread( client.get_file, self.repo_owner, REPO_NAME, path, @@ -890,7 +902,7 @@ async def publish_capture( if not await self.ensure_setup(): return None - client = ForgejoClient(url=self.code_url, token=self._creds.token) + client = ForgejoClient(url=self.code_url, token=self._setup_creds.token) resolved_title = classification.get("title") or title_hint title = resolved_title or "Capture" @@ -1084,7 +1096,7 @@ async def publish_email_message( if not await self.ensure_setup(): return None - client = ForgejoClient(url=self.code_url, token=self._creds.token) + client = ForgejoClient(url=self.code_url, token=self._setup_creds.token) resolved_title = classification.get("title") or title_hint title = resolved_title or "Email thread" diff --git a/stacklets/messages/cli/_matrix.py b/stacklets/messages/cli/_matrix.py index 0795bb8e..061a3cc3 100644 --- a/stacklets/messages/cli/_matrix.py +++ b/stacklets/messages/cli/_matrix.py @@ -21,6 +21,7 @@ import urllib.parse import urllib.request from pathlib import Path +from typing import Any # ── TLS ────────────────────────────────────────────────────────────────────── # Synapse is on the LAN — skip certificate verification. @@ -53,7 +54,7 @@ def resolve_login(sender, secrets): # ── HTTP helpers ───────────────────────────────────────────────────────────── -def _api(method, url, body=None, token=None): +def _api(method, url, body=None, token=None) -> tuple[int, dict[str, Any]]: """Make an HTTP request to the Matrix API. Returns (status_code, parsed_json). On HTTP errors the status code diff --git a/tools/agent-lab/rig/lab-api.py b/tools/agent-lab/rig/lab-api.py index 63f43e7c..a715981e 100644 --- a/tools/agent-lab/rig/lab-api.py +++ b/tools/agent-lab/rig/lab-api.py @@ -26,7 +26,8 @@ import urllib.request from pathlib import Path -ARGS = None +# Parsed command line, set by main() before the server takes a request. +ARGS: argparse.Namespace # Run the real store transform, so the rig cannot drift from production. # The transforms are pure; the module also imports store deps at load From 32e57e85cfb815dd39428c97760f84a77eeb322c Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 21 Sep 2026 14:27:00 +0200 Subject: [PATCH 4/4] refactor: match single declarations to the values they hold - Pass string defaults to `Stack._cfg`, which returns `str`; the two wiki settings render the same `"true"` and `"180"` as before. - `stack doctor` hands `check_release` an empty string when there is no release tag, which it already reads as "no release". - `_format_size` takes a float, since it divides the value in place. - The spinners declare their thread at class level; `__enter__` starts it and `__exit__` joins it, so the None placeholder is gone. - `disable_thinking` falls back to a module-level no-op logger rather than redefining its own parameter. - Read a module docstring as `__doc__ or ""`, and use `Image.Resampling.BICUBIC`, the name Pillow types. No behaviour change. --- lib/stack/cli.py | 2 +- lib/stack/output.py | 3 ++- lib/stack/prompt.py | 3 ++- lib/stack/stack.py | 4 ++-- stacklets/ai/backend.py | 2 +- stacklets/ai/thinking.py | 7 +++++-- stacklets/docs/bot/cli_entrypoint.py | 2 +- stacklets/memory/bot/cli_entrypoint.py | 2 +- tools/family-docs/renderers.py | 2 +- 9 files changed, 16 insertions(+), 11 deletions(-) diff --git a/lib/stack/cli.py b/lib/stack/cli.py index 9ba2ed36..e61fa0a7 100644 --- a/lib/stack/cli.py +++ b/lib/stack/cli.py @@ -942,7 +942,7 @@ def missing_secrets(stacklet_id): checkout = Checkout(stck.root) position = checkout.describe() if checkout.is_git() else "" - latest = latest_tag(checkout.tags()) if position else "" + latest = (latest_tag(checkout.tags()) or "") if position else "" release = doctor.check_release( position, latest, up_to_date=bool(latest) and checkout.contains(latest)) if release: diff --git a/lib/stack/output.py b/lib/stack/output.py index 44ae6497..4a0527dd 100644 --- a/lib/stack/output.py +++ b/lib/stack/output.py @@ -147,10 +147,11 @@ def fail(self, hint=None): class _TerminalSpinner: """Animated braille spinner. Use as context manager.""" + _thread: threading.Thread # started by __enter__ + def __init__(self, msg): self.msg = msg self._running = False - self._thread = None self._ok = True self._hint = None diff --git a/lib/stack/prompt.py b/lib/stack/prompt.py index a4474142..b7f919cf 100644 --- a/lib/stack/prompt.py +++ b/lib/stack/prompt.py @@ -189,10 +189,11 @@ def banner(product, subtitle=""): class Spinner: """Minimal braille spinner for long operations.""" + thread: threading.Thread # started by __enter__ + def __init__(self, msg): self.msg = msg self.running = False - self.thread = None self.ok = True def __enter__(self): diff --git a/lib/stack/stack.py b/lib/stack/stack.py index 5759d861..b3df3f42 100644 --- a/lib/stack/stack.py +++ b/lib/stack/stack.py @@ -339,8 +339,8 @@ def _build_template_vars(self) -> dict: "shared_bucket": self._cfg("core", "shared_bucket", "family"), # Wiki freshness (the memory stacklet's curator sidecar). # Bool/int from stack.toml render as env-friendly strings. - "wiki_auto_rebuild": str(self._cfg("memory", "wiki_auto_rebuild", True)).lower(), - "wiki_rebuild_quiet_secs": str(self._cfg("memory", "wiki_rebuild_quiet_secs", 180)), + "wiki_auto_rebuild": self._cfg("memory", "wiki_auto_rebuild", "true").lower(), + "wiki_rebuild_quiet_secs": self._cfg("memory", "wiki_rebuild_quiet_secs", "180"), "wiki_nightly": self._cfg("memory", "wiki_nightly", "03:30"), # AI service URLs — host-side for CLI, docker-side for containers "ai_openai_url": ai_openai_url, diff --git a/stacklets/ai/backend.py b/stacklets/ai/backend.py index c74ddc22..ec2935d8 100644 --- a/stacklets/ai/backend.py +++ b/stacklets/ai/backend.py @@ -265,7 +265,7 @@ def _render_progress(percent: float, downloaded: int, total: int): end="", flush=True) -def _format_size(size: int) -> str: +def _format_size(size: float) -> str: """Format bytes as human-readable size.""" for unit in ("B", "KB", "MB", "GB", "TB"): if size < 1024: diff --git a/stacklets/ai/thinking.py b/stacklets/ai/thinking.py index cb9b7fde..0ce10df4 100644 --- a/stacklets/ai/thinking.py +++ b/stacklets/ai/thinking.py @@ -18,6 +18,10 @@ def is_qwen3(model_id: str) -> bool: return "qwen3" in lower or "qwen-3" in lower +def _quiet(msg: str) -> None: + return None + + def disable_thinking(client, model_id: str, log=None) -> bool: """Disable thinking for a Qwen3-family model via oMLX admin API. @@ -29,8 +33,7 @@ def disable_thinking(client, model_id: str, log=None) -> bool: Returns True if thinking is now disabled, False on failure. """ if log is None: - def log(msg): - return None + log = _quiet if not is_qwen3(model_id): log(f"Not a Qwen3 model ({model_id}), skipping") diff --git a/stacklets/docs/bot/cli_entrypoint.py b/stacklets/docs/bot/cli_entrypoint.py index ad34d348..3ff46e30 100644 --- a/stacklets/docs/bot/cli_entrypoint.py +++ b/stacklets/docs/bot/cli_entrypoint.py @@ -110,7 +110,7 @@ async def main(argv: list[str]) -> int: def _usage() -> None: - err(__doc__.rstrip()) + err((__doc__ or "").rstrip()) if __name__ == "__main__": diff --git a/stacklets/memory/bot/cli_entrypoint.py b/stacklets/memory/bot/cli_entrypoint.py index ff5af7a0..9db5ebc5 100644 --- a/stacklets/memory/bot/cli_entrypoint.py +++ b/stacklets/memory/bot/cli_entrypoint.py @@ -90,7 +90,7 @@ async def main(argv: list[str]) -> int: def _usage() -> None: - _err(__doc__.rstrip()) + _err((__doc__ or "").rstrip()) if __name__ == "__main__": diff --git a/tools/family-docs/renderers.py b/tools/family-docs/renderers.py index 0f5f066b..20bb8c7c 100644 --- a/tools/family-docs/renderers.py +++ b/tools/family-docs/renderers.py @@ -428,7 +428,7 @@ def centered(text, font, fill, yy): ("SPRINGFIELD COUNTY", 178)): w = sd.textlength(text, font=sf) sd.text((125 - w / 2, yy), text, font=sf, fill=ink_a) - seal = seal.rotate(-13, expand=True, resample=Image.BICUBIC) + seal = seal.rotate(-13, expand=True, resample=Image.Resampling.BICUBIC) img.paste(seal, (150, H - 360), seal) # Cursive registrar signature over a ruled line.