fix(channels): gate a Mattermost webhook post the way the stream is - #685
Closed
DEENUU1 wants to merge 2 commits into
Closed
fix(channels): gate a Mattermost webhook post the way the stream is#685DEENUU1 wants to merge 2 commits into
DEENUU1 wants to merge 2 commits into
Conversation
The addressing rule added for the event stream was the stream's alone. `_from_webhook` never set `addressed`, so every outgoing-webhook delivery arrived as `None` - which the router reads as "the platform did not say" and answers. A bot reached by outgoing webhook therefore replied to every post Mattermost handed it, including two colleagues talking to each other, and posted its unknown-handle refusals out loud in a channel it was never addressed in. The docs' addressing table sat under the Mattermost section without saying it applied to one transport only. An outgoing-webhook body carries no mention list - Mattermost sends the post, not who it notified - so there is no account to look ourselves up in. What it does carry is `trigger_word`: the word an operator told this integration to fire on, which is Mattermost's own record that the post was for us. Empty means the webhook fired on its channel filter alone, and a channel filter delivers every post exactly as the socket does, so that reads as `False` for the same reason a `posted` event with no mentions does. A direct message and an `@agent-slug` are unaffected - both are decided before the flag is read. The cost, and it is in the docs beside the setup steps: `@the-bot` is not readable on this transport, because nothing in the body says which account the bot is. An operator who wants that sets the trigger word to the bot's handle. Verified: three new cases in tests/test_channel_addressing.py fail against the old parser (`assert None is False`) and pass with it, and they assert `_is_overheard` rather than the field alone. Full `make test` green at 100%, `make lint-backend` and `make docs-build` clean. Not fixed here, and deliberately: `@channel` / `@all` / `@here` on the stream transport. The bot answers a broadcast wherever Mattermost lists it among the notified; that is now written down in docs/channels.md as intended rather than left as a question, since a private notion of "who was this for" would disagree with the one the channel was shown. Nobody has checked a real server to see when Mattermost puts a bot in that list. Closes #662
Member
Author
|
CI: everything green except Four failures, all
The base branch has the same four at its own head. Run
|
This was referenced Aug 13, 2026
All four surfaces specs failed in publishPage on `await expect(submit).toHaveCount(0)`, and the write they were waiting for had already landed: the failure snapshot shows the row, its /e/ link, and no form. A role name is matched as a case-insensitive substring, so once the form unmounted and the picker came back, the Public API card - "…Nothing to publish here." - kept the count at 1 for the full 15s. Introduced by 530d699, which added the wait; the card predates it. The picker lookup had the same latent shape: /Hosted page/ also matches "Edit Hosted page" on a row already published, and only the click beating the embeds fetch kept `.first()` on the card. Anchored, since the specs accumulate a page per run on the draft agent. Both locators verified against a fixture carrying the two accessible names from the failing run: substring "Publish" resolves to the API card with the form gone, exact resolves to nothing; /Hosted page/ to three buttons, /^Hosted page/ to one. Pre-existing on feat/agent-surfaces rather than caused here - the same four failures are red on its own run - and carried on this branch so its e2e job can pass. Refs #634
DEENUU1
added a commit
that referenced
this pull request
Aug 13, 2026
## What this fixes Closes #690. A channel turn that ran perfectly left its `ChatFile` rows with `message_id` NULL. `link_files_to_message` was called from exactly one place - `persist_user_turn` in the web chat path - so a file sent to a Slack, Telegram or Mattermost bot was stored, fed to the agent, and then orphaned. `chat_files` carries no `organization_id`, so a row with no message is scoped by `user_id` alone: reachable through `GET /files/{id}` by whoever sent it and by nothing else, while the conversation holding the question does not know the file exists. A transcript of a channel thread showed the question and not the spreadsheet it was asked about. ## What changed - `TranscriptService.record` takes the turn's `attachments` and links them to the user message it writes. That is the right place because it is where that message first exists, and it is the one write every non-streaming surface reaches - the same reason the transcript itself lives there (#205): a thing each surface has to remember is a thing the next surface will not. - **The link gets a SAVEPOINT of its own** inside the transcript's. It is the only write there touching rows the conversation does not own, so sharing the outer one would mean a run that has already spent money losing its answer and its tool calls because a file could not be attached - the opposite of the trade web chat makes for the same write. Skipped entirely when nothing was attached: a SAVEPOINT and its release on every turn in the deployment is a real cost for a list that is almost always empty. - `AgentRunnerService.execute` passes the attachments through `_run` to get them there. A resume passes none and records no prompt, so nothing links. - Web chat is untouched. It streams, writes its own transcript and links its own files through `persist_user_turn`, so there is no second link and no double write. - `docs/channels.md`, the Files section: a file the turn ran on belongs to that turn, why an unlinked row matters given no `organization_id`, and what the link widens - the metadata, never the bytes (see Noticed below). No repository, schema or route change. `chat_file_repo.link_to_message` already existed; only its second caller is new. ## How it failed before 1. Drop a `.csv` on a channel bot with any question. 2. The agent answers using the file. 3. `SELECT message_id FROM chat_files WHERE id = …` → NULL. ## Testing - `backend/tests/integration/test_transcript_savepoint.py` — a channel turn with an attachment against a real database, **reading the column back**, which is what #690 asked for. The repository boundary is not where this was ever broken: `link_to_message` worked and no surface but web chat called it, so a test that stops there proves the half that already worked. Fails against `main`'s `transcript.py`. - `backend/tests/test_surface_transcripts.py` — the same turn through its real entry point (`ChannelAgentRouter.answer_default`), asserting the link carries the id of the **user** message and not the assistant one. - `backend/tests/test_transcript.py` — a link that raises costs the files and not the turn: the assistant message and its tool calls are still written. - `make lint-backend` green. `make test` green: 4378 passed, platform layer still at 100%. `make test-integration` green: 423 passed. - Not run locally: `make test-e2e` (backend-only change). CI's `e2e` job passed. ## Overlaps Based on `origin/main`; the code being fixed is on `main`, so no stacking was needed. Four PRs are open on this same channel-attachment path and **merge order matters** — whichever lands second needs a rebase, though none of them touch the files changed here: - **#684** (#660) — moves `_receive_files` above `_answer_mention`. **This one gates half of the claim above.** Until it lands, a message that names no handle stores each file twice — `_answer_mention` receives them, discards them on `UnaddressedMessage`, and `_route_inner` receives them again — and only the second copy is the one this links. The row the agent actually read comes out linked either way; the duplicate stays orphaned until #660 is fixed. Worth merging first. - **#689** (#661) — discards a *refused* turn's stored files. This PR is the success half of the same story: #689 deletes rows for a turn that never ran, this links rows for a turn that did. Both touch `docs/channels.md` § Files in different paragraphs — a textual conflict there is likely and trivial. - **#685** (#662) — Mattermost webhook gating, based on `feat/agent-surfaces`. - **#547** — both transports bypassing `parse_incoming`, in progress elsewhere. ## Noticed, not fixed - **#706 (new, `security`)** — `chat_file_repo.link_to_message` is a blind `UPDATE … WHERE id IN (…)` with no owner predicate, and the *web* path feeds it client-supplied ids, so a chat turn can attach another user's file to its own message. Pre-existing and not reachable from the caller added here — a channel turn links rows the adapter created server-side for that sender in that turn — but adding a second caller is what put the function under review. - **#704 (new)** — an attachment that yields no text at all (an inline image on a workspace-less agent, a file with no `parsed_content`, a routing failure) leaves `prompt` empty, so no user turn is written and there is nothing to link to. I filed this with the wrong scope first and have corrected it: a caption-less `.csv` *is* linked, because `build_prompt` prepends an `Attached file: …` reference. - **Shared channels widen metadata by design.** A channel's conversation is owned by whoever spoke first, so a colleague's file now appears in a transcript other members can read — name, type, size. The bytes still answer only the owner, so the chip is visible and the download is not. Documented in `docs/channels.md` rather than left to be discovered.
DEENUU1
added a commit
that referenced
this pull request
Aug 13, 2026
## What this fixes Closes #547 — Telegram's polling loop and Mattermost's outgoing webhook each built an `IncomingMessage` of their own instead of calling `parse_incoming`, and both copies had drifted the same way: neither read attachments. ## Base is `main`, and the defect is there Checked before branching, because two issues this week turned out to live only on a feature branch. This one does not: | Symbol | On `main`? | |---|---| | `TelegramAdapter._handle_update` (`telegram.py:407`) | Yes — text only, `raw={}`, no `_attachments` | | `MattermostAdapter._from_webhook` (`mattermost.py:709`) | Yes — no `_attachments` | | `TelegramAdapter.parse_incoming` / `_attachments` | Yes | | `MattermostAdapter._from_socket` / `_attachments` | Yes | So this branch is cut from `origin/main` and is not stacked. ## What changed - **`backend/app/services/channels/telegram.py`** — `_handle_update` dumps the aiogram `Message` back into Bot API shape and hands it to `parse_incoming` rather than normalising it a second time. `by_alias` because `from` is a Python keyword and aiogram renames it; `exclude_none` because Telegram omits a field it has nothing for and `_attachments` reads presence. - **`backend/app/services/channels/mattermost.py`** — `_from_webhook` calls `_attachments`, and a post with files and no text is a message, the way the socket path already treats one. `_attachments` reads **both spellings** of `file_ids`: the socket sends a list, the outgoing webhook one comma-separated string, which a list-only reader would have iterated a character at a time. - **`backend/tests/test_channel_adapter_attachments.py`** — a `TestTelegramPolling` class driving `_handle_update` with the router and the session patched, plus three Mattermost webhook cases and one for the shared sender rule. - **`docs/channels.md`** — the Files section now says the parser is one per adapter and both transports reach it; the linking bullet carries the sender rule; and the fetch table carries the Mattermost webhook limitation below. **One behaviour was decided rather than merely deduplicated.** A message with no sender: the polling loop always refused one, the webhook parser did not. The Bot API leaves `from` empty for a message sent to a channel, and the webhook parser answered it under a chat identity keyed on an empty user id — one row shared by every senderless post the bot ever sees. `parse_incoming` refuses it on both transports now. That is the one place where unifying the two copies changes what the webhook path does, and it is the stricter half winning. The second commit is a correction to that reasoning, found reviewing my own diff: the first one said `from` is absent because that is how Telegram delivers an **anonymous group admin's** post. It is not — such a post carries a stand-in sender and meets the ordinary linking refusal instead. The guard is right; the reason given for it was wrong in the docstring, the docs page and a test, and a page promising operators a refusal that never fires is worse than no page. Two other claims went the same way: `exclude_none` is there for the display name (aiogram holds `last_name=None` where Telegram omits it, so leaving it in renames every surname-less sender to `Ada None`) and not for `_attachments`, which reads a `None` fine; and polling never sees an `edited_message` at all, because `@dp.message()` is the only handler, where the webhook receiver does read one. Both are now stated where they are true, and the first is pinned by a test that fails with exactly that string when the flag is dropped. ## How it failed before A spreadsheet dropped on a Telegram bot in **polling mode** — the mode a development or self-hosted deployment runs — was discarded with nothing logged, and the agent answered about a document it never received. A caption-only photo was dropped whole, the question about the picture along with it, because the handler read `message.text` and a caption is not text. The Mattermost outgoing-webhook path did the same to any post carrying files. This is exactly the failure #113 fixed for the webhook and socket transports; these two paths were never brought along, which is what the docstring on `base.py:101-109` claims is fixed. ## Testing Six new assertions fail against the old parsers and pass with these — confirmed by stashing the two `app/` files and re-running, not assumed: ``` FAILED TestTelegramReceiving::test_a_message_with_no_sender_is_nothing_on_this_transport_too FAILED TestTelegramPolling::test_a_document_sent_to_a_polling_bot_reaches_the_router FAILED TestTelegramPolling::test_a_caption_is_the_text_here_too FAILED TestTelegramPolling::test_the_update_is_carried_whole_rather_than_thrown_away FAILED TestMattermostReceiving::test_an_outgoing_webhook_post_carries_its_files_too FAILED TestMattermostReceiving::test_a_webhook_post_with_a_file_and_no_text_is_still_a_message ``` - `uv run pytest tests/test_channel_adapter_attachments.py tests/test_mattermost_channel.py tests/test_channel_mentions.py tests/test_channel_dedupe.py tests/test_channel_bot_org_scope.py tests/test_channel_supervisor.py tests/test_channel_attachments.py -q` → 159 passed - `make lint-backend` → clean - `make test` → 4384 passed, 6 skipped, platform layer at 100% - `make docs-build` → clean - CI on the first commit: `lint`, `test`, `e2e`, `docs`, `CodeQL`, `Security Scan` all pass; `test-frontend` and `docker` skip on changed paths. **`e2e` passed here** — worth saying because #685, on the `feat/agent-surfaces` base, reports four `surfaces.spec.ts` failures. That spec does not exist on `main`, which is the base this branch uses. Not covered by any of that: no real Telegram bot and no real Mattermost server were talked to. The aiogram dump is exercised against the library's own model, so what is verified is that `parse_incoming` reads what `Message.model_dump` writes — not that Telegram writes the same thing. The Bot API field names are what aiogram aliases to, which is the strongest local check available. ## Overlaps — merge order matters Four open branches touch this turn path. This one is the only one in the adapters; the other three are in the router or its neighbours, so nothing here conflicts textually with #684 or #689. - **#685** (#662) — also changes `_from_webhook`, adding `addressed=` to the same `IncomingMessage` return. **This will conflict**, and it is a one-line resolution: keep both `addressed=` and `attachments=` on the return, and keep this PR's `if not text and not attachments` in place of its `if not text`. #685 is stacked on `feat/agent-surfaces`; this one is on `main`, so whichever lands second rebases. - **#684** (#660) — `ChannelMessageRouter._receive_files` moved earlier in `_route_inner`. Different file, no textual overlap — but **this PR is what makes #660 reachable on Telegram polling at all.** `_answer_mention` receives the files before `ChannelAgentRouter.answer` raises `UnaddressedMessage` (`mentions.py:298`), which is the common case and the *only* case for a caption-less file, and `_route_inner` then receives them again: two downloads and two `ChatFile` rows per attachment, the first orphaned. A polling bot had no attachments before this branch, so it had nothing to duplicate. #684 is worth landing near this one. - **#689** (#661) — `_discard_files` on the router. Different file; no overlap. - **#690** — a successful channel turn never links its `ChatFile` rows. Not worked yet; this PR increases how often it has rows to fail to link. ## Noticed, not fixed - **Filed #692** — a Mattermost bot in **webhook mode** never reaches `remember_server`, because the only caller (`open_inbound_stream`) is fed by `get_active_polling_bots`, which filters `webhook_mode.is_(False)`. So an attachment on that transport now parses with an empty handle, `download_attachment` refuses it by design, and the reply says `<name>: could not be downloaded.` That is a strict improvement on dropping it silently, and it is not the file arriving. `docs/channels.md` records the limitation with the issue number. - `_attachments` reads `post["metadata"]` as a mapping without checking, so a payload where it is a string would raise. Pre-existing on the socket path and reachable only by whoever holds the bot token or the webhook secret; Mattermost itself never sends `metadata` in a webhook body, so the guard would be a branch no test could reach honestly. Left alone, and recorded here rather than silently. - The split on `file_ids` does not `strip()` its parts. Mattermost joins with a bare comma, so `"f1, f2"` is not a shape it produces; a lenient strip would be guessing at a format nobody sends. - #565 (lift the channel transport boilerplate into `base.py`) is the structural fix that would stop this class of drift. Not attempted here — this PR removes the duplication that had already drifted, not the shape that allowed it.
DEENUU1
added a commit
that referenced
this pull request
Aug 13, 2026
…718) Reopens #685, which GitHub closed automatically when its base branch `feat/agent-surfaces` was deleted on merge. Its own diff is replayed onto `main` rather than the stale branch rebased — that branch carries a pre-squash copy of all of #634 and conflicted in 35 files. An outgoing-webhook body carries no mention list, so the path left `addressed` unset and the router answered every post in a channel the bot was merely invited to. `trigger_word` is the payload's own record that the post was for this integration. **Two of #685's hunks are deliberately dropped**, both because main is newer: - the `@channel` / `@all` / `@here` paragraph — c900eaa decided those are *never* a mention, and #685 said the opposite; - the e2e publish locators — main already anchors them and drops the `.first()` #685 kept. Verified: `test_channel_addressing.py`, `test_channel_adapter_attachments.py` and `test_channel_mentions.py`, 109 passed. Refs #662
Merged
DEENUU1
added a commit
that referenced
this pull request
Aug 13, 2026
Gate a Mattermost webhook post the way the stream is — #718, closes #662. Replaces #685, which GitHub closed automatically when its base branch `feat/agent-surfaces` was deleted on merge; its diff was replayed onto `main` rather than the stale branch rebased, because that branch carried a pre-squash copy of all of #634 and conflicted in 35 files. Two of its hunks were deliberately dropped, both because main is newer: the `@channel` / `@all` / `@here` paragraph, where c900eaa decided those are never a mention and #685 said the opposite, and the e2e publish locators, which main already anchors. Version bumped in `backend/pyproject.toml`, `backend/uv.lock` and `frontend/package.json`.
DEENUU1
added a commit
that referenced
this pull request
Aug 15, 2026
## What this fixes Closes #690. A channel turn that ran perfectly left its `ChatFile` rows with `message_id` NULL. `link_files_to_message` was called from exactly one place - `persist_user_turn` in the web chat path - so a file sent to a Slack, Telegram or Mattermost bot was stored, fed to the agent, and then orphaned. `chat_files` carries no `organization_id`, so a row with no message is scoped by `user_id` alone: reachable through `GET /files/{id}` by whoever sent it and by nothing else, while the conversation holding the question does not know the file exists. A transcript of a channel thread showed the question and not the spreadsheet it was asked about. ## What changed - `TranscriptService.record` takes the turn's `attachments` and links them to the user message it writes. That is the right place because it is where that message first exists, and it is the one write every non-streaming surface reaches - the same reason the transcript itself lives there (#205): a thing each surface has to remember is a thing the next surface will not. - **The link gets a SAVEPOINT of its own** inside the transcript's. It is the only write there touching rows the conversation does not own, so sharing the outer one would mean a run that has already spent money losing its answer and its tool calls because a file could not be attached - the opposite of the trade web chat makes for the same write. Skipped entirely when nothing was attached: a SAVEPOINT and its release on every turn in the deployment is a real cost for a list that is almost always empty. - `AgentRunnerService.execute` passes the attachments through `_run` to get them there. A resume passes none and records no prompt, so nothing links. - Web chat is untouched. It streams, writes its own transcript and links its own files through `persist_user_turn`, so there is no second link and no double write. - `docs/channels.md`, the Files section: a file the turn ran on belongs to that turn, why an unlinked row matters given no `organization_id`, and what the link widens - the metadata, never the bytes (see Noticed below). No repository, schema or route change. `chat_file_repo.link_to_message` already existed; only its second caller is new. ## How it failed before 1. Drop a `.csv` on a channel bot with any question. 2. The agent answers using the file. 3. `SELECT message_id FROM chat_files WHERE id = …` → NULL. ## Testing - `backend/tests/integration/test_transcript_savepoint.py` — a channel turn with an attachment against a real database, **reading the column back**, which is what #690 asked for. The repository boundary is not where this was ever broken: `link_to_message` worked and no surface but web chat called it, so a test that stops there proves the half that already worked. Fails against `main`'s `transcript.py`. - `backend/tests/test_surface_transcripts.py` — the same turn through its real entry point (`ChannelAgentRouter.answer_default`), asserting the link carries the id of the **user** message and not the assistant one. - `backend/tests/test_transcript.py` — a link that raises costs the files and not the turn: the assistant message and its tool calls are still written. - `make lint-backend` green. `make test` green: 4378 passed, platform layer still at 100%. `make test-integration` green: 423 passed. - Not run locally: `make test-e2e` (backend-only change). CI's `e2e` job passed. ## Overlaps Based on `origin/main`; the code being fixed is on `main`, so no stacking was needed. Four PRs are open on this same channel-attachment path and **merge order matters** — whichever lands second needs a rebase, though none of them touch the files changed here: - **#684** (#660) — moves `_receive_files` above `_answer_mention`. **This one gates half of the claim above.** Until it lands, a message that names no handle stores each file twice — `_answer_mention` receives them, discards them on `UnaddressedMessage`, and `_route_inner` receives them again — and only the second copy is the one this links. The row the agent actually read comes out linked either way; the duplicate stays orphaned until #660 is fixed. Worth merging first. - **#689** (#661) — discards a *refused* turn's stored files. This PR is the success half of the same story: #689 deletes rows for a turn that never ran, this links rows for a turn that did. Both touch `docs/channels.md` § Files in different paragraphs — a textual conflict there is likely and trivial. - **#685** (#662) — Mattermost webhook gating, based on `feat/agent-surfaces`. - **#547** — both transports bypassing `parse_incoming`, in progress elsewhere. ## Noticed, not fixed - **#706 (new, `security`)** — `chat_file_repo.link_to_message` is a blind `UPDATE … WHERE id IN (…)` with no owner predicate, and the *web* path feeds it client-supplied ids, so a chat turn can attach another user's file to its own message. Pre-existing and not reachable from the caller added here — a channel turn links rows the adapter created server-side for that sender in that turn — but adding a second caller is what put the function under review. - **#704 (new)** — an attachment that yields no text at all (an inline image on a workspace-less agent, a file with no `parsed_content`, a routing failure) leaves `prompt` empty, so no user turn is written and there is nothing to link to. I filed this with the wrong scope first and have corrected it: a caption-less `.csv` *is* linked, because `build_prompt` prepends an `Attached file: …` reference. - **Shared channels widen metadata by design.** A channel's conversation is owned by whoever spoke first, so a colleague's file now appears in a transcript other members can read — name, type, size. The bytes still answer only the owner, so the chip is visible and the download is not. Documented in `docs/channels.md` rather than left to be discovered.
DEENUU1
added a commit
that referenced
this pull request
Aug 15, 2026
## What this fixes Closes #547 — Telegram's polling loop and Mattermost's outgoing webhook each built an `IncomingMessage` of their own instead of calling `parse_incoming`, and both copies had drifted the same way: neither read attachments. ## Base is `main`, and the defect is there Checked before branching, because two issues this week turned out to live only on a feature branch. This one does not: | Symbol | On `main`? | |---|---| | `TelegramAdapter._handle_update` (`telegram.py:407`) | Yes — text only, `raw={}`, no `_attachments` | | `MattermostAdapter._from_webhook` (`mattermost.py:709`) | Yes — no `_attachments` | | `TelegramAdapter.parse_incoming` / `_attachments` | Yes | | `MattermostAdapter._from_socket` / `_attachments` | Yes | So this branch is cut from `origin/main` and is not stacked. ## What changed - **`backend/app/services/channels/telegram.py`** — `_handle_update` dumps the aiogram `Message` back into Bot API shape and hands it to `parse_incoming` rather than normalising it a second time. `by_alias` because `from` is a Python keyword and aiogram renames it; `exclude_none` because Telegram omits a field it has nothing for and `_attachments` reads presence. - **`backend/app/services/channels/mattermost.py`** — `_from_webhook` calls `_attachments`, and a post with files and no text is a message, the way the socket path already treats one. `_attachments` reads **both spellings** of `file_ids`: the socket sends a list, the outgoing webhook one comma-separated string, which a list-only reader would have iterated a character at a time. - **`backend/tests/test_channel_adapter_attachments.py`** — a `TestTelegramPolling` class driving `_handle_update` with the router and the session patched, plus three Mattermost webhook cases and one for the shared sender rule. - **`docs/channels.md`** — the Files section now says the parser is one per adapter and both transports reach it; the linking bullet carries the sender rule; and the fetch table carries the Mattermost webhook limitation below. **One behaviour was decided rather than merely deduplicated.** A message with no sender: the polling loop always refused one, the webhook parser did not. The Bot API leaves `from` empty for a message sent to a channel, and the webhook parser answered it under a chat identity keyed on an empty user id — one row shared by every senderless post the bot ever sees. `parse_incoming` refuses it on both transports now. That is the one place where unifying the two copies changes what the webhook path does, and it is the stricter half winning. The second commit is a correction to that reasoning, found reviewing my own diff: the first one said `from` is absent because that is how Telegram delivers an **anonymous group admin's** post. It is not — such a post carries a stand-in sender and meets the ordinary linking refusal instead. The guard is right; the reason given for it was wrong in the docstring, the docs page and a test, and a page promising operators a refusal that never fires is worse than no page. Two other claims went the same way: `exclude_none` is there for the display name (aiogram holds `last_name=None` where Telegram omits it, so leaving it in renames every surname-less sender to `Ada None`) and not for `_attachments`, which reads a `None` fine; and polling never sees an `edited_message` at all, because `@dp.message()` is the only handler, where the webhook receiver does read one. Both are now stated where they are true, and the first is pinned by a test that fails with exactly that string when the flag is dropped. ## How it failed before A spreadsheet dropped on a Telegram bot in **polling mode** — the mode a development or self-hosted deployment runs — was discarded with nothing logged, and the agent answered about a document it never received. A caption-only photo was dropped whole, the question about the picture along with it, because the handler read `message.text` and a caption is not text. The Mattermost outgoing-webhook path did the same to any post carrying files. This is exactly the failure #113 fixed for the webhook and socket transports; these two paths were never brought along, which is what the docstring on `base.py:101-109` claims is fixed. ## Testing Six new assertions fail against the old parsers and pass with these — confirmed by stashing the two `app/` files and re-running, not assumed: ``` FAILED TestTelegramReceiving::test_a_message_with_no_sender_is_nothing_on_this_transport_too FAILED TestTelegramPolling::test_a_document_sent_to_a_polling_bot_reaches_the_router FAILED TestTelegramPolling::test_a_caption_is_the_text_here_too FAILED TestTelegramPolling::test_the_update_is_carried_whole_rather_than_thrown_away FAILED TestMattermostReceiving::test_an_outgoing_webhook_post_carries_its_files_too FAILED TestMattermostReceiving::test_a_webhook_post_with_a_file_and_no_text_is_still_a_message ``` - `uv run pytest tests/test_channel_adapter_attachments.py tests/test_mattermost_channel.py tests/test_channel_mentions.py tests/test_channel_dedupe.py tests/test_channel_bot_org_scope.py tests/test_channel_supervisor.py tests/test_channel_attachments.py -q` → 159 passed - `make lint-backend` → clean - `make test` → 4384 passed, 6 skipped, platform layer at 100% - `make docs-build` → clean - CI on the first commit: `lint`, `test`, `e2e`, `docs`, `CodeQL`, `Security Scan` all pass; `test-frontend` and `docker` skip on changed paths. **`e2e` passed here** — worth saying because #685, on the `feat/agent-surfaces` base, reports four `surfaces.spec.ts` failures. That spec does not exist on `main`, which is the base this branch uses. Not covered by any of that: no real Telegram bot and no real Mattermost server were talked to. The aiogram dump is exercised against the library's own model, so what is verified is that `parse_incoming` reads what `Message.model_dump` writes — not that Telegram writes the same thing. The Bot API field names are what aiogram aliases to, which is the strongest local check available. ## Overlaps — merge order matters Four open branches touch this turn path. This one is the only one in the adapters; the other three are in the router or its neighbours, so nothing here conflicts textually with #684 or #689. - **#685** (#662) — also changes `_from_webhook`, adding `addressed=` to the same `IncomingMessage` return. **This will conflict**, and it is a one-line resolution: keep both `addressed=` and `attachments=` on the return, and keep this PR's `if not text and not attachments` in place of its `if not text`. #685 is stacked on `feat/agent-surfaces`; this one is on `main`, so whichever lands second rebases. - **#684** (#660) — `ChannelMessageRouter._receive_files` moved earlier in `_route_inner`. Different file, no textual overlap — but **this PR is what makes #660 reachable on Telegram polling at all.** `_answer_mention` receives the files before `ChannelAgentRouter.answer` raises `UnaddressedMessage` (`mentions.py:298`), which is the common case and the *only* case for a caption-less file, and `_route_inner` then receives them again: two downloads and two `ChatFile` rows per attachment, the first orphaned. A polling bot had no attachments before this branch, so it had nothing to duplicate. #684 is worth landing near this one. - **#689** (#661) — `_discard_files` on the router. Different file; no overlap. - **#690** — a successful channel turn never links its `ChatFile` rows. Not worked yet; this PR increases how often it has rows to fail to link. ## Noticed, not fixed - **Filed #692** — a Mattermost bot in **webhook mode** never reaches `remember_server`, because the only caller (`open_inbound_stream`) is fed by `get_active_polling_bots`, which filters `webhook_mode.is_(False)`. So an attachment on that transport now parses with an empty handle, `download_attachment` refuses it by design, and the reply says `<name>: could not be downloaded.` That is a strict improvement on dropping it silently, and it is not the file arriving. `docs/channels.md` records the limitation with the issue number. - `_attachments` reads `post["metadata"]` as a mapping without checking, so a payload where it is a string would raise. Pre-existing on the socket path and reachable only by whoever holds the bot token or the webhook secret; Mattermost itself never sends `metadata` in a webhook body, so the guard would be a branch no test could reach honestly. Left alone, and recorded here rather than silently. - The split on `file_ids` does not `strip()` its parts. Mattermost joins with a bare comma, so `"f1, f2"` is not a shape it produces; a lenient strip would be guessing at a format nobody sends. - #565 (lift the channel transport boilerplate into `base.py`) is the structural fix that would stop this class of drift. Not attempted here — this PR removes the duplication that had already drifted, not the shape that allowed it.
DEENUU1
added a commit
that referenced
this pull request
Aug 15, 2026
…718) Reopens #685, which GitHub closed automatically when its base branch `feat/agent-surfaces` was deleted on merge. Its own diff is replayed onto `main` rather than the stale branch rebased — that branch carries a pre-squash copy of all of #634 and conflicted in 35 files. An outgoing-webhook body carries no mention list, so the path left `addressed` unset and the router answered every post in a channel the bot was merely invited to. `trigger_word` is the payload's own record that the post was for this integration. **Two of #685's hunks are deliberately dropped**, both because main is newer: - the `@channel` / `@all` / `@here` paragraph — c900eaa decided those are *never* a mention, and #685 said the opposite; - the e2e publish locators — main already anchors them and drops the `.first()` #685 kept. Verified: `test_channel_addressing.py`, `test_channel_adapter_attachments.py` and `test_channel_mentions.py`, 109 passed. Refs #662
DEENUU1
added a commit
that referenced
this pull request
Aug 15, 2026
Gate a Mattermost webhook post the way the stream is — #718, closes #662. Replaces #685, which GitHub closed automatically when its base branch `feat/agent-surfaces` was deleted on merge; its diff was replayed onto `main` rather than the stale branch rebased, because that branch carried a pre-squash copy of all of #634 and conflicted in 35 files. Two of its hunks were deliberately dropped, both because main is newer: the `@channel` / `@all` / `@here` paragraph, where c900eaa decided those are never a mention and #685 said the opposite, and the e2e publish locators, which main already anchors. Version bumped in `backend/pyproject.toml`, `backend/uv.lock` and `frontend/package.json`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
Closes #662 — the Mattermost outgoing-webhook transport answered every post it
was handed, while the event stream had already stopped doing that.
Stacked, deliberately — base is
feat/agent-surfaces(#634), notmainNot an accident, and worth checking before merging. The addressing rule this
defect is about exists only on that branch:
main?IncomingMessage.addressedChannelMessageRouter._is_overheard/_names_the_botMattermostAdapter._addressed/_own_ids/_own_user_idtests/test_channel_addressing.pymain's router has no addressing gate on either transport — it answers everychannel message on both — so there is nothing on
mainto fix, and a regressiontest written there would assert behaviour that does not exist. Same shape as #658
finding the triggers feature only on PR #537.
Consequence for the merge: this is a stacked PR, so
gh pr merge --adminwill refuse it. Land #634 first and this retargets to
maincleanly; or mergemaininto the base and de-stack.What changed
backend/app/services/channels/mattermost.py—_from_webhooknow setsaddressed. An outgoing-webhook body carries no mention list, so what standsin for one is
trigger_word: the word an operator told this integration tofire on, which is Mattermost's own record that the post was for us. Empty means
the webhook fired on its channel filter, which delivers every post exactly as
the socket does —
False, for the same reason apostedevent with nomentions is. The reasoning is in the docstring.
backend/tests/test_channel_addressing.py— four cases for the webhooktransport, asserting
_is_overheardand not only the field.docs/channels.md— the addressing table now says it is the bot's rule andboth transports obey it, with a second table for how each one reads "named".
The trigger-word consequence is repeated once in the webhook setup steps,
where an operator is standing when they choose them.
A direct message and an
@agent-slugare untouched: both are decided before theflag is read, so
@sales what is the refund windowstill works over achannel-filtered webhook and a DM still answers everything.
The cost, and it is documented:
@the-botis not readable on this transport,because nothing in the body says which account the bot is. An operator who wants
that sets the trigger word to the bot's handle.
How it failed before
_from_webhookleftaddressedat its defaultNone. The router readsNoneas "the platform did not say" — deliberately, so Slack and Telegram keep working
— and answers. So a bot on a channel-watched outgoing webhook replied to
colleagues talking to each other, and posted its unknown-handle refusals
("@ada is not available on this bot") out loud in a channel it was never
addressed in.
Testing
The three new field assertions fail against the old parser with
AssertionError: assert None is Falseand pass with the fix; the overheardassertion fails with them.
uv run pytest tests/test_channel_addressing.py tests/test_mattermost_channel.py tests/test_channel_mentions.py -q→ 85 passedmake lint-backend→ cleanmake test→ 4622 passed, 7 skipped, coverage 100%make docs-build→ cleanCI is not fully green, and the red is not mine
lint,docs,Security Scanandtest(the backend suite plus the 100% gate)pass.
test-frontendanddockerskip — backend-and-docs-only paths, and askipped required check is a pass.
e2efails: 4 failed, 97 passed. All four aree2e/surfaces.spec.ts, allfour are the same assertion in the
publishPagehelper —expect(submit).toHaveCount(0)timing out at 15s with the Publish button stillon screen:
:64A published surface › is editable after it exists, and the change survives a reload:96A published surface › takes a picture of its own once it exists:128What the page offers a visitor › starts a fresh thread without losing the link:170What the page offers a visitor › offers dictation only where the operator turned it onConfirmed already red on the base ref rather than assumed. Run
31692218637
on
feat/agent-surfacesat sha3f58155— the head this branch was cut from —reports
4 failed, 96 passed: the same four specs, the same assertion. Mine hasone more pass, which is this branch's own added coverage.
The spec does not exist on
main(git ls-tree origin/main frontend/e2e/surfaces.spec.tsis empty) and nothing in this diff reaches ahosted surface — the whole change is a Mattermost webhook parser's return value,
its docstring, tests and docs. Not touched here; it belongs to #634.
Overlaps
#660 (a non-mention channel message with attachments stores every file twice)
and #661 (a channel turn refused before the run orphans its ChatFile rows) are
open against neighbouring code —
ChannelMessageRouter._receive_filesand itstwo call sites — and are being worked in parallel. Merge order matters.
This branch does not touch either: the whole diff is
_from_webhook's returnvalue, its docstring, tests and docs. Nothing here needs what #660 or #661 owns.
Noticed, not fixed
@channel/@all/@hereon the stream transport. The bot answers abroadcast wherever Mattermost lists it among the notified. #662 raised that as an
open question; this PR answers it by writing it down in
docs/channels.mdasintended — a private notion of "who was this for" here would disagree with the
one the whole channel was shown. Nobody has checked a real server to see when
Mattermost actually puts a bot in that list, so if it turns out to be noisy in
practice that is a new issue rather than a silent filter added on a guess.