fix(channels/telegram): wire ApprovalGate end-to-end for Telegram turns (#3098 sub-issue 2)#3232
Conversation
…ns (tinyhumansai#3098) Sub-issue 2 of tinyhumansai#3098 is more serious than the "Telegram is read-only" framing suggests. The actual gap: every non-web channel (Telegram, Discord, Slack, iMessage, Mattermost) silently bypasses the `ApprovalGate`. The dispatch loop in `channels/runtime/dispatch.rs` invokes the agent turn without setting an `ApprovalChatContext`, so the gate's "no chat context → allow straight through" branch (`approval/gate.rs:219-231`) fires for every `Prompt`-class tool call. A user on `level=supervised` gets the same unprompted behavior as `level=full` over Telegram — which voids the entire supervised approval model and is the most likely reason the reporter saw "files can't be committed via Telegram" (the operation either ran silently or produced a result the bot never surfaced clearly). This PR closes the gap for Telegram only: - New `TelegramApprovalSurfaceSubscriber` (`channels/providers/telegram/ approval_surface.rs`) listens on the `channel` + `approval` event domains. Inbound `ChannelMessageReceived` events on `telegram` populate a small `thread_id → (reply_target, thread_ts)` index. Outbound `ApprovalRequested` events with `client_id="telegram"` are rendered as a "🔐 Approval needed / reply yes or no" message sent through the registered `TelegramChannel`. - `dispatch.rs` scopes the agent turn invocation in an `APPROVAL_CHAT_CONTEXT.scope(...)` for channels that have a registered approval surface — currently Telegram only via `channel_has_approval_surface("telegram")`. With the context set, the gate parks `Prompt`-class tool calls instead of allowing them. - `dispatch.rs` also intercepts inbound `yes`/`no` chat replies BEFORE the normal turn dispatch via `try_route_approval_reply`. When a parked approval exists for the same `conversation_history_key`, the reply is routed to `ApprovalGate::decide` (resuming the parked tool call) instead of starting a fresh turn. Mirrors the web channel intercept at `channels/providers/web.rs:493-525`. - `startup.rs` registers `TelegramApprovalSurfaceSubscriber` when the Telegram channel is enabled, alongside the existing `TelegramRemoteSubscriber`. Discord / Slack / iMessage / Mattermost intentionally STAY in the legacy "no chat context → silently allow" state. Setting the context without a surface subscriber would make every parked approval TTL-deny with no UI for the user to answer — strictly worse than the bypass. Each will get its own per-channel surface PR. Tests added: - 8 unit tests on the subscriber (`approval_surface_tests.rs`) covering inbound context capture, channel scoping (reject non-telegram and non-web events), client_id scoping, missing-context safety, and the prompt body format. - 3 inline gating tests on `channel_has_approval_surface` in `dispatch.rs` pin the matrix: telegram in, others out. All 1163 channels + approval tests pass. cargo fmt clean. Closes sub-issue 2 of tinyhumansai#3098 for the Telegram path. Three follow-ups remain (Discord / Slack / iMessage approval surfaces), each tracked separately.
📝 WalkthroughWalkthroughThis PR introduces a Telegram approval surface: a subscriber that records inbound Telegram message context, routes parked tool-call approvals into Telegram chat threads as yes/no prompts, integrates approval-reply detection into the channel dispatch loop, and wires the subscriber at startup. ChangesTelegram Approval Surface
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/channels/providers/telegram/approval_surface.rs (1)
162-166: 💤 Low valueConsider redacting
reply_targetin logs to avoid PII leakage.The
reply_targetfield may contain user identifiers (e.g., Telegram chat IDs or usernames) that could be considered PII. As per coding guidelines, logs should not contain full PII.🔒 Proposed redaction approach
tracing::info!( "{LOG_PREFIX} surfacing approval prompt request_id={request_id} tool={tool_name} \ - thread_id={thread_id} reply_target={}", - reply_ctx.reply_target + thread_id={thread_id} reply_target=[REDACTED]" );Based on learnings, never log secrets or full PII — redact sensitive information in logs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/channels/providers/telegram/approval_surface.rs` around lines 162 - 166, The log currently prints reply_ctx.reply_target (via tracing::info with LOG_PREFIX, request_id, tool_name, thread_id), which may leak PII; change the call to log a redacted or derived value instead (e.g., mask most characters, emit only a hashed or last-N chars, or a boolean like "present") by replacing reply_ctx.reply_target with a sanitized string produced by a helper (e.g., redact_pii or mask_reply_target) and use that sanitized value in the tracing::info invocation so only non-identifying info is logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/channels/providers/telegram/approval_surface.rs`:
- Around line 162-166: The log currently prints reply_ctx.reply_target (via
tracing::info with LOG_PREFIX, request_id, tool_name, thread_id), which may leak
PII; change the call to log a redacted or derived value instead (e.g., mask most
characters, emit only a hashed or last-N chars, or a boolean like "present") by
replacing reply_ctx.reply_target with a sanitized string produced by a helper
(e.g., redact_pii or mask_reply_target) and use that sanitized value in the
tracing::info invocation so only non-identifying info is logged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 653453ca-88fa-473b-9575-7e300a72ec96
📒 Files selected for processing (5)
src/openhuman/channels/providers/telegram/approval_surface.rssrc/openhuman/channels/providers/telegram/approval_surface_tests.rssrc/openhuman/channels/providers/telegram/mod.rssrc/openhuman/channels/runtime/dispatch.rssrc/openhuman/channels/runtime/startup.rs
Summary
ApprovalChatContextso theApprovalGateactually fires forPrompt-class tool calls instead of silently allowing them. The user gets an in-chat approval prompt; replyingyes/noresumes the parked tool call viaApprovalGate::decide.TelegramApprovalSurfaceSubscriberbridgesDomainEvent::ApprovalRequestedevents whoseclient_id="telegram"into Telegram messages, looking up the right reply target from inbound message history.Problem
Sub-issue 2 of #3098 says "cannot commit files via Telegram." On investigation the actual gap is much more serious than the reported symptom:
channels/runtime/dispatch.rsinvokes the agent turn (request_native_global(agent.run_turn, ...)) without wrapping it in anAPPROVAL_CHAT_CONTEXT.scope(...).ApprovalGateexplicitly handles "no chat context" as "background / triage / cron turn — pre-authorized, allow straight through" (approval/gate.rs:219-231).Prompt-class tool call (file_write, edit_file, shellcd && git commit, curl, npm_exec, …) initiated over Telegram executes without any approval prompt, regardless of autonomy tier.level=supervisedis silently identical tolevel=fullon Telegram — voiding the supervised approval model.The reporter's stated symptom ("can't commit files") is most plausibly explained by this gap interacting with
level=readonly(policy-layer block fires before the gate even runs and produces a cryptic error), or bylevel=fullactually committing files silently but the bot reply not surfacing the result clearly.This same gap affects Discord, Slack, iMessage, and Mattermost — but this PR is scoped to Telegram (the reported channel).
Solution
Mirror the existing web channel pattern for Telegram:
1. Scope the agent turn (
channels/runtime/dispatch.rs)Add
channel_has_approval_surface(channel: &str) -> boolthat returnstrueonly for channels with a registered approval surface (Telegram today). In the dispatch loop, wrap the agent turn invocation inAPPROVAL_CHAT_CONTEXT.scope(ApprovalChatContext { thread_id: history_key, client_id: msg.channel }, agent_call)when the gate returnstrue. Other channels execute the agent turn unscoped, preserving the legacy bypass until they get their own surface.2. Intercept yes/no replies for parked approvals
Add
try_route_approval_reply(msg) -> boolthat — for channels with a surface — checksApprovalGate::pending_for_thread(&history_key)and routes ayes/noreply toApprovalGate::decide(...). Mirrorschannels/providers/web.rs:493-525. Non-yes/no replies fall through to the normal dispatch (the user is redirecting; existing turn cancellation kicks in).3. New
TelegramApprovalSurfaceSubscriberchannels/providers/telegram/approval_surface.rs. Subscribes to bothchannelandapprovaldomains:ChannelMessageReceivedontelegram→ records(thread_id, reply_target, thread_ts)in an in-memory index. Samethread_idshape asconversation_history_key(pinned bytelegram_history_key_matches_format_in_dispatch_helpertest — drift would silently miss every parked approval).ApprovalRequestedwithclient_id="telegram"→ looks up the index, renders a🔐 Approval needed / Tool: X / Action: Y / Reply yes to approve or no to deny.message, sends it via the registeredTelegramChannel.Registered in
channels/runtime/startup.rsonly when Telegram is enabled in config.Submission Checklist
channel_has_approval_surface. Failure paths covered: missing context, non-telegram client_id, missing client_id, non-telegram channel inbound, unknown channel.send_approval_prompthappy/missing-context paths,record_reply_contextfrom inbound events,channel_has_approval_surfacefor both telegram and rejected channels,format_approval_promptbody shape, history-key format parity.N/A: behavior fix on the existing "Telegram channel" capability; no new feature row.## Related—N/A: no matrix row added or removed.RecordingChannelmock; no Telegram Bot API calls.docs/RELEASE-MANUAL-SMOKE.md) —N/A: behavioral fix on an existing surface; no new release-cut item.Closes #NNNin the## Relatedsection — see below (Telegram interface ignores local model selection + can't commit files or run skills via Telegram on macOS #3098 has 4 sub-issues; this PR closes sub-issue 2 only, issue stays open).Impact
level=supervised: previously silently bypassed; now actually get an approval prompt in the chat for eachPrompt-class tool call. This is a security improvement — the supervised model now works as documented for Telegram.level=full: identical behavior for the Allow paths; Network/Install/Destructive tools (alreadyPrompt-class underfull) now correctly prompt instead of silently running.level=readonly: unchanged. Policy-layer block still fires before the gate.ApprovalChatContextand uses its own subscriber.channel_has_approval_surfacereturns false for them — they keep the legacy bypass until each gets its own surface PR (intentional; seeapproval_surface_gating_tests::other_channels_do_not_yet_have_an_approval_surface).tokio::task_local!scope per Telegram turn (negligible) and one extraHashMaplookup per inbound Telegram message (subscriber's index update). No new I/O on the hot path.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/3098-telegram-approval-surfacea49b1e3842f108f913b8ca0bec52dee07d4e3282Validation Run
pnpm --filter openhuman-app format:check—N/A: Rust-only change.pnpm typecheck—N/A: no TS/TSX touched.cargo test --lib -p openhuman channels:: approval::→ 1163 passed, 0 failed. Includes the 8 new subscriber tests + 3 new dispatch gating tests + all pre-existing channels/approval coverage.cargo fmt --checkclean;cargo check --libclean (pre-existing warnings only).N/A: app/src-tauri not touched.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
ApprovalGateinstead of silently bypassing it.level=supervisedtool calls park and surface as a Telegram message asking yes/no; the user's reply resumes or denies the parked call.supervisednow see approval prompts in the chat (previously: silent execution). Telegram users onfullsee prompts for Network / Install / Destructive tools (previously: silent execution). Telegram users onreadonlysee no change.Parity Contract
channel_has_approval_surfacegate is a per-channel allowlist; onlytelegramevaluatestrue. Pinned byapproval_surface_gating_tests::other_channels_do_not_yet_have_an_approval_surface. The web channel is unaffected — it sets its ownApprovalChatContextinchannels/providers/web.rs:472-477and has its own surface subscriber.ApprovalGate's "no chat context → allow" branch (approval/gate.rs:219-231) is intentionally NOT changed — background / triage / cron turns continue to flow through unchanged. The gate's TTL deny remains the safety net. The Telegram intercept (yes/no reply routing) only fires whenpending_for_threadreturnsSome, so non-approval messages dispatch normally.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Tests