Skip to content

ci(actionlint): lint this repo's own workflows + clear the 29 pre-existing findings - #66

Merged
LukasWodka merged 3 commits into
developfrom
feat/actionlint-ci
Jul 26, 2026
Merged

ci(actionlint): lint this repo's own workflows + clear the 29 pre-existing findings#66
LukasWodka merged 3 commits into
developfrom
feat/actionlint-ci

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

This repo publishes 16 reusable workflows that every other repo in the org consumes at @main, and it had no CI of its own — nothing linted or validated a workflow file before it merged and went org-wide. This PR closes that gap and clears the backlog that closing it exposed.

Two commits, deliberately separate:

  1. fix(workflows) — clear all 29 actionlint findings across 8 pre-existing workflows.
  2. ci(actionlint) — add the gate.

Follows #65 (code-quality.yml), which added the org's first shared code quality workflow. That file is already clean and is not touched here. Part of epic tracebloc/backend#930.

actionlint findings: 29 → 0

RuleCountDisposition
SC201623Suppressed per-line (correct code)
SC21292Fixed by grouping redirects
SC20342One removed as dead, one suppressed with a reason
SC21881Fixed
SC20861Fixed (see the warning below)

Verified locally with actionlint 1.7.12 + shellcheck 0.11.0: zero findings across all 17 workflow files, and all 17 still parse as YAML.

⚠️ Behavioural change — one, and it fixes a latent break

advance-deploy-env.yml (SC2086) was the only finding whose naive fix would have broken production automation. The git log range was an unquoted $RANGE:

if [ "$BEFORE"="0000...0" ];then
RANGE="--max-count=50 $SHA"# <- TWO argv entrieselse
RANGE="$BEFORE..$SHA"# <- ONE argv entryfi
PRS=$(git log --format='%s'$RANGE ...)

The unquoted expansion was load-bearing: on the first push to a branch the range must reach git log as two separate arguments. Simply quoting it to "$RANGE" — the obvious "fix" — would have passed --max-count=50 <sha> as a single bogus argument, and PR discovery on that path would have failed silently. Rewritten as an array instead:

RANGE=(--max-count=50 "$SHA") # or: RANGE=("$BEFORE..$SHA")
PRS=$(git log --format='%s'"${RANGE[@]}" ...)

Verified against a real git repo with PR-style commit subjects: both push shapes produce identical PR lists before and after, and the naive-quote variant does indeed fail git log. Everything else in this PR is lint-only or comments.

Verdict on kanban-reconcile.yml's label — real bug or dead code?

Neither. It is load-bearing, and removing it would have introduced a bug.

while IFS=$'\t'read -r itemId optId reason label;do

label is field 4 of moves.tsv (the move category). It is never referenced in that step, which is why SC2034 fires — but it is not dead: read assigns the remainder of the line to its last variable. Drop label and $reason silently absorbs field 4, so every log line becomes [OK] backend#12 closed-not-merged (Code review)<TAB>cancelled. Confirmed by running both forms.

The field itself is consumed — by awk -F'\t' '{print $4}' in the Per-label summary step. So nothing is silently not happening: the reconciler's per-category summary works today.

Kept as-is, with a comment explaining the sink and a targeted # shellcheck disable=SC2034. No behavioural change.

Other fixes (all lint-only, verified equivalent)

  • wip-limit-check.yml (SC2034)REPO_NAME was assigned and never used, a copy-paste leftover from the workflows that do use it (fr-gate, advance-deploy-env, and others pass it as -F repo=). This one uses $REPO_FULL directly. Genuinely dead → removed.
  • wip-limit-check.yml (SC2188)> /tmp/items.txt is a redirection with no command. Not a broken line: it is a deliberate truncate before the append loop. Now : > /tmp/items.txt, which is both the portable form and the one already used in kanban-reconcile.yml. Byte-identical.
  • kanban-reconcile.yml + fr-pass-comment.yml (SC2129) — grouped consecutive >> "$GITHUB_OUTPUT" redirects into one { ... } >> block. Chose grouping over suppression: it is the cleaner code, and the appended bytes are identical (verified with cmp).

The 23 SC2016 suppressions

All are single-quoted GraphQL documents where $org / $num / $p / $i / $f / $o are GraphQL variables interpolated server-side by gh api graphql -F. Preventing shell expansion is precisely the point.

Each gets a # shellcheck disable=SC2016 on the line immediately above the specific command, with the reason inline. No file-wide directive, no global disable, and no .github/actionlint.yaml exclusion — a repo-wide exclusion would silently cover future code as well as the lines it was written for. Verified that the directives are line-scoped: an identical unsuppressed expression on the next line still reports.

