Unpack only the .lift member for streaming zip reads - #17
Open
imnasnainaec wants to merge 8 commits into
Open
Conversation
The stats and export commands stream entries and resolve neither companion ranges nor media, so extracting a whole package wrote its audio and writing-system files to a temporary directory for nothing. lift_source() now narrows the write to the single .lift member; load(), validate, and check-media still unpack the full folder, which they need to resolve companions and check media presence. The aggregate uncompressed-size cap guards bytes written to disk, so it now applies to the full extraction only; the per-file cap and the entry count and path-traversal checks apply to both paths, the latter two over the whole listing however much of it gets written. Selecting the .lift moves from globbing the extracted tree to matching archive member names, letting both paths share one rule rather than growing a second near-duplicate. That makes the suffix match case-insensitive on every platform, where globbing resolved a .LIFT member on Windows but not on Linux or macOS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
De-duplicate archive member names before the "multiple .lift files" count, so a package storing one path twice — some writers append a record rather than replace it — still resolves to the single file that extraction, which overwrites, leaves on disk. Report which member overflowed when the streaming path's running cap fires: there the limit bounds one file rather than the package, so the package-wide wording misdescribes it. Document two consequences of matching member names: a .LIFT member's conventional sibling companion does not resolve on a case-sensitive filesystem, since that candidate is derived from the suffix, and narrowing the write leaves one member able to reach the full cap before refusal. Derive the extraction-scope test's temp root from the member's own depth and assert it is the temp directory, so the check cannot silently walk above it if the fixture layout changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The note read as though matching member names case-insensitively created the companion-resolution question. It does not: a case-variant .lift filename raises the same question loaded from a plain folder, and _resolve_ranges is what answers it. Point there instead of describing an outcome this function does not control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0.1.0 has not shipped, so there is no released behavior for an Unreleased entry to be changing — unpacking only the .lift for streaming reads is simply what the first release does. Fold it into the zipped-packages bullet and drop the note about how the member is located, an implementation detail with no released counterpart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The prose around the streaming read used a second word for concepts the package already names: "unpack" beside "extract", "decompression bomb" beside the "zip bomb" the changelog and interop guide use, and "record" for a zip listing entry, which collides with the writer's own records. Settle on one term for each. Replace the shorthand where it stands in for the mechanism: the "aggregate declared-size check" is the up-front check of the whole listing's declared sizes, "the streaming two" are the streaming commands, and media that "dwarfs" the limit is media far larger than it. The test named after the aggregate cap follows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
imnasnainaec
force-pushed
the
zip-read-only-lazy-extract
branch
from
August 12, 2026 19:11
7340c01 to
be718c8
Compare
A streaming read skips the whole-listing size pre-check, since it writes one member rather than the package. That left an honestly declared zip bomb in the .lift to stream up to the 10 GiB cap into the temp directory before being refused; checking that one member's declared size instead refuses it with nothing written. The count as bytes stream still guards a declared size that lies. Resolving the member through getinfo() also collapses a path stored twice in the listing to the single entry extraction would leave on disk, so it is no longer written -- and counted against the cap -- twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
imnasnainaec
marked this pull request as ready for review
August 12, 2026 20:07
imnasnainaec
marked this pull request as draft
August 13, 2026 18:16
jasonleenaylor
left a comment
There was a problem hiding this comment.
Consider the api surface and user documentation detail level with regards to security hardening (probably less is more)
@jasonleenaylor reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).
The changelog and interop guide explained why the guards exist -- hostile archives, zip bombs, a declared size that can lie -- and how the check is split between the declared sizes and the bytes as they stream. None of that changes what a reader does: the limits and the refusal do. Give the two numbers, say a package over either is refused, and say the same of one whose member paths escape the extraction directory. The reasoning stays in _zip.py, beside the constants and the extraction it governs. Keep the streaming narrowing's user-visible consequence, which the cap text carried: extracting only the .lift is what makes stats and export cheap on a media-heavy package, and it is why their limit applies to that one file rather than to the media beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The path a zip read hands back is built from the archive member name rather than found on disk, so the write and the yield share a derivation that nothing checks still agrees. A backslash separator is where the two would come apart: POSIX takes it as one filename and Windows as a directory split, so normalizing it on either side alone leaves the yield pointing at a path nothing wrote. Extract a member stored with that separator and read the .lift's bytes back through the yielded path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
imnasnainaec
force-pushed
the
zip-read-only-lazy-extract
branch
from
August 13, 2026 20:36
8288db4 to
bf34666
Compare
imnasnainaec
marked this pull request as ready for review
August 13, 2026 20:41
imnasnainaec
commented
Aug 13, 2026
imnasnainaec
left a comment
Collaborator
Author
There was a problem hiding this comment.
User doc details adjusted/reduced.
User story added to pr body to justify api surface.
@imnasnainaec reviewed all commit messages and made 1 comment.
Reviewable status: 2 of 5 files reviewed, all discussions resolved (waiting on jasonleenaylor).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
statsandexportstream entries and resolve neither companion ranges nor media, but reading a zipped package extracted the whole archive first — writing a media-heavy package's audio andWritingSystems/to a temp directory that nothing then read. On a real audio-carrying project that is gigabytes of pure overhead per invocation.lift_source()now narrows the write to the single.liftmember.load(),validate, andcheck-mediastill unpack the full folder, which they need to resolve companions and check media presence.User story
A Combine or FieldWorks project backup is a
.zipwhose shape is lopsided: single-digit MB of.liftXML beside gigabytes of audio and images. A project manager receiving those backups runssil-lift stats backup.zip --format jsonon each one — in CI, or in a triage script over a folder of them — to track entry and sense counts and catch a truncated export before it is imported. Counting reads only the XML.Before this change, every invocation wrote the whole package to a temp directory, read a few MB of it, and discarded the rest. On a CI runner with bounded temp disk, a handful of those in sequence is a real failure mode. Now
statswrites one file.exporthas the same shape — pulling a TSV of glosses out of a package to hand to a reviewer reads the XML and nothing else.Guards
statson a package whose media dwarfs the limit now succeeds whereload()still refuses it. Deliberate, and covered by a test.Member selection
Locating the
.liftmoves from globbing the extracted tree to matching archive member names, so both paths share one rule instead of the change growing a second near-duplicate;_find_lift_root()is gone.That makes the suffix match case-insensitive on every platform. Previously it inherited
Path.rglob's platform behavior, so a.LIFTmember resolved on Windows but not on Linux or macOS.Verification
python scripts/check.pygreen (382 passed, 94% total,_zip.pyat 100%), plusmkdocs build --strictfor the docs edits. New tests cover the extraction narrowing directly — asserting the temp tree holds the.liftand nothing else — along with the traversal and per-file cap paths in streaming mode and the deliberate aggregate-cap divergence.Draft: opening for review of the guard trade-off above before marking ready.
🤖 Generated with Claude Code
Devin review: https://app.devin.ai/review/sillsdev/python-sil-lift/pull/17
This change is