Skip to content

Commit e80874d

Browse files
Robert E. Leeruvnet
andcommitted
fix: Address critical/high findings from QE code review
- C1: Replace unwrap() with if-let for thread_id Option (bridge.rs) - C2: Remove unwrap() on DB prepare/query_map, use and_then (session.rs) - C3: Guard against negative duration with .max(0) (bridge.rs) - H1: Fix potential deadlock by resolving tmux target before injector lock (bridge.rs) - H3: Fix dead code in format_topic_name logic (bridge.rs) - H4/H5: UTF-8 safe truncation and chunk splitting (formatting.rs) - M5: Keep i32 offset for teloxide compat (bridge.rs) - M6: Fail with clear error instead of /tmp fallback (config.rs, main.rs) - M10: Specific error message for invalid chat_id parse (config.rs) Co-Authored-By: claude-flow <ruv@ruv.net>
1 parent d9cfb81 commit e80874d

File tree

5 files changed

+75
-48
lines changed

5 files changed

+75
-48
lines changed

src/bridge.rs

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -272,11 +272,11 @@ impl BridgeShared {
272272
sessions.get_session_thread(&session_id)
273273
};
274274

275-
if thread_id.is_some() {
275+
if let Some(tid) = thread_id {
276276
self.session_threads
277277
.write()
278278
.await
279-
.insert(session_id.clone(), thread_id.unwrap() as i32);
279+
.insert(session_id.clone(), tid as i32);
280280
} else if self.config.use_threads {
281281
let topic_name = format_topic_name(
282282
&session_id,
@@ -326,7 +326,7 @@ impl BridgeShared {
326326
};
327327

328328
if let Some(session) = session {
329-
let duration = (chrono::Utc::now() - session.started_at).num_milliseconds() as u64;
329+
let duration = (chrono::Utc::now() - session.started_at).num_milliseconds().max(0) as u64;
330330
let thread_id = self.get_session_thread_id(&msg.session_id).await;
331331

332332
let _ = self
@@ -702,24 +702,29 @@ impl BridgeShared {
702702
None => return, // Unknown topic, ignore (multi-bot)
703703
};
704704

705-
// Restore tmux target
706-
let mut inj = self.injector.lock().await;
705+
// Resolve tmux target BEFORE acquiring injector lock (prevent deadlock)
707706
let tmux_target = self.session_tmux_targets.read().await.get(&session.id).cloned();
708-
if let Some(target) = &tmux_target {
709-
inj.set_target(target, session.tmux_socket.as_deref());
707+
let (resolved_target, resolved_socket) = if let Some(target) = tmux_target {
708+
(Some(target), session.tmux_socket.clone())
710709
} else {
711710
// Restore from database
712711
let (db_target, db_socket) = {
713712
let sessions = self.sessions.lock().await;
714713
sessions.get_tmux_info(&session.id)
715714
};
716715
if let Some(target) = &db_target {
717-
inj.set_target(target, db_socket.as_deref());
718716
self.session_tmux_targets
719717
.write()
720718
.await
721719
.insert(session.id.clone(), target.clone());
722720
}
721+
(db_target, db_socket)
722+
};
723+
724+
// Now acquire injector lock with no other locks held
725+
let mut inj = self.injector.lock().await;
726+
if let Some(target) = &resolved_target {
727+
inj.set_target(target, resolved_socket.as_deref());
723728
}
724729

725730
// Check for cc command prefix
@@ -1064,11 +1069,11 @@ fn format_topic_name(session_id: &str, hostname: Option<&str>, project_dir: Opti
10641069
.chars()
10651070
.take(8)
10661071
.collect::<String>();
1067-
parts.push(short_id.clone());
10681072

10691073
if parts.is_empty() {
10701074
format!("Session {}", short_id)
10711075
} else {
1076+
parts.push(short_id);
10721077
parts.join(" \u{2022} ")
10731078
}
10741079
}

src/config.rs

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,10 @@ struct ConfigFile {
5050
topic_delete_delay_minutes: Option<u64>,
5151
}
5252

53-
fn config_dir() -> PathBuf {
54-
dirs::home_dir()
55-
.unwrap_or_else(|| PathBuf::from("/tmp"))
56-
.join(".config")
57-
.join("claude-telegram-mirror")
53+
fn config_dir() -> Result<PathBuf> {
54+
let home = dirs::home_dir()
55+
.ok_or_else(|| AppError::Config("Cannot determine home directory".to_string()))?;
56+
Ok(home.join(".config").join("claude-telegram-mirror"))
5857
}
5958

6059
/// Ensure config directory exists with secure permissions (0o700)
@@ -128,7 +127,7 @@ fn env_usize(key: &str, default: usize) -> usize {
128127

129128
/// Load configuration: env vars > config file > defaults
130129
pub fn load_config(require_auth: bool) -> Result<Config> {
131-
let dir = config_dir();
130+
let dir = config_dir()?;
132131
ensure_config_dir(&dir)?;
133132

134133
let file = load_config_file(&dir);
@@ -144,7 +143,17 @@ pub fn load_config(require_auth: bool) -> Result<Config> {
144143
.or_else(|| file.chat_id.map(|id| id.to_string()))
145144
.unwrap_or_default();
146145

147-
let chat_id: i64 = chat_id_str.parse().unwrap_or(0);
146+
let chat_id: i64 = if chat_id_str.is_empty() {
147+
0
148+
} else {
149+
chat_id_str.parse().map_err(|_| {
150+
AppError::Config(format!(
151+
"TELEGRAM_CHAT_ID '{}' is not a valid integer.\n\
152+
Chat IDs for supergroups start with -100.",
153+
chat_id_str
154+
))
155+
})?
156+
};
148157

149158
if require_auth {
150159
if bot_token.is_empty() {

src/formatting.rs

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -125,12 +125,17 @@ pub fn format_help() -> String {
125125
.to_string()
126126
}
127127

128-
/// Truncate text with ellipsis
128+
/// Truncate text with ellipsis (UTF-8 safe)
129129
fn truncate(text: &str, max_len: usize) -> String {
130-
if text.len() <= max_len {
130+
if text.chars().count() <= max_len {
131131
text.to_string()
132132
} else {
133-
format!("{}...", &text[..max_len.saturating_sub(3)])
133+
let boundary = text
134+
.char_indices()
135+
.nth(max_len.saturating_sub(3))
136+
.map(|(i, _)| i)
137+
.unwrap_or(text.len());
138+
format!("{}...", &text[..boundary])
134139
}
135140
}
136141

@@ -288,11 +293,18 @@ pub fn chunk_message(text: &str, max_length: usize) -> Vec<String> {
288293
chunks.push(current);
289294
current = String::new();
290295
}
291-
// If single line exceeds max, split it
296+
// If single line exceeds max, split it (UTF-8 safe)
292297
if line.len() > max_length {
293298
let mut remaining = line;
294299
while remaining.len() > max_length {
295-
let (chunk, rest) = remaining.split_at(max_length);
300+
// Find a char boundary at or before max_length
301+
let boundary = remaining
302+
.char_indices()
303+
.take_while(|(i, _)| *i <= max_length)
304+
.last()
305+
.map(|(i, _)| i)
306+
.unwrap_or(remaining.len());
307+
let (chunk, rest) = remaining.split_at(boundary);
296308
chunks.push(chunk.to_string());
297309
remaining = rest;
298310
}

src/main.rs

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -149,10 +149,14 @@ async fn cmd_doctor(fix: bool) -> anyhow::Result<()> {
149149

150150
// Check config directory
151151
println!("\n[2/6] Config directory...");
152-
let config_dir = dirs::home_dir()
153-
.unwrap_or_else(|| PathBuf::from("/tmp"))
154-
.join(".config")
155-
.join("claude-telegram-mirror");
152+
let config_dir = match dirs::home_dir() {
153+
Some(home) => home.join(".config").join("claude-telegram-mirror"),
154+
None => {
155+
println!(" ERROR: Cannot determine home directory");
156+
issues += 1;
157+
return Ok(());
158+
}
159+
};
156160

157161
if config_dir.exists() {
158162
// Check permissions

src/session.rs

Lines changed: 20 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -149,14 +149,13 @@ impl SessionManager {
149149
}
150150

151151
pub fn get_active_sessions(&self) -> Vec<Session> {
152-
let mut stmt = self
153-
.db
152+
self.db
154153
.prepare("SELECT * FROM sessions WHERE status = 'active' ORDER BY last_activity DESC")
155-
.unwrap();
156-
stmt.query_map([], |row| Self::row_to_session(row))
157-
.unwrap()
158-
.filter_map(|r| r.ok())
159-
.collect()
154+
.and_then(|mut stmt| {
155+
let rows = stmt.query_map([], |row| Self::row_to_session(row))?;
156+
Ok(rows.filter_map(|r| r.ok()).collect())
157+
})
158+
.unwrap_or_default()
160159
}
161160

162161
pub fn set_session_thread(&self, session_id: &str, thread_id: i64) {
@@ -323,31 +322,29 @@ impl SessionManager {
323322

324323
pub fn get_stale_session_candidates(&self, timeout_hours: u64) -> Vec<Session> {
325324
let cutoff = Utc::now() - chrono::Duration::hours(timeout_hours as i64);
326-
let mut stmt = self
327-
.db
325+
self.db
328326
.prepare(
329327
"SELECT * FROM sessions WHERE status = 'active' AND last_activity < ?1 ORDER BY last_activity ASC",
330328
)
331-
.unwrap();
332-
stmt.query_map(params![cutoff.to_rfc3339()], |row| {
333-
Self::row_to_session(row)
334-
})
335-
.unwrap()
336-
.filter_map(|r| r.ok())
337-
.collect()
329+
.and_then(|mut stmt| {
330+
let rows = stmt.query_map(params![cutoff.to_rfc3339()], |row| {
331+
Self::row_to_session(row)
332+
})?;
333+
Ok(rows.filter_map(|r| r.ok()).collect())
334+
})
335+
.unwrap_or_default()
338336
}
339337

340338
pub fn get_orphaned_thread_sessions(&self) -> Vec<Session> {
341-
let mut stmt = self
342-
.db
339+
self.db
343340
.prepare(
344341
"SELECT * FROM sessions WHERE status = 'ended' AND thread_id IS NOT NULL ORDER BY last_activity ASC",
345342
)
346-
.unwrap();
347-
stmt.query_map([], |row| Self::row_to_session(row))
348-
.unwrap()
349-
.filter_map(|r| r.ok())
350-
.collect()
343+
.and_then(|mut stmt| {
344+
let rows = stmt.query_map([], |row| Self::row_to_session(row))?;
345+
Ok(rows.filter_map(|r| r.ok()).collect())
346+
})
347+
.unwrap_or_default()
351348
}
352349

353350
pub fn is_tmux_target_owned_by_other(&self, tmux_target: &str, exclude_session_id: &str) -> bool {

0 commit comments

Comments
 (0)