docs: add and improve docstrings across entire codebase#75
Conversation
Add Google-style docstrings to all modules, classes, methods, and functions across 127 files. Standardize existing NumPy-style docstrings to Google-style. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThis PR adds extensive documentation across the codebase and implements several new features: password‑protected API key vault and dialog, AppConfiguration singleton, CodeDrop network discovery/send functionality with workers, richer Jupyter notebook support, Jedi-based editor enhancements, new UI widgets, extended tooling (many omega tools), and NapariBridge queue-based Qt-thread execution. Most changes preserve existing APIs while introducing new modules, methods, and attributes. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Dialog as APIKeyDialog
participant Vault as KeyVault
participant FS as Filesystem<br/>~/.omega_api_keys/
User->>Dialog: Open dialog for provider
alt No stored key
Dialog->>User: Show key + password fields
User->>Dialog: Enter key & password
Dialog->>Vault: write_api_key(name, key, password)
Vault->>Vault: Derive key (salt), encrypt API key
Vault->>FS: Save JSON {salt, encrypted}
FS-->>Vault: File persisted
Dialog->>User: Accept
else Stored key exists
Dialog->>User: Show password field only
User->>Dialog: Enter password
Dialog->>Vault: read_api_key(name, password)
Vault->>FS: Load JSON
FS-->>Vault: Data loaded
Vault->>Vault: Derive key, decrypt
Vault-->>Dialog: Return api_key
Dialog->>User: Accept
end
sequenceDiagram
participant Client as CodeDropClient
participant Discover as DiscoverWorker
participant Multicast as Network Multicast
participant SendW as SendWorker (TCP)
participant Server as CodeDropServer
Client->>Client: start_discovering()
Client->>Discover: spawn worker in QThread
Discover->>Multicast: bind & listen
Multicast-->>Discover: announce(host:port)
Discover->>Client: emit server_discovered(host, port)
Client->>Client: send_code_message(addr, port, filename, code)
Client->>SendW: create_send_worker(...)
SendW->>Server: TCP connect & send JSON payload
Server-->>SendW: ack/response
SendW->>Client: emit send_result
sequenceDiagram
participant App as Application
participant Config as AppConfiguration
participant Disk as ~/.{app}/config.yaml
participant Mem as In-memory config
App->>Config: AppConfiguration(app_name, default_config)
Config->>Disk: ensure dir exists
Config->>Config: load_default_config()
Config->>Config: load_configurations() (merge)
Config->>Mem: store config_data
App->>Config: get(key, default)
alt key exists
Mem-->>App: return value
else
Config->>Mem: set default
Config->>Disk: save_configurations()
Disk-->>Config: written
Config-->>App: return default
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (35)
src/napari_chatgpt/utils/strings/trailing_code.py (1)
27-27:⚠️ Potential issue | 🟡 MinorStale inline comment contradicts the code.
The comment says "Assumes standard 4-space indentation" but
is_indentedon line 23 checks for any leading space or tab — which is the correct behavior and matches the new docstring. The inline comment is misleading.Suggested fix
- if is_indented(lines[i]): # Assumes standard 4-space indentation + if is_indented(lines[i]):src/napari_chatgpt/utils/qt/one_time_disclaimer_dialog.py (1)
12-24:⚠️ Potential issue | 🟡 MinorDocstring is still NumPy-style and incomplete — convert to Google-style and document all parameters.
The PR objective is to convert NumPy-style docstrings to Google-style for consistency, but this docstring still uses
Parameters\n----------/Returns\n-------format. Additionally,message_titleandapp_nameare undocumented, and theReturnssection lacks a description.📝 Proposed fix: convert to Google-style and document all args
""" Show a one-time disclaimer dialog with a message in HTML format. - Parameters - ---------- - html_message: - The message to display in HTML format. - - Returns - ------- - int - + Args: + html_message: The message to display in HTML format. + message_title: The title of the dialog window. + app_name: Application name used for persisting acknowledgement state. + + Returns: + The QMessageBox response code (e.g. ``QMessageBox.Ok``). """src/napari_chatgpt/utils/openai/check_api_key.py (1)
7-19: 🛠️ Refactor suggestion | 🟠 MajorDocstring style inconsistency: NumPy-style should be converted to Google-style.
The PR objective states all NumPy-style docstrings are being converted to Google-style for consistency, but this function's docstring still uses NumPy format (
Parameters\n----------,Returns\n-------).♻️ Suggested conversion to Google-style
""" Check if the OpenAI API key is valid. - Parameters - ---------- - openai_api_key : str - The OpenAI API key to check. - - Returns - ------- - bool - True if the API key is valid, False otherwise. + Args: + openai_api_key: The OpenAI API key to check. + + Returns: + True if the API key is valid, False otherwise. """src/napari_chatgpt/utils/napari/layer_snapshot.py (1)
10-26:⚠️ Potential issue | 🟡 MinorNumPy-style docstring not converted to Google-style.
The PR objective states all NumPy-style docstrings should be converted to Google-style for consistency, but this function still uses the NumPy
Parameters / ----------format.Proposed conversion to Google-style
""" Capture a snapshot of the canvas of the napari viewer with only the given layer visible. - Parameters - ---------- - viewer : Viewer - The napari viewer. - layer_name : str - The name of the layer to capture the snapshot of. Can be None, in which case all visible layers are captured. - reset_view : bool - Whether to reset the view before taking the snapshot. - - Returns - ------- - Image - The snapshot of the canvas of the napari viewer with only the given layer visible. + Args: + viewer: The napari viewer. + layer_name: The name of the layer to capture the snapshot of. + Can be None, in which case all visible layers are captured. + reset_view: Whether to reset the view before taking the snapshot. + + Returns: + The snapshot of the canvas of the napari viewer with only the given + layer visible. """src/napari_chatgpt/utils/web/scrapper.py (1)
115-122:⚠️ Potential issue | 🟡 MinorInconsistent type hint:
max_text_snippetsshould beint | None.Line 118 uses
int = Nonewhile the same parameter intext_from_html(line 43) correctly usesint | None = None. This is inconsistent and technically incorrect —Noneis not anint.Proposed fix
def text_from_url( url: str, cleanup: bool = True, - max_text_snippets: int = None, + max_text_snippets: int | None = None, min_words_per_snippet: int = 5, sort_snippets_by_decreasing_size: bool = True, max_query_freq_hz: float = 100, ) -> str:src/napari_chatgpt/utils/python/exception_guard.py (1)
29-29:⚠️ Potential issue | 🟡 MinorIncorrect type annotation:
exception_valuestores an exception instance, notstr.On line 41,
self.exception_valueis assignedexc_value(aBaseExceptioninstance), but the type hint here declares it asstr.Proposed fix
- self.exception_value: str = None + self.exception_value: BaseException | None = NoneWhile at it, the other attributes initialized to
Nonewould also benefit from| Noneannotations (Python 3.10+ is supported):Full annotation cleanup
- self.exception: Exception = None - self.exception_type_name: str = None - self.exception_value: str = None + self.exception: type[BaseException] | None = None + self.exception_type_name: str | None = None + self.exception_value: BaseException | None = None self.exception_traceback = None - self.exception_description: str = None + self.exception_description: str | None = Nonesrc/napari_chatgpt/utils/configuration/app_configuration.py (1)
55-69:⚠️ Potential issue | 🟡 MinorDocstring promises
dictbutyaml.safe_loadcan returnNonefor empty files.If the YAML file exists but is empty,
yaml.safe_loadreturnsNone, not{}. This contradicts the docstring's "empty dict if the source is unavailable" claim and will also cause aTypeErrorat line 84 ({**None, ...}). A one-line guard would align the code with the docstring:Suggested fix
with open(default_config_path) as default_file: - return yaml.safe_load(default_file) + return yaml.safe_load(default_file) or {}The same issue applies to
user_configinload_configurations(line 79):- user_config = yaml.safe_load(user_file) + user_config = yaml.safe_load(user_file) or {}src/napari_chatgpt/utils/system/folders.py (1)
14-32:⚠️ Potential issue | 🟡 MinorImprove robustness and document potential exceptions.
The current implementation has a few issues:
Race condition (TOCTOU): Lines 29-30 check if the folder exists, then create it. If another process creates the folder between the check and
os.mkdir(), the function will raiseFileExistsError.Limited path handling:
os.mkdir()only creates a single directory. Iffolder_namecontains path separators (e.g.,"foo/bar"), the function will fail.Missing documentation: The docstring doesn't include a
Raisessection to document potentialOSErrorexceptions.🛡️ Proposed fix using os.makedirs
def get_or_create_folder_in_home(folder_name: str): """Return the path to ``~/{folder_name}``, creating it if necessary. Args: folder_name: Name of the folder inside the home directory. Returns: Absolute path to the folder. + + Raises: + OSError: If folder creation fails due to permissions or other I/O errors. """ home_folder = get_home_folder() # Create the path to the cache folder folder = os.path.join(home_folder, folder_name) - # Check if the cache folder exists, and create it if it doesn't - if not os.path.exists(folder): - os.mkdir(folder) + # Create the folder and any necessary parent directories + os.makedirs(folder, exist_ok=True) return foldersrc/napari_chatgpt/microplugin/code_editor/clickable_icon.py (2)
112-137:⚠️ Potential issue | 🔴 CriticalBug: Modified pixel colors are never written back to the image.
The loop computes inverted RGB values on a local
QColorbut never callsimage.setPixel(...)to persist those changes — thesetPixelcall is commented out at lines 122-134. As a result,_modify_pixmap_for_dark_uireturns the original, unmodified pixmap.🐛 Proposed fix — write the inverted color back
# Invert colors color.setRed(255 - color.red()) color.setGreen(255 - color.green()) color.setBlue(255 - color.blue()) - # # Rotate hue by 180 degrees - # if ( - # color.hue() != -1 - # ): # Check if color is not grayscale (hue is undefined for grayscale) - # hue = (color.hue() + 180) % 360 - # color.setHsv( - # hue, color.saturation(), color.value(), color.alpha() - # ) # Preserve alpha - # - # # Set the modified color back to the image - # image.setPixel( - # x, y, color.rgba() - # ) # Use rgba() to include the alpha channel + # Set the modified color back to the image + image.setPixel(x, y, color.rgba())
39-39:⚠️ Potential issue | 🟠 Major
self.sizeshadowsQWidget.size()method.
QLabelinheritsQWidget.size()which returns aQSize. Storing anintinself.sizewill break any internal Qt or external code that callsself.size(). Use a different attribute name (e.g.,self.icon_size).Proposed fix
- self.size = size + self.icon_size = sizeThen update the two references on lines 48 and 51 accordingly:
- pixmap = icon.pixmap(self.size, self.size) + pixmap = icon.pixmap(self.icon_size, self.icon_size) ... - pixmap = icon.scaled( - self.size, self.size, Qt.KeepAspectRatio, Qt.SmoothTransformation + pixmap = icon.scaled( + self.icon_size, self.icon_size, Qt.KeepAspectRatio, Qt.SmoothTransformationsrc/napari_chatgpt/microplugin/code_editor/text_input_widget.py (1)
188-188:⚠️ Potential issue | 🟡 MinorCopy-paste error in log message: should say
"on_cancel"not"on_enter".Proposed fix
- aprint(f"Error in on_enter: {e}") + aprint(f"Error in on_cancel: {e}")src/napari_chatgpt/microplugin/code_editor/python_code_editor_widget.py (1)
140-162:⚠️ Potential issue | 🟡 Minor
elif show_completions:on line 160 is unreachable (dead code).The
ifon line 140 already enters whenshow_completionsisTrue(due to theorclause). By the time execution reaches theelif,show_completionsis guaranteed to beFalse, so the branch at line 160–162 can never execute.Proposed fix — collapse the else branch or remove it
if completion_list: self.completer.setCompletionPrefix(text_under_cursor) cr = self.cursorRect() cr.setWidth( self.completer.popup().sizeHintForColumn(0) + self.completer.popup().verticalScrollBar().sizeHint().width() ) self.completer.complete(cr) # popup it up! - elif show_completions: - # Close the completer if there's no text under cursor and no force show - self.completer.popup().hide() + else: + self.completer.popup().hide()src/napari_chatgpt/utils/notebook/jupyter_notebook.py (1)
190-206:⚠️ Potential issue | 🟡 MinorType hint
Imagerefers to the PIL module, not theImage.Imageclass.The import
from PIL import ImagebindsImageto the module. The correct type for an image instance isImage.Image. The new docstring ("A PILImageinstance") reinforces this ambiguity. While it works at runtime (no enforcement), it will confuse type-checkers and readers.Two minor pre-existing nits in the same method:
- Line 203:
"PNG"(uppercase) is passed as the MIME subtype, producingdata:image/PNG;…. The canonical lowercase"png"would be more consistent withadd_image_cell(line 183).- Line 206:
self._modified = Trueis redundant —_add_imagealready sets it on line 172.Suggested diff
- def add_image_cell_from_PIL_image(self, pil_image: Image, text: str = ""): - """Add a markdown cell with an embedded image from a PIL ``Image``. + def add_image_cell_from_PIL_image(self, pil_image: Image.Image, text: str = ""): + """Add a markdown cell with an embedded image from a PIL ``Image.Image``. Args: - pil_image: A PIL ``Image`` instance. + pil_image: A PIL ``Image.Image`` instance. text: Optional text to display alongside the image. """ # Convert PIL image to base64 buffered = BytesIO() pil_image.save(buffered, format="PNG") base64_string = b64encode(buffered.getvalue()).decode() # Use the existing method to add the image with text - self._add_image(base64_string, "PNG", text) - - # Mark as modified: - self._modified = True + self._add_image(base64_string, "png", text)src/napari_chatgpt/omega_agent/tools/special/human_input_tool.py (1)
50-53:⚠️ Potential issue | 🟠 Major
pydantic.Field()misused in__init__—prompt_funcandinput_funcwill beFieldInfoobjects, not callables, causing runtime failure.
Field(default_factory=...)is designed for class-level declarations in Pydantic models and returns aFieldInfodescriptor. Using it as a direct instance attribute assignment in__init__meansself.prompt_funcbecomes aFieldInfoobject, not the callable. This causesself.prompt_func(query)on line 68 to raiseTypeErrorat runtime.The new docstrings (lines 32–33) explicitly document these as
Callable, making this bug more apparent. Fix by assigning the callables directly:Proposed fix
- self.prompt_func: Callable[[str], None] = Field( - default_factory=lambda: _print_func - ) - self.input_func: Callable = Field(default_factory=lambda: input) + self.prompt_func: Callable[[str], None] = _print_func + self.input_func: Callable = inputsrc/napari_chatgpt/utils/llm/vision.py (1)
62-83:⚠️ Potential issue | 🟡 MinorDocstring parameters don't match the function signature.
max_tokensis documented (line 73–74) but does not exist in the signature.apiis in the signature but missing from the docstring.Also, both docstrings in this file use NumPy-style formatting while the rest of this PR adopts Google-style. Consider converting for consistency.
Proposed fix (Google-style, matching signature)
""" - Describe an image using GPT-vision. - - Parameters - ---------- - image_path: str - Path to the image to describe - query : str - Query to send to GPT - model_name : str - Model to use - max_tokens : int - Maximum number of tokens to use - number_of_tries : int - Number of times to try to send the request to GPT. - - Returns - ------- - str - Description of the image - + Describe an image using a vision-capable LLM. + + Args: + image_path: Path to the image to describe. + query: Query to send to the model. + api: Optional API instance. If None, uses CombinedApi. + model_name: Model to use. If None or lacking vision support, + the best available vision model is selected. + number_of_tries: Number of times to retry the request. + + Returns: + Description of the image, or an error message on failure. """src/napari_chatgpt/microplugin/network/receive_worker.py (1)
110-116:⚠️ Potential issue | 🟡 MinorPre-existing:
server_socketmay be unbound in thefinallyblock.If
socket.socket(...)on line 62 raises, the outerfinallyon line 115 will attemptserver_socket.close()but the variable won't be defined, causing aNameErrorthat masks the original exception. Not introduced by this PR, but worth noting since the docstrings now describe this lifecycle.Suggested defensive fix
try: aprint(f"Listening for messages on port: {self.port}") # Create a socket to listen for incoming connections: server_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + server_socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) ... except Exception as e: import traceback traceback.print_exc() self.error.emit(e) finally: - server_socket.close() + try: + server_socket.close() + except NameError: + passsrc/napari_chatgpt/omega_agent/tools/special/exception_catcher_tool.py (1)
112-121:⚠️ Potential issue | 🟡 MinorMinor TOCTOU between
qsize()andget_nowait().In a multi-threaded environment,
exception_queue.qsize()can become stale by the timeget_nowait()executes, leading to a potentialqueue.Emptyexception. Since this is a diagnostic tool the impact is low, but wrapping in a try/except would be more robust.Proposed defensive fix
while exception_queue.qsize() > 0 and number_of_exceptions > 0: - value = exception_queue.get_nowait() + try: + value = exception_queue.get_nowait() + except queue.Empty: + break description = exception_description(value)src/napari_chatgpt/_widget.py (3)
140-141:⚠️ Potential issue | 🟡 Minor
self.model_labelis overwritten by_tool_model_selection.Line 140 assigns
self.model_labelfor the main model, then line 168 reassigns it for the tool model. The first label widget survives in the layout but the instance attribute is shadowed. Use distinct names (e.g.self.main_model_labelandself.tool_model_label).Also applies to: 167-168
258-258:⚠️ Potential issue | 🟡 MinorTypo: "Chose" → "Choose".
Proposed fix
- self.creativity_label = QLabel("Chose the level of creativity:") + self.creativity_label = QLabel("Choose the level of creativity:")
529-534:⚠️ Potential issue | 🟡 MinorCopy-paste error: wrong error message in
_show_editor.The message says "Omega failed to start" but this method is about showing the code editor.
Proposed fix
aprint(f"Error: {e}") aprint( - "Omega failed to start. Please check the console for more information." + "Failed to show editor. Please check the console for more information." )src/napari_chatgpt/omega_agent/tools/napari/napari_plugin_tool.py (1)
66-70:⚠️ Potential issue | 🟠 MajorInfo-mode regex may over-match, intercepting execution queries.
The pattern
\b(list|what|available|show|which\s+plugin|discover|catalog)\bwill match common words like "show" and "what" that are likely to appear in execution queries too (e.g., "show me a denoised version of this image using plugin X"). This would route the query to info mode instead of code generation.Consider either requiring multiple keyword matches, using a more specific pattern, or checking that the query primarily asks about plugins (e.g., matching "list plugins" or "what plugins" as multi-word phrases).
💡 One possible tightening
_INFO_KEYWORDS = re.compile( - r"\b(list|what|available|show|which\s+plugin|discover|catalog)\b", + r"\b(list\s+(available\s+)?plugins?|what\s+plugins?|available\s+plugins?|show\s+(available\s+)?plugins?|which\s+plugins?|discover\s+plugins?|plugin\s+catalog)\b", re.IGNORECASE, )Also applies to: 433-437
src/napari_chatgpt/chat_server/chat_server.py (1)
541-548:⚠️ Potential issue | 🟠 MajorBusy-wait loop has no timeout — server startup failure causes an infinite loop.
If
_start_uvicorn_serverfails before settingself.running = True, this loop will spin forever. Consider adding a timeout.🐛 Proposed fix
# Wait for the server to start: aprint("Waiting for chat server to start...") - while not chat_server.running: + timeout = 30 # seconds + elapsed = 0.0 + while not chat_server.running: + if elapsed >= timeout: + raise RuntimeError( + f"Chat server failed to start within {timeout}s" + ) sleep(0.1) + elapsed += 0.1src/napari_chatgpt/llm/api_keys/demo/api_key_vault_dialog_demo.py (1)
29-31:⚠️ Potential issue | 🟡 MinorAPI key printed to stdout in demo script.
Line 31 prints the raw API key. Even though this is a demo, it could end up in logs or terminal history. Consider masking or redacting part of the key (e.g., showing only the last 4 characters).
src/napari_chatgpt/omega_agent/tools/napari/delegated_code/cellpose.py (1)
80-127:⚠️ Potential issue | 🟡 Minor
labelsmay be unbound ifimage.shapehas length 1.The dimension check on line 81 only rejects
len(image.shape) > 3. If a 1D array is passed, neither the== 2nor== 3branch executes, andlabelsis referenced unbound on line 126. Pre-existing issue, but a simpleelse: raisewould close the gap.Proposed fix
elif len(image.shape) == 3: ... + else: + raise ValueError("The input image must be 2D or 3D.")src/napari_chatgpt/microplugin/network/demo/code_drop_client_demo.py (1)
31-33:⚠️ Potential issue | 🟡 MinorDead code:
print(client)on line 33 is unreachable aftersys.exit().This statement will never execute. Consider removing it.
src/napari_chatgpt/microplugin/network/discover_worker.py (3)
72-107:⚠️ Potential issue | 🟠 MajorPotential
TypeError/UnboundLocalErrorif no multicast group binds successfully.If every
bind()in the loop at Lines 75–103 raisesOSError, thenavailable_multicast_groupremainsNoneandbroadcast_listening_socketmay reference only the last failed socket. Line 107 will crash onsocket.inet_aton(None), and thefinallyblock at Line 168 may hit anUnboundLocalErrorif the variable was never assigned successfully.Consider adding a guard after the loop:
Proposed fix
aprint( f"Most likely the multicast group is already in use by another instance of Omega! Only affects sending of code snippets." ) + if available_multicast_group is None: + aprint("Could not bind to any multicast group.") + self.finished.emit() + return + # Tell the kernel to add the multicast group to the multicast group: mreq = struct.pack(
115-116:⚠️ Potential issue | 🟡 MinorStale inline comment: says "5 seconds" but timeout is 1.0 second.
Since this PR focuses on improving documentation, this stale comment is worth fixing too.
Proposed fix
- # Set a timeout of 5 seconds + # Set a timeout of 1 second broadcast_listening_socket.settimeout(1.0)
130-130:⚠️ Potential issue | 🟡 MinorStale inline comment: says
'hostname:port'but actual format is'username:hostname:port'.The code on Line 132 correctly splits into three parts (
user_name, server_name, server_port), but the comment is outdated.Proposed fix
- # Directly use the received data, assuming it's in 'hostname:port' format + # Directly use the received data, assuming it's in 'username:hostname:port' formatsrc/napari_chatgpt/omega_agent/tools/search/web_search_tool.py (1)
38-43:⚠️ Potential issue | 🟡 MinorMissing space between sentences in the description string.
The string on line 41 ends with
'...about this question.'and line 42 begins with'Use it sparingly...', producing"...about this question.Use it sparingly..."at runtime due to implicit string concatenation without a trailing space or leading space.🐛 Proposed fix
"For example, if the input is: 'What year is it?', the tool will return information about this question." - "Use it sparingly and refrain from using it if you already know the answer!" + " Use it sparingly and refrain from using it if you already know the answer!"src/napari_chatgpt/omega_agent/tools/napari/cell_nuclei_segmentation_tool.py (1)
327-348:⚠️ Potential issue | 🟡 Minor
stardist_package_massagingreturnsNoneon non-macOS platforms.The
@cachedecorator is appropriate for a one-shot helper. However, on non-Darwin platforms, the function falls through without an explicitreturn, yieldingNone. The docstring states it returns "Pip install output messages concatenated together", which is inaccurate for this path. Either add an explicitreturn ""for non-Darwin or document theNonereturn.🐛 Proposed fix
`@cache` def stardist_package_massaging() -> str: """Install StarDist and TensorFlow dependencies on macOS (Apple Silicon). This is a cached one-shot helper that handles the special pip install sequence required for StarDist on Apple M-series chips. Returns: - Pip install output messages concatenated together. + Pip install output messages concatenated together, or an empty + string on non-macOS platforms. """ message = "" if sys.platform == "darwin": with asection(f"Stardist requires special treatment on M1"): # purge_tensorflow() # conda_uninstall(['numpy', 'stardist']) # pip_uninstall(['numpy', 'stardist']) # conda_install(['tensorflow-deps'], channel='apple') message += pip_install(["tensorflow-macos", "tensorflow-metal"]) message += pip_install(["stardist"]) # and 'arm64' in sys.version.lower() return message + + return messagesrc/napari_chatgpt/microplugin/code_editor/code_drop_send_widget.py (2)
24-52:⚠️ Potential issue | 🟠 Major
get_code_callableis not initialized in__init__, riskingAttributeError.
send_code(line 152) callsself.get_code_callable(), but this attribute is only assigned later inshow_send_dialog(line 212). Ifsend_codeis ever invoked beforeshow_send_dialog(e.g., programmatically or via a stale signal), anAttributeErrorwill be raised.🐛 Proposed fix — initialize in __init__
# field for timer: self.timer = None + + # Callable to retrieve (filename, code); set by show_send_dialog: + self.get_code_callable = NoneYou'd also want a guard in
send_code:if self.get_code_callable is None: aprint("No code callable set.") return
182-194:⚠️ Potential issue | 🟡 MinorPotential timer leak if
start_server_list_refreshis called twice.If
start_server_list_refreshis called whileself.timeralready exists (e.g.,show_send_dialogcalled twice withoutcanceledin between), line 189 creates a newQTimerwithout stopping/deleting the existing one, leaking it.🛡️ Proposed guard
def start_server_list_refresh(self): """Start a periodic timer to refresh the discovered server list.""" + # Stop any existing timer first: + self.stop_server_list_refresh() + # Initial refresh: self.update_server_list()src/napari_chatgpt/llm/api_keys/api_key_vault_dialog.py (1)
88-93:⚠️ Potential issue | 🟡 MinorTypo in tooltip text: missing "to".
Line 92:
"to give Omega access your API key."should be"to give Omega access to your API key.".📝 Proposed fix
self.password_textbox.setToolTip( "Enter a password to secure your API key.\n" "You will need to remember this password\n" - "to give Omega access your API key." + "to give Omega access to your API key." )src/napari_chatgpt/microplugin/network/code_drop_client.py (2)
160-195:⚠️ Potential issue | 🔴 CriticalDeadlock:
send_message_by_addressholdssending_lockwhile waiting for the send thread, which also needssending_lock.
send_message_by_address(line 161) acquiresself.sending_lockand then at line 164-166 waits for the runningsend_threadto finish. However,SendWorker.send(line 216) also acquiresparent_self.sending_lock. The send thread cannot proceed because the calling thread holds the lock, and the calling thread cannot proceed because it's waiting for the send thread — classic deadlock.🔒 Proposed fix — don't hold the lock while waiting for the thread
def send_message_by_address( self, server_address: str, server_port: int, message: str ): - - with self.sending_lock: - # Check if there's already a thread running for sending messages: - max_number_of_attempts: int = 10 - while self.send_thread is not None and self.send_thread.isRunning(): - self.send_thread.quit() - self.send_thread.wait() - - aprint("A send thread is already running. Wait for it to finish.") - sleep(0.1) - max_number_of_attempts -= 1 - - # If the thread is taking too long, stop waiting and don't send the message: - if max_number_of_attempts == 0: - aprint("Max number of attempts reached. Can't send message.") - return - - # Create a QThread each time for sending messages - self.send_thread = QThread() + # Wait for any in-progress send to finish WITHOUT holding the lock, + # so the worker can release the lock and complete. + max_number_of_attempts: int = 10 + while self.send_thread is not None and self.send_thread.isRunning(): + self.send_thread.quit() + self.send_thread.wait() + + aprint("A send thread is already running. Wait for it to finish.") + sleep(0.1) + max_number_of_attempts -= 1 + + if max_number_of_attempts == 0: + aprint("Max number of attempts reached. Can't send message.") + return + + with self.sending_lock: + self.send_thread = QThread()Also applies to: 214-234
217-233:⚠️ Potential issue | 🟡 Minor
sock.close()infinallymay raiseNameErrorifsocket.socket()itself fails.If
socket.socket()throws before assigning tosock, thefinallyblock at line 232 will reference an undefined variable. Use atry/finallypattern that initializessockbefore thetryblock, or guard the close.🛡️ Proposed fix
`@Slot`() def send(self): with parent_self.sending_lock: + sock = None try: sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) sock.connect((server_address, server_port)) sock.sendall(message.encode()) aprint( f"Message of length: {len(message)} sent to {server_address}:{server_port}" ) - except Exception as e: aprint(f"Error sending message: {e}") import traceback - traceback.print_exc() parent_self.handle_error(e) finally: - sock.close() + if sock is not None: + sock.close() self.finished.emit() parent_self.send_worker = None
🤖 Fix all issues with AI agents
In `@src/napari_chatgpt/chat_server/chat_server.py`:
- Around line 234-238: The notebook is being given result (a list[Message])
directly, producing a Python repr; convert the message list into a single
markdown string before calling self.notebook.add_markdown_cell. Locate where
result is created/returned (see send_final_response_to_user and the variable
result) and transform it into text the same way send_final_response_to_user does
for user output — e.g., extract each Message's text/content and join them (or
call the existing utility that renders Message -> string) and then pass that
string to self.notebook.add_markdown_cell(f"### Omega:\n {rendered_text}").
In `@src/napari_chatgpt/microplugin/code_editor/python_code_completer.py`:
- Around line 1-2: Remove the dead file python_code_completer.py because the
PythonCodeCompleter class it defines is unused; confirm that no other modules
import PythonCodeCompleter, then delete the file and any associated tests or
references; note that PythonCodeEditor in python_code_editor_widget.py already
provides an inline QCompleter using jedi.Script, so leave that implementation
intact and run project tests/linters to ensure nothing else referenced
PythonCodeCompleter.
In `@src/napari_chatgpt/omega_agent/tools/napari/file_open_tool.py`:
- Around line 53-67: The docstring promises a complete summary but currently
treats falsy returns from open_in_napari as neither success nor error; update
the loop in the open_files tool (where open_in_napari is called) to treat a
falsy result as a failure by appending an entry to encountered_errors (include
the filename and a clear message like "open_in_napari returned falsy/no layers")
and only append to opened_files when the call returns truthy; keep the existing
try/except for exceptions but ensure the non-exception falsy-path is recorded so
the final summary accurately reflects all files.
In `@src/napari_chatgpt/omega_agent/tools/search/web_image_search_tool.py`:
- Line 55: The attribute self.prompt is annotated as str but set to None; change
its type to allow None (e.g., use `str | None` for Python 3.10+ or
`Optional[str]` with `from typing import Optional`) and update the assignment to
`self.prompt: str | None = None` (or `self.prompt: Optional[str] = None`) inside
the class initializer in web_image_search_tool.py; also add the necessary import
(`Optional`) if you choose that form.
In `@src/napari_chatgpt/omega_agent/tools/special/exception_catcher_tool.py`:
- Around line 24-34: The custom sys.excepthook in _uncaught_exception_handler
currently calls traceback.print_exc(), which doesn't print the exception there
and leaves exctype unused; replace the final call with
traceback.print_exception(exctype, value, _traceback) so the hook prints the
correct traceback and uses the exctype argument (keeping the existing
enqueue_exception(value) and aprint(value) calls intact).
In `@src/napari_chatgpt/utils/configuration/app_configuration.py`:
- Line 1: The module docstring overstates thread-safety: only __new__ is
protected by _lock while get, __setitem__, save_configurations, and
load_configurations access shared config_data and the YAML file without
synchronization; either wrap these methods' critical sections with the same
_lock (acquire _lock in get, __setitem__, save_configurations,
load_configurations when reading/writing config_data or the file) or modify the
module docstring to state that only singleton creation is thread-safe and
callers must synchronize concurrent config access; reference __new__, _lock,
get, __setitem__, save_configurations, load_configurations, and config_data when
making the change.
In `@src/napari_chatgpt/utils/download/download_files.py`:
- Around line 73-82: The return type annotation for download_file_stealth is
incorrect because the function can return None on failure; update the signature
from -> str to -> Optional[str] or -> str | None and add the necessary typing
import (Optional or use PEP604 union) so static checkers match runtime behavior;
ensure any related docstring or callers that rely on the annotation are
consistent with the nullable return.
In `@src/napari_chatgpt/utils/openai/default_model.py`:
- Around line 12-19: The function get_default_openai_model_name currently reads
_default_openai_model_name but never assigns to it and is annotated as -> str;
add a global declaration for _default_openai_model_name at the top of
get_default_openai_model_name, assign the fetched model string to
_default_openai_model_name before returning so subsequent calls use the cached
value, and update the function signature return type to -> str | None to reflect
that it may return None.
In `@src/napari_chatgpt/utils/python/check_code_safety.py`:
- Around line 46-61: The function in
src/napari_chatgpt/utils/python/check_code_safety.py whose signature currently
ends with "-> str:" returns a two-element tuple (explanation, rank); update its
return type annotation to reflect that (use either tuple[str, str] or
typing.Tuple[str, str]) and add the corresponding import (from typing import
Tuple) if you choose Tuple; ensure the annotated return type matches all return
sites that produce (explanation, rank).
In `@src/napari_chatgpt/utils/python/conda_utils.py`:
- Around line 1-5: The module docstring claims "duplicate filtering" but only
conda_install currently deduplicates packages; update conda_uninstall to mirror
conda_install by forcing list_of_packages to be a list and deduplicating (e.g.,
replace incoming list_of_packages with list(set(list_of_packages))) before
processing, and preserve the existing error_occurred flow in conda_uninstall so
behavior remains consistent with conda_install.
In `@src/napari_chatgpt/utils/python/dynamic_import.py`:
- Around line 18-29: The docstring for the module-loading function (the function
that writes code to a temp file and loads it using importlib, e.g.,
load_module_from_code) incorrectly states it returns None on failure; update the
Returns section to state the function returns the loaded module and that it will
raise the underlying exception on failure (do not claim it returns None), and
optionally remove the `| None` from the function's return type hint so the
signature matches the documented behavior.
In `@src/napari_chatgpt/utils/python/exception_guard.py`:
- Line 25: The attribute self.allow_print set in ExceptionGuard.__init__ is
never used; update ExceptionGuard.__exit__ to honor this flag (e.g., only emit
stacktraces or prints when both self.print_stacktrace and self.allow_print are
true) so allow_print actually gates output (or if the feature is unwanted,
remove the allow_print parameter/assignment from __init__ and related docs).
Also update the __init__ docstring/parameter list to reflect the chosen
behavior.
In `@src/napari_chatgpt/utils/python/pip_utils.py`:
- Around line 209-216: The log message in the pip_uninstall function/docstring
mismatch: update the pre-existing log call that currently says "Installing up to
{len(list_of_packages)} packages with pip:" to accurately reflect uninstallation
(e.g., "Uninstalling up to {len(list_of_packages)} packages with pip:"), keeping
the same interpolation and surrounding logic in the pip_uninstall function so
the message matches the docstring and behavior.
In `@src/napari_chatgpt/utils/strings/find_integer_in_parenthesis.py`:
- Around line 4-17: The function find_integer_in_parenthesis has a docstring
saying it may return None but its signature declares -> tuple[str, int]; update
the return type to reflect the None path by changing the annotation to
Optional[tuple[str, int]] and add the necessary import (from typing import
Optional) or use typing.Optional; ensure the function signature and docstring
are consistent (i.e., -> Optional[tuple[str, int]] and docstring describing the
None return).
| if self.notebook: | ||
| # Add agent response to notebook: | ||
| self.notebook.add_markdown_cell(f"### Omega:\n {result}") | ||
| self.notebook.add_markdown_cell( | ||
| f"### Omega:\n {result}" | ||
| ) |
There was a problem hiding this comment.
Bug: result is list[Message], not a string — notebook cell will contain an unhelpful repr.
result is typed as list[Message] (see send_final_response_to_user signature on line 283). Writing it directly into an f-string produces the list's __repr__, not readable markdown. It should be converted to text first, consistent with how send_final_response_to_user handles it a few lines above.
🐛 Proposed fix
if self.notebook:
# Add agent response to notebook:
+ text_result = [m for m in result if m.has(Text)]
+ message_str = "\n\n".join(
+ [m.to_plain_text() for m in text_result]
+ )
self.notebook.add_markdown_cell(
- f"### Omega:\n {result}"
+ f"### Omega:\n {message_str}"
)🤖 Prompt for AI Agents
In `@src/napari_chatgpt/chat_server/chat_server.py` around lines 234 - 238, The
notebook is being given result (a list[Message]) directly, producing a Python
repr; convert the message list into a single markdown string before calling
self.notebook.add_markdown_cell. Locate where result is created/returned (see
send_final_response_to_user and the variable result) and transform it into text
the same way send_final_response_to_user does for user output — e.g., extract
each Message's text/content and join them (or call the existing utility that
renders Message -> string) and then pass that string to
self.notebook.add_markdown_cell(f"### Omega:\n {rendered_text}").
| """Jedi-based Python code completer for QPlainTextEdit widgets.""" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if PythonCodeCompleter is imported/used anywhere besides its own file
rg -n "PythonCodeCompleter" --type=py -g '!**/python_code_completer.py'Repository: royerlab/napari-chatgpt
Length of output: 49
🏁 Script executed:
# Also check for imports of this module
rg -n "from.*python_code_completer\|import.*python_code_completer" --type=pyRepository: royerlab/napari-chatgpt
Length of output: 49
🏁 Script executed:
# Read the full file to see the code at all mentioned lines
cat -n src/napari_chatgpt/microplugin/code_editor/python_code_completer.pyRepository: royerlab/napari-chatgpt
Length of output: 1805
🏁 Script executed:
# Check python_code_editor_widget.py to see how it handles completion
cat -n src/napari_chatgpt/microplugin/code_editor/python_code_editor_widget.py | head -100Repository: royerlab/napari-chatgpt
Length of output: 4530
Remove unused PythonCodeCompleter class — it is not imported or used anywhere in the codebase.
The class is dead code. PythonCodeEditor in python_code_editor_widget.py implements its own inline QCompleter with jedi.Script and never instantiates PythonCodeCompleter. Since this module provides no value to the project, remove the entire file python_code_completer.py.
🤖 Prompt for AI Agents
In `@src/napari_chatgpt/microplugin/code_editor/python_code_completer.py` around
lines 1 - 2, Remove the dead file python_code_completer.py because the
PythonCodeCompleter class it defines is unused; confirm that no other modules
import PythonCodeCompleter, then delete the file and any associated tests or
references; note that PythonCodeEditor in python_code_editor_widget.py already
provides an inline QCompleter using jedi.Script, so leave that implementation
intact and run project tests/linters to ensure nothing else referenced
PythonCodeCompleter.
| """Open one or more image files in the napari viewer. | ||
|
|
||
| Parses the newline-delimited query for file paths and optional reader | ||
| plugin names, then attempts to open each file. Returns a summary | ||
| indicating which files were opened successfully and which failed. | ||
|
|
||
| Args: | ||
| query: Newline-delimited list of file paths / URLs (with optional | ||
| ``[plugin_name]`` suffixes). | ||
| code: Unused (no LLM code generation for this tool). | ||
| viewer: The active napari viewer instance. | ||
|
|
||
| Returns: | ||
| A summary message listing opened files and any errors encountered. | ||
| """ |
There was a problem hiding this comment.
Docstring is accurate, but note a pre-existing silent-failure gap.
The docstring promises a summary of "which files were opened successfully and which failed." However, if open_in_napari returns a falsy value without raising an exception (line 100-104), the file lands in neither opened_files nor encountered_errors, making the summary incomplete — the final condition on line 121 could report "all files opened" even when some silently failed.
This is pre-existing logic, not introduced by this PR, but worth being aware of since the new docstring codifies the expected contract.
Also applies to: 100-109
🤖 Prompt for AI Agents
In `@src/napari_chatgpt/omega_agent/tools/napari/file_open_tool.py` around lines
53 - 67, The docstring promises a complete summary but currently treats falsy
returns from open_in_napari as neither success nor error; update the loop in the
open_files tool (where open_in_napari is called) to treat a falsy result as a
failure by appending an entry to encountered_errors (include the filename and a
clear message like "open_in_napari returned falsy/no layers") and only append to
opened_files when the call returns truthy; keep the existing try/except for
exceptions but ensure the non-exception falsy-path is recorded so the final
summary accurately reflects all files.
| @@ -43,20 +55,22 @@ def __init__(self, **kwargs): | |||
| self.prompt: str = None | |||
There was a problem hiding this comment.
Type annotation mismatch: str but assigned None.
self.prompt is annotated as str but initialized to None. For Python 3.10+, this should be str | None.
Proposed fix
- self.prompt: str = None
+ self.prompt: str | None = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.prompt: str = None | |
| self.prompt: str | None = None |
🤖 Prompt for AI Agents
In `@src/napari_chatgpt/omega_agent/tools/search/web_image_search_tool.py` at line
55, The attribute self.prompt is annotated as str but set to None; change its
type to allow None (e.g., use `str | None` for Python 3.10+ or `Optional[str]`
with `from typing import Optional`) and update the assignment to `self.prompt:
str | None = None` (or `self.prompt: Optional[str] = None`) inside the class
initializer in web_image_search_tool.py; also add the necessary import
(`Optional`) if you choose that form.
| def _uncaught_exception_handler(exctype, value, _traceback): | ||
| # Store the exception information in the queue | ||
| """Custom ``sys.excepthook`` that enqueues exceptions for later retrieval. | ||
|
|
||
| Args: | ||
| exctype: The exception class. | ||
| value: The exception instance. | ||
| _traceback: The traceback object. | ||
| """ | ||
| enqueue_exception(value) | ||
| aprint(value) | ||
| traceback.print_exc() |
There was a problem hiding this comment.
traceback.print_exc() does not print the uncaught exception's traceback inside sys.excepthook.
traceback.print_exc() prints the traceback from sys.exc_info(), which is empty inside the excepthook (there's no active exception being handled). Use traceback.print_exception with the hook's arguments instead. This also resolves the Ruff ARG001 warning about exctype being unused.
🐛 Proposed fix
def _uncaught_exception_handler(exctype, value, _traceback):
"""Custom ``sys.excepthook`` that enqueues exceptions for later retrieval.
Args:
exctype: The exception class.
value: The exception instance.
_traceback: The traceback object.
"""
enqueue_exception(value)
aprint(value)
- traceback.print_exc()
+ traceback.print_exception(exctype, value, _traceback)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _uncaught_exception_handler(exctype, value, _traceback): | |
| # Store the exception information in the queue | |
| """Custom ``sys.excepthook`` that enqueues exceptions for later retrieval. | |
| Args: | |
| exctype: The exception class. | |
| value: The exception instance. | |
| _traceback: The traceback object. | |
| """ | |
| enqueue_exception(value) | |
| aprint(value) | |
| traceback.print_exc() | |
| def _uncaught_exception_handler(exctype, value, _traceback): | |
| """Custom ``sys.excepthook`` that enqueues exceptions for later retrieval. | |
| Args: | |
| exctype: The exception class. | |
| value: The exception instance. | |
| _traceback: The traceback object. | |
| """ | |
| enqueue_exception(value) | |
| aprint(value) | |
| traceback.print_exception(exctype, value, _traceback) |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 24-24: Unused function argument: exctype
(ARG001)
🤖 Prompt for AI Agents
In `@src/napari_chatgpt/omega_agent/tools/special/exception_catcher_tool.py`
around lines 24 - 34, The custom sys.excepthook in _uncaught_exception_handler
currently calls traceback.print_exc(), which doesn't print the exception there
and leaves exctype unused; replace the final call with
traceback.print_exception(exctype, value, _traceback) so the hook prints the
correct traceback and uses the exctype argument (keeping the existing
enqueue_exception(value) and aprint(value) calls intact).
| """Conda package management utilities. | ||
|
|
||
| Provides functions to install and uninstall Python packages using conda, | ||
| with duplicate filtering and pre-installation checks. | ||
| """ |
There was a problem hiding this comment.
Module docstring claims "duplicate filtering" but only conda_install implements it.
The module docstring states that functions provide "duplicate filtering," but only conda_install (line 25) removes duplicates via list(set(list_of_packages)). The conda_uninstall function does not filter duplicates, making the module-level claim slightly misleading.
Suggested fixes
Option 1: Clarify the module docstring to note that only installation filters duplicates:
-Provides functions to install and uninstall Python packages using conda,
-with duplicate filtering and pre-installation checks.
+Provides functions to install and uninstall Python packages using conda.
+Installation includes duplicate filtering and pre-installation checks.Option 2: Add duplicate filtering to conda_uninstall for consistency:
# In conda_uninstall at line 70
def conda_uninstall(list_of_packages: list[str]) -> bool:
"""..."""
# Ensure it is a list and remove duplicates:
list_of_packages = list(set(list_of_packages))
error_occurred = False
# ... rest of function📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Conda package management utilities. | |
| Provides functions to install and uninstall Python packages using conda, | |
| with duplicate filtering and pre-installation checks. | |
| """ | |
| """Conda package management utilities. | |
| Provides functions to install and uninstall Python packages using conda. | |
| Installation includes duplicate filtering and pre-installation checks. | |
| """ |
🤖 Prompt for AI Agents
In `@src/napari_chatgpt/utils/python/conda_utils.py` around lines 1 - 5, The
module docstring claims "duplicate filtering" but only conda_install currently
deduplicates packages; update conda_uninstall to mirror conda_install by forcing
list_of_packages to be a list and deduplicating (e.g., replace incoming
list_of_packages with list(set(list_of_packages))) before processing, and
preserve the existing error_occurred flow in conda_uninstall so behavior remains
consistent with conda_install.
| """Dynamically load a Python code string as an importable module. | ||
|
|
||
| Writes the code to a temporary file and loads it as a Python module | ||
| using ``importlib``. | ||
|
|
||
| Args: | ||
| module_code: Python source code to load as a module. | ||
| name: Optional module name. If None, a random name is generated. | ||
|
|
||
| Returns: | ||
| The loaded module object, or None if loading fails. | ||
| """ |
There was a problem hiding this comment.
Docstring inaccurately claims None is returned on failure.
The function has no try/except; if loading fails it will raise, never return None. The Returns section should reflect the actual behavior. The existing | None in the type hint (line 17) is similarly misleading but is pre-existing.
📝 Suggested docstring fix
Returns:
- The loaded module object, or None if loading fails.
+ The loaded module object.
+
+ Raises:
+ AttributeError: If the module spec cannot be created.
+ Exception: If the module code fails to compile or execute.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Dynamically load a Python code string as an importable module. | |
| Writes the code to a temporary file and loads it as a Python module | |
| using ``importlib``. | |
| Args: | |
| module_code: Python source code to load as a module. | |
| name: Optional module name. If None, a random name is generated. | |
| Returns: | |
| The loaded module object, or None if loading fails. | |
| """ | |
| """Dynamically load a Python code string as an importable module. | |
| Writes the code to a temporary file and loads it as a Python module | |
| using ``importlib``. | |
| Args: | |
| module_code: Python source code to load as a module. | |
| name: Optional module name. If None, a random name is generated. | |
| Returns: | |
| The loaded module object. | |
| Raises: | |
| AttributeError: If the module spec cannot be created. | |
| Exception: If the module code fails to compile or execute. | |
| """ |
🤖 Prompt for AI Agents
In `@src/napari_chatgpt/utils/python/dynamic_import.py` around lines 18 - 29, The
docstring for the module-loading function (the function that writes code to a
temp file and loads it using importlib, e.g., load_module_from_code) incorrectly
states it returns None on failure; update the Returns section to state the
function returns the loaded module and that it will raise the underlying
exception on failure (do not claim it returns None), and optionally remove the
`| None` from the function's return type hint so the signature matches the
documented behavior.
| """ | ||
|
|
||
| def __init__(self, allow_print=False, print_stacktrace=True): | ||
| self.allow_print = allow_print |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the structure of the exception_guard.py file
cat -n src/napari_chatgpt/utils/python/exception_guard.pyRepository: royerlab/napari-chatgpt
Length of output: 2161
🏁 Script executed:
#!/bin/bash
# Check if allow_print is referenced anywhere in the codebase
rg -n 'allow_print' --type=pyRepository: royerlab/napari-chatgpt
Length of output: 279
allow_print is assigned but never read.
self.allow_print is set in __init__ (line 25) but no code path in the class or codebase references it. This is dead code or an incomplete feature—perhaps __exit__ should gate its output on allow_print rather than, or in addition to, print_stacktrace.
🤖 Prompt for AI Agents
In `@src/napari_chatgpt/utils/python/exception_guard.py` at line 25, The attribute
self.allow_print set in ExceptionGuard.__init__ is never used; update
ExceptionGuard.__exit__ to honor this flag (e.g., only emit stacktraces or
prints when both self.print_stacktrace and self.allow_print are true) so
allow_print actually gates output (or if the feature is unwanted, remove the
allow_print parameter/assignment from __init__ and related docs). Also update
the __init__ docstring/parameter list to reflect the chosen behavior.
| """Uninstall packages using pip, skipping packages not installed. | ||
|
|
||
| Args: | ||
| list_of_packages: Package names to uninstall. | ||
|
|
||
| Returns: | ||
| True if all uninstallations succeeded, False if any errors occurred. | ||
| """ |
There was a problem hiding this comment.
Docstring says "Uninstall" but the log message on Line 222 says "Installing".
The new docstring correctly describes uninstallation, but the pre-existing log message at Line 222 reads "Installing up to {len(list_of_packages)} packages with pip:" — a copy-paste leftover from pip_install. Consider fixing it while you're touching this function.
Proposed fix
- with asection(f"Installing up to {len(list_of_packages)} packages with pip:"):
+ with asection(f"Uninstalling up to {len(list_of_packages)} packages with pip:"):🤖 Prompt for AI Agents
In `@src/napari_chatgpt/utils/python/pip_utils.py` around lines 209 - 216, The log
message in the pip_uninstall function/docstring mismatch: update the
pre-existing log call that currently says "Installing up to
{len(list_of_packages)} packages with pip:" to accurately reflect uninstallation
(e.g., "Uninstalling up to {len(list_of_packages)} packages with pip:"), keeping
the same interpolation and surrounding logic in the pip_uninstall function so
the message matches the docstring and behavior.
| def find_integer_in_parenthesis(string: str) -> tuple[str, int]: | ||
| """Finds an integer surrounded by parenthesis in a string and returns that integer and the text without the number in parenthesis. | ||
| """Find an integer in parentheses and return it with the cleaned text. | ||
|
|
||
| Locates the first parenthesized integer in the string, extracts it, | ||
| and returns both the integer and the string with the parenthesized | ||
| portion removed. | ||
|
|
||
| Args: | ||
| string: The string to search. | ||
| string: The string to search for a parenthesized integer. | ||
|
|
||
| Returns: | ||
| A tuple containing the integer and the text without the number in parenthesis. | ||
| A tuple of (cleaned_text, integer) where cleaned_text has the | ||
| parenthesized number removed, or None if no parenthesized | ||
| integer is found. |
There was a problem hiding this comment.
Return type annotation doesn't reflect the None return paths documented in the new docstring.
The docstring (line 16) correctly states the function can return None, but the signature on line 4 declares -> tuple[str, int]. Since this PR is specifically about improving documentation accuracy, the type hint should match:
Proposed fix
-def find_integer_in_parenthesis(string: str) -> tuple[str, int]:
+def find_integer_in_parenthesis(string: str) -> tuple[str, int] | None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def find_integer_in_parenthesis(string: str) -> tuple[str, int]: | |
| """Finds an integer surrounded by parenthesis in a string and returns that integer and the text without the number in parenthesis. | |
| """Find an integer in parentheses and return it with the cleaned text. | |
| Locates the first parenthesized integer in the string, extracts it, | |
| and returns both the integer and the string with the parenthesized | |
| portion removed. | |
| Args: | |
| string: The string to search. | |
| string: The string to search for a parenthesized integer. | |
| Returns: | |
| A tuple containing the integer and the text without the number in parenthesis. | |
| A tuple of (cleaned_text, integer) where cleaned_text has the | |
| parenthesized number removed, or None if no parenthesized | |
| integer is found. | |
| def find_integer_in_parenthesis(string: str) -> tuple[str, int] | None: | |
| """Find an integer in parentheses and return it with the cleaned text. | |
| Locates the first parenthesized integer in the string, extracts it, | |
| and returns both the integer and the string with the parenthesized | |
| portion removed. | |
| Args: | |
| string: The string to search for a parenthesized integer. | |
| Returns: | |
| A tuple of (cleaned_text, integer) where cleaned_text has the | |
| parenthesized number removed, or None if no parenthesized | |
| integer is found. |
🤖 Prompt for AI Agents
In `@src/napari_chatgpt/utils/strings/find_integer_in_parenthesis.py` around lines
4 - 17, The function find_integer_in_parenthesis has a docstring saying it may
return None but its signature declares -> tuple[str, int]; update the return
type to reflect the None path by changing the annotation to Optional[tuple[str,
int]] and add the necessary import (from typing import Optional) or use
typing.Optional; ensure the function signature and docstring are consistent
(i.e., -> Optional[tuple[str, int]] and docstring describing the None return).
Convert remaining NumPy-style docstrings to Google-style for consistency. Fix typo "scrapper" -> "scraper", fix missing __init__ docstring, fix incorrect return type annotation, and standardize formatting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/napari_chatgpt/omega_agent/tools/napari/delegated_code/aydin_classic.py (1)
13-30:⚠️ Potential issue | 🟡 MinorNumPy-style docstring not converted to Google-style.
The PR objective states all NumPy-style docstrings were converted to Google-style for consistency, but this function still uses NumPy-style (
Parameters\n----------,Returns\n-------). Convert to Google-style (Args:,Returns:) to match the rest of the codebase.📝 Proposed conversion to Google-style docstring
- """Denoise an image using Aydin's Classic filter-based restoration algorithms. - - Parameters - ---------- - image : numpy.ndarray - Image to denoise - batch_axes : array_like, optional - Indices of batch axes. Batch dimensions/axes are dimensions/axes for which there is no-spatiotemporal correlation in the data. For example: different instance images stored in the same array. - chan_axes : array_like, optional - Indices of channel axes. This is the dimensions/axis of the numpy array that corresponds to the channel dimension of the image. Dimensions/axes that are not batch or channel dimensions are your standard X,Y,Z or T dimensions over which the data exhibits some spatiotemporal correlation. - variant : str - Algorithm variant. Can be: 'bilateral', 'butterworth', 'gaussian', 'gm', 'harmonic', 'nlm', 'pca', 'spectral', 'tv', 'wavelet'. - - Returns - ------- - Denoised image : numpy.ndarray - - """ + """Denoise an image using Aydin's Classic filter-based restoration algorithms. + + Args: + image: Image to denoise (numpy.ndarray). + batch_axes: Indices of batch axes. Batch dimensions/axes are + dimensions/axes for which there is no spatiotemporal correlation + in the data. For example: different instance images stored in + the same array. + chan_axes: Indices of channel axes. This is the dimensions/axis of the + numpy array that corresponds to the channel dimension of the image. + Dimensions/axes that are not batch or channel dimensions are your + standard X, Y, Z or T dimensions over which the data exhibits some + spatiotemporal correlation. + variant: Algorithm variant. Can be: 'bilateral', 'butterworth', + 'gaussian', 'gm', 'harmonic', 'nlm', 'pca', 'spectral', 'tv', + 'wavelet'. + + Returns: + Denoised image as a numpy.ndarray. + """src/napari_chatgpt/utils/python/check_code_safety.py (1)
71-71:⚠️ Potential issue | 🟡 MinorTrailing comma inside the string looks like a typo.
"No code is safe code,"has a trailing comma that appears unintentional — it reads like a sentence fragment rather than a complete statement.Proposed fix
- return "No code is safe code,", "A" + return "No code is safe code.", "A"
🤖 Fix all issues with AI agents
In `@src/napari_chatgpt/llm/api_keys/api_key_vault_dialog.py`:
- Around line 213-225: The function request_if_needed_api_key_dialog has a
misleading return annotation: it can return None as documented and in code
paths, so change its signature to return Optional[str] instead of str and add
the necessary typing import (from typing import Optional) at the top; update any
local references or annotations that depend on its return type if present
(search for request_if_needed_api_key_dialog) to accept Optional[str].
In `@src/napari_chatgpt/utils/llm/vision.py`:
- Around line 13-22: Update the docstring for the function that checks vision
support (refer to the model_name parameter and get_best_model usage) to
accurately state that when model_name is None the code calls get_best_model(...)
to pick a single best model with vision support (it does not iterate over all
models), and change the return description to specify it returns True only if
the chosen model supports vision capabilities (Image + TextGeneration), not just
general availability; mention api parameter behavior remains unchanged and keep
wording concise and precise.
- Around line 56-67: The docstring for the image description function is
misleading: change references from "GPT" and "GPT-vision" to provider-agnostic
wording (e.g., "LLM/vision model" or "LiteMind CombinedApi") and mention that
the implementation uses CombinedApi / LiteMind abstractions (see
get_litemind_api, get_llm, get_model_list) rather than a specific GPT provider;
also update the Returns section to state that on failure the function returns an
error-description string (after number_of_tries) instead of raising, and
document key params (image_path, query, api, model_name, number_of_tries)
accordingly so callers know the function's behavior.
In `@src/napari_chatgpt/utils/python/exception_guard.py`:
- Around line 10-22: The docstring says exception_value holds "The exception
instance" but the attribute is annotated as str; update the type annotation for
exception_value to reflect an exception instance (e.g., Optional[BaseException]
or Optional[Exception]) and import typing.Optional if needed, ensuring its
default remains None; keep other attributes (exception_type_name,
exception_traceback, exception_description) unchanged and update any related
type hints or doc comment to match the new annotation in the
ExceptionGuard/context manager class.
🧹 Nitpick comments (2)
src/napari_chatgpt/omega_agent/tools/napari/widget_maker_tool.py (1)
226-238: Docstring is mostly accurate, minor precision nit.The phrase "Exceptions propagate to
ExceptionGuard" could be slightly misleading — this method itself doesn't catch or wrap exceptions intoExceptionGuard; that wrapping happens in the queue infrastructure (the caller checks the response). Consider rewording to "Exceptions raised here are caught by the queue executor and returned asExceptionGuardinstances" for clarity.src/napari_chatgpt/utils/python/exception_guard.py (1)
44-57:__exit__is missing a docstring — gap in a docs-focused PR.Given that this PR aims to add docstrings across the codebase,
__exit__(the most interesting method in this class) would benefit from a short docstring documenting its suppression behavior and what it captures.
| def request_if_needed_api_key_dialog(api_key_name: str) -> str: | ||
| """Show the API key dialog if the user has not already been prompted. | ||
|
|
||
| Each provider is only prompted once per session. Subsequent calls for | ||
| the same ``api_key_name`` return ``None`` immediately. | ||
|
|
||
| Args: | ||
| api_key_name: Provider name (e.g. ``"OpenAI"``). | ||
|
|
||
| Returns: | ||
| The decrypted API key, or ``None`` if the user dismissed the | ||
| dialog or was already prompted. | ||
| """ |
There was a problem hiding this comment.
Return type annotation doesn't match documented (and actual) behavior.
The docstring correctly states the function can return None, but the signature declares -> str. Since the function returns None in at least two paths (line 227 and when the dialog is dismissed), the annotation should reflect that.
Proposed fix
-def request_if_needed_api_key_dialog(api_key_name: str) -> str:
+def request_if_needed_api_key_dialog(api_key_name: str) -> str | None:🤖 Prompt for AI Agents
In `@src/napari_chatgpt/llm/api_keys/api_key_vault_dialog.py` around lines 213 -
225, The function request_if_needed_api_key_dialog has a misleading return
annotation: it can return None as documented and in code paths, so change its
signature to return Optional[str] instead of str and add the necessary typing
import (from typing import Optional) at the top; update any local references or
annotations that depend on its return type if present (search for
request_if_needed_api_key_dialog) to accept Optional[str].
| """Check if vision is available. | ||
|
|
||
| Args: | ||
| api: API to use for checking availability. If None, uses CombinedApi. | ||
| model_name: Specific model name to check for vision capabilities. | ||
| If None, checks all models. | ||
|
|
||
| Returns: | ||
| True if the model is available, False otherwise. | ||
| """ |
There was a problem hiding this comment.
Docstring inaccuracies for model_name and return value.
- Line 18: "If None, checks all models" is misleading — the code calls
get_best_model(...)to select the single best model with vision support, then checks that model. It doesn't iterate over all models. - Line 21: "True if the model is available" undersells what's checked — it's specifically about vision (Image + TextGeneration) capability, not general availability.
📝 Suggested docstring fix
"""Check if vision is available.
Args:
api: API to use for checking availability. If None, uses CombinedApi.
model_name: Specific model name to check for vision capabilities.
- If None, checks all models.
+ If None, selects the best available model with vision support.
Returns:
- True if the model is available, False otherwise.
+ True if a model with vision capabilities is available, False otherwise.
"""🤖 Prompt for AI Agents
In `@src/napari_chatgpt/utils/llm/vision.py` around lines 13 - 22, Update the
docstring for the function that checks vision support (refer to the model_name
parameter and get_best_model usage) to accurately state that when model_name is
None the code calls get_best_model(...) to pick a single best model with vision
support (it does not iterate over all models), and change the return description
to specify it returns True only if the chosen model supports vision capabilities
(Image + TextGeneration), not just general availability; mention api parameter
behavior remains unchanged and keep wording concise and precise.
| """Describe an image using GPT-vision. | ||
|
|
||
| Args: | ||
| image_path: Path to the image to describe. | ||
| query: Query to send to GPT. | ||
| api: API to use for describing the image. If None, uses CombinedApi. | ||
| model_name: Model to use. | ||
| number_of_tries: Number of times to try to send the request to GPT. | ||
|
|
||
| Returns: | ||
| Description of the image. | ||
| """ |
There was a problem hiding this comment.
Docstring references "GPT" but the implementation is provider-agnostic.
The function uses CombinedApi from LiteMind, which can route to any configured provider — not just GPT. References to "GPT-vision" (line 56) and "GPT" (line 60) are misleading.
Additionally, the Returns section doesn't document the important behavior that on failure, the function returns an error description string (lines 113-115) instead of raising an exception. Callers need to be aware of this.
📝 Suggested docstring fix
- """Describe an image using GPT-vision.
+ """Describe an image using an LLM with vision capabilities.
Args:
image_path: Path to the image to describe.
- query: Query to send to GPT.
+ query: Query to send to the vision model.
api: API to use for describing the image. If None, uses CombinedApi.
model_name: Model to use.
- number_of_tries: Number of times to try to send the request to GPT.
+ number_of_tries: Number of times to retry the request on failure.
Returns:
- Description of the image.
+ Description of the image, or an error message string if the
+ request fails.
"""Based on learnings: "Use LiteMind library abstractions (get_litemind_api(), get_llm(), get_model_list()) from llm/litemind_api.py for LLM interactions instead of direct provider APIs" — the docstring should reflect the provider-agnostic nature of the implementation.
🤖 Prompt for AI Agents
In `@src/napari_chatgpt/utils/llm/vision.py` around lines 56 - 67, The docstring
for the image description function is misleading: change references from "GPT"
and "GPT-vision" to provider-agnostic wording (e.g., "LLM/vision model" or
"LiteMind CombinedApi") and mention that the implementation uses CombinedApi /
LiteMind abstractions (see get_litemind_api, get_llm, get_model_list) rather
than a specific GPT provider; also update the Returns section to state that on
failure the function returns an error-description string (after number_of_tries)
instead of raising, and document key params (image_path, query, api, model_name,
number_of_tries) accordingly so callers know the function's behavior.
| """Context manager that catches exceptions and stores their details. | ||
|
|
||
| Any exception raised within the guarded block is suppressed and its | ||
| details (type, value, traceback, description) are stored as attributes | ||
| for later inspection. | ||
|
|
||
| Attributes: | ||
| exception: The exception class that was raised, or None. | ||
| exception_type_name: Name of the exception class, or None. | ||
| exception_value: The exception instance, or None. | ||
| exception_traceback: The traceback object, or None. | ||
| exception_description: Human-readable description of the error, or None. | ||
| """ |
There was a problem hiding this comment.
exception_value docstring contradicts its type annotation.
Line 19 correctly documents exception_value as "The exception instance", but the existing type annotation on line 37 declares it as str. The annotation is the bug (it actually stores an Exception), but now the new docstring and the annotation tell two different stories. Consider fixing the annotation while you're here:
Proposed fix
- self.exception_value: str = None
+ self.exception_value: Exception = None🤖 Prompt for AI Agents
In `@src/napari_chatgpt/utils/python/exception_guard.py` around lines 10 - 22, The
docstring says exception_value holds "The exception instance" but the attribute
is annotated as str; update the type annotation for exception_value to reflect
an exception instance (e.g., Optional[BaseException] or Optional[Exception]) and
import typing.Optional if needed, ensuring its default remains None; keep other
attributes (exception_type_name, exception_traceback, exception_description)
unchanged and update any related type hints or doc comment to match the new
annotation in the ExceptionGuard/context manager class.
1 participant
Summary
AttributessectionsArgs,Returns, andRaisessections where appropriateModules covered:
_widget.py,_sample_data.py,__init__.py)chat_server/)llm/, including API key vault)microplugin/code_editor/)microplugin/network/)omega_agent/)omega_agent/tools/)omega_agent/tools/napari/delegated_code/)utils/python/,utils/strings/,utils/web/,utils/qt/,utils/napari/,utils/segmentation/,utils/system/, etc.)Test plan
py_compilesyntax checking🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements