docs(spec): telegram remote-control phase 2 — inline approvals (#1805)#2502
Conversation
…tinyhumansai#1805) Design doc for phase 2 of tinyhumansai#1805. Scope: route ApprovalGate prompts to all bound Telegram chats with inline approve / approve-always / deny buttons, edit messages cross-chat on decision with actor + surface attribution, and add /pending for on-demand recovery. Approvals only — Q&A and other remote-control slices (live ticks, /abort, /task, /files, /models, /worktree) are explicit non-goals, each its own future spec.
📝 WalkthroughWalkthroughThis PR introduces a comprehensive design specification for Telegram inline approvals (phase 2 of the remote-control initiative). The spec details multi-chat approval broadcasting, callback-driven decision handling with identity attribution, a ChangesTelegram Inline Approvals Specification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
the
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@docs/superpowers/specs/2026-05-23-telegram-inline-approvals-design.md`:
- Line 478: The table row contains unescaped pipe characters in the snippet
`appr:<o|a|d>:` which breaks the Markdown table parse; update the table cell to
escape the pipes (e.g. `appr:<o\|a\|d>:`) or wrap the entire fragment in a code
span that preserves pipes, and ensure the same escaped form or explanatory note
is used where `approval_keyboard` is described so the table stays valid and the
construction-time validation remark still matches the token format.
- Around line 25-77: The fenced architecture diagram block (the block showing
ApprovalGate, DomainEvent::ApprovalRequested, TelegramApprovalSubscriber,
channel_recv.rs callback_query handler, etc.) is missing a language tag which
triggers MD040; update the opening fence from ``` to ```text so the diagram is
fenced as a text block and no other changes are needed. Ensure the closing fence
remains ``` and leave the inner diagram (ApprovalGate, Telegram Bot API,
TelegramApprovalSubscriber.handle, pending_map.remove) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f49d7b8-1355-492c-8588-413c25de5f09
📒 Files selected for processing (1)
docs/superpowers/specs/2026-05-23-telegram-inline-approvals-design.md
| ``` | ||
| ┌──────────────────┐ | ||
| │ ApprovalGate │ intercepts external-effect tool calls | ||
| │ (existing) │ parks future on oneshot | ||
| └────────┬─────────┘ | ||
| │ publishes | ||
| ▼ | ||
| ┌───────────────────────────────┐ | ||
| │ DomainEvent::ApprovalRequested│ | ||
| └─────────────┬─────────────────┘ | ||
| │ event bus | ||
| ┌─────────────────────┼─────────────────────┐ | ||
| ▼ ▼ ▼ | ||
| ┌────────────────┐ ┌──────────────────────┐ ┌────────────────┐ | ||
| │ Desktop UI │ │ TelegramApproval- │ │ (future │ | ||
| │ (existing) │ │ Subscriber (new) │ │ providers) │ | ||
| └───────┬────────┘ └──────────┬───────────┘ └────────────────┘ | ||
| │ │ | ||
| │ │ for every bound chat: | ||
| │ │ sendMessage + inline_keyboard | ||
| │ │ record (chat_id, msg_id) in pending_map | ||
| │ ▼ | ||
| │ ┌─────────────────┐ | ||
| │ │ Telegram Bot │ | ||
| │ │ API │ | ||
| │ └────────┬────────┘ | ||
| │ │ user taps button | ||
| │ ▼ | ||
| │ ┌─────────────────────────┐ | ||
| │ │ channel_recv.rs: │ | ||
| │ │ callback_query handler │ ← allowlist re-check | ||
| │ │ (new branch) │ | ||
| │ └────────────┬────────────┘ | ||
| │ │ approval_decide( | ||
| │ │ request_id, decision, | ||
| │ │ actor=@user, surface="telegram") | ||
| ▼ ▼ | ||
| approval_decide RPC ───────┐ | ||
| │ publishes | ||
| ▼ | ||
| ┌──────────────────────────────────┐ | ||
| │ DomainEvent::ApprovalDecided │ | ||
| │ (extended w/ decided_by_actor + │ | ||
| │ decided_by_surface) │ | ||
| └────────────────┬─────────────────┘ | ||
| │ | ||
| ▼ | ||
| TelegramApprovalSubscriber.handle() | ||
| for each (chat_id, msg_id) in pending_map: | ||
| editMessageText("✓ decided by @who via X") | ||
| [remove inline_keyboard] | ||
| pending_map.remove(request_id) | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the architecture fenced block.
Line 25 opens a fenced code block without a language, which triggers MD040 and reduces renderer/tooling consistency.
Proposed fix
-```
+```text
┌──────────────────┐
│ ApprovalGate │ intercepts external-effect tool calls
...
pending_map.remove(request_id)
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| ┌──────────────────┐ | |
| │ ApprovalGate │ intercepts external-effect tool calls | |
| │ (existing) │ parks future on oneshot | |
| └────────┬─────────┘ | |
| │ publishes | |
| ▼ | |
| ┌───────────────────────────────┐ | |
| │ DomainEvent::ApprovalRequested│ | |
| └─────────────┬─────────────────┘ | |
| │ event bus | |
| ┌─────────────────────┼─────────────────────┐ | |
| ▼ ▼ ▼ | |
| ┌────────────────┐ ┌──────────────────────┐ ┌────────────────┐ | |
| │ Desktop UI │ │ TelegramApproval- │ │ (future │ | |
| │ (existing) │ │ Subscriber (new) │ │ providers) │ | |
| └───────┬────────┘ └──────────┬───────────┘ └────────────────┘ | |
| │ │ | |
| │ │ for every bound chat: | |
| │ │ sendMessage + inline_keyboard | |
| │ │ record (chat_id, msg_id) in pending_map | |
| │ ▼ | |
| │ ┌─────────────────┐ | |
| │ │ Telegram Bot │ | |
| │ │ API │ | |
| │ └────────┬────────┘ | |
| │ │ user taps button | |
| │ ▼ | |
| │ ┌─────────────────────────┐ | |
| │ │ channel_recv.rs: │ | |
| │ │ callback_query handler │ ← allowlist re-check | |
| │ │ (new branch) │ | |
| │ └────────────┬────────────┘ | |
| │ │ approval_decide( | |
| │ │ request_id, decision, | |
| │ │ actor=@user, surface="telegram") | |
| ▼ ▼ | |
| approval_decide RPC ───────┐ | |
| │ publishes | |
| ▼ | |
| ┌──────────────────────────────────┐ | |
| │ DomainEvent::ApprovalDecided │ | |
| │ (extended w/ decided_by_actor + │ | |
| │ decided_by_surface) │ | |
| └────────────────┬─────────────────┘ | |
| │ | |
| ▼ | |
| TelegramApprovalSubscriber.handle() | |
| for each (chat_id, msg_id) in pending_map: | |
| editMessageText("✓ decided by @who via X") | |
| [remove inline_keyboard] | |
| pending_map.remove(request_id) | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/superpowers/specs/2026-05-23-telegram-inline-approvals-design.md` around
lines 25 - 77, The fenced architecture diagram block (the block showing
ApprovalGate, DomainEvent::ApprovalRequested, TelegramApprovalSubscriber,
channel_recv.rs callback_query handler, etc.) is missing a language tag which
triggers MD040; update the opening fence from ``` to ```text so the diagram is
fenced as a text block and no other changes are needed. Ensure the closing fence
remains ``` and leave the inner diagram (ApprovalGate, Telegram Bot API,
TelegramApprovalSubscriber.handle, pending_map.remove) unchanged.
| | Existing desktop frontend calls `approval_decide` without the new params | Server defaults `surface = Some("desktop")` when both are absent. Frontend update is additive. | | ||
| | `Arc<dyn Channel>` → `Arc<TelegramChannel>` downcast may need a small trait change | Plan covers a `try_downcast` helper option first; falls back to `as_any` only if needed. | | ||
| | Real Telegram users tap stale buttons after core restart | Acceptable — "already decided or expired" toast + best-effort edit covers it. | | ||
| | `callback_data` length cap (64 bytes) is exceeded by long request_ids | Request ids are uuids (36 bytes); `appr:<o|a|d>:` adds 7. Total 43. Comfortable headroom. Validate at construction time in `approval_keyboard` to fail loud if this ever changes. | |
There was a problem hiding this comment.
Escape pipe characters inside the table cell to preserve column structure.
Line 478 includes unescaped | inside a table row (appr:<o|a|d>:), which breaks the 2-column table parse (MD056).
Proposed fix
-| `callback_data` length cap (64 bytes) is exceeded by long request_ids | Request ids are uuids (36 bytes); `appr:<o|a|d>:` adds 7. Total 43. Comfortable headroom. Validate at construction time in `approval_keyboard` to fail loud if this ever changes. |
+| `callback_data` length cap (64 bytes) is exceeded by long request_ids | Request ids are uuids (36 bytes); `appr:<o\|a\|d>:` adds 7. Total 43. Comfortable headroom. Validate at construction time in `approval_keyboard` to fail loud if this ever changes. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `callback_data` length cap (64 bytes) is exceeded by long request_ids | Request ids are uuids (36 bytes); `appr:<o|a|d>:` adds 7. Total 43. Comfortable headroom. Validate at construction time in `approval_keyboard` to fail loud if this ever changes. | | |
| | `callback_data` length cap (64 bytes) is exceeded by long request_ids | Request ids are uuids (36 bytes); `appr:<o\|a\|d>:` adds 7. Total 43. Comfortable headroom. Validate at construction time in `approval_keyboard` to fail loud if this ever changes. | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 478-478: Table column count
Expected: 2; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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 `@docs/superpowers/specs/2026-05-23-telegram-inline-approvals-design.md` at
line 478, The table row contains unescaped pipe characters in the snippet
`appr:<o|a|d>:` which breaks the Markdown table parse; update the table cell to
escape the pipes (e.g. `appr:<o\|a\|d>:`) or wrap the entire fragment in a code
span that preserves pipes, and ensure the same escaped form or explanatory note
is used where `approval_keyboard` is described so the table stays valid and the
construction-time validation remark still matches the token format.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Solid spec. The architecture is well-reasoned — event-bus subscriber pattern keeps Telegram concerns out of the approval domain, the pre-insert race mitigation is the right move, security section is thorough, and the test matrix covers the meaningful edge cases. A few things worth flagging before the implementation PR picks this up:
| **Issue**: [#1805](https://github.com/tinyhumansai/openhuman/issues/1805) (phase 2 of N) | ||
| **Date**: 2026-05-23 | ||
| **Status**: Approved for implementation | ||
| **Phase 1**: [PR #2249](https://github.com/tinyhumansai/openhuman/pull/2249) (merged) — `/status`, `/sessions`, `/new`, `/help` |
There was a problem hiding this comment.
[minor] Status: Approved for implementation is set before this PR has merged. It reads as accurate post-merge, but during review it's confusing — implementation PR authors who read this in-flight might take it as pre-approved direction when it's still pending. Consider Pending approval here and a follow-up commit to flip it once this lands, or note in the PR description that the status reflects intended final state.
|
|
||
| --- | ||
|
|
||
| ## Files |
There was a problem hiding this comment.
[minor] The OnceLock<Arc<TelegramApprovalSubscriber>> singleton pattern works fine in production, but the test matrix doesn't address how the integration test (telegram_integration.rs) handles multiple tests attempting set_global. OnceLock can only be initialized once per process — a second call will silently return the first value (or panic depending on the wrapper). If the integration suite runs multiple approval scenarios in the same process, only the first set_global takes effect. The implementation PR should either pass the subscriber instance directly into handle_callback_query (eliminating the global lookup in tests), or expose a reset_global_for_test helper behind #[cfg(test)].
|
|
||
| | Concern | Mitigation | | ||
| |---|---| | ||
| | Non-allowlisted user taps a button | Re-check `from.username` + `from.id` against the allowlist on every callback. Non-allowlisted → toast "Not authorized" + log. Identical to the message-path check. | |
There was a problem hiding this comment.
[minor] The pre-insert mitigation handles the case where ApprovalDecided fires before any sends — good. But there's a second case: ApprovalDecided fires while the broadcast loop is mid-flight. The remove() in the Decided handler drains the map. Subsequent successful sends in the Requested handler do get_mut(...).map(|v| v.push(...)) — that's a no-op because the entry was just removed. Those chats end up with live buttons that never get the "decided" edit. They self-correct on tap ("already decided or expired"), which is the degraded-but-acceptable path, but it's different from what the spec implies when it says "edit every recorded posted message."
The spec should explicitly call this out as an accepted tradeoff rather than leaving it implicit. If you want tighter behavior, the Requested handler could re-insert a tombstone entry after the remove, or the Decided handler could post the final edit directly to any in-flight sends it learns about. Up to the implementer, but the spec should commit to one.
3 participants
Summary
ApprovalGateprompts to all bound Telegram chats with inline approve / approve-always / deny buttons./pendingslash command for on-demand recovery when the auto-broadcast was missed (e.g. across a core restart).ApprovalDecidedevent +approval_decideRPC +approval_auditrow with optionaldecided_by_actor/decided_by_surface.Problem
Phase 1 (#2249) shipped
/status,/sessions,/new,/help— useful for visibility, but operators on Telegram still can't act on permission requests. Today theApprovalGateparks tool calls and surfaces them only to the desktop UI. The fully-stated goal of #1805 covers many more slices (live ticks,/abort,/task,/files,/models,/worktree); each is its own spec/plan/PR cycle. This spec narrowly addresses the inline approvals slice — the most-requested operator-visible gap.Solution
TelegramApprovalSubscriberlistens on theapprovaldomain of the event bus:ApprovalRequested→ broadcasts a prompt + 3-button inline keyboard to every chat that has a session binding; records(chat_id, message_id)in an in-memorypending_map.ApprovalDecided(from any surface) → edits every recorded message to a final "✓ decided by @who via X" state and removes the keyboard.A new
callback_querybranch inchannel_recv.rsre-checks the allowlist on every tap, callsapproval_decidewithactor=@username, surface=\"telegram\", and toasts the result viaanswerCallbackQuery. Stale taps (already-decided / expired) self-correct to a toast + best-effort message edit.Key design choices (full rationale in the spec):
pub(crate)methods onTelegramChannel(send_with_inline_keyboard,edit_message_text,answer_callback_query); genericChanneltrait untouched.ApprovalDecidedevent + RPC + audit row extended with optionaldecided_by_actor/decided_by_surface. Server defaultssurface=\"desktop\"when absent for back-compat with stale frontend builds./pendingis chat-local (not a broadcast) and dedupes per chat againstpending_map.Explicit non-goals for this slice: agent Q&A elicitation, active expiry sweeping, real Telegram E2E automation, and every other #1805 slice. Each gets its own spec.
Submission Checklist
## Related— N/A: no code surfaces touched yet.#NNNin the## Relatedsection (this PR documents phase 2 of Bring OpenHuman’s Telegram channel to OpenCode-level remote-control parity #1805; the implementation PR will close it).Impact
surface=\"desktop\"). Existing desktop frontend continues to work unchanged until the wrapper is updated alongside the implementation PR.callback_query, reuses existingapproval/redact.rsscrubbing, never logs message bodies / args / bot token.[telegram-approval]log prefix throughout.approval_audit— additive, no data loss.Related
/status,/sessions,/new,/help)/abort,/task,/files,/models,/worktree) tracked separately as future phase specs.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/1805-telegram-inline-approvals-spec4d18cddd70a09da09a33d41306edb0fac2abef7bValidation Run
pnpm --filter openhuman-app format:check— N/A (no app code changed)pnpm typecheck— N/A (no TS changed)Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
approval_decidecallers).Duplicate / Superseded PR Handling
Summary by CodeRabbit