Repository navigation
Fix --resume manifest truncation bug - #2
Conversation
There was a problem hiding this comment.
Summary
This PR successfully fixes the manifest truncation bug by hoisting ManifestMeta construction and adding write_meta() in the resume path. The test coverage expansion from 4 to 8 tests is excellent and validates the fix.
Key Changes:
- ✅ Hoisted
ManifestMetaconstruction above resume/fresh-scan branches - ✅ Added
write_meta()call in resume path to update header without truncation - ✅ Comprehensive test coverage for resume scenarios
Review Items:
- 1 comment regarding metadata timestamp behavior during resume operations
All tests pass and the core bug fix is correct. Please review the comment regarding created_at timestamp handling.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| meta = ManifestMeta( | ||
| input_dir=os.path.abspath(input_dir), | ||
| total_files=total_discovered, | ||
| created_at=datetime.now(timezone.utc).isoformat(), | ||
| settings={ | ||
| "workers": config.workers, | ||
| "include_hashes": config.include_hashes, | ||
| "skip_pixel_stats": config.skip_pixel_stats, | ||
| "artifact_threshold": config.artifact_threshold, | ||
| "dark_threshold": config.dark_threshold, | ||
| "overexposed_threshold": config.overexposed_threshold, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🛑 Logic Error: Setting created_at to current time on resume overwrites the original scan creation timestamp. During resume, the metadata should preserve the original created_at from the existing manifest to maintain accurate scan history. Currently, every resume resets this timestamp to "now", losing the original creation time.
| meta = ManifestMeta( | |
| input_dir=os.path.abspath(input_dir), | |
| total_files=total_discovered, | |
| created_at=datetime.now(timezone.utc).isoformat(), | |
| settings={ | |
| "workers": config.workers, | |
| "include_hashes": config.include_hashes, | |
| "skip_pixel_stats": config.skip_pixel_stats, | |
| "artifact_threshold": config.artifact_threshold, | |
| "dark_threshold": config.dark_threshold, | |
| "overexposed_threshold": config.overexposed_threshold, | |
| }, | |
| ) | |
| meta = ManifestMeta( | |
| input_dir=os.path.abspath(input_dir), | |
| total_files=total_discovered, | |
| created_at=datetime.now(timezone.utc).isoformat(), | |
| settings={ | |
| "workers": config.workers, | |
| "include_hashes": config.include_hashes, | |
| "skip_pixel_stats": config.skip_pixel_stats, | |
| "artifact_threshold": config.artifact_threshold, | |
| "dark_threshold": config.dark_threshold, | |
| "overexposed_threshold": config.overexposed_threshold, | |
| }, | |
| ) | |
| if config.resume and not config.force and output.exists(): | |
| processed_set, existing_records = load_processed_set(output_path) | |
| already_processed = len(existing_records) | |
| pending = filter_pending(all_images, processed_set) | |
| # Preserve original creation timestamp during resume | |
| from imgeda.io.manifest_io import read_manifest | |
| existing_meta, _ = read_manifest(output_path) | |
| if existing_meta and existing_meta.created_at: | |
| meta.created_at = existing_meta.created_at | |
| # Update metadata header without truncating existing records | |
| write_meta(output_path, meta) |
1db0352 to
49cdda6
Compare
The resume code path in runner.py never called write_meta() to update the manifest header, and the ManifestMeta construction was buried in the else (fresh-scan) branch. Hoisted meta construction above the if/else and added a write_meta() call in the resume branch so the metadata header is atomically updated while preserving all existing image records. Also expanded pipeline tests from 4 to 8 covering: no-duplicate appends on resume, record preservation, partial-run resume, force truncation with fresh timestamps, resume with nonexistent manifest, and metadata refresh on resume. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
49cdda6 to
191dfa7
Compare
Summary
runner.pynever updated the manifest metadata header —ManifestMetawas only constructed in the fresh-scan branch. On resume, the header was stale and records could be lost ifcreate_manifest()was reached instead ofwrite_meta().ManifestMetaconstruction above the if/else and added awrite_meta()call in the resume branch to atomically update the header while preserving all existing image records.Test plan
uv run pytest— all 69 tests passuv run ruff check src/ tests/— cleanuv run mypy src/imgeda/— clean🤖 Generated with Claude Code