Add option for ignore all system exit codes#2017
Conversation
tests/debug/config.py
Outdated
| PROPERTIES = { | ||
| # Common | ||
| "breakOnSystemExitZero": False, | ||
| "breakOnSystemExit": (), |
There was a problem hiding this comment.
Copilot generated:
The default "breakOnSystemExit": () is risky. If the test framework serializes all config properties (including defaults) into launch args, () becomes [] in JSON, which the adapter interprets as "never break on any SystemExit" — silently breaking all existing SystemExit tests. Use None instead: the adapter's args.get("breakOnSystemExit", None) already handles None correctly by falling through to legacy behavior.
| :param list ranges: | ||
| List of (from_code, to_code) tuples (inclusive) to break on. | ||
| """ | ||
| self._break_on_system_exit = (codes, ranges) |
There was a problem hiding this comment.
Copilot generated:
-952
Behavior regression for string exit codes. When _break_on_system_exit is set and sys.exit("fatal error") is called: the string is not in codes_set, isinstance("fatal error", int) is False so ranges are skipped, and the method returns True (ignore). Previously, string codes were NOT in _ignore_system_exit_codes so the debugger would break. This silently suppresses breaks that users previously got. Consider treating non-int, non-None codes as "always break" when _break_on_system_exit is set, or document this limitation clearly.
| self._exclude_by_filter_cache = {} | ||
| self._apply_filter_cache = {} | ||
| self._ignore_system_exit_codes = set() | ||
| self._break_on_system_exit = None # None = default behavior, tuple = (codes_set, ranges_list) |
There was a problem hiding this comment.
Copilot generated:
Dual state variables can conflict silently. _break_on_system_exit and _ignore_system_exit_codes are independent state for the same decision. If a caller uses the old set_ignore_system_exit_codes API and another sets set_break_on_system_exit, the new one silently wins with no warning. Consider clearing _ignore_system_exit_codes when set_break_on_system_exit is called (or vice versa), or unifying the internal representation.
| assert isinstance(ignore_system_exit_codes, (list, tuple, set)) | ||
| self._ignore_system_exit_codes = set(ignore_system_exit_codes) | ||
|
|
||
| def set_break_on_system_exit(self, codes, ranges): |
There was a problem hiding this comment.
Copilot generated:
-952
Method name ignore_system_exit_code is now semantically misleading. When _break_on_system_exit is set, the method answers "should I ignore this?" but the configuration semantics are "should I break on this?" — the logic is inverted. Consider adding a clearer wrapper like should_break_on_system_exit_code() or at minimum updating the docstring to explain the dual semantics.
|
|
||
| if break_on_system_exit is not None: | ||
| codes = set() | ||
| ranges = [] |
There was a problem hiding this comment.
Copilot generated:
-456
Silent opposite-of-intended behavior for string items. If a user writes "breakOnSystemExit": ["1", "2"] (strings instead of ints), all items are silently dropped because none match int, None, or dict. Result: codes=set(), ranges=[] — meaning "never break on anything," the exact opposite of intent. Add a log warning for unrecognized item types, or coerce numeric strings to int.
| if item is None: | ||
| codes.add(None) | ||
| elif isinstance(item, int): | ||
| codes.add(item) |
There was a problem hiding this comment.
Copilot generated:
-453
Non-integer range values cause TypeError crash. If a user writes {"from": "abc", "to": "xyz"}, item.get("from", 0) stores the string. Later in ignore_system_exit_code, "abc" <= code <= "xyz" (where code is an int) raises TypeError in Python 3. This crashes during exception handling. Validate that range_from and range_to are ints during parsing, and skip with a log warning if not.
| if item is None: | ||
| codes.add(None) | ||
| elif isinstance(item, int): | ||
| codes.add(item) |
There was a problem hiding this comment.
Copilot generated:
-453
No validation that from ≤ to in ranges. {"from": 100, "to": 3} silently matches nothing. While fail-safe, a pydev_log.info warning for inverted ranges would save users debugging time when their configuration doesn't work as expected.
| self.api.set_show_return_values(py_db, self._options.show_return_value) | ||
|
|
||
| if not self._options.break_system_exit_zero: | ||
| break_on_system_exit = args.get("breakOnSystemExit", None) |
There was a problem hiding this comment.
Copilot generated:
-456
Consider extracting the breakOnSystemExit parsing into a small helper function (e.g., _parse_break_on_system_exit(args)) that validates input and returns (codes, ranges) or None. This would isolate the parsing logic from the large _set_arguments method and make validation easier to test independently.
| @@ -0,0 +1,48 @@ | |||
| --- | |||
There was a problem hiding this comment.
Copilot generated:
📍 .github/skills/django/SKILL.md (and all 7 skill files)
The Rules reviewer found these skill files are under .github/skills/ instead of the repo-canonical .claude/skills/ directory. The customization loader is scoped to .claude/skills/*/SKILL.md, so these files won't be discovered. Move all seven to .claude/skills/<name>/SKILL.md and update PythonSelfImproving.selfEval.json's skillsDir accordingly.
3 participants
Potentially a solution for #1591.
Adds a new launch.json entry that allows the user to specify the breakOnSystemExit codes. Like so:
{ "name": "Python: Current File", "type": "debugpy", "request": "launch", "program": "${file}", "breakOnSystemExit": [] }That launch.json will NEVER break on System.exit. The default is break on everything except zero or None.
You can also specify ranges:
{ "name": "Python: Current File", "type": "debugpy", "request": "launch", "program": "${file}", "breakOnSystemExit": [0, {"from": 3, "to": 100}] }That launch.json would NOT break on 1,2 but break on everything else.