From 5a309c180f20face5e1b7f61482ca6cb25b62407 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 30 Sep 2026 08:51:15 -0700 Subject: [PATCH] ci(vale): report the contract verdict and upstream's new format readers on the upgrade PR The upgrade workflow left the contract suites to Validate, "a red check on a pull request that already carries upstream's notes". Validate does not run there: a pull_request run triggered by github-actions[bot] waits at action_required until a maintainer approves it. Every bot-opened upgrade, Vale and ast-grep alike, measured the same way, and #427 read as a pull request with no checks while three contract tests failed. The job now runs vale-schema-contract and vale-vendor-contract itself, continue-on-error so a changed verdict still proposes the upgrade, and vale-upgrade-report.cjs renders the verdict into the body. The same report lists the files upstream added under internal/lint/. That is the source check capabilities.ts asked for by hand on every bump, and the one that would have caught kt.go on 3.23.0: an extension with no VALE_FORMAT_TIERS row is never probed, so no test can. Neither half gates. A missing test report or a failed compare fetch is said in the body, so "not checked" never reads as a pass. --- .github/scripts/vale-upgrade-report.cjs | 208 +++++++++++++++++++ .github/scripts/vale-upgrade-report.test.cjs | 195 +++++++++++++++++ .github/workflows/vale-upgrade.yml | 71 ++++++- packages/cli/src/rules/capabilities.ts | 6 + 4 files changed, 472 insertions(+), 8 deletions(-) create mode 100644 .github/scripts/vale-upgrade-report.cjs create mode 100644 .github/scripts/vale-upgrade-report.test.cjs diff --git a/.github/scripts/vale-upgrade-report.cjs b/.github/scripts/vale-upgrade-report.cjs new file mode 100644 index 00000000..94ef5914 --- /dev/null +++ b/.github/scripts/vale-upgrade-report.cjs @@ -0,0 +1,208 @@ +#!/usr/bin/env node +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * What a Vale upgrade pull request needs a reviewer to see, and CI does not + * show them: the contract suites' verdict, and the format readers upstream + * added. + * + * THE CONTRACT VERDICT, BECAUSE VALIDATE DOES NOT RUN. vale-upgrade.yml opens + * its pull request as `github-actions[bot]`, and every `pull_request` run that + * actor triggers waits at `action_required` until a maintainer approves it. + * Measured on every bot-opened upgrade so far, Vale and ast-grep alike: #427's + * Validate sat at `action_required` with a 0s runtime, and on each earlier one + * Validate first ran when a human pushed a child commit. So the red check the + * workflow header relies on ("left to Validate, it is a red check on a pull + * request that already carries upstream's notes") never appeared: #427 read as + * a pull request with no checks, while three contract tests were failing. + * + * The suites are still NOT a gate. The header's reasoning stands: a changed + * verdict is a measured edit for a child pull request, and gating on it turns + * a scheduled run red every morning with no pull request to read. So the job + * runs them, keeps going whatever they say, and this script puts the verdict + * in the body. + * + * THE FORMAT READERS, BECAUSE NO TABLE CAN REPORT ONE. `VALE_FORMAT_TIERS` in + * capabilities.ts is re-probed row by row on every bump, which catches a format + * that MOVED. It cannot catch a format Vale LEARNED, since an extension with no + * row is never probed; the table's own header asks for a source check instead, + * by hand. 3.23.0 is the bump where nobody did it: `internal/lint/code/kt.go` + * moved Kotlin from plaintext to the comment tier and every test stayed green. + * The benign direction. The dangerous one is a reader that shells out to a + * converter, which turns the same missing row into a crash that takes down the + * run. Listing the files upstream ADDED under `internal/lint/` is the source + * check, mechanically. + * + * Both halves report and never throw. A failed fetch or a missing test report + * is said in the body rather than failing the run: the proposal is still worth + * opening, and "we could not check" must not read like "nothing to see". + */ + +const { readFileSync, writeFileSync } = require("node:fs"); + +const { baseVersion } = require("./vale-upgrade-detect.cjs"); + +/** The compare API returns at most this many files, with no marker when cut. */ +const COMPARE_FILE_CAP = 300; + +/** Where upstream keeps its format readers, and which of those files are. */ +const READER_PREFIX = "internal/lint/"; +const isReader = (filename) => + filename.startsWith(READER_PREFIX) && + filename.endsWith(".go") && + !filename.endsWith("_test.go"); + +/** + * The failures in a vitest `--reporter=json` report. + * + * `undefined` when there is no report at all, which is a different finding + * from zero failures: the suites did not run, or crashed before writing. + */ +function summarizeContract(report) { + if (report === undefined) return undefined; + const failed = []; + for (const file of report.testResults ?? []) { + for (const assertion of file.assertionResults ?? []) { + if (assertion.status !== "failed") continue; + failed.push({ + file: String(file.name ?? "").split("/").pop(), + name: assertion.fullName, + message: String(assertion.failureMessages?.[0] ?? "").split("\n")[0], + }); + } + } + return { total: report.numTotalTests ?? 0, failed }; +} + +/** Read a vitest JSON report, or `undefined` if it was never written. */ +function readContractReport(path) { + try { + return JSON.parse(readFileSync(path, "utf8")); + } catch { + return undefined; + } +} + +/** + * The reader files upstream added between two tags. + * + * Only ADDED files. A modified reader changes what a format sees, which the + * release notes and the contract suites cover; an added one can route an + * extension no row names, which only this covers. + */ +async function fetchAddedReaders(repository, from, to) { + const headers = { + accept: "application/vnd.github+json", + "user-agent": "taskless-vale-upgrade-report", + }; + if (process.env.GITHUB_TOKEN) { + headers.authorization = `Bearer ${process.env.GITHUB_TOKEN}`; + } + const url = `https://api.github.com/repos/${repository}/compare/v${from}...v${to}`; + const response = await fetch(url, { headers }); + if (!response.ok) { + throw new Error(`GET ${url} responded ${response.status}`); + } + const files = (await response.json()).files ?? []; + return { + added: files + .filter((file) => file.status === "added" && isReader(file.filename)) + .map((file) => file.filename) + .toSorted(), + complete: files.length < COMPARE_FILE_CAP, + url: `https://github.com/${repository}/compare/v${from}...v${to}`, + }; +} + +/** A fence no line of `text` can close early. */ +function fence(text) { + const longest = Math.max( + 2, + ...[...text.matchAll(/`+/g)].map((match) => match[0].length) + ); + const marker = "`".repeat(longest + 1); + return `${marker}\n${text}\n${marker}`; +} + +function formatContract(contract) { + const heading = "### Contract suites"; + if (contract === undefined) { + return `${heading}\n\n**Did not run.** No test report was written, so this upgrade has not been checked against the recorded contract. Run \`pnpm --filter @taskless/cli test --project cli vale-schema-contract vale-vendor-contract\` on this branch.\n`; + } + if (contract.failed.length === 0) { + return `${heading}\n\nAll ${contract.total} tests in \`vale-schema-contract\` and \`vale-vendor-contract\` pass against the new binary. No recorded verdict changed.\n`; + } + const lines = contract.failed.map( + (failure) => `${failure.file} > ${failure.name}\n ${failure.message}` + ); + return `${heading}\n\n**${contract.failed.length} of ${contract.total} failed.** The new Vale changed a recorded behaviour. The measured edit, the \`update\` ledger, and any migration belong in a child pull request that merges down into this branch.\n\n${fence(lines.join("\n"))}\n`; +} + +function formatReaders(readers, error) { + const heading = "### Format readers upstream added"; + if (readers === undefined) { + return `${heading}\n\n**Could not check** (${error}). Compare \`internal/lint/\` between the two tags by hand before merging.\n`; + } + const caveat = readers.complete + ? "" + : `\n\nThe comparison touched ${COMPARE_FILE_CAP} or more files, where the API stops listing them, so this may be incomplete. Check [the full diff](${readers.url}).`; + if (readers.added.length === 0) { + return `${heading}\n\nNone under \`${READER_PREFIX}\` ([diff](${readers.url})). No extension can have changed tier without a row that re-probes it.${caveat}\n`; + } + const list = readers.added.map((file) => `- \`${file}\``).join("\n"); + return `${heading}\n\n${list}\n\nEach can route an extension that \`VALE_FORMAT_TIERS\` in \`capabilities.ts\` has no row for, and an unlisted extension is never probed. Find where \`internal/core/format.go\` sends it, probe the extension against the new binary, and add a row. A reader that needs an external converter is the dangerous case: the missing row becomes a crash that aborts every Vale rule in the run. ([diff](${readers.url}))${caveat}\n`; +} + +function formatReport({ from, to, contract, readers, readersError }) { + return `## Measured against Vale ${to}\n\nCompared with ${from}, which is what \`main\` pins. Validate on a pull request this workflow opens waits for a maintainer to approve it, so this section is the check that ran.\n\n${formatContract(contract)}\n${formatReaders(readers, readersError)}`; +} + +function readArgument(argv, name) { + const index = argv.indexOf(name); + if (index === -1 || index + 1 >= argv.length) { + throw new Error(`missing ${name} `); + } + return argv[index + 1]; +} + +async function main({ + argv = process.argv.slice(2), + addedReaders = fetchAddedReaders, + readReport = readContractReport, +} = {}) { + const from = baseVersion(readArgument(argv, "--pinned")); + const to = readArgument(argv, "--to"); + const repository = readArgument(argv, "--repository"); + const out = readArgument(argv, "--out"); + + const contract = summarizeContract(readReport(readArgument(argv, "--contract"))); + let readers; + let readersError; + try { + readers = await addedReaders(repository, from, to); + } catch (error) { + readersError = error.message; + } + + const report = formatReport({ from, to, contract, readers, readersError }); + writeFileSync(out, report); + console.log(report); + return report; +} + +module.exports = { + COMPARE_FILE_CAP, + fetchAddedReaders, + formatReport, + isReader, + main, + summarizeContract, +}; + +if (require.main === module) { + main().catch((error) => { + console.error(`\nvale-upgrade-report failed: ${error.message}`); + process.exitCode = 1; + }); +} diff --git a/.github/scripts/vale-upgrade-report.test.cjs b/.github/scripts/vale-upgrade-report.test.cjs new file mode 100644 index 00000000..10ad7604 --- /dev/null +++ b/.github/scripts/vale-upgrade-report.test.cjs @@ -0,0 +1,195 @@ +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * Tests for vale-upgrade-report.cjs. + * + * The failure worth guarding against is a report that reads as reassurance + * when nothing was checked. "No failures" and "no report", "no new readers" + * and "could not ask", each pair must render differently, because #427 was a + * pull request whose silence looked exactly like a pass. + */ + +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { mkdtempSync, readFileSync, rmSync } = require("node:fs"); +const { tmpdir } = require("node:os"); +const { join } = require("node:path"); + +const { + COMPARE_FILE_CAP, + formatReport, + isReader, + main, + summarizeContract, +} = require("./vale-upgrade-report.cjs"); + +/** The shape vitest's JSON reporter writes, reduced to what is read. */ +const vitestReport = (...failures) => ({ + numTotalTests: 204, + testResults: [ + { + name: "/w/packages/cli/test/vale-schema-contract.test.ts", + assertionResults: [ + { status: "passed", fullName: "a passing test", failureMessages: [] }, + ...failures.map(([fullName, message]) => ({ + status: "failed", + fullName, + failureMessages: [`${message}\n at somewhere (file.ts:1:1)`], + })), + ], + }, + ], +}); + +const readers = (added, complete = true) => ({ + added, + complete, + url: "https://github.com/vale-cli/vale/compare/v3.22.0...v3.23.0", +}); + +const report = (overrides) => + formatReport({ + from: "3.22.0", + to: "3.23.0", + contract: summarizeContract(vitestReport()), + readers: readers([]), + ...overrides, + }); + +test("contract: names each failure with its file and first message line", () => { + const contract = summarizeContract( + vitestReport([ + "Vale schema contract agrees with every recorded verdict", + "AssertionError: scope/doc-leaf-standalone (scope: doc(h1)): recorded ignored, Vale 3.23.0 says accepted", + ]) + ); + assert.equal(contract.total, 204); + assert.deepEqual(contract.failed, [ + { + file: "vale-schema-contract.test.ts", + name: "Vale schema contract agrees with every recorded verdict", + message: + "AssertionError: scope/doc-leaf-standalone (scope: doc(h1)): recorded ignored, Vale 3.23.0 says accepted", + }, + ]); + const section = report({ contract }); + assert.match(section, /\*\*1 of 204 failed\.\*\*/); + assert.match(section, /vale-schema-contract\.test\.ts > Vale schema contract/); + assert.doesNotMatch(section, /at somewhere/); +}); + +test("contract: a missing report says it did not run, never that it passed", () => { + const section = report({ contract: summarizeContract(undefined) }); + assert.match(section, /\*\*Did not run\.\*\*/); + assert.doesNotMatch(section, /pass against the new binary/); +}); + +test("contract: a clean report says so, with the count", () => { + assert.match(report({}), /All 204 tests .* pass against the new binary/); +}); + +test("contract: a failure message cannot close the fence around it", () => { + const contract = summarizeContract( + vitestReport(["x", "expected ```` to equal ``` ```"]) + ); + const section = report({ contract }); + const fences = section.match(/^`{5}$/gm) ?? []; + assert.equal(fences.length, 2, "a fence longer than any run in the text"); +}); + +test("readers: lists only added, non-test Go files under internal/lint/", () => { + assert.equal(isReader("internal/lint/code/kt.go"), true); + assert.equal(isReader("internal/lint/quote.go"), true); + assert.equal(isReader("internal/lint/code/kt_test.go"), false); + assert.equal(isReader("internal/core/format.go"), false); + assert.equal(isReader("internal/lint/testdata/a.md"), false); +}); + +test("readers: an added reader asks for a probe and a row", () => { + const section = report({ + readers: readers(["internal/lint/code/kt.go", "internal/lint/code/toml.go"]), + }); + assert.match(section, /- `internal\/lint\/code\/kt\.go`/); + assert.match(section, /VALE_FORMAT_TIERS/); + assert.match(section, /internal\/core\/format\.go/); +}); + +test("readers: none added is said, with the diff to check it by", () => { + const section = report({}); + assert.match(section, /None under `internal\/lint\/`/); + assert.match(section, /compare\/v3\.22\.0\.\.\.v3\.23\.0/); +}); + +test("readers: a failed fetch says it could not check, never that none were added", () => { + const section = report({ readers: undefined, readersError: "GET x responded 502" }); + assert.match(section, /\*\*Could not check\*\* \(GET x responded 502\)/); + assert.doesNotMatch(section, /None under/); +}); + +test("readers: a comparison at the API's file cap is flagged as possibly incomplete", () => { + const section = report({ readers: readers([], false) }); + assert.match(section, new RegExp(`${COMPARE_FILE_CAP} or more files`)); +}); + +test("main: compares from the pinned base version and writes the report", async () => { + const directory = mkdtempSync(join(tmpdir(), "vale-upgrade-report-")); + try { + const out = join(directory, "report.md"); + const asked = []; + await main({ + argv: [ + "--pinned", + "3.22.0-20260921180930", + "--to", + "3.23.0", + "--repository", + "vale-cli/vale", + "--contract", + join(directory, "absent.json"), + "--out", + out, + ], + addedReaders: async (...args) => { + asked.push(args); + return readers(["internal/lint/code/kt.go"]); + }, + readReport: () => undefined, + }); + assert.deepEqual(asked, [["vale-cli/vale", "3.22.0", "3.23.0"]]); + const written = readFileSync(out, "utf8"); + assert.match(written, /^## Measured against Vale 3\.23\.0$/m); + assert.match(written, /Compared with 3\.22\.0/); + assert.match(written, /\*\*Did not run\.\*\*/); + } finally { + rmSync(directory, { recursive: true, force: true }); + } +}); + +test("main: a fetch that throws is reported, not raised", async () => { + const directory = mkdtempSync(join(tmpdir(), "vale-upgrade-report-")); + try { + const out = join(directory, "report.md"); + await main({ + argv: [ + "--pinned", + "3.22.0-20260921180930", + "--to", + "3.23.0", + "--repository", + "vale-cli/vale", + "--contract", + "unused", + "--out", + out, + ], + addedReaders: async () => { + throw new Error("GET x responded 403"); + }, + readReport: () => vitestReport(), + }); + assert.match(readFileSync(out, "utf8"), /Could not check\*\* \(GET x responded 403\)/); + } finally { + rmSync(directory, { recursive: true, force: true }); + } +}); diff --git a/.github/workflows/vale-upgrade.yml b/.github/workflows/vale-upgrade.yml index 2ba0eb4b..61211e2d 100644 --- a/.github/workflows/vale-upgrade.yml +++ b/.github/workflows/vale-upgrade.yml @@ -56,7 +56,9 @@ # (the measured `Vale ` block in vale-vendor-contract.test.ts and the # verdict corpus in vale-schema-contract.test.ts), the `update` ledger, any # migration, and the changeset prose. Those tests are NOT gated here, on -# purpose: see the check step. +# purpose: see the check step. They ARE run, and their verdict is written into +# the pull request body, because Validate does not run on a pull request this +# job opens until a maintainer approves it (see the report step). # # Action refs are pinned to commit SHAs; the trailing comment records the tag. @@ -215,14 +217,56 @@ jobs: # DOES, and a new Vale that changes a verdict fails them — correctly, and # the fix is a measured edit that belongs in the child pull request. Gated # here, that signal would become a scheduled run failing every morning - # with no pull request and no changelog to read; left to Validate, it is - # a red check on a pull request that already carries upstream's notes. + # with no pull request and no changelog to read. The next two steps run + # them anyway and report the verdict in the body instead. - name: Check the bump is self-consistent if: steps.detect.outputs.update == 'true' run: >- pnpm --filter @taskless/cli test --project cli engine-version-consistency + # REPORTED, NOT GATED, AND NOT LEFT TO VALIDATE. The comment above used to + # end "left to Validate, it is a red check on a pull request". It was + # not: a pull_request run triggered by github-actions[bot] waits at + # `action_required` until a maintainer approves it. Measured on every + # bot-opened upgrade, Vale and ast-grep alike, and #427 read as a pull + # request with no checks while three contract tests were failing. + # + # So the suites run here, `continue-on-error` so a changed verdict still + # proposes the upgrade, and write a JSON report the next step renders. + # The report path is absolute because `--filter` runs vitest from + # packages/cli. + - name: Run the Vale contract suites + if: steps.detect.outputs.update == 'true' + continue-on-error: true + run: >- + pnpm --filter @taskless/cli exec vitest run --project cli + vale-schema-contract vale-vendor-contract + --reporter=default --reporter=json + --outputFile.json="${RUNNER_TEMP}/vale-contract.json" + + # Renders the contract verdict, and the source check capabilities.ts asks + # for on every bump by hand: the files upstream ADDED under + # internal/lint/, where a new format reader lands. A reader adds an + # extension no VALE_FORMAT_TIERS row probes, so no test can catch it; + # 3.23.0's kt.go is the one that slipped through. See the script header. + # `continue-on-error` because the body is worth opening without it, and + # the propose step says so when the file is missing. + - name: Report the contract and upstream's new format readers + if: steps.detect.outputs.update == 'true' + continue-on-error: true + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PINNED_VERSION: ${{ steps.detect.outputs.pinned_version }} + BASE_VERSION: ${{ steps.detect.outputs.base_version }} + run: | + node .github/scripts/vale-upgrade-report.cjs \ + --pinned "$PINNED_VERSION" \ + --to "$BASE_VERSION" \ + --repository "$(node -p 'require("./.github/scripts/vale-manifest.json").upstream.repository')" \ + --contract "${RUNNER_TEMP}/vale-contract.json" \ + --out "${RUNNER_TEMP}/vale-upgrade-report.md" + - name: Propose the upgrade if: steps.detect.outputs.update == 'true' env: @@ -269,11 +313,11 @@ jobs: \`${BASE_VERSION}\` and regenerates \`src/generated/vale-vocabulary.ts\` from the new binary, and the workflow ran \`engine-version-consistency\` against the result before pushing. What - this does **not** carry is judgment: if \`vale-schema-contract\` or - \`vale-vendor-contract\` is red on this pull request, Vale - ${BASE_VERSION} changed a measured behaviour, and that edit, the - \`update\` ledger, any migration, and the release note's prose belong - in a child pull request. + this does **not** carry is judgment: if the contract suites below + report a failure, Vale ${BASE_VERSION} changed a measured behaviour, + and that edit, the \`update\` ledger, any migration, and the release + note's prose belong in a child pull request. So does a row for each + format reader listed below. **This is the half that reaches a user.** Republishing the platform packages changes nobody's install, because the CLI pins each one @@ -286,6 +330,17 @@ jobs: not write. BODY + # Ours, rendered from the two steps above. Missing only if the report + # step itself failed, which must not read as a clean upgrade. + report="${RUNNER_TEMP}/vale-upgrade-report.md" + if [ -f "$report" ]; then + printf '\n' >> "$body" + cat "$report" >> "$body" + else + printf '\n## Measured against Vale %s\n\n**Not measured.** The report step failed; read its log in this run.\n' "$BASE_VERSION" >> "$body" + echo "::warning::no upgrade report at ${report}" + fi + # Absent only if the detect step reported ahead and then wrote no # notes, which it cannot — so warn rather than skip silently. notes="${RUNNER_TEMP}/vale-release-notes.md" diff --git a/packages/cli/src/rules/capabilities.ts b/packages/cli/src/rules/capabilities.ts index 76a58ad2..5b25304b 100644 --- a/packages/cli/src/rules/capabilities.ts +++ b/packages/cli/src/rules/capabilities.ts @@ -271,6 +271,12 @@ const CONVERTER_TIER_PREFIX = "converter:"; * plain text today, but the moment Vale routes it to a converter the same * omission is a crash that takes down every Vale rule in the run. * + * The upgrade pull request lists the files upstream ADDED under + * `internal/lint/` (`.github/scripts/vale-upgrade-report.cjs`), which is the + * source check the notes below did by hand. It names the readers, not the + * extensions, so each one still needs `internal/core/format.go` read and the + * extension probed before a row is added. + * * 3.22.0 → 3.23.0 LEARNED ONE FORMAT, AND NO TEST NOTICED UNTIL IT WAS * PROBED. Every existing row was re-probed against the 3.23.0 binary and none * moved, so the contract suite was green. The source check is what shows the