[build] add smart targeting and --lint flag to format.sh

[titusfortner]

Improve speed of format.sh so it can be used in pre-commit and pre-push hooks

🔗 Related Issues

independent of, but same implementation from Rake tasks in #17020 in format.sh

💥 What does this PR do?

Enhances scripts/format.sh with smart targeting and an optional --lint flag:

• Smart targeting: Compares HEAD to trunk's merge-base and only runs formatters for languages with changed files
• Fall back: Format everything if trunk ref not found
• --lint flag: Optionally runs linters (ruff check, rubocop, shellcheck + actionlint) after formatting
• Always runs: buildifier and copyright (fast, affect everything)
• Exit check: Fails with exit 1 if formatters modified files (useful for CI/hooks)

🔧 Implementation Notes

• Could have called the Rake tasks from the shell script, but I'd rather some duplication here than have shell
  files call Rake tasks. Rake tasks should always be entry point and wrap other things to keep flow consistent.

💡 Additional Considerations

• Future: Could add more linters as they become available (eslint, .NET analyzers)

🔄 Types of changes

• New feature (non-breaking change which adds functionality)

[qodo-code-review]

PR Type

Enhancement

---

Description

• Add smart targeting to only format changed files based on trunk merge-base

• Implement optional --lint flag to run linters after formatting

• Always run buildifier and copyright checks for consistency

• Exit with error if formatters modify files for CI/hook integration

• Reorganize formatter execution with conditional checks per language

---

File Walkthrough

Relevant files

Enhancement

format.sh Smart targeting and linting enhancements for format.sh      scripts/format.sh Added smart targeting logic to detect changed files by comparing HEAD to trunk merge-base Implemented --lint command-line flag to optionally run linters (ruff check, rubocop, shellcheck, actionlint) Wrapped language-specific formatters in conditional blocks that only execute if relevant files changed Added exit check that fails with code 1 if formatters modified any files Reordered formatter execution to always run buildifier and copyright first, then conditional language formatters +97/-24

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Missing edge handling: The script silently ignores unknown CLI arguments and uses a fragile shellcheck path extraction that may yield an empty path, causing later lint steps to fail without an actionable, validated error message. Referred Code run_lint=false for arg in "$@"; do case "$arg" in --lint) run_lint=true ;; esac done section() { echo "- $*" >&2 } # Find what's changed compared to trunk trunk_ref="$(git rev-parse --verify selenium/trunk 2>/dev/null \ || git rev-parse --verify origin/trunk 2>/dev/null \ || git rev-parse --verify trunk 2>/dev/null \ || echo "")" if [[ -n "$trunk_ref" ]]; then base="$(git merge-base HEAD "$trunk_ref" 2>/dev/null || echo "")" if [[ -n "$base" ]]; then changed="$(git diff --name-only "$base" HEAD)" ... (clipped 79 lines) Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Latest suggestions up to cb468d4

Category	Suggestion	Impact
Possible issue	Detect changes via diff snapshots Replace git status --porcelain with git diff and git diff --cached to accurately detect formatter-induced changes in already modified files. scripts/format.sh [71-145] # Capture baseline to detect formatter-introduced changes (allows pre-existing uncommitted work) -baseline="$(git status --porcelain)" +baseline_diff="$(git diff)" +baseline_cached_diff="$(git diff --cached)" ... -# Check if formatting introduced new changes (comparing to baseline) -if [[ "$(git status --porcelain)" != "$baseline" ]]; then +# Check if formatting introduced new changes (comparing to baseline diffs) +after_diff="$(git diff)" +after_cached_diff="$(git diff --cached)" +if [[ "$after_diff" != "$baseline_diff" || "$after_cached_diff" != "$baseline_cached_diff" ]]; then echo "" >&2 echo "Formatters modified files:" >&2 git diff --name-only >&2 exit 1 fi [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant bug in the change detection logic where formatter modifications to already-dirty files would be missed, and provides a robust solution using git diff.	High
	Validate external tool resolution Add a check to validate that the $SHELLCHECK path is resolved correctly before passing it to actionlint to prevent potential errors. scripts/format.ps1 [125-131] # Run shellcheck and actionlint when -Lint is passed if ($Lint) { section "Shell/Actions" Write-Host " actionlint (with shellcheck)" - $SHELLCHECK = (bazel run --run_under=echo @multitool//tools/shellcheck) + $SHELLCHECK = (bazel run --run_under=echo @multitool//tools/shellcheck 2>$null) + if (-not $SHELLCHECK -or -not (Test-Path -LiteralPath $SHELLCHECK)) { + Write-Error "Failed to resolve shellcheck binary via Bazel" + exit 1 + } bazel run @multitool//tools/actionlint:cwd -- -shellcheck "$SHELLCHECK" } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion improves script robustness by adding a check to ensure the shellcheck binary is resolved correctly before use, which provides clearer error messages and prevents silent failures.	Medium
	Guard formatter tool path Add a check to validate that the GOOGLE_JAVA_FORMAT path is resolved correctly before using it with find -exec to prevent potential errors. scripts/format.sh [84-89] if changed_matches '^java/'; then section "Java" echo " google-java-format" >&2 - GOOGLE_JAVA_FORMAT="$(bazel run --run_under=echo //scripts:google-java-format)" + GOOGLE_JAVA_FORMAT="$(bazel run --run_under=echo //scripts:google-java-format 2>/dev/null || true)" + if [[ -z "${GOOGLE_JAVA_FORMAT}" || ! -x "${GOOGLE_JAVA_FORMAT}" ]]; then + echo "ERROR: failed to resolve google-java-format binary via Bazel" >&2 + exit 1 + fi find "${WORKSPACE_ROOT}/java" -type f -name '*.java' -exec "$GOOGLE_JAVA_FORMAT" --replace {} + fi Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion improves script robustness by adding a check to ensure the google-java-format binary is resolved correctly before use, which provides clearer error messages and prevents potential failures.	Medium
Learned best practice	Validate resolved workspace directory Validate WORKSPACE_ROOT is non-empty and a directory before using it, and exit with a clear error if it can’t be resolved. scripts/format.sh [69] WORKSPACE_ROOT="$(bazel info workspace)" +if [[ -z "${WORKSPACE_ROOT}" || ! -d "${WORKSPACE_ROOT}" ]]; then + echo "ERROR: failed to resolve Bazel workspace root" >&2 + exit 1 +fi Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - When deriving filesystem paths (e.g., workspace root), validate the resolved path exists before using it for find/reads/writes to avoid operating on wrong directories.	Low
	Fix strict-mode shell options Fix the strict-mode flags to the intended set -euo pipefail so the script reliably fails on errors, unset variables, and pipeline failures. scripts/format.sh [9] -set -eufo pipefail +set -euo pipefail Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - In automation scripts, enable strict execution with correct shell options (e.g., set -euo pipefail) to fail fast and avoid undefined behavior.	Low
More

---

Previous suggestions

Suggestions up to commit 2df9efa

Category	Suggestion	Impact
Possible issue	Fail fast outside Git repo Add a check to ensure the script is run from within a git repository, and exit with a clear error message if it is not. scripts/format.sh [71-72] +# Ensure we're running inside a git work tree (otherwise git commands will fail under set -e) +if ! git rev-parse --is-inside-work-tree >/dev/null 2>&1; then + echo "ERROR: format.sh must be run from inside a git repository." >&2 + exit 1 +fi + # Capture baseline to detect formatter-introduced changes (allows pre-existing uncommitted work) baseline="$(git status --porcelain)" Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the script will fail if run outside a git repository and proposes adding an explicit check to provide a clearer error message, which improves robustness and user experience.	Low
	Make pattern matching robust Replace echo "$changed" | grep with a more robust here-string grep <<<"$changed" to handle filenames with special characters correctly. scripts/format.sh [65-67] changed_matches() { - [[ "$format_all" == "true" ]] || echo "$changed" | grep -qE "$1" + [[ "$format_all" == "true" ]] || grep -qE -- "$1" <<<"$changed" } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out the potential issues of using echo with variable data and proposes a more robust solution using a here-string, which prevents bugs with special filenames and is slightly more efficient.	Low
Learned best practice	Validate resolved tool paths Ensure the resolved SHELLCHECK path is non-empty and executable before passing it to actionlint, and exit with an actionable error if resolution fails. scripts/format.sh [132-137] if [[ "$run_lint" == "true" ]]; then section "Shell/Actions" echo " actionlint (with shellcheck)" >&2 - SHELLCHECK="$(bazel run --run_under=echo @multitool//tools/shellcheck)" + SHELLCHECK="$(bazel run --run_under=echo @multitool//tools/shellcheck 2>/dev/null || true)" + if [[ -z "${SHELLCHECK}" || ! -x "${SHELLCHECK}" ]]; then + echo "ERROR: failed to resolve shellcheck binary via Bazel" >&2 + exit 1 + fi bazel run @multitool//tools/actionlint:cwd -- -shellcheck "$SHELLCHECK" fi Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Validate derived command outputs (e.g., tool paths) before use and fail fast with clear errors.	Low

Suggestions up to commit 9f58114

Category	Suggestion	Impact
Possible issue	Only fail on hook checks Modify the script to only exit with an error when formatting introduces changes in hook modes (--pre-commit, --pre-push). In the default mode, the script should exit successfully after applying formatting. scripts/format.sh [146-154] # Check if formatting introduced new changes (comparing to baseline) if [[ "$(git status --porcelain)" != "$baseline" ]]; then echo "" >&2 echo "Formatters modified files:" >&2 - git diff --name-only >&2 - exit 1 + { + git diff --name-only + git diff --name-only --cached + } | sed '/^$/d' | LC_ALL=C sort -u >&2 + + case "$mode" in + pre-commit|pre-push) + exit 1 + ;; + *) + echo "" >&2 + echo "Formatting applied." >&2 + exit 0 + ;; + esac fi echo "Format check passed." >&2 Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the script always fails if formatting is applied, which is counter-intuitive. It proposes to change this behavior to only fail in hook modes, significantly improving the script's usability and logical correctness.	Medium
Learned best practice	Validate workspace root before use Validate that bazel info workspace succeeded and that WORKSPACE_ROOT is non-empty and an existing directory; otherwise exit with an actionable error to avoid using paths like /java when empty. scripts/format.sh [76] -WORKSPACE_ROOT="$(bazel info workspace)" +WORKSPACE_ROOT="$(bazel info workspace 2>/dev/null || true)" +if [[ -z "${WORKSPACE_ROOT}" || ! -d "${WORKSPACE_ROOT}" ]]; then + echo "ERROR: failed to resolve Bazel workspace root (is bazel installed and are you in a workspace?)" >&2 + exit 1 +fi Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Validate derived filesystem paths/command outputs before use, failing fast with a clear error to avoid accidentally operating on the wrong directories.	Low
	Ignore deleted files in targeting Exclude deleted files from the changed-file list so removed paths don’t trigger formatters/linters unnecessarily, reducing noise and wasted work. scripts/format.sh [54-58] -committed="$(git diff --name-only "$base" HEAD)" -staged="$(git diff --name-only --cached)" -unstaged="$(git diff --name-only)" +committed="$(git diff --name-only --diff-filter=ACMRT "$base" HEAD)" +staged="$(git diff --name-only --diff-filter=ACMRT --cached)" +unstaged="$(git diff --name-only --diff-filter=ACMRT)" untracked="$(git ls-files --others --exclude-standard)" changed="$(printf '%s\n%s\n%s\n%s' "$committed" "$staged" "$unstaged" "$untracked" | sort -u)" Suggestion importance[1-10]: 5 __ Why: Relevant best practice - In automation scripts, validate/sanitize inputs and intermediate outputs before use to keep runs deterministic and avoid unnecessary work/noise.	Low

✅ Suggestions up to commit 89ed7c5

Category	Suggestion	Impact
Possible issue	✅ Enforce mutually exclusive modes Suggestion Impact: Updated argument parsing so that selecting --pre-commit or --pre-push errors out if a non-default mode was already set, preventing use of both flags together. code diff: @@ -13,8 +13,14 @@ case "$arg" in --lint) run_lint=true ;; - --pre-commit) mode="pre-commit" ;; - --pre-push) mode="pre-push" ;; + --pre-commit) + [[ "$mode" == "default" ]] || { echo "Cannot use both --pre-commit and --pre-push" >&2; exit 1; } + mode="pre-commit" + ;; + --pre-push) + [[ "$mode" == "default" ]] || { echo "Cannot use both --pre-commit and --pre-push" >&2; exit 1; } + mode="pre-push" + ;; Add a check to ensure that the --pre-commit and --pre-push flags are mutually exclusive, exiting with an error if both are provided. scripts/format.sh [10-24] run_lint=false mode="default" +seen_mode="" for arg in "$@"; do case "$arg" in --lint) run_lint=true ;; - --pre-commit) mode="pre-commit" ;; - --pre-push) mode="pre-push" ;; + --pre-commit|--pre-push) + if [[ -n "$seen_mode" && "$seen_mode" != "$arg" ]]; then + echo "ERROR: --pre-commit and --pre-push are mutually exclusive" >&2 + exit 1 + fi + seen_mode="$arg" + [[ "$arg" == "--pre-commit" ]] && mode="pre-commit" || mode="pre-push" + ;; *) echo "Unknown option: $arg" >&2 echo "Usage: $0 [--pre-commit] [--pre-push] [--lint]" >&2 exit 1 ;; esac done [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that --pre-commit and --pre-push should be mutually exclusive and adds a check to enforce this, preventing unexpected behavior from misconfiguration.	Medium
	✅ Harden formatter file piping Suggestion Impact: Replaced the `find ... | xargs` pipeline with `find ... -exec ... {} +`, which similarly avoids filename-splitting issues and hardens the formatter invocation. code diff: section "Java" echo " google-java-format" >&2 GOOGLE_JAVA_FORMAT="$(bazel run --run_under=echo //scripts:google-java-format)" - find "${WORKSPACE_ROOT}/java" -type f -name '*.java' | xargs "$GOOGLE_JAVA_FORMAT" --replace + find "${WORKSPACE_ROOT}/java" -type f -name '*.java' -exec "$GOOGLE_JAVA_FORMAT" --replace {} + fi Improve the robustness of the find | xargs pipeline by using find -print0 and xargs -0 -r to correctly handle filenames containing spaces or newlines. scripts/format.sh [82-87] if changed_matches '^java/'; then section "Java" echo " google-java-format" >&2 GOOGLE_JAVA_FORMAT="$(bazel run --run_under=echo //scripts:google-java-format)" - find "${WORKSPACE_ROOT}/java" -type f -name '*.java' | xargs "$GOOGLE_JAVA_FORMAT" --replace + find "${WORKSPACE_ROOT}/java" -type f -name '*.java' -print0 | xargs -0 -r "$GOOGLE_JAVA_FORMAT" --replace fi [Suggestion processed] Suggestion importance[1-10]: 5 __ Why: The suggestion correctly applies shell scripting best practices to make the find | xargs pipeline more robust against filenames with special characters, which improves code quality.	Low
Incremental [*]	Filter empty paths before sorting Filter empty lines from the list of changed files by piping the output of printf through sed '/^$/d' before sorting. scripts/format.sh [51-52] untracked="$(git ls-files --others --exclude-standard)" -changed="$(printf '%s\n%s\n%s\n%s' "$committed" "$staged" "$unstaged" "$untracked" | sort -u)" +changed="$(printf '%s\n' "$committed" "$staged" "$unstaged" "$untracked" | sed '/^$/d' | LC_ALL=C sort -u)" Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that empty git command outputs can create empty lines in the changed variable and proposes using sed to filter them out, which improves the script's robustness.	Low
	Use portable output printing Replace echo with the more portable printf command to ensure consistent output across different shell environments. scripts/format.sh [132] -echo " actionlint (with shellcheck)" >&2 +printf ' actionlint (with shellcheck)\n' >&2 Suggestion importance[1-10]: 3 __ Why: The suggestion correctly points out that printf is more portable than echo, which is a good practice for shell scripting, although echo is unlikely to cause issues in this specific context.	Low

✅ Suggestions up to commit 60aabb3

Category	Suggestion	Impact
Possible issue	✅ Detect untracked formatting outputs Suggestion Impact: The commit improved handling of untracked files by adding `git ls-files --others --exclude-standard` into the computed `changed` set, ensuring untracked files are considered when determining what formatting should run (though it did not directly replace `git diff --quiet` with `git status --porcelain` as suggested). code diff: @@ -28,7 +37,21 @@ if [[ -n "$trunk_ref" ]]; then base="$(git merge-base HEAD "$trunk_ref" 2>/dev/null || echo "")" if [[ -n "$base" ]]; then - changed="$(git diff --name-only "$base" HEAD)" + case "$mode" in + pre-commit) + changed="$(git diff --name-only --cached)" + ;; + pre-push) + changed="$(git diff --name-only "$base" HEAD)" + ;; + default) + committed="$(git diff --name-only "$base" HEAD)" + staged="$(git diff --name-only --cached)" + unstaged="$(git diff --name-only)" + untracked="$(git ls-files --others --exclude-standard)" + changed="$(printf '%s\n%s\n%s\n%s' "$committed" "$staged" "$unstaged" "$untracked" | sort -u)" + ;; + esac Replace git diff --quiet with git status --porcelain to detect both tracked and untracked file changes, ensuring the script correctly identifies all modifications made by formatters. scripts/format.sh [114-122] -# Check if formatting made changes -if ! git diff --quiet; then +# Check if formatting made changes (including untracked files) +if [[ -n "$(git status --porcelain)" ]]; then echo "" >&2 echo "Formatters modified files:" >&2 - git diff --name-only >&2 + git status --porcelain >&2 exit 1 fi echo "Format check passed." >&2 Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that git diff --quiet misses untracked files, and using git status --porcelain is a more robust way to check for a dirty working tree, improving the script's correctness.	Medium
	Ignore deleted files in targeting Modify the git diff command to exclude deleted files by using the --diff-filter=ACMR flag, preventing formatters from running unnecessarily on file deletions. scripts/format.sh [29-35] base="$(git merge-base HEAD "$trunk_ref" 2>/dev/null || echo "")" if [[ -n "$base" ]]; then - changed="$(git diff --name-only "$base" HEAD)" + changed="$(git diff --name-only --diff-filter=ACMR "$base" HEAD)" else format_all=true changed="" fi Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that deleted files can trigger unnecessary formatter runs, and the proposed change using git diff --diff-filter=ACMR is a valid and robust improvement to prevent this.	Low
Learned best practice	Validate workspace root path Validate that WORKSPACE_ROOT is non-empty and points to an existing directory (and provide a clear error) to avoid running formatters against unintended paths when Bazel isn’t available or misconfigured. scripts/format.sh [47] -WORKSPACE_ROOT="$(bazel info workspace)" +WORKSPACE_ROOT="$(bazel info workspace 2>/dev/null || true)" +if [[ -z "$WORKSPACE_ROOT" || ! -d "$WORKSPACE_ROOT" ]]; then + echo "ERROR: failed to resolve Bazel workspace root (is Bazel installed/configured?)" >&2 + exit 1 +fi Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Resolve and validate derived filesystem paths before use (e.g., workspace root), failing with an actionable error if missing/invalid.	Low
	✅ Reject unknown CLI arguments Suggestion Impact: Added a default case in the CLI argument parser to treat any unrecognized argument as an error, print a usage message, and exit non-zero. code diff: @@ -11,6 +11,11 @@ for arg in "$@"; do case "$arg" in --lint) run_lint=true ;; + *) + echo "Unknown option: $arg" >&2 + echo "Usage: $0 [--lint]" >&2 + exit 1 + ;; esac Treat any unrecognized argument as an error and print a short usage message so CI/manual runs don’t silently ignore typos or unsupported flags. scripts/format.sh [10-15] run_lint=false for arg in "$@"; do case "$arg" in --lint) run_lint=true ;; + -h|--help) + echo "Usage: $0 [--lint]" >&2 + exit 0 + ;; + *) + echo "ERROR: unknown argument: $arg" >&2 + echo "Usage: $0 [--lint]" >&2 + exit 2 + ;; esac done [Suggestion processed] Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Validate and sanitize CLI inputs in automation scripts; fail fast with a clear error on unknown/invalid arguments.	Low

✅ Suggestions up to commit b1f4b20

