fix: replace 42 bare excepts with except Exception#180
Closed
haosenwang1018 wants to merge 1 commit intosimular-ai:mainfrom
Closed
fix: replace 42 bare excepts with except Exception#180haosenwang1018 wants to merge 1 commit intosimular-ai:mainfrom
haosenwang1018 wants to merge 1 commit intosimular-ai:mainfrom
Conversation
Bare except catches KeyboardInterrupt and SystemExit, masking real errors.
📝 WalkthroughWalkthroughSystematic replacement of bare Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
osworld_setup/s1/run.py (1)
224-228:⚠️ Potential issue | 🟡 MinorUnconditional
env.controller.end_recording()may raiseAttributeErrorif the provider has no controller.The s3 counterpart (
osworld_setup/s3/run_local.py, lines 287–290) guards this call withif hasattr(env, "controller") and env.controller is not None. This file does not, so any non-VMware provider that lacks acontrollerattribute will raise anAttributeErrorinside theexcepthandler itself, masking the original error.This is a pre-existing issue, but now that the surrounding handler is explicitly narrowed it is more visible.
🛡️ Proposed fix
- env.controller.end_recording( - os.path.join(example_result_dir, "recording.mp4") - ) + if hasattr(env, "controller") and env.controller is not None: + env.controller.end_recording( + os.path.join(example_result_dir, "recording.mp4") + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osworld_setup/s1/run.py` around lines 224 - 228, The except block logs the error and then unconditionally calls env.controller.end_recording(...), which can raise AttributeError if env has no controller; update the handler in run.py to check hasattr(env, "controller") and env.controller is not None before calling env.controller.end_recording with os.path.join(example_result_dir, "recording.mp4") so the cleanup is only attempted when a controller exists (use the same guard approach as in osworld_setup/s3/run_local.py).osworld_setup/s3/bbon/generate_facts.py (1)
28-35:⚠️ Potential issue | 🟡 MinorSilent
except Exception: passon JSON parse discards the actual error.When
json.loadsfails, the real error (e.g.,json.JSONDecodeError) is swallowed and replaced by the genericValueErroron line 35. A caller seeing"No pyautogui action found for step N"has no way to distinguish a malformed line from a genuinely missing action. Consider logging the parse error, and narrowing the catch tojson.JSONDecodeError.🔧 Proposed fix
try: data = json.loads(trajectory_lines[i]) pyautogui_action = data.get("exec_code") - except Exception: - pass + except json.JSONDecodeError as e: + print(f"Warning: could not parse trajectory line {i}: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osworld_setup/s3/bbon/generate_facts.py` around lines 28 - 35, The try/except currently swallows all errors when parsing trajectory_lines[i]; narrow the except to catch json.JSONDecodeError (not Exception), capture the exception as e, and log the parse failure including the index i and the offending line (trajectory_lines[i]) and the error message (e) before continuing; keep the existing behavior that later raises ValueError if pyautogui_action is None. Ensure you import or use the module logger (e.g., logging) and reference the variables and context names shown (trajectory_lines, pyautogui_action, i) so the log makes it clear this was a JSON parse error rather than a missing exec_code.
🧹 Nitpick comments (16)
gui_agents/s1/cli_app.py (1)
42-43:except Exceptionis correct but still broader than necessary.The only realistically expected failures inside
get_charare:
ImportError— whentermios/ttyormsvcrtare unavailableio.UnsupportedOperation/OSError— whenstdin.fileno()ortermiosoperations fail in a non-terminal environmentNarrowing the catch avoids silently swallowing unexpected errors (e.g., a
MemoryErroror a logic bug in a future edit):♻️ Proposed narrowing
- except Exception: + except (ImportError, OSError): return input() # Fallback for non-terminal environments🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s1/cli_app.py` around lines 42 - 43, The except block in get_char is too broad; narrow it to only the expected exceptions by catching ImportError, io.UnsupportedOperation and OSError (the ones raised when termios/tty or stdin.fileno()/termios fail) instead of Exception, and ensure io is imported so io.UnsupportedOperation is available; keep the existing fallback return input() for non-terminal environments.osworld_setup/s3/run_local.py (1)
356-365: Narrow to(OSError, ValueError)inget_result.
float(open(...).read())raises onlyOSError(I/O failure) orValueError(non-numeric content). Usingexcept Exceptionhides any other unexpected errors silently.♻️ Proposed narrowing
- except Exception: + except (OSError, ValueError): all_result.append(0.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osworld_setup/s3/run_local.py` around lines 356 - 365, The try/except in get_result currently swallows all exceptions; narrow it to only catch I/O and parse errors by replacing the broad "except Exception" with "except (OSError, ValueError)" around the float(open(...).read()) read/parse block (and keep appending 0.0 on those specific failures) so other unexpected exceptions propagate; reference the get_result logic that builds all_result and the float(open(os.path.join(example_path, "result.txt"), "r").read()) call when making the change.gui_agents/s3/bbon/comparative_judge.py (1)
21-25: Narrow to the two expected exception types inextract_step_num.Only
IndexError(insufficient split parts) orValueError(int()on non-numeric content) can realistically occur here:♻️ Proposed narrowing
def extract_step_num(filename): try: return int(filename.split("_")[1].split(".")[0]) - except Exception: + except (IndexError, ValueError): return 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s3/bbon/comparative_judge.py` around lines 21 - 25, The function extract_step_num currently catches all exceptions; narrow the exception handling to only the expected types by catching IndexError and ValueError explicitly (i.e., replace except Exception with except (IndexError, ValueError)) so only parsing/split errors return 0 and other unexpected exceptions bubble up.gui_agents/s3/cli_app.py (1)
45-46: Sameget_charnarrowing opportunity asgui_agents/s1/cli_app.py.Prefer
except (ImportError, OSError):for the same reasons noted there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s3/cli_app.py` around lines 45 - 46, Update the broad except in get_char to only catch the expected import/terminal failures: replace "except Exception:" with "except (ImportError, OSError):" (or the same tuple used in gui_agents/s1/cli_app.py) so only import/OS-level terminal issues fall back to input(); keep the existing fallback return input() and ensure the change references get_char to locate the handler.gui_agents/s2/cli_app.py (1)
44-45: Sameget_charnarrowing opportunity asgui_agents/s1/cli_app.py.Prefer
except (ImportError, OSError):for the same reasons noted there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s2/cli_app.py` around lines 44 - 45, In the get_char function replace the broad "except Exception:" fallback with a narrowed except clause "except (ImportError, OSError):" so only import/OS-related failures fall back to input(); update the except block that currently returns input() to use these two specific exceptions (refer to the get_char function and its fallback except block).gui_agents/s2_5/cli_app.py (1)
44-45: Sameget_charnarrowing opportunity asgui_agents/s1/cli_app.py.Prefer
except (ImportError, OSError):for the same reasons noted there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s2_5/cli_app.py` around lines 44 - 45, The except block in get_char (cli_app.py) is too broad; narrow the exception handling to only ImportError and OSError (i.e., use except (ImportError, OSError):) so we catch the expected module/load errors while letting other exceptions surface, and keep the same fallback return input() behavior inside that narrowed handler.osworld_setup/s1/run.py (1)
294-303: Narrow to(OSError, ValueError)inget_result.Same as
osworld_setup/s3/run_local.py— onlyOSErrororValueErrorare expected fromfloat(open(...).read()).♻️ Proposed narrowing
- except Exception: + except (OSError, ValueError): all_result.append(0.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osworld_setup/s1/run.py` around lines 294 - 303, The broad except Exception in get_result should be narrowed to only catch the expected errors: replace the generic except with except (OSError, ValueError) around the float(open(os.path.join(example_path, "result.txt"), "r").read()) call so only file/IO and conversion errors are swallowed; keep appending 0.0 to all_result for those cases and leave other exceptions to surface. Use the existing all_result and example_path variables and the get_result function context when making this change.osworld_setup/s3/run.py (1)
515-524: Narrow to(OSError, ValueError)inget_result.Same as the other
run.pyvariants — onlyOSErrororValueErrorare expected here.♻️ Proposed narrowing
- except Exception: + except (OSError, ValueError): all_result.append(0.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osworld_setup/s3/run.py` around lines 515 - 524, The except clause in get_result currently catches all exceptions; narrow it to only catch OSError and ValueError (the expected errors when opening/reading/parsing result.txt) and leave other exceptions to surface; locate the try/except around reading os.path.join(example_path, "result.txt") which appends to all_result and replace "except Exception" with "except (OSError, ValueError)" so only file I/O and float conversion errors are handled.gui_agents/s2/core/knowledge.py (2)
88-93: Prefer targeted read exceptions over blanketExceptionin cache loads.These handlers still swallow unexpected runtime bugs and silently coerce to
{}. Restrict to expected file/JSON failures and let unexpected exceptions surface.Proposed refactor
- except Exception: + except (FileNotFoundError, json.JSONDecodeError, OSError): formulate_query = {}- except Exception: + except (FileNotFoundError, json.JSONDecodeError, OSError): exist_search_results = {}Also applies to: 129-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s2/core/knowledge.py` around lines 88 - 93, Replace the blanket "except Exception" in the cache load blocks so only expected I/O/parse errors are swallowed: catch FileNotFoundError and json.JSONDecodeError (and optionally PermissionError) around the open/json.load in the block that assigns formulate_query (and the similar block around the other cache load at lines 129-133), set formulate_query = {} on those specific exceptions, and let all other exceptions propagate so unexpected runtime bugs are not silently ignored.
272-275: Avoid silent KB reset on unexpected load failures before persisting.Defaulting to
{}for any exception can lead to unintended overwrite behavior in subsequent writes. Keep fallback only for expected read/parsing failures.Also applies to: 297-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s2/core/knowledge.py` around lines 272 - 275, The current try/except around load_knowledge_base(self.episodic_memory_path) silently swallows all exceptions and defaults kb to {}, which can cause unintended overwrites on subsequent writes; update the block so it only catches expected read/parsing errors (e.g., FileNotFoundError, JSONDecodeError or whatever load_knowledge_base raises for missing/corrupt data) and falls back to {} in those cases, but re-raises or propagates any other unexpected exceptions so they aren’t hidden. Apply the same change to the analogous block later (around lines handling the second load) and reference the load_knowledge_base call and self.episodic_memory_path when making the change.gui_agents/s1/core/Knowledge.py (1)
82-86: Use targeted exceptions for JSON/cache reads.Catching
Exceptionhere still suppresses non-I/O defects. Prefer handlingFileNotFoundError/json.JSONDecodeError/OSErrorexplicitly.Also applies to: 123-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s1/core/Knowledge.py` around lines 82 - 86, Replace the broad except Exception in the JSON/cache read blocks (the try that loads formulate_query from query_path and the similar block around lines 123-127) with targeted exception handling: catch FileNotFoundError, json.JSONDecodeError, and OSError and set formulate_query = {} (or the equivalent fallback) in those cases, and re-raise any other unexpected exceptions so non-I/O bugs aren’t silently swallowed; reference the variables/query_path and the formulate_query load logic to locate both spots to update.gui_agents/s1/aci/MacOSACI.py (1)
308-312: Narrow fallback catches aroundfind_elementto expected input/index errors.
except Exceptioncan hide unrelated bugs and silently route actions to fallback targets. Catch concrete exceptions (TypeError,ValueError,IndexError) instead.Also applies to: 403-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s1/aci/MacOSACI.py` around lines 308 - 312, The current broad except Exception around the call to self.find_element(element_id) (which sets node) should be narrowed to only catch the expected input/index errors; update the try/except blocks in MacOSACI.py around the find_element usage (both the block where node = self.find_element(element_id) and the similar block at the later occurrence) to catch TypeError, ValueError, and IndexError instead of Exception, leaving other exceptions to propagate; ensure the except sets node = None as before and keep the original behavior for valid errors.gui_agents/s2/agents/agent_s.py (1)
349-353: Tighten memory-file read error handling in update paths.Please catch expected JSON/file exceptions only;
except Exceptionhere can hide unexpected bugs and silently reset loaded state to{}.Safer pattern
- try: - reflections = json.load(open(reflection_path)) - except Exception: + try: + with open(reflection_path, "r", encoding="utf-8") as f: + reflections = json.load(f) + except (FileNotFoundError, json.JSONDecodeError, OSError): reflections = {}Also applies to: 385-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s2/agents/agent_s.py` around lines 349 - 353, Replace the broad except Exception around json.load(open(reflection_path)) with targeted error handling: open the file using a context manager (with open(reflection_path, "r") as f) and call json.load(f), and catch only expected exceptions such as json.JSONDecodeError, FileNotFoundError, and OSError/PermissionError to set reflections = {} (and optionally log a warning); let all other exceptions propagate so unexpected bugs are not silenced. Apply the same change to the other occurrence around lines 385-391 that uses reflection_path.gui_agents/s1/aci/windowsagentarena/GroundingAgent.py (1)
307-314: Avoid catch-all fallbacks in action-selection paths.Using
except Exceptionhere can mask state bugs and unintentionally act on element0. Catch only expected conversion/index failures.Also applies to: 375-379, 548-551
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s1/aci/windowsagentarena/GroundingAgent.py` around lines 307 - 314, The find_element method currently swallows all Exceptions and falls back to self.nodes[0]; replace the broad except with targeted exception handling for ValueError (int conversion) and IndexError (out-of-range) so you don't mask other bugs: parse element_id with int inside a try and except (ValueError, IndexError) only, log or set self.index_out_of_range_flag and return None (or raise a specific error) instead of defaulting to nodes[0]; apply the same change to the other action-selection sites mentioned (the similar blocks at lines corresponding to the other occurrences) by replacing except Exception with except (ValueError, IndexError) and removing the implicit fallback to index 0.gui_agents/s1/aci/LinuxOSACI.py (1)
414-418: Outertry/except Exceptionintype()andscroll()is redundant.
find_element()(lines 344–351) already catchesIndexErrorinternally and never re-raises it. The outerexcept Exceptionhere will only fire for genuinely unexpected errors (e.g.,AttributeErroronself.nodes), which is unlikely and unrelated to the stated intent. Consider removing the outer guard or, if intentional, add a comment explaining what it catches.Also applies to: 517-520
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s1/aci/LinuxOSACI.py` around lines 414 - 418, Remove the redundant outer try/except around the call to find_element in the type() method (and the analogous block in scroll()); instead directly assign node = self.find_element(element_id) if element_id is not None else None because find_element already handles IndexError internally—if you actually intend to catch other unexpected errors, replace the broad except with a specific exception or add an inline comment in type() and scroll() explaining which non-IndexError cases you want to guard against.gui_agents/s1/aci/WindowsOSACI.py (1)
303-306: Outertry/except Exceptionwrappingfind_element()is redundant in bothtype()andscroll().
WindowsACI.find_element()(lines 228–233) already catchesIndexErrorinternally and returnsself.nodes[0]. The outerexcept Exceptionhere only fires for errors unrelated to the index bounds (e.g.,self.nodesbeing uninitialized), which are separate, unexpected failures better left to propagate. This mirrors the same pattern flagged inLinuxOSACI.Also applies to: 387-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gui_agents/s1/aci/WindowsOSACI.py` around lines 303 - 306, The outer try/except that wraps calls to self.find_element() in WindowsACI.type() and WindowsACI.scroll() is redundant and suppresses unexpected errors; remove the surrounding try/except blocks so that WindowsACI.find_element() (which already handles IndexError) returns normally or lets other exceptions propagate—specifically, eliminate the try/except around the node = self.find_element(element_id) if element_id is not None else None lines in both type() and scroll() so only intentional IndexError handling stays inside find_element().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gui_agents/s1/aci/LinuxOSACI.py`:
- Around line 670-673: The try/except around calling attr_func() that sets
attribute_dict[f"{value_key}{attr_name}"] currently swallows all exceptions;
change the except to log the exception at debug level instead of passing so
failures are observable. Specifically, inside the except Exception block in
LinuxOSACI.py where attribute_dict, value_key, attr_name, and attr_func are
used, call the module’s logger (or create one if missing) to emit a debug log
with a clear message including the attribute key and the caught exception
information (e.g., exception text/traceback) and then continue preserving the
current behavior (do not re-raise).
In `@osworld_setup/s3/bbon/generate_facts.py`:
- Around line 90-94: Replace the silent catch around opening/reading
trajectory_file with a narrow OSError handler: attempt the with
open(trajectory_file, "r") as f: trajectory_lines = f.readlines() block, and on
failure catch except OSError as e: set trajectory_lines = [] (or ensure it
remains defined) and log the error including trajectory_file and e (use the
module/logger variable you have), rather than using a bare except Exception:
pass, so permission/IO errors are surfaced for debugging.
---
Outside diff comments:
In `@osworld_setup/s1/run.py`:
- Around line 224-228: The except block logs the error and then unconditionally
calls env.controller.end_recording(...), which can raise AttributeError if env
has no controller; update the handler in run.py to check hasattr(env,
"controller") and env.controller is not None before calling
env.controller.end_recording with os.path.join(example_result_dir,
"recording.mp4") so the cleanup is only attempted when a controller exists (use
the same guard approach as in osworld_setup/s3/run_local.py).
In `@osworld_setup/s3/bbon/generate_facts.py`:
- Around line 28-35: The try/except currently swallows all errors when parsing
trajectory_lines[i]; narrow the except to catch json.JSONDecodeError (not
Exception), capture the exception as e, and log the parse failure including the
index i and the offending line (trajectory_lines[i]) and the error message (e)
before continuing; keep the existing behavior that later raises ValueError if
pyautogui_action is None. Ensure you import or use the module logger (e.g.,
logging) and reference the variables and context names shown (trajectory_lines,
pyautogui_action, i) so the log makes it clear this was a JSON parse error
rather than a missing exec_code.
---
Nitpick comments:
In `@gui_agents/s1/aci/LinuxOSACI.py`:
- Around line 414-418: Remove the redundant outer try/except around the call to
find_element in the type() method (and the analogous block in scroll()); instead
directly assign node = self.find_element(element_id) if element_id is not None
else None because find_element already handles IndexError internally—if you
actually intend to catch other unexpected errors, replace the broad except with
a specific exception or add an inline comment in type() and scroll() explaining
which non-IndexError cases you want to guard against.
In `@gui_agents/s1/aci/MacOSACI.py`:
- Around line 308-312: The current broad except Exception around the call to
self.find_element(element_id) (which sets node) should be narrowed to only catch
the expected input/index errors; update the try/except blocks in MacOSACI.py
around the find_element usage (both the block where node =
self.find_element(element_id) and the similar block at the later occurrence) to
catch TypeError, ValueError, and IndexError instead of Exception, leaving other
exceptions to propagate; ensure the except sets node = None as before and keep
the original behavior for valid errors.
In `@gui_agents/s1/aci/windowsagentarena/GroundingAgent.py`:
- Around line 307-314: The find_element method currently swallows all Exceptions
and falls back to self.nodes[0]; replace the broad except with targeted
exception handling for ValueError (int conversion) and IndexError (out-of-range)
so you don't mask other bugs: parse element_id with int inside a try and except
(ValueError, IndexError) only, log or set self.index_out_of_range_flag and
return None (or raise a specific error) instead of defaulting to nodes[0]; apply
the same change to the other action-selection sites mentioned (the similar
blocks at lines corresponding to the other occurrences) by replacing except
Exception with except (ValueError, IndexError) and removing the implicit
fallback to index 0.
In `@gui_agents/s1/aci/WindowsOSACI.py`:
- Around line 303-306: The outer try/except that wraps calls to
self.find_element() in WindowsACI.type() and WindowsACI.scroll() is redundant
and suppresses unexpected errors; remove the surrounding try/except blocks so
that WindowsACI.find_element() (which already handles IndexError) returns
normally or lets other exceptions propagate—specifically, eliminate the
try/except around the node = self.find_element(element_id) if element_id is not
None else None lines in both type() and scroll() so only intentional IndexError
handling stays inside find_element().
In `@gui_agents/s1/cli_app.py`:
- Around line 42-43: The except block in get_char is too broad; narrow it to
only the expected exceptions by catching ImportError, io.UnsupportedOperation
and OSError (the ones raised when termios/tty or stdin.fileno()/termios fail)
instead of Exception, and ensure io is imported so io.UnsupportedOperation is
available; keep the existing fallback return input() for non-terminal
environments.
In `@gui_agents/s1/core/Knowledge.py`:
- Around line 82-86: Replace the broad except Exception in the JSON/cache read
blocks (the try that loads formulate_query from query_path and the similar block
around lines 123-127) with targeted exception handling: catch FileNotFoundError,
json.JSONDecodeError, and OSError and set formulate_query = {} (or the
equivalent fallback) in those cases, and re-raise any other unexpected
exceptions so non-I/O bugs aren’t silently swallowed; reference the
variables/query_path and the formulate_query load logic to locate both spots to
update.
In `@gui_agents/s2_5/cli_app.py`:
- Around line 44-45: The except block in get_char (cli_app.py) is too broad;
narrow the exception handling to only ImportError and OSError (i.e., use except
(ImportError, OSError):) so we catch the expected module/load errors while
letting other exceptions surface, and keep the same fallback return input()
behavior inside that narrowed handler.
In `@gui_agents/s2/agents/agent_s.py`:
- Around line 349-353: Replace the broad except Exception around
json.load(open(reflection_path)) with targeted error handling: open the file
using a context manager (with open(reflection_path, "r") as f) and call
json.load(f), and catch only expected exceptions such as json.JSONDecodeError,
FileNotFoundError, and OSError/PermissionError to set reflections = {} (and
optionally log a warning); let all other exceptions propagate so unexpected bugs
are not silenced. Apply the same change to the other occurrence around lines
385-391 that uses reflection_path.
In `@gui_agents/s2/cli_app.py`:
- Around line 44-45: In the get_char function replace the broad "except
Exception:" fallback with a narrowed except clause "except (ImportError,
OSError):" so only import/OS-related failures fall back to input(); update the
except block that currently returns input() to use these two specific exceptions
(refer to the get_char function and its fallback except block).
In `@gui_agents/s2/core/knowledge.py`:
- Around line 88-93: Replace the blanket "except Exception" in the cache load
blocks so only expected I/O/parse errors are swallowed: catch FileNotFoundError
and json.JSONDecodeError (and optionally PermissionError) around the
open/json.load in the block that assigns formulate_query (and the similar block
around the other cache load at lines 129-133), set formulate_query = {} on those
specific exceptions, and let all other exceptions propagate so unexpected
runtime bugs are not silently ignored.
- Around line 272-275: The current try/except around
load_knowledge_base(self.episodic_memory_path) silently swallows all exceptions
and defaults kb to {}, which can cause unintended overwrites on subsequent
writes; update the block so it only catches expected read/parsing errors (e.g.,
FileNotFoundError, JSONDecodeError or whatever load_knowledge_base raises for
missing/corrupt data) and falls back to {} in those cases, but re-raises or
propagates any other unexpected exceptions so they aren’t hidden. Apply the same
change to the analogous block later (around lines handling the second load) and
reference the load_knowledge_base call and self.episodic_memory_path when making
the change.
In `@gui_agents/s3/bbon/comparative_judge.py`:
- Around line 21-25: The function extract_step_num currently catches all
exceptions; narrow the exception handling to only the expected types by catching
IndexError and ValueError explicitly (i.e., replace except Exception with except
(IndexError, ValueError)) so only parsing/split errors return 0 and other
unexpected exceptions bubble up.
In `@gui_agents/s3/cli_app.py`:
- Around line 45-46: Update the broad except in get_char to only catch the
expected import/terminal failures: replace "except Exception:" with "except
(ImportError, OSError):" (or the same tuple used in gui_agents/s1/cli_app.py) so
only import/OS-level terminal issues fall back to input(); keep the existing
fallback return input() and ensure the change references get_char to locate the
handler.
In `@osworld_setup/s1/run.py`:
- Around line 294-303: The broad except Exception in get_result should be
narrowed to only catch the expected errors: replace the generic except with
except (OSError, ValueError) around the float(open(os.path.join(example_path,
"result.txt"), "r").read()) call so only file/IO and conversion errors are
swallowed; keep appending 0.0 to all_result for those cases and leave other
exceptions to surface. Use the existing all_result and example_path variables
and the get_result function context when making this change.
In `@osworld_setup/s3/run_local.py`:
- Around line 356-365: The try/except in get_result currently swallows all
exceptions; narrow it to only catch I/O and parse errors by replacing the broad
"except Exception" with "except (OSError, ValueError)" around the
float(open(...).read()) read/parse block (and keep appending 0.0 on those
specific failures) so other unexpected exceptions propagate; reference the
get_result logic that builds all_result and the
float(open(os.path.join(example_path, "result.txt"), "r").read()) call when
making the change.
In `@osworld_setup/s3/run.py`:
- Around line 515-524: The except clause in get_result currently catches all
exceptions; narrow it to only catch OSError and ValueError (the expected errors
when opening/reading/parsing result.txt) and leave other exceptions to surface;
locate the try/except around reading os.path.join(example_path, "result.txt")
which appends to all_result and replace "except Exception" with "except
(OSError, ValueError)" so only file I/O and float conversion errors are handled.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
gui_agents/s1/aci/LinuxOSACI.pygui_agents/s1/aci/MacOSACI.pygui_agents/s1/aci/WindowsOSACI.pygui_agents/s1/aci/windowsagentarena/GroundingAgent.pygui_agents/s1/cli_app.pygui_agents/s1/core/AgentS.pygui_agents/s1/core/Knowledge.pygui_agents/s2/agents/agent_s.pygui_agents/s2/agents/grounding.pygui_agents/s2/cli_app.pygui_agents/s2/core/knowledge.pygui_agents/s2_5/agents/grounding.pygui_agents/s2_5/cli_app.pygui_agents/s3/agents/grounding.pygui_agents/s3/bbon/comparative_judge.pygui_agents/s3/cli_app.pyosworld_setup/s1/run.pyosworld_setup/s2/run.pyosworld_setup/s2_5/run.pyosworld_setup/s2_5/run_local.pyosworld_setup/s3/bbon/generate_facts.pyosworld_setup/s3/run.pyosworld_setup/s3/run_local.py
Comment on lines
670
to
673
| try: | ||
| attribute_dict[f"{value_key}{attr_name}"] = str(attr_func()) | ||
| except: | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Silent except Exception: pass discards diagnostic info — add a log statement.
Any failure in attr_func() (e.g., a COM error or unexpected pyatspi error beyond NotImplementedError) is silently swallowed. Ruff S110 correctly flags this. A debug-level log preserves observability without changing behavior.
🔧 Proposed fix
try:
attribute_dict[f"{value_key}{attr_name}"] = str(attr_func())
- except Exception:
- pass
+ except Exception as e:
+ logger.debug("Could not read value attribute %s for node %s: %s", attr_name, node_name, e)📝 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.
Suggested change
| try: | |
| attribute_dict[f"{value_key}{attr_name}"] = str(attr_func()) | |
| except: | |
| except Exception: | |
| pass | |
| try: | |
| attribute_dict[f"{value_key}{attr_name}"] = str(attr_func()) | |
| except Exception as e: | |
| logger.debug("Could not read value attribute %s for node %s: %s", attr_name, node_name, e) |
🧰 Tools
🪛 Ruff (0.15.2)
[error] 672-673: try-except-pass detected, consider logging the exception
(S110)
[warning] 672-672: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gui_agents/s1/aci/LinuxOSACI.py` around lines 670 - 673, The try/except
around calling attr_func() that sets attribute_dict[f"{value_key}{attr_name}"]
currently swallows all exceptions; change the except to log the exception at
debug level instead of passing so failures are observable. Specifically, inside
the except Exception block in LinuxOSACI.py where attribute_dict, value_key,
attr_name, and attr_func are used, call the module’s logger (or create one if
missing) to emit a debug log with a clear message including the attribute key
and the caught exception information (e.g., exception text/traceback) and then
continue preserving the current behavior (do not re-raise).
Comment on lines
90
to
94
| try: | ||
| with open(trajectory_file, "r") as f: | ||
| trajectory_lines = f.readlines() | ||
| except: | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Silent except Exception: pass on trajectory file read loses the I/O error.
If open(trajectory_file) or f.readlines() fails for any reason other than a missing file (e.g., permissions, encoding), the error is silently swallowed and trajectory_lines stays empty, causing all subsequent steps to lack action data. Narrowing to OSError and adding a log line improves debuggability.
🔧 Proposed fix
try:
with open(trajectory_file, "r") as f:
trajectory_lines = f.readlines()
- except Exception:
- pass
+ except OSError as e:
+ print(f"Warning: could not read trajectory file {trajectory_file}: {e}")📝 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.
Suggested change
| try: | |
| with open(trajectory_file, "r") as f: | |
| trajectory_lines = f.readlines() | |
| except: | |
| except Exception: | |
| pass | |
| try: | |
| with open(trajectory_file, "r") as f: | |
| trajectory_lines = f.readlines() | |
| except OSError as e: | |
| print(f"Warning: could not read trajectory file {trajectory_file}: {e}") |
🧰 Tools
🪛 Ruff (0.15.2)
[error] 93-94: try-except-pass detected, consider logging the exception
(S110)
[warning] 93-93: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osworld_setup/s3/bbon/generate_facts.py` around lines 90 - 94, Replace the
silent catch around opening/reading trajectory_file with a narrow OSError
handler: attempt the with open(trajectory_file, "r") as f: trajectory_lines =
f.readlines() block, and on failure catch except OSError as e: set
trajectory_lines = [] (or ensure it remains defined) and log the error including
trajectory_file and e (use the module/logger variable you have), rather than
using a bare except Exception: pass, so permission/IO errors are surfaced for
debugging.
Author
|
Closing per backlog policy; can revisit with narrower scope. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace 42 bare
except:withexcept Exception:across 23 files. Bareexcept:catchesKeyboardInterruptandSystemExit, masking real errors and preventing clean process termination.Summary by CodeRabbit