Skip to content

fix(channels): keep slash ButtonCallback actions dispatchable through debounce coalescing - #8117

Merged
houko merged 6 commits into
librefang:mainfrom
DaBlitzStein:fix/telegram-agent-switch-buttons
Sep 2, 2026
Merged

houko merged 6 commits into
librefang:mainfrom
DaBlitzStein:fix/telegram-agent-switch-buttons

Conversation

@DaBlitzStein

@DaBlitzStein DaBlitzStein commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Tapping a slash-command inline-keyboard button (e.g. /agent X from the /agents keyboard) inside a message-debounce window did nothing.
    content_to_text rendered every ButtonCallback as [Button: {action}], and the debounced merge (drain -> merged Text -> dispatch_message) builds agent-facing text from those placeholders, so the coalesced message arrived as prose and the slash-command dispatcher never saw it.
    The immediate (non-debounced) path was never affected: dispatch_message's own ButtonCallback arm already returns the bare action for slash-prefixed callbacks.
  • The fix is one arm in content_to_text (crates/librefang-channels/src/bridge.rs): a slash-prefixed action renders as the bare command line, non-command actions keep the human-readable [Button: …] placeholder.

Out-of-scope history

  • The first iteration added a ~58-line early-return in dispatch_message that re-implemented the command dispatcher; it was rejected in review (it preempted the canonical dispatcher, short-circuited the /agents and /models inline-keyboard special cases, and hard-refused policy-blocked commands instead of forwarding them to the agent as text) and has been reverted (0b99d59).
    This iteration is the one-line normalization the review asked for, plus tests.

Verification

  • cargo check -p librefang-channels --lib — clean
  • cargo clippy -p librefang-channels --lib -- -D warnings — zero warnings
  • cargo test -p librefang-channels — 655 pass / 0 fail across all suites (607 lib unit, 32 bridge integration, 7 sidecar protocol conformance, 6 sidecar version contract, 3 doc-tests)
  • New tests (both verified individually by name):
    • bridge::tests::test_content_to_text_button_callback_slash_action_is_command_text — content_to_text(&ButtonCallback { action: "/agent foo" }) returns /agent foo, not [Button: /agent foo]
    • bridge::tests::message_debouncer::test_debouncer_button_callback_slash_actions_coalesce_as_command_text — two buffered slash-action ButtonCallbacks drain as dispatchable command text (/agent foo + /agent bar), not [Button: …] placeholders
  • Doc correction on the same file: sanitizer_text_to_check's doc listed ButtonCallback among the None variants, but its arm returns Some(action) since fix: security and correctness hardening from repo-wide audit #6438 (c7fa79a).
  • Changelog fragment: changelog.d/fixed/8117-button-callback-slash-debounce-coalescing.md

…d dispatcher

ButtonCallback actions from inline keyboards (e.g. "/agent X" buttons
generated by /agents) were converted to "[Button: /agent X]" by
content_to_text, which never matched the starts_with('/') slash-command
re-parser. Only prov:/model:/back: prefixed callbacks were intercepted.

Route any ButtonCallback whose action starts with '/' through the same
command dispatcher used for typed commands: parse, authorize, dispatch
via handle_command, and return.

@houko houko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The premise this patch is built on does not hold for the code path it patches, and as written it preempts the canonical dispatcher and changes two behaviours.

Blocking

1. dispatch_message already routes slash-prefixed button actions through the dispatcher.

The comment says "content_to_text wraps them in [Button: /agent X] which never matches the starts_with('/') check". content_to_text is not what extracts the text here. dispatch_message has its own inline let text = match &message.content at bridge.rs:4483, and its ButtonCallback arm at bridge.rs:4538-4551 is:

// If action starts with '/', treat it as a slash command directly.
// This allows interactive buttons (e.g. Approve/Reject on approval
// notifications) to trigger commands like /approve or /reject.
if action.starts_with('/') {
    action.clone()
} else { /* "[Button clicked: …]" */ }

