From 3b3c696654f2aa8397f9c18f7b4b698f33b5c8f6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 13:38:45 +0000 Subject: [PATCH] chore(ci): gate job-level `if:` reads of needs.*.outputs.* on naming a status function (#5343) GitHub wraps any `if:` that names no status function in an IMPLICIT success(). So a job-level condition written to read an upstream job's OUTPUT VALUE silently also carries a status decision nobody wrote, and the two completely different facts "the upstream job DIED" and "the upstream job said do not run" arrive as the same skip -- which every checks list renders as green. This repo has paid for that twice by hand, months apart: #4900's release integrity guard stopped guarding exactly when the job before it failed, and #4928 found seven ci.yml gates that turned themselves off whenever something upstream broke. The rule is statically decidable, so it is a gate now rather than a third manual reading. scripts/check-workflow-status-functions.mjs (`pnpm check:workflow-status-functions`, wired into lint.yml's ESLint job) holds every job-level `if:` that reads `needs.*.outputs.*` to naming one of always() / !cancelled() / success() / failure(). Step-level conditions and `needs.*.result` are deliberately out of scope: the implicit success() in a step is about that job's own earlier steps and is usually what the author wants, and a `.result` read is already a status expression. That boundary is what keeps the rule free of intent-guessing and of an exemption list -- there is no skip-list. The parse is a real YAML parse rather than a grep over `if:` lines, and that is load-bearing: a grep cannot tell a malformed workflow from a clean one (both are zero matches) and cannot see a folded expression at all, which two workflows here already use. Missing input is a failure, never a pass (#4690): absent directory, zero workflow files, parse error, no `jobs:` map and a non-scalar `if:` all exit non-zero. `--self-test` runs 34 assertions through the real scan() path over temp fixture roots; `--list` prints the audit table #5343 had to build by hand. publish-smoke.yml carried the one existing violation. pack-smoke's behaviour is unchanged -- smoke only a resolve that SUCCEEDED and said run -- but success() is now written out, and a new resolve-guard job goes red when resolve fails, then reports that failure as a commit status on the release PR head. #4928's "when in doubt, run everything" shape is deliberately NOT copied here: ci.yml's filter defaults its outputs to run (`|| 'true'`), resolve does not and also computes the ref, so running on doubt would check out an empty ref and spend 45 minutes reporting a verdict about the wrong thing. Measured while fixing it: `publish-smoke / packed-tarballs` is NOT a required check on main (branch protection requires TypeScript Type Check, Build Core, Test Core, Dogfood Regression Gate), so the status resolve failure never writes blocks nothing and is simply invisible on the release PR. That is the hole resolve-guard fills. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GX3sL71LFq8m2usg6VqTSE --- .changeset/workflow-status-function-gate.md | 20 + .github/workflows/lint.yml | 21 + .github/workflows/publish-smoke.yml | 66 ++- package.json | 4 +- pnpm-lock.yaml | 3 + scripts/check-workflow-status-functions.mjs | 609 ++++++++++++++++++++ 6 files changed, 721 insertions(+), 2 deletions(-) create mode 100644 .changeset/workflow-status-function-gate.md create mode 100644 scripts/check-workflow-status-functions.mjs diff --git a/.changeset/workflow-status-function-gate.md b/.changeset/workflow-status-function-gate.md new file mode 100644 index 0000000000..664abb286d --- /dev/null +++ b/.changeset/workflow-status-function-gate.md @@ -0,0 +1,20 @@ +--- +--- + +chore(ci): job 级 `if:` 读 `needs.*.outputs.*` 必须显式点名状态函数,并修掉 publish-smoke 的存量违规 (#5343) + +GitHub 会给任何**不含状态函数**的 `if:` 隐式包一层 `success()`。于是一个本意是「读上游输出值做数据驱动决策」的 job 级条件,悄悄同时携带了一个作者从未写下的状态决策,而「上游挂了」和「上游说不用跑」这两件完全不同的事,到达时是同一个 skipped —— 在任何 checks 列表里都渲染成绿。 + +这个坑本仓已经手工踩过两次,相隔数月:#4900 的发布完整性守卫恰好在它前面那个 job 失败时停止守卫;#4928 在 `ci.yml` 里找到七处 `needs.filter.outputs.*` 闸门,一旦上游出事就自己把自己关掉。规则可静态判定,所以本次把它变成门禁而不是第三次靠人读出来。 + +**新门禁 `scripts/check-workflow-status-functions.mjs`(`pnpm check:workflow-status-functions`,接进 `lint.yml` 的 ESLint job)。** 范围严格限定在 **job 级** `if:` 且读 `needs.*.outputs.*`;step 级条件与 `needs.*.result` 刻意在外(前者的隐式 `success()` 说的是「本 job 前面的步骤挂了就别继续」,通常正是作者要的;后者本来就是状态读,作者已经在推理状态,不是被塞了一层没写的包装)。这条边界是「无需猜意图、也无需豁免名单」的前提,脚本因此**没有**任何 skip-list。 + +判据用**真 YAML 解析**而非 grep `if:` 行,这一步是承重的:grep 分不清「文件坏了」和「没有违规」—— 两者都是零匹配 —— 而且看不见折叠标量。本仓已有两个 workflow 写 `if: >-` 跨行(`merge-queue-triage.yml`、`pr-automation.yml`),#5343 那张靠手工 grep 得出的审计表在那里是盲的。输入缺失一律判红(目录不存在、零个 workflow 文件、YAML 解析失败、没有 `jobs:` 映射、`if:` 不是标量),绝不 `exit 0` 静默放过 —— 即 #4690 反模式的正面。`--self-test` 34 条断言跑真实 `scan()` 路径,`--list` 直接输出审计表(现有 9 处 job 级 `needs.*.outputs.*` 读)。 + +**`publish-smoke.yml` 的唯一存量违规按「显式红,不跑、不猜」修掉。** `pack-smoke` 的行为不变(仍是 resolve 成功且 `run == 'true'` 才跑),但 `success() &&` 现在写了出来;真正新增的是 `resolve-guard` job:`always() && needs.resolve.result == 'failure'` 时判红,并尽力把一条 failure commit status 写回 release PR head。 + +这里**没有**照搬 #4928 的 `!cancelled() && ... != 'false'` 形状,原因是那条「存疑就全跑」在本 workflow 不成立:`ci.yml` 的 filter job 输出带 `|| 'true'` 兜底,而 `resolve` 没有,且 `ref` 也是它算出来的 —— 存疑就跑会 checkout 一个空 ref,花 45 分钟 smoke 掉不知道什么东西,再把结论当作「release candidate 通过」报出去。发布完整性上的**假绿**比没有答案更糟。 + +顺带实测(#5343 留的核实项):`publish-smoke / packed-tarballs` 在 `main` 的分支保护里**不是**必需检查(必需的是 `TypeScript Type Check`、`Build Core`、`Test Core`、`Dogfood Regression Gate` 四项)。所以 resolve 挂掉时那条 status 根本不写的后果不是「PR 被卡住」,而是**它在 release PR 上完全不可见** —— 这正是新增 guard job 回写 status 所填的洞。 + +纯工具链改动,不发版。 diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index a599cfc5c1..822ea99b12 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -301,6 +301,27 @@ jobs: - name: Node-version drift guard run: pnpm check:node-version + # Workflow status-function guard (#5343, from #4900 and #4928). GitHub + # wraps any `if:` that names no status function in an IMPLICIT success(). + # So a job-level condition written to read an upstream job's OUTPUT VALUE + # silently also carries a status decision nobody wrote, and the two facts + # "the upstream job DIED" and "the upstream job said don't run" arrive as + # the same skip — which every checks list renders as green. #4900 found a + # release-integrity guard that stopped guarding exactly when the job before + # it failed; #4928 found seven ci.yml gates that turned themselves off + # whenever something upstream broke. Both were caught by hand, months + # apart, and the rule is statically decidable — so it is a gate now. + # Job-level `if:` reading `needs.*.outputs.*` only: step-level conditions + # and `needs.*.result` are deliberately out of scope (see the script + # header). Real YAML parse, not a grep over `if:` lines — a grep cannot + # distinguish a malformed workflow from a clean one and cannot see the + # folded `if: >-` two of these workflows already use. Runs its own + # --self-test first: the detector can break while every workflow is fine, + # and a scan that quietly stops matching would report OK while reading + # nothing (#4690's family). + - name: Workflow status-function guard + run: pnpm check:workflow-status-functions + # #4248 packaging-hygiene guard. Without a `files` whitelist npm packs the # whole package directory, and 20 of the 49 publishable packages declared # none — so consumers installed TypeScript sources, unit tests and build diff --git a/.github/workflows/publish-smoke.yml b/.github/workflows/publish-smoke.yml index c9c5097e8b..377bd36b63 100644 --- a/.github/workflows/publish-smoke.yml +++ b/.github/workflows/publish-smoke.yml @@ -34,6 +34,14 @@ # commit status on the branch head so it IS visible on the release PR. # Not wired into normal PR CI on purpose: full build + pack + clean install # is far too slow for the inner loop. +# +# That reporting path is also why there is a third job, resolve-guard (#5343): +# every verdict this workflow can produce reaches a human through that commit +# status, so a `resolve` that FAILS writes nothing at all — the release PR then +# looks exactly like "there was nothing to smoke". The status is not a required +# check (branch protection on main requires TypeScript Type Check, Build Core, +# Test Core and Dogfood Regression Gate), so an absent one blocks nothing and is +# simply invisible. resolve-guard turns that silence into an explicit red. name: Publish Smoke @@ -92,10 +100,66 @@ jobs: echo "No open release PR — nothing to smoke" fi + # resolve failing is NOT "nothing to smoke" — it is "we do not know". Say so + # out loud (#5343). Without this job the two outcomes are indistinguishable + # everywhere it matters: pack-smoke is skipped in both, and a skipped job reads + # as green; and since this workflow runs on `workflow_run`, its own red run is + # filed against the default branch, nowhere near the release PR a human is + # about to merge. + # + # The alternative shape — #4928's `!cancelled() && ... != 'false'`, i.e. "when + # in doubt, run everything" — is wrong HERE, and that is why the decision was + # taken per workflow rather than copied. ci.yml's filter job defaults its + # outputs to "run it" (`|| 'true'`); resolve does not, and it also computes the + # `ref` to check out. Running on doubt would check out an EMPTY ref (the + # workflow_run default), spend 45 minutes smoking whatever that resolves to, + # and report the verdict as if it were about the release candidate. A false + # green on release integrity is worse than no answer. + resolve-guard: + name: Resolve target failed — the candidate was NOT smoked + needs: resolve + if: always() && needs.resolve.result == 'failure' + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + statuses: write + steps: + - name: Report the gap on the release PR, then fail + env: + GH_TOKEN: ${{ github.token }} + run: | + # Best-effort, and deliberately a SECOND lookup rather than a reuse of + # resolve's outputs: resolve failed, so its outputs are empty — that is + # the whole problem. Same query as resolve's (`--head + # changeset-release/main`); keep the two in step if that convention + # ever changes. Whatever this lookup does, the job still fails below, + # so the fallback can never turn into a silent pass. + release_head=$(gh pr list --repo "$GITHUB_REPOSITORY" \ + --head changeset-release/main --state open \ + --json headRefOid -q '.[0].headRefOid // empty') || release_head='' + if [ -n "$release_head" ]; then + gh api "repos/$GITHUB_REPOSITORY/statuses/$release_head" \ + -f state=failure -f context='publish-smoke / packed-tarballs' \ + -f description='Target resolution failed — the candidate was never smoked' \ + -f target_url="$GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" \ + || echo "::warning::could not write the commit status on $release_head" + else + echo "::warning::no open changeset-release/main PR found — the failure is reported here only" + fi + echo "::error title=Publish smoke did not run::resolve failed, so the release candidate was never smoked. This is NOT the same as 'nothing to smoke' — re-run the workflow, or check the release PR by hand before merging." + exit 1 + pack-smoke: name: Packed-tarball smoke (release candidate) needs: resolve - if: needs.resolve.outputs.run == 'true' + # success() is spelled out rather than left to GitHub's implicit wrapper + # (#5343): an `if:` naming no status function is silently wrapped in one, so + # "resolve failed" and "resolve said don't run" both arrive here as the same + # skip. The behaviour is unchanged — smoke only a resolve that SUCCEEDED and + # said run — but it is now written down, and resolve-guard above states the + # failure case that this condition deliberately does not cover. + if: success() && needs.resolve.outputs.run == 'true' runs-on: ubuntu-latest timeout-minutes: 45 permissions: diff --git a/package.json b/package.json index 1c719c099f..5fd689e34d 100644 --- a/package.json +++ b/package.json @@ -53,6 +53,7 @@ "check:release-notes": "node scripts/check-release-notes.mjs", "check:release-body": "node scripts/release-github-releases.mjs --self-test", "check:node-version": "node scripts/check-node-version.mjs", + "check:workflow-status-functions": "node scripts/check-workflow-status-functions.mjs --self-test && node scripts/check-workflow-status-functions.mjs", "check:published-files": "node scripts/check-published-files.mjs --self-test && node scripts/check-published-files.mjs", "check:type-check-coverage": "node scripts/check-type-check-coverage.mjs --self-test && node scripts/check-type-check-coverage.mjs", "check:driver-conformance": "node scripts/check-driver-conformance.mjs --self-test && node scripts/check-driver-conformance.mjs", @@ -78,7 +79,8 @@ "tsup": "^8.5.1", "tsx": "^4.23.1", "turbo": "^2.10.7", - "typescript": "^6.0.3" + "typescript": "^6.0.3", + "yaml": "^2.9.0" }, "engines": { "node": ">=22.0.0" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7bf02840b0..6c058d4715 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -68,6 +68,9 @@ importers: typescript: specifier: ^6.0.3 version: 6.0.3 + yaml: + specifier: ^2.9.0 + version: 2.9.0 apps/docs: dependencies: diff --git a/scripts/check-workflow-status-functions.mjs b/scripts/check-workflow-status-functions.mjs new file mode 100644 index 0000000000..51ed8b373c --- /dev/null +++ b/scripts/check-workflow-status-functions.mjs @@ -0,0 +1,609 @@ +#!/usr/bin/env node +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +// +// check-workflow-status-functions -- a JOB-LEVEL `if:` that reads +// `needs..outputs.` must name a status function. +// +// node scripts/check-workflow-status-functions.mjs +// node scripts/check-workflow-status-functions.mjs --self-test # the checker itself +// node scripts/check-workflow-status-functions.mjs --list # the full audit table +// +// ## The rule (#5343, distilled from #4900 and #4928) +// +// Any job-level `if:` that reads `needs.SOMEJOB.outputs.SOMEKEY` must name one +// of `always()`, `!cancelled()`, `success()` or `failure()`. +// +// GitHub wraps every `if:` that contains no status function in an IMPLICIT +// `success()`. So a condition written to read an upstream job's OUTPUT VALUE -- +// a data-driven decision -- silently also carries a status decision the author +// never wrote. The two completely different facts +// +// the upstream job DIED (status) +// the upstream job said "do not run" (data) +// +// are then compressed into the same `skipped`, and a skipped job reads as green +// in every checks list. That is how #4900 shipped a release-integrity guard that +// stopped guarding whenever the job before it failed, and how #4928 found seven +// `needs.filter.outputs.*` gates in ci.yml that turned themselves off exactly +// when something upstream broke. +// +// Wanting `success()` semantics is perfectly legitimate -- pack-smoke in +// publish-smoke.yml genuinely wants them. The rule is that you WRITE IT: +// `success() && needs.x.outputs.y == 'true'`. Declared beats remembered; the +// next reader must not have to carry GitHub's implicit wrapper in their head. +// +// ## Scope, and why it is drawn here +// +// JOB level only, and only `needs.*.outputs.*`. Both halves are deliberate. +// +// - A STEP-level `if:` reading `steps.*.outputs.*` is out. There the implicit +// `success()` means "an earlier step of THIS job failed, so stop", which is +// almost always exactly what the author wants. Scanning those would produce +// a batch of false positives on day one, and a gate born with a long +// exemption list is one step from the #4690 anti-pattern it exists to +// prevent. +// - A STEP-level `if:` reading `needs.*.outputs.*` is out for the same reason: +// the implicit `success()` there is still about the steps of its own job, +// and the job it sits in already had to pass this rule at job level. +// publish-smoke.yml's two `needs.resolve.outputs.report-sha` steps are the +// live specimens -- both correct, neither this gate's business. +// - `needs.*.result` / `needs.*.conclusion` are out. Those ARE status reads; +// an author writing one is already reasoning about upstream status rather +// than being handed a wrapper they did not write. This gate is about the +// silent compression of status into data, not about status expressions. +// +// The scope is what makes the rule statically decidable with no intent-guessing +// and no exemption hatch. There is intentionally NO skip-list: if a job-level +// `if:` ever genuinely needs one, that is a decision to take in the open. +// +// ## Missing input is a failure, never a pass (#4690) +// +// `.github/workflows/` absent, zero workflow files, a file that does not parse, +// a workflow with no `jobs:` map, an `if:` that is not a scalar -- every one of +// them exits non-zero. A checker that cannot read its input has not verified +// anything, and reporting OK for that is the exact defect #4690 recorded: a gate +// that skipped silently and exited 0 read as "no violations" for months. +// +// This is also why the parse is a real YAML parse rather than a grep over `if:` +// lines. A grep cannot tell a malformed workflow from a clean one -- it just +// finds nothing, which is indistinguishable from "no violations" -- and it +// cannot see a folded expression at all. Two workflows in this repo already +// write `if: >-` across several lines (merge-queue-triage.yml, pr-automation.yml); +// the hand-run grep audit in #5343 could not have seen a violation there. + +import { execFileSync } from 'node:child_process'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { LineCounter, isMap, isScalar, parseDocument } from 'yaml'; + +const WORKFLOW_DIR = '.github/workflows'; + +/** + * Reads an upstream job's OUTPUT value: `needs..outputs.` in either + * the dot or the index spelling, with GitHub's tolerated whitespace. + * Deliberately stops at `outputs` + accessor, so `needs.build.result` and a bare + * `toJSON(needs)` do not match -- see the scope note in the header. + */ +const READS_NEEDS_OUTPUTS = /\bneeds\s*(?:\.\s*[A-Za-z_][A-Za-z0-9_-]*|\[[^\]]*\])\s*\.\s*outputs\s*(?:\.|\[)/i; + +/** GitHub's four status check functions. Case-insensitive, as the expression language is. */ +const NAMES_STATUS_FUNCTION = /\b(?:always|cancelled|success|failure)\s*\(\s*\)/i; + +/** The four status functions, for the failure prescription. */ +const STATUS_FUNCTIONS = ['success()', '!cancelled()', 'always()', 'failure()']; + +// ── Scanning ──────────────────────────────────────────────────────────────── + +/** + * Every job-level `if:` in the workflow directory, judged. + * + * @param {string} root repository root (or, in --self-test, a fixture root) + * @returns {{ + * violations: { file: string, line: number, job: string, expression: string }[], + * compliant: { file: string, line: number, job: string, expression: string }[], + * problems: string[], + * files: number, + * jobs: number, + * jobIfs: number, + * }} + */ +export function scan(root) { + const violations = []; + const compliant = []; + const problems = []; + let jobs = 0; + let jobIfs = 0; + + const dir = join(root, WORKFLOW_DIR); + if (!existsSync(dir) || !statSync(dir).isDirectory()) { + problems.push(`${WORKFLOW_DIR}/ does not exist -- nothing was scanned, so nothing was verified.`); + return { violations, compliant, problems, files: 0, jobs, jobIfs }; + } + + const files = readdirSync(dir) + .filter((f) => f.endsWith('.yml') || f.endsWith('.yaml')) + .sort(); + if (files.length === 0) { + problems.push(`${WORKFLOW_DIR}/ holds no .yml/.yaml file -- nothing was scanned, so nothing was verified.`); + return { violations, compliant, problems, files: 0, jobs, jobIfs }; + } + + for (const name of files) { + const rel = `${WORKFLOW_DIR}/${name}`; + const source = readFileSync(join(dir, name), 'utf8'); + const lineCounter = new LineCounter(); + let doc; + try { + doc = parseDocument(source, { lineCounter }); + } catch (err) { + problems.push(`${rel}: YAML parse threw -- ${err.message}`); + continue; + } + if (doc.errors.length > 0) { + problems.push(`${rel}: YAML parse error -- ${doc.errors[0].message}`); + continue; + } + + const jobsNode = doc.get('jobs'); + if (!isMap(jobsNode)) { + problems.push(`${rel}: no \`jobs:\` mapping -- a workflow this gate cannot read is a workflow it cannot clear.`); + continue; + } + + for (const pair of jobsNode.items) { + const job = String(pair.key?.value ?? pair.key); + jobs++; + const body = pair.value; + if (!isMap(body)) { + problems.push(`${rel}: job \`${job}\` is not a mapping -- cannot read its \`if:\`.`); + continue; + } + const ifPair = body.items.find((p) => String(p.key?.value) === 'if'); + if (!ifPair) continue; + + const line = lineCounter.linePos(ifPair.key.range[0]).line; + if (!isScalar(ifPair.value) || ifPair.value.value === null || ifPair.value.value === undefined) { + problems.push(`${rel}:${line}: job \`${job}\` has an \`if:\` that is not a scalar expression.`); + continue; + } + jobIfs++; + + const expression = String(ifPair.value.value).replace(/\s+/g, ' ').trim(); + if (!READS_NEEDS_OUTPUTS.test(expression)) continue; + + const entry = { file: rel, line, job, expression }; + if (NAMES_STATUS_FUNCTION.test(expression)) compliant.push(entry); + else violations.push(entry); + } + } + + return { violations, compliant, problems, files: files.length, jobs, jobIfs }; +} + +// ── Reporting ─────────────────────────────────────────────────────────────── + +/** GitHub workflow-command escaping for a message body. */ +function escapeData(value) { + return String(value).replace(/%/g, '%25').replace(/\r/g, '%0D').replace(/\n/g, '%0A'); +} + +/** GitHub workflow-command escaping for an annotation property value. */ +function escapeProperty(value) { + return escapeData(value).replace(/:/g, '%3A').replace(/,/g, '%2C'); +} + +function annotate({ file, line, job, expression }) { + const message = escapeData( + `Job \`${job}\` reads needs.*.outputs.* in its job-level if: without naming a status function, ` + + `so GitHub wraps it in an implicit success() and an upstream FAILURE becomes indistinguishable ` + + `from an upstream "do not run". Write the status function you mean, e.g. \`success() && ${expression}\`.`, + ); + const title = escapeProperty('Job-level if: reads needs.*.outputs.* without a status function'); + return `::error file=${escapeProperty(file)},line=${line},title=${title}::${message}`; +} + +const PRESCRIPTION = ` +Pick the status function you actually mean and write it: + +${STATUS_FUNCTIONS.map((fn) => ` if: \${{ ${fn} && }}`).join('\n')} + + • success() keeps today's behaviour, written down instead of implied. + • !cancelled() runs unless the whole run was cancelled -- the #4928 shape for + a filter job whose outputs default to "run it" (\`|| 'true'\`). + • always() runs even when the run is being cancelled. + • failure() runs only on upstream failure (a guard/reporter job). + +!cancelled() is NOT automatically the right answer. It is only correct when the +upstream outputs still carry a usable value after a failure. publish-smoke.yml's +resolve job is the counter-example #5343 worked through: its \`run\` output has no +\`|| 'true'\` default AND it also computes the \`ref\` to check out, so "when in +doubt run everything" would start a 45-minute job against an empty ref -- smoking +the wrong thing, then reporting green about it. That workflow instead states the +failure explicitly (a guard job that goes red), which is the third option this +rule exists to make visible. + +Scope: job-level \`if:\` only, and only \`needs.*.outputs.*\`. Step-level \`if:\` and +\`needs.*.result\` are deliberately out -- see the header of +scripts/check-workflow-status-functions.mjs.`; + +function repoRoot() { + return execFileSync('git', ['rev-parse', '--show-toplevel'], { encoding: 'utf8' }).trim(); +} + +function summarise({ files, jobs, jobIfs, compliant, violations }) { + const reads = compliant.length + violations.length; + return `scanned ${files} workflow file(s), ${jobs} job(s), ${jobIfs} job-level if: expression(s); ${reads} read needs.*.outputs.*`; +} + +function reportProblems(problems) { + console.error(`check-workflow-status-functions: ${problems.length} input problem(s) -- the scan is NOT a pass\n`); + for (const p of problems) console.error(` • ${p}`); + console.error(` +A gate that could not read its input has verified nothing. Reporting OK here is +the #4690 anti-pattern (a check that silently skipped and exited 0), so this +exits non-zero instead. Fix the input, or fix the checker -- never both quietly.`); +} + +function main() { + const result = scan(repoRoot()); + const { violations, problems } = result; + + if (problems.length > 0) { + reportProblems(problems); + process.exit(1); + } + + if (violations.length === 0) { + console.log(`check-workflow-status-functions: OK (${summarise(result)}, all naming a status function).`); + process.exit(0); + } + + const plural = violations.length === 1 ? 'job-level if:' : 'job-level if: expressions'; + console.error( + `check-workflow-status-functions: ${violations.length} ${plural} read needs.*.outputs.* without naming a status function\n`, + ); + for (const v of violations) { + console.error(` • ${v.file}:${v.line} job \`${v.job}\``); + console.error(` if: ${v.expression}`); + console.log(annotate(v)); + } + console.error(PRESCRIPTION); + process.exit(1); +} + +function list() { + const result = scan(repoRoot()); + if (result.problems.length > 0) { + reportProblems(result.problems); + process.exit(1); + } + const rows = [...result.violations.map((e) => ({ ...e, verdict: 'VIOLATION' })), ...result.compliant.map((e) => ({ ...e, verdict: 'ok' }))].sort( + (a, b) => a.file.localeCompare(b.file) || a.line - b.line, + ); + for (const r of rows) console.log(`${r.verdict.padEnd(9)} ${r.file}:${r.line} ${r.job}\n if: ${r.expression}`); + console.log(`\n${summarise(result)}`); +} + +// ── Self-test ──────────────────────────────────────────────────────────────── +// +// Fixture workflows in a temp dir, run through the SAME scan() main() calls -- +// the check-nul-bytes.mjs convention. scan() reads the directory rather than the +// git index, so a temp dir (no `git init`) is the faithful analogue of that +// script's temp repo: it exercises the real discovery path, not an imitation. + +function selfTest() { + const failures = []; + let checked = 0; + const assert = (cond, msg) => { + checked++; + if (!cond) failures.push(msg); + }; + + const roots = []; + const makeRoot = (files) => { + const dir = mkdtempSync(join(tmpdir(), 'check-workflow-status-fn-')); + roots.push(dir); + for (const [rel, contents] of Object.entries(files)) { + const full = join(dir, rel); + mkdirSync(dirname(full), { recursive: true }); + writeFileSync(full, contents); + } + return dir; + }; + + try { + // ── 1. A violating sample must go red ──────────────────────────────────── + // + // Verbatim shape of publish-smoke.yml:98 as #5343 found it. + const violating = makeRoot({ + '.github/workflows/publish-smoke.yml': `name: Publish Smoke +on: + workflow_run: + workflows: [Release] + types: [completed] +jobs: + resolve: + runs-on: ubuntu-latest + outputs: + run: \${{ steps.target.outputs.run }} + steps: + - id: target + run: echo "run=true" >> "$GITHUB_OUTPUT" + pack-smoke: + needs: resolve + if: needs.resolve.outputs.run == 'true' + runs-on: ubuntu-latest + steps: + - run: echo smoke +`, + }); + const red = scan(violating); + assert(red.problems.length === 0, `a well-formed fixture reports no input problems, got ${red.problems[0]}`); + assert(red.violations.length === 1, `the violating sample is flagged once, got ${red.violations.length}`); + assert(red.violations[0]?.job === 'pack-smoke', `the violating JOB is named, got ${red.violations[0]?.job}`); + assert(red.violations[0]?.line === 16, `the reported line points at the if:, got ${red.violations[0]?.line}`); + assert( + red.violations[0]?.file === '.github/workflows/publish-smoke.yml', + `the file is reported repo-relative, got ${red.violations[0]?.file}`, + ); + // The annotation is the acceptance criterion's own wording -- pin its shape. + const line = annotate(red.violations[0]); + assert( + line.startsWith('::error file=.github/workflows/publish-smoke.yml,line=16,title='), + `the annotation carries file= and line=, got ${line.slice(0, 80)}`, + ); + assert(!/[\r\n]/.test(line.slice(2)), 'the annotation is a single line (raw newlines are escaped)'); + + // ── 2. Compliant samples must stay green ───────────────────────────────── + // + // All four status functions, the #4928 shape, and the negated form -- a rule + // that only recognised `success()` would pass this fixture for the wrong + // reason, so each spelling is asserted separately below. + const green = makeRoot({ + '.github/workflows/ci.yml': `name: CI +on: [push] +jobs: + filter: + runs-on: ubuntu-latest + outputs: + core: \${{ steps.f.outputs.core || 'true' }} + steps: + - id: f + run: echo "core=true" >> "$GITHUB_OUTPUT" + build: + needs: filter + if: \${{ !cancelled() && needs.filter.outputs.core != 'false' }} + runs-on: ubuntu-latest + steps: + - run: echo build + pack: + needs: filter + if: success() && needs.filter.outputs.core == 'true' + runs-on: ubuntu-latest + steps: + - run: echo pack + report: + needs: filter + if: always() && needs.filter.outputs.core == 'true' + runs-on: ubuntu-latest + steps: + - run: echo report + oncall: + needs: filter + if: failure() && needs.filter.outputs.core == 'true' + runs-on: ubuntu-latest + steps: + - run: echo page +`, + }); + const ok = scan(green); + assert(ok.problems.length === 0, `the compliant fixture reports no input problems, got ${ok.problems[0]}`); + assert(ok.violations.length === 0, `the compliant sample is green, got ${ok.violations.length} violation(s)`); + assert(ok.compliant.length === 4, `all four status-function spellings are recognised, got ${ok.compliant.length}`); + for (const fn of ['!cancelled()', 'success()', 'always()', 'failure()']) { + assert( + ok.compliant.some((c) => c.expression.includes(fn)), + `${fn} is recognised as naming a status function`, + ); + } + + // ── 3. Missing input must go red, in all four shapes (#4690) ───────────── + const noDir = makeRoot({ 'README.md': '# no workflows here\n' }); + const missing = scan(noDir); + assert(missing.problems.length === 1, `a missing ${WORKFLOW_DIR}/ is an input problem, got ${missing.problems.length}`); + assert(missing.files === 0 && missing.violations.length === 0, 'a missing directory scans nothing (and says so)'); + + const emptyDir = makeRoot({ '.github/workflows/.gitkeep': '' }); + const empty = scan(emptyDir); + assert(empty.problems.length === 1, `an empty ${WORKFLOW_DIR}/ is an input problem, got ${empty.problems.length}`); + + const malformed = makeRoot({ + '.github/workflows/broken.yml': 'name: Broken\non: [push]\njobs:\n a:\n - this is not\n a mapping: [\n', + }); + const parseFail = scan(malformed); + assert(parseFail.problems.length >= 1, 'a YAML parse failure is an input problem, not a pass'); + assert( + parseFail.problems[0].includes('broken.yml'), + `the unparseable file is named, got ${parseFail.problems[0]}`, + ); + + const jobless = makeRoot({ '.github/workflows/jobless.yml': 'name: Jobless\non: [push]\n' }); + const noJobs = scan(jobless); + assert(noJobs.problems.length === 1, 'a workflow with no jobs: mapping is an input problem'); + + // ── 4. Scope: step level is out, in BOTH spellings ─────────────────────── + // + // This is the boundary the rule was narrowed to on purpose (#5343). Pinning + // it here means a later widening has to be deliberate rather than accidental + // -- and the `needs.*` step case is the one a careless regex would sweep in. + const steps = makeRoot({ + '.github/workflows/steps.yml': `name: Steps +on: [push] +jobs: + resolve: + runs-on: ubuntu-latest + outputs: + report-sha: \${{ steps.t.outputs.sha }} + steps: + - id: t + run: echo "sha=deadbeef" >> "$GITHUB_OUTPUT" + smoke: + needs: resolve + if: success() && needs.resolve.outputs.report-sha != '' + runs-on: ubuntu-latest + steps: + - id: cache + run: echo "cache-hit=true" >> "$GITHUB_OUTPUT" + - name: step reading its own job's step outputs + if: steps.cache.outputs.cache-hit != 'true' + run: echo build + - name: step reading an upstream job's outputs + if: needs.resolve.outputs.report-sha != '' + run: echo report +`, + }); + const stepScope = scan(steps); + assert(stepScope.problems.length === 0, `the step-scope fixture is well-formed, got ${stepScope.problems[0]}`); + assert(stepScope.violations.length === 0, `step-level if: is out of scope, got ${stepScope.violations.length}`); + assert(stepScope.jobIfs === 1, `only the job-level if: is counted, got ${stepScope.jobIfs}`); + + // ── 5. Scope: needs.*.result is a status read, not a data read ─────────── + const resultRead = makeRoot({ + '.github/workflows/guard.yml': `name: Guard +on: [push] +jobs: + resolve: + runs-on: ubuntu-latest + steps: + - run: echo hi + guard: + needs: resolve + if: needs.resolve.result == 'failure' + runs-on: ubuntu-latest + steps: + - run: exit 1 +`, + }); + assert(scan(resultRead).violations.length === 0, 'needs.*.result is outside the rule'); + + // ── 6. A folded expression is read as one expression ───────────────────── + // + // The reason this gate parses YAML instead of grepping `if:` lines: the + // #5343 audit table was built by grep, and a grep over lines cannot see the + // violation below at all -- the `needs.` read and the (absent) status + // function sit on different source lines. Two workflows in this repo + // already write `if: >-`. + const folded = makeRoot({ + '.github/workflows/folded.yml': `name: Folded +on: [push] +jobs: + a: + runs-on: ubuntu-latest + outputs: + go: \${{ steps.s.outputs.go }} + steps: + - id: s + run: echo "go=true" >> "$GITHUB_OUTPUT" + b: + needs: a + if: >- + github.event_name == 'push' && + needs.a.outputs.go == 'true' + runs-on: ubuntu-latest + steps: + - run: echo b +`, + }); + const foldedResult = scan(folded); + assert(foldedResult.violations.length === 1, `a folded violating if: is flagged, got ${foldedResult.violations.length}`); + assert(foldedResult.violations[0]?.line === 13, `the folded if: is located at its key, got ${foldedResult.violations[0]?.line}`); + assert( + !/\n/.test(foldedResult.violations[0]?.expression ?? '\n'), + 'a folded expression is normalised to one line for the report', + ); + // ...and the same expression WITH a status function is green, so the fixture + // proves the parser, not merely that folded scalars are always flagged. + const foldedOk = makeRoot({ + '.github/workflows/folded-ok.yml': readFileSync(join(folded, '.github/workflows/folded.yml'), 'utf8').replace( + " github.event_name == 'push' &&", + " !cancelled() && github.event_name == 'push' &&", + ), + }); + assert(scan(foldedOk).violations.length === 0, 'the same folded expression with !cancelled() is green'); + + // ── 7. Spelling variants the expression language accepts ───────────────── + const variants = makeRoot({ + '.github/workflows/variants.yml': `name: Variants +on: [push] +jobs: + a: + runs-on: ubuntu-latest + steps: + - run: echo hi + index-form: + needs: a + if: \${{ needs['a'].outputs['go'] == 'true' }} + runs-on: ubuntu-latest + steps: + - run: echo b + mixed-case-status: + needs: a + if: \${{ Always() && needs.a.outputs.go == 'true' }} + runs-on: ubuntu-latest + steps: + - run: echo c + wrapped-in-fromjson: + needs: a + if: \${{ contains(fromJSON(needs.a.outputs.list), 'x') }} + runs-on: ubuntu-latest + steps: + - run: echo d +`, + }); + const variantResult = scan(variants); + assert( + variantResult.violations.some((v) => v.job === 'index-form'), + 'the needs[...] index spelling is read as an outputs read', + ); + assert( + variantResult.violations.some((v) => v.job === 'wrapped-in-fromjson'), + 'an outputs read wrapped in fromJSON() is still an outputs read', + ); + assert( + !variantResult.violations.some((v) => v.job === 'mixed-case-status'), + 'the expression language is case-insensitive, so Always() counts', + ); + + // ── 8. The real repository is what this gate actually guards ───────────── + // + // Running scan() over the checkout itself is the reverse direction of the + // fixtures above: fixtures prove the detector can go red, this proves the + // tree it ships with is green. It also fails loudly if the repo's workflow + // directory ever moves out from under the gate. + const real = scan(repoRoot()); + assert(real.problems.length === 0, `the repo's own workflows parse cleanly, got ${real.problems[0]}`); + assert(real.files > 0 && real.jobs > 0, 'the repo scan actually reads workflows'); + assert( + real.compliant.length > 0, + 'the repo has at least one job-level needs.*.outputs.* read -- if this ever hits 0, the gate is guarding nothing', + ); + } finally { + for (const dir of roots) rmSync(dir, { recursive: true, force: true }); + } + + if (failures.length) { + console.error(`✗ check-workflow-status-functions --self-test -- ${failures.length} failure(s)\n`); + for (const f of failures) console.error(` • ${f}`); + process.exit(1); + } + console.log(`✓ check-workflow-status-functions --self-test: ${checked} assertions over temp fixture roots (real scan() path)`); +} + +if (process.argv.includes('--self-test')) { + selfTest(); +} else if (process.argv.includes('--list')) { + list(); +} else { + main(); +}