Skip to content

fix: consolidated protocol robustness improvements#88

Closed
echobt wants to merge 1 commit intomainfrom
consolidated/protocol-robustness
Closed

fix: consolidated protocol robustness improvements#88
echobt wants to merge 1 commit intomainfrom
consolidated/protocol-robustness

Conversation

@echobt
Copy link
Contributor

@echobt echobt commented Feb 4, 2026

Summary

This PR consolidates 2 protocol robustness improvements.

Included PRs:

Key Changes:

  • Added MAX_BUFFER_SIZE constant (10,000 events) for StreamProcessor
  • Modified process() to drop oldest events when buffer is full
  • Pre-allocated buffer capacity in new() for better performance
  • Added can_transition_to() method to ToolState enum
  • Updated update_tool_state to log warnings on invalid transitions
  • Documented valid state machine transitions

State Machine

Pending -> Running -> Completed
    |         |
    |         +-> Error
    +-> Completed
    +-> Error

Files Modified:

  • src/cortex-engine/src/streaming.rs
  • src/cortex-protocol/src/protocol/message_parts.rs
  • Cargo.lock
  • src/cortex-protocol/Cargo.toml

Closes #84, #85

## Summary

This PR consolidates **2 protocol robustness improvements**.

### Included PRs:
- #84: Add buffer size limit to StreamProcessor
- #85: Add ToolState transition validation

### Key Changes:
- Added MAX_BUFFER_SIZE constant (10,000 events) for StreamProcessor
- Modified process() to drop oldest events when buffer is full
- Pre-allocated buffer capacity in new() for better performance
- Added can_transition_to() method to ToolState enum
- Updated update_tool_state to log warnings on invalid transitions
- Documented valid state machine transitions

### Files Modified:
- src/cortex-engine/src/streaming.rs
- src/cortex-protocol/src/protocol/message_parts.rs

Closes #84, #85
@greptile-apps
Copy link

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

This PR consolidates two protocol robustness improvements by adding buffer management and state transition validation.

Key Changes

StreamProcessor Buffer Management (src/cortex-engine/src/streaming.rs):

  • Added MAX_BUFFER_SIZE constant (10,000 events) to prevent unbounded memory growth
  • When buffer reaches capacity, oldest event is dropped via pop_front()
  • Pre-allocated buffer capacity of 1,024 events for performance optimization

ToolState Transition Validation (src/cortex-protocol/src/protocol/message_parts.rs):

  • Implemented can_transition_to() method with documented state machine transitions
  • Added validation in update_tool_state() to log warnings on invalid transitions
  • State machine: Pending → Running/Completed/Error, Running → Completed/Error (terminal states cannot transition)
  • Added tracing dependency to cortex-protocol for logging

Notes

The ToolState validation is currently non-blocking - invalid transitions are logged but still applied. This maintains backward compatibility but may allow unintended state changes from terminal states.

Confidence Score: 4/5

  • This PR is safe to merge with low risk - implements defensive programming patterns
  • The changes add robustness protections (buffer limits and state validation) without breaking existing functionality. The buffer size limit properly prevents memory exhaustion, and the state validation provides helpful logging. Minor consideration: state transitions log but don't block invalid changes, which is intentional for backward compatibility.
  • No files require special attention - changes are straightforward defensive improvements

Important Files Changed

Filename Overview
src/cortex-engine/src/streaming.rs Added MAX_BUFFER_SIZE limit (10,000 events) with pop_front() when buffer is full, pre-allocated capacity of 1,024
src/cortex-protocol/src/protocol/message_parts.rs Added can_transition_to() method with state machine validation and warning logs for invalid transitions

Sequence Diagram

sequenceDiagram
    participant Client
    participant MessageWithParts
    participant ToolState
    
    Note over ToolState: Initial State: Pending
    
    Client->>MessageWithParts: update_tool_state(call_id, Running)
    MessageWithParts->>ToolState: can_transition_to(Running)?
    ToolState-->>MessageWithParts: true (Pending → Running valid)
    MessageWithParts->>ToolState: Update state to Running
    MessageWithParts-->>Client: true
    
    Note over ToolState: Current State: Running
    
    Client->>MessageWithParts: update_tool_state(call_id, Completed)
    MessageWithParts->>ToolState: can_transition_to(Completed)?
    ToolState-->>MessageWithParts: true (Running → Completed valid)
    MessageWithParts->>ToolState: Update state to Completed
    MessageWithParts-->>Client: true
    
    Note over ToolState: Current State: Completed (Terminal)
    
    Client->>MessageWithParts: update_tool_state(call_id, Running)
    MessageWithParts->>ToolState: can_transition_to(Running)?
    ToolState-->>MessageWithParts: false (Completed is terminal)
    MessageWithParts->>MessageWithParts: Log warning
    MessageWithParts->>ToolState: Update state anyway
    MessageWithParts-->>Client: true
    
    Note over MessageWithParts,ToolState: Invalid transition logged but state still updated
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.

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +605 to 613
if !state.can_transition_to(&new_state) {
tracing::warn!(
"Invalid ToolState transition from {:?} to {:?} for call_id {}",
state,
new_state,
call_id
);
}
*state = new_state;
Copy link

