[CI] replace shellcheck script with shellcheck-py hook by wjabbour · Pull Request #52572 · vllm-project/vllm

wjabbour · 2026-08-17T04:44:48Z

Summary

Swaps tools/pre_commit/shellcheck.sh for the shellcheck-precommit hook, pinned to a commit hash (99470f5e1... = v0.11.0) instead of a version tag, per @hmellor's comment on [CI] replace shellcheck script with shellcheck-py hook #37743.
Fixes the warnings the hook surfaces on current main (severity=warning): unquoted array/command-substitution expansions, an embedded-literal-quote string-building pattern in two NixlConnector integration test scripts, and one dead variable.

Context

Creating to replace #37743 (thanks @SoluMilken for the original idea and hook config). That PR predates several changes to files it touched (e.g. the NixlConnector test scripts gained an HMA feature and a producer/consumer kv_transfer-config split since then), so rather than resolve stale conflicts I rebuilt the fix against current main and re-ran shellcheck fresh to make sure every change is still correct today.

Test plan

shellcheck -s bash --format=gcc --severity=warning passes clean across the whole repo
bash -n on every touched script
CI (pre-commit hook itself needs Docker in CI to actually execute; verify it runs there)

claude

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

wjabbour · 2026-08-17T05:15:04Z

@hmellor @SoluMilken Created this PR to help move along the shellcheck effort, incorporated Solu's changes and Harry's feedback (commit hash instead of version tag)

mergify · 2026-08-19T01:32:20Z

Documentation preview: https://vllm--52572.org.readthedocs.build/en/52572/

Swaps the custom tools/pre_commit/shellcheck.sh wrapper for the shellcheck-precommit hook, pinned to a commit hash rather than a version tag. Fixes the warnings the new hook surfaces on the current tree (word-splitting, unquoted array expansions, an embedded-quote string-building pattern in two NixlConnector test scripts). Supersedes vllm-project#37743, which had gone stale against main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Turner <doubleujabbour@gmail.com>

koalaman/shellcheck-precommit shells out to a Docker image, which isn't available in every dev/CI environment and made the hook fail to run locally. shellcheck-py/shellcheck-py wraps a pip-installed shellcheck binary instead (matches the PR title and vllm-project#37743's own choice) and runs cleanly with no Docker dependency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Turner <doubleujabbour@gmail.com>

Addresses hmellor's review comment on vllm-project#52572 asking to fix the warnings instead of indefinitely excluding the file. Only 3 warnings remained (SC2155, SC2089, SC2090); the latter two follow the same string-to-array fix already applied to GPU_DEVICES in run-multi-node-test.sh earlier in this PR. Signed-off-by: Turner <doubleujabbour@gmail.com>

… findings across the repo Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Turner <doubleujabbour@gmail.com> Signed-off-by: Turner Jabbour <doubleujabbour@gmail.com>

vllm-project#52264 added a chain of failure-diagnostic-collection functions (append_failure_diagnostic_*, run_failure_diagnostic, collect_*_diagnostic(s)) that are only reachable transitively through handle_amd_runner_exit, the EXIT trap handler. handle_amd_runner_exit itself already carries a shellcheck disable for this reason, but shellcheck's unused-function check doesn't do full transitive reachability from an indirectly-invoked entry point - everything that function calls still reads as dead code. Same false positive already worked around three times earlier in this file, just needed on the 13 new functions too now that this branch is rebased onto main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Turner <doubleujabbour@gmail.com>

Per hmellor's review on vllm-project#52572: - Replace the 13 per-function SC2329 disable comments in run-amd-test.sh with a single file-level directive - every function in this file is only reachable transitively through the EXIT trap handler, so the false positive applies uniformly rather than needing individual justification per function. Trimmed the redundant ,SC2329 off the three pre-existing SC2317 disables for the same reason (SC2317 stays per-function since it's still meaningful elsewhere in the file). - Prefix the deferred-cleanup shellcheck disable comments (SC2001 x3, SC2012, SC1001) with TODO: so they're greppable for the follow-up PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Turner <doubleujabbour@gmail.com>

hmellor · 2026-08-20T08:50:34Z

/ci run

github-actions · 2026-08-20T08:50:52Z

✅ Triggered Buildkite CI #84782 for commit 8307f0a61429.

…52572) Signed-off-by: Turner <doubleujabbour@gmail.com> Signed-off-by: Turner Jabbour <doubleujabbour@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Wyett <wyettzeng@gmail.com>

