narrow _update_workspace_settings_with_version_info except clause to specific exception types#769
Conversation
…specific exception types The bare `except:` in the per-workspace version-detect loop was catching everything including KeyboardInterrupt and SystemExit, which made a Ctrl+C during workspace activation get swallowed and re-logged as a version detection error. Replaced with the specific exceptions that the body actually raises: - `from packaging.version import parse as parse_version` -> ImportError - `_run_tool` (subprocess path) -> OSError, subprocess.SubprocessError - `parse_version(actual_version)` -> ValueError on a malformed version - `first_line.splitlines(...)[0]` -> IndexError on empty stdout - regex / attribute access on the result -> AttributeError A Ctrl+C now propagates out of the loop as expected instead of being logged as a version-detection error.
| except (ImportError, OSError, subprocess.SubprocessError, ValueError, IndexError, AttributeError): | ||
| log_to_output( | ||
| f"Error while detecting black version:\r\n{traceback.format_exc()}" | ||
| ) |
There was a problem hiding this comment.
📍 bundled/tool/lsp_server.py:376
The enumerated tuple under-catches real, reachable exceptions and converts a deliberately-total boundary into a leaky one. This function is the last statement of the INITIALIZE handler, so any escaping exception can fail server startup. The Skeptic verified that EOFError (raised by the JSON-RPC reader in the importStrategy: fromEnvironment/use_rpc path) and StreamClosedException are not subclasses of any listed type; the Architect traced that KeyError from settings[...] subscripts and any Exception re-raised by _run_tool's in-process branch (except Exception: ... raise) also escape. These were previously swallowed-and-logged. The PR's stated goal — stop catching SystemExit/KeyboardInterrupt — is fully achieved by except Exception: alone (both are BaseException, not Exception), which also preserves the "never break init" contract and removes coupling to _run_tool's open-ended exception surface. Recommend except Exception:.
[verified]
| ) | ||
|
|
||
| except: # pylint: disable=bare-except | ||
| except (ImportError, OSError, subprocess.SubprocessError, ValueError, IndexError, AttributeError): |
There was a problem hiding this comment.
📍 bundled/tool/lsp_server.py:376
If the team chooses to keep an enumerated list rather than except Exception:, it must at minimum add KeyError — _run_tool reads settings["path"], settings["interpreter"], settings["importStrategy"], settings["workspaceFS"], and the body reads settings["workspaceFS"]. The settings dict built at this call site is not merged with _get_global_defaults(), so a client omitting any key produces an uncaught KeyError that escapes initialize. Also add a comment explaining why this boundary must stay broad, so it isn't "tidied" again later.
[verified]
|
The intent (stop catching SystemExit/KeyboardInterrupt) is good, but |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR via the Review Center. Other automated reviewers defer to this claim to avoid duplicate, conflicting feedback. The claim is refreshed while the review is active; if it is not refreshed within the ownership timeout it is treated as vacant and another reviewer may take over. This is an automated marker. |
| ) | ||
|
|
||
| except: # pylint: disable=bare-except | ||
| except (ImportError, OSError, subprocess.SubprocessError, ValueError, IndexError, AttributeError): |
There was a problem hiding this comment.
📍 bundled/tool/lsp_server.py:376
_run_tool explicitly re-raises arbitrary Exception (:701-703: except Exception: log_error(...); raise). The in-process black path can therefore surface RuntimeError/TypeError (and the RPC path StreamClosedException/EOFError/KeyError) that the narrowed tuple does not catch, regressing the previous log-and-continue behavior. Because this probe runs inside the initialize handler, those now propagate and can fail initialization. Use except Exception: — it preserves the PR's goal (still excludes SystemExit/KeyboardInterrupt, which derive from BaseException) while decoupling this best-effort probe from _run_tool's internals; an enumerated tuple cannot stay correct against a callee that re-raises Exception.
|
The intent (drop the bare |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR via the Review Center. Other automated reviewers defer to this claim to avoid duplicate, conflicting feedback. The claim is refreshed while the review is active; if it is not refreshed within the ownership timeout it is treated as vacant and another reviewer may take over. This is an automated marker. |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR via the Review Center. Other automated reviewers defer to this claim to avoid duplicate, conflicting feedback. The claim is refreshed while the review is active; if it is not refreshed within the ownership timeout it is treated as vacant and another reviewer may take over. This is an automated marker. |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR. |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR. |
2 participants
The bare
except:clause in_update_workspace_settings_with_version_info(bundled/tool/lsp_server.py:378) was catching everything includingSystemExitandKeyboardInterrupt. Replaced it with a tuple of the specific exception types that the function body can actually raise:ImportError(for the in-linefrom packaging.version import parse as parse_version),OSError/subprocess.SubprocessError(for the underlying_run_toolsubprocess),ValueErrorandIndexError(for the.stdout.splitlines(...)[0]and version-string parsing), andAttributeError(for theresult.stdoutaccess when the subprocess produces no output). Thelog_to_output(...)debug behavior is preserved on all caught paths.