🧹 Refactor exception handling in manifest-merge to use OSError#39
🧹 Refactor exception handling in manifest-merge to use OSError#39savvides wants to merge 1 commit into
Conversation
Narrowed the scope of exception handling in bin/idstack-manifest-merge. Replaced overly broad `except Exception:` blocks with `except OSError:` for file reading and atomic writing. This improves maintainability and prevents masking of unrelated bugs. Co-authored-by: savvides <1580637+savvides@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors exception handling in bin/idstack-manifest-merge by narrowing broad except Exception blocks to except OSError when reading from stdin, payload files, and manifest files. The reviewer correctly identified that sys.stdin.read() and handle.read() can raise a UnicodeDecodeError (which inherits from ValueError rather than OSError). Restricting the catch block to only OSError would allow decode errors to propagate uncaught and crash the script. The reviewer suggested catching both OSError and UnicodeDecodeError to ensure graceful termination.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try: | ||
| text = sys.stdin.read() | ||
| except Exception as exc: | ||
| except OSError as exc: |
There was a problem hiding this comment.
Since sys.stdin.read() can raise a UnicodeDecodeError if the input stream contains invalid UTF-8 bytes, catching only OSError will allow UnicodeDecodeError (which inherits from ValueError) to propagate uncaught. This would cause the script to crash with a traceback instead of exiting gracefully via die(). Consider catching both OSError and UnicodeDecodeError.
| except OSError as exc: | |
| except (OSError, UnicodeDecodeError) as exc: |
| with open(payload_arg, "r", encoding="utf-8") as handle: | ||
| text = handle.read() | ||
| except Exception as exc: | ||
| except OSError as exc: |
There was a problem hiding this comment.
Since handle.read() can raise a UnicodeDecodeError if the payload file contains invalid UTF-8 bytes, catching only OSError will allow UnicodeDecodeError (which inherits from ValueError) to propagate uncaught. This would cause the script to crash with a traceback instead of exiting gracefully via die(). Consider catching both OSError and UnicodeDecodeError.
| except OSError as exc: | |
| except (OSError, UnicodeDecodeError) as exc: |
| with open(manifest_path, "r", encoding="utf-8") as handle: | ||
| text = handle.read() | ||
| except Exception as exc: | ||
| except OSError as exc: |
There was a problem hiding this comment.
Since handle.read() can raise a UnicodeDecodeError if the manifest file contains invalid UTF-8 bytes, catching only OSError will allow UnicodeDecodeError (which inherits from ValueError) to propagate uncaught. This would cause the script to crash with a traceback instead of exiting gracefully via die(). Consider catching both OSError and UnicodeDecodeError.
| except OSError as exc: | |
| except (OSError, UnicodeDecodeError) as exc: |
1 participant
🎯 What: Replaced overly broad
except Exception:blocks withexcept OSError:for file reading and writing operations inbin/idstack-manifest-merge.💡 Why: Catching
Exceptionis too broad and can mask programming errors (likeTypeErrororNameError) by making them appear as I/O failures. By specifically catchingOSError, only actual filesystem errors trigger the script'sdie()utility.✅ Verification: Ran the full test suite (
./test/test-manifest-merge.sh,./test/smoke-test.sh,./test/integration-test.sh). All tests pass, ensuring no existing behaviors were broken. Code changes were visually reviewed and confirmed viagit diff.✨ Result: Improved code health and maintainability by utilizing more precise exception handling.
PR created automatically by Jules for task 5745360703855924417 started by @savvides