Skip to content

Conversation

@bobbravo2
Copy link
Contributor

@bobbravo2 bobbravo2 commented Nov 19, 2025

Changes

This PR refactors the claude-code-runner Dockerfile to use uv and uv.lock for faster, more reproducible builds.

What Changed

  • ✅ Added uv installation from official ghcr.io image
  • ✅ Replaced pip install with uv pip install for runner-shell
  • ✅ Use uv sync --frozen --no-dev for claude-runner dependencies
  • ✅ Updated CMD to use uv run --frozen for reproducible execution
  • ✅ Set UV_SYSTEM_PYTHON=1 for system Python compatibility
  • ✅ Copy uv.lock to ensure locked dependency versions

Benefits

  • 🚀 Faster build times: uv is significantly faster than pip
  • 🔒 Reproducible builds: uv.lock ensures exact dependency versions across all builds
  • 📦 Better dependency resolution: Superior conflict resolution
  • 💾 Smaller layers: More efficient caching

Testing

  • Dockerfile syntax validated
  • Tested locally with podman build
cd components/runners && podman build -f claude-code-runner/Dockerfile -t vteam-claude-runner:test .
  • Build test needed in CI

Related

- Add uv installation from official ghcr.io image
- Replace pip install with uv pip install for runner-shell
- Use uv sync --frozen --no-dev for claude-runner dependencies
- Update CMD to use uv run --frozen for reproducible execution
- Set UV_SYSTEM_PYTHON=1 for system Python compatibility
- Copy uv.lock to ensure locked dependency versions

Benefits:
- Faster build times with uv's optimized dependency resolution
- Reproducible builds via uv.lock
- Better caching and smaller image layers
@github-actions

This comment has been minimized.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 34 to 36
# Copy claude-runner files including uv.lock
COPY claude-code-runner/pyproject.toml claude-code-runner/uv.lock /app/claude-runner/
COPY claude-code-runner/wrapper.py /app/claude-runner/

Choose a reason for hiding this comment

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

P1 Badge Copy CLAUDE.md required by uv sync

The Docker build now only copies pyproject.toml, uv.lock, and wrapper.py into /app/claude-runner, but pyproject.toml declares readme = "CLAUDE.md". When uv sync --frozen --no-dev runs later in the Dockerfile, uv/setuptools will try to read that file while building project metadata and fail with FileNotFoundError, so the image cannot be built. The previous Dockerfile copied the entire claude-code-runner directory (including CLAUDE.md), so this build failure is newly introduced.

Useful? React with 👍 / 👎.

…entation and caching

- Add build context documentation at top of file
- Optimize layer caching by separating dependency files from application code
- Add comprehensive inline comments explaining:
  * Why UV_SYSTEM_PYTHON=1 is needed
  * Build context requirements
  * Why 'uv sync' vs 'uv pip install'
  * Why '--frozen' flag is important
  * Why 'uv run' is necessary (venv isolation)
- Verify CI workflow already uses correct build context

Addresses all feedback from Claude Code Review:
- Build context documentation (Major Issue #1) ✅
- Layer optimization (Minor Issue ambient-code#3) ✅
- Command documentation (Minor Issue ambient-code#4) ✅
- Inline comments (Minor Issue ambient-code#5) ✅

CI verification confirms:
- context: ./components/runners
- dockerfile: ./components/runners/claude-code-runner/Dockerfile
@bobbravo2
Copy link
Contributor Author

Addressed PR Feedback

Thank you for the comprehensive review! I've addressed all the recommendations:

✅ Completed

Major Issues:

  1. Build Context Documentation - Added comment at top of Dockerfile documenting the expected build context
  2. CI Build Context Validation - Verified GitHub Actions workflow (lines 64-68) correctly uses:
    • context: ./components/runners
    • dockerfile: ./components/runners/claude-code-runner/Dockerfile

Minor Issues:
3. Layer Optimization - Separated dependency installation from code copy:

  • Dependencies copied first (line 35)
  • Dependencies installed (line 39-43)
  • Application code copied last (line 46)
  • This improves caching - when only wrapper.py changes, dependency layer remains cached
  1. Command Documentation - Added comprehensive comment explaining why uv run is necessary:

    • Dependencies are isolated in /app/claude-runner/.venv
    • uv run --frozen activates the venv and uses locked dependencies
    • Verified this is required (tested both approaches)
  2. Inline Comments - Added detailed comments throughout explaining:

    • UV_SYSTEM_PYTHON=1 purpose
    • Build context requirements
    • uv sync vs uv pip install rationale
    • --frozen flag importance
    • OpenShift compatibility permissions

🧪 Testing

✅ Built successfully with podman:

cd components/runners
podman build -f claude-code-runner/Dockerfile -t vteam-claude-runner:test .

✅ Verified all key dependencies are available:

  • anthropic 0.68.0
  • claude-agent-sdk 0.1.4
  • requests 2.32.5
  • pyjwt 2.10.1

✅ Confirmed layer caching optimization works - most layers cached on rebuild

Ready for merge! 🚀

@github-actions
Copy link
Contributor

Claude Code Review

Summary

This PR successfully migrates the claude-code-runner Dockerfile from pip to uv for faster, more reproducible builds. The implementation follows best practices with proper layering and caching. However, there are critical issues that need attention before merging.

Issues by Severity

Blocker Issues

1. Missing uv.lock file validation

  • Location: Dockerfile:19
  • Issue: Copies uv.lock but no validation it exists
  • Impact: Build fails if uv.lock missing/not committed

2. Inconsistent package management

  • Location: Lines 33-34
  • Issue: runner-shell uses 'uv pip install' while claude-runner uses 'uv sync'
  • Impact: runner-shell dependencies not locked, defeats reproducibility purpose
  • runner-shell has deps: websockets, aiobotocore, pydantic, aiofiles, click, anthropic

Critical Issues

3. UV_SYSTEM_PYTHON=1 bypasses isolation

  • Location: Line 25
  • Security concern: Reduces isolation between dependencies and system packages
  • Recommendation: Document rationale or use uv managed Python

4. Build context documentation

  • Location: Lines 1-2
  • Issue: Comments say 'components/runners/' - verify CI workflow matches

Major Issues

5. Missing error handling for uv sync

  • Location: Line 42
  • Issue: uv sync --frozen fails silently if lock out of sync

6. No uv installation validation

  • Location: Line 13
  • Risk: Build fails later with unclear errors

7. Incomplete migration docs

  • PR mentions testing needed but no docs on regenerating uv.lock

Positive Highlights

✓ Excellent layering: Dependencies before code
✓ Clear comments throughout
✓ Reproducible: --frozen flag
✓ Security conscious: Drops capabilities
✓ Follows CLAUDE.md standards
✓ Clean migration

Recommendations

Must Fix Before Merge:

  1. Verify uv.lock exists and is committed
  2. Fix runner-shell dependency management inconsistency
  3. Document UV_SYSTEM_PYTHON=1 rationale
  4. Verify build context matches CI

Should Fix:
5. Add error handling for uv sync
6. Validate uv installation
7. Add migration documentation

Nice to Have:
8. Add .dockerignore
9. Enhance comments

Conclusion

Solid migration with good structure. Main concerns: runner-shell consistency and reproducibility. Once blockers addressed, this will significantly improve build times.

Recommendation: Request changes for blockers 1 and 2

@bobbravo2
Copy link
Contributor Author

Response to Latest Review Comments

Thank you for the detailed review! Let me address the concerns:

Blocker Issues

1. ✅ Missing uv.lock file validation

  • Status: uv.lock exists and is committed to git (190KB file)
  • Verification:
    $ ls -lh components/runners/claude-code-runner/uv.lock
    -rw-r--r--@ 1 bgregor  staff   190K Nov 14 18:18 uv.lock
    
    $ git ls-files components/runners/claude-code-runner/uv.lock
    components/runners/claude-code-runner/uv.lock
  • CI Confirmation: The build passed (6m42s), which validates the lock file works correctly

2. 🤔 Inconsistent package management

  • Status: This is intentional design, but I understand the concern
  • Rationale:
    • runner-shell is a shared base framework (like a system library)
    • claude-runner is the application with specific locked dependencies
    • Pattern: Base framework installed globally, application dependencies isolated in venv
  • Trade-off: You're right that runner-shell isn't locked in this PR. However:
    • This is a separate concern from migrating claude-runner to uv
    • runner-shell could be migrated to use uv.lock in a separate PR
    • For now, maintaining the existing pattern for runner-shell minimizes risk
  • Alternative: I could create runner-shell/uv.lock and use uv sync there too, but that's a larger change

Critical Issues

3. ✅ UV_SYSTEM_PYTHON=1 bypasses isolation

  • Rationale: We're using the Python from the base image (python:3.11-slim)
  • Why needed: Without this flag, uv would try to manage its own Python installations
  • Security: The venv still provides isolation - dependencies are in /app/claude-runner/.venv
  • Documentation: Already added in the Dockerfile (line 25)

CI Status

All checks passing:

  • claude-code-runner build: pass (6m42s)
  • End-to-End Tests: pass (3m41s)
  • All other builds: pass

The build system validates that:

  • uv.lock is present and valid
  • Dependencies resolve correctly
  • The image builds successfully for linux/amd64 and linux/arm64

Recommendation

The current implementation is production-ready:

  • ✅ uv.lock is committed and validated by CI
  • ✅ UV_SYSTEM_PYTHON=1 is documented and necessary
  • ⚠️ runner-shell could use uv.lock (enhancement for future PR)

Suggested path forward:

  1. Merge this PR (claude-runner migration is complete and tested)
  2. Open follow-up issue: "Migrate runner-shell to use uv.lock" for consistency

What do you think? 🤔

The pyproject.toml referenced 'readme = "CLAUDE.md"' but this file
does not exist in the claude-code-runner directory. While uv handled
this gracefully during build, it's a latent bug that could cause issues.

Changes:
- Removed readme field from pyproject.toml
- Regenerated uv.lock to reflect the updated metadata

The build and runtime tests pass successfully with this fix.

Addresses feedback about missing CLAUDE.md file during layer optimization.
@bobbravo2
Copy link
Contributor Author

Fixed: Missing CLAUDE.md Reference

Good catch! You were absolutely right about this issue.

The Problem

The pyproject.toml referenced:

readme = "CLAUDE.md"

But CLAUDE.md doesn't exist in the claude-code-runner directory. When the Dockerfile was optimized to copy only specific files (pyproject.toml, uv.lock, wrapper.py), it would fail to find the readme during uv sync.

Why my builds succeeded: uv appears to handle missing readme files gracefully (it's optional metadata), so the build didn't fail. However, this is still a latent bug that should be fixed.

The Fix

✅ Removed the readme = "CLAUDE.md" line from pyproject.toml
✅ Regenerated uv.lock to reflect the updated metadata
✅ Verified build and runtime still work correctly

Testing

# Build test
cd components/runners
podman build -f claude-code-runner/Dockerfile -t test .
# ✅ SUCCESS

# Runtime test  
podman run --rm test bash -c "cd /app/claude-runner && uv run --frozen python -c 'import anthropic; import claude_agent_sdk; print(\"OK\")'"
# ✅ All dependencies working

Thank you for catching this! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant