-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
.pr_agent_accepted_suggestions
| PR 3106 (2026-04-03) |
[reliability] SIGTERM swallowed pre-async
SIGTERM swallowed pre-async
In event-driven mode, main() installs SIGTERM/SIGINT handlers that only set a flag and return, so a stop request received before asyncio.run() installs loop signal handlers will not terminate or initiate shutdown and can hang until supervisord SIGKILLs the process.In SE_VIDEO_EVENT_DRIVEN=true mode, video_recorder.py installs temporary SIGTERM/SIGINT handlers that only set a flag and return. If supervisord (or the user) sends SIGTERM during startup (before the asyncio loop handlers in video_service.py are installed), the process will ignore the stop request and keep running until supervisord hits stopwaitsecs and SIGKILLs.
The event-driven service later installs its own handlers using loop.add_signal_handler(...), but those are not active during the early startup window.
- Video/video_recorder.py[48-57]
Update _mark_external_shutdown to also preserve termination semantics during the pre-async window. For example:
- For SIGTERM: set the flag then
raise SystemExit(0)(or re-raise the signal after restoringSIG_DFL). - For SIGINT: set the flag then call
signal.default_int_handler(signum, frame). This keeps the flag behavior while ensuring SIGTERM/SIGINT still stop the process when received before asyncio installs its handlers.
[correctness] Event-driven shutdown misclassified
Event-driven shutdown misclassified
In event-driven mode, SIGTERM/SIGINT delivered after the asyncio loop starts are handled by video_service.py (loop.add_signal_handler), leaving _external_shutdown false and causing video_recorder.py to treat an external stop as self-initiated and signal supervisord redundantly.In event-driven mode, _external_shutdown is only set by the temporary signal.signal(...) handlers in video_recorder.py. Once video_service.py installs loop.add_signal_handler(...), SIGTERM/SIGINT no longer update _external_shutdown, so video_recorder.py may call _signal_supervisord() even when shutdown was initiated by supervisord.
This is redundant (and possibly confusing) signaling: supervisord already initiated shutdown.
- Video/video_recorder.py[48-57]
- Video/video_recorder.py[71-73]
Make event-driven signal handling set the external-shutdown flag in the active handlers:
- Option A: Modify
video_service.main()to accept a callback/flag-setter, and have theloop.add_signal_handlerlambda set bothexternal_shutdown=Trueandservice.shutdown_event.set(). - Option B: In
video_recorder.py, avoid the temporarysignal.signalapproach and instead haveservice_main()return an exit reason (e.g.,"external_signal"vs"drain") so the controller can decide whether to signal supervisord.
| PR 3098 (2026-03-24) |
[correctness] GHCR login user mismatch
GHCR login user mismatch
Workflows log into ghcr.io using `github.repository_owner` as the username while using `SELENIUM_CI_TOKEN` as the password, which can fail authentication if the token belongs to a different account and block all GHCR mirror steps.The workflows log into GHCR with -u ${{ github.repository_owner }} while using SELENIUM_CI_TOKEN as the password. If that token belongs to a bot/user (common for a PAT used to push commits), the GHCR username must match the token owner; otherwise docker login may fail and all subsequent mirroring steps will fail.
This affects deploy/nightly and browser-version release workflows that then invoke make release_ghcr* and make *_ghcr targets.
- .github/workflows/deploy.yml[148-166]
- .github/workflows/nightly.yml[100-117]
- .github/workflows/release-chrome-versions.yml[115-117]
- .github/workflows/release-chrome-for-testing-versions.yml[115-117]
- .github/workflows/release-edge-versions.yml[115-117]
- .github/workflows/release-firefox-versions.yml[115-117]
Introduce a dedicated secret/var for the GHCR username (the account that owns SELENIUM_CI_TOKEN) and use it in all docker login ghcr.io steps, e.g.:
| PR 3072 (2026-02-13) |
[general] Use shallow clone for FFmpeg
✅ 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.
-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 importance[1-10]: 5
__
Why: This is a valid optimization that improves build time and reduces image layer size by performing a shallow clone. While beneficial, it does not address the critical bug that the target branch for the clone does not exist, which would cause the build to fail regardless of this optimization.
| PR 3016 (2025-11-01) |
[possible issue] Fix malformed Docker image tags
Fix malformed Docker image tags
Fix the malformed Docker image tags in the newly added chrome-for-testing_*.md files. The script output shows a failure to retrieve Chrome and ChromeDriver versions, which needs to be corrected.
CHANGELOG/4.38.0/chrome-for-testing_113.md [1-21]
./tag_and_push_browser_images.sh 4.38.0 20251025 selenium false chrome-for-testing true Tagging images for browser chrome-for-testing, version 4.38.0, build date 20251025, namespace selenium Selenium Grid version -> 4.38.0-20251025 -Chrome for Testing version -> -Short Chrome for Testing version -> . -ChromeDriver version -> -Short ChromeDriver version -> . -Tagged selenium/node-chrome-for-testing:-chromedriver--grid-4.38.0-20251025 -Tagged selenium/standalone-chrome-for-testing:-chromedriver--grid-4.38.0-20251025 -Tagged selenium/node-chrome-for-testing:-chromedriver--20251025 -Tagged selenium/standalone-chrome-for-testing:-chromedriver--20251025 -Tagged selenium/node-chrome-for-testing:-20251025 -Tagged selenium/standalone-chrome-for-testing:-20251025 -Tagged selenium/node-chrome-for-testing:.-chromedriver-.-grid-4.38.0-20251025 -Tagged selenium/standalone-chrome-for-testing:.-chromedriver-.-grid-4.38.0-20251025 -Tagged selenium/node-chrome-for-testing:.-chromedriver-.-20251025 -Tagged selenium/standalone-chrome-for-testing:.-chromedriver-.-20251025 -Tagged selenium/node-chrome-for-testing:.-20251025 -Tagged selenium/standalone-chrome-for-testing:.-20251025 +Chrome for Testing version -> 113.0.5672.63 +Short Chrome for Testing version -> 113 +ChromeDriver version -> 113.0.5672.63 +Short ChromeDriver version -> 113 +Tagged selenium/node-chrome-for-testing:113.0.5672.63-chromedriver-113.0.5672.63-grid-4.38.0-20251025 +Tagged selenium/standalone-chrome-for-testing:113.0.5672.63-chromedriver-113.0.5672.63-grid-4.38.0-20251025 +Tagged selenium/node-chrome-for-testing:113.0.5672.63-chromedriver-113.0.5672.63-20251025 +Tagged selenium/standalone-chrome-for-testing:113.0.5672.63-chromedriver-113.0.5672.63-20251025 +Tagged selenium/node-chrome-for-testing:113.0.5672.63-20251025 +Tagged selenium/standalone-chrome-for-testing:113.0.5672.63-20251025 +Tagged selenium/node-chrome-for-testing:113-chromedriver-113-grid-4.38.0-20251025 +Tagged selenium/standalone-chrome-for-testing:113-chromedriver-113-grid-4.38.0-20251025 +Tagged selenium/node-chrome-for-testing:113-chromedriver-113-20251025 +Tagged selenium/standalone-chrome-for-testing:113-chromedriver-113-20251025 +Tagged selenium/node-chrome-for-testing:113-20251025 +Tagged selenium/standalone-chrome-for-testing:113-20251025
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical issue where all new changelog files contain malformed Docker tags due to a script failure, rendering the primary contribution of this PR incorrect.
| PR 2960 (2025-09-19) |
[security] Pin version and add checksum validation
✅ Pin version and add checksum validation
Pin the kubectl version and add checksum validation during download to improve security and ensure reproducible builds.
+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 importance[1-10]: 9
__
Why: The suggestion addresses a significant security risk by adding checksum validation for the downloaded binary and improves build reproducibility by pinning the kubectl version, which are critical improvements.
| PR 2912 (2025-07-30) |
[possible issue] Fix tolerations placement in pod spec
✅ 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 importance[1-10]: 9
__
Why: The suggestion correctly identifies that the tolerations block is incorrectly placed within the container definition, which would cause the Kubernetes job to fail, and provides the correct fix.
| PR 2873 (2025-07-02) |
[possible issue] Fix incorrect repository URL
✅ 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 importance[1-10]: 10
__
Why: The suggestion correctly identifies that the version for CRI-Dockerd is fetched from the wrong repository (kubernetes-sigs/cri-tools instead of Mirantis/cri-dockerd), which would cause the script to fail.
| PR 2868 (2025-06-17) |
[general] Fix timestamp format condition check
✅ 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 importance[1-10]: 8
__
Why: The suggestion correctly identifies a bug in the timestamp formatting logic. The code checks for ,%f in the original ts_format string, but it should check for it in the modified ts_format_python string. This error would prevent the millisecond trimming logic from executing when intended, leading to incorrect timestamp formatting.
[possible issue] Configure session timeout properly
✅ Configure session timeout properly
The max_time parameter is not being used in the session configuration. Configure the session with proper timeout settings to ensure requests respect the timeout value.
Video/validate_endpoint.py [33-36]
def create_session(max_time=1):
"""Create requests session with timeout configuration."""
session = requests.Session()
+ session.timeout = max_time
return sessionSuggestion importance[1-10]: 4
__
Why: The suggestion correctly points out that the max_time parameter in create_session is unused, which is misleading. While the timeout is correctly applied at the request level in session.get and session.post, setting it at the session level as suggested would improve code clarity and make the create_session function's signature and docstring accurate.
| PR 2851 (2025-06-02) |
[possible issue] Handle boolean values correctly
✅ 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 importance[1-10]: 7
__
Why: The suggestion correctly identifies that boolean False values from JSON aren't handled properly, which could cause incorrect behavior when record_video is a boolean instead of a string. This is a legitimate bug fix.
[possible issue] Improve regex pattern handling
✅ 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 importance[1-10]: 4
__
Why: While the suggestion adds support for more POSIX character classes, the current implementation may be sufficient for the specific use case. This is an enhancement rather than fixing a critical issue.
| PR 2769 (2025-04-08) |
[possible issue] Fix missing command continuation
✅ 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.
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.bashrcSuggestion importance[1-10]: 10
__
Why: The suggestion correctly identifies a critical syntax error in the Dockerfile. Without the '&&' operator before the echo command, the build would fail as the echo command would be executed as a separate command outside the RUN instruction, causing a syntax error. This is a high-impact fix that prevents build failure.
| PR 2692 (2025-03-04) |
[possible issue] Verify Firefox ARM64 compatibility
✅ 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.
-if [ "$(dpkg --print-architecture)" = "amd64" ] || [ $FIREFOX_VERSION = "latest" ]; then \
+if [ "$(dpkg --print-architecture)" = "amd64" ] || ([ $FIREFOX_VERSION = "latest" ] && firefox --version >/dev/null 2>&1); then \Suggestion importance[1-10]: 8
__
Why: The suggestion addresses a potential compatibility issue by adding a runtime check for Firefox on ARM64 platforms, preventing installation failures if Firefox is not supported for a specific version on ARM64 architecture.
| PR 2660 (2025-02-17) |
[possible issue] Fix variable concatenation syntax
✅ 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 importance[1-10]: 8
__
Why: The current syntax would make the underscore part of the prefix variable, leading to incorrect alias generation. This fix is critical for proper certificate alias creation and system functionality.
[general] Handle special characters in filenames
✅ 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")"Suggestion importance[1-10]: 7
__
Why: Properly quoting the basename command prevents potential script failures when certificate filenames contain spaces or special characters, improving script reliability.