Skip to content

Commit 7829b2a

Browse files
iavigorpecovnik
authored andcommitted
ci(lint-scripts): scope on PR-API filenames, publish report on gate fail
The Maintenance: Lint scripts workflow has two UX gaps that surface together as "red Check with no inline comments and no readable cause": 1. **Diverging sources of "what changed in this PR".** * The red-cross gate and the artifact-producing step previously used `tj-actions/changed-files` (two-dot `git diff base..head`), which on a stale-rebased PR balloons to every commit since the branch forked — hundreds of legacy files. * The post workflow's `reviewdog -filter-mode=added` reads `pulls/N/files` (three-dot, merge-base, same as the UI tab). * When the two disagree, the gate trips on a legacy file outside the PR-API set, the Check goes red, and reviewdog drops the matching finding because it isn't in *its* (smaller) added-line set. The author sees red with no comment, no Summary, nothing to act on. 2. **Gate failure swallows the diagnostic.** The gate prints findings to stdout, only visible in the collapsed job log. The post workflow is the only surface that publishes inline review comments, and it can no-op on its own (cross-fork PR with limited permissions; filter discrepancy as in (1); reviewdog itself failing). Resolution: * Replace `tj-actions/changed-files` with a `gh api --paginate pulls/N/files --jq '.[].filename | @JSON'` step, written to a workspace file `pr-changed-files.lines` as one **JSON-encoded** path per line. Both consumers (`ShellCheck — produce findings` and `Red-cross gate`) `jq -r .` each line into `$file` — an assignment, not a `${{ }}` interpolation, so any `$(cmd)` inside a filename stays a literal string. JSON escapes newlines and quotes inside the path; the real-newline separator can never appear inside an encoded entry, so a path with a literal newline survives intact. * Add `pull-requests: read` to workflow permissions (the `pulls/N/files` endpoint requires it under fine-grained / app tokens; otherwise the call fails with `Resource not accessible by integration`). * Fail closed early when `pull_request.changed_files > 3000` (the API's prefix cap). PRs of that size are rare; a local merge-base fallback is straightforward to add when one shows up. * In the gate step, tee findings to a temp log. On `ret != 0`: * dump the log into `$GITHUB_STEP_SUMMARY` under a fenced code block (visible directly on the Check page); * walk the gcc-format lines and emit `::error file=,line=,col=` annotations — each error gets an inline red marker on the offending line in the Files-changed tab. Fully decoupled from the post workflow: if reviewdog never runs, the author still sees what failed and where. Per-file ShellCheck migrated from `for file in $CHANGED_FILES` (word-split on $IFS, mangles whitespace) to per-line decode via `while IFS= read -r line ... file=$(jq -r . <<< "$line") ... done < pr-changed-files.lines`. Assisted-by: Claude:claude-opus-4.7
1 parent ea2041f commit 7829b2a

1 file changed

Lines changed: 110 additions & 18 deletions

File tree

.github/workflows/maintenance-lint-scripts.yml

Lines changed: 110 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@ on:
2525

2626
permissions:
2727
contents: read
28+
# Required for the `gh api repos/.../pulls/N/files` call below.
29+
# `contents: read` alone leaves all other scopes at `none` under
30+
# fine-grained / app tokens, and the endpoint then returns
31+
# `Resource not accessible by integration`.
32+
pull-requests: read
2833

2934
concurrency:
3035
group: pipeline-lint-${{ github.event.pull_request.number }}
@@ -60,9 +65,46 @@ jobs:
6065
with:
6166
fetch-depth: 2
6267

63-
- name: Get changed files
64-
id: changed-files
65-
uses: tj-actions/changed-files@9426d40962ed5378910ee2e21d5f8c6fcbf2dd96 # v46.0.5
68+
# PR-scope file list. Uses the GitHub PR API (merge-base diff,
69+
# the same set the PR's "Files changed" tab and reviewdog's
70+
# filter_mode=added operate on), not `git diff base..head` —
71+
# the latter expands to every commit since the head branch
72+
# forked, which on stale-rebased PRs balloons to hundreds of
73+
# legacy files and produces red Checks unrelated to the PR.
74+
#
75+
# The list is written to `pr-changed-files.lines` as one
76+
# JSON-encoded path per line. Both the `$GITHUB_OUTPUT`
77+
# multiline-heredoc and the env-block alternatives are too
78+
# fragile here: Git allows newlines and quotes in paths, and
79+
# filename data is attacker-controlled (any PR can add any
80+
# path), so any encoding that uses raw newlines as separator
81+
# — or any in-script `${{ … }}` interpolation — is a route
82+
# to either truncation, lost files, or command injection at
83+
# parse time. JSON-per-line cleanly addresses all three: the
84+
# newline separator cannot appear inside a JSON-encoded path,
85+
# and consumers `jq -r .` to recover the raw string into a
86+
# shell variable that already exists (no parsing of the path
87+
# *as* shell code).
88+
- name: Get changed files (PR-scope, merge-base)
89+
env:
90+
GH_TOKEN: ${{ github.token }}
91+
PR_FILE_COUNT: ${{ github.event.pull_request.changed_files }}
92+
run: |
93+
# `pulls/N/files` caps at 3000 entries. Above that GitHub
94+
# silently returns only a prefix, the downstream gate would
95+
# lint a partial PR, and SC errors in trailing files could
96+
# ship as green. Refuse loudly instead — a local merge-base
97+
# fallback is possible but rarely worth carrying since PRs
98+
# of this size are exceptional.
99+
if (( PR_FILE_COUNT > 3000 )); then
100+
echo "::error::PR changes ${PR_FILE_COUNT} files; GitHub's pulls/N/files API caps at 3000. Split the PR, or extend this workflow with a local merge-base fallback."
101+
exit 1
102+
fi
103+
gh api --paginate \
104+
"repos/${GITHUB_REPOSITORY}/pulls/${{ github.event.pull_request.number }}/files" \
105+
--jq '.[].filename | @json' > pr-changed-files.lines
106+
echo "PR-scope changed files (#${{ github.event.pull_request.number }}):"
107+
jq -r . < pr-changed-files.lines | sed 's/^/ /'
66108
67109
- name: Install shfmt (used by `shfmt -f .` enumerator and the drift artifact)
68110
run: sudo apt-get update -qq && sudo apt-get install -y shfmt
@@ -103,15 +145,21 @@ jobs:
103145
# in the red-cross gate at end of job, but their output
104146
# also needs to land in the artifact so the post phase can
105147
# inline-comment on them. Same exclusion regex as the gate.
106-
CHANGED_FILES="${{ steps.changed-files.outputs.all_changed_files }}"
107-
for file in $CHANGED_FILES; do
148+
#
149+
# Read each path as JSON (escapes embedded newlines/quotes)
150+
# then jq-decode into $file. Because $file is *assigned*, not
151+
# interpolated, even a path like `x"$(id)".sh` becomes a
152+
# plain string here — no command substitution at parse time.
153+
while IFS= read -r line; do
154+
[[ -z "$line" ]] && continue
155+
file=$(jq -r . <<< "$line")
108156
[[ -f "$file" ]] || continue
109157
if [[ ! "$file" =~ lib/|extensions/|\.py$|\.service$|\.rules$|\.network$|\.netdev$ ]]; then
110158
if grep -qE '^#!/.*bash' "$file" 2>/dev/null; then
111159
"$SC" --shell=bash --severity=error --format=gcc "$file" >> shellcheck-findings.txt 2>&1 || true
112160
fi
113161
fi
114-
done
162+
done < pr-changed-files.lines
115163
116164
- name: ShellCheck — produce auto-fix diff (PR-suggestion artifact)
117165
run: |
@@ -180,22 +228,66 @@ jobs:
180228
# ============================================================
181229
- name: Red-cross gate (framework run + critical per-file)
182230
run: |
183-
bash lib/tools/shellcheck.sh
184-
185-
# Assign the changed-files template output into a shell
186-
# variable first so its content cannot be parsed by bash as
187-
# code (a malicious PR could otherwise name a file
188-
# `$(cmd).sh` and execute that command on template
189-
# expansion — zizmor template-injection).
190-
CHANGED_FILES="${{ steps.changed-files.outputs.all_changed_files }}"
191-
ret=0
192-
for file in $CHANGED_FILES; do
231+
# Capture every gate diagnostic so a non-zero exit can publish
232+
# a readable report to the Step Summary. Without this the only
233+
# artifact of a fail is the bare job log buried under collapsed
234+
# step groups.
235+
gate_log="$(mktemp)"
236+
trap 'rm -f "$gate_log"' EXIT
237+
238+
set +e
239+
bash lib/tools/shellcheck.sh 2>&1 | tee -a "$gate_log"
240+
framework_ret=${PIPESTATUS[0]}
241+
242+
# Same cached, version-pinned ShellCheck binary the artifact
243+
# step uses — not the runner's system `shellcheck`, whose
244+
# version may differ and report findings the gate and the
245+
# artifact disagree on.
246+
SC=$(find cache/tools/shellcheck -name 'shellcheck-v*' -type f -executable | head -1)
247+
248+
# Read each path as JSON-encoded (escapes newlines/quotes)
249+
# then jq-decode into $file. Path content is attacker-
250+
# controlled (any PR can add any filename), so it must
251+
# never reach the shell parser as code — see comments on
252+
# the `Get changed files` step above.
253+
per_file_ret=0
254+
while IFS= read -r line; do
255+
[[ -z "$line" ]] && continue
256+
file=$(jq -r . <<< "$line")
193257
[[ -f "$file" ]] || continue
194258
if [[ ! "$file" =~ lib/|extensions/|\.py$|\.service$|\.rules$|\.network$|\.netdev$ ]]; then
195259
if grep -qE '^#!/.*bash' "$file" 2>/dev/null; then
196-
shellcheck --shell=bash --severity=error "$file" || ret=$?
260+
# gcc format keeps the captured log compact and
261+
# file:line-prefixed for the Step Summary.
262+
"$SC" --shell=bash --severity=error --format=gcc "$file" 2>&1 | tee -a "$gate_log"
263+
rc=${PIPESTATUS[0]}
264+
[[ $rc -ne 0 ]] && per_file_ret=$rc
197265
fi
198266
fi
199-
done
267+
done < pr-changed-files.lines
268+
set -e
269+
270+
ret=0
271+
[[ $framework_ret -ne 0 ]] && ret=$framework_ret
272+
[[ $per_file_ret -ne 0 ]] && ret=$per_file_ret
273+
274+
if [[ $ret -ne 0 ]]; then
275+
# Step Summary — the readable report a PR author sees on the
276+
# Check's run page. Inline per-line markers are intentionally
277+
# left to the post workflow's reviewdog pass (which also
278+
# surfaces them on the Conversation tab); emitting `::error`
279+
# annotations here too would double up on the same lines.
280+
# The Summary is the fallback that still works when reviewdog
281+
# no-ops (cross-fork token, filter mismatch, reviewdog down).
282+
{
283+
echo "## Red-cross gate failed"
284+
echo
285+
echo "ShellCheck reported findings in PR-scope files. Details:"
286+
echo
287+
echo '```'
288+
cat "$gate_log"
289+
echo '```'
290+
} >> "$GITHUB_STEP_SUMMARY"
291+
fi
200292
201293
exit $ret

0 commit comments

Comments
 (0)