Skip to content

Conversation

@cooktheryan
Copy link
Collaborator

No description provided.

Signed-off-by: Ryan Cook <rcook@redhat.com>
@Gkrumbach07
Copy link
Collaborator

Tracked in Jira: https://issues.redhat.com/browse/RHOAIENG-39126

Signed-off-by: Ryan Cook <rcook@redhat.com>
@codecov
Copy link

codecov bot commented Dec 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

This comment has been minimized.

Signed-off-by: Ryan Cook <rcook@redhat.com>
Signed-off-by: Ryan Cook <rcook@redhat.com>
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2025

Claude Code Review

Summary

This PR successfully converts the Claude Code runner from Debian-based python:3.11-slim to Red Hat UBI9 with Python 3.11. This is an important migration for Red Hat OpenShift compatibility and enterprise support. The changes are well-executed with proper OpenShift permissions and multi-stage improvements.

Overall Assessment: ✅ Approved with minor recommendations

Issues by Severity

🟡 Major Issues

1. Package Manager Change: dnf vs microdnf

  • Current: Uses dnf (line 6-9)
  • Pattern: Other UBI-based images in repo use microdnf (backend, operator, frontend)
  • Impact: Larger image size, slower builds
  • Evidence:
    • components/backend/Dockerfile:23: microdnf install -y git
    • components/operator/Dockerfile:24: microdnf install -y procps
  • Recommendation: Switch to microdnf for consistency and smaller images
  • Note: The commit message (a709849) says "dnf not microdnf" suggesting this was intentional, but goes against established patterns

2. Hardcoded Base Image Digest

  • Current: Uses pinned digest @sha256:d0b35f779ca0ae87deaf17cd1923461904f52d3ef249a53dbd487e02bdabdde6
  • Benefit: Reproducible builds ✅
  • Risk: Image becomes stale, missing security patches
  • Recommendation: Document image update process or use Dependabot/Renovate for automated digest updates
  • Alternatives: Consider using version tag :latest or :9.5 with periodic manual updates

🔵 Minor Issues

1. Missing curl Installation

  • Previous: Installed curl explicitly
  • Current: Removed
  • Analysis: UBI9 Python image likely includes curl by default, but not verified
  • Recommendation: Add curl explicitly if needed for debugging or runtime operations
  • Risk: Low - likely already present in base image

2. Missing ca-certificates Installation

  • Previous: Installed ca-certificates
  • Current: Removed
  • Analysis: UBI images include CA certs, but explicit installation ensures HTTPS works
  • Recommendation: Add back if SSL/TLS operations fail
  • Risk: Low - UBI includes certs by default

3. GitHub CLI Repository Add Method Change

  • Previous: curl -fsSL ... -o /etc/yum.repos.d/gh-cli.repo
  • Current: dnf config-manager --add-repo ...
  • Analysis: Both methods work; new method requires dnf-command(config-manager) package
  • Impact: Neutral - different approach, same result
  • Note: Adds extra package dependency but cleaner

Positive Highlights

Excellent OpenShift Compatibility

  • Proper USER 0 → 1001 switching (lines 3, 37)
  • Correct group permissions: chmod -R g=u (line 35)
  • Follows established pattern from backend/operator Dockerfiles

Reproducible Builds

  • Pinned base image digest ensures consistency
  • dnf clean all reduces image size (line 10)

Security Best Practices

  • Non-root user (1001) for runtime
  • Minimal package installation
  • No unnecessary privileges

Code Quality

  • Clean, readable Dockerfile structure
  • Proper layer ordering (dependencies → app code)
  • Uses pip --no-cache-dir flag (lines 17, 23-24)

Correct Dependency Migration

  • Removed npm/node.js (not needed - uses Python claude-agent-sdk instead)
  • Retained git, jq, gh (all required for runtime operations)
  • Previous review suggesting npm was needed was incorrect

Recommendations

Priority 1: Consistency (Medium Priority)

Consider switching to microdnf to match other components:

# Add GitHub CLI repository and install packages
RUN curl -fsSL https://cli.github.com/packages/rpm/gh-cli.repo -o /etc/yum.repos.d/gh-cli.repo && \
    microdnf install -y git jq gh && \
    microdnf clean all

Rationale: Matches backend.Dockerfile:23 and operator.Dockerfile:24 patterns

Priority 2: Image Maintenance (Low Priority)

Document base image update process:

  • Add comment above FROM line explaining digest update policy
  • Or: Set up automated digest updates via Dependabot/Renovate

Priority 3: Build Verification (Complete)

✅ CI/CD pipeline shows build succeeded for both linux/amd64 and linux/arm64

Testing Recommendations

Before merging, verify:

  • Image builds successfully (✅ already passing in CI)
  • Container starts with USER 1001 (no permission errors)
  • Git operations work (git --version, clone operations)
  • GitHub CLI works (gh --version, auth operations)
  • Python packages install correctly
  • Runtime AgenticSession can clone repos and execute

Additional Context

Why npm was removed (commit 7d79c81):

claude-agent-sdk now bundles its own CLI so no longer needed to npm install. Without this change, the runner breaks on next update.

Architecture Note: This runner uses Python claude-agent-sdk (pip package), NOT the npm-based @anthropic-ai/claude-code CLI. The previous review suggesting npm installation was based on outdated understanding of the architecture.

Multi-platform Support: Workflow successfully builds for both linux/amd64 and linux/arm64 as shown in CI checks.


Final Verdict: This is a solid migration to UBI. The only concern is the inconsistency with dnf vs microdnf. Given the intentional commit message, this may be deliberate (perhaps gh CLI repo requires full dnf). If so, consider adding a comment explaining why.

Recommendation: ✅ Approve and merge after considering microdnf consistency.

@ambient-code ambient-code deleted a comment from github-actions bot Dec 1, 2025
@ambient-code ambient-code deleted a comment from github-actions bot Dec 1, 2025
@cooktheryan cooktheryan changed the title WIP: convert the runner to UBI convert the runner to UBI Dec 1, 2025
Copy link
Collaborator

@sallyom sallyom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sallyom sallyom merged commit 45fad93 into ambient-code:main Dec 3, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants