Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

.pr_agent_auto_best_practices

qodo-merge-bot edited this pageJan 27, 2026 ·15 revisions

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+done

Suggestion 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/dist

Example 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: false

Example 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: true
Relevant 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).

Examples:

.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)

Solution Walkthrough:

Before:

-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') }}...

After:

-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: true

Suggestion 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 }} --force

Suggestion 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: read

Suggestion 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: false

Suggestion 4:

Secure .npmrc file permissions

Set the file permissions of .npmrc to 0o600 after writing to it to protect the contained credentials.

Rakefile [414-418]

 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.

Rakefile [411-416]

 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 end

Pattern 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.

Examples:

.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)

Solution Walkthrough:

Before:

# 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) ...

After:

# 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]

Notice for Users

This wiki is not where you want to be!Visit the Wiki Home for more useful links

For Developers and Contributors

Getting Involved
Triaging Issues
Releasing Selenium

Binding Specific Information

Ruby Development
Python Bindings
Ruby Bindings
WebDriverJs

Being Processed

This content is being evaluated for where it belongs

Architectural Overview
Automation Atoms
HtmlUnitDriver
Lift Style API
LoadableComponent
Logging
PageFactory
RemoteWebDriver
Xpath In WebDriver

Archived

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

Clone this wiki locally


[8]ページ先頭

©2009-2026 Movatter.jp