-
Notifications
You must be signed in to change notification settings - Fork 3
fix(tui): fix cursor positioning and underflow in selection list #58
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
Conversation
Fixes #5292 and #5293 - ToolResponseStore memory issues. Problem: 1. Store grows without limit 2. Consumed responses not removed Solution: - Added ToolResponseStore with configurable max size (default: 500 entries) - Entries are removed when consumed via take() method (#5293) - Automatic periodic cleanup of expired entries based on TTL - Eviction of oldest entries when capacity is reached (#5292) Features: - MAX_STORE_SIZE constant (500) to prevent unbounded growth - DEFAULT_TTL (5 minutes) for automatic expiration - CLEANUP_INTERVAL (1 minute) for periodic cleanup - get() for peeking without removal - take() for consuming and removing entries - cleanup_expired() and cleanup_read() for manual cleanup - Stats tracking for monitoring store behavior
- server.rs: Hold write lock during entire state check and modification in handle_initialize to prevent concurrent initialization races - registry.rs: Use HashMap entry API to atomically check-and-insert plugin registration to prevent duplicate registration races Fixes #5262, #5260
Fixes bypass attempts where blocked commands like 'rm -rf' could be evaded using: - Extra whitespace: 'rm -rf' → 'rm -rf' - Quoted parts: "'rm' -rf" → 'rm -rf' - Path variants: '/bin/rm -rf' → 'rm -rf' Added normalize_command() function that: 1. Collapses whitespace by splitting/joining 2. Strips surrounding quotes from command parts 3. Extracts basename for command (first part) Also added comprehensive tests for bypass scenarios.
- Fix #5181: validate_path_safety() now checks path components instead of substring match, preventing false positives for filenames like 'file..txt' - Fix #5183: expand_tilde() now handles bare '~' path in addition to '~/' prefix - Updated tests to verify correct behavior
Fixes #5208, #5210, #5244 - Session storage error handling improvements. Changes: - list_sessions: Now properly collects and reports IO errors instead of silently ignoring them. Returns available sessions while logging errors for failed ones. - SessionManager: Only updates in-memory state after successful storage operations. If storage fails, the session is marked as modified for later retry. - Default impl: Improved panic message to help diagnose initialization failures (permissions, disk space).
Greptile OverviewGreptile SummaryWARNING: PR title and description are completely incorrect. This PR has nothing to do with TUI cursor positioning or selection lists. The actual changes address multiple security and reliability issues: Security Fixes
Concurrency Fixes
Type Safety
Memory Management
Reliability Improvements
Critical Issues Found
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/cortex-cli/src/utils/paths.rs | Improved tilde expansion to handle bare ~ and fixed path traversal detection using component analysis instead of string contains |
| src/cortex-engine/src/tools/response_store.rs | New module: bounded tool response storage with TTL, capacity limits, and automatic cleanup to prevent unbounded memory growth |
| src/cortex-engine/src/validation.rs | Added command normalization to prevent bypass via whitespace, quotes, or path variants in blocked command validation |
| src/cortex-mcp-server/src/server.rs | Fixed TOCTOU race by holding write lock during entire check-and-set operation in handle_initialize |
| src/cortex-plugins/src/registry.rs | Fixed TOCTOU race using Entry API for atomic check-and-insert in plugin registration |
| src/cortex-tui/src/session/manager.rs | Enhanced error handling and consistency in message operations - restores state on storage failure in pop_last_exchange |
Sequence Diagram
sequenceDiagram
participant User
participant CLI
participant Validator
participant Config
participant MCPServer
participant PluginRegistry
participant Storage
participant ToolStore
Note over User,ToolStore: Path Validation Flow
User->>CLI: provide path with "~"
CLI->>CLI: expand_tilde("~/path")
CLI->>CLI: validate_path_safety()
CLI->>CLI: check components for ParentDir
CLI-->>User: safe expanded path
Note over User,ToolStore: Command Validation Flow
User->>Validator: submit command
Validator->>Validator: normalize_command()
Note right of Validator: Remove quotes, whitespace,<br/>extract basename
Validator->>Validator: check against blocked list
Validator-->>User: validation result
Note over User,ToolStore: Config Validation Flow
User->>Config: load config with ratios
Config->>Config: deserialize_threshold_percent()
Config->>Config: validate 0.0 <= value <= 1.0
Config-->>User: validated config or error
Note over User,ToolStore: TOCTOU Race Prevention
User->>MCPServer: initialize (concurrent)
User->>MCPServer: initialize (concurrent)
MCPServer->>MCPServer: acquire write lock
MCPServer->>MCPServer: check state == Uninitialized
MCPServer->>MCPServer: set state = Initializing
MCPServer->>MCPServer: release write lock
Note right of MCPServer: Second request blocked<br/>until state change complete
MCPServer-->>User: one succeeds, one fails
User->>PluginRegistry: register plugin (concurrent)
PluginRegistry->>PluginRegistry: acquire write lock
PluginRegistry->>PluginRegistry: Entry API check-and-insert
PluginRegistry-->>User: atomic operation
Note over User,ToolStore: Storage Error Handling
User->>Storage: list_sessions()
Storage->>Storage: iterate directory entries
alt entry read fails
Storage->>Storage: collect error, continue
end
alt session load fails
Storage->>Storage: collect error, continue
end
Storage-->>User: available sessions + error logs
Note over User,ToolStore: Tool Response Store
User->>ToolStore: store(call_id, result)
alt at capacity
ToolStore->>ToolStore: evict oldest entry
end
ToolStore->>ToolStore: store new response
User->>ToolStore: take(call_id)
ToolStore->>ToolStore: remove and return
Note right of ToolStore: Prevents unbounded growth
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.
6 files reviewed, 2 comments
| // Restore in reverse order (prev was popped second, so push it first) | ||
| if let Some(msg) = prev { | ||
| self.messages.push(msg); | ||
| } | ||
| if let Some(msg) = last { | ||
| self.messages.push(msg); |
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.
Incorrect restore order causes message ordering bug. Line 221-222 pop in order last, prev, but restore pushes prev, last, resulting in reversed order.
| // Restore in reverse order (prev was popped second, so push it first) | |
| if let Some(msg) = prev { | |
| self.messages.push(msg); | |
| } | |
| if let Some(msg) = last { | |
| self.messages.push(msg); | |
| // Restore in correct order: prev was popped second, last was popped first | |
| if let Some(msg) = last { | |
| self.messages.push(msg); | |
| } | |
| if let Some(msg) = prev { | |
| self.messages.push(msg); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cortex-tui/src/session/manager.rs
Line: 287:292
Comment:
Incorrect restore order causes message ordering bug. Line 221-222 pop in order `last, prev`, but restore pushes `prev, last`, resulting in reversed order.
```suggestion
// Restore in correct order: prev was popped second, last was popped first
if let Some(msg) = last {
self.messages.push(msg);
}
if let Some(msg) = prev {
self.messages.push(msg);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/cortex-cli/src/utils/paths.rs
Line: 88:90
Comment:
This pattern still has false positives: `..hidden` starts with ".." but is NOT a parent directory traversal - it's a valid filename starting with two dots.
```suggestion
let safe_patterns = ["file..txt", "...hidden", "test...file", "foo/bar..baz/file"];
```
How can I resolve this? If you propose a fix, please make it concise. |
… prevention This PR consolidates the following fixes: - #38: Prevent usize to u16 overflow in interactive renderer - #42: Prevent usize to u16 overflow in card count displays - #58: Fix cursor positioning and underflow in selection list - #59: Fix mention popup positioning and Unicode width calculation - #60: Improve autocomplete popup positioning and width calculation - #64: Prevent underflow in dropdown navigation and scroll calculations - #66: Prevent panic in HelpBrowserState when sections empty All changes target the TUI components to improve robustness: - Added saturating casts for u16 conversions - Fixed cursor positioning calculations - Added bounds checking for empty sections - Improved Unicode width handling for popups
|
Consolidated into #69 - fix(tui): consolidated TUI fixes for overflow, positioning, and panic prevention |
Summary
Fixes #5175, #5174, #5173 - Selection list cursor and layout issues.
Problems
Solution
Used char-aware cursor positioning and saturating arithmetic.