Choose a reason for hiding this comment

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

The state transition validation logs a warning but still updates the state. This means invalid transitions (e.g., from terminal states) are allowed at runtime. Consider whether invalid transitions should be rejected instead of just logged.

Suggested change
if !state.can_transition_to(&new_state) {
tracing::warn!(
"Invalid ToolState transition from {:?} to {:?} for call_id {}",
state,
new_state,
call_id
);
}
*state = new_state;
if cid == call_id {
if !state.can_transition_to(&new_state) {
tracing::warn!(
"Invalid ToolState transition from {:?} to {:?} for call_id {}",
state,
new_state,
call_id
);
return false;
}
*state = new_state;
return true;
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cortex-protocol/src/protocol/message_parts.rs
Line: 605:613

Comment:
The state transition validation logs a warning but still updates the state. This means invalid transitions (e.g., from terminal states) are allowed at runtime. Consider whether invalid transitions should be rejected instead of just logged.

```suggestion
                if cid == call_id {
                    if !state.can_transition_to(&new_state) {
                        tracing::warn!(
                            "Invalid ToolState transition from {:?} to {:?} for call_id {}",
                            state,
                            new_state,
                            call_id
                        );
                        return false;
                    }
                    *state = new_state;
                    return true;
                }
```

How can I resolve this? If you propose a fix, please make it concise.

@echobt
Copy link
Contributor Author

echobt commented Feb 4, 2026

Closing this PR to consolidate into a single mega-PR combining all bug fixes. The changes will be included in a new consolidated PR.

@echobt echobt closed this Feb 4, 2026
echobt added a commit that referenced this pull request Feb 4, 2026
This PR consolidates all bug fixes and security improvements from PRs #69-88 into a single cohesive change.

## Categories

### Security Fixes
- Path traversal prevention in MCP and session storage
- Shell injection prevention in restore scripts
- Secure random temp files for external editor
- TOCTOU race condition fixes

### TUI Improvements
- Overflow prevention for u16 conversions
- Cursor positioning fixes in selection lists
- Unicode width handling for popups
- Empty section handling in help browser

### Error Handling
- Graceful semaphore and init failure handling
- Improved error propagation in middleware
- Better client access error handling
- SystemTime operation safety

### Memory and Storage
- Cache size limits to prevent unbounded growth
- File lock cleanup for memory leak prevention
- fsync after critical writes for durability
- Bounded ToolResponseStore with automatic cleanup

### Protocol Robustness
- Buffer size limits for StreamProcessor
- ToolState transition validation
- State machine documentation

### Numeric Safety
- Saturating operations to prevent overflow/underflow
- Safe UTF-8 string slicing throughout codebase

### Tools
- Parameter alias support for backward compatibility
- Handler name consistency fixes

## Files Modified
Multiple files across cortex-tui, cortex-engine, cortex-exec, cortex-common,
cortex-protocol, cortex-storage, cortex-mcp-server, and other crates.

Closes #69, #70, #71, #73, #75, #80, #82, #87, #88
echobt added a commit that referenced this pull request Feb 4, 2026
This PR consolidates all bug fixes and security improvements from PRs #69-88 into a single cohesive change.

## Categories

### Security Fixes
- Path traversal prevention in MCP and session storage
- Shell injection prevention in restore scripts
- Secure random temp files for external editor
- TOCTOU race condition fixes

### TUI Improvements
- Overflow prevention for u16 conversions
- Cursor positioning fixes in selection lists
- Unicode width handling for popups
- Empty section handling in help browser

### Error Handling
- Graceful semaphore and init failure handling
- Improved error propagation in middleware
- Better client access error handling
- SystemTime operation safety

### Memory and Storage
- Cache size limits to prevent unbounded growth
- File lock cleanup for memory leak prevention
- fsync after critical writes for durability
- Bounded ToolResponseStore with automatic cleanup

### Protocol Robustness
- Buffer size limits for StreamProcessor
- ToolState transition validation
- State machine documentation

### Numeric Safety
- Saturating operations to prevent overflow/underflow
- Safe UTF-8 string slicing throughout codebase

### Tools
- Parameter alias support for backward compatibility
- Handler name consistency fixes

## Files Modified
Multiple files across cortex-tui, cortex-engine, cortex-exec, cortex-common,
cortex-protocol, cortex-storage, cortex-mcp-server, and other crates.

Closes #69, #70, #71, #73, #75, #80, #82, #87, #88
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