Skip to content

fix: narrow broad exception handlers to prevent silent error swallowing - #216

Open
RohithPariki wants to merge 1 commit into
microsoft:mainfrom
RohithPariki:fix/narrow-exception-handling
Open

fix: narrow broad exception handlers to prevent silent error swallowing#216
RohithPariki wants to merge 1 commit into
microsoft:mainfrom
RohithPariki:fix/narrow-exception-handling

Conversation

@RohithPariki

Copy link
Copy Markdown

Fixes #215

Summary of Changes

Replaced generic except Exception: blocks with targeted exception types (OSError, ValueError, TypeError, json.JSONDecodeError, KeyError, AttributeError) across core components:

  • skillopt/engine/trainer.py: Catch targeted JSON/IO/attribute errors when reading patch files.
  • skillopt/envs/spreadsheetbench/adapter.py & rollout.py: Replace broad exception handlers with specific JSON/KeyError/TypeError handling during cached results loading.
  • skillopt/gradient/aggregate.py: Replace broad except Exception: with targeted exceptions when parsing LLM merge responses.
  • skillopt_sleep/backend.py: Narrow exception scope across 12 locations (file removal, CLI output parsing, JSONL decoding) so unexpected system/programming bugs are not silently swallowed.

Verification

  • Verified via pytest: 146 core engine tests passed cleanly.

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks for working on this. Most of the file/JSON cleanup changes look reasonable, but the two changes in skillopt/gradient/aggregate.py alter an intentional resilience contract and need adjustment before we can merge this.

Those try blocks cover the chat_optimizer(...) call as well as response parsing. Backend/provider/subprocess failures commonly surface as RuntimeError or backend-specific exceptions, which are not in the new tuple. I reproduced this by making chat_optimizer raise RuntimeError: main returns the documented fallback concatenation, while this branch propagates the exception and aborts the hierarchical merge.

Could you please preserve the fallback for optimizer-call failures while making it observable—for example, keep a broad catch at this boundary and emit a sanitized warning, or introduce/catch a shared backend exception type plus the parsing exceptions? Please also add regression tests for a backend/runtime failure and malformed model output, asserting that the fallback is returned. The narrower cleanup/parser-only catches elsewhere can remain. A full test-suite and Ruff run would also be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code Quality] Silent exception swallowing via broad "except Exception: pass" blocks across the codebase

2 participants