Skip to content

fix(check-affected): leave the guardrails git-hook copies to the excluded lint - #9464

Closed
kyle-sexton wants to merge 2 commits into
mainfrom
fix/check-affected-skip-guardrails-copies
Closed

kyle-sexton wants to merge 2 commits into
mainfrom
fix/check-affected-skip-guardrails-copies

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 23, 2026 •

Copy link
Copy Markdown
Member

No related issue: unblocks pushes of the guardrails shared git-hook rollout (#9462), whose plugin-owned copies the local pre-push check and CI's ShellCheck step both lint.

Summary

The guardrails plugin's shared install writes plugin-owned copies of its git-hook scripts under .githooks/guardrails/. They fail this repository's ShellCheck rules (their source paths assume the plugin layout), so a branch that installs or refreshes them cannot be pushed without skipping the check-affected job, and CI's ShellCheck step fails on them. This PR excludes those copies from both, and only from ShellCheck.

Fix

Verification

  • New stay-quiet case in scripts/check-affected.test.sh: a .githooks/guardrails/copy.sh with a ShellCheck finding passes --uncommitted and is reported SKIP shellcheck (no changed shell). It fails on main's script (FAIL shellcheck ... .githooks/guardrails/copy.sh) and passes with the fix.
  • bash scripts/check-affected.test.sh: PASS=48 FAIL=0. bash scripts/check-affected-gates.test.sh: PASS=7 FAIL=0.
  • shellcheck --rcfile .shellcheckrc scripts/check-affected.sh and actionlint on the workflow: clean.
  • Verify gate (scripts/check-affected.sh --tests) on the head: pass.
  • Local code and security reviews ran before ready; their findings were fixed (narrower arm, routing assertion, CI exclude moved in with the comment it backs).

Related

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits October 23, 2026 12:45
…uded lint

The pre-push check-affected job shellchecks every changed .sh file, so a
commit that installs or refreshes the plugin-owned .githooks/guardrails/
copies cannot be pushed, though the workflow's shellcheck step excludes them.
The job runs the merge-base copy of this script, so the fix must land first.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…em in CI too

Review follow-up: the skip arm covers .sh and .bash only, matching what the
workflow excludes; the workflow's ShellCheck step gets the matching exclude so
the comment holds on main; the stay-quiet case asserts the routing message.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-23T17:00:49.865023Z 4a2d77c Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 4a2d77c7f05b3cde65d9275f7dac053754042ebf: 0 finding(s), 0 as inline comments.

I found no security issues in this pull request.

The change excludes .githooks/guardrails/ from ShellCheck in two places. In the CI workflow, .github/workflows/pr-require-checks.yml gets an exclude: input. In scripts/check-affected.sh, those paths now go into no lint list. A test case covers the new behaviour.

  • Linting is not a security control. Dropping ShellCheck on those copies removes a style and correctness lint, not an access or secret-scanning guard. The copies are committed hooks, and any change to them still goes through normal PR review. I did not find a check in this diff or the tree that the copies match their plugins/guardrails sources. The PR description says they are linted there. Without such a check, a copy could drift from its source without being linted, but that is hardening, not an exploitable path in this diff.
  • Nothing was widened. The skip patterns match only .githooks/guardrails/*.sh and *.bash. The case pattern has no user-controlled input, and the change adds no workflow permissions, triggers, secrets or action pins. The new line in the with: block is a YAML comment plus a literal exclude: value.
  • The secret and path scanners are untouched. plugins/guardrails/git-hooks/pre-commit-content.sh still enforces its own rules, and this PR does not change it.
  • No manifest changes. No plugin.json or marketplace.json is in the diff.
  • No embedded instructions. The PR context text contained no instructions aimed at the review.
  • No instruction-surface changes. The diff does not delete or soften a standing instruction.
  • No trust-gate paths. The diff does not touch trusted-actors, check-trusted-trigger, CODEOWNERS or .gitmodules.

critical-findings: 0

@github-actions

Copy link
Copy Markdown
Contributor

claude-review reviewed 4a2d77c7f05b3cde65d9275f7dac053754042ebf: 0 finding(s), 0 as inline comments.

Code review of #9464 at 4a2d77c: no findings.

I read the full diff (3 files) and the PR context, and checked the changed code against the surrounding script and workflow. I ran no tests or commands. The PR's PASS=48 FAIL=0 and "push ran with no skip" claims are the author's and I did not confirm them.

  • scripts/check-affected.sh:245-247: The new case arm comes before *.md and *.sh | *.bash, so it matches first. It sends .githooks/guardrails/*.sh and *.bash into no lint list. The glob's * also matches nested paths, which is the wanted behaviour. The shebang check on line 253 still runs for these files. That seems fine, since only ShellCheck is meant to skip them.
  • scripts/check-affected.test.sh: The new case drops the .shellcheckrc removal until after the guardrails check, so the file would fail ShellCheck if it were linted. It asserts RC == 0 and the SKIP shellcheck (no changed shell) line. That string matches what skip() and run_check build in check-affected.sh. The cleanup rm -rf "$repo/.shellcheckrc" "$repo/.githooks" covers everything the case creates.
  • manifest items: The diff touches no plugin manifest, so none apply.
  • Hold classes: None. The change adds no model spend and touches no trust-gate path.

Two notes for the author. Neither is a finding.

  1. PR description: It says the workflow's ShellCheck step "already excludes those copies (chore(git-hooks): install the guardrails git hooks as shared lefthook jobs #9462)". This diff is what adds exclude: .githooks/guardrails/ to that step (.github/workflows/pr-require-checks.yml:1507-1508). The description should say so, or the exclusion has to exist on another branch.
  2. Pinned action: I could not read melodic-software/ci-workflows/.github/actions/shellcheck@v0.58.0 here, so I did not confirm it accepts an exclude input or how it matches the value, including the trailing-slash directory form. The other exclude: inputs in this workflow belong to different actions. If the input is unsupported, GitHub only warns about an unexpected input. The exclusion would then silently not apply, and the local script and CI would disagree again. Run it on a PR that changes a .githooks/guardrails/*.sh file to check.

Comment thread .github/workflows/pr-require-checks.yml
Comment thread scripts/check-affected.sh
Comment thread scripts/check-affected.test.sh
@kyle-sexton
kyle-sexton marked this pull request as draft October 23, 2026 21:35
@kyle-sexton kyle-sexton added the do-not-merge Hard merge gate: do not merge while applied. label Oct 24, 2026
@kyle-sexton

Copy link
Copy Markdown
Member Author

Superseded by #9504: guardrails git hooks reach repositories through a lefthook remotes entry, so per-repo shared copies are no longer used.

@github-actions
github-actions Bot deleted the fix/check-affected-skip-guardrails-copies branch October 24, 2026 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge Hard merge gate: do not merge while applied.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant