Scan container images for R/Python libraries; survive legacy and broken images

[kennedydane]

Summary

Closes the README's "Container roots are not scanned for libraries" gap, and fixes today's production scan failure on the astro container root. Six commits:

• 0b2c257 — text-based metadata parser + in-image find builders in libraries.py. The container scanner needs the module scanner's layout knowledge without its filesystem: library_from_metadata_text parses cat output with the same three header rules, and the two *_metadata_find_command builders render the layout constants as bounded in-image finds — kept beside those constants so a new layout is one edit. shlex.quote on every interpolated prefix: it comes from the image's own $PATH, the collector's first shell interpolation of image-controlled data.
• d12b9ee — production hardening. Today's incident: a squashfs image renamed .sif made apptainer convert it to a /tmp sandbox on every exec (36 min CPU, permission-denied FATALs), and the unwrapped $PATH probe then killed the whole root's scan with exit 1. Discovery now sniffs leading bytes and skips known legacy formats (squashfs hsqs, raw ext superblock — a blocklist, so ordinary files aren't misjudged) with a loud warning; an image whose probes fail is recorded with its digest and empty listings (dropping it would be a removal under full-snapshot semantics); an unreadable/vanished file is skipped. A container root's scan now always survives its worst image. .simg files were already invisible to discovery (it globs *.sif); the sniff is for legacy images renamed .sif, which is exactly what production hit.
• 4bafd9d — snapshot-cache format marker. A cached payload from an older collector validates perfectly while silently missing what the new scanner records (libraries=[]), and the digest-skip would reuse it forever. save_scan stamps CACHE_FORMAT; a mismatch is ignored at INFO — one full rescan per container root after upgrading, then normal speed. snapshot_cache.py joins CI's 100% coverage list.
• 013e385 — the scan itself. Prefixes from the image's own PATH (bin/sbin parents, PATH order, deduped) — deliberately without modules.py's system-prefix exclusion, because inside an image /usr/local IS the payload (rocker R, conda). Two bounded in-image finds per prefix (R trees; python site-packages + Debian's dist-packages, .dist-info and both .egg-info shapes) and one cat per metadata file — the man walk's cost profile, absorbed by the digest-skip after the first scan. Output is deterministic and R-correct: site-library beats library (the copy library(x) loads), PATH order decides cross-prefix duplicates (conda's numpy beats the distro's), final sort by (ecosystem, name). Batching all cats into one delimited sh loop (~100× fewer execs) was measured and rejected: a metadata body containing the delimiter would silently shift versions onto wrong names.
• c1f3191 — docs. README gap removed and renumbered ("module or container provides them"); CLAUDE.md pipeline rows updated; deploy guide warns about the one post-upgrade long scan.

No catalog-side changes: VersionPayload.libraries already existed and ingest/search/screens are kind-agnostic — container versions simply start filling the libraries tab and library search results.

The find commands were validated against real images (python:3.13-slim, Alpine/busybox find, debian:bookworm-slim, this repo's own web image — 46 dists found under its venv prefix, returned in readdir order, which is the empirical case for the explicit sort).

Testing

• Full suite in the containerised harness: 1471 passed, 5 environment-dependent skips; coverage 100% total (both CI gates, including the newly-gated snapshot_cache.py).
• pre-commit run --all-files and mypy clean.
• 41 new tests across test_libraries.py, test_containers.py, test_snapshot_cache.py — including command-count proofs via a recording runtime (empty PATH ⇒ exactly two probes), both find-failure paths, per-file failure isolation, R-precedence, PATH-order dedup, and the format-marker rejection paths.

Deploy notes

• The astro root's failing scan is fixed two ways: the legacy image is skipped by content sniffing, and even a still-broken image can no longer kill the scan.
• First container scan after deploying this collector re-enters every image (cache format bump) and will run long once; subsequent scans digest-skip as before.

🤖 Generated with Claude Code

https://claude.ai/code/session_01P7PAEFm2MwFjAaPAJLAqK4

[Copilot]

Pull request overview

This PR extends the ilifu collector’s container scanning so it can discover R/Python libraries inside container images (using in-image find + cat), and hardens container-root scans against legacy or broken images while keeping snapshot-cache correctness via a cache-format marker.

Changes:

• Add container-image library discovery using shared metadata parsing and in-image find command builders.
• Harden container discovery/scanning: skip legacy Singularity formats by sniffing magic bytes; record broken images with empty listings instead of killing the whole scan.
• Introduce a CACHE_FORMAT marker in the snapshot cache so upgrades invalidate stale cached payloads safely; update docs/CI coverage gating accordingly.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
README.md	Updates user-facing description and removes the “containers not scanned for libraries” known gap; updates coverage-gate text.
collector/src/ilifu_collector/libraries.py	Adds shared metadata-text parser and in-image find command builders for container scanning.
collector/src/ilifu_collector/containers.py	Implements library scanning inside images; adds legacy-image sniffing and improved failure isolation.
collector/src/ilifu_collector/snapshot_cache.py	Adds cache-format marker enforcement to prevent digest-skip from reusing outdated payloads.
collector/tests/test_libraries.py	Adds unit tests for metadata-text parsing and find command builders.
collector/tests/test_containers.py	Adds tests for legacy-image skipping, probe failure isolation, and in-image library scanning behavior.
collector/tests/test_snapshot_cache.py	Adds tests for cache format marker behavior (missing/newer format).
CLAUDE.md	Updates pipeline documentation for container library scanning and cache format behavior.
ANSIBLE_DEPLOY.md	Adds deploy note about one-time longer scans after collector upgrades due to cache invalidation.
.github/workflows/ci.yml	Adds snapshot_cache.py to the 100%-coverage gated module list.

Suppressed comments (1)

collector/src/ilifu_collector/containers.py:337

• The "could not read library metadata" warning drops the underlying ContainerExecutionError, which likely contains the apptainer stderr/exit code. Logging the exception message will help pinpoint whether the failure is permissions, a missing cat, or an image runtime error.

    try:
        return runtime.exec_capture(image_path, ('cat', file_path))
    except ContainerExecutionError:
        logger.warning('could not read library metadata %s inside %s', file_path, image_path)
        return None

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kennedydane]

Multi-agent code review

Six agents: four independent finders (line-by-line, removed-behavior audit, cross-file tracing + Python pitfalls, cleanup/conventions) and two adversarial verifiers over the pooled candidates. 34 candidates, 1 refuted, the rest merged into the 21 findings below.

Note on line references. These were reviewed against local branch HEAD 72424e9, which is 13 commits ahead of this PR's head (c1f3191). Several findings concern code that only exists in those unpushed commits — tolerant_find_command arrived in 3bb4a2f, the up-front apptainer check in ef7172f — which is why this is a plain comment rather than inline anchors. Push the branch and I'll re-post these as inline review comments.

---

Blocking

1. find argument injection through an image-controlled $PATH — containers.py:536 (also :546, libraries.py:374, :390)

shlex.quote protects the shell, not find's own argument parsing. shlex.quote('-delete') returns -delete unquoted (- is in shlex's safe set), so an image whose $PATH contains -delete renders:

find -delete -maxdepth 1 -type f -executable 2>/dev/null || [ $? -eq 1 ]

GNU find defaults its start point to . when the first argument is a predicate, and apptainer exec runs with the host cwd, bind-mounting $HOME/$PWD//tmp read-write. Reproduced by the verifier: in a directory containing two files, that exact command emptied it and exited 0 — the 2>/dev/null || [ $? -eq 1 ] wrapper swallows every complaint.

Same root cause, containers.py:334: _install_prefixes has no absoluteness check either. An image whose $PATH contains . or a bare bin yields prefix ., so r_metadata_find_command renders start points like lib/R/site-library, which apptainer resolves against the host cwd it bind-mounts — run the collector from a directory holding a lib/python3.11/site-packages tree and the host's distributions are catalogued as installed inside the image.

The fix is not more quoting: reject any PATH entry / prefix that is not absolute before it reaches a command builder.

2. || [ $? -eq 1 ] forgives every find failure, not just missing start points — libraries.py:333

The docstring's premise is too narrow. GNU find exits 1 for all errors, verified locally: an unknown predicate (find /tmp -maxdepth 1 -bogus → 1) and an existing-but-unreadable start directory (mode 000 → exit 1, no stdout) both exit 1, and wrapping either gives exit 0 with empty output.

apptainer exec runs as the invoking uid, so a root-owned mode-0700 directory on an image's PATH is exactly this case — as is any EACCES subdirectory partway through the recursive man and library finds, which then return a truncated listing recorded as a complete inventory. failures.mark() never fires, so the image is treated as cleanly scanned, save_scan persists the blank/partial payload, and since a .sif's bytes never change the digest-skip replays it forever. main at least logged could not list %s inside %s here.

This is the silent-blank-forever failure cacheable_packages exists to prevent, re-entering through the tolerance wrapper. Distinguish a missing start point from other exit-1 causes — pre-test the start points, or keep stderr and forgive only when the complaint is No such file or directory.

3. Every spawn-time OSError becomes ApptainerNotAvailableError — runtime.py:109

This PR multiplies execs per image from a handful to one cat per metadata file plus two finds per prefix — hundreds for a conda image. One fork() returning EAGAIN/ENOMEM, or EMFILE, on a busy login node now raises ApptainerNotAvailableError, which no probe site catches (they all catch ContainerExecutionError). It unwinds scan_container_root, past run_scan, and cli.scan reports it as typer.BadParameter: cannot run apptainer: [Errno 11] ... — exit 2, an "Invalid value" argument error, no snapshot uploaded, and the operator pointed at a configuration problem that does not exist.

Verified by injecting BlockingIOError(11) into subprocess.run. Reserve the conversion for a genuinely missing or unrunnable binary; let a transient spawn failure stay a ContainerExecutionError so it costs one probe, not the scan.

4. Two paths turn a transient failure into a catalogue deletion

containers.py:292 — a failed PATH probe uploads blanks while previous_version is in hand. The comment is right that an omitted image is a removal — but under full-snapshot semantics these empty listings are a removal of everything the image contributed. Server side: ingest.py does version.binaries.all().delete() and version.libraries.all().delete(), and _sync_manpages deletes every stored key absent from the payload. One 60s --exec-timeout on a loaded node and blast/2.15.0 goes from 40 binaries and 12 man pages to zero of each. previous_version is right there at line 275 and is consulted only for the digest-match early return, while the sibling OSError path at line 164 deliberately does the opposite — the two disagree about whether a transiently unreachable image flaps out of the catalogue.

containers.py:157 — a digest read error with no cached payload drops the image. main called _compute_digest unguarded, so an EIO or stale NFS handle aborted the root's scan and the catalogue stayed intact. Now the image is omitted, which the server's diff turns into a deletion of the version (and the package, if it was the only one). Because CACHE_FORMAT moves to 3 in this same PR, load_previous_versions returns {} for every root on the first run after deploying, so every image sits on this branch for exactly one scan.

---

Plausible — worth a decision, not necessarily a change

• containers.py:508 — one unreadable file permanently disqualifies its image from the scan cache. main only logged; this marks the whole image degraded, so a dangling symlink in man1/ or a root-owned mode-0600 page (apptainer runs as the invoking uid) sets degraded on every scan. cacheable_packages() drops that version, so a multi-GB .sif is re-hashed and fully re-probed every time — under watch, forever, which is precisely what snapshot_cache.py was added to eliminate. Consider not degrading the image when a single file read fails but the listings succeeded.
• containers.py:205 — legacy images already in the catalogue are removed by the skip (accepted, noted for the deploy notes). If a root is entirely legacy the snapshot is empty, run_scan warns but still uploads, scan prints accepted and exits 0, and _refuse_a_total_wipe discards it in a background apply the collector never observes — so that root silently stops updating.
• containers.py:264 — the sniff tests fixed offsets. Singularity 2.4+ images commonly carry a #!/usr/bin/env run-singularity launch header (the same 32-byte launch field SIF uses), which shifts hsqs to offset 32 — where this code instead compares SIF_MAGIC — and the ext magic off 1080. Apptainer's own detection searches the head buffer rather than testing offset 0. Worth a head -c 64 | xxd on the production image that caused the incident. The ext arm is also a bare two-byte test: any non-SIF file (a truncated build, an OCI archive) has a ~1/65536 chance of being dropped with a warning naming the wrong cause.
• containers.py:489 — .splitlines() + .strip() with no -print0: an in-image path holding a newline is split into two bogus paths, both cats fail, and the image is degraded — hence cache-excluded and fully re-probed — on every scan for the life of the deployment. (The man-page half is safe; parse_manpage_filename rejects the split path before any cat.)
• containers.py:400 — one apptainer exec cat per metadata file, on a first scan the CACHE_FORMAT bump forces for every root at once. Hundreds of distributions per conda image at ~0.2–1s per spawn. Not as bad as it first looks: _watch_loop calls maybe_rescan() synchronously, so rescans serialise rather than lapping. One very long scan, not a broken one — the nonce-batching TODO is the real fix.
• containers.py:391 — two find execs per prefix where two per image would do; a stock image PATH dedups to three prefixes, so six container starts list library metadata. Both builders already emit multiple start points.
• containers.py:96 — cacheable_packages() re-derives by string-matching str(image_path) what the scan loop at 166-169 already knew per version. The whole degraded-payloads-must-not-be-cached rule now rests on two independently-produced strings staying equal; normalise one and not the other and a degraded payload is cached forever with no error. Appending to a cacheable_versions list in the loop deletes the set, this method, and the model_copy rebuild.
• containers.py:450 — R's precedence is encoded twice: _R_LIBRARY_DIR_NAMES order ("in R's own precedence order") drives the find, while this sort key restates it as the substring '/site-library/'. Add a tree to the tuple and the find picks it up while this ranks it last, so dedup keeps the bundled copy and the catalogue reports the version library(x) does not load.

---

Cleanup

• modules.py:80 — the #: comment for the deleted _BIN_DIR_NAMES was left attached to _SYSTEM_PREFIXES, which now carries two contradictory descriptions.
• cli.py:126 — the empty-scan warning fires for module roots too but closes with "check the collector log above for skipped images", advice a module scan never produces. (The still-upload half is documented as deliberate.)
• cli.py:210 vs :333 — the same "check this kind's tool up front" rule in two encodings, one inline and container-only, one dispatched. A third kind in _VALID_KINDS and scan silently reverts to what this PR just fixed. One _require_tool_for(kind) used by both.
• libraries.py:126 — 'site-packages' spelled twice (also _SITE_PACKAGES, line 104), one feeding the module walk and one the in-image finds.
• runtime.py:46 — find_apptainer() returns a resolved path both call sites discard while exec_capture re-resolves the bare name; the signature advertises a guarantee the code does not keep. Contrast find_lmod, whose return is consumed.
• test_libraries.py:495 — local import logging duplicating the module-level one this same PR added.
• test_libraries.py:36 — test_should_share_one_bin_dir_rule_between_the_two_scanners asserts two imports are the same object; _derive_prefix and _install_prefixes could both be broken and it would still pass, while counting toward the 100% gate. Feeding one PATH entry through both derivations is strictly stronger and just as cheap.

---

Refuted (1)

The .xz/.bz2/.zst man-page mojibake path cannot occur: _read_manpages only calls _read_file_text after parse_manpage_filename succeeds, and its regex rejects every non-.gz compression suffix, so such a page is logged and skipped before any cat.

---

The scan logic itself is sound and the test suite is genuinely thorough — everything above is about what happens when an image, a directory, or a fork() misbehaves. Fixes for the blocking items and the two data-loss paths are on the way in follow-up commits, under the rule that any doubt keeps an item in the catalogue: only a file actually gone from disk may become a removal. Legacy images are accepted as disappearing.

🤖 Generated with Claude Code

[kennedydane]

Multi-agent code review — inline

The findings from my earlier summary comment, now anchored to the lines they concern. That comment holds the overview and the reasoning for the ranking; these are the 21 findings themselves.

Six agents produced them: four independent finders (line-by-line, removed-behavior audit, cross-file tracing + Python pitfalls, cleanup/conventions) and two adversarial verifiers over the pooled candidates — 34 candidates, 1 refuted.

Fixes for the four blocking items and the two data-loss paths are already written and verified against the full suite; they are not in this push, which is the branch as reviewed.

🤖 Generated with Claude Code