refactor: replace install-common.sh with declarative artifact system

[eoinfennessy]

Summary

• Replaces install-common.sh (which used runtime tools like hf download, docling-tools, tiktoken) with a shared declarative artifact system consistent with how downstream Konflux hermetic builds fetch artifacts
• Adds artifacts.lock.yaml declaring all ML model artifacts with download URLs and SHA256 checksums, pinned to specific revisions
• Adds fetch_artifacts.py (async downloader with checksum verification) and copy-artifacts.sh (copies artifacts into correct cache layouts for both standard and Konflux builds)
• Fixes opencv-python swap: the previous opencv-python<0 constraint silently dropped rapidocr and downgraded docling; replaced with a post-install swap pinned to the resolved version
• Fixes HF cache location to use APP_ROOT instead of HOME (build-time vs runtime user mismatch)
• Adds path traversal validation to fetch_artifacts.py
• Derives tiktoken cache key from URL instead of hardcoding

Supersedes #470 (pushed to upstream branch for CI).

Test plan

• Build container image locally and verify all 27 artifacts are fetched and checksums verified
• Verify tiktoken loads from cache and encodes correctly (cl100k_base)
• Verify granite embedding model loads from HF cache and produces meaningful 768-dim embeddings (semantic similarity check)
• Verify docling DocumentConverter initializes and processes a PDF end-to-end (layout analysis, RapidOCR, text extraction all using prefetched models)
• Verify opencv-python is absent and opencv-python-headless is installed at the same version (4.13.0.92)
• Run smoke tests against the built image (requires vLLM + PostgreSQL — covered by CI)

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Container builds now fetch and cache required model and tokenizer artifacts from a pinned lockfile, improving consistency across environments.
  • Added support for a Hugging Face cache location and artifact path configuration during image builds.

• Bug Fixes

  • Replaced the standard OpenCV package with the headless variant to prevent runtime failures caused by missing system graphics libraries.
  • Artifact downloads are now verified with checksums and copied into the image safely.

[coderabbitai]

Warning

Review limit reached

@eoinfennessy, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 96e67974-5e93-448d-887e-4981e981ce45

📥 Commits

Reviewing files that changed from the base of the PR and between e7825d4 and eecbac6.

📒 Files selected for processing (3)

• Containerfile
• Containerfile.in
• distribution/fetch_artifacts.py

📝 Walkthrough

Walkthrough

This PR replaces install-common.sh's inline model provisioning with a lockfile-driven artifact pipeline: a new artifacts.lock.yaml pins download URLs, SHA-256 checksums, and filenames; fetch_artifacts.py asynchronously downloads and verifies these artifacts with path-traversal checks; copy-artifacts.sh places them into the app cache and bootstraps a Hugging Face refs/main file. Containerfile and Containerfile.in are updated to invoke this pipeline and to swap opencv-python for a version-pinned opencv-python-headless, plus set DOCLING_ARTIFACTS_PATH and HF_HOME.

Estimated code review effort: 4 (Complex) | ~45 minutes

Security Notes

• CWE-494 (Download of Code Without Integrity Check) — mitigated but verify enforcement order: fetch_artifacts.py computes SHA-256 during streaming and deletes the temp file on mismatch before rename — confirm no partial-file window is exploitable (TOCTOU on temp path) and that the temp file is created with restrictive permissions, not world-readable/writable defaults.
• CWE-22 (Path Traversal): main() in fetch_artifacts.py validates filenames against absolute paths, .. segments, and resolves against output_dir — confirm resolution happens before any filesystem write and that symlink attacks (CWE-59) against the resolved output_dir aren't possible if output_dir itself is attacker-influenced.
• CWE-295 (Improper Certificate Validation): httpx.AsyncClient usage not shown to explicitly enforce TLS verification — confirm default cert verification isn't disabled anywhere in the download path.
• Supply chain / lockfile trust boundary (CWE-829, Reliance on Untrusted Inclusion): artifacts.lock.yaml hardcodes download_url + checksum for third-party model/tokenizer artifacts (docling, RapidOCR, Granite, MiniLM). No mechanism shown for verifying the lockfile itself hasn't been tampered with (no signature/provenance check) — a compromised PR merge could silently repoint download_url to a malicious host while keeping a matching checksum for attacker-controlled content.
• CWE-噁78/88-adjacent — shell script injection surface: copy-artifacts.sh accepts an optional first argument overriding the source directory with no input sanitization; confirm this is never fed untrusted external input in CI.
• Deleted install-common.sh: prior ownership/permission-adjustment logic during model provisioning is removed with no equivalent shown in copy-artifacts.sh — confirm cache files don't end up owned by root or with overly permissive modes (CWE-276, Incorrect Default Permissions) in the final image.
• CI/build cache poisoning: /tmp/artifacts cleanup happens after copy, but no verification that copy-artifacts.sh's .cache overlay doesn't clobber existing trusted cache contents with attacker-supplied files of the same name.

🚥 Pre-merge checks | ✅ 8 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Contribution Quality And Spam Detection	⚠️ Warning	FAIL: AI-templated Summary/Test plan body plus new build-time path-validation (CWE-20-style) ships with no tests changed.	Add targeted tests for fetch_artifacts checksum/path checks and rewrite the PR body in repo-specific prose with concrete issue context and risk model.
No Hardcoded Secrets	⚠️ Warning	FAIL: Containerfile embeds a base64-encoded config blob that decodes to hardcoded POSTGRES_PASSWORD:=ogx, violating CWE-259/CWE-798.	Remove the embedded config blob or externalize it; replace hardcoded password/token defaults with env-only placeholders or secret mounts.

✅ Passed checks (8 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main refactor: replacing install-common.sh with a declarative artifact system.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No Weak Cryptography	✅ Passed	PASS: No CWE-327 issues; touched code uses SHA-256 only, SHA-1 is just a tiktoken cache key, and no secret/HMAC comparisons or roll-your-own crypto appear.
No Injection Vectors	✅ Passed	No CWE-89/78/94/502/79 patterns found; fetch_artifacts.py uses yaml.safe_load and path traversal checks, and shell usage is fixed/trusted.
No Privileged Containers	✅ Passed	No explicit privileged flags, host* modes, SYS_ADMIN, allowPrivilegeEscalation, or USER root/runAsUser: 0 were found in changed Dockerfiles/manifests (CWE-266).
No Sensitive Data In Logs	✅ Passed	No CWE-532/CWE-312 issue: touched code only prints artifact names/counts, and no passwords, tokens, PII, or raw bodies are logged.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mergify]

⚠️ Heads up! This PR modifies Containerfile.
A corresponding change may be needed in the Konflux Dockerfile: https://github.com/red-hat-data-services/ogx-distribution/blob/main/Dockerfile.konflux
Please verify if the changes need to be synchronized.

[nathan-weinberg]

One quick question - if we want to add or remove any hermetic artifacts, do we have some path to do so that aligns with this new system?

[derekhiggins]

So I guess doesn't streat we'll have a downstream version of fetch_artifacts.py?

[derekhiggins]

I wonder distribution/copy-artifacts.sh could be avoided completely if we stored the absolute path for the destination here ? distribution/fetch_artifacts.py could then put them where they are needed

We wouldn't then need different copy functions for each model ?

[eoinfennessy]

Yes, this is much better. Maybe we could have a staged file tree in the tmp dir (or the cache dir in the case of Hermeto) and simply copy this tree to the app root. That would simplify things quite a bit.

[eoinfennessy]

Done in 5e922b5. I also cleaned up some other unnecessary stuff in the copy script in 091f69b and e7825d4

[eoinfennessy]

One quick question - if we want to add or remove any hermetic artifacts, do we have some path to do so that aligns with this new system?

Yes, any new files that need to be downloaded at build time must be added to distribution/artifacts.lock.yaml - this is the single source for all non-Python file downloads. The copy script would also need to be modified to copy any new files to the appropriate directory in the container image.

EDIT: the copy script has been improved and is now a generic cp -r from the staging dir to the cache dir. It no longer requires changes when a new artifact is added.

[eoinfennessy]

So I guess doesn't streat we'll have a downstream version of fetch_artifacts.py?

No, Hermeto is configured to fetch all files in distribution/artifacts.lock.yaml, populating a directory that gets mounted at build time. The copy script added in this PR copies files from this dir to the correct location in the container image.

See the draft RHDS PR red-hat-data-services#124 for an indication of how it will look downsteam, and the Hermeto docs for more detail on the generic pre-fetcher: https://github.com/hermetoproject/hermeto/blob/main/docs/generic.md

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@distribution/copy-artifacts.sh`:
- Line 12: The copy step in copy-artifacts.sh is copying the .cache directory
node instead of its contents, which can nest a second .cache under an existing
destination. Update the copy logic around the cp command to copy only the
contents of ${ARTIFACTS_DIR}/.cache into ${APP_ROOT}/.cache so the existing
cache tree is preserved and the HF_HOME, TIKTOKEN_CACHE_DIR, and
DOCLING_ARTIFACTS_PATH paths remain correct.

In `@distribution/fetch_artifacts.py`:
- Around line 1-6: The inline uv dependency list in fetch_artifacts.py is too
loose and can drift during image builds. Update the script’s dependency
declarations to use exact pinned versions for httpx and pyyaml, or switch this
script to a lockfile-backed setup so Containerfile and Containerfile.in builds
resolve deterministic packages. Keep the fix local to the fetch_artifacts.py
dependency metadata so the build environment remains reproducible.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 9eef236a-dcb9-4e17-ae26-cf5e3aa71b7e

📥 Commits

Reviewing files that changed from the base of the PR and between a063894 and e7825d4.

📒 Files selected for processing (6)

• Containerfile
• Containerfile.in
• distribution/artifacts.lock.yaml
• distribution/copy-artifacts.sh
• distribution/fetch_artifacts.py
• distribution/install-common.sh

💤 Files with no reviewable changes (1)

• distribution/install-common.sh

[derekhiggins]

lgtm, one nice followup would be to only download files if they didn't exist, we could then mount a cache dir into the build container when building locally (to avoid downloading each time)

[eoinfennessy]

lgtm, one nice followup would be to only download files if they didn't exist, we could then mount a cache dir into the build container when building locally (to avoid downloading each time)

@derekhiggins good call. Implemented cached mount dir in eecbac6

[derekhiggins]

lgtm, thanks

[Artemon-line]

lgtm, thank you @eoinfennessy

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request