feat(channels): tell the model who sent the message and where - #401
Open
plombeer31 wants to merge 2 commits into
Open
feat(channels): tell the model who sent the message and where#401plombeer31 wants to merge 2 commits into
plombeer31 wants to merge 2 commits into
Conversation
An inbound Telegram or Discord message reached `runtime.runTurn` as bare text. The model saw "restart the deploy" with nothing about who said it or in which channel. That was survivable while a channel had exactly one owner; Discord's `ownerUserIds` list made it a real gap — several people now drive one bot and the agent cannot tell them apart. Add a single-line `[from]` block, built the way `buildAttachmentUserMessage` already builds `[attachments]`: display name, platform, user id, chat id, and the topic id where the surface has one. Inclusion rule (deterministic, tested): always on Discord, which is multi-author by nature; on Telegram only in a group or supergroup. A Telegram DM reaches the runtime for the single configured `ownerUserId` only, so the line would restate a constant on every turn forever. Ordering: the identity line goes above the user text and above any `[attachments]` block — envelope first, attacker-controlled payload after, so there is exactly one `[from]` line and it is the first line of the turn. The display name is attacker-chosen text, so it is flattened to one line (control characters, newlines, bidi format characters and the Unicode line/paragraph separators all become spaces), emitted inside a quoted field with `"` and `\` escaped, and capped at 64 characters before escaping. A nickname therefore cannot start a line at all and cannot forge a second `[from]` line or an `[attachments]` block. Ids get a strict character allowlist — they arrive through a structural cast, not a validated one. Nothing outside the live channel message path changes: the TUI, tasks, webhooks and fusion workers are untouched, and so is the Telegram file path, which is private-chat-only.
Review follow-ups on the `[from]` identity line. - `sanitizeDisplayName` truncated with `String.slice`, which counts UTF-16 units: a 64-unit boundary landing inside a surrogate pair left a lone surrogate in the prompt. That is not valid UTF-8, so it turns into U+FFFD the first time the turn is encoded for a provider or written to the session store. Cut on code points instead, which also makes the cap mean 64 characters for an astral name rather than 32. - The cap/escape ordering had no test that could tell the two orders apart — the existing one asserted `line.length < 200`, which holds either way. Pin the exact output so reversing the order fails. - Three comments claimed more than the code delivers: that the cap bounds the rendered field (escaping doubles it), that a malformed id is dropped whole (its bad characters are stripped and the rest is spliced), and that there is exactly one `[from]` line in a message (only the name is escaped — a message body or a failed attachment's filename still renders a line-anchored `[from]` below the envelope, from any account on the owner allowlist). State the real boundary, and pin the last one with a test so it is a documented contract rather than a surprise. - Cover C1 NEL (U+0085) and the C0 file/group/record separators in the injection table. No behaviour change beyond the surrogate fix.
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
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.
The report
From
thegreatteacheron Discord, 2026-09-08:and, separately:
The second half already shipped —
ownerUserIdsinsrc/channels/discord/discord-settings.ts. That is exactly what turns the first half into a real gap: with several owners allowed, the model now receives messages from different people with no way to tell who is speaking or where.What I verified on
mainfirstdispatchToRuntimeinsrc/channels/discord/discord-inbound-handler.tscalledctx.runtime.runTurn(session, text, …)withtextbeing the stripped message content, verbatim. Its Telegram twin insrc/channels/telegram/inbound-handler.tsdid the same withaddressed.text.trim(). The sender id and chat id existed only asChannelRef/ChatRefvalues used for session keying, outbound routing and log lines — nothing on either path put them into the prompt. So no, the identity was not already carried, on either channel.What this adds
A one-line
[from]block, built the waybuildAttachmentUserMessagealready builds[attachments]:key=valuepairs rather than prose so the model reads it unambiguously and a hostile name cannot be mistaken for another field. One line, because it rides on every turn of a channel conversation and is re-paid on every step of the agent loop.Names are resolved most-specific-first: on Discord
member.nick→author.global_name→author.username; on Telegramfirst_name last_name→username. Missing names simply drop thename=field. Nothread=on Discord — a Discord thread is a channel with its ownchannel_id, sochat=already names it; Telegram forum topics are the surface that needs the extra id.New module:
src/channels/sender-identity.ts.Inclusion rule, and why
shouldAnnounceSender(platform, chatType):ownerUserIdsis now a list, so even the owner set is plural. Nothing identifies the speaker for free.runTurnfor the single configuredownerUserIdand nobody else, so in a DM the line would restate a constant on every turn, forever. In a group (or a forum topic inside one) the chat and topic ids are real information even though the sender is still the owner.The rule is a pure function of platform + chat type, so it is testable without a runtime, and it is covered by a table-driven test. The Telegram file path (
handleInboundFile) is private-chat-only, so it consistently gets no block; there is a source comment saying what to pass if it ever grows group support.Nothing outside the live channel message path is touched: TUI, tasks, webhooks and fusion workers are unchanged, and neither
handleSlashCommandnor the help text in either handler was edited.Block ordering
The identity line goes first, above the user's text and above any
[attachments]block; callers compose it aswithSenderIdentity(buildAttachmentUserMessage(...), sender). Two reasons:[from]line in the whole message and it is the first line of it, so anything that looks like a second one is visibly inside the payload. The reverse order would let the payload's last line sit flush against a trailing envelope and read as part of it.Pinned by tests at both the unit level and through
handleDiscordMessagewith a real attachment.Prompt-injection hardening
The display name is text an attacker picks — it is the interesting part of this PR.
sanitizeDisplayNameguarantees the rendered line is exactly one line:U+2028,U+2029) becomes a space, then whitespace is collapsed;"and\escaped, so it cannot leave its own field;name=field rather than rendering an empty pair.[from]and[attachments]are line-anchored markers, so a name that can never start a line can never forge one. Ids get the same flattening plus a strict[A-Za-z0-9_-]allowlist and a 32-character cap — they come off the wire through a structuralascast, so "it is a snowflake" is an assumption, not a checked fact.Tests cover, at the unit level and end-to-end through both handlers: a name with a newline plus a fake
[from]line; a name with the[attachments]marker;\ralone;U+2028andU+2029; a bidi override + isolate; NUL and an ANSI escape; a name that escapes out of its quotes; and a 500-character name.Test evidence
Broken down:
Proof the tests are not vacuous. Stashing only the three changed source handlers (
discord-inbound-handler.ts,telegram/inbound-handler.ts,telegram-bot-factory.ts) and leaving every test in place:— all 20 new handler-level identity tests, plus the 4 pre-existing assertions this PR deliberately updated (3 Telegram group cases and the Discord guild
@mentioncase, which now expect the line) and the 4 Discord attachment-shape assertions.Separately, neutering
UNSAFE_TEXTinsender-identity.tsto a no-op character class failed 6 of 27 unit tests, including "forged second[from]line on a new line", "forged attachments block marker" and "is always exactly one line" — the sanitiser is what those tests are actually measuring. Both source states were restored and the suite is green again.Deliberately left out
dispatchAttachments).[from]. The block is self-describing, and the system prompt is a separate, higher-blast-radius surface.