text then reaches the dispatcher at bridge.rs:4605, which does the identical splitn(2, ' ') / is_channel_command / is_command_allowed / authorize_channel_user(ct_str, …, "chat") / handle_command / suppress_button_command_ack sequence this PR adds — including the RBAC check whose comment already names the button case ("reconstructed from a button", bridge.rs:4622-4627, the #6441 follow-up).

If you have a reproduction where a /agent X button does nothing, please attach it — the fix is likely somewhere else, see #3 below.

2. The new block short-circuits the /agents and /models inline-keyboard special cases.

Because it returns before let text = …, a button action of /agents or /models no longer reaches bridge.rs:4639-4710 and gets a flat text reply instead of an inline keyboard. Both other dispatch sites carry these special cases (bridge.rs:4230-4293 for typed commands, bridge.rs:4639-4710 for text/button slash commands); this makes a third site that does not, so a clicked command and a typed command diverge.

3. Policy-blocked commands now hard-refuse instead of reaching the agent.

Both existing sites treat "not allowed in this channel" as forward the text to the agent:

debug!(command = cmd, channel = ct_str,
       "Command blocked by channel policy — forwarding to agent as text");

(bridge.rs:4310-4314 and bridge.rs:4727-4733, with the explicit // Other slash commands (and blocked ones) pass through to the agent at 4735.)

The new branch instead sends "Command /{cmd} is not allowed in this channel." and returns, so the agent never sees the message. That is a deliberate policy decision made in two other places and reversed here without discussion.

4. 58 duplicated lines is the wrong shape for this even if a gap existed.

The comment itself states the goal — "must be re-injected as command text so the slash-command dispatcher at L4608+ picks them up" — and then re-implements the dispatcher rather than re-injecting. The auth check, the allow-list check, the ack suppression and the arg parsing now exist in three copies; the next change to any of them (a rate limit, an audit record, quoted-argument parsing) will land in one or two of the three. If a gap is found, normalize the content into text and fall through.

Where the real bug probably is

content_to_text's ButtonCallback arm (bridge.rs:1056) does render [Button: {action}] with no slash passthrough, and it feeds flush_debounced (bridge.rs:970-982, 1185). So a button press that arrives inside a debounce window is coalesced into [Button: /agent X] and dispatched as prose — the dispatcher never sees it. That matches the symptom, is one line rather than 58, and is testable without a live channel:

  • content_to_text(&ButtonCallback { action: "/agent foo" }) returns /agent foo.
  • A flush_debounced case where a single buffered ButtonCallback with a slash action produces dispatchable command text.

Either way this needs a test — bridge.rs has the fixtures for it already (see suppress_button_command_ack_* at bridge.rs:7429-7467 and the ButtonCallback constructions at 10164 / 10306) — and a changelog fragment (changelog.d/fixed/8117-....md).

@github-actions github-actions Bot added the needs-changes Changes requested by reviewer label Sep 1, 2026
@DaBlitzStein

Copy link
Copy Markdown
Contributor Author

Fixes #8118

…ssage

Reverts 1a9e35e per review: dispatch_message's own ButtonCallback arm
already returns the bare action for slash-prefixed callbacks and feeds
the canonical dispatcher, so the early-return re-implemented it. It also
short-circuited the /agents and /models inline-keyboard special cases
and hard-refused policy-blocked commands instead of forwarding them to
the agent as text.
… debounce coalescing

content_to_text rendered every ButtonCallback as "[Button: {action}]".
The debounced merge (drain -> merged Text -> dispatch_message) builds
agent-facing text from those placeholders, so a slash-prefixed action
such as "/agent X" that coalesced with another message arrived as
prose and the slash-command dispatcher never saw it. Slash-prefixed
actions now render as the bare command line; non-command actions keep
the human-readable placeholder.
@DaBlitzStein DaBlitzStein changed the title fix(channels): route ButtonCallback slash commands through the command dispatcher fix(channels): keep slash ButtonCallback actions dispatchable through debounce coalescing Sep 1, 2026
The doc listed ButtonCallback among the variants that return None, but
the arm added in the same commit (46ffd3b, librefang#6438) returns
Some(action) — the action is attacker-controlled text rendered into the
prompt, so it is scanned. State the actual behaviour.
…angelog fragment

The debouncer gains a single-buffered drain case for a slash-action ButtonCallback, pinning that the lone-message fast path returns the callback untouched so dispatch_message's ButtonCallback arm still dispatches it as a slash command.
The changelog fragment moves to changelog.d/fixed/8117-telegram-button-callback.md per the review request, and the sanitizer_text_to_check doc comment sentence is split at a sentence boundary.
@DaBlitzStein

Copy link
Copy Markdown
Contributor Author

Addressed the review feedback.

The branch keeps the content_to_text slash-passthrough fix and now pins it with three unit tests: test_content_to_text_button_callback_slash_action_is_command_text, test_debouncer_button_callback_slash_actions_coalesce_as_command_text, and test_debouncer_single_button_callback_slash_action_survives_drain_unchanged.
Changelog fragment renamed to changelog.d/fixed/8117-telegram-button-callback.md.

Verified: cargo test -p librefang-channels --lib → 608 passed, 0 failed.
Commit 5f5cd0f.

@houko houko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right fix. The 58-line dispatcher copy is gone, the one-line change is in content_to_text, and the diff is now +98/-2 with most of that being tests.

Verified rather than taken from the commit messages:

The fix is at the layer that had the bug. content_to_text's ButtonCallback arm now passes a slash-prefixed action through bare, so the debounced merge at bridge.rs:970-982 / :1195 builds dispatchable command text instead of [Button: /agent X]. The immediate path was never broken — dispatch_message has its own inline extraction — and this change leaves it untouched, so the two paths now agree instead of one preempting the other.

No behaviour was reversed this time. The /agents and /models inline-keyboard special cases and the "blocked commands forward to the agent" policy are both reached exactly as before, since nothing returns early any more.

The stale sanitizer_text_to_check doc was worth fixing and the fix is doc-only, correctly. The implementation already returned Some(action.clone()) on main (bridge.rs:1133) while the doc listed "ButtonCallback action" among the variants that return None — actively wrong, and wrong in the direction that matters, because that list is what the next person adding a text-bearing variant will use to classify it. Confirmed the arm is Some(action.clone()) on both main and this head, so there is no Block-mode gap here and this PR does not introduce or close one.

Breadth-scanned the other consumers. Besides the debounce sites, the only non-test caller is the message journal at bridge.rs:6835. A slash-action tap now journals as /agent foo rather than [Button: /agent foo], which is more faithful to what actually gets dispatched and matches the immediate path. Nothing else reads this.

Tests are the right three: the unit case on content_to_text, the lone-callback drain (the realistic single-tap path, which exercises drain's single-message fast path so dispatch_message still sees the ButtonCallback arm), and the two-message coalesce. test_content_to_text_button_callback with a non-slash approve action still asserts the placeholder, so the non-command shape is pinned too. Changelog fragment present and scripts/check-changelog-attribution.py passes.

Non-blocking observation

test_debouncer_button_callback_slash_actions_coalesce_as_command_text documents that two rapid taps merge to /agent foo\n/agent bar, which the dispatcher's splitn(2, ' ') will read as /agent with args foo\n/agent bar. That is pre-existing debounce behaviour for any two slash commands in one window, typed ones included, and not something this PR introduces — but the test now makes it visible, which is better than leaving it implicit. If it ever turns out to matter in practice it wants its own issue against the coalescer, not a change here.

Approving.

@houko
houko merged commit 00ad340 into librefang:main Sep 2, 2026
42 checks passed
@github-actions github-actions Bot added ready-for-review PR is ready for maintainer review and removed needs-changes Changes requested by reviewer labels Sep 2, 2026
@DaBlitzStein
DaBlitzStein deleted the fix/telegram-agent-switch-buttons branch September 11, 2026 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/channels Messaging channel adapters ready-for-review PR is ready for maintainer review size/M 50-249 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants