Conversation
When exec'ing into service containers with custom images (like postgres), the app's entrypoint was incorrectly being applied. This happened because all containers in a sandbox share the same version-entity label, even when they use different images. Now we compare the container's image to the AppVersion's image and only apply the entrypoint when they match. Service containers with custom images get a plain shell without the app entrypoint wrapper.
📝 WalkthroughWalkthroughThe changes implement image-based gating for app entrypoint application in the exec server. A new helper function validates whether a container's image matches the app version's configured image. Enhanced fallback logic now prioritizes console commands from the app version, then the entrypoint, then defaults to /bin/sh. Comprehensive tests validate command resolution, OCI spec generation, terminal handling, and image matching scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
servers/exec/exec.go (1)
259-274: Consider adding empty string guard.The image matching logic correctly handles exact matches and registry prefixes. However, if both
containerImageandappVersionImageare empty strings, the exact match would returntrue, potentially applying an empty entrypoint.While this edge case is unlikely in practice (containers should always have images, and AppVersions typically have ImageUrls), an explicit guard would make the intent clearer.
🔎 Optional improvement
func imageMatchesAppVersion(containerImage, appVersionImage string) bool { + // Empty app version image means no entrypoint should be applied + if appVersionImage == "" { + return false + } + // Exact match if containerImage == appVersionImage { return true }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
servers/exec/exec.goservers/exec/exec_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow standard Go formatting conventions
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Good comments should explain complex logic, document assumptions, or clarify non-obvious behavior rather than restating the code
Function/method comments should explain the purpose and any important side effects, not just restate the function name
Files:
servers/exec/exec_test.goservers/exec/exec.go
🧬 Code graph analysis (1)
servers/exec/exec_test.go (3)
servers/exec/exec.go (1)
Server(20-27)api/core/core_v1alpha/schema.gen.go (3)
AppVersion(82-91)Commands(308-311)Env(571-576)api/exec/exec_v1alpha/rpc.gen.go (2)
ShellOptions(76-78)WindowSize(18-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-binaries (darwin, arm64, macos-latest)
- GitHub Check: build-binaries (darwin, amd64, macos-latest)
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: test-e2e
🔇 Additional comments (3)
servers/exec/exec.go (2)
110-126: LGTM! Effective fix for the service container entrypoint issue.The image-matching logic correctly gates the application of the app's entrypoint configuration. The error-tolerant approach (logging and continuing when image retrieval fails) is appropriate, ensuring that exec operations can proceed even if image metadata is temporarily unavailable. Debug logging provides good visibility for troubleshooting mismatches.
221-240: LGTM! Solid nil-safety and fallback logic.The nil check prevents panics when dealing with service containers, and the fallback chain (console command → entrypoint + shell → plain shell) is well-structured and intuitive.
servers/exec/exec_test.go (1)
1-387: Excellent test coverage! Well-structured and comprehensive.The test suite thoroughly validates the fix across multiple dimensions:
- Command resolution with/without entrypoints
- OCI spec generation for app vs. service containers
- Terminal handling with console sizing
- Image matching logic with registry prefixes and edge cases
The tests also serve as excellent documentation, particularly
TestEntrypointNotAppliedToCustomImageContainers, which explicitly captures the bug fix behavior.
Summary
miren sandbox execincorrectly applying the app's entrypoint to service containers with custom images (like postgres)imageMatchesAppVersion()helper to compare container images and only apply entrypoint when they matchspec(),command(), and image matching logicTest plan
command()functionspec()function covering all entrypoint scenariosimageMatchesAppVersion()with various image name formatsgo test ./servers/exec/...)golangci-lint run ./servers/exec/...)Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.