Fix: Improve Llama.cpp model path handling and error handling

[qnixsynapse]

Describe Your Changes

This change refactors the load_llama_model function to improve how it handles and validates the model path.

Previously, the function extracted the model path but did not perform any validation. This change adds the following improvements:

It now checks for the presence of the -m flag.

It verifies that a path is provided after the -m flag.

It validates that the specified model path actually exists on the filesystem.

It ensures that the SessionInfo struct stores the canonical display path of the model, which is a more robust approach.

These changes make the model loading process more reliable and provide better error handling for invalid or missing model paths.

Fixes Issues [TBD]

• Closes bug: v0.6.6 Unable to load any models #6037 (probably)
• Closes #

Self Checklist

• Added relevant comments, esp in complex areas
• Updated docs (for bug fixes / features)
• Created issues for follow-up changes or refactoring needed

---

Important

Improves load_llama_model path handling and error handling, especially for Windows, by validating model paths and using short paths.

• Behavior:
  • load_llama_model in server.rs now checks for -m flag presence and validates the model path.
  • On Windows, uses get_short_path to convert model paths to short paths.
  • Removes error channel logic, simplifying error handling.
• Error Handling:
  • Adds detailed error messages for missing -m flag and invalid model paths.
  • Refines process exit error handling by removing redundant error checks.
• Windows Specific:
  • Adds get_short_path function to handle Windows path conversion.
  • Updates Cargo.toml to include windows-sys and tempfile dependencies for Windows.
• Tests:
  • Adds test_path_with_uncommon_dir_names to verify path handling with non-ASCII characters.
  • Updates existing tests to align with new error handling logic.

^{This description was created by for 9444d70. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to db6b1df in 1 minute and 6 seconds. Click for details.

• Reviewed 54 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:76

• Draft comment:
  Marking 'args' as mutable is fine; add a brief comment explaining why in-place modification is needed.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:109

• Draft comment:
  Improved '-m' flag check is good. Consider also trimming the model path string to avoid issues with accidental whitespace.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:119

• Draft comment:
  Consider checking that the provided model path is a file (e.g., using is_file()) rather than just exists(), and optionally use canonicalize() to obtain an absolute path.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

4. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:126

• Draft comment:
  Overwriting the argument with model_path_pb.display().to_string() works, but ensure the display form meets backend expectations. Consider using canonicalize for a fully resolved path if required.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

5. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:348

• Draft comment:
  The SessionInfo model_path is set using model_path_pb.display().to_string(). Ensure this representation is suitable for downstream consumers; consider using an absolute canonical path if needed.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_oIWwLa8EpM4XuQuq

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[github-actions]

Barecheck - Code coverage report

Total: 33.09%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 805f2f4 in 1 minute and 16 seconds. Click for details.

• Reviewed 54 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:76

• Draft comment:
  Changing the args parameter to mutable is required for updating it later. Consider if modifying the original args vector is necessary or if a copy could be used instead.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

2. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:109

• Draft comment:
  Good improvement checking for the presence of the '-m' flag. The error message is clear and concise.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:114

• Draft comment:
  The check for a provided model path after '-m' is a nice addition. Consider if you want to also canonicalize the path (e.g., with canonicalize()) to resolve any symlinks or relative segments.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

4. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:119

• Draft comment:
  Validating that the model path exists is useful. However, instead of using display() directly, you might consider using canonicalize() to store a fully resolved absolute path for robustness.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

5. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:348

• Draft comment:
  Returning the model_path using model_path_pb.display().to_string() works, but if a canonical path is desired (as mentioned in the PR description), consider using canonicalize() here as well.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_q5RhnyTIO1FU34VK

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed 1da08f6 in 2 minutes and 2 seconds. Click for details.

• Reviewed 68 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src-tauri/Cargo.toml:66

• Draft comment:
  Dependency 'windows-sys' added with the necessary feature looks good. Make sure it remains only for Windows.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:80

• Draft comment:
  Consider using path.as_os_str().encode_wide() instead of OsStr::new(path.as_ref()) for clarity, since you already have a Path.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

3. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:380

• Draft comment:
  Consider updating the SessionInfo struct to store the canonical (short) path on Windows if available, so that both the arguments and SessionInfo reflect the same validated model path.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_AACvmwhU2I49Ssv3

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 7a8ce7c in 1 minute and 52 seconds. Click for details.

• Reviewed 63 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:230

• Draft comment:
  The error channel (error_tx/error_rx) was removed entirely. This simplifies the async flow and prevents generic error words from prematurely failing the load, but it also means critical errors are no longer signaled immediately. Ensure that relying solely on process exit detection is sufficient for catching non‐ready failures.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:277

• Draft comment:
  The block that checked stderr lines for error indicators (e.g., 'error loading model', 'fatal', etc.) has been removed. This reduces false positives but may delay reporting of critical errors if the process doesn’t exit. Confirm that process exit and timeout checks adequately capture real failures.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_wOImLsuivMDLM4RN

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed 34feca9 in 2 minutes and 11 seconds. Click for details.

• Reviewed 67 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src-tauri/Cargo.toml:73

• Draft comment:
  Good: The addition of the 'tempfile' dependency under the Windows dev-dependencies is appropriate for enabling platform‐specific tests.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:86

• Draft comment:
  Suggestion: Consider handling cases where the path length exceeds the fixed buffer size (260) in get_short_path by dynamically resizing the buffer if needed.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_tAPa7bZFmxR2tHiz

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[louis-jan]

LGTM