-
Notifications
You must be signed in to change notification settings - Fork 312
Fix mypy errors with --check-untyped-defs#2491
Fix mypy errors with --check-untyped-defs#2491rhatdan wants to merge 2 commits intocontainers:mainfrom
Conversation
Resolve all 42 mypy errors reported by:
mypy --exclude=.venv --exclude=venv --exclude=.tox --exclude=build
--exclude test /home/dwalsh/ramalama --check-untyped-defs
- compat: add type: ignore[call-overload] for NamedTemporaryFile wrapper
- toml_parser: type value as object and guard with isinstance(value, dict)
- http_client: only rename when output_file_partial is not None
- engine: narrow pull/network/oci_runtime/device with getattr and guards
- chat: fix choice/content type (isinstance(choice, dict)); use setattr for mcp callback; type i and total_time_slept as float; use getattr for server_process and name in kills()
- transports/base: use getattr for resolve_model, strategy, mount_cmd (OCI)
- transports/oci: getattr for runtime._convert_to_gguf
- rag: getattr(self.args, 'rag', None) for BaseEngineArgsType
- hf_style_repo_base: type blob_url, model_hash, model_filename, mmproj_* as str | None; assert mmproj_* in mmproj_file()
- transports/huggingface: annotate safetensors_files; getattr for hf_cli_available
- cli: getattr for inspect_metadata; type: ignore[call-arg] for model.inspect
- daemon: type idle_check_timer as Timer | None; type idle_check_interval; guard expiration_date for None in check_model_expiration
Made-with: Cursor
Summary by Sourcery
Resolve mypy --check-untyped-defs issues by tightening type usage, guarding optional attributes, and adding targeted ignores across core modules.
Bug Fixes:
- Prevent crashes in chat streaming, RAG label/mount setup, and HTTP downloads by checking types and None values before use.
- Avoid attribute errors when optional arguments, strategies, runtimes, or CLI capabilities are missing in engine, transports, and runtime integration.
- Fix incorrect model expiration handling by safely handling missing expiration_date attributes.
Enhancements:
- Add explicit type annotations and runtime assertions for configuration objects, timers, and repository metadata structures to improve static typing and safety.
Reviewer's GuideThis PR resolves mypy errors (with --check-untyped-defs) across multiple modules by tightening type annotations, guarding attribute access with getattr/isinstance, and adding a few targeted type: ignore directives while keeping runtime behavior effectively unchanged. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the type safety and robustness of the codebase by resolving 42 mypy errors. The changes primarily involve adding explicit type hints, implementing safer attribute access patterns using Highlights
New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with and on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Hey - I've found 2 issues
Prompt for AI Agents
## Individual Comments
### Comment 1
<location path="ramalama/transports/base.py" line_range="367-371" />
if self.type == "Ollama":
args.UNRESOLVED_MODEL = args.MODEL
- args.MODEL = self.resolve_model()
+ resolve_model = getattr(self, "resolve_model", None)
+ if callable(resolve_model):
+ args.MODEL = resolve_model()
+ else:
+ args.MODEL = args.MODEL
self.engine = self.new_engine(args)
if args.subcommand == "run" and not getattr(args, "ARGS", None) and sys.stdin.isatty():
**issue (bug_risk):** Silently ignoring missing `resolve_model` may obscure configuration errors.
Previously an AttributeError would fail fast when `resolve_model` was missing; now the code quietly continues with the original `args.MODEL`. This risks hiding misconfiguration and making debugging harder. Prefer either preserving the hard failure or explicitly raising a clear error when `resolve_model` is missing or not callable, rather than silently falling back.
### Comment 2
<location path="ramalama/http_client.py" line_range="52-53" />
finally:
del out # Ensure file is closed before rename
- if output_file:
+ if output_file and output_file_partial is not None:
os.rename(output_file_partial, output_file)
**suggestion (bug_risk):** Additional `output_file_partial is not None` guard may hide earlier failures to create the temp file.
This change avoids a crash but can mask earlier logic errors: if `output_file` is set but `output_file_partial` was never assigned, the rename is now silently skipped. Consider either guaranteeing `output_file_partial` is always set whenever `output_file` is truthy, or explicitly raising/logging when `output_file` is provided but `output_file_partial` is unexpectedly `None`.
```suggestion
if output_file:
if output_file_partial is None:
# If we expected to write to an output file but never created
# a corresponding temporary file, fail explicitly rather than
# silently skipping the rename.
raise RuntimeError(
"output_file is set but output_file_partial is None; "
"temporary output file was never created"
)
os.rename(output_file_partial, output_file)
```
Help me be more useful! Please click or on each comment and I'll use the feedback to improve your reviews.
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.
Code Review
This pull request effectively addresses a number of mypy errors by improving type safety and code robustness. However, the audit identified critical security vulnerabilities: untrusted output from LLMs is printed directly to the terminal, posing a risk of Terminal Escape Sequence Injection, and user-supplied RAG paths are used in container mount arguments without sufficient sanitization, which could lead to Argument Injection. These require immediate attention through proper sanitization for terminal output and validation of container mount arguments. Additionally, a minor code quality suggestion involves removing a redundant code block for improved conciseness.
ramalama/chat.py
Outdated
| print(f"{color_yellow}{choice}{color_default}", end="", flush=True) | ||
| assistant_response += choice | ||
| if content: | ||
| print(f"{color_yellow}{content}{color_default}", end="", flush=True) |
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.
The code prints untrusted output from the LLM directly to the console. If the output contains ANSI escape sequences, it can be used to manipulate the user's terminal (Terminal Escape Sequence Injection). This could lead to information disclosure or other malicious actions depending on the terminal emulator. It is recommended to sanitize the output by removing or escaping non-printable characters and potentially dangerous escape sequences.
0c6f3df to
841693d
Compare
|
Looks good overall. Could you fix the lint issue? |
mypy --exclude=.venv --exclude=venv --exclude=.tox --exclude=build
--exclude test /home/dwalsh/ramalama --check-untyped-defs
- compat: add type: ignore[call-overload] for NamedTemporaryFile wrapper
- toml_parser: type value as object and guard with isinstance(value, dict)
- http_client: only rename when output_file_partial is not None
- engine: narrow pull/network/oci_runtime/device with getattr and guards
- chat: fix choice/content type (isinstance(choice, dict)); use setattr for
mcp callback; type i and total_time_slept as float; use getattr for
server_process and name in kills()
- transports/base: use getattr for resolve_model, strategy, mount_cmd (OCI)
- transports/oci: getattr for runtime._convert_to_gguf
- rag: getattr(self.args, 'rag', None) for BaseEngineArgsType
- hf_style_repo_base: type blob_url, model_hash, model_filename, mmproj_*
as str | None; assert mmproj_* in mmproj_file()
- transports/huggingface: annotate safetensors_files; getattr for hf_cli_available
- cli: getattr for inspect_metadata; type: ignore[call-arg] for model.inspect
- daemon: type idle_check_timer as Timer | None; type idle_check_interval;
guard expiration_date for None in check_model_expiration
Made-with: Cursor
Signed-off-by: Daniel J Walsh
841693d to
c9d58e1
Compare