Category	Suggestion	Impact
High-level	✅ Refine change detection logic Suggestion Impact: Added a `format_all` flag that is set when trunk ref/merge-base cannot be determined, and updated `changed_matches` to use that flag instead of treating an empty `changed` list as “match everything”, preventing all formatters from running when there are no changes. code diff: # Find what's changed compared to trunk +format_all=false trunk_ref="$(git rev-parse --verify selenium/trunk 2>/dev/null \ || git rev-parse --verify origin/trunk 2>/dev/null \ || git rev-parse --verify trunk 2>/dev/null \ @@ -26,16 +27,18 @@ if [[ -n "$base" ]]; then changed="$(git diff --name-only "$base" HEAD)" else + format_all=true changed="" fi else # No trunk ref found, format everything + format_all=true changed="" fi # Helper to check if a pattern matches changed files changed_matches() { - [[ -z "$changed" ]] || echo "$changed" | grep -qE "$1" + [[ "$format_all" == "true" ]] || echo "$changed" | grep -qE "$1" } Modify the change detection logic to differentiate between "no changed files" and "unable to determine changes". This prevents the script from inefficiently running all formatters when no files have actually been modified. Examples: scripts/format.sh [24-39] if [[ -n "$trunk_ref" ]]; then base="$(git merge-base HEAD "$trunk_ref" 2>/dev/null || echo "")" if [[ -n "$base" ]]; then changed="$(git diff --name-only "$base" HEAD)" else changed="" fi else # No trunk ref found, format everything changed="" ... (clipped 6 lines) Solution Walkthrough: Before: # ... find trunk ref ... if [[ -n "$trunk_ref" ]]; then # ... find merge-base ... changed="$(git diff --name-only "$base" HEAD)" # This is empty if no files changed else changed="" # This is empty if trunk is not found fi changed_matches() { # This is TRUE if `changed` is empty, causing all formatters to run [[ -z "$changed" ]] || echo "$changed" | grep -qE "$1" } if changed_matches '^java/'; then # run java formatter fi # ... and so on for all other formatters After: # Use a flag to control behavior run_all_formatters=false if [[ -z "$trunk_ref" ]]; then run_all_formatters=true changed="" else # ... find merge-base ... changed="$(git diff --name-only "$base" HEAD)" fi changed_matches() { # Run all if flagged, otherwise only if there are changes that match "$run_all_formatters" || ([[ -n "$changed" ]] && echo "$changed" | grep -qE "$1") } if changed_matches '^java/'; then # run java formatter fi # ... and so on for all other formatters Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant logical flaw where the script inefficiently runs all formatters when no files have changed, which contradicts the PR's primary goal of improving performance for pre-commit hooks.	High
General	Fix shell option flags Correct the set command flags from -eufo pipefail to -euo pipefail to enable globbing and correctly apply the pipefail option. scripts/format.sh [5] -set -eufo pipefail +set -euo pipefail Suggestion importance[1-10]: 8 __ Why: The suggestion corrects a subtle but important bug in the shell options. The -f flag disables globbing, which is likely unintended and could cause issues, and -eufo pipefail does not correctly set pipefail. This is a critical fix for script correctness and robustness.	Medium
	Format only changed Java files Improve performance by formatting only the changed Java files, instead of all files in the java/ directory. scripts/format.sh [53-58] if changed_matches '^java/'; then section "Java" - echo " google-java-format" >&2 - GOOGLE_JAVA_FORMAT="$(bazel run --run_under=echo //scripts:google-java-format)" - find "$PWD/java" -type f -name '*.java' | xargs "$GOOGLE_JAVA_FORMAT" --replace + changed_java_files=$(echo "$changed" | grep '\.java$' || true) + if [[ -n "$changed_java_files" ]]; then + echo " google-java-format" >&2 + GOOGLE_JAVA_FORMAT="$(bazel run --run_under=echo //scripts:google-java-format)" + echo "$changed_java_files" | xargs "$GOOGLE_JAVA_FORMAT" --replace + fi fi Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the script formats all Java files instead of only the changed ones, which contradicts the PR's goal of targeted formatting. Applying this change improves performance and aligns the script's behavior with its intended purpose.	Medium
	✅ Handle filenames safely in xargs Suggestion Impact: The commit removed the unsafe find | xargs pipeline and replaced it with find -exec ... {} +, which similarly avoids filename-splitting issues and makes handling spaces/special characters safe. code diff: section "Java" echo " google-java-format" >&2 GOOGLE_JAVA_FORMAT="$(bazel run --run_under=echo //scripts:google-java-format)" - find "${WORKSPACE_ROOT}/java" -type f -name '*.java' | xargs "$GOOGLE_JAVA_FORMAT" --replace + find "${WORKSPACE_ROOT}/java" -type f -name '*.java' -exec "$GOOGLE_JAVA_FORMAT" --replace {} + fi Modify the find and xargs command to use null delimiters (-print0 and -0) to handle filenames containing spaces or special characters correctly. scripts/format.sh [57] -find "$PWD/java" -type f -name '*.java' | xargs "$GOOGLE_JAVA_FORMAT" --replace +find "$PWD/java" -type f -name '*.java' -print0 | xargs -0 "$GOOGLE_JAVA_FORMAT" --replace Suggestion importance[1-10]: 6 __ Why: This suggestion improves the script's robustness by making the find | xargs pipe safe for filenames with spaces or special characters. It's a best practice that prevents potential errors with unusually named files.	Low
	✅ Simplify shellcheck path lookup Suggestion Impact: Replaced the fragile `bazel build ... | grep ...` shellcheck path lookup with `bazel run --run_under=echo @multitool//tools/shellcheck`. code diff: - SHELLCHECK="$(bazel build @multitool//tools/shellcheck 2>&1 | grep -oE 'bazel-out/\S+/shellcheck$')" + SHELLCHECK="$(bazel run --run_under=echo @multitool//tools/shellcheck)" bazel run @multitool//tools/actionlint:cwd -- -shellcheck "$SHELLCHECK" Use bazel run --run_under=echo to reliably get the shellcheck binary path instead of parsing bazel build output. scripts/format.sh [104-105] -SHELLCHECK="$(bazel build @multitool//tools/shellcheck 2>&1 | grep -oE 'bazel-out/\\S+/shellcheck$')" +SHELLCHECK="$(bazel run --run_under=echo @multitool//tools/shellcheck)" bazel run @multitool//tools/actionlint:cwd -- -shellcheck "$SHELLCHECK" [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: The suggestion replaces a fragile method of finding an executable (parsing bazel build output) with a robust and idiomatic approach (bazel run --run_under=echo) that is already used elsewhere in the script. This improves maintainability and reliability.	Low

