Download external resources for archive content types (#233)#690
Download external resources for archive content types (#233)#690rtibblesbot wants to merge 6 commits into
Conversation
rtibbles
left a comment
There was a problem hiding this comment.
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.
| class ArchiveProcessingBaseHandler(ExtensionMatchingHandler): | ||
| CONTEXT_CLASS = ArchiveProcessingContextMetadata | ||
|
|
||
| # Per-handler opt-in for downloading external resource references and |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good call — the base class no longer knows anything about H5P. I followed kolibri-zip's Mapper design:
- Added a
ReferenceMapperabstraction — each file type gets a mapper that knows which files ithandles(), what references theyextract(), and how torewrite()them. references.pynow exposes only the generic web mappers (HTMLMapper,CSSMapper) asDEFAULT_MAPPERS.H5PContentMapper— and all knowledge of H5P, including thecontent/content.jsonpath andpath-key walking — lives with theH5PConversionHandlerinconvert.py. It opts in viaREFERENCE_MAPPERS = DEFAULT_MAPPERS + (H5PContentMapper(),).- The base handler now carries only
REFERENCE_MAPPERS = ()and the archive processor dispatches throughmapper.handles/extract/rewrite, so it is fully format-agnostic. TheSCAN_HTML/SCAN_CSS/SCAN_H5P_JSONflags are gone.
New file types are now supported by supplying a mapper, not by editing the base class or the processor. Done in 2b8d02f.
| shutil.rmtree(out_dir, ignore_errors=True) | ||
|
|
||
| def test_h5p_json(self): | ||
| from ricecooker.utils.pipeline import archive_assets |
There was a problem hiding this comment.
Please clean up all unnecessary inline imports.
There was a problem hiding this comment.
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\(['\"]?(.*?)['\"]?\)") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done in edab23c — downloader.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
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Copy file? File pipeline does this.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| try: | ||
| # Create partial for reading & compressing subfiles | ||
| file_converter = partial( |
There was a problem hiding this comment.
Even this code would be better handled by just running every file through the file pipeline (or a reduced version of it).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.pythe only detector, a faithful port of kolibri-zip'sfileUtils.jsmappers — not a second one alongsidedownload_static_assets. - Delete
downloader.pyrather than importing its regexes. - Migrate its live callers to the pipeline:
read(html_writer, pdf),get_archive_filename/thumbnail (chefs). - Drop the
_get_conversion_stagemodule-global. - Recurse into the running
FilePipelineviaHandler.parentinstead. - 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.jsonexternals undercontent/with a content-relative path. ../_static/will not resolve againstH5P.getPath.- Run the mappers for every archive format, including EPUB/KPUB/Bloom.
- Walk all files rather than assuming
index.htmlas 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 |
There was a problem hiding this comment.
Why just the download stage? What if the file needs to be converted? Why aren't we just reusing most of the pipeline here?
There was a problem hiding this comment.
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.pyis the sole detector — a faithful port of kolibri-zip's per-file-type mappers (CSSurl()/@import; DOMsrc/stylesheethref/srcset/inline +<style>).downloader.pyis 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.jsonexternals land insidecontent/with a content-relativepath— the only formH5P.getPathresolves (../_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
left a comment
There was a problem hiding this comment.
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. | |||
There was a problem hiding this comment.
Difficult for me to review this at the moment, because the code comments are so extraordinarily long, doing a reasonable review is challenging.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| # 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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| try: | ||
| # Create partial for reading & compressing subfiles | ||
| file_converter = partial( |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Done — moved ricecooker/utils/pipeline/archive_assets.py → ricecooker/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 |
There was a problem hiding this comment.
utils.utils is such a stupid module path - we should file a follow up issue to clean this up and make things more sensible.
| def process(self): | ||
| """Download external refs, then compress media, in place.""" | ||
| self._download_external_refs() | ||
| if config.COMPRESS: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| KPUBConversionHandler().validate_archive(path) | ||
|
|
||
|
|
||
| class _FakePipeline: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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>
898582e to
c41574f
Compare
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-typeReferenceMapperclasses — HTML and CSS by default, plus an H5P-specific mapper — modeled on kolibri-zip's per-file-type mappers. HTML rewriting is surgical: a stdlibHTMLParserwalk collects reference spans and_apply_editssplices in the replacements, leaving the rest of the document byte-for-byte.A
utils/utility (archive_assets.py) drives the archive-level work.ArchiveProcessortakes 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 itscontent.jsonreferences.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 olddownloader.pyredundant: it is deleted (~1000 lines), and its two remaining live callers — the module-levelread()and the YouTube thumbnail fetch — are migrated onto the pipeline (read()now wrapsDownloadStageHandler, and the thumbnail is transcoded by the CONVERT stage from its actual bytes instead of Content-Type sniffing). Theloadjsselenium/pyppeteer render path is intentionally dropped as a follow-up.Compression is no longer driven by a global flag:
config.COMPRESSis removed, andSushiChef.runderives explicit ffmpeg settings from--compressand threads them through the pipeline's newdefault_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.utilsmodule).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, inlinestyle,<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 atempfile.TemporaryDirectory(); confirm nothing downstream retains a path into that dir past thewithblock, 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?