fix: narrow JSON parse exception in analyze_bi.py - #116
Conversation
analyze_bi.py used a bare while parsing JSONL lines, which could mask KeyboardInterrupt and hide malformed input. Catch json.JSONDecodeError explicitly instead. Adds a regression test that loads the script, feeds it a JSONL file with one invalid line, and asserts valid lines are still parsed. Signed-off-by: Andrew White <andrewh@cdw.com>
The module imported nbformat at top level, but nbformat is not a declared dependency, so tests importing the module failed in CI with ModuleNotFoundError. Move the import into build_notebook(), the only function that uses it.
|
|
The regression test previously only checked that malformed JSON lines are skipped. It did not verify the actual bug fix: narrowing the bare to so non-JSON exceptions are no longer swallowed. Add a second test that monkeypatches to raise and asserts the exception propagates through .
|
Repair update: the regression test now verifies the actual fix. In addition to checking that lines are skipped, it asserts that a non-JSON exception () raised inside propagates through instead of being swallowed by the previous bare . This addresses the adversarial audit finding that the original test did not exercise the narrowed exception clause. |
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
Verified the narrowing is real: restoring the bare except: while keeping your tests makes test_parse_all_propagates_non_json_exceptions fail exactly as intended, and the lazy nbformat import move is actually required (nbformat isn't declared in any dependency file, so the module-scope import would break the test in CI). Full suite green with the change. Approving the code.
Two asks before merge:
- Please sign the CLA — merge is held on it.
- Refresh the PR description: it predates the second test and doesn't mention the
nbformatimport move, which is the larger source change.
3 participants
Closing this stale PR. Happy to reopen if the maintainers would like to revisit it.