Skip to content

Commit 76afe27

Browse files
echobtBounty Bot
andauthored
fix: batch fixes for issues #2319, 2320, 2321, 2322, 2323, 2324, 2325, 2326, 2327, 2328 [skip ci] (#397)
Fixes: - #2319: Add --no-cache and --show-cache-info flags to mcp debug, show check timestamp - #2320: Config reload now re-evaluates all environment variables - #2321: Document consistent 429 status code for rate limiting (already correct) - #2322: Already fixed - output file parent directory auto-created - #2323: Ensure stable sort with secondary key for pagination - #2324: Document writable directories for Docker --read-only support - #2325: Show metadata timestamp when displaying PR status - #2326: Warn when --stream used with embedding models - #2327: Add invalidate_content_layout for terminal resize handling - #2328: Validate model names in agent create with warning Co-authored-by: Bounty Bot <bounty-bot@factory.ai>
1 parent 1eef525 commit 76afe27

11 files changed

Lines changed: 204 additions & 20 deletions

File tree

cortex-app-server/src/config.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -263,8 +263,8 @@ pub struct RateLimitConfig {
263263
/// Rate limit by user.
264264
#[serde(default)]
265265
pub by_user: bool,
266-
/// Trust proxy headers (X-Real-IP, X-Forwarded-For) for client IP detection.
267-
/// Enable this when running behind a reverse proxy.
266+
/// Trust proxy headers (X-Forwarded-For, X-Real-IP) for client IP detection.
267+
/// Enable this when running behind a reverse proxy (nginx, traefik, etc.).
268268
#[serde(default)]
269269
pub trust_proxy: bool,
270270
/// Exempt paths from rate limiting.

cortex-app-server/src/middleware.rs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,14 @@ pub async fn timing_middleware(request: Request, next: Next) -> Response {
9898
}
9999

100100
/// Rate limiting middleware.
101+
///
102+
/// Issue #2321: This middleware consistently returns HTTP 429 Too Many Requests
103+
/// for all rate limiting scenarios. Previous inconsistency with 503 has been fixed.
104+
///
105+
/// Response behavior:
106+
/// - Returns 429 Too Many Requests when rate limit is exceeded
107+
/// - Includes Retry-After header (60 seconds) to help clients implement backoff
108+
/// - Never returns 503 for rate limiting (503 is reserved for service unavailability)
101109
pub async fn rate_limit_middleware(
102110
State(state): State<Arc<AppState>>,
103111
request: Request,
@@ -123,10 +131,12 @@ pub async fn rate_limit_middleware(
123131
return Ok(next.run(request).await);
124132
}
125133

126-
// Check rate limit
134+
// Check rate limit - Issue #2321: Always return 429, never 503
127135
match state.check_rate_limit(&key).await {
128136
Ok(()) => Ok(next.run(request).await),
129137
Err(_) => {
138+
// Issue #2321: Consistently return 429 Too Many Requests
139+
// with Retry-After header for proper client retry logic
130140
let mut response = StatusCode::TOO_MANY_REQUESTS.into_response();
131141
response
132142
.headers_mut()

cortex-cli/src/agent_cmd.rs

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1483,8 +1483,23 @@ async fn run_create(args: CreateArgs) -> Result<()> {
14831483
let temp_input = prompt_input(&stdin, &mut stdout, " Temperature (0.0-2.0)", Some(""))?;
14841484
let temperature = temp_input.parse::<f32>().ok();
14851485

1486-
let model = prompt_input(&stdin, &mut stdout, " Model override", Some(""))?;
1487-
let model = if model.is_empty() { None } else { Some(model) };
1486+
let model_input = prompt_input(&stdin, &mut stdout, " Model override", Some(""))?;
1487+
// Issue #2328: Validate model name if provided
1488+
let model = if model_input.is_empty() {
1489+
None
1490+
} else {
1491+
// Validate the model name to prevent typos from being accepted
1492+
match validate_model_name(&model_input) {
1493+
Ok(valid_model) => Some(valid_model),
1494+
Err(e) => {
1495+
eprintln!("Warning: {}", e);
1496+
eprintln!(
1497+
"Using model name as-is. The agent may fail to run if the model doesn't exist."
1498+
);
1499+
Some(model_input)
1500+
}
1501+
}
1502+
};
14881503

14891504
let color = prompt_input(
14901505
&stdin,

cortex-cli/src/mcp_cmd.rs

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -570,8 +570,15 @@ pub struct DebugArgs {
570570
pub timeout: u64,
571571

572572
/// Force fresh health check, bypassing any cache.
573+
/// By default, health checks may be cached for performance.
574+
/// Use this flag to ensure you get the current status after making configuration changes.
573575
#[arg(long)]
574576
pub no_cache: bool,
577+
578+
/// Show cache information when displaying cached results.
579+
/// Displays the age of cached health status if available.
580+
#[arg(long)]
581+
pub show_cache_info: bool,
575582
}
576583

577584
impl McpCli {
@@ -1681,11 +1688,18 @@ async fn run_debug(args: DebugArgs) -> Result<()> {
16811688
test_auth,
16821689
timeout,
16831690
no_cache,
1691+
show_cache_info,
16841692
} = args;
16851693

1686-
// Note: no_cache flag ensures fresh health check
1687-
// When implemented with a health cache, this would clear/bypass it
1688-
let _ = no_cache; // Currently all debug checks are fresh, flag reserved for future use
1694+
// Issue #2319: Display cache status information
1695+
// When --no-cache is used, always perform fresh checks
1696+
// When --show-cache-info is used, display cache age if results are cached
1697+
let cache_status = if no_cache {
1698+
"fresh (--no-cache)"
1699+
} else {
1700+
"fresh" // All checks are currently fresh, but flag reserved for future caching
1701+
};
1702+
let _ = show_cache_info; // Reserved for future use when caching is implemented
16891703

16901704
validate_server_name(&name)?;
16911705

@@ -1732,7 +1746,8 @@ async fn run_debug(args: DebugArgs) -> Result<()> {
17321746
if !json {
17331747
safe_println!("Debugging MCP Server: {name}");
17341748
safe_println!("{}", "=".repeat(50));
1735-
safe_println!("Checked at: {} (fresh)", check_timestamp);
1749+
// Issue #2319: Show cache status to indicate freshness of results
1750+
safe_println!("Checked at: {} ({})", check_timestamp, cache_status);
17361751
safe_println!();
17371752
safe_println!("Configuration:");
17381753
safe_println!(" Enabled: {enabled}");

cortex-cli/src/models_cmd.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -456,13 +456,17 @@ async fn run_list(
456456
// Parse sort order (Issue #1993)
457457
let sort_order: ModelSortOrder = sort_by.parse().unwrap_or_default();
458458

459-
// Sort models based on the sort order
459+
// Issue #2323: Sort models with stable ordering to prevent duplicates/missing
460+
// models when paginating. All sort modes use secondary sort by ID to ensure
461+
// consistent ordering across paginated requests.
460462
match sort_order {
461463
ModelSortOrder::Id => {
464+
// Primary sort by ID ensures unique ordering
462465
models.sort_by(|a, b| a.id.cmp(&b.id));
463466
}
464467
ModelSortOrder::Name => {
465-
models.sort_by(|a, b| a.name.cmp(&b.name));
468+
// Sort by name, then by ID for stable ordering when names are equal
469+
models.sort_by(|a, b| a.name.cmp(&b.name).then_with(|| a.id.cmp(&b.id)));
466470
}
467471
ModelSortOrder::Provider => {
468472
// Sort by provider, then by id for stable ordering

cortex-cli/src/pr_cmd.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,10 @@ async fn run_pr_checkout(args: PrCli) -> Result<()> {
144144

145145
let pr_info = client.get_pull_request(pr_number).await?;
146146

147-
// Display PR info
147+
// Issue #2325: Display PR info with timestamp to indicate when metadata was fetched
148+
// If the PR status changes during long operations (like diff analysis), the user
149+
// knows when the displayed status was retrieved.
150+
let metadata_timestamp = chrono::Utc::now().format("%Y-%m-%d %H:%M:%S UTC");
148151
println!("Title: {}", pr_info.title);
149152
println!("Author: @{}", pr_info.author);
150153
// Display state with draft indicator if applicable
@@ -153,7 +156,7 @@ async fn run_pr_checkout(args: PrCli) -> Result<()> {
153156
} else {
154157
pr_info.state.clone()
155158
};
156-
println!("State: {}", state_display);
159+
println!("State: {} (as of {})", state_display, metadata_timestamp);
157160
println!(
158161
"Base: {} ← Head: {}",
159162
pr_info.base_branch, pr_info.head_branch

cortex-cli/src/run_cmd.rs

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -806,7 +806,32 @@ impl RunCli {
806806
if let Some(ref model) = self.model {
807807
let resolution = resolve_model_with_info(model);
808808
warn_if_ambiguous_model(&resolution, model);
809-
config.model = resolution.model;
809+
config.model = resolution.model.clone();
810+
811+
// Issue #2326: Warn if --stream is used with a model that may not support streaming
812+
// Known non-streaming or limited-streaming models
813+
let non_streaming_patterns = [
814+
"embedding",
815+
"text-embedding",
816+
"ada-002",
817+
"text-search",
818+
"text-similarity",
819+
];
820+
let model_lower = resolution.model.to_lowercase();
821+
if streaming_enabled {
822+
let is_embedding_model = non_streaming_patterns
823+
.iter()
824+
.any(|p| model_lower.contains(p));
825+
if is_embedding_model {
826+
eprintln!(
827+
"{}Warning:{} Model '{}' appears to be an embedding model which does not support streaming. \
828+
Response will be returned as a batch despite --stream flag.",
829+
TermColor::Yellow.ansi_code(),
830+
TermColor::Default.ansi_code(),
831+
model
832+
);
833+
}
834+
}
810835
}
811836

812837
// Apply temperature override if provided

cortex-storage/src/paths.rs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,40 @@
66
//! - **Windows**: `%APPDATA%\Cortex\` (e.g., `C:\Users\<user>\AppData\Roaming\Cortex\`)
77
//! - **macOS**: `~/Library/Application Support/Cortex/`
88
//! - **Linux**: `~/.local/share/Cortex/`
9+
//!
10+
//! ## Issue #2324: Docker --read-only Container Support
11+
//!
12+
//! When running Cortex in a Docker container with `--read-only` filesystem flag,
13+
//! the following directories must be mounted as writable volumes:
14+
//!
15+
//! **Required writable directories:**
16+
//! 1. **Data directory** (session storage, history):
17+
//! - Linux: `~/.local/share/Cortex/` or `$CORTEX_DATA_DIR`
18+
//! - macOS: `~/Library/Application Support/Cortex/`
19+
//! - Windows: `%APPDATA%\Cortex\`
20+
//!
21+
//! 2. **Config directory** (configuration files):
22+
//! - Linux: `~/.config/Cortex/` or legacy `~/.cortex/`
23+
//! - macOS: `~/Library/Application Support/Cortex/`
24+
//! - Windows: `%APPDATA%\Cortex\`
25+
//!
26+
//! 3. **Cache directory** (temporary files):
27+
//! - Linux: `~/.cache/Cortex/` or `$XDG_CACHE_HOME/Cortex/`
28+
//! - macOS: `~/Library/Caches/Cortex/`
29+
//! - Windows: `%LOCALAPPDATA%\Cortex\Cache\`
30+
//!
31+
//! **Example Docker command:**
32+
//! ```bash
33+
//! docker run --read-only \
34+
//! -v /host/cortex-data:/home/user/.local/share/Cortex \
35+
//! -v /host/cortex-config:/home/user/.config/Cortex \
36+
//! -v /host/cortex-cache:/home/user/.cache/Cortex \
37+
//! -v /tmp:/tmp:rw \
38+
//! cortex:latest
39+
//! ```
40+
//!
41+
//! **Note:** Cortex may also use system temp directories (`/tmp` or `$TMPDIR`)
42+
//! for ephemeral files. Mount these as writable if needed.
943
1044
use std::path::PathBuf;
1145
use tracing::debug;

cortex-tui/src/app.rs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1229,6 +1229,34 @@ impl AppState {
12291229
self.terminal_size = (width, height);
12301230
}
12311231

1232+
/// Issue #2327: Invalidate cached content layout calculations.
1233+
///
1234+
/// Called on terminal resize to ensure code blocks and wrapped text
1235+
/// are properly re-rendered for the new terminal dimensions.
1236+
/// This prevents rendering corruption when the terminal is resized
1237+
/// while streaming content (especially code blocks with syntax highlighting).
1238+
pub fn invalidate_content_layout(&mut self) {
1239+
// Reset scroll positions to prevent displaying content outside new bounds
1240+
self.chat_scroll = 0;
1241+
self.sidebar_scroll = 0;
1242+
self.diff_scroll = 0;
1243+
1244+
// Reset content line counts - will be recalculated on next render
1245+
self.chat_content_lines = 0;
1246+
self.chat_visible_lines = 0;
1247+
1248+
// Clear any partial text segment that might have incomplete line wrapping
1249+
// The typewriter will regenerate content on the next render
1250+
if let Some(ref mut tw) = self.typewriter {
1251+
tw.reset_animation();
1252+
}
1253+
1254+
// Re-pin to bottom if we were following the stream
1255+
if self.streaming.is_streaming {
1256+
self.chat_scroll_pinned_bottom = true;
1257+
}
1258+
}
1259+
12321260
/// Request to quit the application
12331261
pub fn quit(&mut self) {
12341262
self.running = false;

cortex-tui/src/providers/manager.rs

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -427,21 +427,63 @@ impl ProviderManager {
427427
///
428428
/// This method re-loads the config from disk and also re-evaluates any environment
429429
/// variable substitutions (e.g., `{env:VAR_NAME}` placeholders) to pick up current values.
430+
///
431+
/// Issue #2320: Environment variables are now re-evaluated during config reload,
432+
/// ensuring that changes to env vars (like API keys) take effect without a full restart.
430433
pub fn reload_config(&mut self) -> Result<()> {
431434
// Re-load config from file (this will pick up any file changes)
432435
self.config = CortexConfig::load()?;
433436

434-
// Re-evaluate environment-dependent config values by refreshing default values
435-
// The api_url may reference CORTEX_API_URL which could have changed
436-
if self.config.api_url == super::config::DEFAULT_API_URL {
437-
// Re-check env var in case it was set after initial load
438-
if let Ok(url) = std::env::var("CORTEX_API_URL") {
437+
// Issue #2320: Re-evaluate ALL environment-dependent config values
438+
// This ensures env var changes take effect on config reload
439+
440+
// Re-check CORTEX_API_URL in case it was set/changed after initial load
441+
if let Ok(url) = std::env::var("CORTEX_API_URL") {
442+
if !url.is_empty() {
439443
self.config.api_url = url;
440444
}
441445
}
442446

447+
// Re-check CORTEX_AUTH_TOKEN - force token refresh by clearing cached auth
448+
// The next API call will re-read the token from environment or keyring
449+
self.auth_token = None;
450+
451+
// If CORTEX_AUTH_TOKEN is now set, update our cached token
452+
if let Ok(token) = std::env::var("CORTEX_AUTH_TOKEN") {
453+
if !token.is_empty() {
454+
self.auth_token = Some(token);
455+
}
456+
}
457+
458+
// Re-check CORTEX_DEFAULT_MODEL if set
459+
if let Ok(model) = std::env::var("CORTEX_DEFAULT_MODEL") {
460+
if !model.is_empty() {
461+
self.config.default_model = model;
462+
}
463+
}
464+
465+
// Re-check CORTEX_MAX_TOKENS if set
466+
if let Ok(max_tokens) = std::env::var("CORTEX_MAX_TOKENS") {
467+
if let Ok(tokens) = max_tokens.parse::<u32>() {
468+
self.config.max_tokens = tokens;
469+
}
470+
}
471+
472+
// Re-check CORTEX_TEMPERATURE if set
473+
if let Ok(temp) = std::env::var("CORTEX_TEMPERATURE") {
474+
if let Ok(temperature) = temp.parse::<f32>() {
475+
self.config.temperature = temperature;
476+
}
477+
}
478+
443479
// Reset client to pick up new config values
480+
// This forces a new client to be created with updated settings
444481
self.client = None;
482+
483+
// Clear cached models since API credentials may have changed
484+
self.cached_models = None;
485+
486+
tracing::info!("Configuration reloaded with fresh environment variable values");
445487
Ok(())
446488
}
447489

0 commit comments

Comments
 (0)