[Copilot]

Pull request overview

Updates scripts/format.sh to make it fast enough for local hooks by only running formatters for languages that changed relative to trunk (with an optional --lint mode).

Changes:

• Adds “smart targeting” by diffing HEAD vs trunk merge-base and conditionally running per-language formatters.
• Adds --lint flag to optionally run additional linters after formatting.
• Adds an exit-1 check when formatters modify files (useful for CI/hooks), while always running buildifier + copyright.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.

[cgoldberg]

I don't think you can run this with set -eufo pipefail.

set -e will exit if any command returns non-zero status.. so if a linter/formatter finds an error, it will stop and you'll never see the diff or changed files at the end (unless bazel commands always return 0?).

If you remove set -e, it will always run to completion. If we care about the entire script exiting with a non-zero status, a nice pattern is a wrapper around every command, like this:

#!/usr/bin/env bash

set -ufo pipefail

failed=0

run() {
  "$@" || failed=1
}

run bazel run //whatever1
run bazel run //whatever2

exit "$failed"

[cgoldberg]

see comment

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

[titusfortner]

@cgoldberg formatters exit 0. We fail the script at the end if there is a diff not as a result of the failure. Presumably the linter failures can't be auto-fixed, so if there's a problem, I think fail-fast for that is ok locally.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

[cgoldberg]

Presumably the linter failures can't be auto-fixed, so if there's a problem, I think fail-fast for that is ok locally.

With the Python change I put in yesterday, the linter can autofix something and will fail-fast (it will show what it fixed), so you won't see the diff at the end if it also made formatting changes. Not a big deal.. it's probably fine as-is.

[titusfortner]

Yeah, it's a little muddled in rubocop as well since it can fix some linting things. hard to know exactly where to split these hairs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.