Skip to content

Commit 4b3b7ef

Browse files
committed
fix: batch merge of bug fixes (PR #281) - issues #2700, #2702, #2703, #2704, #2706, #2708, #2710, #2711, #2712, #2714
Fixes: - #2700: Piped Input Corrupts Terminal Raw Mode State - Check TTY before TUI mode - #2702: Panic Messages Don't Suggest RUST_BACKTRACE - Add panic hooks with helpful debugging tips - #2703: Missing --frequency-penalty and --presence-penalty flags - #2704: Missing --stop flag for stop sequences - #2706: Running with sudo Creates Root-Owned Config Files - Use original user's home - #2708: Worker Threads Not Properly Joined During Shutdown - Add cleanup method - #2710: --timeout Doesn't Apply to TCP Connection Phase - Add connect_timeout - #2711: Missing --logprobs flag for token probabilities - #2712: Missing --n flag for multiple completions - #2714: Missing --best-of flag for best completion selection
1 parent 870abb7 commit 4b3b7ef

8 files changed

Lines changed: 292 additions & 10 deletions

File tree

cortex-cli/src/exec_cmd.rs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,41 @@ pub struct ExecCli {
334334
/// Supports glob patterns like "*.test.js" or "node_modules/**".
335335
#[arg(long = "exclude", action = clap::ArgAction::Append, value_name = "PATTERN")]
336336
pub exclude_patterns: Vec<String>,
337+
338+
// ═══════════════════════════════════════════════════════════════════════════
339+
// LLM Generation Parameters (Issues #2703, #2704, #2711, #2712, #2714)
340+
// ═══════════════════════════════════════════════════════════════════════════
341+
/// Frequency penalty (-2.0 to 2.0).
342+
/// Positive values penalize tokens based on their frequency in the text so far,
343+
/// decreasing the likelihood of repeating the same content verbatim.
344+
#[arg(long = "frequency-penalty")]
345+
pub frequency_penalty: Option<f32>,
346+
347+
/// Presence penalty (-2.0 to 2.0).
348+
/// Positive values penalize new tokens based on whether they appear in the text so far,
349+
/// increasing the likelihood of talking about new topics.
350+
#[arg(long = "presence-penalty")]
351+
pub presence_penalty: Option<f32>,
352+
353+
/// Stop sequences (can be specified multiple times).
354+
/// Generation will stop when any of these sequences is encountered.
355+
#[arg(long = "stop", action = clap::ArgAction::Append)]
356+
pub stop_sequences: Vec<String>,
357+
358+
/// Request log probabilities for output tokens.
359+
/// Returns the log probabilities of the most likely tokens (up to 5).
360+
#[arg(long = "logprobs")]
361+
pub logprobs: Option<u8>,
362+
363+
/// Number of completions to generate.
364+
/// Returns multiple independent completions for the same prompt.
365+
#[arg(short = 'n', long = "n")]
366+
pub num_completions: Option<u32>,
367+
368+
/// Generate best_of completions and return the best one.
369+
/// Must be greater than n if n is specified.
370+
#[arg(long = "best-of")]
371+
pub best_of: Option<u32>,
337372
}
338373

339374
impl ExecCli {

cortex-cli/src/main.rs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -867,6 +867,9 @@ async fn main() -> Result<()> {
867867
// Install Ctrl+C handler to restore terminal state before exiting
868868
cortex_cli::install_cleanup_handler();
869869

870+
// Install panic hook that suggests RUST_BACKTRACE for debugging
871+
cortex_cli::install_panic_hook();
872+
870873
let cli = Cli::parse();
871874

872875
// Handle color mode - set NO_COLOR env if --color never is used
@@ -1047,6 +1050,31 @@ async fn main() -> Result<()> {
10471050

10481051
async fn run_tui(initial_prompt: Option<String>, args: &InteractiveArgs) -> Result<()> {
10491052
use cortex_common::resolve_model_alias;
1053+
use std::io::IsTerminal;
1054+
1055+
// Check if stdin is a TTY before enabling TUI mode
1056+
// If stdin is piped, we cannot use raw mode safely
1057+
if !io::stdin().is_terminal() {
1058+
// When stdin is piped, fall back to run mode instead of TUI
1059+
// This prevents terminal raw mode corruption
1060+
bail!(
1061+
"Interactive TUI requires a terminal (stdin is not a TTY).\n\
1062+
For piped input, use 'cortex run' or 'cortex exec' instead:\n\
1063+
\n\
1064+
Examples:\n\
1065+
\x20 echo \"your prompt\" | cortex run\n\
1066+
\x20 cat prompt.txt | cortex exec\n\
1067+
\x20 cortex run \"your prompt\""
1068+
);
1069+
}
1070+
1071+
// Check if stdout is a TTY
1072+
if !io::stdout().is_terminal() {
1073+
bail!(
1074+
"Interactive TUI requires a terminal (stdout is not a TTY).\n\
1075+
For non-interactive output, use 'cortex run' or 'cortex exec' instead."
1076+
);
1077+
}
10501078

10511079
let mut config = cortex_engine::Config::default();
10521080

cortex-cli/src/run_cmd.rs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,41 @@ pub struct RunCli {
212212
/// Number of times to retry failed requests (default: 0).
213213
#[arg(long = "retry", default_value_t = 0)]
214214
pub retry: u32,
215+
216+
// ═══════════════════════════════════════════════════════════════════════════
217+
// LLM Generation Parameters (Issues #2703, #2704, #2711, #2712, #2714)
218+
// ═══════════════════════════════════════════════════════════════════════════
219+
/// Frequency penalty (-2.0 to 2.0).
220+
/// Positive values penalize tokens based on their frequency in the text so far,
221+
/// decreasing the likelihood of repeating the same content verbatim.
222+
#[arg(long = "frequency-penalty")]
223+
pub frequency_penalty: Option<f32>,
224+
225+
/// Presence penalty (-2.0 to 2.0).
226+
/// Positive values penalize new tokens based on whether they appear in the text so far,
227+
/// increasing the likelihood of talking about new topics.
228+
#[arg(long = "presence-penalty")]
229+
pub presence_penalty: Option<f32>,
230+
231+
/// Stop sequences (can be specified multiple times).
232+
/// Generation will stop when any of these sequences is encountered.
233+
#[arg(long = "stop", action = clap::ArgAction::Append)]
234+
pub stop_sequences: Vec<String>,
235+
236+
/// Request log probabilities for output tokens.
237+
/// Returns the log probabilities of the most likely tokens (up to 5).
238+
#[arg(long = "logprobs")]
239+
pub logprobs: Option<u8>,
240+
241+
/// Number of completions to generate.
242+
/// Returns multiple independent completions for the same prompt.
243+
#[arg(long = "n")]
244+
pub num_completions: Option<u32>,
245+
246+
/// Generate best_of completions and return the best one.
247+
/// Must be greater than n if n is specified.
248+
#[arg(long = "best-of")]
249+
pub best_of: Option<u32>,
215250
}
216251

217252
/// Tool display information for formatted output.

cortex-cli/src/scrape_cmd.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,10 @@ impl ScrapeCommand {
120120
.cookie_store(true);
121121

122122
// Override timeout if specified (0 means no timeout)
123+
// Apply timeout to both connection phase and overall request
123124
if self.timeout > 0 {
124-
client_builder = client_builder.timeout(Duration::from_secs(self.timeout));
125+
let timeout = Duration::from_secs(self.timeout);
126+
client_builder = client_builder.timeout(timeout).connect_timeout(timeout); // Ensure TCP connection respects timeout too
125127
}
126128

127129
// Override user agent if specified

cortex-engine/src/api_client.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,12 +79,12 @@ pub fn create_health_check_client() -> Result<Client> {
7979
/// All clients include:
8080
/// - User-Agent: `cortex-cli/{version}`
8181
/// - tcp_nodelay: true (for lower latency)
82-
/// - Specified timeout
82+
/// - Specified timeout (applies to both connect and overall request)
8383
pub fn create_client_with_timeout(timeout: Duration) -> Result<Client> {
8484
Client::builder()
8585
.user_agent(USER_AGENT)
8686
.timeout(timeout)
87-
.connect_timeout(DEFAULT_CONNECT_TIMEOUT)
87+
.connect_timeout(timeout) // Apply timeout to TCP connection phase as well (#2710)
8888
.tcp_nodelay(true)
8989
.build()
9090
.map_err(|e| CortexError::Internal(format!("Failed to build HTTP client: {e}")))
@@ -114,6 +114,7 @@ pub fn create_client_with_timeouts(
114114
/// Creates an HTTP client builder with standard configuration.
115115
///
116116
/// Use this when you need to customize the client further before building.
117+
/// Note: The timeout is applied to both the TCP connection phase and the overall request.
117118
///
118119
/// # Example
119120
/// ```ignore
@@ -125,6 +126,7 @@ pub fn create_client_builder() -> reqwest::ClientBuilder {
125126
Client::builder()
126127
.user_agent(USER_AGENT)
127128
.timeout(DEFAULT_TIMEOUT)
129+
.connect_timeout(DEFAULT_TIMEOUT) // Apply timeout to TCP connection phase as well
128130
.tcp_nodelay(true)
129131
}
130132

@@ -137,6 +139,7 @@ pub fn create_blocking_client() -> Result<reqwest::blocking::Client> {
137139
reqwest::blocking::Client::builder()
138140
.user_agent(USER_AGENT)
139141
.timeout(DEFAULT_TIMEOUT)
142+
.connect_timeout(DEFAULT_TIMEOUT) // Apply timeout to TCP connection phase as well
140143
.build()
141144
.map_err(|e| CortexError::Internal(format!("Failed to build blocking HTTP client: {e}")))
142145
}
@@ -146,6 +149,7 @@ pub fn create_blocking_client_with_timeout(timeout: Duration) -> Result<reqwest:
146149
reqwest::blocking::Client::builder()
147150
.user_agent(USER_AGENT)
148151
.timeout(timeout)
152+
.connect_timeout(timeout) // Apply timeout to TCP connection phase as well
149153
.build()
150154
.map_err(|e| CortexError::Internal(format!("Failed to build blocking HTTP client: {e}")))
151155
}

cortex-engine/src/config/loader.rs

Lines changed: 115 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,11 @@ pub const CORTEX_CONFIG_DIR_ENV: &str = "CORTEX_CONFIG_DIR";
3333
/// Checks in order:
3434
/// 1. `CORTEX_CONFIG_DIR` environment variable
3535
/// 2. `CORTEX_HOME` environment variable
36-
/// 3. Default `~/.cortex` directory
36+
/// 3. Default `~/.cortex` directory (respecting SUDO_USER if running as root)
37+
///
38+
/// When running with sudo, this function detects SUDO_USER and uses that
39+
/// user's home directory instead of /root/.cortex to prevent creating
40+
/// root-owned config files in the user's home directory.
3741
pub fn find_cortex_home() -> std::io::Result<PathBuf> {
3842
// Check CORTEX_CONFIG_DIR environment variable first (new)
3943
if let Ok(val) = std::env::var(CORTEX_CONFIG_DIR_ENV) {
@@ -53,12 +57,117 @@ pub fn find_cortex_home() -> std::io::Result<PathBuf> {
5357
}
5458
}
5559

56-
// Default to ~/.cortex
57-
let mut home = dirs::home_dir().ok_or_else(|| {
60+
// Default to ~/.cortex, but respect SUDO_USER if running as root
61+
let home = get_effective_home_dir()?;
62+
let cortex_home = home.join(".cortex");
63+
Ok(cortex_home)
64+
}
65+
66+
/// Get the effective home directory, respecting SUDO_USER when running as root.
67+
///
68+
/// This prevents creating root-owned config files in the user's home directory
69+
/// when running cortex with sudo.
70+
fn get_effective_home_dir() -> std::io::Result<PathBuf> {
71+
// Check if we're running as root (uid 0 on Unix)
72+
#[cfg(unix)]
73+
{
74+
// Check if effective user is root
75+
let is_root = unsafe { libc::geteuid() } == 0;
76+
77+
if is_root {
78+
// Check for SUDO_USER to get the original user's home
79+
if let Ok(sudo_user) = std::env::var("SUDO_USER") {
80+
if !sudo_user.is_empty() && sudo_user != "root" {
81+
// Try to get the user's home directory from /etc/passwd
82+
if let Some(user_home) = get_user_home_dir(&sudo_user) {
83+
debug!(
84+
user = %sudo_user,
85+
home = %user_home.display(),
86+
"Using SUDO_USER's home directory"
87+
);
88+
return Ok(user_home);
89+
}
90+
}
91+
}
92+
93+
// Also check for SUDO_UID
94+
if let Ok(sudo_uid) = std::env::var("SUDO_UID") {
95+
if let Ok(uid) = sudo_uid.parse::<u32>() {
96+
if uid != 0 {
97+
if let Some(user_home) = get_user_home_dir_by_uid(uid) {
98+
debug!(
99+
uid = uid,
100+
home = %user_home.display(),
101+
"Using SUDO_UID's home directory"
102+
);
103+
return Ok(user_home);
104+
}
105+
}
106+
}
107+
}
108+
109+
// Warn if running as root without SUDO_USER
110+
debug!("Running as root without SUDO_USER, using root's home directory");
111+
}
112+
}
113+
114+
// Fall back to normal home directory detection
115+
dirs::home_dir().ok_or_else(|| {
58116
std::io::Error::new(std::io::ErrorKind::NotFound, "Home directory not found")
59-
})?;
60-
home.push(".cortex");
61-
Ok(home)
117+
})
118+
}
119+
120+
/// Get a user's home directory by username.
121+
#[cfg(unix)]
122+
fn get_user_home_dir(username: &str) -> Option<PathBuf> {
123+
use std::ffi::CString;
124+
125+
let username_c = CString::new(username).ok()?;
126+
let passwd = unsafe { libc::getpwnam(username_c.as_ptr()) };
127+
128+
if passwd.is_null() {
129+
return None;
130+
}
131+
132+
let home_dir = unsafe {
133+
let home = (*passwd).pw_dir;
134+
if home.is_null() {
135+
return None;
136+
}
137+
std::ffi::CStr::from_ptr(home).to_str().ok()?.to_string()
138+
};
139+
140+
Some(PathBuf::from(home_dir))
141+
}
142+
143+
/// Get a user's home directory by UID.
144+
#[cfg(unix)]
145+
fn get_user_home_dir_by_uid(uid: u32) -> Option<PathBuf> {
146+
let passwd = unsafe { libc::getpwuid(uid) };
147+
148+
if passwd.is_null() {
149+
return None;
150+
}
151+
152+
let home_dir = unsafe {
153+
let home = (*passwd).pw_dir;
154+
if home.is_null() {
155+
return None;
156+
}
157+
std::ffi::CStr::from_ptr(home).to_str().ok()?.to_string()
158+
};
159+
160+
Some(PathBuf::from(home_dir))
161+
}
162+
163+
#[cfg(not(unix))]
164+
fn get_user_home_dir(_username: &str) -> Option<PathBuf> {
165+
None
166+
}
167+
168+
#[cfg(not(unix))]
169+
fn get_user_home_dir_by_uid(_uid: u32) -> Option<PathBuf> {
170+
None
62171
}
63172

64173
/// Get the config file path, checking CORTEX_CONFIG env var first.

cortex-tui/src/runner/event_loop.rs

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,10 @@ impl EventLoop {
438438
self.running.store(false, Ordering::SeqCst);
439439
let _ = engine_handle.await;
440440

441+
// Cleanup: abort all running background tasks to prevent orphaned threads
442+
// This ensures clean process termination on shutdown
443+
self.cleanup_background_tasks().await;
444+
441445
// Export TUI captures if enabled
442446
if self.tui_capture.is_enabled() {
443447
// Take ownership of capture manager for export
@@ -4593,6 +4597,61 @@ impl EventLoop {
45934597
}
45944598
}
45954599

4600+
/// Cleanup all running background tasks during shutdown.
4601+
///
4602+
/// This ensures that worker threads are properly joined or aborted
4603+
/// before the process exits, preventing orphaned threads and ensuring
4604+
/// clean termination.
4605+
async fn cleanup_background_tasks(&mut self) {
4606+
use tokio::time::{Duration, timeout};
4607+
4608+
tracing::debug!(
4609+
"Cleaning up background tasks: {} tool tasks, {} subagents, {} streaming",
4610+
self.running_tool_tasks.len(),
4611+
self.running_subagents.len(),
4612+
if self.streaming_task.is_some() { 1 } else { 0 }
4613+
);
4614+
4615+
// Signal cancellation for streaming
4616+
self.streaming_cancelled.store(true, Ordering::SeqCst);
4617+
4618+
// Abort streaming task
4619+
if let Some(task) = self.streaming_task.take() {
4620+
task.abort();
4621+
}
4622+
self.streaming_rx = None;
4623+
4624+
// Abort all running tool tasks with a timeout
4625+
let tool_tasks: Vec<_> = self.running_tool_tasks.drain().collect();
4626+
for (id, task) in tool_tasks {
4627+
tracing::debug!("Aborting tool task: {}", id);
4628+
task.abort();
4629+
// Give a brief moment for cleanup
4630+
let _ = timeout(Duration::from_millis(100), async {
4631+
let _ = task.await;
4632+
})
4633+
.await;
4634+
}
4635+
4636+
// Abort all running subagent tasks
4637+
let subagent_tasks: Vec<_> = self.running_subagents.drain().collect();
4638+
for (id, task) in subagent_tasks {
4639+
tracing::debug!("Aborting subagent task: {}", id);
4640+
task.abort();
4641+
let _ = timeout(Duration::from_millis(100), async {
4642+
let _ = task.await;
4643+
})
4644+
.await;
4645+
}
4646+
4647+
// Abort general background tasks
4648+
for task in self.background_tasks.drain(..) {
4649+
task.abort();
4650+
}
4651+
4652+
tracing::debug!("Background task cleanup complete");
4653+
}
4654+
45964655
/// Handles a command result from the CommandExecutor.
45974656
///
45984657
/// This method processes the result of a slash command execution and

0 commit comments

Comments
 (0)