You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Repair issue #38 by enforcing one commandF-owned compressed package archive size limit across both public registry acquisition and local-mirror acquisition.
Base at branch creation:
main: eeecb0bc03c7040bb18b70bce8b69d618384f783
Current head:
a7dbda4eac62adf8aaff15519ac3876ce4828d74
This PR is Draft for early qualification only; do not merge ahead of the current cache-integrity repair sequence without re-reading live main and reconciling dependencies.
Repair
define MAX_PACKAGE_ARCHIVE_BYTES = 128 MiB once in package-source code;
registry acquisition continues using the same 128 MiB bound, now via the shared constant;
LocalMirrorSource::archive opens the selected package and reads at most MAX + 1 bytes instead of fs::read allocating the whole caller-controlled file;
oversize input fails closed with a stable InvalidRequest classification;
package-not-found behavior and source/provenance semantics remain unchanged.
Regression
The bounded reader is generic over Read, allowing cheap unit tests that prove:
exact bound accepted;
bound+1 rejected;
without allocating a 128 MiB fixture in CI.
The existing ignored real-registry smoke expectations are preserved unchanged.
Before Ready/merge: reconcile against live canonical main, run all exact-head mandatory/path-applicable workflows, obtain fresh Qodo + CodeRabbit review truth, and require zero unresolved substantive findings.
Fixes issue #38 by applying the same 128 MiB compressed archive size limit to local mirror reads that the public registry already enforced. Oversized local archives now fail closed with InvalidRequest instead of being fully read into memory.
Extracts the archive limit into a shared constant used by both registry and local mirror acquisition.
Adds a bounded reader that is generic over Read, so unit tests verify the exact-limit and over-limit behavior without allocating a 128 MiB fixture.
Package-not-found behavior and source/provenance semantics are unchanged.
Written for commit a7dbda4. Summary will update on new commits.
Please perform an early review of exact Draft head a7dbda4eac62adf8aaff15519ac3876ce4828d74. Focus on whether the 128 MiB limit remains identical for registry and local mirror, whether local reads are truly bounded before full allocation, preservation of PackageNotFound/source semantics, correctness of the small-limit regression seam, and accidental changes outside #38. This is early review only, not merge qualification.
Please perform an early review of exact Draft head a7dbda4eac62adf8aaff15519ac3876ce4828d74. Focus on whether the 128 MiB limit remains identical for registry and local mirror, whether local reads are truly bounded before full allocation, preservation of PackageNotFound/source semantics, correctness of the small-limit regression seam, and accidental changes outside #38. This is early review only, not merge qualification.
Early review of draft a7dbda4
I found no correctness issue in the supplied diff for the requested scope; no findings to report.
Shared 128 MiB limit:MAX_PACKAGE_ARCHIVE_BYTES is defined once as 128 * 1024 * 1024 in source.rs:9 and imported by registry.rs:8-10. Registry downloads apply it at registry.rs:148-153; local mirrors apply the same constant at source.rs:99. The accepted maximum is therefore identical.
Local-read bounding:source.rs:103-113 opens the file first and reads through Take(max_bytes + 1), so it can consume at most 128 MiB plus one probe byte—not the complete file—before returning InvalidRequest. It no longer uses fs::read, which would allocate the entire archive.
Not-found and source semantics: The File::open error mapping at source.rs:89-98 preserves the prior PackageNotFound behavior for missing archives and Io for other open failures. source_id() and registry PackageArchive.source behavior are unchanged.
Regression seam:read_bounded_archive accepts an injected max_bytes; the tests at source.rs:122-132 cover exactly-at-limit success and limit-plus-one failure without requiring a 128 MiB fixture. The saturating_add(1) is also safe for the helper’s arbitrary test inputs.
Scope: The diff is limited to the shared archive-limit extraction and local bounded reading in source.rs, plus registry reuse in registry.rs; I found no accidental changes outside that scope.
One minor nuance: the local path intentionally reads one byte beyond the limit to distinguish an exact-limit file from an oversized one, so its failure path may transiently hold 128 MiB + 1 byte. That does not change the 128 MiB accepted limit or permit full-file allocation.
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
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.
Scope
Repair issue #38 by enforcing one commandF-owned compressed package archive size limit across both public registry acquisition and local-mirror acquisition.
Base at branch creation:
Current head:
This PR is Draft for early qualification only; do not merge ahead of the current cache-integrity repair sequence without re-reading live main and reconciling dependencies.
Repair
MAX_PACKAGE_ARCHIVE_BYTES = 128 MiBonce in package-source code;LocalMirrorSource::archiveopens the selected package and reads at mostMAX + 1bytes instead offs::readallocating the whole caller-controlled file;InvalidRequestclassification;Regression
The bounded reader is generic over
Read, allowing cheap unit tests that prove:without allocating a 128 MiB fixture in CI.
The existing ignored real-registry smoke expectations are preserved unchanged.
Non-goals
Qualification
Before Ready/merge: reconcile against live canonical main, run all exact-head mandatory/path-applicable workflows, obtain fresh Qodo + CodeRabbit review truth, and require zero unresolved substantive findings.
Refs #38.
Summary by cubic
Fixes issue #38 by applying the same 128 MiB compressed archive size limit to local mirror reads that the public registry already enforced. Oversized local archives now fail closed with
InvalidRequestinstead of being fully read into memory.Read, so unit tests verify the exact-limit and over-limit behavior without allocating a 128 MiB fixture.Written for commit a7dbda4. Summary will update on new commits.