Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.6k
.pr_agent_auto_best_practices
Pattern 1: In CI/workflow and automation scripts, enable strict execution (e.g.,set -euo pipefail) and validate/sanitize all inputs/outputs (required env vars, CLI args, step outputs) before use, failing fast with a clear, actionable error message when invalid or missing.
Example code before:
#!/usr/bin/env bashTAG="$1"avail=$(df -k / | awk 'NR==2 {print $4}')echo "version=$(echo "$TAG" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+')" >> "$GITHUB_OUTPUT"Example code after:
#!/usr/bin/env bashset -euo pipefailTAG="$(echo "${1:-}" | xargs)"if [[ -z "$TAG" ]]; then echo "ERROR: tag is required" >&2 exit 1fiVERSION="$(echo "$TAG" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -n1 || true)"if [[ -z "$VERSION" ]]; then echo "ERROR: invalid tag (no version found): $TAG" >&2 exit 1fiecho "version=$VERSION" >> "$GITHUB_OUTPUT"Relevant past accepted suggestions:
Suggestion 1:
Gate Slack notify on inputs
Only run Slack reporting when the webhook secret exists and the disk step produced a valid output, otherwise this can fail the workflow or evaluate invalid comparisons.
.github/workflows/bazel.yml [277-287]
- name: Report disk space- if: always()+ if: always() && secrets.SLACK_WEBHOOK_URL != '' && steps.disk.outputs.avail != '' uses: rtCamp/action-slack-notify@v2 env: SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK_URL }} SLACK_CHANNEL: ci-disk-alerts SLACK_USERNAME: Disk Monitor SLACK_TITLE: "${{ steps.disk.outputs.avail }}GB remaining" SLACK_MESSAGE: "${{ inputs.name }} on ${{ inputs.os }}" SLACK_COLOR: ${{ steps.disk.outputs.avail < 10 && 'danger' || (steps.disk.outputs.avail < 30 && 'warning' || 'good') }} SLACK_ICON_EMOJI: ":floppy_disk:"
Suggestion 2:
Harden author check validation
Make the step more robust by enabling strict shell mode, trimming/validating the retrieved author, and preferring a stable identifier (e.g., author email) rather than a display name.
.github/workflows/ci-lint.yml [79-88]
- name: Check last commit author id: check+ shell: bash run: |- LAST_AUTHOR=$(git log -1 --format='%an')- if [ "$LAST_AUTHOR" = "Selenium CI Bot" ]; then+ set -euo pipefail++ LAST_AUTHOR_EMAIL="$(git log -1 --format='%ae' | tr -d '\r' | xargs || true)"+ if [ -z "$LAST_AUTHOR_EMAIL" ]; then+ echo "::warning::Could not determine last commit author email; skipping auto-commit to avoid loops"+ echo "should-commit=false" >> "$GITHUB_OUTPUT"+ elif [ "$LAST_AUTHOR_EMAIL" = "selenium-ci-bot@example.com" ]; then echo "::notice::Last commit was from Selenium CI Bot - skipping commit-fixes" echo "should-commit=false" >> "$GITHUB_OUTPUT" else echo "should-commit=true" >> "$GITHUB_OUTPUT" fi
Suggestion 3:
Fail fast on missing inputs
Validate that $DOTNET_DIR exists and that at least one .csproj was found before running, so mis-invocations fail fast with a clear message.
dotnet/private/dotnet_format.bzl [49-59]
# Find the workspace root WORKSPACE_ROOT="${{BUILD_WORKSPACE_DIRECTORY:-$RUNFILES_DIR/_main}}" DOTNET_DIR="$WORKSPACE_ROOT/dotnet"+if [[ ! -d "$DOTNET_DIR" ]]; then+ echo "ERROR: Could not find dotnet directory at $DOTNET_DIR" >&2+ exit 1+fi+ cd "$DOTNET_DIR" echo "Running dotnet format on all projects..."-find "$DOTNET_DIR/src" "$DOTNET_DIR/test" -name "*.csproj" 2>/dev/null | while read -r proj; do+mapfile -t projects < <(find "$DOTNET_DIR/src" "$DOTNET_DIR/test" -name "*.csproj" 2>/dev/null)+if [[ "${#projects[@]}" -eq 0 ]]; then+ echo "ERROR: No .csproj files found under $DOTNET_DIR/src or $DOTNET_DIR/test" >&2+ exit 1+fi+for proj in "${projects[@]}"; do echo " Formatting $proj..." "$DOTNET" format "$proj" || exit 1-done || exit 1+doneSuggestion 4:
Validate CLI/env inputs before use
Trim and validate --version/--output and require/validate BUILD_WORKSPACE_DIRECTORY (or explicitly define a fallback) so the script doesn't write to an unexpected relative path or accept invalid versions.
scripts/update_docfx.py [126-135]
-version = choose_version(versions, args.allow_prerelease, args.version)+explicit_version = args.version.strip() if args.version else None+if explicit_version:+ try:+ Version(explicit_version)+ except InvalidVersion as e:+ raise ValueError(f"Invalid --version: {explicit_version}") from e++version = choose_version(versions, args.allow_prerelease, explicit_version) nupkg_url = NUGET_NUPKG_URL.format(version=version) sha256 = sha256_of_url(nupkg_url)-output_path = Path(args.output)+output_arg = (args.output or "").strip()+if not output_arg:+ raise ValueError("--output must be a non-empty path")+output_path = Path(output_arg) if not output_path.is_absolute():- workspace_dir = os.environ.get("BUILD_WORKSPACE_DIRECTORY")- if workspace_dir:- output_path = Path(workspace_dir) / output_path+ workspace_dir = (os.environ.get("BUILD_WORKSPACE_DIRECTORY") or "").strip()+ if not workspace_dir:+ raise EnvironmentError("BUILD_WORKSPACE_DIRECTORY is required when --output is a relative path")+ output_path = Path(workspace_dir) / output_path output_path.write_text(render_docfx_repo(version, sha256))
Suggestion 5:
Validate inputs and fail fast
Validate that COMMIT_MESSAGE is non-empty (and optionally trim it) and enable strict shell options to prevent unexpected behavior from empty inputs or failing commands.
.github/workflows/commit-changes.yml [51-85]
- name: Apply and commit id: commit run: |+ set -euo pipefail+ if [ -f changes.patch ] && [ -s changes.patch ]; then+ if [ -z "${COMMIT_MESSAGE//[[:space:]]/}" ]; then+ echo "::error::Commit message is required"+ echo "committed=false" >> "$GITHUB_OUTPUT"+ exit 1+ fi+ if ! git apply --index changes.patch; then echo "::error::Failed to apply patch" echo "committed=false" >> "$GITHUB_OUTPUT" exit 1 fi git config --local user.email "selenium-ci@users.noreply.github.com" git config --local user.name "Selenium CI Bot" if ! git commit -m "$COMMIT_MESSAGE"; then echo "::error::Failed to commit changes" echo "committed=false" >> "$GITHUB_OUTPUT" exit 1 fi- if [ -n "$PUSH_BRANCH" ]; then+ if [ -n "${PUSH_BRANCH:-}" ]; then if ! git push origin HEAD:"$PUSH_BRANCH" --force; then echo "::error::Failed to push to $PUSH_BRANCH" echo "committed=false" >> "$GITHUB_OUTPUT" exit 1 fi else if ! git push; then echo "::error::Failed to push" echo "committed=false" >> "$GITHUB_OUTPUT" exit 1 fi fi echo "::notice::Changes committed and pushed" echo "committed=true" >> "$GITHUB_OUTPUT" else echo "::notice::No changes to commit" echo "committed=false" >> "$GITHUB_OUTPUT" fi
Suggestion 6:
Validate workflow inputs before use
Validate/sanitize COMMIT_MESSAGE and PUSH_BRANCH (non-empty, trimmed, and restricted charset) before using them, to avoid unexpected behavior or refspec injection.
.github/workflows/commit-changes.yml [51-72]
- name: Apply and commit id: commit run: |+ set -euo pipefail++ COMMIT_MESSAGE="${COMMIT_MESSAGE#"${COMMIT_MESSAGE%%[![:space:]]*}"}"+ COMMIT_MESSAGE="${COMMIT_MESSAGE%"${COMMIT_MESSAGE##*[![:space:]]}"}"+ if [ -z "$COMMIT_MESSAGE" ]; then+ echo "::error::commit-message must be non-empty"+ exit 1+ fi++ if [ -n "${PUSH_BRANCH:-}" ] && ! [[ "$PUSH_BRANCH" =~ ^[A-Za-z0-9._/-]+$ ]]; then+ echo "::error::push-branch contains invalid characters"+ exit 1+ fi+ if [ -f changes.patch ] && [ -s changes.patch ]; then git apply --index changes.patch git config --local user.email "selenium-ci@users.noreply.github.com" git config --local user.name "Selenium CI Bot" git commit -m "$COMMIT_MESSAGE"- if [ -n "$PUSH_BRANCH" ]; then+ if [ -n "${PUSH_BRANCH:-}" ]; then git push origin HEAD:"$PUSH_BRANCH" --force else git push fi echo "::notice::Changes committed and pushed" echo "committed=true" >> "$GITHUB_OUTPUT" else echo "::notice::No changes to commit" echo "committed=false" >> "$GITHUB_OUTPUT" fi env: COMMIT_MESSAGE: ${{ inputs.commit-message }} PUSH_BRANCH: ${{ inputs.push-branch }}
Suggestion 7:
Validate version format
Add input validation to the Normalize version step to ensure the version string is in X.Y or X.Y.Z format, and fail the job if the format is invalid.
.github/workflows/pre-release.yml [137-144]
- name: Normalize version id: version run: | VERSION="${{ github.event.inputs.version }}"+ if [[ ! "$VERSION" =~ ^[0-9]+\.[0-9]+(\.[0-9]+)?$ ]]; then+ echo "Error: version '${VERSION}' is not in X.Y or X.Y.Z format." >&2+ exit 1+ fi if [[ "$VERSION" =~ ^[0-9]+\.[0-9]+$ ]]; then VERSION="${VERSION}.0" fi echo "value=$VERSION" >> "$GITHUB_OUTPUT"Suggestion 8:
Validate workflow inputs before use
Validate/trim inputs.tag and ensure a version was extracted (and language is one of the allowed values) before writing outputs, failing fast if invalid to avoid checking out an unexpected ref.
.github/workflows/update-documentation.yml [46-67]
- name: Parse tag id: parse env: TAG: ${{ inputs.tag }} INPUT_LANG: ${{ inputs.language }} run: |- echo "version=$(echo "$TAG" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+')" >> "$GITHUB_OUTPUT"+ set -euo pipefail- # Check for language suffix and map to internal names+ TAG="$(echo "${TAG:-}" | xargs)"+ if [ -z "$TAG" ]; then+ echo "inputs.tag is required" >&2+ exit 1+ fi++ VERSION="$(echo "$TAG" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -n1 || true)"+ if [ -z "$VERSION" ]; then+ echo "Invalid tag (no version found): $TAG" >&2+ exit 1+ fi+ echo "version=$VERSION" >> "$GITHUB_OUTPUT"+ case "$TAG" in *-javascript) LANG="node" ;; *-python) LANG="py" ;; *-ruby) LANG="rb" ;; *-java) LANG="java" ;; *-dotnet) LANG="dotnet" ;; *) LANG="all" ;; esac- if [ "$INPUT_LANG" != "all" ]; then- LANG="$INPUT_LANG"+ if [ -n "${INPUT_LANG:-}" ] && [ "$INPUT_LANG" != "all" ]; then+ case "$INPUT_LANG" in+ java|rb|py|dotnet|node|all) LANG="$INPUT_LANG" ;;+ *) echo "Invalid language: $INPUT_LANG" >&2; exit 1 ;;+ esac fi+ echo "language=$LANG" >> "$GITHUB_OUTPUT"Pattern 2: For scripts/workflows that derive filesystem paths (repo root, Bazel execroot/runfiles, artifact destinations), resolve paths using robust primitives (runfiles/bazel info/explicit workspace variables) and validate existence beforecd, reading, or writing; avoid accidental nested directories when downloading artifacts.
Example code before:
WORKSPACE_ROOT="$(cd "$(dirname "$0")/../.." && pwd)"cd "$WORKSPACE_ROOT/bazel-bin/../../.."actions/download-artifact: with: path: build/distExample code after:
WORKSPACE_ROOT="${BUILD_WORKSPACE_DIRECTORY:-$RUNFILES_DIR/_main}"if [[ -z "${WORKSPACE_ROOT}" || ! -d "${WORKSPACE_ROOT}" ]]; then echo "ERROR: workspace root not found" >&2 exit 1fiEXEC_ROOT="$(cd "$WORKSPACE_ROOT" && bazel info execution_root 2>/dev/null || true)"if [[ -z "$EXEC_ROOT" || ! -d "$EXEC_ROOT" ]]; then echo "ERROR: failed to resolve execution_root" >&2 exit 1fi# In workflows: download to '.' to avoid path nesting# (artifact already contains build/dist/*)actions/download-artifact: with: path: .Relevant past accepted suggestions:
Suggestion 1:
Check correct filesystem space
Modify the disk space check to use df on $GITHUB_WORKSPACE instead of / to ensure the measurement is for the correct filesystem where the job workspace resides.
.github/workflows/bazel.yml [269-276]
- name: Check disk space if: always() shell: bash id: disk run: |- avail=$(df -k / | awk 'NR==2 {printf "%.0f", $4/1024/1024}')++ avail=$(df -k "$target" | awk 'NR==2 {printf "%.0f", $4/1024/1024}') echo "Remaining disk space: ${avail}GB" echo "avail=$avail" >> "$GITHUB_OUTPUT"
Suggestion 2:
Fix workspace root resolution
Replace the brittle workspace root discovery logic with a more robust method using the Bazel runfiles tree to prevent unexpected failures.
dotnet/private/dotnet_format.bzl [49-51]
# Find the workspace root-WORKSPACE_ROOT="${{BUILD_WORKSPACE_DIRECTORY:-$(cd "$(dirname "$0")/../.." && pwd)}}"+WORKSPACE_ROOT="${BUILD_WORKSPACE_DIRECTORY:-$RUNFILES_DIR/{workspace}}" DOTNET_DIR="$WORKSPACE_ROOT/dotnet"+if [[ ! -d "$DOTNET_DIR" ]]; then+ echo "ERROR: Could not find dotnet/ directory at $DOTNET_DIR" >&2+ exit 1+fi+
Suggestion 3:
Resolve repo root via runfiles
Replace the brittle workspace root discovery logic with a more robust method using the Bazel runfiles tree to prevent unexpected failures.
dotnet/private/paket_deps.bzl [50-52]
# Find the workspace root (where dotnet/.config/dotnet-tools.json lives)-WORKSPACE_ROOT="${{BUILD_WORKSPACE_DIRECTORY:-$(cd "$(dirname "$0")/../.." && pwd)}}"+WORKSPACE_ROOT="${BUILD_WORKSPACE_DIRECTORY:-$RUNFILES_DIR/{workspace}}" DOTNET_DIR="$WORKSPACE_ROOT/dotnet"+if [[ ! -d "$DOTNET_DIR" ]]; then+ echo "ERROR: Could not find dotnet/ directory at $DOTNET_DIR" >&2+ exit 1+fi+
Suggestion 4:
Fix execution root path resolution
Fix the logic for calculating the EXEC_ROOT path in the Unix shell script template. The current implementation is incorrect and will fail to find the required executables.
dotnet/private/docfx.bzl [38-41]
-# Resolve execution root from bazel-bin symlink (bin -> config -> bazel-out -> exec_root)-EXEC_ROOT=$(cd "$BUILD_WORKSPACE_DIRECTORY/bazel-bin/../../.." && pwd -P)+# Resolve Bazel execution root from the real bazel-bin path.+BIN_REAL="$(cd "$BUILD_WORKSPACE_DIRECTORY/bazel-bin" && pwd -P)"+OUTPUT_BASE="${BIN_REAL%%/bazel-out/*}"+WS_NAME="$(basename "$BUILD_WORKSPACE_DIRECTORY")"+EXEC_ROOT="${OUTPUT_BASE}/execroot/${WS_NAME}" exec "$EXEC_ROOT/{dotnet}" exec \ "$EXEC_ROOT/{docfx}" {config} "$@"
Suggestion 5:
Add robust path existence checks
Add explicit existence checks and a clear error message if bazel-bin isn’t present or the derived EXEC_ROOT is empty/invalid, to avoid confusing failures when run outside Bazel or before a build.
dotnet/private/docfx.bzl [35-53]
_UNIX_TEMPLATE = """#!/usr/bin/env bash set -euo pipefail cd "$BUILD_WORKSPACE_DIRECTORY"+if [ ! -e "$BUILD_WORKSPACE_DIRECTORY/bazel-bin" ]; then+ echo "bazel-bin not found; run a Bazel build first." >&2+ exit 1+fi # Resolve execution root from bazel-bin symlink (bin -> config -> bazel-out -> exec_root) EXEC_ROOT=$(cd "$BUILD_WORKSPACE_DIRECTORY/bazel-bin/../../.." && pwd -P)+if [ -z "${EXEC_ROOT}" ] || [ ! -d "${EXEC_ROOT}" ]; then+ echo "Failed to resolve Bazel execution root." >&2+ exit 1+fi exec "$EXEC_ROOT/{dotnet}" exec \ "$EXEC_ROOT/{docfx}" {config} "$@" """ _WINDOWS_TEMPLATE = """@echo off setlocal cd /d "%BUILD_WORKSPACE_DIRECTORY%"+if not exist "%BUILD_WORKSPACE_DIRECTORY%\\bazel-bin" (+ echo bazel-bin not found; run a Bazel build first. 1>&2+ exit /b 1+) rem Resolve execution root from bazel-bin junction (bin -> config -> bazel-out -> exec_root) cd /d "%BUILD_WORKSPACE_DIRECTORY%\\bazel-bin\\..\\..\\.." set EXEC_ROOT=%CD%+if not exist "%EXEC_ROOT%" (+ echo Failed to resolve Bazel execution root. 1>&2+ exit /b 1+) cd /d "%BUILD_WORKSPACE_DIRECTORY%" "%EXEC_ROOT%\\{dotnet}" exec ^ "%EXEC_ROOT%\\{docfx}" {config} %* """
Suggestion 6:
Fix artifact path nesting
In .github/workflows/update-documentation.yml, change the download path for the documentation artifact from docs/api to . to prevent creating a nested and incorrect directory structure.
.github/workflows/update-documentation.yml [94-98]
- name: Download documentation uses: actions/download-artifact@v4 with: name: documentation- path: docs/api+ path: .
Suggestion 7:
Prevent nested dist directories
In .github/workflows/nightly.yml, change the download path for the nightly-grid artifact from build/dist to . to avoid creating a nested directory structure and ensure the release step finds the files.
.github/workflows/nightly.yml [191-195]
- name: Download grid packages uses: actions/download-artifact@v4 with: name: nightly-grid- path: build/dist+ path: .
Suggestion 8:
Avoid doubled output directories
In .github/workflows/release.yml, change the download path for the release-packages artifact from build/dist to . to prevent a nested directory issue that would cause the release creation to fail.
.github/workflows/release.yml [72-76]
- name: Download release packages uses: actions/download-artifact@v4 with: name: release-packages- path: build/dist+ path: .
Pattern 3: Make CI workflows and release/commit automation rerunnable and noise-free by gating steps on meaningful conditions (e.g., only notify on warning thresholds, only apply/commit when a patch is non-empty) and making operations idempotent (safe re-runs when releases already exist).
Example code before:
- name: Notify if: always() run: send_alert- name: Apply patch run: git apply --index changes.patch || echo "No changes to apply"- name: Create release with: allowUpdates: falseExample code after:
- name: Notify if: always() && steps.disk.outputs.avail != '' && steps.disk.outputs.avail < 30 run: send_alert- name: Apply patch shell: bash run: | set -euo pipefail if [[ ! -s changes.patch ]]; then echo "No changes to apply" exit 0 fi git apply --index changes.patch- name: Create release with: allowUpdates: true omitBodyDuringUpdate: true omitNameDuringUpdate: trueRelevant past accepted suggestions:
Suggestion 1:
Only send alerts on low disk
The current implementation sends a Slack notification on every CI run. To reduce noise, it should be modified to only send alerts when disk space is low (e.g., below the 'warning' threshold).
.github/workflows/bazel.yml [277-287]
-name:Report disk spaceif:always()uses:rtCamp/action-slack-notify@v2env:SLACK_WEBHOOK:${{ secrets.SLACK_WEBHOOK_URL }}SLACK_CHANNEL:ci-disk-alertsSLACK_USERNAME:Disk MonitorSLACK_TITLE:"${{ steps.disk.outputs.avail }}GB remaining"SLACK_MESSAGE:"${{ inputs.name }} on ${{ inputs.os }}"SLACK_COLOR:${{ steps.disk.outputs.avail < 10 && 'danger' || (steps.disk.outputs.avail < 30 && 'warning' || 'good') }}... (clipped 1 lines)
-name:Report disk spaceif:always()uses:rtCamp/action-slack-notify@v2env:SLACK_WEBHOOK:${{ secrets.SLACK_WEBHOOK_URL }}SLACK_CHANNEL:ci-disk-alertsSLACK_TITLE:"${{ steps.disk.outputs.avail }}GB remaining"SLACK_MESSAGE:"${{ inputs.name }} on ${{ inputs.os }}"SLACK_COLOR:${{ steps.disk.outputs.avail < 10 && 'danger' || (steps.disk.outputs.avail < 30 && 'warning' || 'good') }}...
-name:Report disk spaceif:always() && steps.disk.outputs.avail < 30uses:rtCamp/action-slack-notify@v2env:SLACK_WEBHOOK:${{ secrets.SLACK_WEBHOOK_URL }}SLACK_CHANNEL:ci-disk-alertsSLACK_TITLE:"${{ steps.disk.outputs.avail }}GB remaining"SLACK_MESSAGE:"${{ inputs.name }} on ${{ inputs.os }}"SLACK_COLOR:${{ steps.disk.outputs.avail < 10 && 'danger' || 'warning' }}...
Suggestion 2:
Make release creation idempotent
Set allowUpdates: true to prevent workflow failures on re-runs if a release already exists, making the job idempotent.
.github/workflows/release.yml [103]
-allowUpdates: ${{ github.run_attempt > 1 }}+allowUpdates: true
Suggestion 3:
Make release creation rerunnable
Make the release step safe to re-run by allowing updates or skipping when the release already exists, to avoid failures when the tag/release is present from a partial run.
.github/workflows/release.yml [101-109]
- name: Create GitHub release uses: ncipollo/release-action@v1 with: artifacts: "build/dist/*.*" bodyFile: "scripts/github-actions/release_header.md" generateReleaseNotes: true name: "Selenium ${{ needs.prepare.outputs.version }}" tag: "${{ needs.prepare.outputs.tag }}" commit: "${{ github.sha }}"+ allowUpdates: true+ omitBodyDuringUpdate: true+ omitNameDuringUpdate: trueSuggestion 4:
Upload artifacts even on failure
Add always() to the conditions for saving and uploading artifacts in bazel.yml to ensure they run even if the main run step fails.
.github/workflows/bazel.yml [254-264]
- name: Save git diff- if: inputs.artifact-name != '' && inputs.artifact-path == ''+ if: always() && inputs.artifact-name != '' && inputs.artifact-path == '' run: git diff > changes.patch - name: Upload artifact- if: inputs.artifact-name != ''+ if: always() && inputs.artifact-name != '' uses: actions/upload-artifact@v4 with: name: ${{ inputs.artifact-name }} path: ${{ inputs.artifact-path || 'changes.patch' }} retention-days: 6 if-no-files-found: ${{ inputs.artifact-path != '' && 'error' || 'warn' }}
Suggestion 5:
Prevent job failure if no changes
In the push-rust-version job, add a condition to check if changes.patch is non-empty before attempting to apply it and commit, preventing job failure when there are no changes.
.github/workflows/pre-release.yml [43-66]
push-rust-version: name: Push Rust Version needs: generate-rust-version runs-on: ubuntu-latest steps: - name: Checkout repo uses: actions/checkout@v4 with: token: ${{ secrets.SELENIUM_CI_TOKEN }} fetch-depth: 0 - name: Download rust version patch uses: actions/download-artifact@v4 with: name: rust-version - name: Apply patch- run: git apply --index changes.patch+ run: |+ if [ -s changes.patch ]; then+ git apply --index changes.patch+ echo "CHANGES_APPLIED=true" >> "$GITHUB_ENV"+ fi - name: Prep git+ if: env.CHANGES_APPLIED == 'true' run: | git config --local user.email "selenium-ci@users.noreply.github.com" git config --local user.name "Selenium CI Bot" - name: Push changes+ if: env.CHANGES_APPLIED == 'true' run: | git commit -m "update selenium manager version and rust changelog" git push origin HEAD:rust-release-${{ inputs.version }} --forceSuggestion 6:
Avoid swallowing patch-apply errors
Replace the unconditional || echo ... with an explicit file existence/size check so real git apply failures fail the job instead of being silently ignored.
.github/workflows/release.yml [189-190]
- name: Apply patch- run: git apply --index changes.patch || echo "No changes to apply"+ shell: bash+ run: |+ if [ ! -f changes.patch ] || [ ! -s changes.patch ]; then+ echo "No changes to apply"+ exit 0+ fi+ git apply --index changes.patch
Pattern 4: Harden automation security by minimizing secret exposure and tightening permissions: set least-privilege job permissions, avoid persisting GitHub credentials unless needed, restrict file permissions for artifacts/credential files, and avoid shell interpolation when executing commands derived from inputs.
Example code before:
- uses: actions/checkout@v4 with: persist-credentials: truesystem("which #{cmd} > /dev/null 2>&1")FileUtils.chmod(0o666, "dist/output.zip")Example code after:
- uses: actions/checkout@v4 with: persist-credentials: falsesystem("which", cmd, out: File::NULL, err: File::NULL)FileUtils.chmod(0o644, "dist/output.zip")File.chmod(0o600, ".npmrc")Relevant past accepted suggestions:
Suggestion 1:
Tighten artifact file permissions
Change the file permissions for the generated .zip artifacts from world-writable 0o666 to a more secure 0o644 to prevent tampering.
rake_tasks/dotnet.rake [21-24]
FileUtils.copy('bazel-bin/dotnet/release.zip', "build/dist/selenium-dotnet-#{dotnet_version}.zip")-FileUtils.chmod(0o666, "build/dist/selenium-dotnet-#{dotnet_version}.zip")+FileUtils.chmod(0o644, "build/dist/selenium-dotnet-#{dotnet_version}.zip") FileUtils.copy('bazel-bin/dotnet/strongnamed.zip', "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")-FileUtils.chmod(0o666, "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")+FileUtils.chmod(0o644, "build/dist/selenium-dotnet-strongnamed-#{dotnet_version}.zip")Suggestion 2:
Allow artifact downloads permission
Add actions: read permission to the commit job to ensure the download-artifact step can reliably fetch artifacts in restricted environments.
.github/workflows/commit-changes.yml [38-39]
permissions: contents: write+ actions: readSuggestion 3:
Avoid persisting Git credentials
Set persist-credentials: false in the checkout action and only authenticate when needed to reduce the exposure of secrets.
.github/workflows/release.yml [82-84]
with: token: ${{ secrets.SELENIUM_CI_TOKEN }}- persist-credentials: true+ persist-credentials: falseSuggestion 4:
Secure .npmrc file permissions
Set the file permissions of .npmrc to 0o600 after writing to it to protect the contained credentials.
if File.exist?(npmrc) File.open(npmrc, 'a') { |f| f.puts(auth_line) } else File.write(npmrc, "#{auth_line}\n") end+File.chmod(0o600, npmrc)Suggestion 5:
Prevent potential command injection vulnerability
Prevent a potential command injection vulnerability in credential_valid? by using the multi-argument form of system to check for command existence, which avoids shell interpolation.
def credential_valid?(cred) has_env = cred[:env]&.all? { |vars| vars.any? { |v| ENV.fetch(v, nil) } } has_file = cred[:file]&.call- has_cmd = cred[:cmd].nil? || system("which #{cred[:cmd]} > /dev/null 2>&1") || system("where #{cred[:cmd]} > nul 2>&1")+ has_cmd = cred[:cmd].nil? || system('which', cred[:cmd], out: File::NULL, err: File::NULL) || system('where', cred[:cmd], out: File::NULL, err: File::NULL) has_env || has_file || has_cmd endPattern 5: When complex parsing/validation logic is duplicated across workflows or scripts (e.g., release tag rules), centralize it into a shared script/reusable workflow and ensure all callers rely on the same validated outputs.
Example code before:
# workflow Arun: | # parse TAG # validate format # compute VERSION/LANG echo "version=$VERSION" >> "$GITHUB_OUTPUT"# workflow Brun: | # same parsing but slightly different rules echo "version=$VERSION" >> "$GITHUB_OUTPUT"Example code after:
# scripts/parse-release-tag.shset -euo pipefailTAG="$(echo "${1:-}" | tr -d '[:space:]')"# validate + compute VERSION/LANGUAGE...echo "tag=$TAG"echo "version=$VERSION"echo "language=$LANGUAGE"# workflow A/B- name: Parse tag shell: bash run: ./scripts/parse-release-tag.sh "${{ inputs.tag }}" >> "$GITHUB_OUTPUT"Relevant past accepted suggestions:
Suggestion 1:
Centralize duplicated release tag parsing logic
The release tag parsing and validation logic is duplicated across pre-release.yml and release.yml. This logic should be extracted into a single reusable workflow to serve as a single source of truth, improving maintainability.
.github/workflows/pre-release.yml [117-166]
run:| TAG="${{ inputs.tag }}" TAG="${TAG//[[:space:]]/}" # Validate tag format: selenium-X.Y.Z or selenium-X.Y.Z-lang if [[ ! "$TAG" =~ ^selenium-[0-9]+\.[0-9]+\.[0-9]+(-[a-z]+)?$ ]]; then echo "::error::Invalid tag format: '$TAG'. Expected selenium-X.Y.Z or selenium-X.Y.Z-lang" exit 1 fi... (clipped 40 lines)
.github/workflows/release.yml [38-79]
run:| if [ "$EVENT_NAME" == "workflow_dispatch" ]; then TAG="$INPUT_TAG" else # Extract tag from branch name: release-preparation-selenium-4.28.1-ruby -> selenium-4.28.1-ruby TAG=$(echo "$PR_HEAD_REF" | sed 's/^release-preparation-//') fi # Extract version VERSION=$(echo "$TAG" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+')... (clipped 32 lines)
# In .github/workflows/pre-release.ymljobs:parse-tag:steps: -name:Parse tagrun:| TAG="${{ inputs.tag }}" # ... complex parsing and validation logic ... if [[ "$PATCH" -gt 0 && -z "$LANG_SUFFIX" ]]; then echo "::error::Patch releases must specify a language" exit 1 fi # ... more validation ...# In .github/workflows/release.ymljobs:prepare:steps: -name:Extract and parse tagrun:| # ... logic to get TAG from branch or input ... # ... complex parsing and validation logic (duplicated) ... if [[ "$PATCH" -gt 0 && -z "$LANG_SUFFIX" ]]; then echo "::error::Patch releases must specify a language" exit 1 fi # ... more validation (duplicated) ...
# In .github/workflows/parse-release-tag.yml (new file)on:workflow_call:inputs:tag_string:type:stringoutputs:tag:version:language:jobs:parse:steps: -name:Parse tagrun:| # ... single source of truth for parsing and validation ... echo "tag=$TAG" >> "$GITHUB_OUTPUT" echo "version=$VERSION" >> "$GITHUB_OUTPUT" echo "language=$LANGUAGE" >> "$GITHUB_OUTPUT"# In .github/workflows/pre-release.yml and release.ymljobs:parse-tag:# or 'prepare' jobuses:./.github/workflows/parse-release-tag.ymlwith:tag_string:${{ inputs.tag }}# or ${{ github.event.pull_request.head.ref }}
Suggestion 2:
Centralize tag parsing logic
Move tag parsing/validation into a single shared script (or reusable workflow/composite action) and call it from all workflows to prevent future drift and inconsistent rules.
.github/workflows/pre-release.yml [114-166]
- name: Parse tag id: parse shell: bash run: |- TAG="${{ inputs.tag }}"- TAG="${TAG//[[:space:]]/}"+ ./scripts/parse-release-tag.sh "${{ inputs.tag }}" >> "$GITHUB_OUTPUT"- # Validate tag format: selenium-X.Y.Z or selenium-X.Y.Z-lang- if [[ ! "$TAG" =~ ^selenium-[0-9]+\.[0-9]+\.[0-9]+(-[a-z]+)?$ ]]; then- echo "::error::Invalid tag format: '$TAG'. Expected selenium-X.Y.Z or selenium-X.Y.Z-lang"- exit 1- fi-- # Extract version (strip 'selenium-' prefix and optional language suffix)- VERSION=$(echo "$TAG" | sed -E 's/^selenium-([0-9]+\.[0-9]+\.[0-9]+)(-[a-z]+)?$/\1/')- PATCH=$(echo "$VERSION" | cut -d. -f3)-- # Extract language suffix (default to 'all' if no suffix)- if [[ "$TAG" =~ -([a-z]+)$ ]]; then- LANG_SUFFIX="${BASH_REMATCH[1]}"- else- LANG_SUFFIX=""- fi-- # Patch releases must have a language suffix- if [[ "$PATCH" -gt 0 && -z "$LANG_SUFFIX" ]]; then- echo "::error::Patch releases must specify a language (e.g., selenium-${VERSION}-ruby)"- exit 1- fi-- # Full releases (X.Y.0) must not have a language suffix- if [[ "$PATCH" -eq 0 && -n "$LANG_SUFFIX" ]]; then- echo "::error::Full releases (X.Y.0) cannot have a language suffix"- exit 1- fi-- # Validate language suffix (rake namespace aliases allow full names)- case "$LANG_SUFFIX" in- ruby|python|javascript|java|dotnet)- LANGUAGE="$LANG_SUFFIX"- ;;- "")- LANGUAGE="all"- ;;- *)- echo "::error::Invalid language suffix: '$LANG_SUFFIX'. Expected ruby, python, javascript, java, or dotnet"- exit 1- ;;- esac-- echo "tag=$TAG" >> "$GITHUB_OUTPUT"- echo "version=$VERSION" >> "$GITHUB_OUTPUT"- echo "language=$LANGUAGE" >> "$GITHUB_OUTPUT"-
Suggestion 3:
Add missing tag validation logic
Add validation to the prepare job in release.yml to ensure full releases do not have a language suffix, aligning its logic with the pre-release.yml workflow.
.github/workflows/release.yml [32-79]
- name: Extract and parse tag id: parse env: EVENT_NAME: ${{ github.event_name }} INPUT_TAG: ${{ inputs.tag }} PR_HEAD_REF: ${{ github.event.pull_request.head.ref }} run: | if [ "$EVENT_NAME" == "workflow_dispatch" ]; then TAG="$INPUT_TAG" else # Extract tag from branch name: release-preparation-selenium-4.28.1-ruby -> selenium-4.28.1-ruby TAG=$(echo "$PR_HEAD_REF" | sed 's/^release-preparation-//') fi # Extract version VERSION=$(echo "$TAG" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+') PATCH=$(echo "$VERSION" | cut -d. -f3) # Extract language suffix if [[ "$TAG" =~ -([a-z]+)$ ]]; then LANG_SUFFIX="${BASH_REMATCH[1]}" else LANG_SUFFIX="" fi # Patch releases must have a language suffix if [[ "$PATCH" -gt 0 && -z "$LANG_SUFFIX" ]]; then echo "::error::Patch releases must specify a language (e.g., selenium-${VERSION}-ruby)" exit 1 fi+ # Full releases (X.Y.0) must not have a language suffix+ if [[ "$PATCH" -eq 0 && -n "$LANG_SUFFIX" ]]; then+ echo "::error::Full releases (X.Y.0) cannot have a language suffix"+ exit 1+ fi+ # Validate language suffix (rake namespace aliases allow full names) case "$LANG_SUFFIX" in ruby|python|javascript|java|dotnet) LANGUAGE="$LANG_SUFFIX" ;; "") LANGUAGE="all" ;; *) echo "::error::Invalid language suffix: '$LANG_SUFFIX'. Expected ruby, python, javascript, java, or dotnet" exit 1 ;; esac echo "tag=$TAG" >> "$GITHUB_OUTPUT" echo "version=$VERSION" >> "$GITHUB_OUTPUT" echo "language=$LANGUAGE" >> "$GITHUB_OUTPUT"[Auto-generated best practices - 2026-01-27]
This wiki is not where you want to be!Visit the Wiki Home for more useful links
Getting Involved
Triaging Issues
Releasing Selenium
Ruby Development
Python Bindings
Ruby Bindings
WebDriverJs
This content is being evaluated for where it belongs
Architectural Overview
Automation Atoms
HtmlUnitDriver
Lift Style API
LoadableComponent
Logging
PageFactory
RemoteWebDriver
Xpath In WebDriver
Moved toOfficial Documentation
Bot Style Tests
Buck
Continuous Integration
Crazy Fun Build
Design Patterns
Desired Capabilities
Developer Tips
Domain Driven Design
Firefox Driver
Firefox Driver Internals
Focus Stealing On Linux
Frequently Asked Questions
Google Summer Of Code
Grid Platforms
History
Internet Explorer Driver
InternetExplorerDriver Internals
Next Steps
PageObjects
RemoteWebDriverServer
Roadmap
Scaling WebDriver
SeIDE Release Notes
Selenium Emulation
Selenium Grid 4
Selenium Help
Shipping Selenium 3
The Team
TLC Meetings
Untrusted SSL Certificates
WebDriver For Mobile Browsers
Writing New Drivers