The gate itself (.github/workflows/actionlint.yml)

  • Triggerpull_request on paths: ['.github/workflows/**'], plus workflow_dispatch for on-demand runs.
  • Hard gate from day one. Any finding fails the job. That is only affordable because commit 1 cleared the backlog, so the tree starts at zero and the job is green — it can be marked a required status check right away. (Contrast code-quality.yml, which ships soft-fail: true because it targets repos with unlinted backlogs.)
  • Supply chain — actionlint installed from its release tarball pinned by version and verified against a pinned SHA-256 (cross-checked against the upstream checksums.txt asset), rather than via a wrapper action that downloads it for us. actions/checkout is pinned to a full commit SHA (11d5960 = v4, the same pin feat(quality): shared code-quality reusable workflow + house-rules checker #65 uses).
  • concurrency — included; a re-push supersedes the previous run.
  • permissions: contents: read only.
  • Guards against silent degradation — a step asserts shellcheck is on the runner and passes -shellcheck shellcheck explicitly. actionlint skips every shell check and still exits 0 when the binary is missing; measured on a deliberately broken workflow, that is 3 findings vs 0. Without this guard the gate could quietly stop checking the thing this PR is mostly about.
  • Findings are annotated on the diff and written to the job summary.

Before requiring the check

The paths: filter means the job does not run on a PR touching no workflow file (a README-only change, say), and a required check that never runs leaves such a PR waiting for a status forever. Either drop the filter when you mark it required, or keep the filter and leave the check advisory. This is documented in the workflow header rather than decided here.

Test plan

  • actionlint (1.7.12 + shellcheck 0.11.0): 29 findings → 0, exit 0
  • All 17 workflow files parse as YAML
  • advance-deploy-env.yml range rewrite: identical PR extraction on both push shapes, in a real git repo
  • Naive "$RANGE" quote confirmed to break git log (proves the array was required)
  • Both SC2129 groupings produce byte-identical $GITHUB_OUTPUT content (cmp)
  • label removal confirmed to corrupt [OK]/[FAIL] lines (proves it is load-bearing)
  • Gate's own lint step executed verbatim: exit 0 on the clean tree; exit 1 with annotations + summary on a deliberately broken workflow
  • The new workflow passes its own linter (it caught an unused array in an early draft)

Reviewer notes

Behaviour of the kanban automation is unchanged except for the advance-deploy-env.yml array rewrite described above, which restores intended behaviour under quoting. No workflow's trigger, permissions, status transitions, or gate logic were altered. code-quality.yml from #65 is untouched.

🤖 Generated with Claude Code


Note

Medium Risk
Touches org-wide reusable deploy/kanban automation; the advance-deploy-env array change fixes first-push PR discovery but is the one path worth extra verification.

Overview
Adds a blockingactionlint job on every pull request so reusable workflows are validated before they ship at @main. The installer is version- and SHA-256–pinned, shellcheck presence is asserted (so shell checks cannot silently skip), and findings are annotated on the diff and in the job summary. The trigger deliberately has nopaths: filter so a required check cannot leave unrelated PRs stuck waiting.

Clears 29 pre-existing findings across eight workflows: mostly per-line SC2016 suppressions on GraphQL gh api snippets, grouped >> "$GITHUB_OUTPUT" writes (SC2129), removal of unused REPO_NAME in wip-limit-check.yml, : > for file truncate (SC2188), and a documented label sink in kanban-reconcile.yml’s read loop (SC2034).

The only behavioral workflow change is in advance-deploy-env.yml: the git log range is a bash array ("${RANGE[@]}") instead of an unquoted string, so the first-push path still passes --max-count=50 and the SHA as separate arguments—naive quoting would have broken PR discovery on that path.

Reviewed by Cursor Bugbot for commit c7bcdf6. Bugbot is set up for automated code reviews on this repo. Configure here.

LukasWodkaand others added 2 commits July 25, 2026 10:16
actionlint (with its native shellcheck pass) reported 29 findings across 8
of this repo's reusable workflows. None had ever been checked by CI. Fixed
the real ones, suppressed the correct-code ones at the line.
Real fixes:
- advance-deploy-env.yml (SC2086): the git log range was an unquoted
$RANGE. Rewritten as an array, NOT simply quoted -- on the first push to
a branch the range is two argv entries (--max-count=50 and the SHA), so
"$RANGE" would have handed git one bogus argument and broken PR
discovery on that path entirely. Verified both push shapes produce
identical PR lists before and after.
- wip-limit-check.yml (SC2034): REPO_NAME was assigned and never used --
a copy-paste leftover from the workflows that do use it. Removed.
- wip-limit-check.yml (SC2188): `> /tmp/items.txt` is a redirection with
no command. Now `: > /tmp/items.txt`, matching the form already used in
kanban-reconcile.yml. Byte-identical truncate.
- kanban-reconcile.yml + fr-pass-comment.yml (SC2129): consecutive
redirects to $GITHUB_OUTPUT grouped into a single `{ ... } >>` block.
Verified byte-identical output.
Suppressed as correct code (23x SC2016): single-quoted GraphQL documents
where $org / $num / $p / $i / $f / $o are GraphQL variables interpolated
server-side. Expansion is exactly what must NOT happen. Each gets a
per-line `# shellcheck disable=SC2016` with the reason -- no file-wide
directive and no actionlint config exclusion, so future code is not
silently covered.
kanban-reconcile.yml (SC2034, `label`): NOT dead code. It is the 4th
`read` variable and exists to stop `read` folding field 4 of moves.tsv
into $reason -- removing it would append "<TAB><category>" to every
[OK]/[FAIL] log line. Kept, documented, suppressed.
Behaviour is otherwise unchanged: the remaining diff is comments.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This repo publishes 16 reusable workflows that every other repo consumes
at `@main`, and it had no CI of its own -- nothing validated a workflow
file before it merged and went org-wide. This closes that gap.
actionlint parses each workflow, type-checks every ${{ }} expression,
validates the runs-on / uses / needs wiring, and runs shellcheck over
every `run:` block.
A hard gate from day one (any finding fails the job), which is only
affordable because the preceding commit cleared the backlog -- the tree
is at zero findings, so the job is green and can be marked a required
status check immediately.
Notes:
- actionlint is pinned by version AND verified against a pinned SHA-256
of the release tarball, rather than via a wrapper action that downloads
it for us. actions/checkout is pinned to a full commit SHA.
- A step asserts shellcheck is present: actionlint silently skips every
shell check when the binary is missing, which would leave the gate
green while checking far less.
- Includes a concurrency group so a re-push supersedes the previous run.
- Requests only `contents: read`.
- The `paths:` filter means the job does not run on PRs touching no
workflow file; the header documents what that implies before marking
the check required.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 8516ac9. Configure here.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Independent verification — and my brief was wrong on one point

I asked for the SC2086 finding to be fixed by "quoting it". That would have broken this workflow. Confirmed against origin/develop:

95: RANGE="--max-count=50 $SHA" # ← two argv entries when word-split
97: RANGE="$BEFORE..$SHA" # ← one
102: PRS=$(git log --format='%s' $RANGE \

On the first push to a branch (line 95), "$RANGE" would hand git log a single argument --max-count=50 <sha>silently breaking PR discovery on exactly that path, which is the path that matters for a brand-new branch. The array rewrite in this PR is the correct fix, and it was verified against both push shapes. Good catch; the naive fix was mine.

Likewise verified on kanban-reconcile.yml's SC2034 label:

289: while IFS=$'\t' read -r itemId optId reason label; do
324: awk -F'\t' '{print $4}' moves.tsv | ...
328: awk -F'\t' '{print "- "$3" -> `"$4"`"}' moves.tsv

read assigns the line remainder to its last variable, so dropping label would make $reason absorb field 4 and corrupt every log line — and field 4 is genuinely consumed downstream by both awk steps. SC2034 was superficially right and substantively wrong here; acting on it would have introduced the bug it appeared to describe. Keeping it with a documented suppression is the correct call.

One thing to settle before this is marked required

The workflow's paths: .github/workflows/** filter (which I specified) means the job does not run on a PR that touches no workflow file — a README-only change, for instance. A required status check that never runs leaves such a PR waiting for a status that will never arrive.

So it's one or the other:

  • drop the paths: filter and let it run on every PR (cheap here — the job is ~7s), then mark it required; or
  • keep the filter and leave the check advisory.

My preference is the former: at 7 seconds, the saved runner time isn't worth a class of stuck PR, and this repo's whole problem was having no gate at all. Flagged rather than decided, since it changes branch-protection setup.

This generalises to epic #930 — the "flip soft-fail: false, then mark required" step in the shared code-quality.yml rollout hits the same trap wherever a caller uses a paths: filter. Worth deciding the convention once, org-wide, rather than per repo.

Also worth keeping: the silent-degradation guard in this job. actionlint skips every shell check and still exits 0 when shellcheck is absent — measured at 3 findings vs 0 on a deliberately broken workflow. Since shellcheck produced nearly all of the 29 findings here, a job without that assertion would have looked green while checking almost nothing.

A required status check with a `paths:` filter never runs on a PR that
touches nothing matching it, and GitHub then waits forever for a status
that cannot arrive -- so the PR is stuck. The job takes ~7s, which is far
cheaper than that failure mode.
Raised on the PR as the one thing to settle before marking it required.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Pushed the paths: removal we discussed — this was the one thing standing between this job and being markable as required.

Rationale is now in the workflow header: a required check filtered by path never runs on a PR that touches nothing matching it, and GitHub then waits indefinitely for a status that cannot arrive. At ~7s per run, dropping the filter is far cheaper than that class of stuck PR.

The same principle applies to the shared code-quality.yml callers in #65 when those get flipped to required — worth keeping the convention consistent org-wide.

bugbot run

@LukasWodka
LukasWodka merged commit 151cc3f into developJul 26, 2026
2 checks passed
LukasWodka added a commit that referenced this pull request Jul 26, 2026
Picks up #66 (actionlint gate + the 29 shellcheck cleanups), #67 (closure
router), #69 (CODEOWNERS), and #71 (fr-gate fail-closed). Without these the
code-quality run here was linting the pre-#66 workflow copies and failing on
findings already fixed on develop.
@LukasWodka
LukasWodka deleted the feat/actionlint-ci branch August 1, 2026 21:45
Sign up for freeto 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

@LukasWodka