.pr_agent_auto_best_practices

Pattern 1: When fetching or installing external dependencies in Dockerfiles/CI scripts, make the build reproducible and safer by pinning explicit versions, using the correct upstream source/repository, and validating downloads (e.g., checksums) or compatibility before proceeding.

Example code before:

# Dockerfile
RUN curl -L -o tool "https://example.com/tool/releases/latest/tool-linux-amd64" \
    && chmod +x tool && mv tool /usr/local/bin/tool

Example code after:

# Dockerfile
ARG TOOL_VERSION="v1.2.3"
RUN TOOL_URL="https://example.com/tool/releases/${TOOL_VERSION}/tool-linux-amd64" \
    && curl -L -o tool "${TOOL_URL}" \
    && curl -L -o tool.sha256 "${TOOL_URL}.sha256" \
    && echo "$(cat tool.sha256) tool" | sha256sum --check \
    && chmod +x tool && mv tool /usr/local/bin/tool \
    && rm -f tool.sha256

Relevant past accepted suggestions:

---

Suggestion 1:

Use shallow clone for FFmpeg

Optimize the git clone command by adding the --depth 1 flag to perform a shallow clone, which can speed up the build process.

.ffmpeg/Dockerfile [65]

-git clone -b release/${FFMPEG_VERSION} --single-branch https://github.com/FFmpeg/FFmpeg.git \
+git clone --depth 1 -b release/${FFMPEG_VERSION} --single-branch https://github.com/FFmpeg/FFmpeg.git \

---

Suggestion 2:

Pin version and add checksum validation

Pin the kubectl version and add checksum validation during download to improve security and ensure reproducible builds.

.tools/Dockerfile [7-11]

+ARG KUBECTL_VERSION=v1.30.3
 RUN apt-get update && apt-get install -y curl && \
-    curl -LO "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/$(dpkg --print-architecture)/kubectl" && \
+    KUBECTL_URL="https://dl.k8s.io/release/${KUBECTL_VERSION}/bin/linux/$(dpkg --print-architecture)/kubectl" && \
+    curl -LO "${KUBECTL_URL}" && \
+    curl -LO "${KUBECTL_URL}.sha256" && \
+    echo "$(cat kubectl.sha256) kubectl" | sha256sum --check && \
     chmod +x kubectl && \
     mv kubectl /usr/local/bin/ && \
+    rm kubectl.sha256 && \
     apt-get clean && rm -rf /var/lib/apt/lists/*

---

Suggestion 3:

Fix incorrect repository URL

The CRI-Dockerd version is being fetched from the wrong repository. It should fetch from the Mirantis/cri-dockerd repository instead of kubernetes-sigs/cri-tools.

tests/charts/make/chart_setup_env.sh [113]

-CRI_DOCKERD_VERSION="$(curl -s -L -o /dev/null -w '%{url_effective}\n' https://github.com/kubernetes-sigs/cri-tools/releases/latest | sed -E 's#.*/tag/(v[0-9.]+).*#\1#')"
+CRI_DOCKERD_VERSION="$(curl -s -L -o /dev/null -w '%{url_effective}\n' https://github.com/Mirantis/cri-dockerd/releases/latest | sed -E 's#.*/tag/(v[0-9.]+).*#\1#')"

---

Suggestion 4:

Verify Firefox ARM64 compatibility

The condition allows installing Firefox latest version on ARM64 without verifying if Firefox actually supports ARM64 for that version. Add explicit version check for ARM64 compatibility.

NodeFirefox/Dockerfile [24]

-if [ "$(dpkg --print-architecture)" = "amd64" ] || [ $FIREFOX_VERSION = "latest" ]; then \
+if [ "$(dpkg --print-architecture)" = "amd64" ] || ([ $FIREFOX_VERSION = "latest" ] && firefox --version >/dev/null 2>&1); then \

---

Pattern 2: In scripts/config templates, harden correctness by placing configuration at the proper hierarchy level and by handling common edge cases explicitly (quoting variables/paths, correct conditional checks, strict command chaining, and robust parsing of booleans/regex/format strings).

Example code before:

# bash
NAME=$PREFIX_$(basename $file)
do_something $NAME
apt-get update \
  && apt-get install -y pkg
  echo "done"

Example code after:

# bash
NAME="${PREFIX}_$(basename "$file")"
do_something "$NAME"
apt-get update \
  && apt-get install -y pkg \
  && echo "done"

Relevant past accepted suggestions:

---

Suggestion 1:

Fix tolerations placement in pod spec

The tolerations field should be at the pod spec level, not under the container. Move it to be a sibling of containers and volumes for proper Kubernetes pod scheduling.

charts/selenium-grid/templates/patch-keda/delete-keda-objects-job.yaml [39-41]

-{{- with $.Values.autoscaling.patchObjectFinalizers.tolerations  }}
-  tolerations : {{ toYaml . | nindent 12 }}
+{{- with $.Values.autoscaling.patchObjectFinalizers.tolerations }}
+  tolerations: {{ toYaml . | nindent 8 }}
 {{- end }}

---

Suggestion 2:

Fix timestamp format condition check

The condition checks for ,%f in ts_format but should check for %f in ts_format_python since that's the format being used. This could cause incorrect timestamp formatting.

Video/validate_endpoint.py [23-27]

-if ',%f' in ts_format:
+if '%f' in ts_format_python:
     # Find the microseconds part and trim to milliseconds
     parts = timestamp.rsplit(',', 1)
     if len(parts) == 2 and len(parts[1]) == 6:
         timestamp = parts[0] + ',' + parts[1][:3]

---

Suggestion 3:

Handle boolean values correctly

The current logic doesn't properly handle boolean values in JSON. If record_video is a boolean False (not string "false"), it will be incorrectly set to "true". Add a check for boolean type to correctly process both string and boolean values.

Video/video_nodeQuery.py [48-52]

 # Check if enabling to record video
-if isinstance(record_video, str) and record_video.lower() == "false":
+if (isinstance(record_video, str) and record_video.lower() == "false") or record_video is False:
     record_video = "false"
 else:
     record_video = "true"

---

Suggestion 4:

Improve regex pattern handling

The regex pattern handling is incomplete for POSIX character classes. The current implementation only handles [:alnum:] but ignores other potential classes like [:digit:] or [:alpha:]. Implement a more comprehensive POSIX class conversion.

Video/video_nodeQuery.py [93-101]

 # Convert trim pattern to regex
 # Handle character classes like [:alnum:]
-if "[:alnum:]" in trim_pattern:
-    # Create regex pattern for alphanumeric characters plus other allowed chars
-    allowed_chars = trim_pattern.replace("[:alnum:]", "a-zA-Z0-9")
-    pattern = f"[^{allowed_chars}]"
-else:
-    # Direct character set
-    pattern = f"[^{re.escape(trim_pattern)}]"
+posix_classes = {
+    "[:alnum:]": "a-zA-Z0-9",
+    "[:alpha:]": "a-zA-Z",
+    "[:digit:]": "0-9",
+    "[:space:]": " \t\n\r\f\v"
+}
 
+allowed_chars = trim_pattern
+for posix_class, replacement in posix_classes.items():
+    if posix_class in allowed_chars:
+        allowed_chars = allowed_chars.replace(posix_class, replacement)
+
+pattern = f"[^{re.escape(allowed_chars)}]"
+

---

Suggestion 5:

Fix missing command continuation

The command is missing a backslash continuation character after the package installation line, which will cause the echo command to fail. Add a backslash after the apt cleanup command.

Base/Dockerfile [76-81]

 RUN apt-get -qqy update \
     && apt-get upgrade -yq \
     && apt-get -qqy --no-install-recommends install \
     python3 python3-pip python3-venv \
     && rm -rf /var/lib/apt/lists/* /var/cache/apt/* \
-    echo "source $VENV_PATH/bin/activate" >> /etc/bash.bashrc
+    && echo "source $VENV_PATH/bin/activate" >> /etc/bash.bashrc

---

Suggestion 6:

Fix variable concatenation syntax

The ALIAS variable concatenation is incorrect. The underscore is part of the prefix instead of being a separator. Add a space before the underscore to properly separate prefix from filename.

charts/selenium-grid/certs/add-cert-helper.sh [78]

-ALIAS="$ALIAS_PREFIX_$(basename $cert_file)"
+ALIAS="${ALIAS_PREFIX}_$(basename $cert_file)"

---

Suggestion 7:

Handle special characters in filenames

The basename could contain spaces or special characters. Quote the basename command to prevent word splitting and globbing issues.

charts/selenium-grid/certs/add-cert-helper.sh [78]

-ALIAS="$ALIAS_PREFIX_$(basename $cert_file)"
+ALIAS="${ALIAS_PREFIX}_$(basename "$cert_file")"

---

[Auto-generated best practices - 2026-03-24]