…52572) Signed-off-by: Turner <doubleujabbour@gmail.com> Signed-off-by: Turner Jabbour <doubleujabbour@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Zhu, Zufang <zufang.zhu@intel.com>

…52572) Signed-off-by: Turner <doubleujabbour@gmail.com> Signed-off-by: Turner Jabbour <doubleujabbour@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: khushali9 <khushali.desai9@gmail.com>

…52572) Signed-off-by: Turner <doubleujabbour@gmail.com> Signed-off-by: Turner Jabbour <doubleujabbour@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

Upstream sync 28/N: merge 1eab6fe [CI] replace shellcheck script with shellcheck-py hook (vllm-project#52572) (conflict)

wjabbour requested review from ApostaC, Harry-Chen, hmellor, ivanium, khluu and orozery as code owners August 17, 2026 04:44

claude Bot reviewed Aug 17, 2026

View reviewed changes

mergify Bot added ci/build kv-connector labels Aug 17, 2026

wjabbour force-pushed the rocm/shellcheck-py-hook branch 2 times, most recently from 78dd9d6 to 4874cab Compare August 17, 2026 05:12

hmellor reviewed Aug 17, 2026

View reviewed changes

Comment thread .pre-commit-config.yaml Outdated

hmellor reviewed Aug 17, 2026

View reviewed changes

Comment thread .pre-commit-config.yaml Outdated

wjabbour force-pushed the rocm/shellcheck-py-hook branch from 42d1977 to 78bd77d Compare August 18, 2026 04:54

mergify Bot added the documentation Improvements or additions to documentation label Aug 19, 2026

wjabbour and others added 5 commits August 18, 2026 22:40

[CI] Drop shellcheck --severity=warning, add GH matcher, quote SC2086…

b8962c6

… findings across the repo Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Turner <doubleujabbour@gmail.com> Signed-off-by: Turner Jabbour <doubleujabbour@gmail.com>

wjabbour force-pushed the rocm/shellcheck-py-hook branch from e3c40aa to 6cbc057 Compare August 19, 2026 04:47

hmellor reviewed Aug 19, 2026

View reviewed changes

Comment thread .buildkite/scripts/hardware_ci/run-amd-test.sh

hmellor mentioned this pull request Aug 19, 2026

[CI] replace shellcheck script with shellcheck-py hook #37743

Closed

5 tasks

wjabbour and others added 2 commits August 19, 2026 10:32

Merge branch 'main' into rocm/shellcheck-py-hook

8307f0a

hmellor enabled auto-merge (squash) August 20, 2026 08:50

github-actions Bot added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 20, 2026

hmellor approved these changes Aug 20, 2026

View reviewed changes

hmellor merged commit 1eab6fe into vllm-project:main Aug 20, 2026
134 checks passed

wjabbour mentioned this pull request Aug 22, 2026

[CI] Replace shellcheck-suppressed patterns flagged in #52572 with clean equivalents #53341

Open

3 tasks

This was referenced Sep 7, 2026

Upstream sync 27/N: merge 5b1e7a812b (121 commits, conflict-free) ROCm/vllm#1274

Merged

Upstream sync 28/N: merge 1eab6fef01 [CI] replace shellcheck script with shellcheck-py hook (#52572) (conflict) ROCm/vllm#1275

Merged

eble-amd added a commit to ROCm/vllm that referenced this pull request Sep 15, 2026

Merge pull request #1275 from ROCm/rogarcia.merge-upstream-124

ca2a4d5

Upstream sync 28/N: merge 1eab6fe [CI] replace shellcheck script with shellcheck-py hook (vllm-project#52572) (conflict)

Navigation Menu

Uh oh!

Uh oh!

[CI] replace shellcheck script with shellcheck-py hook - #52572

[CI] replace shellcheck script with shellcheck-py hook#52572
hmellor merged 7 commits into
vllm-project:mainfrom
wjabbour:rocm/shellcheck-py-hook

wjabbour commented Aug 17, 2026 •

edited

Loading

Uh oh!

claude Bot left a comment

Uh oh!

wjabbour commented Aug 17, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

mergify Bot commented Aug 19, 2026

Uh oh!

Uh oh!

hmellor commented Aug 20, 2026

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

wjabbour commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Context

Test plan

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

wjabbour commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mergify Bot commented Aug 19, 2026

Uh oh!

Uh oh!

hmellor commented Aug 20, 2026

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wjabbour commented Aug 17, 2026 •

edited

Loading

wjabbour commented Aug 17, 2026 •

edited

Loading