Fix custom image tag handling in installer smoke#216
Conversation
- let install.sh combine DEVA_DOCKER_IMAGE with DEVA_DOCKER_TAG - stop the smoke workflow from handing deva a double-tagged image name - keep custom local smoke images usable without registry pulls
|
Claude finished @lroolle's task in 2m 12s —— View job Security Audit Complete ✅Performed paranoid security audit of PR #216 focusing on container boundaries, auth flows, shell injection, and privilege escalation. Todo List
Security Assessment: APPROVE WITH MONITORING✅ No Blocking Security Issues FoundShell Injection: SECURE
Command Injection: SECURE
Logic Vulnerabilities: SECURE
|
There was a problem hiding this comment.
Pull request overview
This PR fixes Docker image reference construction for the installer and CI smoke test by separating repository and tag handling, preventing doubly-tagged image names and keeping locally-built smoke images usable.
Changes:
- Added an
image_refhelper ininstall.shto combine image repo + tag (or preserve pre-tagged/digest refs) and updated default/fallback image resolution. - Updated CI smoke workflow to export
DEVA_DOCKER_IMAGEandDEVA_DOCKER_TAGseparately instead of passing a pre-tagged image string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| install.sh | Introduces image_ref and updates how DOCKER_IMAGE / DOCKER_IMAGE_FALLBACK are computed from env vars. |
| .github/workflows/ci.yml | Adjusts smoke job env vars to avoid passing a pre-tagged image name to the installer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if [ -n "$tag" ]; then | ||
| printf '%s:%s' "$repo" "$tag" | ||
| return | ||
| fi | ||
|
|
||
| if [[ "$tail" == *:* ]]; then | ||
| printf '%s' "$repo" | ||
| return | ||
| fi |
There was a problem hiding this comment.
image_ref appends :$tag before checking whether repo already contains a tag (via tail containing :). If DEVA_DOCKER_IMAGE is already tagged (e.g. deva-smoke:ci) and DEVA_DOCKER_TAG is also set, this will produce an invalid ref like deva-smoke:ci:ci. Consider detecting an existing tag first (or stripping it) when a separate tag is provided, and define clear precedence between an inline tag and DEVA_DOCKER_TAG to avoid generating invalid image refs.
| if [ -n "${DEVA_DOCKER_IMAGE+x}" ]; then | ||
| DOCKER_IMAGE="$(image_ref "$DEVA_DOCKER_IMAGE" "${DEVA_DOCKER_TAG:-}" "latest")" | ||
| else | ||
| DOCKER_IMAGE="$(image_ref "ghcr.io/thevibeworks/deva" "${DEVA_DOCKER_TAG:-}" "latest")" | ||
| fi |
There was a problem hiding this comment.
The DEVA_DOCKER_IMAGE presence check uses ${DEVA_DOCKER_IMAGE+x}, so an explicitly-empty DEVA_DOCKER_IMAGE will be treated as set and passed to image_ref, resulting in an invalid image like :latest. Previously, an empty value would fall back to the default image via ${VAR:-default}. Consider treating empty DEVA_DOCKER_IMAGE the same as unset (e.g., check -n "${DEVA_DOCKER_IMAGE:-}"), or validate and error with a clear message.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22f1ff7348
ℹ️ 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".
| if [ -n "${DEVA_DOCKER_IMAGE+x}" ]; then | ||
| DOCKER_IMAGE="$(image_ref "$DEVA_DOCKER_IMAGE" "${DEVA_DOCKER_TAG:-}" "latest")" |
There was a problem hiding this comment.
Treat empty DEVA_DOCKER_IMAGE as unset
If DEVA_DOCKER_IMAGE is present but empty (a common outcome of templated .env files), this branch still treats it as configured and calls image_ref with an empty repo, which produces :latest; the subsequent docker image inspect/docker pull then fails with an invalid image reference. Prior to this commit, ${DEVA_DOCKER_IMAGE:-...} fell back to the default image for empty values, so this is a regression in installer behavior for blank env configuration.
Useful? React with 👍 / 👎.
Summary
Verification
Problem
main CI failed because the workflow built
deva-smoke:cibut deva still looked fordeva-smoke:ci:latest