Conversation
Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ibuziuk The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ibuziuk
changed the title
📝 WalkthroughWalkthroughBoth the ubi10 and ubi9 base Dockerfiles are updated with a new build step that downloads and installs NVIDIA OpenShell and OpenShell gateway RPM packages, selecting architecture-specific package files based on TARGETARCH, skipping or exiting for unsupported architectures, and cleaning up temporary files afterward. ChangesNVIDIA OpenShell RPM Installation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Build as Docker Build
participant Script as Install Script
participant GitHub as GitHub Release
participant RPM as rpm Package Manager
Build->>Script: Determine TARGETARCH
alt ppc64le or unsupported
Script-->>Build: Skip / exit
else amd64 or arm64
Script->>GitHub: curl download OpenShell RPM
Script->>GitHub: curl download OpenShell gateway RPM
alt Downloads succeed
Script->>RPM: rpm -ivh --nodeps
RPM-->>Script: Packages installed
else Download fails
Script-->>Build: Log skip message
end
Script->>Script: Remove temp directory
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@base/ubi9/Dockerfile`:
- Around line 196-201: The OpenShell install block currently swallows download
failures for supported arches and uses rpm -ivh --nodeps, which allows the layer
to pass without a valid install. Update the Dockerfile logic around the
OPENSHELL_URL/OPENSHELL_RPM downloads so that amd64 and arm64 builds fail
immediately if either RPM cannot be fetched, while non-supported arches may
still skip installation. Replace the rpm -ivh --nodeps step with a dnf-based
local RPM install so dependency validation is enforced during the build.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea969413-f800-46f1-bacb-523a906d597d
📒 Files selected for processing (2)
base/ubi10/Dockerfilebase/ubi9/Dockerfile
| if curl -fsSL "${OPENSHELL_URL}/${OPENSHELL_RPM}" -o "${OPENSHELL_RPM}" && \ | ||
| curl -fsSL "${OPENSHELL_URL}/${OPENSHELL_GW_RPM}" -o "${OPENSHELL_GW_RPM}"; then \ | ||
| rpm -ivh --nodeps "${OPENSHELL_RPM}" "${OPENSHELL_GW_RPM}"; \ | ||
| else \ | ||
| echo "OpenShell binary not found for ${TARGETARCH}, skipping installation."; \ | ||
| fi; \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding context.
git ls-files base/ubi9/Dockerfile
wc -l base/ubi9/Dockerfile
sed -n '160,220p' base/ubi9/Dockerfile
# Look for related OpenShell handling and any dnf/rpm install patterns in the repo.
rg -n "OPENSHELL_|rpm -ivh --nodeps|dnf install -y \"\./" base/ubi9/Dockerfile base -g 'Dockerfile' -g '*.sh' -g '*.yml' -g '*.yaml' -g '*.md'Repository: devfile/developer-images
Length of output: 5235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether OpenShell is referenced elsewhere as optional or required.
rg -n "OpenShell|openshell|gateway" . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'
# Inspect the rest of the UBI9 Dockerfile for dnf usage patterns and whether package installs normally fail the build.
sed -n '1,160p' base/ubi9/Dockerfile
sed -n '160,260p' base/ubi9/DockerfileRepository: devfile/developer-images
Length of output: 13009
Fail the layer when OpenShell is unavailable
Supported amd64/arm64 builds can still succeed without OpenShell if either download fails, and rpm -ivh --nodeps skips dependency validation. Fail the build for supported arches and install the local RPMs with dnf instead.
🤖 Prompt for 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.
In `@base/ubi9/Dockerfile` around lines 196 - 201, The OpenShell install block
currently swallows download failures for supported arches and uses rpm -ivh
--nodeps, which allows the layer to pass without a valid install. Update the
Dockerfile logic around the OPENSHELL_URL/OPENSHELL_RPM downloads so that amd64
and arm64 builds fail immediately if either RPM cannot be fetched, while
non-supported arches may still skip installation. Replace the rpm -ivh --nodeps
step with a dnf-based local RPM install so dependency validation is enforced
during the build.
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
|
Pull Request UBI 10 images published ✨ Base: quay.io/devfile/base-developer-image:ubi10-pr-260 |
|
Pull Request images published ✨ Base: quay.io/devfile/base-developer-image:pr-260 |
|
/che-ai-assistant ok-pr-review Review is complete. Please check the review comments below. Review is complete. Please check the review comments below. |
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Comprehensive PR Review
Verdict: 🔄 Request Changes - Critical security and reliability issues must be addressed before merge.
This PR adds NVIDIA OpenShell v0.0.78 to the base developer images. The implementation follows existing patterns correctly, but has critical gaps in supply chain security, error handling, and testing.
❌ Must Fix (Blockers)
- Missing SHA256 checksum verification - Supply chain vulnerability for security-critical sandbox runtime
- Silent failure on download errors - Build succeeds even when curl fails on supported architectures
- No functional testing - fc44 (Fedora 44) RPMs on RHEL-based UBI compatibility unverified
⚠️ Should Fix
- README.md tool matrix not updated
- Missing use case justification in PR description
- No post-install smoke test to verify binaries work
📋 Review Coverage
- ✅ Standard code review (correctness, security, best practices)
- ✅ Deep design analysis (thoughtfulness, testing rigor, anti-patterns)
- ✅ System-level impact (operational readiness, supply chain, compatibility, evolvability)
Detailed findings in inline comments below ⬇️
Positive Feedback ✅
- Clean architecture detection logic with proper RPM arch mapping
- Consistent implementation across UBI 9 and UBI 10
- Pattern matches existing tools (gh-cli, bat, ripgrep)
- Proper version pinning and temp directory cleanup
- Comment links directly to GitHub release
OpenShell Project Health
- ✅ Actively maintained (commits within 24 hours)
- ✅ Apache 2.0 license compatible with EPL 2.0
⚠️ Pre-1.0 version (0.0.78) - API instability expected⚠️ Released yesterday (July 7, 2026) - very recent
Image Size Impact
Base image grows 800MB → ~815MB (+1.9%, still under 1GB guideline). Universal image inherits the same increase.
Design Question for Maintainers
Should OpenShell be in the universal image instead of base?
OpenShell is a "sandbox runtime for autonomous AI agents" - fundamentally a runtime environment, not a CLI tool like git/jq. The universal image tier is designed for "language SDKs and runtimes" which seems like a better fit.
Impact: Current placement adds ~28MB to every base image consumer who doesn't need AI agent sandboxing.
Worth discussing the right tier for this tool.
Review generated by che-ai-assistant with comprehensive code, design, and impact analysis.
❌ BLOCKER: Add SHA256 checksum verificationOpenShell is a security-sensitive sandbox runtime for AI agents. Downloading RPMs without checksum verification exposes the supply chain to attacks. Current code downloads without verification: curl -fsSL "${OPENSHELL_URL}/${OPENSHELL_RPM}" -o "${OPENSHELL_RPM}" && \
curl -fsSL "${OPENSHELL_URL}/${OPENSHELL_GW_RPM}" -o "${OPENSHELL_GW_RPM}" && \
rpm -ivh --nodeps "${OPENSHELL_RPM}" "${OPENSHELL_GW_RPM}"Required: Fetch checksums from https://github.com/NVIDIA/OpenShell/releases/tag/v0.0.78 and verify before installation: # Architecture-specific checksums from release page
case "$TARGETARCH" in
amd64)
OPENSHELL_SHA256="<x86_64-openshell-checksum-from-release>"
OPENSHELL_GW_SHA256="<x86_64-gateway-checksum-from-release>" ;;
arm64)
OPENSHELL_SHA256="<aarch64-openshell-checksum-from-release>"
OPENSHELL_GW_SHA256="<aarch64-gateway-checksum-from-release>" ;;
esac
curl -fsSL "${OPENSHELL_URL}/${OPENSHELL_RPM}" -o "${OPENSHELL_RPM}" && \
curl -fsSL "${OPENSHELL_URL}/${OPENSHELL_GW_RPM}" -o "${OPENSHELL_GW_RPM}" && \
echo "${OPENSHELL_SHA256} ${OPENSHELL_RPM}" | sha256sum -c - && \
echo "${OPENSHELL_GW_SHA256} ${OPENSHELL_GW_RPM}" | sha256sum -c - && \
rpm -ivh --nodeps "${OPENSHELL_RPM}" "${OPENSHELL_GW_RPM}"Why critical: A backdoored OpenShell could compromise sandbox isolation in Eclipse Che workspaces. This aligns with Red Hat's security standards for container content. Industry standard: Kubernetes, Docker official images, and Red Hat packages all verify checksums for external downloads. |
❌ BLOCKER: Fix silent failure on download errorsThe current else branch has no exit statement. If curl fails on amd64/arm64, the build continues and produces an image without OpenShell but CI shows green. Problem scenario:
Fix: Add explicit exit on download failure: if curl -fsSL "${OPENSHELL_URL}/${OPENSHELL_RPM}" -o "${OPENSHELL_RPM}" && \
curl -fsSL "${OPENSHELL_URL}/${OPENSHELL_GW_RPM}" -o "${OPENSHELL_GW_RPM}"; then \
rpm -ivh --nodeps "${OPENSHELL_RPM}" "${OPENSHELL_GW_RPM}"; \
else \
echo "ERROR: Failed to download OpenShell for ${TARGETARCH}"; \
exit 1; \
fiThis distinguishes:
|
|
2 participants
Summary by CodeRabbit