fix: robustness and efficiency improvements #58
Merged
Conversation
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
- Guard _find_paper_by_source_url against missing PAPERS_DIR (#56 finding 1) - Wrap download_source temp file in try/finally for guaranteed cleanup (#56 finding 2) - Prevent duplicate log handlers in _setup_debug_logging (#56 finding 5) - Add tests for temp file cleanup and handler deduplication Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ions - Build O(1) lookup indexes (full path + basename) from single getmembers() call, replacing O(n²) inner loop (#56 finding 3) - Handle basename collisions by prefixing with parent dir name, e.g. figs/plot.png + results/plot.png → plot.png + results_plot.png (#56 finding 7) - Update test to assert both files extracted with distinct contents Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Deduplicate figure extraction by source member to prevent inflated count when same \includegraphics ref appears multiple times - Re-check dest_name uniqueness after parent-dir prefixing - Replace stale log handlers instead of early-return guard, fixing ValueError on closed streams (CliRunner/captured output scenarios) - Add tests for duplicate-ref dedup and stale-handler replacement Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No issues found. Checked for bugs and CLAUDE.md compliance. |
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.
Summary
Address 6 of 7 non-security findings from code audit (issue #56). Finding 4 (O(n) URL check) deferred as acceptable at current scale.
Closes: #56
Changes
_find_paper_by_source_urlagainst missingPAPERS_DIR(finding 1)download_sourcetemp file intry/finallyfor guaranteed cleanup (finding 2)"skill"→"skills", remove nonexistent"prompts"(finding 6a)cli.py.backup, add*.backupto.gitignore(finding 6b)\includegraphicsrefs to same tar memberType of Change
Testing
make check(required)New tests:
TestDownloadSourceTempFileCleanup— verifies temp file cleanup on OSErrorTestSetupDebugLogging— handler dedup + stale handler replacementtest_deduplicates_repeated_includegraphics_refs— same figure ref'd 3× → extracted 1×test_handles_duplicate_figure_names_from_different_dirs— asserts 2 distinct filesChecklist