[codex] Add release sanity checks#491
Merged
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new release sanity check script (ci/release/sanity.sh), integrates it into the Makefile via the release-sanity target, and updates the release process documentation. The script automates local checks, package validation, remote artifact verification, and Docker Compose end-to-end smoke tests. Feedback on the changes focuses on security and reliability improvements in the shell script. Specifically, the reviewer recommends replacing insecure temporary file creation (using $$ in /tmp) with mktemp or direct evaluation, fixing a logic bug where platform validation failures are silently ignored due to && chaining, redirecting Helm template output to /dev/null to avoid shared environment conflicts, and ensuring all temporary files are properly cleaned up in the exit trap handler.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+85
to
+166
| load_detected_versions() { | ||
| need python3 | ||
|
|
||
| local versions_file | ||
| versions_file="${TMPDIR:-/tmp}/flame-release-sanity-versions.$$" | ||
|
|
||
| python3 - "$ROOT_DIR" >"$versions_file" <<'PY' | ||
| import ast | ||
| import pathlib | ||
| import shlex | ||
| import sys | ||
|
|
||
| try: | ||
| import tomllib | ||
| except ModuleNotFoundError as exc: | ||
| raise SystemExit("python3 with tomllib is required") from exc | ||
|
|
||
| root = pathlib.Path(sys.argv[1]) | ||
|
|
||
|
|
||
| def load_toml(path): | ||
| with path.open("rb") as f: | ||
| return tomllib.load(f) | ||
|
|
||
|
|
||
| def shell_var(name, value): | ||
| if not value: | ||
| raise SystemExit(f"missing value for {name}") | ||
| print(f"{name}={shlex.quote(str(value))}") | ||
|
|
||
|
|
||
| def package_version(path): | ||
| data = load_toml(path) | ||
| return data["package"]["version"] | ||
|
|
||
|
|
||
| def pyproject_version(path): | ||
| data = load_toml(path) | ||
| return data["project"]["version"] | ||
|
|
||
|
|
||
| def python_init_version(path): | ||
| tree = ast.parse(path.read_text()) | ||
| for node in tree.body: | ||
| if not isinstance(node, ast.Assign): | ||
| continue | ||
| for target in node.targets: | ||
| if isinstance(target, ast.Name) and target.id == "__version__": | ||
| if isinstance(node.value, ast.Constant) and isinstance(node.value.value, str): | ||
| return node.value.value | ||
| raise SystemExit(f"missing __version__ assignment in {path}") | ||
|
|
||
|
|
||
| def chart_top_level(path, key): | ||
| prefix = f"{key}:" | ||
| for line in path.read_text().splitlines(): | ||
| if line.startswith((" ", "\t", "#")): | ||
| continue | ||
| stripped = line.strip() | ||
| if stripped.startswith(prefix): | ||
| return stripped.split(":", 1)[1].strip().strip("\"'") | ||
| raise SystemExit(f"missing {key} in {path}") | ||
|
|
||
|
|
||
| sdk_rust = load_toml(root / "sdk/rust/Cargo.toml") | ||
| stdng_dep = sdk_rust["dependencies"]["stdng"] | ||
| macros_dep = sdk_rust["dependencies"]["flame-rs-macros"] | ||
|
|
||
| shell_var("DETECTED_PYPROJECT_VERSION", pyproject_version(root / "sdk/python/pyproject.toml")) | ||
| shell_var("DETECTED_PYTHON_INIT_VERSION", python_init_version(root / "sdk/python/src/flamepy/__init__.py")) | ||
| shell_var("DETECTED_FLAME_RS_VERSION", package_version(root / "sdk/rust/Cargo.toml")) | ||
| shell_var("DETECTED_FLAME_RS_MACROS_VERSION", package_version(root / "sdk/rust/macros/Cargo.toml")) | ||
| shell_var("DETECTED_STDNG_VERSION", package_version(root / "stdng/Cargo.toml")) | ||
| shell_var("DETECTED_CHART_APP_VERSION", chart_top_level(root / "charts/flame/Chart.yaml", "appVersion")) | ||
| shell_var("DETECTED_FLAME_RS_STDNG_DEP_VERSION", stdng_dep.get("version")) | ||
| shell_var("DETECTED_FLAME_RS_MACROS_DEP_VERSION", macros_dep.get("version")) | ||
| PY | ||
|
|
||
| # shellcheck disable=SC1090 | ||
| . "$versions_file" | ||
| rm -f "$versions_file" | ||
| } |
Contributor
There was a problem hiding this comment.
The current implementation of load_detected_versions creates a temporary file in /tmp using the process ID ($$), which is insecure and can lead to symlink attacks or conflicts in shared environments. Additionally, if the script fails before the file is deleted, it leaves orphaned files behind.
We can completely avoid creating any temporary files by directly evaluating the stdout of the Python script using eval. Since the Python script is a hardcoded heredoc and safely quotes its output using shlex.quote, this is both secure and highly efficient.
load_detected_versions() {
need python3
eval "$(python3 - "$ROOT_DIR" <<'PY'
import ast
import pathlib
import shlex
import sys
try:
import tomllib
except ModuleNotFoundError as exc:
raise SystemExit("python3 with tomllib is required") from exc
root = pathlib.Path(sys.argv[1])
def load_toml(path):
with path.open("rb") as f:
return tomllib.load(f)
def shell_var(name, value):
if not value:
raise SystemExit(f"missing value for {name}")
print(f"{name}={shlex.quote(str(value))}")
def package_version(path):
data = load_toml(path)
return data["package"]["version"]
def pyproject_version(path):
data = load_toml(path)
return data["project"]["version"]
def python_init_version(path):
tree = ast.parse(path.read_text())
for node in tree.body:
if not isinstance(node, ast.Assign):
continue
for target in node.targets:
if isinstance(target, ast.Name) and target.id == "__version__":
if isinstance(node.value, ast.Constant) and isinstance(node.value.value, str):
return node.value.value
raise SystemExit(f"missing __version__ assignment in {path}")
def chart_top_level(path, key):
prefix = f"{key}:"
for line in path.read_text().splitlines():
if line.startswith((" ", "\t", "#")):
continue
stripped = line.strip()
if stripped.startswith(prefix):
return stripped.split(":", 1)[1].strip().strip("\"'")
raise SystemExit(f"missing {key} in {path}")
sdk_rust = load_toml(root / "sdk/rust/Cargo.toml")
stdng_dep = sdk_rust["dependencies"]["stdng"]
macros_dep = sdk_rust["dependencies"]["flame-rs-macros"]
shell_var("DETECTED_PYPROJECT_VERSION", pyproject_version(root / "sdk/python/pyproject.toml"))
shell_var("DETECTED_PYTHON_INIT_VERSION", python_init_version(root / "sdk/python/src/flamepy/__init__.py"))
shell_var("DETECTED_FLAME_RS_VERSION", package_version(root / "sdk/rust/Cargo.toml"))
shell_var("DETECTED_FLAME_RS_MACROS_VERSION", package_version(root / "sdk/rust/macros/Cargo.toml"))
shell_var("DETECTED_STDNG_VERSION", package_version(root / "stdng/Cargo.toml"))
shell_var("DETECTED_CHART_APP_VERSION", chart_top_level(root / "charts/flame/Chart.yaml", "appVersion"))
shell_var("DETECTED_FLAME_RS_STDNG_DEP_VERSION", stdng_dep.get("version"))
shell_var("DETECTED_FLAME_RS_MACROS_DEP_VERSION", macros_dep.get("version"))
PY
)"
}
Comment on lines
+336
to
+373
| inspect_image_manifest() { | ||
| local image="$1" | ||
| local manifest_file | ||
|
|
||
| manifest_file="${TMPDIR:-/tmp}/flame-release-sanity-manifest.$$.$(basename "$image").json" | ||
|
|
||
| if command -v docker >/dev/null 2>&1; then | ||
| log "docker buildx imagetools inspect --raw ${image}" | ||
| if docker buildx imagetools inspect --raw "$image" >"$manifest_file" 2>/dev/null && check_manifest_platforms "$image" "$manifest_file"; then | ||
| rm -f "$manifest_file" | ||
| return | ||
| fi | ||
| fi | ||
|
|
||
| if command -v podman >/dev/null 2>&1; then | ||
| log "podman manifest inspect ${image}" | ||
| if podman manifest inspect "$image" >"$manifest_file" 2>/dev/null && check_manifest_platforms "$image" "$manifest_file"; then | ||
| rm -f "$manifest_file" | ||
| return | ||
| fi | ||
|
|
||
| log "podman pull ${image}" | ||
| podman pull "$image" >/dev/null | ||
| log "podman image inspect ${image}" | ||
| podman image inspect "$image" >"$manifest_file" | ||
| check_manifest_platforms "$image" "$manifest_file" | ||
| elif command -v docker >/dev/null 2>&1; then | ||
| log "docker pull ${image}" | ||
| docker pull "$image" >/dev/null | ||
| log "docker image inspect ${image}" | ||
| docker image inspect "$image" >"$manifest_file" | ||
| check_manifest_platforms "$image" "$manifest_file" | ||
| else | ||
| fail "podman or docker is required for image manifest checks" | ||
| fi | ||
|
|
||
| rm -f "$manifest_file" | ||
| } |
Contributor
There was a problem hiding this comment.
There are two issues here:
- Creating a temporary file using
$$in/tmpis insecure and prone to symlink attacks or race conditions. We should usemktempinstead. - Because
check_manifest_platformsis chained with&&inside theifcondition, any validation failure (e.g., missing expected platforms) will be silently ignored by theifstatement, causing the script to fall back to other tools (like pulling the image locally) instead of failing immediately. Since a successful manifest inspect is definitive, any platform mismatch should fail the script immediately.
We can resolve both issues by using mktemp and separating the inspection command from the validation check.
inspect_image_manifest() {
local image="$1"
MANIFEST_FILE="$(mktemp "${TMPDIR:-/tmp}/flame-release-sanity-manifest.XXXXXX")"
if command -v docker >/dev/null 2>&1; then
log "docker buildx imagetools inspect --raw ${image}"
if docker buildx imagetools inspect --raw "$image" >"$MANIFEST_FILE" 2>/dev/null; then
check_manifest_platforms "$image" "$MANIFEST_FILE"
rm -f "$MANIFEST_FILE"
MANIFEST_FILE=""
return
fi
fi
if command -v podman >/dev/null 2>&1; then
log "podman manifest inspect ${image}"
if podman manifest inspect "$image" >"$MANIFEST_FILE" 2>/dev/null; then
check_manifest_platforms "$image" "$MANIFEST_FILE"
rm -f "$MANIFEST_FILE"
MANIFEST_FILE=""
return
fi
log "podman pull ${image}"
podman pull "$image" >/dev/null
log "podman image inspect ${image}"
podman image inspect "$image" >"$MANIFEST_FILE"
check_manifest_platforms "$image" "$MANIFEST_FILE"
elif command -v docker >/dev/null 2>&1; then
log "docker pull ${image}"
docker pull "$image" >/dev/null
log "docker image inspect ${image}"
docker image inspect "$image" >"$MANIFEST_FILE"
check_manifest_platforms "$image" "$MANIFEST_FILE"
else
fail "podman or docker is required for image manifest checks"
fi
rm -f "$MANIFEST_FILE"
MANIFEST_FILE=""
}
Comment on lines
+60
to
+73
| cleanup() { | ||
| if [ "$COMPOSE_STARTED" = "1" ] && enabled "$COMPOSE_DOWN"; then | ||
| log "Stopping Docker Compose project ${COMPOSE_PROJECT_NAME}" | ||
| compose_command down -v || true | ||
| fi | ||
|
|
||
| if [ -n "$COMPOSE_OVERRIDE_FILE" ]; then | ||
| rm -f "$COMPOSE_OVERRIDE_FILE" | ||
| fi | ||
|
|
||
| if [ -n "$PYPI_CHECK_SCRIPT" ]; then | ||
| rm -f "$PYPI_CHECK_SCRIPT" | ||
| fi | ||
| } |
Contributor
There was a problem hiding this comment.
To ensure that the temporary manifest file created during remote checks is always cleaned up (even if the script exits prematurely due to a validation failure), we should add it to the cleanup function.
Suggested change
| cleanup() { | |
| if [ "$COMPOSE_STARTED" = "1" ] && enabled "$COMPOSE_DOWN"; then | |
| log "Stopping Docker Compose project ${COMPOSE_PROJECT_NAME}" | |
| compose_command down -v || true | |
| fi | |
| if [ -n "$COMPOSE_OVERRIDE_FILE" ]; then | |
| rm -f "$COMPOSE_OVERRIDE_FILE" | |
| fi | |
| if [ -n "$PYPI_CHECK_SCRIPT" ]; then | |
| rm -f "$PYPI_CHECK_SCRIPT" | |
| fi | |
| } | |
| cleanup() { | |
| if [ "$COMPOSE_STARTED" = "1" ] && enabled "$COMPOSE_DOWN"; then | |
| log "Stopping Docker Compose project ${COMPOSE_PROJECT_NAME}" | |
| compose_command down -v || true | |
| fi | |
| if [ -n "$COMPOSE_OVERRIDE_FILE" ]; then | |
| rm -f "$COMPOSE_OVERRIDE_FILE" | |
| fi | |
| if [ -n "$PYPI_CHECK_SCRIPT" ]; then | |
| rm -f "$PYPI_CHECK_SCRIPT" | |
| fi | |
| if [ -n "${MANIFEST_FILE:-}" ]; then | |
| rm -f "$MANIFEST_FILE" | |
| fi | |
| } |
Comment on lines
+240
to
+243
| log "helm template flame charts/flame" | ||
| helm template flame "$ROOT_DIR/charts/flame" \ | ||
| --set "global.imageRegistry=${IMAGE_REGISTRY}" \ | ||
| --set "global.imageTag=${DOCKER_TAG}" >/tmp/flame-release-sanity-rendered.yaml |
Contributor
There was a problem hiding this comment.
Writing the rendered Helm template to a hardcoded path /tmp/flame-release-sanity-rendered.yaml can cause permission conflicts if multiple users run the script on the same machine. Since the goal is only to verify that helm template succeeds without errors, we can safely redirect the output to /dev/null.
Suggested change
| log "helm template flame charts/flame" | |
| helm template flame "$ROOT_DIR/charts/flame" \ | |
| --set "global.imageRegistry=${IMAGE_REGISTRY}" \ | |
| --set "global.imageTag=${DOCKER_TAG}" >/tmp/flame-release-sanity-rendered.yaml | |
| log "helm template flame charts/flame" | |
| helm template flame "$ROOT_DIR/charts/flame" \ | |
| --set "global.imageRegistry=${IMAGE_REGISTRY}" \ | |
| --set "global.imageTag=${DOCKER_TAG}" >/dev/null |
Comment on lines
+453
to
+455
| write_compose_override() { | ||
| COMPOSE_OVERRIDE_FILE="${TMPDIR:-/tmp}/flame-release-sanity-compose.$$.yaml" | ||
| cat >"$COMPOSE_OVERRIDE_FILE" <<EOF |
Contributor
There was a problem hiding this comment.
Use mktemp instead of process ID ($$) to securely create the temporary compose override file.
Suggested change
| write_compose_override() { | |
| COMPOSE_OVERRIDE_FILE="${TMPDIR:-/tmp}/flame-release-sanity-compose.$$.yaml" | |
| cat >"$COMPOSE_OVERRIDE_FILE" <<EOF | |
| write_compose_override() { | |
| COMPOSE_OVERRIDE_FILE="$(mktemp "${TMPDIR:-/tmp}/flame-release-sanity-compose.XXXXXX")" | |
| cat >"$COMPOSE_OVERRIDE_FILE" <<EOF |
Comment on lines
+471
to
+473
| write_pypi_check_script() { | ||
| PYPI_CHECK_SCRIPT="${TMPDIR:-/tmp}/flame-release-sanity-pypi-check.$$.sh" | ||
| cat >"$PYPI_CHECK_SCRIPT" <<'EOF' |
Contributor
There was a problem hiding this comment.
Use mktemp instead of process ID ($$) to securely create the temporary PyPI check script.
Suggested change
| write_pypi_check_script() { | |
| PYPI_CHECK_SCRIPT="${TMPDIR:-/tmp}/flame-release-sanity-pypi-check.$$.sh" | |
| cat >"$PYPI_CHECK_SCRIPT" <<'EOF' | |
| write_pypi_check_script() { | |
| PYPI_CHECK_SCRIPT="$(mktemp "${TMPDIR:-/tmp}/flame-release-sanity-pypi-check.XXXXXX")" | |
| cat >"$PYPI_CHECK_SCRIPT" <<'EOF' |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ci/release/sanity.shfor non-publishing release validation.make release-sanitytarget.flamepyfrom PyPI in a clean Python image.Validation
bash -n ci/release/sanity.shRELEASE_SANITY_LOCAL_CHECKS=0 RELEASE_SANITY_PACKAGE_CHECKS=0 RELEASE_SANITY_REMOTE_CHECKS=0 RELEASE_SANITY_K8S_E2E=0 RELEASE_SANITY_COMPOSE_E2E=0 make release-sanityCONTAINER_RUNTIME=podman RELEASE_TAG=v0.6.0-rc2 CARGO_VERSION=0.6.0-rc2 PYTHON_VERSION=0.6.0rc2 DOCKER_TAG=v0.6.0-rc2 IMAGE_REGISTRY=docker.io/xflops RELEASE_SANITY_METADATA_CHECKS=0 RELEASE_SANITY_LOCAL_CHECKS=0 RELEASE_SANITY_PACKAGE_CHECKS=0 RELEASE_SANITY_REMOTE_CHECKS=0 RELEASE_SANITY_K8S_E2E=0 RELEASE_SANITY_COMPOSE_E2E=1 RELEASE_SANITY_COMPOSE_E2E_TASKS=1 RELEASE_SANITY_COMPOSE_TIMEOUT_SECONDS=300 make release-sanityThe compose smoke pulled
docker.io/xflops/flame-*:{v0.6.0-rc2}, found theflmruntemplate, installedflamepy==0.6.0rc2from PyPI inpython:3.12-slim, imported it from/usr/local/lib/python3.12/site-packages, and completedpython -m flamepy.runner.e2e --tasks 1 --json.