Skip to content

Conversation

@echobt
Copy link
Contributor

@echobt echobt commented Feb 4, 2026

Summary

Replaces an unsafe unwrap() call in the ExecRunner::init_client method with expect() containing a descriptive error message.

Problem

The init_client method uses self.client.as_ref().unwrap().as_ref() which could panic without context if the client is unexpectedly None. While the code structure ensures client is set before this line, the unwrap provides no debugging context if this invariant is violated.

Changes

  • Updated /workspace/src/cortex-exec/src/runner.rs to use expect() with a descriptive message

Testing

  • Verified code compiles with cargo check -p cortex-exec
  • The expect() message clarifies that this should never fail because init_client sets the client field

Verification

cargo check -p cortex-exec

@greptile-apps
Copy link

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

Replaced unwrap() with expect() containing a descriptive error message in the init_client method. The change improves error diagnostics without altering the functional behavior of the code.

Key Changes:

  • Updated line 190 in src/cortex-exec/src/runner.rs to use expect("Client should be initialized in init_client") instead of unwrap()
  • The logic guarantees that self.client is always Some(...) at this point, making the panic theoretically unreachable
  • If the invariant is ever violated due to a future code change, the error message now provides clear context

Code Analysis:
The init_client method ensures self.client is initialized before the return statement:

  • If self.client.is_none() is true, line 187 sets it to Some(client)
  • If self.client.is_none() is false, it was already Some(...)
  • Therefore, line 190's expect() should never panic under normal execution

This is a minor defensive improvement that follows Rust best practices for better error messages.

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • The change is a simple, low-risk improvement that replaces unwrap() with expect() and adds a descriptive error message. The logic analysis confirms that the client is always initialized before this line executes, so the panic condition is theoretically unreachable. The change improves code maintainability and debugging without any functional behavior changes. The code compiles successfully and follows Rust best practices.
  • No files require special attention

Important Files Changed

Filename Overview
src/cortex-exec/src/runner.rs Replaced unwrap() with expect() containing a descriptive error message for better debugging context

Sequence Diagram

sequenceDiagram
    participant Caller
    participant ExecRunner
    participant init_client
    participant create_client
    
    Caller->>ExecRunner: run_inner()
    ExecRunner->>init_client: init_client()
    
    alt client is None
        init_client->>create_client: create_client(provider_id, model, api_key, None)
        create_client-->>init_client: Result<Box<dyn ModelClient>>
        alt creation succeeds
            init_client->>init_client: self.client = Some(client)
        else creation fails
            init_client-->>ExecRunner: Err(CortexError)
        end
    else client already exists
        Note over init_client: client already Some(...)
    end
    
    init_client->>init_client: self.client.as_ref().expect("...")
    Note over init_client: Changed from unwrap() to expect()<br/>with descriptive error message
    init_client-->>ExecRunner: Ok(&dyn ModelClient)
    ExecRunner->>ExecRunner: Continue execution
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.

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

echobt added a commit that referenced this pull request Feb 4, 2026
This PR consolidates the following fixes:
- #74: Prevent shell injection in restore_script via path escaping
- #76: Replace unsafe unwrap() with expect() in init_client
- #78: Use secure random temp files in external editor to prevent symlink attacks
- #79: Add per-chunk streaming timeout to prevent indefinite hangs

Key changes:
- Added shell escaping for paths in shell-snapshot restore scripts
- Replaced unwrap() with expect() for better error context in exec runner
- Use secure random temp files instead of predictable names
- Added streaming chunk timeout to prevent hangs during LLM responses
@echobt
Copy link
Contributor Author

echobt commented Feb 4, 2026

Consolidated into #82 - fix: consolidated security and robustness improvements

@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