-
Notifications
You must be signed in to change notification settings - Fork 9
refactor(telegram): extract remote-control helpers #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,9 +118,50 @@ pub fn conversation_history_key_candidates(msg: &ChannelMessage) -> LegacySessio | |
| } | ||
| } | ||
|
|
||
| /// Derive a stable host-local thread key from inbound channel facts. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add tests for new public derive_inbound_thread_id function
[RULE] missing-test-coverage · |
||
| pub fn derive_inbound_thread_id( | ||
| channel: &str, | ||
| sender: Option<&str>, | ||
| reply_target: Option<&str>, | ||
| thread_ts: Option<&str>, | ||
| ) -> String { | ||
| let mut key = format!("channel:{channel}"); | ||
| if let Some(sender) = sender.and_then(nonempty) { | ||
| key.push('/'); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Encode sender and reply components unambiguously These components are concatenated with Additional
|
||
| key.push_str(sender); | ||
| } | ||
| if let Some(reply_target) = reply_target.and_then(nonempty) { | ||
| key.push('/'); | ||
| key.push_str(reply_target); | ||
| } | ||
| let provider = channel.split(':').next().unwrap_or(""); | ||
| if !matches!(provider, "telegram" | "tg") { | ||
| if let Some(thread_ts) = thread_ts.and_then(nonempty) { | ||
| key.push_str("#thread:"); | ||
| key.push_str(thread_ts); | ||
| } | ||
| } | ||
| key | ||
| } | ||
|
|
||
| /// Derive a stable client identifier for an inbound channel sender. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| pub fn derive_inbound_client_id(channel: &str, sender: Option<&str>) -> String { | ||
| let channel = channel.trim(); | ||
| match sender.map(str::trim).filter(|sender| !sender.is_empty()) { | ||
| Some(sender) if !channel.is_empty() => format!("inbound:{channel}:{sender}"), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prevent client identifier collisions The channel and sender are interpolated into a colon-delimited identifier without escaping or validation. For example, [RULE] ambiguous-identifier-encoding · |
||
| Some(sender) => format!("inbound:{sender}"), | ||
| None => "inbound".to_string(), | ||
| } | ||
| } | ||
|
Comment on lines
+122
to
+155
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: rg -n 'derive_inbound_(thread|client)_id' crates/tinychannels-bus --glob '*.rs'
sed -n '110,185p' crates/tinychannels-bus/src/channel/session.rs
find crates/tinychannels-bus -name 'AGENTS.md' -o -name 'TESTING.md'Repository: tinyhumansai/tinychannels Length of output: 2385 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- exact helper references ---'
rg -n -F 'derive_inbound_thread_id' . --glob '*.rs'
rg -n -F 'derive_inbound_client_id' . --glob '*.rs'
printf '%s\n' '--- session implementation and nearby tests ---'
sed -n '1,230p' crates/tinychannels-bus/src/channel/session.rs
printf '%s\n' '--- test files in bus crate ---'
find crates/tinychannels-bus -type f -name '*.rs' -print | sortRepository: tinyhumansai/tinychannels Length of output: 7881 Add focused tests for the public derivation helpers. No test directly exercises The repository guideline requires tests for every behavior change. 🧰 Tools🪛 GitHub Actions: CI / 0_Rust SDK.txt[error] 138-143: Cargo Clippy (--all-targets -- -D warnings) reported clippy::collapsible-if: the nested if statement can be collapsed using && let. This warning is treated as an error, causing compilation to fail. 🪛 GitHub Actions: CI / Rust SDK[error] 138-143: Cargo Clippy ( 🤖 Prompt for AI Agents |
||
|
|
||
| fn normalize_namespace(namespace: &str) -> &str { | ||
| match namespace.trim() { | ||
| "" | "default" => "main", | ||
| value => value, | ||
| } | ||
| } | ||
|
|
||
| fn nonempty(value: &str) -> Option<&str> { | ||
| let value = value.trim(); | ||
| (!value.is_empty()).then_some(value) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| [package] | ||
| name = "tinychannels-runtime" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move task spawning to the root crate This manifest introduces the runtime crate whose implementation contains [RULE] task-spawning-location · |
||
| version.workspace = true | ||
| edition = "2024" | ||
| license = "GPL-3.0-only" | ||
| description = "Reusable listener supervision and runtime helpers for TinyChannels." | ||
| repository = "https://github.com/tinyhumansai/tinychannels" | ||
| publish = false | ||
|
|
||
| [dependencies] | ||
| anyhow = "1" | ||
| rand = "0.10" | ||
| tinychannels-bus = { version = "0.1.2", path = "../tinychannels-bus" } | ||
| tokio = { version = "1", default-features = false, features = ["rt", "sync", "time"] } | ||
| tokio-util = { version = "0.7", default-features = false, features = ["rt"] } | ||
| tracing = "0.1" | ||
|
|
||
| [dev-dependencies] | ||
| async-trait = "0.1" | ||
| tokio = { version = "1", features = ["macros", "rt", "sync", "test-util", "time"] } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,202 @@ | ||||||||||||
| //! Runtime mechanics shared by TinyChannels hosts. | ||||||||||||
| //! | ||||||||||||
| //! This crate deliberately owns no provider, persistence, event bus, or host | ||||||||||||
| //! policy. Hosts observe listener lifecycle through [`ListenerObserver`]. | ||||||||||||
|
|
||||||||||||
| use std::sync::Arc; | ||||||||||||
| use std::time::Duration; | ||||||||||||
|
|
||||||||||||
| use rand::RngExt as _; | ||||||||||||
| use tinychannels_bus::{Channel, ChannelMessage}; | ||||||||||||
| use tokio_util::sync::CancellationToken; | ||||||||||||
|
|
||||||||||||
| /// Maximum reconnect jitter added to a listener retry. | ||||||||||||
| pub const MAX_JITTER_MS: u64 = 1_000; | ||||||||||||
|
|
||||||||||||
| /// Host callback for listener lifecycle facts. | ||||||||||||
| pub trait ListenerObserver: Send + Sync { | ||||||||||||
| /// A listener is about to enter its receive loop. | ||||||||||||
| fn connected(&self, _channel: &str) {} | ||||||||||||
| /// A listener exited and will be retried. | ||||||||||||
| fn disconnected(&self, _channel: &str, _reason: &str, _failed: bool) {} | ||||||||||||
| /// A retry has been scheduled after a listener exit. | ||||||||||||
| fn restarted(&self, _channel: &str) {} | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// A listener observer with no host side effects. | ||||||||||||
| #[derive(Debug, Default)] | ||||||||||||
| pub struct NoopListenerObserver; | ||||||||||||
| impl ListenerObserver for NoopListenerObserver {} | ||||||||||||
|
|
||||||||||||
| /// Compute the bounded listener queue capacity for a provider count. | ||||||||||||
| pub fn compute_max_in_flight_messages(channel_count: usize) -> usize { | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the original in-flight message constants The new helper hardcodes the parallelism, minimum, and maximum values instead of using Additional
|
||||||||||||
| channel_count.saturating_mul(4).clamp(8, 64) | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the shared channel runtime constants The function duplicates the values from [RULE] shared-runtime-constants · |
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// Deterministically choose a broadly-supported acknowledgement reaction. | ||||||||||||
| pub fn select_acknowledgment_reaction(content: &str) -> &'static str { | ||||||||||||
| let lower = content.to_lowercase(); | ||||||||||||
| let variant = content | ||||||||||||
| .len() | ||||||||||||
| .wrapping_add(content.chars().next().map_or(0, |ch| ch as usize)) | ||||||||||||
| & 1; | ||||||||||||
| let contains = |words: &[&str]| words.iter().any(|word| lower.contains(word)); | ||||||||||||
| let starts = |words: &[&str]| words.iter().any(|word| lower.starts_with(word)); | ||||||||||||
| let options: &[&str] = if contains(&["thank", "thx", "appreciate", "grateful", "cheers"]) { | ||||||||||||
| &["❤️", "🙏"] | ||||||||||||
| } else if contains(&[ | ||||||||||||
| "amazing", | ||||||||||||
| "awesome", | ||||||||||||
| "incredible", | ||||||||||||
| "love it", | ||||||||||||
| "congrat", | ||||||||||||
| "!!", | ||||||||||||
| ]) { | ||||||||||||
| &["🔥", "🎉"] | ||||||||||||
| } else if contains(&[ | ||||||||||||
| "price", "btc", "eth", "crypto", "trade", "pump", "dump", "market", "token", "wallet", | ||||||||||||
| "defi", "nft", "sol", "bnb", | ||||||||||||
|
Comment on lines
+57
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
These short strings are matched as arbitrary substrings, so ordinary messages are assigned unrelated reactions before the later question/greeting branches run; for example, “Can we work together?” contains Useful? React with 👍 / 👎. |
||||||||||||
| ]) { | ||||||||||||
| &["💯", "⚡"] | ||||||||||||
| } else if contains(&[ | ||||||||||||
| "code", | ||||||||||||
| "function", | ||||||||||||
| "api", | ||||||||||||
| "deploy", | ||||||||||||
| "build", | ||||||||||||
| "debug", | ||||||||||||
| "script", | ||||||||||||
| "git", | ||||||||||||
| "rust", | ||||||||||||
| "python", | ||||||||||||
| "js", | ||||||||||||
| "typescript", | ||||||||||||
| ]) { | ||||||||||||
| &["👨💻", "🤓"] | ||||||||||||
| } else if starts(&[ | ||||||||||||
| "hi", | ||||||||||||
| "hello", | ||||||||||||
| "hey", | ||||||||||||
| "sup", | ||||||||||||
| "good morning", | ||||||||||||
| "good evening", | ||||||||||||
| "good afternoon", | ||||||||||||
| ]) || lower == "yo" | ||||||||||||
| || lower.starts_with("yo ") | ||||||||||||
| { | ||||||||||||
| &["🤗", "😁"] | ||||||||||||
| } else if lower.contains('?') | ||||||||||||
| || starts(&[ | ||||||||||||
| "how", | ||||||||||||
| "what", | ||||||||||||
| "why", | ||||||||||||
| "when", | ||||||||||||
| "where", | ||||||||||||
| "who", | ||||||||||||
| "can you", | ||||||||||||
| "could you", | ||||||||||||
| "would you", | ||||||||||||
| "is ", | ||||||||||||
| "are ", | ||||||||||||
| "do you", | ||||||||||||
| "does", | ||||||||||||
| ]) | ||||||||||||
| { | ||||||||||||
| &["🤔", "✍️"] | ||||||||||||
| } else { | ||||||||||||
| &["👀", "✍️"] | ||||||||||||
| }; | ||||||||||||
| options[variant % options.len()] | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// Spawn a reconnecting listener. Host-specific observability is delivered to | ||||||||||||
| /// `observer`; the retry policy remains identical for every host. | ||||||||||||
| pub fn spawn_supervised_listener( | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move task spawning to the root crate This function calls Additional
|
||||||||||||
| channel: Arc<dyn Channel>, | ||||||||||||
| tx: tokio::sync::mpsc::Sender<ChannelMessage>, | ||||||||||||
| initial_backoff_secs: u64, | ||||||||||||
| max_backoff_secs: u64, | ||||||||||||
| observer: Arc<dyn ListenerObserver>, | ||||||||||||
| ) -> tokio::task::JoinHandle<()> { | ||||||||||||
| tokio::spawn(async move { | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move task spawning to the root crate This runtime crate now directly spawns the supervised listener task, contrary to the repository rule that anything spawning a task belongs in the root crate. The same issue also occurs in Additional
|
||||||||||||
| let name = channel.name().to_owned(); | ||||||||||||
| let mut backoff = initial_backoff_secs.max(1); | ||||||||||||
| let max_backoff = max_backoff_secs.max(backoff); | ||||||||||||
| loop { | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '110,155p' crates/tinychannels-runtime/src/lib.rs
sed -n '80,135p' crates/tinychannels-bus/src/traits.rs
rg -n 'async fn listen|fn listen' src crates --glob '*.rs'Repository: tinyhumansai/tinychannels Length of output: 6660 Check receiver closure before each listener attempt. If the receiver closes during the backoff delay, the next loop iteration calls Check Proposed fix loop {
+ if tx.is_closed() {
+ break;
+ }
observer.connected(&name);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| observer.connected(&name); | ||||||||||||
| let result = channel.listen(tx.clone()).await; | ||||||||||||
| if tx.is_closed() { | ||||||||||||
| break; | ||||||||||||
| } | ||||||||||||
| match result { | ||||||||||||
| Ok(()) => observer.disconnected(&name, "exited unexpectedly", false), | ||||||||||||
| Err(error) => observer.disconnected(&name, &error.to_string(), true), | ||||||||||||
| } | ||||||||||||
| observer.restarted(&name); | ||||||||||||
| tokio::time::sleep( | ||||||||||||
| Duration::from_secs(backoff) + Duration::from_millis(jitter_millis(backoff)), | ||||||||||||
| ) | ||||||||||||
| .await; | ||||||||||||
| backoff = backoff.saturating_mul(2).min(max_backoff); | ||||||||||||
| } | ||||||||||||
| }) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// Sample full reconnect jitter, bounded to avoid dwarfing the base retry. | ||||||||||||
| pub fn jitter_millis(backoff_secs: u64) -> u64 { | ||||||||||||
| let window = backoff_secs.saturating_mul(1_000).min(MAX_JITTER_MS); | ||||||||||||
| (window != 0) | ||||||||||||
| .then(|| rand::rng().random_range(0..window)) | ||||||||||||
| .unwrap_or(0) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// Log a failed worker join without imposing host-specific error reporting. | ||||||||||||
| pub fn log_worker_join_result(result: Result<(), tokio::task::JoinError>) { | ||||||||||||
| if let Err(error) = result { | ||||||||||||
| tracing::error!("Channel message worker crashed: {error}"); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// Maintain a typing indicator until `cancellation_token` is cancelled. | ||||||||||||
| pub fn spawn_scoped_typing_task( | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move typing task spawning to the root crate This function also calls [RULE] task-spawning-location · |
||||||||||||
| channel: Arc<dyn Channel>, | ||||||||||||
| recipient: String, | ||||||||||||
| cancellation_token: CancellationToken, | ||||||||||||
| refresh_interval: Duration, | ||||||||||||
| ) -> tokio::task::JoinHandle<()> { | ||||||||||||
| tokio::spawn(async move { | ||||||||||||
| loop { | ||||||||||||
| tokio::select! { | ||||||||||||
| () = cancellation_token.cancelled() => break, | ||||||||||||
| _ = tokio::time::sleep(refresh_interval) => { | ||||||||||||
| if let Err(error) = channel.start_typing(&recipient).await { | ||||||||||||
|
Comment on lines
+171
to
+172
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When callers rely on this helper to manage the indicator, the loop waits for the entire Useful? React with 👍 / 👎. |
||||||||||||
| tracing::debug!(channel = channel.name(), "typing start failed: {error}"); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| if let Err(error) = channel.stop_typing(&recipient).await { | ||||||||||||
| tracing::debug!(channel = channel.name(), "typing stop failed: {error}"); | ||||||||||||
| } | ||||||||||||
| }) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| #[cfg(test)] | ||||||||||||
| mod tests { | ||||||||||||
| use super::*; | ||||||||||||
|
|
||||||||||||
| #[test] | ||||||||||||
| fn acknowledgement_selection_is_stable_and_contextual() { | ||||||||||||
| assert!(matches!( | ||||||||||||
| select_acknowledgment_reaction("thanks"), | ||||||||||||
| "❤️" | "🙏" | ||||||||||||
| )); | ||||||||||||
| assert_eq!( | ||||||||||||
| select_acknowledgment_reaction("thanks"), | ||||||||||||
| select_acknowledgment_reaction("thanks") | ||||||||||||
| ); | ||||||||||||
| assert!(jitter_millis(1) < MAX_JITTER_MS); | ||||||||||||
| assert_eq!(jitter_millis(0), 0); | ||||||||||||
| assert_eq!(compute_max_in_flight_messages(100), 64); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| //! Telegram approval-prompt vocabulary shared by hosts. | ||
|
|
||
| /// Identifier used for Telegram-originated approval contexts. | ||
| pub const TELEGRAM_APPROVAL_CLIENT_ID: &str = "telegram"; | ||
|
|
||
| /// Render an approval request as a Telegram message body. | ||
| pub fn format_approval_prompt(tool_name: &str, action_summary: &str) -> String { | ||
| format!( | ||
| "🔐 Approval needed\nTool: `{tool_name}`\nAction: {action_summary}\n\nReply `yes` to approve or `no` to deny." | ||
| ) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn approval_prompt_includes_action_and_reply_instructions() { | ||
| let body = format_approval_prompt("git_operations", "git commit -m fix"); | ||
| assert!(body.contains("git_operations")); | ||
| assert!(body.contains("git commit")); | ||
| assert!(body.contains("yes") && body.contains("no")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding
tinychannels-runtimeas a fourth workspace layer putsspawn_supervised_listener—which directly callstokio::spawn—outside the root implementation crate, while also placing the host-facingListenerObserverboundary outside the contract crate. Keep the supervisor intinychannelsand move any genuinely cross-boundary vocabulary totinychannels-busso the prescribed dependency split remains enforceable.AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.