Decompose IMS Content Packages (incl. SCORM) into a native node subtree (#332) by rtibblesbot · Pull Request #721 · learningequality/ricecooker

rtibblesbot · 2026-07-24T18:44:18Z

Summary

An IMS Content Package (including SCORM) could only be imported as one opaque HTML5 zip, so selecting a single node in Kolibri pulled the whole package, and plain media stayed trapped in a JS sandbox instead of playing as a native node. This migrates the imscp manifest parser into ricecooker and drives decomposition from the file-processing pipeline:

IMSCPConversionHandler walks imsmanifest.xml into a tree-shaped ContentNodeMetadata and classifies each webcontent resource up a conservative ladder — native media node, else HTML5 zip or (when it qualifies) KPUB — rejecting assessment and QTI resources. Each surviving leaf re-enters the pipeline and is sealed into its own file rather than backed by the shared package zip.
ContentNode._process_uri expands that tree into a Topic subtree, building each descendant through its own node constructor (TreeNode.from_metadata) and processing/validating the subtree leaves-first.
SCORM_metadata (ported from ricecooker PR Initial integration of QTI and IMSCP import #468) maps the manifest's LOM metadata onto node fields — title, description, tags, language, learning activities, resource types, beginner level, and a license inferred from <rights>.
utils/scorm.py holds the classifiers: SCORM LMS-API boilerplate detection (so a plumbing-only SCO does not read as interactive), assessment semantics, and single-media-wrapper detection.

References

Implements #332. Parser migrated from learningequality/imscp. QTI item ingestion deferred to #337. Cross-node reference rewriting descoped per the issue's resolved design.

Reviewer guidance

ricecooker/utils/pipeline/convert.py — IMSCPConversionHandler is registered before HTML5ConversionHandler in the FirstHandlerOnly convert stage. Confirm an IMSCP .zip is claimed for decomposition rather than wrapped whole by the HTML5 handler.
ricecooker/utils/pipeline/convert.py — HTML5ConversionHandler._promote_to_kpub applies to every HTML5 zip, not just IMSCP leaves, and it strips SCORM boilerplate scripts, .css members and stylesheet <link>s rather than letting them disqualify the zip (this flips an existing test_transfer.py assertion). Confirm both the preset change and the styling loss are acceptable for existing chefs that ship static HTML5 zips expecting HTML5_ZIP.
.pre-commit-config.yaml — check-added-large-files is excluded for tests/testcontent/imscp/, which adds ~4.4 MB of real IMSCP/SCORM zip fixtures.
ricecooker/classes/nodes.py — _process_uri adds and processes descendant nodes during the node's own process_files(), after ChannelManager.process_tree has snapshotted the node list. See the upload gap under Deviations.

Deviations from the issue spec

KPUB promotion is not scoped to IMSCP leaves — it runs for every HTML5 zip. Restricting it to the decomposition path would have meant a second, near-identical qualification branch.
Promotion is lossy by design: SCORM boilerplate scripts and stylesheets are removed so a plumbing-only SCO still qualifies, so a promoted zip renders unstyled. Only genuine scripting or a non-boilerplate .js member keeps a zip as HTML5_ZIP.
Decomposed leaf files are not uploaded yet. ChannelManager.process_tree snapshots the node list before calling process_files(), so descendants created during expansion never enter file_map; the subtree structure serializes but the leaf bytes are never diffed or uploaded. Verified locally: process_tree() returns [] for a tree-expanding node. The fix belongs in managers/tree.py and is not in this branch.

AI usage

Used Claude Code to implement the migrated parser, classifiers, KPUB-promotion branch, and IMSCP handler from a pre-approved plan using test-driven development, following the existing pipeline handler patterns. Verified with the full test suite and prek/ruff.

@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
Rebased onto the target branch and resolved any conflicts
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

	🟡 Waiting for feedback Last updated: 2026-07-26 21:11 UTC

rtibbles

Oh, I assumed you would look more at #468 than at the archived imscp repository. Please make sure to go back to the prior work on ricecooker here to better shape the code conventions, implementation, and above all, the fidelity to the IMSCP spec.

rtibblesbot · 2026-07-24T19:38:29Z

Went back through #468 and shaped the parser against it rather than the archived imscp repo. Pushed in 258fc20.

Adopted from #468

QTI recognised by the spec's imsqti_ type prefix (is_qti_resource), so QTI resources are rejected intentionally in the IMSCP handler rather than dropped as an unknown type.
A missing identifierref or <dependency> now logs a warning instead of being silently skipped.
flatten_single_child_topics collapses an <organization> that wraps a single content-root <item> (eventos/gitta now sit their content root directly under the manifest).

Kept divergent from #468, for robustness against untrusted manifests

Cyclic-<dependency> guard plus member dedup in file derivation.
../ path-traversal guards when staging leaves out of the package.
xml:base applied to the index path as well as the member files.

Out of scope here

Initial integration of QTI and IMSCP import #468's LOM → le_utils metadata mapping (SCORM_metadata.py): the agreed design for this issue descoped metadata mapping to a follow-up.
Per-item QTI ingestion: owned by Handle QTI exercises #337.

Say the word if you want the LOM mapping or the #468 divergences reconciled in this PR.

rtibbles · 2026-07-24T20:53:37Z

I do think including the metadata mapping in this pull request would be very helpful, actually.

rtibblesbot · 2026-07-24T21:16:54Z

Added the LOM → le_utils metadata mapping in 46c6276.

Ported from #468 — ricecooker/utils/SCORM_metadata.py, metadata_dict_to_content_node_fields:

learningResourceType → learning_activities (with the interactivity downgrade)
learningResourceType / intendedEndUserRole → resource_types
rights → license / license_description
lifeCycle contribute VCARDs → author / provider / copyright_holder
keyword → tags
difficulty → learner_needs
language normalized

Parser — imscp.py now collects the raw LOM dict (general/rights/educational/lifeCycle) per node:

resolves external adlcp:location metadata refs
carries the metadata through single-child flattening

Wiring

IMSCPConversionHandler applies the mapping onto each topic/leaf tree dict and the package root
the fields ride the existing content_node_metadata plumbing onto the expanded nodes
ContentNode._apply_content_metadata routes license fields through set_license
a mapped licence inherits the package's copyright holder when the LOM names none, so a holder-requiring licence stays valid
ContentNodeMetadata gained language / tags

Divergences from #468, for parity with the rest of this PR

stdlib ElementTree ({*} wildcards) instead of lxml
<value><langstring>… recursion so LOMv1.2 vocab terms are read, not the whitespace around them

Tests

mapping unit tests: tests/utils/test_scorm_metadata.py
LOM-collection parser tests: tests/utils/test_imscp.py
end-to-end mapped-field assertions on a decomposed leaf/node: tests/pipeline/test_convert.py

rtibblesbot · 2026-07-24T21:20:38Z

Rebased onto main to clear the merge conflicts with #714.

Two conflicts, both from #714's a51fc2e promoting the HTML entry-point helpers out of convert.py into utils/zip:

convert.py — kept upstream's deletion of _find_common_root/_find_entry_html and the find_common_root/find_html_entrypoint imports; kpub_disqualifiers and the KPUB-promotion branch in HTML5ConversionHandler.handle_file now sit where those helpers used to be, and validate_archive/_prepare_archive call the utils/zip versions.
tests/pipeline/test_convert.py — dropped the moved _find_common_root/_find_entry_html imports and their tests (they live in tests/test_zip.py now), kept the IMSCP/KPUB tests.

No behaviour change from the rebase. uvx prek run --all-files clean; 561 passed, 12 skipped locally, with only the 7 pre-existing test_gdrive_* failures from a missing local googleapiclient.

rtibbles

Some concerns with architecture and overall structure, as well as a big concern about the regression for KPUB behaviour that you decided to add.

I also would like you to review the test suites you have written - please make sure they are focused on integration testing of input SCORM files to outputted ricecooker classes.

Any duplication, overly granular testing, or other redundant tests should be cleaned up.

rtibbles · 2026-07-26T19:41:49Z

            self._validate_uri()
        super(ContentNode, self)._validate()

+    # File.__init__ has no **kwargs; any non-constructor key (path, license, ...)


Why not just add **kwargs to the file init?

Done — File.__init__ takes **kwargs now (classes/files.py:73), so a pipeline file-metadata dict splats straight in and nodes.py no longer keeps a copy of the parameter list. _file_from_metadata is down to inheriting the node language.

rtibbles · 2026-07-26T19:45:43Z

+        kwargs.setdefault("language", self.language)
+        return File(**kwargs)
+
+    # License-related fields live on the node's License object, not as plain


This special handling feels incorrect. Isn't this duplicating some of what already exists in init methods? Can't we extract that into a method and reuse that?

Agreed — removed. Descendants are now built by their own constructors (TreeNode.from_metadata), which is where that metadata handling already lived, so there is no second setattr implementation.

For the one node that already exists when metadata arrives (the node whose uri was decomposed), there is Node.set_metadata — a single method applying a partial metadata dict, sharing _set_license_fields with __init__ rather than reimplementing the license routing.

The "inferred license needs a copyright holder" special case moved out of nodes.py entirely to utils/SCORM_metadata.py:_drop_unattributable_license, which is where the inference happens: a license requiring attribution is only inferred when the LOM also names someone to attribute it to.

rtibbles · 2026-07-26T19:48:10Z

+            )
+        self._apply_content_metadata(self, content_metadata)
+
+    def _expand_content_tree(self, content_metadata):


It feels like this logic belongs on TreeNode, rather than ContentNode? The differential behaviour/polymorphism feels like it could be better expressed through inheritance rather than things being hardcoded into ContentNode.

Moved to TreeNode and made polymorphic:

TreeNode.from_metadata picks the class (TopicNode when the metadata has no concrete kind, else ContentNode) and hands the non-structural keys to its constructor.

Each class supplies its own add_metadata_content: TreeNode attaches children, ContentNode attaches files. No dict-shape branching in ContentNode any more.

expand_metadata_tree / process_metadata_subtree are TreeNode methods, so any tree node can be the root of a decomposed subtree.

ContentNode keeps only what is genuinely ContentNode-specific: INHERITED_METADATA_KEYS adding license, and the _validate early return for a node that has become a folder.

rtibbles · 2026-07-26T19:50:18Z

+
+
+def _kpub_disqualifier(names, index_html, entry="index.html"):
+    """The first reason a KPUB candidate fails the criteria; None ⇒ it qualifies.


Let's apply /terse-writing to all code comments introduced in this PR please. Concise, precise comments are the only ones allowed.

Done in 9a23c9f — went through every comment and docstring this PR adds and cut the restatements and the padding. The multi-line ones that survive each carry one non-obvious fact.

rtibbles · 2026-07-26T19:52:48Z

+        against the criteria just as much as ones the archive shipped with.
+        """
+        names = _archive_member_names(temp_dir)
+        # A .js/.css member disqualifies whatever the markup says, so test that


This seems to entirely contradict the stripping of SCORM specific stuff later in the file? Even then, just the existence of CSS is insufficient to rule out KPUBs - using a full HTML5 zip simply because of a little bit of unnecessary CSS styling is not helpful.

You are right on both counts, and they have the same fix: promotion now removes the things that are not content instead of refusing to promote because of them.

HTML5ConversionHandler._promote_to_kpub (convert.py:485):

SCORM boilerplate scripts are stripped from the entry document and the wrapper .js members are deleted, so the discount and the shipped output finally agree. A static SCO wrapped in SCORM_API_wrapper.js is now a KPUB.

.css members and their <link rel=stylesheet> are stripped too (new references.strip_stylesheet_links), so a bit of unnecessary styling no longer costs a full HTML5 zip.

Only a non-boilerplate .js member or a surviving inline <script> keeps a zip as HTML5.

Still judged after reference resolution, so an externally-referenced stylesheet that was downloaded into the archive is stripped rather than sealed into the .kpub.

rtibbles · 2026-07-26T19:55:04Z

-            # Check for inline <script> tags (parsed without namespaces)
-            for _ in dom.iter("script"):
+            try:
+                index_html = zf.read("index.html")


Sorry, why have you changed the contract here? KPUB is allowed to have an entry specified: https://github.com/learningequality/kolibri/blob/develop/kolibri/plugins/safe_html5_viewer/frontend/views/SafeHtml5RendererIndex.vue#L72

But you've decided that we shouldn't allow this any more even though it's completely supported, why?

Not intended — thank you, that was a straight regression in the refactor. Both handlers now honour a specified entry.

HTML5ConversionHandler and KPUBConversionHandler share a new WebArchiveConversionHandler, which denests the zip, finds the entry with find_html_entrypoint, validates that document, and records it in extra_fields.options.entry when it is not a root index.html. So:

a hand-authored .kpub whose entry is content.html validates and ships with the hint;

an HTML5 zip whose static article is at article.html is promoted to KPUB and keeps its entry hint.

sanitize_kpub_directory takes the entry rather than assuming index.html. Tests: TestKPUBValidation.test_non_index_entry_point_accepted and TestKPUBPromotion.test_non_index_entry_promoted_with_entry_hint.

rtibbles · 2026-07-26T19:56:14Z

+_IMSCP_MANIFEST = "imsmanifest.xml"
+
+
+class _Package:


Feels like this could live in the imscp.py utils package and be imported?

Moved — it is IMSCPPackage in ricecooker/utils/imscp.py now and imported by convert.py. The path-containment helper it depends on was already there.

rtibbles · 2026-07-26T19:58:42Z

+    def _process_leaf(self, node_dict, package, index_html):
+        """Run the resource up the ladder and return its sub-pipeline result.
+
+        A resource that reduces to a single wrapped media file is processed as


Another very verbose comment.

Trimmed, along with the rest of the comments this PR adds (9a23c9f).

rtibbles · 2026-07-26T19:59:20Z

@@ -0,0 +1,201 @@
+"""Conservative classifiers for SCORM/IMSCP webcontent resources.


This doesn't belong in the pipeline module, it should be in the general utils folder.

Moved to ricecooker/utils/scorm.py.

rtibbles · 2026-07-26T20:00:00Z

+SCORM_STATUS_RE = re.compile(r"cmi\.core\.lesson_status", re.IGNORECASE)
+
+# HotPotatoes quiz engine globals. Their presence means the page IS an exercise.
+_HOTPOTATOES_GLOBALS_RE = re.compile(r"JQuiz|JCloze|JMatch|JMix|JCross", re.IGNORECASE)


What is the provenance of hot potatoes metadata?

It comes from the migrated fixture. tests/testcontent/imscp/test_quiz.zip — one of the three examples in the archived learningequality/imscp repo, and the only assessment sample we have — is a Hot Potatoes export:

imsmanifest.xml carries 

test_quiz.htm has <meta name="author" content="Created with Hot Potatoes by Half-Baked Software, registered to kevino."/> plus a Hot Potatoes, Half-Baked Software, ... keywords meta, and defines the JQuiz global.

So the signal is "this is the shape the one real quiz package we have takes", not something from the IMSCP/SCORM spec. Note it is matched on <meta ... content="...Hot Potatoes..."> rather than name="generator", because that is what the real file uses; JQuiz/JCloze/JMatch/JMix/JCross are the engine globals for its five exercise types.

Worth saying that it is belt-and-braces here: test_quiz.htm writes cmi.core.score from an inline script three times, so has_assessment_semantics rejects it on the score signal even with the Hot Potatoes checks removed (it declares no adlcp:masteryscore). Happy to drop the tool-specific checks if you would rather not special-case one authoring tool — the fixture still gets rejected, we would just lose the ability to catch a Hot Potatoes page that reports nothing back to the LMS.

rtibblesbot · 2026-07-26T20:31:47Z

Pushed 62677c1, 8de9d91, 9a23c9f. All eleven inline threads have replies; two things worth calling out here.

The KPUB regression is gone, and then some. Promotion no longer refuses over things that are not content — it removes them. SCORM wrapper scripts and their .js members, stylesheet <link>s and .css members are stripped, and only genuine scripting keeps a zip as HTML5. KPUB also honours a specified entry point again: HTML5ConversionHandler and KPUBConversionHandler share a WebArchiveConversionHandler that finds the entry, validates that document, and records it in extra_fields.options.entry.

Test suites reviewed and cut. Net −11 tests, with coverage moved onto real inputs:

Deleted TestSCORMClassifiers (11 unit tests poking scorm.py with HTML strings). The signals it covered are now exercised by decomposing packages: Hot Potatoes via test_quiz.zip, and adlcp:masteryscore / cmi.core.score writes via a parametrized synthetic package.
tests/utils/test_scorm_metadata.py: 13 → 8. The per-function probes (infer_license_from_rights, extract_lifecycle_contributors, map_scorm_to_educator_resource_types, infer_beginner_level_from_difficulty) folded into one full-mapping test asserting the whole output dict; the three license cases became one parametrized test.
tests/utils/test_imscp.py: 16 → 11. Assertions against the same fixture merged into one test per fixture; the trivial one-liners (is_qti_resource, source_id-from-identifier) folded into the tests that already parsed that file.
TestIMSCPDecomposition: the repeated tempdir/build/pipeline boilerplate is one _decompose helper, the two near-identical article tests are parametrized, and the LOM assertions that were split across a tree-dict test and a node test are now a single test on the expanded ContentNode.

Full suite green locally (517 passed; the 7 test_gdrive_* failures are a missing googleapiclient in this environment, not this branch).

…, task 1) Port learningequality/imscp core.py's manifest walk to stdlib xml.etree.ElementTree with {*} namespace wildcards (no lxml). Emits a tree of dicts: topics carry `children`; webcontent leaves carry `files` (own <file> members plus flattened <dependency> resources, order-preserving and deduped). Includes the item{n} source_id fallback lifted from legacy ricecooker_utils.py. Migrates the three SCORM fixtures (gitta_ims, eventos, test_quiz) and adds parser unit tests in tests/utils/test_imscp.py. Extends the check-added-large-files exclusion to cover the imscp fixtures, mirroring the existing testcontent/samples exclusion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…equality#332, task 2) Lets a pipeline handler return a tree of content-node metadata: a topic node sets `children` (nested content-node dicts) and a leaf sets `files` (fully-processed file-metadata dicts). Kept as generic lists of dicts so FileMetadata.to_dict()/merge() round-trip them unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ngequality#332, task 3) When a ContentNode's pipeline result carries a `children` tree, it becomes a Topic/Folder subtree instead of a flat leaf: `_process_uri` detects the tree and delegates to `_expand_content_tree`, which builds TopicNode/ContentNode descendants via `_build_descendant` and self-processes them (ChannelManager snapshots the node list before processing, so dynamically-added children must process/validate in place). `_validate` gains a top-of-method topic guard. File construction is factored into `_file_from_metadata` (filters to File.__init__'s params) and scalar-metadata application into `_apply_content_metadata`, shared by the flat and tree paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ningequality#332, task 4) Add ricecooker/utils/pipeline/scorm.py with conservative classifiers for SCORM/IMSCP webcontent resources: strip_scorm_boilerplate discounts LMS plumbing (pipwerks/LMSInitialize/SetValue) before interactivity is judged; has_assessment_semantics flags HotPotatoes/score/tracking resources; and single_media_member detects a resource that reduces to one wrapped media file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…rningequality#332, task 5) Extract kpub_disqualifiers() (inline <script>, .js/.css member, empty body) and reuse it in KPUBConversionHandler.validate_archive. HTML5ConversionHandler now seals a static-article zip as a KPUB (HTML5_ARTICLE/KPUB_ZIP) instead of an HTML5 zip when it meets the KPUB criteria after SCORM-boilerplate discount; interactive content (scripts, JS/CSS members, non-root entry) stays HTML5. This benefits all HTML5 zips and covers the decomposition ladder's static rung. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…learningequality#332, task 6) Add IMSCPConversionHandler, registered before HTML5ConversionHandler in the convert stage. It parses imsmanifest.xml, walks the manifest into a topic tree, and classifies each webcontent resource up a conservative ladder: assessment resources are rejected; a single wrapped media file becomes a native media node; everything else is sealed into its own zip and re-entered into the pipeline as an HTML5 zip or (when it qualifies) a KPUB. Each leaf is backed by its own files, never the shared package zip. ContentNode tree expansion is refined to detect a decomposed tree on the presence of a children list (a fully-rejected package yields an empty-folder topic) and to tell topic from leaf by kind rather than by having children. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

… assessment grep (issue learningequality#332) Self-review fixes to the decomposition ladder: - parser: apply the resource's xml:base offset to index_file (it was only applied to member files, so a resource declaring xml:base resolved index_file to a nonexistent path and was silently dropped — whole-resource content loss). Guard against a <file>/<resource> with no href. - parser: surface <adlcp:masteryscore> (a child element, not an attribute) so the assessment classifier's mastery-score signal actually fires. - classifier: discount SCORM API boilerplate before grepping for a score/status write, matching the docstring and plan. A plain content SCO that reports cmi.core.lesson_status via inline plumbing is no longer misclassified as an exercise and dropped. Adds parser and classifier regression tests for each. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…on (issue learningequality#332) A crafted or malformed imsmanifest.xml with a <dependency> cycle (A->B->A) sent _derive_files into unbounded recursion, crashing the chef with RecursionError. IMSCP packages are untrusted input, so track resources already on the dependency chain and skip re-entering them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ingequality#332) A manifest href/file path is untrusted content. _build_leaf read the index file and _stage_leaf copied the index fallback using the raw manifest path joined onto the extracted package dir, so a ../ traversal (e.g. href="../../../../etc/passwd") let a hostile package read a file from outside the package and seal it into the decomposed node's output. The member-copy loop already guarded its write side; this extends the same containment check (via a shared _contained_path helper) to the index read, the single-media path, and the index-copy fallback, dropping any resource whose path escapes the package. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…s (issue learningequality#332) Reviewer asked to shape the parser against the prior ricecooker work in PR learningequality#468 (rather than the archived imscp repo) for conventions and IMSCP spec fidelity. Bring across the three parser conventions that fit the agreed decomposition-ladder scope: - Recognise QTI resources by their spec-defined `imsqti_` type prefix (`is_qti_resource`) and reject them intentionally in the IMSCP handler, rather than dropping them incidentally as an unknown type. Per-item QTI ingestion stays deferred to learningequality#337. - Log a warning for every dangling `identifierref`/`<dependency>` instead of silently skipping it, so decomposition losses are visible. - Flatten a redundant single-child topic chain (an `<organization>` wrapping one content-root `<item>`) via `flatten_single_child_topics`. LOM metadata → le_utils mapping (PR learningequality#468's SCORM_metadata.py) stays out of scope per the agreed design, which deferred metadata mapping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…learningequality#332) Tighten docstrings and collapse the HTML5 seal-extension branch. Reuse _contained_path for the leaf-staging destination so the read and write sides share one containment check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ality#332) Port PR learningequality#468's LOM -> le_utils metadata mapping (SCORM_metadata.py) and wire it through the IMSCP decomposition: - SCORM_metadata.py: learningResourceType -> learning_activities / resource_types, rights -> license / license_description, lifeCycle contribute VCARDs -> author / provider / copyright_holder, keyword -> tags, difficulty -> learner_needs, language normalized. - imscp.py: collect the raw LOM dict (general/rights/educational/ lifeCycle) per node via stdlib ElementTree, resolving external adlcp:location metadata refs; carry it through single-child flattening. - IMSCPConversionHandler applies the mapping onto each topic/leaf tree dict and the package root; ContentNode._apply_content_metadata routes license fields through set_license (inheriting the package holder when LOM names none) and topics/root now receive their LOM metadata. - ContentNodeMetadata gains language/tags fields. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…quality#332) Six issues in the branch's own code, all reachable from the shipped fixtures: - Leaf staging copied only the manifest's declared `<file>` members. gitta_ims declares none, so its 23 leaves sealed as unstyled orphan pages. Staging now also follows the assets its members reference (bounded to package members; the HTML mapper reports offline resources only, so a leaf never absorbs the pages it links to). - Staging forced a root `index.html` copy even for an entry living in a subdirectory, breaking every relative reference in it. The alias is now made only when the entry is already at the staging root; a deeper entry stays put and HTML5ConversionHandler records it as an entry hint. - A LOM keyword over 30 characters became a tag that fails node validation, crashing the whole channel (gitta_ims did exactly this). Over-long keywords are dropped with a warning. - has_assessment_semantics stripped any inline script calling the LMS API before grepping for a score write — so the standard `LMSSetValue("cmi.core.score.raw", …)` quiz was never detected. A score write is now an assessment signal regardless; the boilerplate discount applies only to the weaker lesson-status signal. - _apply_license_metadata mutated the License object in place, which descendants share with the package node, leaking one resource's LOM rights onto every other node. It now always builds a fresh License. Topics no longer take a license at all — a holder-less CC license from folder LOM rights failed validation. - A malformed manifest escaped as ET.ParseError (or TypeError when chardet detected no encoding); it is now an InvalidFileException. A single unusable resource likewise drops just that leaf instead of failing the package, and a manifest that lists itself as a resource file no longer recurses forever. Also drops the unused member_names parameter from has_assessment_semantics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…uality#332) Three defects found reviewing the branch against upstream/main: - KPUB qualification was judged before reference resolution, so an HTML5 zip referencing an external stylesheet qualified (no .css member yet), then had that stylesheet downloaded into the archive and was sealed as a .kpub containing CSS — an archive KPUBConversionHandler itself rejects. The check now runs over the processed directory, so it sees what is actually sealed. kpub_disqualifiers takes member names plus the entry markup rather than a ZipFile, so both callers can supply them. - single_media_member accepted image extensions, but Kolibri has no image content kind: a page wrapping one picture produced a kind-less leaf, which the tree expander cannot distinguish from a topic, so it silently became an empty folder. Images are no longer media-promotion targets (such a page stays the static article it already is), and a leaf whose kind cannot be inferred is now dropped with a warning rather than degrading to an empty folder. - has_assessment_semantics matched "hot potatoes" anywhere in the document, silently rejecting any resource that merely wrote about them; it now matches the generator/author <meta> tag HotPotatoes stamps. Tests: hoist the new tests' inline imports to module scope, drop the KPUB-refactor guard that duplicated TestKPUBValidation's existing JS-rejection case, and cover the two behaviour fixes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…lity#332) Self-review fixes on the IMSCP decomposition branch: - LOM fields repeat freely (one <langstring> per language, repeated elements), so the parser hands back strings, lists, and lists of lists. Feeding those straight through set a list on node.title/description (which node validation rejects) and indexed the activity/resource-type mappings with an unhashable list. Flatten to text in one helper and take the first value for fields that must be scalars. - An inferred license that requires a copyright holder none of the metadata supplies no longer replaces the caller's license — applying it failed node validation and aborted the whole package. - Collect LOM <educational><difficulty> so infer_beginner_level_from_difficulty can actually fire; it was unreachable. - A resource's attributes no longer displace the referring item's own identifier (matches ricecooker PR learningequality#468). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…uality#332) Review feedback: - Move the SCORM classifiers out of the pipeline package to ricecooker/utils/scorm.py, and the IMSCP package-staging class out of convert.py into ricecooker/utils/imscp.py as IMSCPPackage. - KPUB may name its entry point (Kolibri's safe_html5_viewer honours extra_fields.options.entry), so stop hardcoding a root index.html. HTML5 and KPUB now share WebArchiveConversionHandler, which denests, validates the discovered entry, and records a non-index entry for the renderer. - KPUB promotion no longer treats SCORM plumbing and stylesheets as disqualifying: neither carries content, so both are stripped and the page is promoted. Only genuine scripting keeps a zip as HTML5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…lity#332) Review feedback: - File.__init__ takes **kwargs, so a pipeline file-metadata dict splats straight in and nodes.py no longer keeps a copy of its parameter list. - Tree expansion moves to TreeNode and dispatches on the node class: from_metadata picks TopicNode or ContentNode and each supplies its own add_metadata_content (children vs files), instead of ContentNode branching on dict shape. - Descendants are built by their constructors rather than a bespoke setattr pass. Node.set_metadata applies partial metadata to an already-built node, and both it and __init__ route license fields through one _set_license_fields method. - The "inferred license needs a copyright holder" guard moves to the LOM mapper, where the decision belongs: a license requiring attribution is only inferred when LOM also names someone to attribute. Tests: drop the SCORM classifier unit tests in favour of decomposing synthetic SCORM packages end to end, and fold the duplicated parser/LOM-mapping cases into single denser tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Review feedback: apply /terse-writing to the comments and docstrings this PR adds — cut restatements of the code and trim the multi-line ones to the fact that is not obvious from reading it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

rtibbles requested changes Jul 24, 2026

View reviewed changes

rtibblesbot marked this pull request as ready for review July 24, 2026 19:24

rtibblesbot force-pushed the issue-332-196d3d branch from acc89c8 to eb2be36 Compare July 24, 2026 21:20

rtibblesbot force-pushed the issue-332-196d3d branch from 536dd64 to f35795d Compare July 24, 2026 22:36

rtibbles requested changes Jul 26, 2026

View reviewed changes

rtibblesbot and others added 18 commits July 26, 2026 14:06

rtibblesbot force-pushed the issue-332-196d3d branch from 9a23c9f to 18bac3c Compare July 26, 2026 21:09



		def _kpub_disqualifier(names, index_html, entry="index.html"):
		"""The first reason a KPUB candidate fails the criteria; None ⇒ it qualifies.

		@@ -0,0 +1,201 @@
		"""Conservative classifiers for SCORM/IMSCP webcontent resources.

Uh oh!

Uh oh!

Conversation

rtibblesbot commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

Deviations from the issue spec

AI usage

🟡 Waiting for feedback

Uh oh!

rtibbles left a comment

Choose a reason for hiding this comment

Uh oh!

rtibblesbot commented Jul 24, 2026

Uh oh!

rtibbles commented Jul 24, 2026

Uh oh!

rtibblesbot commented Jul 24, 2026

Uh oh!

rtibblesbot commented Jul 24, 2026

Uh oh!

rtibbles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot commented Jul 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rtibblesbot commented Jul 24, 2026 •

edited

Loading