Skip to content

ci: add shellcheck to pre-commit and fix existing warnings#1681

Open
ralphbean wants to merge 1 commit into
mainfrom
ci/add-shellcheck
Open

ci: add shellcheck to pre-commit and fix existing warnings#1681
ralphbean wants to merge 1 commit into
mainfrom
ci/add-shellcheck

Conversation

@ralphbean
Copy link
Copy Markdown
Contributor

Summary

  • Add shellcheck-py pre-commit hook so all shell scripts are linted on every commit and in CI (no extra install step needed — the hook bundles its own binary)
  • Fix all existing shellcheck warnings across 7 files (unused vars, bare redirections, useless cat, ungrouped appends)
  • Globally suppress three codes that are false positives in this codebase: SC1091 (dynamic source paths), SC2001 (sed backrefs), SC2016 (intentional single-quoted $vars in GraphQL)

Test plan

  • pre-commit run shellcheck --all-files passes clean
  • make script-test passes — no regressions from the fixes

🤖 Generated with Claude Code

Add shellcheck-py hook to pre-commit so all shell scripts are checked
on every commit and in CI. Fix existing warnings:

- SC2034: remove unused variable, rename to _ prefix
- SC2188: use `: >` instead of bare `> file` for truncation
- SC2002: replace useless cat with direct file arg
- SC2129: group repeated appends into a single redirect block

Globally suppress SC1091 (unresolvable dynamic source paths), SC2001
(sed backreferences that bash substitution cannot express), and SC2016
(intentional single-quoted GraphQL $variables).

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
@github-actions
Copy link
Copy Markdown

Site preview

Preview: https://164e536e-site.fullsend-ai.workers.dev

Commit: adac6c8e5ec022606a26dbe5989d4063830c40f5

@fullsend-ai-review
Copy link
Copy Markdown

Review

Findings

High

  • [protected-path] .pre-commit-config.yaml — This PR modifies a protected infrastructure file (.pre-commit-config.yaml) without a linked issue providing authorization for the change. The modification itself is sound (adding shellcheck-py as a pre-commit hook), but human approval is required for all protected-path changes. Consider linking a tracking issue to this PR.

Notes

The shellcheck fixes across all 7 files are correct and behavioral-preserving:

  • > file: > file — replaces bare redirections (SC2188) with the idiomatic null-command form
  • Grouped consecutive >> $GITHUB_ENV appends into a single { ... } >> $GITHUB_ENV block (SC2129)
  • Removed unused filename variable in hack/lint-workflow-size
  • Renamed unused pr_url to _pr_url in pre-code.sh
  • Replaced cat file | head with head file (SC2002)

The globally suppressed shellcheck codes are reasonable:

  • SC1091 — dynamic source paths that shellcheck cannot follow
  • SC2001 — sed with backreferences that cannot use ${var//search/replace}
  • SC2016 — intentional single-quoted $vars in GraphQL heredocs (consistent with actionlint's existing SC2016 suppression)

Copy link
Copy Markdown

@fullsend-ai-review fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread .pre-commit-config.yaml
hooks:
- id: gitleaks

- repo: https://github.com/shellcheck-py/shellcheck-py
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[high] protected-path

This PR modifies a protected infrastructure file (.pre-commit-config.yaml) without a linked issue providing authorization for the change. The modification itself is sound (adding shellcheck-py as a pre-commit hook), but human approval is required for all protected-path changes. Consider linking a tracking issue to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant