Skip to content

Download external resources for archive content types (#233)#690

Open
rtibblesbot wants to merge 6 commits into
learningequality:mainfrom
rtibblesbot:issue-233-05acfa
Open

Download external resources for archive content types (#233)#690
rtibblesbot wants to merge 6 commits into
learningequality:mainfrom
rtibblesbot:issue-233-05acfa

Conversation

@rtibblesbot

@rtibblesbot rtibblesbot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Archive-based content types (HTML5 zip, H5P, EPUB/KPUB/Bloom) can reference external URLs — images, videos, fonts, stylesheets — that do not resolve in offline Kolibri deployments. The archive conversion handlers now scan archive contents for external references, download the resources into the archive, and rewrite the references to the local copies before the archive is sealed by create_predictable_zip.

The URL detection and rewriting logic lives in a new module (references.py) with no HTTP or filesystem coupling, so it is unit-testable directly. It is organized as per-file-type ReferenceMapper classes — HTML and CSS by default, plus an H5P-specific mapper — modeled on kolibri-zip's per-file-type mappers. HTML rewriting is surgical: a stdlib HTMLParser walk collects reference spans and _apply_edits splices in the replacements, leaving the rest of the document byte-for-byte.

A utils/ utility (archive_assets.py) drives the archive-level work. ArchiveProcessor takes an extracted directory and makes two passes: a reference-led walk that routes every external reference through the running file pipeline (download → convert), places the result next to its referencing file and rewrites the reference; then a scoped pass that recompresses every media file it can. The base archive handler declares the HTML and CSS mappers (DEFAULT_MAPPERS), so every archive format is scanned; H5P extends that set with an H5P-specific mapper for its content.json references.

Each reference is processed through the running pipeline — the handler reaches it via a new Handler.get_pipeline() that walks up the parent chain — so an archive asset gets the same download → convert path, config and caches as every other file, with no module-global stage. Routing archive downloads through the pipeline made the old downloader.py redundant: it is deleted (~1000 lines), and its two remaining live callers — the module-level read() and the YouTube thumbnail fetch — are migrated onto the pipeline (read() now wraps DownloadStageHandler, and the thumbnail is transcoded by the CONVERT stage from its actual bytes instead of Content-Type sniffing). The loadjs selenium/pyppeteer render path is intentionally dropped as a follow-up.

Compression is no longer driven by a global flag: config.COMPRESS is removed, and SushiChef.run derives explicit ffmpeg settings from --compress and threads them through the pipeline's new default_context (folding in #673), so standalone and in-archive media compress consistently.

References

Closes #233. Supersedes #303 and #673. Carries forward the CSS fixes from #636 and #639. Follow-up filed as #692 (rename the utils.utils module).

Reviewer guidance

  • ricecooker/utils/references.py:212 (_map_html_urls) — confirm the walk catches every reference form real HTML5 apps use (src, link[href], srcset, inline style, <style> blocks) so none are silently missed.
  • ricecooker/utils/archive_assets.py:145 (_fetch_into) — assets are co-located with their referrer under a content-hashed basename; confirm that cannot collide across files sharing a directory.
  • ricecooker/utils/pipeline/convert.py:215 (handle_file) — the archive unzips into a tempfile.TemporaryDirectory(); confirm nothing downstream retains a path into that dir past the with block, so untrusted content cannot leak.

AI usage

Used Claude Code to implement a pre-approved plan with test-driven development, writing the pure reference utilities, the archive processor, and the handler integration, then addressing review feedback across several rounds. Verified with the pipeline, references and files test suites and prek lint.


@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?
  • Ran pre-flight CI checks (lint, format, tests) and verified all pass
  • Reorganized commit history into clean, logical commits
  • Audited the diff to ensure only issue-relevant files are changed
  • Built PR body from the repository's PR template with evidence blocks

@rtibblesbot
rtibblesbot marked this pull request as ready for review July 11, 2026 02:15

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be completely rearchitected, following the exemplar referenced in the issue of the kolibri-zip module in the learningequality/kolibri repository.

Only the H5P handler needs to know anything about H5P so it should be the only thing concerned with this. Use file type specific mappers and an extensible architecture to make this properly.

Comment thread ricecooker/utils/pipeline/convert.py Outdated
class ArchiveProcessingBaseHandler(ExtensionMatchingHandler):
CONTEXT_CLASS = ArchiveProcessingContextMetadata

# Per-handler opt-in for downloading external resource references and

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... if it's meant to be opt in - why have we defined it on the base class? Why does the base class know anything about "H5P_JSON"? That should be delegated to the inheriting classes not encoded on the base class.

Did you look at the kolibri-zip implementation at all? It has a clear architecture for allowing opt in and separation of concerns here. You've just bunged everything in the base class and called it a day.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call — the base class no longer knows anything about H5P. I followed kolibri-zip's Mapper design:

  • Added a ReferenceMapper abstraction — each file type gets a mapper that knows which files it handles(), what references they extract(), and how to rewrite() them.
  • references.py now exposes only the generic web mappers (HTMLMapper, CSSMapper) as DEFAULT_MAPPERS.
  • H5PContentMapper — and all knowledge of H5P, including the content/content.json path and path-key walking — lives with the H5PConversionHandler in convert.py. It opts in via REFERENCE_MAPPERS = DEFAULT_MAPPERS + (H5PContentMapper(),).
  • The base handler now carries only REFERENCE_MAPPERS = () and the archive processor dispatches through mapper.handles/extract/rewrite, so it is fully format-agnostic. The SCAN_HTML/SCAN_CSS/SCAN_H5P_JSON flags are gone.

New file types are now supported by supplying a mapper, not by editing the base class or the processor. Done in 2b8d02f.

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline imports

Comment thread tests/pipeline/test_convert.py Outdated
shutil.rmtree(out_dir, ignore_errors=True)

def test_h5p_json(self):
from ricecooker.utils.pipeline import archive_assets

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clean up all unnecessary inline imports.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 69a10b0. Hoisted every inline import in this file to the module-level import block — archive_assets (was duplicated across four tests), config, EPubFile, and the BloomConversionHandler/EPUBConversionHandler/KPUBConversionHandler imports. None had a circular-import justification. uvx prek run --all-files and the full tests/pipeline/test_convert.py suite (28 passed, 2 skipped) are green.

return None


_CSS_URL_RE = re.compile(r"url\(['\"]?(.*?)['\"]?\)")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am rather surprised most of this file hasn't been eliminated and just turned into a thin wrapper around the new logic. Any reason why not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

download_static_assets/ArchiveDownloader are a live web scraper: they resolve refs against a base_url with urljoin, recurse into <a>/<iframe> per link_policy, optionally run JS via selenium, and apply js/css middleware hooks. That is a materially different operation from offline archive rewriting, so it cannot collapse into a thin wrapper around archive_assets. The part the two genuinely share — CSS/HTML reference detection — is already centralized: the regexes live in references.py and downloader imports them. Migrating its inner extract/rewrite functions fully onto the references.py mappers is the #303 refactor over untested legacy scraping paths; I kept it out of this PR to avoid regressing there, and am happy to do it as a focused follow-up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in edab23cdownloader.py is deleted, not wrapped. Its scraping code (download_static_assets, ArchiveDownloader, archive_page, make_request) had no in-repo production callers; the only live entry points — read (html_writer, pdf) and get_archive_filename/thumbnail (chefs) — now go through the pipeline. read resolves via the DOWNLOAD stage; the YouTube thumbnail runs the full pipeline (download + webp→png detected from content, not extension). references.py's mappers are the single detector now, and tests/test_downloader.py → tests/test_references.py.

One heads-up: ricecooker.utils.downloader was a public util, so downstream chef scripts importing it directly will break on the hard delete. Flagging in case you'd rather ship a thin deprecation shim first.

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to have just emulated existing awful code from the downloader (without even deleting much from the module), without taking advantage of any of the affordances of the new pipeline to achieve this in a much more streamlined and consistent way.

If we pass all files through the pipeline, we get free download, file conversion etc - in a completely consistent manner.

from urllib.parse import urlparse

from ricecooker.config import LOGGER
from ricecooker.utils.downloader import make_request

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we using some crappy "make_request" function here, when we literally have an entire pipeline that is dedicated to getting files from their source reference and properly converting them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5947ff7. External references now go through the pipeline's download stage instead of make_request: download_and_rewrite_external_refs builds a DownloadStageHandler and calls .execute(url) per external ref, so archive assets get the same source handling as every other file the pipeline fetches (plain URLs, YouTube, Google Drive, base64, file://…) — with retries, caching and content-hashed storage naming for free. make_request is gone from the module.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseded by 3d71585. Beyond dropping make_request, external refs now go through the whole running pipeline (DOWNLOAD → CONVERT), not just the download stage — so they get the same conversion (media compression, image transcode) as every other file. Full write-up on the archive_assets.py:96 thread.

def _process_file(self, source_path, mapper):
"""Download ``source_path``'s external refs and rewrite them in place."""
try:
with open(source_path, encoding="utf-8") as fh:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy file? File pipeline does this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5947ff7. The hand-rolled fetch-and-write-bytes is gone — the download stage writes each resource to storage under a content-hashed name, and we shutil.copyfile it into the archive's _static/ dir (the pipeline writes to the global storage dir, so the single copy into the archive tree — which is what actually gets zipped — is unavoidable). The open(source_path) still on this line is not a copy: it reads a text file (HTML/CSS/JSON) so its references can be rewritten in place, which is the one thing the file pipeline does not do.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still one shutil.copyfile in 3d71585, and it is unavoidable: the pipeline writes each processed resource into the global storage dir under a content hash, and we copy that one file into the archive tree (next to its referencing file) because that tree is what gets zipped. No pipeline affordance writes into an arbitrary target dir. The hand-rolled fetch-and-write-bytes and make_request are gone.

Comment thread ricecooker/utils/pipeline/convert.py Outdated

try:
# Create partial for reading & compressing subfiles
file_converter = partial(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even this code would be better handled by just running every file through the file pipeline (or a reduced version of it).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5947ff7. _read_and_compress_archive_file no longer hand-rolls the compression — it writes the subfile to a temp path and runs it through ConversionStageHandler.execute, i.e. the same VideoCompressionHandler/AudioCompressionHandler used for standalone media. Together with the download-stage change, external and internal archive media now flow through one conversion path. I kept the mp4/webm/mp3 gate so non-media subfiles pass through untouched — routing every subfile through the full convert stage would, e.g., reconvert in-archive images to PNG, a behaviour change beyond this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 3d71585. _read_and_compress_archive_file no longer hand-rolls compression and the _get_conversion_stage() singleton is gone: internal archive media is routed through the running CONVERT stage via Handler.parent — the same stage the archive itself is in — and external refs go through DOWNLOAD → CONVERT the same way. Both internal and external media now flow through one pipeline path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One transparent divergence to flag: _read_and_compress_archive_file still exists as a thin delegator (writes the subfile out, re-enters the running CONVERT stage, returns the result) rather than being removed in favour of a purely reference-driven walk. Reason: the whole-archive create_predictable_zip pass compresses every media subfile, whereas a reference-only walk would compress just the statically-referenced ones. That is the same "static reachability under-approximates, so don't prune" argument you made for keeping files — media pulled in dynamically (JS, H5P libraries) is reachable at runtime but invisible to a static ref scan, so compressing only referenced media would silently skip it. So the whole-archive compression stays, but now goes through the running pipeline via Handler.parent with no bespoke logic or global. Happy to collapse it further if you'd prefer the reference-only semantics despite that gap.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ArchiveProcessor should be doing two passes - one a reference led walk to ensure all referenced assets are reachable, and another to ensure all assets that can be compressed are compressed.

Basically, I think we should be consolidating as much as we can onto the archive processor, which just takes a directory as input, processes all references, and compresses all media files that it can - then hand it off to create_predictable_zip which just concerns itself with compressing the directory into a new archive.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 804d07a. ArchiveProcessor.process() now runs two passes over the directory: (1) a reference-led walk that fetches every external ref through the running pipeline so referenced assets are reachable, then (2) a scoped conversion pass that recompresses every media file it can, in place. _read_and_compress_archive_file and the file_converter callback are gone; create_predictable_zip(temp_dir) now only zips the finished directory into a new archive.

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rework needed — rearchitect against the design in #233

Design of record: #233 (this comment). This is a rearchitecture, not review tweaks — the pipeline-for-all-files direction was right, but the execution built parallel paths and left correctness unverified.

Concrete changes:

  • Make references.py the only detector, a faithful port of kolibri-zip's fileUtils.js mappers — not a second one alongside download_static_assets.
  • Delete downloader.py rather than importing its regexes.
  • Migrate its live callers to the pipeline: read (html_writer, pdf), get_archive_filename/thumbnail (chefs).
  • Drop the _get_conversion_stage module-global.
  • Recurse into the running FilePipeline via Handler.parent instead.
  • Route each referenced file through DOWNLOAD → CONVERT — archive media compression now happens there.
  • Remove _read_and_compress_archive_file.
  • Replace the str(soup) HTML round-trip with surgical span-replacement.
  • Place H5P content.json externals under content/ with a content-relative path.
  • ../_static/ will not resolve against H5P.getPath.
  • Run the mappers for every archive format, including EPUB/KPUB/Bloom.
  • Walk all files rather than assuming index.html as the entry point.
  • That keeps compatibility with #672's denesting and non-root entry points.

The three "reviewer, please confirm" items are correctness questions, not review asks — resolve them against kolibri-zip / H5PRunner, don't defer.

def __init__(self, temp_dir, mappers, download_stage, assets_dir):
self.temp_dir = temp_dir
self.mappers = mappers
self.download_stage = download_stage

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why just the download stage? What if the file needs to be converted? Why aren't we just reusing most of the pipeline here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3d71585 (with 129e710 and edab23c) — reworked to the design in the issue, not patched.

Each referenced file now goes through the running FilePipeline (DOWNLOAD → CONVERT), reached via Handler.parent, not a bespoke stage. An external ref is downloaded and converted — media compressed, incompatible images transcoded — then rewritten to that processed output, in lockstep. The download-stage-only fetch and the _get_conversion_stage() singleton are both gone; internal archive media re-enters the same running CONVERT stage the same way, so _read_and_compress_archive_file no longer hand-rolls anything.

The rest of the rework:

  • references.py is the sole detector — a faithful port of kolibri-zip's per-file-type mappers (CSS url()/@import; DOM src/stylesheet href/srcset/inline + <style>). downloader.py is deleted (edab23c), not imported; its live callers — read (html_writer, pdf) and the YouTube thumbnail (chefs) — move to the pipeline.
  • HTML rewriting is now surgical span-replacement (129e710), not str(soup), which was corrupting third-party apps.
  • H5P content.json externals land inside content/ with a content-relative path — the only form H5P.getPath resolves (../_static/ would not).
  • Every archive format (HTML5/EPUB/KPUB/Bloom) runs the web mappers; H5P adds its content.json mapper. Ref processing walks all files, not just index.html.

Correctness items resolved against source, not deferred: kolibri-sandbox's H5PRunner keys contentPaths by the path relative to content/, so the asset must live under content/ — pinned by test_h5p_json. Dropped from this PR: a headless JS-render DOWNLOAD handler (the old read(loadjs=…) path), which I'll file as a follow-up.

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea why it was deemed necessary to write multiple essays for module level doc strings, class, and method doc strings.

You should be exercising the /terse-writing discipline for this technical writing - not writing extraordinarily verbose code comments.

@@ -0,0 +1,221 @@
"""Download external resources referenced by an archive and rewrite the refs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Difficult for me to review this at the moment, because the code comments are so extraordinarily long, doing a reasonable review is challenging.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in fd6366c. Ran the /terse-writing discipline over the docstrings and comments this PR added. The archive_assets.py module docstring drops from ~40 lines to 16; its class/method docstrings and the per-file/per-asset comments are cut to the load-bearing facts. Same pass over references.py, convert.py, transfer.py and chefs.py. No behavior change — lint clean, tests/test_references.py + tests/pipeline/test_convert.py green (36 passed).

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better, much more readable - I still think we are still deferring too much to the previous architecture, which is making things more convoluted than they ought to be.

The basic proposal is, the archive handler handles:

  • Unzipping the archive into a temporary directory
  • Passing to the ArchiveProcessor (perhaps it's actually just a DirectoryProcessor?) to handle all recursive pipeline operations (full pipeline pass + scoped conversion pass)
  • Running create predictable zip to recreate the archive file


Returns the extracted temp directory. **The caller owns cleanup.**
"""
if pipeline is None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove this - just make pipeline a required argument - its only caller already passes the pipeline in, so this optionality is just extra confusion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 804d07a. pipeline is now a required positional argument on ArchiveProcessor (the wrapper that carried the pipeline=None default and the fresh-FilePipeline fallback is gone). The only caller — handle_file — passes self._running_pipeline() in.

Comment thread ricecooker/utils/pipeline/convert.py Outdated
# get a fresh temp dir; the rest zip the path as-is.
temp_dir = None
if self.REFERENCE_MAPPERS:
temp_dir = download_and_rewrite_external_refs(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we are returning a directory of files here - this means that all of our code below has to operate either on a zip file, or on a temporary directory path.

We should own creating the temp dir inside handle_file, then apply the _ArchiveProcessor to it. The function itself is a pointless wrapper. The ArchiveProcessor should be the public API.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 804d07a. download_and_rewrite_external_refs is gone. handle_file now owns the temp dir: it unzips the archive into a mkdtemp(), applies ArchiveProcessor to that directory, then hands the directory to create_predictable_zip. Nothing downstream branches on zip-vs-directory anymore, and ArchiveProcessor is the public API.

Comment thread ricecooker/utils/pipeline/convert.py Outdated

try:
# Create partial for reading & compressing subfiles
file_converter = partial(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ArchiveProcessor should be doing two passes - one a reference led walk to ensure all referenced assets are reachable, and another to ensure all assets that can be compressed are compressed.

Basically, I think we should be consolidating as much as we can onto the archive processor, which just takes a directory as input, processes all references, and compresses all media files that it can - then hand it off to create_predictable_zip which just concerns itself with compressing the directory into a new archive.

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It keeps on getting better - still some more cleanup to do, but we're on a clear trajectory towards success.

When you have done the clean up here, please make sure to reorder the commit history - it's currently a very long flip flopping mess, so we want to turn it into a sensible description of the current state of work. Also update the PR body if it is out of date.

# Media the pipeline can recompress in place without changing the container
# (and thus without invalidating references to it); everything else is passed
# through untouched, since a changed extension would break its references.
MEDIA_EXTENSIONS = {file_formats.MP4, file_formats.WEBM, file_formats.MP3}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not infer this, rather than hardcoding it? We have the explicit metadata for this, so we can avoid creating a separate source of truth here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (in d336518). Dropped the hardcoded MEDIA_EXTENSIONS set — media_extensions now reads the recompressable containers straight off the CONVERT stage's own media handlers (VideoCompressionHandler.SUPPORTED_VIDEO_EXTS / AudioCompressionHandler.SUPPORTED_AUDIO_EXTS). Single source of truth: anything the stage would re-encode to a new extension (e.g. gif→png), breaking references to it, declares no such set and is skipped.

@@ -0,0 +1,203 @@
"""Process an extracted archive in place: download external refs, compress media.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this doesn't actually define handlers or a stage in the pipeline, I think I would rather this still live under the general utils module, rather than specifically in the pipeline (this is also suggested by the fact that the mappers/references live in the utils module directly too).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — moved ricecooker/utils/pipeline/archive_assets.pyricecooker/utils/archive_assets.py, alongside references.py. It defines neither a handler nor a stage, so it no longer lives in the pipeline package. convert.py imports it lazily inside handle_file (the module depends on the pipeline's exceptions, so a top-level import would be circular).

from ricecooker.utils.pipeline.exceptions import InvalidFileException
from ricecooker.utils.references import DEFAULT_MAPPERS
from ricecooker.utils.references import is_external_url
from ricecooker.utils.utils import extract_path_ext

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utils.utils is such a stupid module path - we should file a follow up issue to clean this up and make things more sensible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed as #692 (#692) — rename the ricecooker.utils.utils module. Kept out of this PR to avoid unrelated churn across every import site.

def process(self):
"""Download external refs, then compress media, in place."""
self._download_external_refs()
if config.COMPRESS:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR: #673

We remove the global COMPRESS flag. Could you bring those changes into this PR as a discrete commit, so we can avoid making more churn to clean this up? We can then close that PR in preference to this one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in c57a9da, as a discrete commit. config.COMPRESS is removed; SushiChef.run derives explicit ffmpeg settings from --compress and threads them through the pipeline's new default_context, so handlers compress only when settings are provided (standalone and in-archive media alike). #673 can be closed in preference to this.

Comment thread ricecooker/utils/pipeline/convert.py Outdated
through the same stages, config and caches as every other file.
"""
# Imported lazily: ricecooker.utils.pipeline imports this module.
from ricecooker.utils.pipeline import FilePipeline

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would it not be an instance of FilePipeline? It's either a pipeline or its None? I think we can add this resolution to the root handler method "get pipeline" which can then just recurse back up until it finds it, without this awkward inline import.

Still confused why this wouldn't be defined too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 3777f06. Added Handler.get_pipeline() on the base handler, which walks up the parent chain to the root (the pipeline) — no isinstance guard and no inline import. The archive handler just calls self.get_pipeline(). In production the root is always the running FilePipeline; a handler exercised outside a pipeline resolves to itself, and the integration tests now always build a real pipeline (see the test_convert.py thread).

Comment thread tests/pipeline/test_convert.py Outdated
KPUBConversionHandler().validate_archive(path)


class _FakePipeline:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we faking a pipeline here? This goes against our integration testing focus. We should mock at the external boundaries (requests etc) but otherwise exercise real code paths to ensure that our code actually all works together. As soon as we mock an internal API, we can change internal APIs without breaking tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 3777f06. Dropped _FakePipeline. The ArchiveProcessor tests now run a real FilePipeline and mock only the external boundary — config.DOWNLOAD_SESSION — so each external ref flows through the true download → convert path (an unmapped URL raises like a failed request, exercising the leave-unrewritten branch). The end-to-end handler test likewise drives HTMLZipFile.process_file() with only the download session faked.

rtibblesbot and others added 6 commits July 17, 2026 11:32
Port kolibri-zip's fileUtils mappers to a pure-function module
(ricecooker/utils/references.py): CSS url()/@import and HTML
src/href/srcset/inline-style/<style> detection, with surgical
span-replacement rewriting so re-serialization cannot corrupt
third-party archives. No HTTP or filesystem coupling, so the detection
logic unit tests directly — superseding learningequality#303.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The pipeline is now the sole download path, so the legacy web-scraping
module is removed rather than kept as a shim. Its only live entry points
migrate to the pipeline: read() (html_writer, pdf) resolves via the
DOWNLOAD stage in transfer.py, and the YouTube thumbnail runs the full
pipeline — the CONVERT stage transcodes webp-served-as-jpg from actual
content, dropping the bespoke Content-Type sniffing. The shared download
session gains a retry adapter, matching what downloader.py mounted before
it was removed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ArchiveProcessor operates on an extracted archive directory in two
passes: a reference-led walk that routes every external reference through
the running FilePipeline (download + convert), places the result next to
its referencing file and rewrites the reference; then a scoped conversion
pass that recompresses every media file it can. Reference detection is
delegated to references.py mappers, so a new archive type needs only a
mapper. Lives under utils/ since it defines neither a handler nor a stage.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Each archive handler now unzips into a temp dir, hands it to
ArchiveProcessor to download external refs and recompress media in place,
then reseals with create_predictable_zip. Handlers reuse their running
pipeline via a new Handler.get_pipeline() that walks up the parent chain,
so a nested fetch runs through the same stages, config and caches as every
other file — no module-global stage or isinstance guard. All archive
formats run the generic HTML/CSS mappers; H5P adds an H5PContentMapper for
content.json path references. The pipeline gains a default_context so
compression settings can be threaded through every execute() call.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings in PR learningequality#673. config.COMPRESS is gone; SushiChef.run derives explicit
video/audio ffmpeg settings from its --compress flag and passes them
through the pipeline's default context. Handlers now compress only when
settings are provided, rather than consulting a global at processing time.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Recent macOS runner images may not ship python@3.13 as a keg, leaving
nothing to link; the subsequent brew install resolves its own deps.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle external URL references in archives to enable offline use in Kolibri

2 participants