From ea759c154cf347cf6a870583757d97763cff1470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliv=C3=A9r=20Kecskem=C3=A9ty?= Date: Mon, 19 Oct 2026 23:12:18 +0200 Subject: [PATCH 1/2] Add a checker for the workflow convention --- scripts/workflow-style.mjs | 237 ++++++++++++++++++++++++ scripts/workflow-style.test.mjs | 308 ++++++++++++++++++++++++++++++++ 2 files changed, 545 insertions(+) create mode 100644 scripts/workflow-style.mjs create mode 100644 scripts/workflow-style.test.mjs diff --git a/scripts/workflow-style.mjs b/scripts/workflow-style.mjs new file mode 100644 index 0000000..226d296 --- /dev/null +++ b/scripts/workflow-style.mjs @@ -0,0 +1,237 @@ +// Checks a workflow or composite action against the workflow convention (docs/workflows.md). +// The convention keeps files to a small YAML subset (2-space block style, no anchors, no flow +// maps, flow lists of at most one item), so an indentation-based reader is enough; anything +// outside that subset is reported rather than parsed. + +const KEY = /^([A-Za-z_][\w.-]*):(?:\s+(.*))?$/; +const BLOCK_SCALAR = /^[|>][+-]?\d*\s*(#.*)?$/; +const TOP_LEVEL_ORDER = ["name", "run-name", "on", "permissions", "concurrency", "defaults", "env", "jobs"]; + +const withoutComment = (value) => value.replace(/\s+#.*$/, ""); + +function parse(text) { + const lines = text.split("\n"); + if (lines.at(-1) === "") lines.pop(); + const root = { line: 0, indent: -1, key: null, value: "", isItem: false, children: [], scalar: [] }; + const kinds = [null]; + const stack = [root]; + let scalarOwner = null; + let pendingBlanks = []; + + for (const [index, raw] of lines.entries()) { + const number = index + 1; + const trimmed = raw.trim(); + const indent = raw.length - raw.trimStart().length; + + if (scalarOwner) { + if (trimmed === "") { + kinds.push("blank"); + pendingBlanks.push(number); + continue; + } + if (indent > scalarOwner.indent) { + for (const blank of pendingBlanks) kinds[blank] = "scalar"; + pendingBlanks = []; + kinds.push("scalar"); + scalarOwner.scalar.push({ line: number, text: trimmed }); + continue; + } + scalarOwner = null; + } + pendingBlanks = []; + + if (trimmed === "") { + kinds.push("blank"); + continue; + } + if (trimmed.startsWith("#")) { + kinds.push("comment"); + continue; + } + kinds.push(trimmed === "---" ? "document" : "content"); + if (trimmed === "---") continue; + + const isItem = trimmed === "-" || trimmed.startsWith("- "); + while (stack.length > 1) { + const top = stack.at(-1); + // A sequence written at its parent key's indent still belongs to that key. + const ownsItem = isItem && !top.isItem && top.indent === indent && top.value === "" && top.children.length === 0; + if (top.indent < indent || ownsItem) break; + stack.pop(); + } + const parent = stack.at(-1); + + let node; + if (isItem) { + node = { line: number, indent, key: null, value: "", isItem: true, children: [], scalar: [] }; + parent.children.push(node); + stack.push(node); + const rest = trimmed.slice(1).trim(); + const match = KEY.exec(rest); + if (match) { + const child = keyNode(number, indent + 2, match); + node.children.push(child); + stack.push(child); + node = child; + } else { + node.value = rest; + } + } else { + const match = KEY.exec(trimmed); + node = match ? keyNode(number, indent, match) : { line: number, indent, key: null, value: trimmed, isItem: false, children: [], scalar: [] }; + parent.children.push(node); + stack.push(node); + } + if (node.block) scalarOwner = node; + } + + setEndLines(root); + return { root, kinds }; +} + +function keyNode(line, indent, match) { + const raw = match[2] ?? ""; + const block = BLOCK_SCALAR.test(raw); + return { line, indent, key: match[1], raw, value: block ? "" : withoutComment(raw).trim(), block, isItem: false, children: [], scalar: [] }; +} + +function setEndLines(node) { + let end = node.line; + for (const entry of node.scalar) end = Math.max(end, entry.line); + for (const child of node.children) end = Math.max(end, setEndLines(child)); + node.endLine = end; + return end; +} + +const child = (node, key) => node?.children.find((entry) => entry.key === key); +const keys = (node) => (node ? node.children.filter((entry) => entry.key !== null) : []); + +function* walk(node) { + for (const entry of node.children) { + yield entry; + yield* walk(entry); + } +} + +function blanksBetween(kinds, endLine, nextLine) { + let count = 0; + for (let line = endLine + 1; line < nextLine; line++) if (kinds[line] === "blank") count++; + return count; +} + +function blankAbove(kinds, line) { + let above = line - 1; + while (above > 0 && kinds[above] === "comment") above--; + return above > 0 && kinds[above] === "blank"; +} + +function hasTopLevelComma(list) { + let quote = null; + for (const char of list.slice(1, -1)) { + if (quote) { + if (char === quote) quote = null; + } else if (char === '"' || char === "'") { + quote = char; + } else if (char === ",") { + return true; + } + } + return false; +} + +function checkValues(root, report) { + for (const node of walk(root)) { + if (node.key === "<<") report(node.line, "anchors and aliases are not supported"); + const value = node.value; + if (value) { + if (value.startsWith("&") || value.startsWith("*")) report(node.line, "anchors and aliases are not supported"); + if (value.startsWith("{") && value !== "{}") report(node.line, "flow maps are not supported"); + if (value.startsWith("[") && hasTopLevelComma(value)) report(node.line, "flow lists hold at most one item"); + if (value.startsWith("'")) report(node.line, "use double quotes"); + } + if (node.key === "if" && value) { + if (value.includes("${{")) report(node.line, "if without ${{ }}"); + if (/\balways\(\)/.test(value)) report(node.line, "use !cancelled() instead of always()"); + } + if (node.key === "run") { + const message = "expression inside run; pass it through env"; + if (node.raw.includes("${{")) report(node.line, message); + for (const entry of node.scalar) if (entry.text.includes("${{")) report(entry.line, message); + } + } +} + +function checkSteps(steps, kinds, report, { allowShell }) { + const items = steps ? steps.children.filter((entry) => entry.isItem) : []; + items.forEach((item, index) => { + if (index > 0 && blanksBetween(kinds, items[index - 1].endLine, item.line) !== 1) report(item.line, "one empty line between steps"); + const stepKeys = keys(item); + if (!stepKeys.some((entry) => entry.key === "name")) report(item.line, "step has no name"); + else if (stepKeys[0].key !== "name") report(item.line, "name must be the first key of a step"); + const run = stepKeys.findIndex((entry) => entry.key === "run"); + if (run >= 0 && run < stepKeys.length - 1) report(stepKeys[run + 1].line, "run must be the last key of a step"); + const shell = child(item, "shell"); + if (shell && !allowShell) report(shell.line, "no per-step shell"); + }); + if (steps && blankAbove(kinds, steps.line)) report(steps.line, "no empty line before steps"); +} + +function checkTopLevel(root, kinds, report, { workflow }) { + const top = keys(root); + top.forEach((node, index) => { + if (index > 0 && blanksBetween(kinds, top[index - 1].endLine, node.line) !== 1) report(node.line, "one empty line between top-level keys"); + }); + if (!workflow) return; + let highest = -1; + for (const node of top) { + const rank = TOP_LEVEL_ORDER.indexOf(node.key); + if (rank < 0) continue; + if (rank < highest) report(node.line, `top-level key "${node.key}" out of order`); + highest = Math.max(highest, rank); + } +} + +function checkWorkflowRules(root, kinds, report) { + const permissions = child(root, "permissions"); + if (!permissions) report(1, "missing top-level permissions"); + else { + if (permissions.value === "write-all") report(permissions.line, "top-level permissions must be read or none"); + for (const scope of keys(permissions)) if (scope.value === "write") report(scope.line, "top-level permissions must be read or none"); + } + + const on = child(root, "on"); + if (on) { + if (on.value) report(on.line, "on must be a map"); + else if (child(on, "pull_request") && !child(root, "concurrency")) report(on.line, "missing concurrency for pull_request"); + } + + const jobs = keys(child(root, "jobs")); + const hasRun = jobs.some((job) => [...walk(job)].some((node) => node.key === "run")); + if (hasRun && child(child(child(root, "defaults"), "run"), "shell")?.value !== "bash") report(1, "missing defaults.run.shell: bash"); + + jobs.forEach((job, index) => { + if (index > 0 && blanksBetween(kinds, jobs[index - 1].endLine, job.line) !== 1) report(job.line, "one empty line between jobs"); + if (!child(job, "name")) report(job.line, `job "${job.key}" has no name`); + if (!child(job, "timeout-minutes") && !child(job, "uses")) report(job.line, `job "${job.key}" has no timeout-minutes`); + checkSteps(child(job, "steps"), kinds, report, { allowShell: false }); + }); +} + +export function checkWorkflow(text, path) { + const findings = []; + const report = (line, message) => findings.push({ path, line, message }); + const action = /(^|\/)action\.ya?ml$/.test(path); + + if (path.endsWith(".yaml")) report(1, "use the .yml extension"); + const { root, kinds } = parse(text); + kinds.forEach((kind, line) => { + if (kind === "document") report(line, "document start marker"); + }); + + checkValues(root, report); + checkTopLevel(root, kinds, report, { workflow: !action }); + if (action) checkSteps(child(child(root, "runs"), "steps"), kinds, report, { allowShell: true }); + else checkWorkflowRules(root, kinds, report); + + return findings.sort((a, b) => a.line - b.line || a.message.localeCompare(b.message)); +} diff --git a/scripts/workflow-style.test.mjs b/scripts/workflow-style.test.mjs new file mode 100644 index 0000000..595edad --- /dev/null +++ b/scripts/workflow-style.test.mjs @@ -0,0 +1,308 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; +import { checkWorkflow } from "./workflow-style.mjs"; + +const check = (text, path = ".github/workflows/ci.yml") => checkWorkflow(text, path).map(({ line, message }) => ({ line, message })); +const doc = (...parts) => `${parts.flat().join("\n")}\n`; + +// Lines 1–17; the first job starts on line 18. +const header = [ + "name: CI", + "", + "on:", + " pull_request:", + "", + "permissions:", + " contents: read", + "", + "concurrency:", + " group: ci", + " cancel-in-progress: true", + "", + "defaults:", + " run:", + " shell: bash", + "", + "jobs:", +]; +// Lines 18–22; the first step starts on line 23. +const job = [" test:", " name: Test", " runs-on: ubuntu-latest", " timeout-minutes: 10", " steps:"]; +const step = [" - name: Run", " run: echo hi"]; + +const conforming = doc( + "name: CI", + "", + "on:", + " pull_request:", + " push:", + " branches: [main]", + "", + "permissions:", + " contents: read", + "", + "concurrency:", + " group: ${{ github.workflow }}-${{ github.ref }}", + " cancel-in-progress: ${{ github.event_name == 'pull_request' }}", + "", + "defaults:", + " run:", + " shell: bash", + "", + "jobs:", + " test:", + " name: Test", + " runs-on: ubuntu-latest", + " timeout-minutes: 10", + " env:", + " NODE_ENV: test", + " steps:", + " - name: Check out", + " uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1", + " with:", + " persist-credentials: false", + "", + " # Runs the whole suite.", + " - name: Run tests", + ' if: "!cancelled()"', + " env:", + " REF: ${{ github.ref }}", + " run: |", + ' echo "$REF"', + "", + " node --test", + "", + " publish:", + " name: Publish", + " needs: test", + " if: github.event_name != 'pull_request'", + " runs-on: ubuntu-latest", + " timeout-minutes: 5", + " permissions:", + " packages: write", + " steps:", + " - name: Publish", + " run: echo done", +); + +test("conforming sample passes", () => { + assert.deepEqual(check(conforming), []); +}); + +test("findings carry the path", () => { + const [finding] = checkWorkflow(doc(header, job, step), ".github/workflows/ci.yaml"); + assert.equal(finding.path, ".github/workflows/ci.yaml"); +}); + +test("document start marker", () => { + assert.deepEqual(check(doc("---", header, job, step)), [{ line: 1, message: "document start marker" }]); +}); + +test("yml extension", () => { + assert.deepEqual(check(doc(header, job, step), ".github/workflows/ci.yaml"), [{ line: 1, message: "use the .yml extension" }]); +}); + +test("top-level key order", () => { + const text = doc(header.slice(0, 5), header.slice(8), job, step, "", "permissions:", " contents: read"); + assert.deepEqual(check(text), [{ line: 23, message: 'top-level key "permissions" out of order' }]); +}); + +test("missing permissions", () => { + const text = doc(header.slice(0, 5), header.slice(8), job, step); + assert.deepEqual(check(text), [{ line: 1, message: "missing top-level permissions" }]); +}); + +test("top-level write permissions", () => { + const scoped = doc(header.slice(0, 6), " contents: write", header.slice(7), job, step); + assert.deepEqual(check(scoped), [{ line: 7, message: "top-level permissions must be read or none" }]); + const all = doc(header.slice(0, 5), "permissions: write-all", header.slice(7), job, step); + assert.deepEqual(check(all), [{ line: 6, message: "top-level permissions must be read or none" }]); +}); + +test("concurrency required with pull_request", () => { + const text = doc(header.slice(0, 8), header.slice(12), job, step); + assert.deepEqual(check(text), [{ line: 3, message: "missing concurrency for pull_request" }]); + const pushOnly = doc("name: CI", "", "on:", " push:", header.slice(4, 8), header.slice(12), job, step); + assert.deepEqual(check(pushOnly), []); +}); + +test("bash shell required with run steps", () => { + const withoutShell = doc(header.slice(0, 12), header.slice(16), job, step); + assert.deepEqual(check(withoutShell), [{ line: 1, message: "missing defaults.run.shell: bash" }]); + const usesOnly = doc(header.slice(0, 12), header.slice(16), job, " - name: Check out", " uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1"); + assert.deepEqual(check(usesOnly), []); +}); + +test("on must be a map", () => { + const scalar = doc(header.slice(0, 2), "on: push", header.slice(4, 8), header.slice(12), job, step); + assert.deepEqual(check(scalar), [{ line: 3, message: "on must be a map" }]); + const list = doc(header.slice(0, 2), "on: [push, pull_request]", header.slice(4), job, step); + assert.deepEqual(check(list), [ + { line: 3, message: "flow lists hold at most one item" }, + { line: 3, message: "on must be a map" }, + ]); +}); + +test("job name and timeout", () => { + const text = doc(header, " test:", " runs-on: ubuntu-latest", " steps:", step); + assert.deepEqual(check(text), [ + { line: 18, message: 'job "test" has no name' }, + { line: 18, message: 'job "test" has no timeout-minutes' }, + ]); +}); + +test("step keys", () => { + const text = doc( + header, + job, + " - run: echo a", + "", + " - id: x", + " name: Second", + " run: echo b", + "", + " - name: Third", + " run: echo c", + " env:", + " A: b", + "", + " - name: Fourth", + " shell: bash", + " run: echo d", + ); + assert.deepEqual(check(text), [ + { line: 23, message: "step has no name" }, + { line: 25, message: "name must be the first key of a step" }, + { line: 31, message: "run must be the last key of a step" }, + { line: 35, message: "no per-step shell" }, + ]); +}); + +test("if conditions", () => { + const text = doc( + header, + job, + " - name: A", + " if: ${{ github.ref == 'refs/heads/main' }}", + " run: echo a", + "", + " - name: B", + " if: always()", + " run: echo b", + "", + " - name: C", + ' if: "!cancelled()"', + " run: echo c", + ); + assert.deepEqual(check(text), [ + { line: 24, message: "if without ${{ }}" }, + { line: 28, message: "use !cancelled() instead of always()" }, + ]); +}); + +test("expressions inside run", () => { + const text = doc(header, job, " - name: A", " run: echo ${{ github.ref }}", "", " - name: B", " run: |", " echo start", " echo ${{ github.sha }}"); + const message = "expression inside run; pass it through env"; + assert.deepEqual(check(text), [ + { line: 24, message }, + { line: 29, message }, + ]); +}); + +test("spacing between steps", () => { + const none = doc(header, job, " - name: A", " run: echo a", " - name: B", " run: echo b"); + assert.deepEqual(check(none), [{ line: 25, message: "one empty line between steps" }]); + const two = doc(header, job, " - name: A", " run: echo a", "", "", " - name: B", " run: echo b"); + assert.deepEqual(check(two), [{ line: 27, message: "one empty line between steps" }]); +}); + +test("no empty line before steps", () => { + const text = doc(header, job.slice(0, 4), "", " steps:", step); + assert.deepEqual(check(text), [{ line: 23, message: "no empty line before steps" }]); +}); + +test("spacing between jobs and top-level keys", () => { + const text = doc(header.slice(0, 4), header.slice(5), job, step, " lint:", " name: Lint", " runs-on: ubuntu-latest", " timeout-minutes: 10", " steps:", step); + assert.deepEqual(check(text), [ + { line: 5, message: "one empty line between top-level keys" }, + { line: 24, message: "one empty line between jobs" }, + ]); +}); + +test("double quotes", () => { + assert.deepEqual(check(doc(header, job, " - name: 'Run'", " run: echo hi")), [{ line: 23, message: "use double quotes" }]); +}); + +test("unsupported constructs", () => { + const anchors = doc(header, job, " - name: A", " env: &common", " A: b", " run: echo a", "", " - name: B", " env: *common", " run: echo b"); + assert.deepEqual(check(anchors), [ + { line: 24, message: "anchors and aliases are not supported" }, + { line: 29, message: "anchors and aliases are not supported" }, + ]); + const flowMap = doc(header, job, " - name: A", " uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1", " with: { fetch-depth: 0 }"); + assert.deepEqual(check(flowMap), [{ line: 25, message: "flow maps are not supported" }]); + const flowList = doc(header.slice(0, 4), " push:", " branches: [main, dev]", header.slice(4), job, step); + assert.deepEqual(check(flowList), [{ line: 6, message: "flow lists hold at most one item" }]); +}); + +test("CRLF input gives the same findings as LF", () => { + const lf = doc(header, job, " - run: echo a", " - name: B", " shell: bash", " run: echo ${{ github.ref }}"); + const crlf = lf.replaceAll("\n", "\r\n"); + assert.notDeepEqual(check(lf), []); + assert.deepEqual(check(crlf), check(lf)); + assert.deepEqual(check(conforming.replaceAll("\n", "\r\n")), []); +}); + +test("block scalars are opaque except for expressions", () => { + const text = doc( + header, + job, + " - name: A", + " run: |", + " cat < x.yml", + " key: value", + "", + " - item", + " echo ${{ github.ref }}", + " EOF", + "", + " - name: B", + " run: echo b", + ); + assert.deepEqual(check(text), [{ line: 29, message: "expression inside run; pass it through env" }]); +}); + +test("comments do not affect spacing or order", () => { + const text = doc( + header, + job, + " - name: A", + " uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1", + "", + " # Second step.", + " - name: B", + " run: echo b", + ); + assert.deepEqual(check(text), []); +}); + +test("reusable-workflow jobs and composite actions are exempt where GitHub requires it", () => { + const caller = doc(header, " checks:", " name: Checks", " uses: BSStudio/.github/.github/workflows/shared-checks.yml@3d3c42e5aac5ba805825da76410c181273ba90b1 # v1.0.0", " with:", " dependency-review: true"); + assert.deepEqual(check(caller), []); + const action = doc( + "name: Setup", + "", + "description: Install dependencies", + "", + "runs:", + " using: composite", + " steps:", + " - name: Install", + " shell: bash", + " run: pnpm install", + "", + " - name: Build", + " shell: bash", + " run: pnpm build", + ); + assert.deepEqual(check(action, ".github/actions/setup/action.yml"), []); +}); From a45bf05bc1aadace3794e74f00ea6ed35b98bdee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliv=C3=A9r=20Kecskem=C3=A9ty?= Date: Mon, 19 Oct 2026 23:43:36 +0200 Subject: [PATCH 2/2] Add the shared workflow checks and the written convention --- .github/actionlint.yaml | 5 ++ .github/actions/workflow-checks/action.yml | 40 ++++++++++ .github/actions/workflow-checks/zizmor.yml | 9 +++ .github/renovate.json | 10 +++ .github/workflows/checks.yml | 18 +++++ .github/workflows/shared-checks.yml | 47 ++++++++++++ .github/workflows/test.yml | 14 +++- README.md | 6 ++ docs/workflows.md | 87 ++++++++++++++++++++++ scripts/check-workflows.mjs | 26 +++++++ scripts/check-workflows.test.mjs | 57 ++++++++++++++ scripts/workflow-style.mjs | 17 +++-- scripts/workflow-style.test.mjs | 24 ++++++ 13 files changed, 350 insertions(+), 10 deletions(-) create mode 100644 .github/actionlint.yaml create mode 100644 .github/actions/workflow-checks/action.yml create mode 100644 .github/actions/workflow-checks/zizmor.yml create mode 100644 .github/workflows/checks.yml create mode 100644 .github/workflows/shared-checks.yml create mode 100644 docs/workflows.md create mode 100644 scripts/check-workflows.mjs create mode 100644 scripts/check-workflows.test.mjs diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml new file mode 100644 index 0000000..2d2b735 --- /dev/null +++ b/.github/actionlint.yaml @@ -0,0 +1,5 @@ +paths: + .github/workflows/shared-checks.yml: + ignore: + # Callers in other repositories must reach this repository's action at the commit they pinned, which only the $/ self-repository syntax does; actionlint predates it. + - specifying action "\$/ diff --git a/.github/actions/workflow-checks/action.yml b/.github/actions/workflow-checks/action.yml new file mode 100644 index 0000000..30810fa --- /dev/null +++ b/.github/actions/workflow-checks/action.yml @@ -0,0 +1,40 @@ +name: Workflow checks + +description: Runs actionlint, zizmor and the workflow convention checker over the repository's workflows and composite actions. + +inputs: + files: + description: Files for the convention checker, as shell patterns; patterns that match nothing are skipped. + default: .github/workflows/*.yml .github/workflows/*.yaml .github/actions/*/action.yml .github/actions/*/action.yaml + + github-token: + description: Token for zizmor's online audits; read access to the repository is enough. + required: true + +runs: + using: composite + steps: + - name: Run actionlint + shell: bash + env: + # renovate: datasource=docker depName=rhysd/actionlint + ACTIONLINT_IMAGE: rhysd/actionlint:1.7.12@sha256:b1934ee5f1c509618f2508e6eb47ee0d3520686341fec936f3b79331f9315667 + run: docker run --rm --volume "$GITHUB_WORKSPACE:/repo:ro" --workdir /repo "$ACTIONLINT_IMAGE" -color + + - name: Run zizmor + if: "!cancelled()" + shell: bash + env: + GH_TOKEN: ${{ inputs.github-token }} + # renovate: datasource=docker depName=ghcr.io/zizmorcore/zizmor + ZIZMOR_IMAGE: ghcr.io/zizmorcore/zizmor:1.31.1@sha256:42d409dbb851d4c5d35a058cf042b828bea1e3f2884378c526eb0974780e18d9 + run: docker run --rm --env GH_TOKEN --volume "$GITHUB_WORKSPACE:/repo:ro" --volume "$GITHUB_ACTION_PATH:/config:ro" --workdir /repo "$ZIZMOR_IMAGE" --config /config/zizmor.yml --format github . + + - name: Check the workflow convention + if: "!cancelled()" + shell: bash + env: + FILES: ${{ inputs.files }} + run: | + shopt -s nullglob + node "$GITHUB_ACTION_PATH/../../../scripts/check-workflows.mjs" $FILES diff --git a/.github/actions/workflow-checks/zizmor.yml b/.github/actions/workflow-checks/zizmor.yml new file mode 100644 index 0000000..fc36bf9 --- /dev/null +++ b/.github/actions/workflow-checks/zizmor.yml @@ -0,0 +1,9 @@ +rules: + unpinned-uses: + config: + policies: + "*": hash-pin + + # Local actions stay on ./ until actionlint understands the $/ self-repository syntax. + self-repository: + disable: true diff --git a/.github/renovate.json b/.github/renovate.json index 5642241..771a3f7 100644 --- a/.github/renovate.json +++ b/.github/renovate.json @@ -3,5 +3,15 @@ "extends": [ "github>BSStudio/renovate-config", "github>BSStudio/renovate-config:monthly" + ], + "customManagers": [ + { + "customType": "regex", + "description": "Container images that composite actions run with docker run stay pinned by tag and digest; this keeps both current.", + "managerFilePatterns": ["/^\\.github/actions/.+/action\\.yml$/"], + "matchStrings": [ + "# renovate: datasource=(?\\S+) depName=(?\\S+)\\s+\\w+: \\S+?:(?[^@\\s]+)@(?sha256:[a-f0-9]{64})" + ] + } ] } diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml new file mode 100644 index 0000000..8d574a3 --- /dev/null +++ b/.github/workflows/checks.yml @@ -0,0 +1,18 @@ +name: Checks + +on: + pull_request: + push: + branches: [main] + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + checks: + name: Checks + uses: ./.github/workflows/shared-checks.yml diff --git a/.github/workflows/shared-checks.yml b/.github/workflows/shared-checks.yml new file mode 100644 index 0000000..d323f3f --- /dev/null +++ b/.github/workflows/shared-checks.yml @@ -0,0 +1,47 @@ +name: Shared checks + +on: + workflow_call: + inputs: + dependency-review: + description: Review dependency changes for licences and known vulnerabilities. Public repositories only. + type: boolean + default: false + +permissions: + contents: read + +jobs: + workflows: + name: Workflows + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - name: Check out + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Check workflows + uses: $/.github/actions/workflow-checks + with: + github-token: ${{ github.token }} + + dependencies: + name: Dependencies + if: inputs.dependency-review && github.event_name == 'pull_request' + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - name: Review dependency changes + uses: actions/dependency-review-action@a1d282b36b6f3519aa1f3fc636f609c47dddb294 # v5.0.0 + with: + # Every dependency's licence may be combined with GPL-3.0 code that is distributed (public Docker images). + # Missing on purpose, so they fail and need a decision: AGPL-3.0, proprietary and source-available + # licences, non-commercial ones, and GPL-incompatible copyleft (EPL, CDDL, GPL-2.0-only). + allow-licenses: >- + MIT, MIT-0, Apache-2.0, BSD-2-Clause, BSD-3-Clause, ISC, 0BSD, Zlib, BSL-1.0, Unlicense, CC0-1.0, + WTFPL, BlueOak-1.0.0, Python-2.0, PSF-2.0, Artistic-2.0, Unicode-3.0, Unicode-DFS-2016, + MPL-2.0, LGPL-2.1-only, LGPL-2.1-or-later, LGPL-3.0-only, LGPL-3.0-or-later, + GPL-2.0-or-later, GPL-3.0-only, GPL-3.0-or-later, + CC-BY-3.0, CC-BY-4.0, OFL-1.1 diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6aaa8f1..5f43db9 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -8,19 +8,29 @@ on: permissions: contents: read +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +defaults: + run: + shell: bash + jobs: test: name: Test scripts runs-on: ubuntu-latest timeout-minutes: 5 steps: - - name: Checkout + - name: Check out uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false + - name: Set up Node.js uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 with: node-version: 24 + - name: Run tests - run: node --test 'scripts/*.test.mjs' + run: node --test "scripts/*.test.mjs" diff --git a/README.md b/README.md index f57330d..872e1a0 100644 --- a/README.md +++ b/README.md @@ -34,3 +34,9 @@ node scripts/apply-settings.mjs --dry-run BSStudio/backstage # one repo A named repo must be listed in `settings/repos.json`. A run writes only what differs, then reads the repo again and reports anything GitHub did not take. Run it as an admin of the listed repos: without admin rights GitHub leaves some settings out of its answers (they show as `missing`) and refuses the writes. A failed call is reported with the `gh` command that failed. Security features (secret scanning, push protection, Dependabot alerts, CodeQL default setup, private vulnerability reporting) come from the organisation's "GitHub recommended" code security configuration, which is the default for new public repos. The script checks that it is attached and never attaches it. + +## Workflows + +Every workflow follows one convention, written down in [`docs/workflows.md`](docs/workflows.md). Each repo enforces it with a `.github/workflows/checks.yml` that calls `.github/workflows/shared-checks.yml` here (the copyable caller is in the convention): `Checks / Workflows` runs actionlint, zizmor and `scripts/check-workflows.mjs` over the repo's workflows and composite actions, and `Checks / Dependencies` reviews new dependencies for vulnerabilities and licences in repos that turn it on. Wherever a ruleset exists it requires `Checks / Workflows`, plus `Checks / Dependencies` where the review is on. + +Callers pin a tagged release of this repo; Renovate proposes each new release. The checks run from the same commit as the workflow that calls them: `shared-checks.yml` reaches its composite action through the `$/` self-repository syntax. diff --git a/docs/workflows.md b/docs/workflows.md new file mode 100644 index 0000000..e28b496 --- /dev/null +++ b/docs/workflows.md @@ -0,0 +1,87 @@ +# Workflow convention + +Every GitHub Actions workflow and composite action in the studio's repositories follows these rules, so a workflow in one repository reads like one in any other and the same mistakes are caught everywhere. The `Checks / Workflows` check enforces them on every pull request: actionlint for syntax and shell scripts, zizmor for security, and `scripts/check-workflows.mjs` for the rest. The few rules that need judgement are kept in review and marked as such. + +## Files + +- Workflows end in `.yml` and start without a `---` document marker. +- Top-level keys come in this order: `name`, `on`, `permissions`, `concurrency`, `defaults`, `env`, `jobs`. +- `on` is a map. A flow list holds at most one item (`branches: [main]`); two or more items go in a block list. +- The files stay inside the plain subset of YAML the checker reads: block style with 2-space indentation, no anchors or aliases, no flow maps. + +## Permissions and secrets + +- A workflow grants no more than reading by default: `permissions` is always set at the top, normally to `contents: read`. Write scopes go on the one job that needs them. +- A job that runs on pull requests holds a write scope only if every step that uses it is skipped on pull requests. Today this covers the single build-and-push jobs of the publish workflows, until they move to a shared publish workflow that splits building from pushing. +- Secrets are passed only on events other than `pull_request` (review). +- Checkouts set `persist-credentials: false`; a job that pushes keeps the credentials and says why in a comment. +- Every action is pinned by its full commit SHA with the release in a comment, `@ # vX.Y.Z`. Renovate keeps the pins current. + +## Concurrency and shell + +- A new push to a pull request cancels the run it replaces: every workflow that runs on `pull_request` sets `concurrency` with `group: ${{ github.workflow }}-${{ github.ref }}` and `cancel-in-progress: ${{ github.event_name == 'pull_request' }}`. +- A failing command fails its step, including inside a pipe: every workflow with a `run` step sets `defaults.run.shell: bash`. Naming bash explicitly turns on `pipefail`; the implicit default shell does not. +- `working-directory` lives in `defaults.run`: at workflow level when every job shares it, otherwise per job. A step sets its own only when it alone differs. Action inputs such as `with: working-directory` are not affected. + +## Jobs + +- Every job has a `name` and a `timeout-minutes`, about two to three times its usual duration. A job that calls a reusable workflow cannot set a timeout and is exempt. +- A job that is a required status check keeps its name, because renaming it leaves pull requests waiting for a check that never reports (review). +- A value several steps use goes in the job's `env` once. + +## Steps + +- Every step has a sentence-case `name` as its first key, and `run` comes last. +- A step runs one command that lives in the repository: a package script, `uv run …`, or a script file. Shell longer than a few lines moves into a script, so local runs and CI run the same thing and shellcheck sees it (review). +- `if` conditions are written without `${{ }}`, and use `!cancelled()` instead of `always()`. +- No `${{ }}` inside `run`: values reach the shell through `env`, so nothing from the event payload is spliced into a script. + +## Spacing and quoting + +- Exactly one empty line separates top-level keys, jobs and steps; there is none right before `steps`. +- Strings are quoted only where YAML requires it, with double quotes. + +## Composite actions + +The same rules apply to `action.yml` files, except that GitHub requires `shell` on every `run` step there. + +## Calling the shared checks + +Every repository runs the checks through the reusable workflow in this repository, from a `.github/workflows/checks.yml` like this: + +```yaml +name: Checks + +on: + pull_request: + push: + branches: [main] + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + checks: + name: Checks + uses: BSStudio/.github/.github/workflows/shared-checks.yml@ # vX.Y.Z + with: + dependency-review: true +``` + +- The job must be named `Checks`: the checks then report as `Checks / Workflows` and `Checks / Dependencies`, the names the repository's ruleset requires. +- `dependency-review: true` adds the dependency review for repositories with package manifests. It works on public repositories only and checks every new or changed dependency for known vulnerabilities and for a licence outside the allow-list in `shared-checks.yml`. Without the input the `Dependencies` job is skipped, which still satisfies a required check. +- The pin follows this repository's releases: Renovate proposes each new release in every caller, and that pull request runs the new checks against the repository before anything merges. + +## Running the checks locally + +From a repository's root, with actionlint and zizmor installed and this repository cloned next to it: + +```bash +actionlint +GH_TOKEN=$(gh auth token) zizmor --config ../.github/.github/actions/workflow-checks/zizmor.yml . +node ../.github/scripts/check-workflows.mjs .github/workflows/*.yml .github/actions/*/action.yml +``` diff --git a/scripts/check-workflows.mjs b/scripts/check-workflows.mjs new file mode 100644 index 0000000..6f02b14 --- /dev/null +++ b/scripts/check-workflows.mjs @@ -0,0 +1,26 @@ +import { readFileSync } from "node:fs"; +import { checkWorkflow } from "./workflow-style.mjs"; + +const files = process.argv.slice(2); +if (files.length === 0) { + console.error("usage: node scripts/check-workflows.mjs ..."); + process.exit(2); +} + +const annotate = process.env.GITHUB_ACTIONS === "true"; +let status = 0; +for (const file of files) { + let text; + try { + text = readFileSync(file, "utf8"); + } catch (error) { + console.error(`${file}: ${error.message}`); + status = 2; + continue; + } + for (const { path, line, message } of checkWorkflow(text, file)) { + console.log(annotate ? `::error file=${path},line=${line}::${message}` : `${path}:${line}: ${message}`); + status ||= 1; + } +} +process.exitCode = status; diff --git a/scripts/check-workflows.test.mjs b/scripts/check-workflows.test.mjs new file mode 100644 index 0000000..020e846 --- /dev/null +++ b/scripts/check-workflows.test.mjs @@ -0,0 +1,57 @@ +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { after, test } from "node:test"; +import { fileURLToPath } from "node:url"; + +const cli = fileURLToPath(new URL("./check-workflows.mjs", import.meta.url)); +const dir = mkdtempSync(join(tmpdir(), "check-workflows-")); +after(() => rmSync(dir, { recursive: true, force: true })); +const write = (name, text) => { + const file = join(dir, name); + writeFileSync(file, text); + return file; +}; +const run = (files, env = {}) => { + const { GITHUB_ACTIONS, ...rest } = process.env; + return spawnSync(process.execPath, [cli, ...files], { encoding: "utf8", env: { ...rest, ...env } }); +}; + +const clean = write( + "clean.yml", + ["name: CI", "", "on:", " push:", "", "permissions:", " contents: read", "", "jobs:", " test:", " name: Test", " runs-on: ubuntu-latest", " timeout-minutes: 5", " steps:", " - name: Check out", " uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1", ""].join("\n"), +); +const broken = write("broken.yml", "---\nname: CI\n"); + +test("a clean file prints nothing and exits 0", () => { + const result = run([clean]); + assert.equal(result.stdout, ""); + assert.equal(result.status, 0); +}); + +test("findings print as path:line: message and exit 1", () => { + const result = run([broken]); + assert.equal(result.status, 1); + assert.match(result.stdout, new RegExp(`^${broken.replaceAll("\\", "\\\\")}:1: document start marker$`, "m")); +}); + +test("inside GitHub Actions findings become error annotations", () => { + const result = run([broken], { GITHUB_ACTIONS: "true" }); + assert.equal(result.status, 1); + assert.match(result.stdout, /^::error file=.*broken\.yml,line=1::document start marker$/m); +}); + +test("an unreadable file exits 2 after checking the rest", () => { + const result = run([join(dir, "missing.yml"), broken]); + assert.equal(result.status, 2); + assert.match(result.stderr, /missing\.yml/); + assert.match(result.stdout, /document start marker/); +}); + +test("no files is a usage error", () => { + const result = run([]); + assert.equal(result.status, 2); + assert.match(result.stderr, /^usage: /); +}); diff --git a/scripts/workflow-style.mjs b/scripts/workflow-style.mjs index 226d296..0e995df 100644 --- a/scripts/workflow-style.mjs +++ b/scripts/workflow-style.mjs @@ -3,7 +3,7 @@ // maps, flow lists of at most one item), so an indentation-based reader is enough; anything // outside that subset is reported rather than parsed. -const KEY = /^([A-Za-z_][\w.-]*):(?:\s+(.*))?$/; +const KEY = /^(?:"([^"]+)"|([A-Za-z_][\w.-]*)):(?:\s+(.*))?$/; const BLOCK_SCALAR = /^[|>][+-]?\d*\s*(#.*)?$/; const TOP_LEVEL_ORDER = ["name", "run-name", "on", "permissions", "concurrency", "defaults", "env", "jobs"]; @@ -48,14 +48,15 @@ function parse(text) { kinds.push("comment"); continue; } - kinds.push(trimmed === "---" ? "document" : "content"); - if (trimmed === "---") continue; + const isDocument = /^---(\s+#.*)?$/.test(trimmed); + kinds.push(isDocument ? "document" : "content"); + if (isDocument) continue; const isItem = trimmed === "-" || trimmed.startsWith("- "); while (stack.length > 1) { const top = stack.at(-1); // A sequence written at its parent key's indent still belongs to that key. - const ownsItem = isItem && !top.isItem && top.indent === indent && top.value === "" && top.children.length === 0; + const ownsItem = isItem && !top.isItem && top.indent === indent && top.value === "" && top.children.every((entry) => entry.isItem); if (top.indent < indent || ownsItem) break; stack.pop(); } @@ -90,9 +91,9 @@ function parse(text) { } function keyNode(line, indent, match) { - const raw = match[2] ?? ""; + const raw = match[3] ?? ""; const block = BLOCK_SCALAR.test(raw); - return { line, indent, key: match[1], raw, value: block ? "" : withoutComment(raw).trim(), block, isItem: false, children: [], scalar: [] }; + return { line, indent, key: match[1] ?? match[2], raw, value: block ? "" : withoutComment(raw).trim(), block, isItem: false, children: [], scalar: [] }; } function setEndLines(node) { @@ -206,7 +207,7 @@ function checkWorkflowRules(root, kinds, report) { } const jobs = keys(child(root, "jobs")); - const hasRun = jobs.some((job) => [...walk(job)].some((node) => node.key === "run")); + const hasRun = jobs.some((job) => (child(job, "steps")?.children ?? []).some((step) => child(step, "run"))); if (hasRun && child(child(child(root, "defaults"), "run"), "shell")?.value !== "bash") report(1, "missing defaults.run.shell: bash"); jobs.forEach((job, index) => { @@ -220,7 +221,7 @@ function checkWorkflowRules(root, kinds, report) { export function checkWorkflow(text, path) { const findings = []; const report = (line, message) => findings.push({ path, line, message }); - const action = /(^|\/)action\.ya?ml$/.test(path); + const action = /(^|[\\/])action\.ya?ml$/.test(path); if (path.endsWith(".yaml")) report(1, "use the .yml extension"); const { root, kinds } = parse(text); diff --git a/scripts/workflow-style.test.mjs b/scripts/workflow-style.test.mjs index 595edad..e9cd7f8 100644 --- a/scripts/workflow-style.test.mjs +++ b/scripts/workflow-style.test.mjs @@ -244,6 +244,30 @@ test("unsupported constructs", () => { assert.deepEqual(check(flowList), [{ line: 6, message: "flow lists hold at most one item" }]); }); +test("Windows paths still select composite-action mode", () => { + const action = doc("name: Setup", "", "runs:", " using: composite", " steps:", " - name: Install", " shell: bash", " run: pnpm install"); + assert.deepEqual(check(action, ".github\\actions\\setup\\action.yml"), []); +}); + +test("unindented sequences keep every item", () => { + const text = doc(header, job.slice(0, 4), " steps:", " - name: A", " run: echo a", "", " - run: echo b"); + assert.deepEqual(check(text), [{ line: 26, message: "step has no name" }]); +}); + +test("a double-quoted on key is still the on key", () => { + const text = doc(header.slice(0, 2), '"on":', header.slice(3), job, step); + assert.deepEqual(check(text), []); +}); + +test("a document marker with a comment is still a document marker", () => { + assert.deepEqual(check(doc("--- # yaml-language-server: $schema=x", header, job, step)), [{ line: 1, message: "document start marker" }]); +}); + +test("job-level defaults.run is not a run step", () => { + const text = doc(header.slice(0, 12), header.slice(16), " test:", " name: Test", " runs-on: ubuntu-latest", " timeout-minutes: 10", " defaults:", " run:", " working-directory: backend", " steps:", " - name: Check out", " uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1"); + assert.deepEqual(check(text), []); +}); + test("CRLF input gives the same findings as LF", () => { const lf = doc(header, job, " - run: echo a", " - name: B", " shell: bash", " run: echo ${{ github.ref }}"); const crlf = lf.replaceAll("\n", "\r\n");