Skip to content

Conversation

@echobt
Copy link
Contributor

@echobt echobt commented Feb 4, 2026

Summary

Fixes #5149 - HelpBrowserState::current_section() panics on empty sections.

Problem

Direct array indexing without bounds check causes panic when the sections vector is empty.

Solution

  • Changed current_section() to return Option<&HelpSection> instead of &HelpSection
  • Used safe .get() access instead of direct indexing
  • Updated render_content() to handle None gracefully by early returning
  • Added test case for empty sections scenario
  • Updated existing tests to use .expect() for assertions

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).
Changed current_section() to return Option<&HelpSection> and use
safe .get() access instead of direct indexing. Updated render_content()
to handle None gracefully by early returning, and updated tests to use
.expect() for assertions.

Fixes #5149
@greptile-apps
Copy link

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

This PR fixes a panic in HelpBrowserState::current_section() when the sections vector is empty by changing the return type to Option<&HelpSection> and using safe bounds checking with .get() instead of direct array indexing.

The PR also includes several unrelated fixes across the codebase:

  • Path security improvements: Fixed path traversal validation to properly distinguish between .. as a path component vs. in filenames, and improved tilde expansion to handle bare ~
  • Command validation hardening: Added normalization to prevent bypass attempts using whitespace, quotes, or path variations
  • Session storage error handling: Enhanced error logging and handling in session listing operations
  • Type safety improvements: Changed token count fields from i64 to u64 to prevent negative values, added validation for threshold percentage (0.0-1.0)
  • New tool response store: Added bounded storage with TTL and automatic cleanup to prevent unbounded memory growth

The primary TUI fix is solid and well-tested. The additional changes appear to be separate bug fixes bundled together.

Confidence Score: 4/5

  • This PR is generally safe to merge with minor concerns about scope creep
  • The primary TUI panic fix is well-implemented and properly tested. Additional fixes for security (path validation, command normalization), error handling, type safety, and race conditions are all improvements. However, bundling multiple unrelated fixes in a single PR increases review complexity and risk. Each fix appears correct individually, but the broad scope makes thorough validation more challenging.
  • No critical issues found, but review the bundled fixes in src/cortex-engine/src/validation.rs and src/cortex-tui/src/session/manager.rs for complexity

Important Files Changed

Filename Overview
src/cortex-tui/src/widgets/help_browser/state.rs Changed current_section() to return Option<&HelpSection> with safe bounds checking using .get()
src/cortex-tui/src/widgets/help_browser/render.rs Updated to handle None from current_section() with early return, preventing panic on empty sections
src/cortex-tui/src/widgets/help_browser/tests.rs Added test for empty sections scenario, updated existing tests to use .expect() for unwrapping
src/cortex-cli/src/utils/paths.rs Fixed path traversal detection to properly handle filenames containing ".." and improved tilde expansion
src/cortex-engine/src/validation.rs Added command normalization to prevent validation bypass via whitespace, quotes, or path variations
src/cortex-engine/src/tools/response_store.rs Added new bounded tool response storage with TTL, eviction, and cleanup to prevent memory growth

Sequence Diagram

sequenceDiagram
    participant Caller
    participant HelpBrowser
    participant HelpBrowserState
    participant render_content

    Note over Caller,render_content: Before Fix (Panic Scenario)
    Caller->>HelpBrowserState: new()
    HelpBrowserState->>HelpBrowserState: sections.clear()
    Caller->>HelpBrowser: render()
    HelpBrowser->>render_content: render_content()
    render_content->>HelpBrowserState: current_section()
    HelpBrowserState->>HelpBrowserState: &self.sections[index]
    Note over HelpBrowserState: ❌ PANIC: index out of bounds

    Note over Caller,render_content: After Fix (Safe Handling)
    Caller->>HelpBrowserState: new()
    HelpBrowserState->>HelpBrowserState: sections.clear()
    Caller->>HelpBrowser: render()
    HelpBrowser->>render_content: render_content()
    render_content->>HelpBrowserState: current_section()
    HelpBrowserState->>HelpBrowserState: sections.get(index)
    HelpBrowserState-->>render_content: None
    render_content->>render_content: early return
    Note over render_content: ✅ Safe: graceful handling
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

echobt added a commit that referenced this pull request Feb 4, 2026
… 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
@echobt
Copy link
Contributor Author

echobt commented Feb 4, 2026

Consolidated into #69 - fix(tui): consolidated TUI fixes for overflow, positioning, and panic prevention

@echobt echobt closed this Feb 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant