fix(packages): bound archive downloads and gate them in offline mode

[Krunal-Karena]

What changed

ebuild/packages/fetcher.py (PackageFetcher) — four related download
hardening fixes:

1. Timeout. Downloads used urlretrieve(), which has no timeout, so
   one unresponsive mirror hung ebuild build until the user killed it.
   Downloads now stream through urllib.request.urlopen(..., timeout=...)
   — 30 s default, configurable via PackageFetcher(..., timeout=...).

2. Size cap. Anything over 512 MB is rejected while streaming, so a
   server that lies about (or omits) Content-Length still cannot fill
   the disk.

3. Atomic writes. The archive streams to a .part file that is
   renamed into place only when complete. A connection that dies
   mid-body no longer leaves a truncated archive in the download cache —
   _download() short-circuits on existence, so a partial archive would
   otherwise be served by every later fetch as if it were the real thing.

4. Offline gate. EBUILD_OFFLINE=1 and ebuild update-index --offline already governed index synchronization; archive fetching
   was exempt and hit the network anyway (the CHANGELOG itself noted
   "package archive fetching is not yet offline-gated"). A fetch whose
   archive is not already cached now fails with a message naming the
   missing archive. A cached archive still extracts, so air-gapped
   rebuilds work from a warmed cache.

Why

All four are the same failure shape: the fetcher trusted the network
more than it should. A pin (URL + SHA-256) protects integrity, but
nothing bounded availability (hang) or disk use (runaway response), and
a truncated download silently poisoned the cache. The offline gap was an
acknowledged inconsistency between the two halves of the package
subsystem.

Testing / validation

• 6 new tests in tests/ebuild/test_package_fetcher.py: timeout reaches
  urlopen, oversized download rejected with no files left behind,
  truncated download leaves neither a cache entry nor .part litter,
  offline refusal, offline reuse of a warmed cache, default timeout.
• Existing tests were ported from the removed urlretrieve seam to an
  urlopen stub; no test touches the network.
• 22/22 fetcher tests pass. Full suite: 676 passed, identical to
  master's result except for the 6 new passing tests (see note below).
• ruff check clean on touched files; mypy clean on fetcher.py
  apart from one pre-existing error described below.

Note: a pre-existing failure observed, not touched

PackageRecipe.to_dict() was dropped in #112, but
ebuild/packages/index_sync.py:354 still calls it — so ebuild update-index fails with AttributeError on the first recipe it caches,
and 9 tests in tests/unit/test_index_sync.py fail on current master.
This PR deliberately does not fix it to keep the review surface small;
it is recorded in TASKS.md (T-006) and would make a clean follow-up PR.

[Copilot]

🟡 Changes recommended

There are verified correctness issues in the new/updated codepaths/tests (a non-context-manager urlopen stub and an offline-mode crash when recipe.url is missing).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens PackageFetcher’s archive download path to be safer and more predictable under adverse network conditions, and aligns archive fetching with the project’s offline mode behavior.

Changes:

• Replace urlretrieve() with streaming urllib.request.urlopen(..., timeout=...), add a default timeout, and make it configurable via PackageFetcher(timeout=...).
• Enforce a maximum archive size while streaming and write downloads atomically via a .part file + os.replace().
• Gate network downloads in offline mode and add/port tests; document behavior in the changelog and record a separate known issue in TASKS.md.

File summaries

File	Description
ebuild/packages/fetcher.py	Adds streaming downloads with timeout, size cap, atomic .part writes, and offline gating for uncached archives.
tests/ebuild/test_package_fetcher.py	Ports tests from urlretrieve stubs to urlopen stubs and adds coverage for timeout/size/atomicity/offline behavior.
CHANGELOG.md	Documents the download hardening and offline gating fixes.
TASKS.md	Records the pre-existing PackageRecipe.to_dict() callsite issue as T-006.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[srpatcha]

Review — ebuild#126 "fix(packages): bound archive downloads and gate them in offline mode"

head: dea4fc5 author: Krunal-Karena ci: none reported

Verdict: Four well-motivated hardening fixes to the package fetcher, with tests, a CHANGELOG entry and an honest note about a pre-existing failure it declines to fix. Three of the four work as described — I reproduced the size cap, the .part cleanup and the offline gate. The fourth does not: the rewrite narrows the download's exception handler from except Exception to except (URLError, OSError), and the single most likely mid-body failure, http.client.IncompleteRead, is an HTTPException and escapes as an unhandled traceback. That is the exact scenario the PR is named after.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	ebuild/packages/fetcher.py _download, except (urllib.error.URLError, OSError)	Exception handling narrowed against master, and the gap is on the PR's own headline path. http.client.IncompleteRead and http.client.InvalidURL derive from HTTPException, not OSError — verified: IncompleteRead.__mro__ is (IncompleteRead, HTTPException, Exception, ...). Master caught except Exception and always produced a FetchError. I reproduced both escaping this handler. They then escape the caller too: commands.py:1068, :1261 and :1364 all catch (ResolveError, FetchError, BuildError), so ebuild build against a mirror that closes mid-body exits with a raw Python traceback instead of the diagnostic the module's own Raises: FetchError docstring promises. .part cleanup still works in both cases, so there is no cache poisoning — the defect is the crash, not corruption.	Widen the tuple to (urllib.error.URLError, http.client.HTTPException, OSError, ValueError) and import http.client. ValueError covers InvalidURL's second base and any malformed-URL path. Keeping except Exception with raise ... from e would also be defensible here — this is I/O against an untrusted server, and the function's contract is "everything comes out as FetchError".
2	Medium	tests/ebuild/test_package_fetcher.py, test_truncated_download_leaves_no_cache_entry	The truncation test raises OSError("connection reset by peer") — the one truncation exception that is caught — so the suite is green on a path finding 1 shows is broken. _TruncatedResponse.read is the right seam; it is raising the wrong exception to be evidence for the claim in the docstring ("A connection that dies mid-body must not satisfy the cache").	Add a sibling case raising http.client.IncompleteRead(b"partial", 100), or parametrize the existing one over [OSError("connection reset by peer"), http.client.IncompleteRead(b"partial", 100)]. It fails today and passes with finding 1 fixed.
3	Low	tests/ebuild/test_package_fetcher.py, test_failed_download_leaves_no_partial_archive	The test keeps its name but its body was replaced with a size-cap check (MAX_ARCHIVE_SIZE_BYTES monkeypatched to 4, pytest.raises(FetchError, match="maximum size")), making it a near-duplicate of test_oversized_download_is_rejected a few lines below. The scenario the name describes — a download that errors partway — is now covered only by the test in finding 2. Nothing is lost in coverage terms, but a reader auditing "is the failed-download path tested?" gets a false yes from the name.	Rename to test_oversized_download_is_rejected_before_caching, or restore its original scenario and delete the duplication.
4	Low	ebuild/packages/fetcher.py, from ebuild.packages.index_sync import is_offline	The fetcher now imports from the index synchroniser to obtain an environment predicate. is_offline() reads EBUILD_OFFLINE and nothing else — it has no relationship to index synchronisation — and pulling it in drags yaml, json, time and the whole index_sync module into every import of fetcher. No cycle today (index_sync does not import fetcher), so this is design, not breakage.	Move is_offline() to a neutral module — ebuild/core/config.py or a small ebuild/core/env.py — and have both index_sync and fetcher import it from there. Re-export from index_sync if anything external depends on the current location.
5	Low	ebuild/packages/fetcher.py, MAX_ARCHIVE_SIZE_BYTES vs timeout	Two bounds with two different surfaces: the timeout is a constructor parameter, the size cap is a module constant. The tests have to monkeypatch.setattr a module global to exercise it, which is why it reads as untestable configuration. A 512 MB ceiling is also low for a firmware-adjacent artifact — .efw release images and .emodel AI model packages (§11.1) can plausibly exceed it.	Make it PackageFetcher(..., max_archive_bytes=MAX_ARCHIVE_SIZE_BYTES) alongside timeout, keeping the constant as the default. Same shape, both bounds, and the tests stop patching globals.
6	Low	ebuild/packages/fetcher.py _download, urlopen	urlopen follows redirects, so the recipe.url.startswith("https://") check above it is bypassable — a mirror can 302 an https:// URL to http:// and the body arrives in plaintext. Integrity is not at risk: fetch() refuses any recipe without a checksum and _verify_checksum runs before extraction, so a tampered body is rejected. What leaks is which package is being fetched. Pre-existing (urlretrieve behaved the same), but this PR is the hardening pass.	Optional, and worth a line in the PR body either way. An opener with a redirect handler that rejects a scheme downgrade closes it; say explicitly that the checksum is what carries integrity here.
7	Low	ebuild/cli/commands.py:97	The new gate is reachable only through EBUILD_OFFLINE. --offline exists on update-index (commands.py:1730) but not on build, and PackageFetcher is constructed at :97 with no flag plumbed through, so ebuild build --offline is not a thing a user can type. The PR body describes this accurately — it does not overclaim — so this is a gap to record, not a misstatement.	Follow-up: add --offline to build/add and pass it into PackageFetcher, mirroring how update_index passes offline into is_offline(offline).

Architecture conformance

Conforms. §21 places ebuild in Tier 1 — Foundation; ebuild/packages/fetcher.py is SDK-side dependency acquisition and belongs there. §5.1 holds: the one new intra-repo import (index_sync → finding 4) is lateral within Tier 1, not upward, and nothing here becomes a runtime dependency on a device.

This is the right direction under §9.2, which lists "No mandatory cloud connection" as an SDK design rule, and §14.1, which requires a documented threat model for "boot, update, network and package distribution". Bounding a download and refusing to serve a truncated archive from cache are package-distribution threat-model items, and the pin model in recipe.py (_SHA256_RE, plaintext-http:// refusal) already assumes an untrusted mirror. The extraction side is already hardened — _extract_tar/_extract_zip carry CVE-2007-4559 path-traversal checks with a pre-3.12 fallback — so this PR closes the transport half of a story whose storage half was already done. Nothing in the diff weakens either.

On API compatibility (brief item 8): PackageFetcher.__init__ gains timeout: int = DEFAULT_DOWNLOAD_TIMEOUT_SECONDS, a defaulted keyword — additive, existing call sites unaffected, and commands.py:97 needs no change. The one non-additive detail is that ebuild.packages.fetcher.urlretrieve no longer exists as a module attribute; anything monkeypatching that name breaks. In this repo that is only the fetcher tests, which the PR updates. Worth one line in the PR body so it is stated rather than discovered.

Proposed changes

Smallest sequence, in order:

1. Fix finding 1:

   import http.client
   ...
           except (urllib.error.URLError, http.client.HTTPException, OSError, ValueError) as e:
               raise FetchError(
                   f"Failed to download {recipe.name} v{recipe.version} "
                   f"from {recipe.url}: {e}"
               ) from e

2. Extend the truncation test per finding 2 — parametrize over OSError and http.client.IncompleteRead. Confirm it fails before step 1 and passes after.
3. Rename the test in finding 3.
4. Findings 4-7 are follow-ups, not blockers. Finding 4 in particular should not be folded into this PR — moving is_offline touches index_sync, and mixing a relocation into a behaviour change is what .ai/architect.md warns against ("Restructure and change behaviour in the same commit").

The TASKS.md T-006 entry recording the PackageRecipe.to_dict breakage is the right call, and correctly scoped out. For the author's information: three other open PRs now fix it — #119, #124 and #132 — so T-006 is likely to close without work here. #124's implementation is the one that round-trips losslessly.

Not checked

• pytest — NOT RUN. pytest is not importable on this host, and the local ebuild clone has a dirty working tree, left untouched per the rules of engagement. None of the 6 new tests, nor the ported existing ones, were executed by me. The body's "22/22 fetcher tests pass" and "676 passed" are unverified as pytest runs; I verified the underlying behaviours by driving PackageFetcher directly, as recorded below.
• mypy — NOT RUN. Not installed. The timeout: int annotation and the Optional usage are unchecked. yamllint — NOT RUN. Not installed; no YAML in the diff.
• No network test was performed. Every result below comes from a stubbed urlopen. Real timeout behaviour against a stalled mirror, real redirect following (finding 6), and real Content-Length handling are all unverified — I read the code for those.
• Concurrency not tested. commands.py builds packages concurrently when jobs > 1; the .part filename is derived from the archive name, so two processes fetching the same package share one .part. Within a single process the resolver dedupes, so I judged this not worth a finding — but I did not test it, and the atomicity claim in the CHANGELOG is process-local only.
• CI — no checks reported on fix/package-fetch-hardening; GitHub returns no check runs for this branch. mergeable and mergeStateStatus are both UNKNOWN. Findings 1-3 have no CI evidence behind them; they come from the local runs below.
• I did not verify the body's claim that PackageRecipe.to_dict() was removed specifically in #112.

Verified locally, against a git archive export of head dea4fc55, driving PackageFetcher with a stubbed urlopen:

• ruff 0.16.5 check . → 4 errors, all pre-existing on origin/master. This PR adds none.
• Size cap: FetchError: Archive from https://... exceeds the maximum size..., and no file left under the download dir. Works as claimed.
• Offline gate: FetchError: Offline mode (EBUILD_OFFLINE=1): package x v1 is not in the download cache (...). Works as claimed.
• .part cleanup: no *.part left after any failure path I exercised, including the two uncaught ones. Works as claimed.
• http.client.IncompleteRead raised from response.read() → escapes as IncompleteRead(2 bytes read, 100 more expected), not FetchError. Finding 1.
• http.client.InvalidURL raised from urlopen() → escapes as InvalidURL: nonnumeric port, not FetchError. Finding 1.

---

Automated architecture review of dea4fc55e335 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[srpatcha]

Thanks — this is the right shape (streamed, bounded, atomic rename) and the tests are exactly the ones I'd want. I ran the branch locally: fetcher tests pass, the full suite is unchanged apart from the pre-existing test_index_sync failures, ruff and mypy are clean.

A few things to tighten, none large:

• fetcher.py:162 — the old handler caught Exception; the new tuple misses http.client.IncompleteRead/HTTPException, which are not OSErrors. A chunked body cut short would now surface as a traceback from ebuild build rather than a FetchError (the .part is still cleaned up). Adding http.client.HTTPException restores that.
• • fetcher.py:144-161 — when a server closes cleanly before Content-Length, read() returns b"" and the short file is renamed into place; the checksum check then rescues it. Either compare received to Content-Length when present, or soften the "never leaves a truncated archive" wording in CHANGELOG.md:30.
• • • fetcher.py:81 — is_offline() here only sees EBUILD_OFFLINE; the --offline CLI flag is a parameter to sync() and does not reach fetch(). Worth stating precisely in the CHANGELOG.
• • • • fetcher.py:143 — the /3.0 in the User-Agent is already stale (3.0.1).
• • • • • TASKS.md:18 reuses T-006, which #133 also claims; and please add Signed-off-by.
          Approving with those nits.