diff --git a/.github/ISSUE_TEMPLATE/bug-report.yml b/.github/ISSUE_TEMPLATE/bug-report.yml index 01e6cc66..26a5e6ac 100644 --- a/.github/ISSUE_TEMPLATE/bug-report.yml +++ b/.github/ISSUE_TEMPLATE/bug-report.yml @@ -17,7 +17,7 @@ body: id: connect-version attributes: label: "@patchstack/connect version" - description: "`npm ls @patchstack/connect --depth=0` in your project, or the version in your lockfile." + description: "`npx patchstack-connect --version`, or `npm ls @patchstack/connect --depth=0` if the CLI will not run." placeholder: "0.3.31" validations: required: true diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 00b316b8..b80b21f9 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -46,12 +46,15 @@ jobs: - name: Typecheck run: npm run typecheck - - name: Test - run: npm test - + # Build BEFORE test, matching the main CI. Several tests assert against `dist/` and return early when + # it is absent — the edge-safety check, the version surfaces, the canary — so this order is what + # makes the release gate cover what actually ships. - name: Build run: npm run build + - name: Test + run: npm test + # Before publishing anything: does the build about to ship block the exploit the canary is built # around? `PS_REQUIRE_CANARY` refuses to skip, because a skipped canary reads as a passing one. - name: Canary — the build about to ship blocks the exploit @@ -74,6 +77,10 @@ jobs: environment: name: npm url: ${{ steps.version.outputs.url }} + # Exported so the job that records the version on `main` uses the version this job published, rather + # than resolving the tag a second time and being able to differ from it. + outputs: + version: ${{ steps.version.outputs.version }} steps: - name: Resolve version @@ -112,6 +119,35 @@ jobs: - name: Publish to npm run: npm publish --access public --provenance + # A job of its own, separate from `publish`, and the separation carries the invariant. + # + # This runs after the version exists on npm and cannot be unpublished, so its failure is a discovery + # about something already released rather than something preventable. It must therefore not be able to + # stop `record-version`: the state to avoid is a version irreversibly on npm, a tag pointing at it, + # `main` naming a different version, and no pull request proposing the correction. + # + # Publication ends when npm accepts the tarball. Verification and recording are both consequences of + # that, and neither blocks the other. A failure here still turns the workflow red. + verify-published: + name: Verify the published tarball + needs: publish + runs-on: ubuntu-latest + + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + ref: refs/tags/v${{ needs.publish.outputs.version }} + + - name: Setup Node + uses: actions/setup-node@v4 + with: + node-version: 24.x + cache: npm + + - name: Install dependencies + run: npm ci + # The proof that matters most, and the only one that covers PACKAGING. Everything above tests a build # in this checkout; this installs the published tarball from the registry and runs the same canary # against the engine inside it. A file left out of `files`, a wrong `exports` map, or a bad entry @@ -124,7 +160,7 @@ jobs: - name: Canary — the PUBLISHED tarball blocks the exploit run: | set -euo pipefail - version='${{ steps.version.outputs.version }}' + version='${{ needs.publish.outputs.version }}' # npm needs a moment before a fresh version resolves; a failure to install here is not a failure # of the engine and must not be reported as one. @@ -148,3 +184,202 @@ jobs: fi PS_CANARY_ENGINE="$engine" PS_REQUIRE_CANARY=1 npx vitest run tests/protect/canary-engine-proof.test.ts + + # The tag is the source of truth for what gets published, and this brings the repository's own copy in + # line with it afterwards. + # + # Both halves matter. Writing the version in CI is what lets a release happen with no pre-commit, which + # is what makes it work with branch protection. But only the published tarball gets its version from the + # tag: a git installation, an SBOM built from a checkout, `npm pack` from the repository and + # `--version` all read the committed manifest. For a package whose purpose is to shield known + # vulnerabilities, a manifest that names the wrong version means someone believing they have a fix they + # do not have — so the two must not be allowed to drift. + # + # A pull request rather than a push, because branch protection is the point of branch protection. Never + # auto-merged. + record-version: + name: Record the published version on main + # Depends on both, and gated on the PUBLISH result alone. + # + # No event guard, because the recommended release path runs `gh workflow run publish.yml`, which + # arrives as `workflow_dispatch` and not as `release` — a `release`-only condition would skip this on + # the path that publishes most versions while the workflow still reported success. + # + # `if: always()` with an explicit check on `needs.publish.result` rather than a bare `needs:`, because + # a failed VERIFICATION must not prevent recording. The version is on npm either way; the repository + # naming a different one is a second problem, not a safeguard. `always()` also means a dry-run + # dispatch — where `publish` is skipped, not successful — is correctly excluded by the condition. + if: always() && needs.publish.result == 'success' + needs: + - publish + - verify-published + runs-on: ubuntu-latest + permissions: + contents: write + pull-requests: write + # Required to dispatch CI on the branch below. Job-level `permissions` is a complete replacement + # rather than an addition — every scope not listed here is `none` — so without this the dispatch can + # only ever take its deliberately non-fatal warning path, and the branch would be left with no run at + # all while the workflow reported success. + actions: write + + env: + BRANCH: chore/record-published-version + VERSION: ${{ needs.publish.outputs.version }} + + steps: + - name: Checkout the default branch + uses: actions/checkout@v4 + with: + # Deliberately the default branch, not the tag: the proposal targets `main`, and checking out + # the tag would carry every difference between the tag and `main` into the diff. + ref: ${{ github.event.repository.default_branch }} + + - name: Setup Node + uses: actions/setup-node@v4 + with: + node-version: 24.x + cache: npm + + - name: Apply the version to the manifest and the lockfile + id: apply + run: | + set -euo pipefail + test -n "$VERSION" || { echo "::error::No published version was passed to this job."; exit 1; } + + before=$(node -p 'require("./package.json").version') + # `npm version` is what writes all three recorded copies (manifest, lockfile top level, lockfile + # root entry). Doing it by hand is how one of the three gets left behind. + npm version "$VERSION" --no-git-tag-version --allow-same-version >/dev/null + + echo "before=$before" >> "$GITHUB_OUTPUT" + if git diff --quiet -- package.json package-lock.json; then + echo "changed=false" >> "$GITHUB_OUTPUT" + echo "main already records $VERSION." + else + echo "changed=true" >> "$GITHUB_OUTPUT" + fi + + # Evidence for the reviewer, gathered here because it may not be available on the pull request + # itself: a pull request opened with GITHUB_TOKEN does not start workflow runs. The consistency + # invariant is the whole content of this change, so it is checked here and the result stated in the + # body rather than leaving a reviewer to approve two edited strings on trust. + # + # `continue-on-error`, because this check must not be able to prevent the pull request. A step that + # exits non-zero stops the job, and a later step whose condition contains no status function carries + # an implicit `success()` — between them enough to skip the recording at exactly the moment it + # matters. `steps.verify.outcome` carries the real result into the body, and the job is failed at the + # end, once the pull request exists. + - name: Check the invariant this change exists to satisfy + if: steps.apply.outputs.changed == 'true' + id: verify + continue-on-error: true + run: | + set -euo pipefail + npm ci + npm run build + npx vitest run tests/package-version.test.ts + + # `!cancelled()` rather than a bare condition: a condition without a status function is implicitly + # ANDed with `success()`, which would skip this step whenever the check above failed — precisely when + # the pull request matters most. + - name: Open or update the pull request + if: ${{ !cancelled() && steps.apply.outputs.changed == 'true' }} + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + TITLE: "Record published version ${{ env.VERSION }}" + run: | + set -euo pipefail + git config user.name 'github-actions[bot]' + git config user.email '41898282+github-actions[bot]@users.noreply.github.com' + + # The checkout fetched one ref, so on any run after the first there is no remote-tracking ref + # for this branch and a bare `--force-with-lease` would refuse the push as stale — opening the + # pull request once and never updating it again while still reporting success. + git fetch --depth=1 origin "+refs/heads/${BRANCH}:refs/remotes/origin/${BRANCH}" 2>/dev/null || true + + if git rev-parse --verify -q "refs/remotes/origin/$BRANCH" >/dev/null; then + lease="--force-with-lease=${BRANCH}:$(git rev-parse "refs/remotes/origin/${BRANCH}")" + else + # An empty expected value means "expect this branch not to exist" — the honest lease for a + # first push, and it still refuses to clobber a branch somebody created meanwhile. + lease="--force-with-lease=${BRANCH}:" + fi + + git checkout -B "$BRANCH" + git add package.json package-lock.json + git commit -m "$TITLE" -m "Prepared automatically after publishing v${VERSION}. Not auto-merged." + git push "$lease" origin "$BRANCH" + + { + echo "\`${VERSION}\` is published; \`main\` records \`${{ steps.apply.outputs.before }}\`." + echo + echo "The tag is the source of truth for what gets published. This brings the repository's own" + echo "copy in line, so that a git installation, an SBOM built from a checkout, \`npm pack\` and" + echo "\`patchstack-connect --version\` all report the released version." + echo + echo "Prepared by the \`Publish\` workflow. Two version strings and the lockfile entries \`npm" + echo "version\` derives from them; no source changes." + echo + echo "**Version consistency check: ${{ steps.verify.outcome }}** (run inside the workflow)." + echo + echo "**Published tarball verification: ${{ needs['verify-published'].result }}.**" + + # Said plainly and near the top of the body, because it changes what the reader should do + # rather than merely how they should feel. The tarball on npm cannot be unpublished; if the + # engine inside it does not block the exploit, the version needs deprecating and this pull + # request is not the urgent part. + if [ "${{ needs['verify-published'].result }}" != "success" ]; then + echo + echo "The published tarball did NOT pass its canary. \`${VERSION}\` is on npm and cannot be" + echo "unpublished — decide whether to deprecate it before treating this pull request as" + echo "routine. It is opened regardless, because a repository naming a version other than the" + echo "one that was released is a second problem rather than a safeguard against the first." + fi + if [ "${{ steps.verify.outcome }}" != "success" ]; then + echo + echo "The version surfaces disagreed even after applying \`${VERSION}\`. Read the workflow run" + echo "before merging: this pull request may not be sufficient on its own." + fi + echo + echo "A pull request opened with \`GITHUB_TOKEN\` does not start workflow runs, so the checks on" + echo "this pull request may be empty. Two things cover that: the invariant above was verified" + echo "inside the workflow, and CI was dispatched on this branch — a dispatched run proves the" + echo "branch is green but does not attach here, so it does not satisfy a required status." + echo "**Close and reopen this pull request** to get the required checks." + } > /tmp/pr-body.md + + cat /tmp/pr-body.md >> "$GITHUB_STEP_SUMMARY" + + existing=$(gh pr list --head "$BRANCH" --state open --limit 1 --json number --jq '.[0].number // empty') + + if [ -n "$existing" ]; then + # Edited in place: one pull request that stays accurate is readable, a new one per release is not. + gh pr edit "$existing" --title "$TITLE" --body-file /tmp/pr-body.md + echo "Updated #$existing." + else + gh pr create --title "$TITLE" --body-file /tmp/pr-body.md --head "$BRANCH" + fi + + # Start CI on the branch explicitly. A pull request opened with `GITHUB_TOKEN` does not start + # workflow runs, so without this the branch has no run at all and the only evidence is the + # in-workflow check above. + # + # Stated precisely because the distinction decides whether a human has to do something: a + # dispatched run does NOT attach to the pull request and so does NOT satisfy a required status. + # It proves the branch is green; closing and reopening the pull request is what produces the + # required checks. Never fatal — the version is already published and recorded by this point, + # and a dispatch that fails to start is not a reason to fail the release. + if gh workflow run ci.yml --ref "$BRANCH" 2>/dev/null; then + echo "Dispatched CI on $BRANCH (does not attach to the pull request)." >> "$GITHUB_STEP_SUMMARY" + else + echo "::warning::Could not dispatch CI on $BRANCH. The pull request has no run of its own; close and reopen it to get one." + fi + + # Last, so failing is the final act rather than something that prevents the work above: the release + # is loud about a real problem and the correction has still been proposed. + - name: Fail if the invariant did not hold + if: ${{ !cancelled() && steps.verify.outcome == 'failure' }} + run: | + echo "::error::The version surfaces disagree even after applying ${VERSION}. The recording pull request was opened anyway — read it and the run before merging." + exit 1 diff --git a/README.md b/README.md index 9331de33..b46abb9f 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,7 @@ patchstack-connect demo-guide node-serialize Read-only, state-aware instru local demo, including the next exact command, expected proof, and cleanup. patchstack-connect help Print help +patchstack-connect --version Print the installed version Options (for scan, setup, and status): --site-uuid Override the configured site UUID diff --git a/RELEASING.md b/RELEASING.md index abf82632..378f0299 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -2,12 +2,32 @@ The **git tag is the single source of truth** for a release. The `Publish` workflow reads the version from the tag (`v0.3.3` → `0.3.3`), writes it into -`package.json` in CI, then builds and publishes. Nothing is committed back to -`main`, so this works with branch protection and the tag can never disagree -with the published version. +`package.json` in CI, then builds and publishes. The tag can therefore never +disagree with the published version, and nothing has to be committed before a +release — which is what makes this work with branch protection. -`package.json`'s committed `version` is just a placeholder — it does not need to -be bumped before a release. +You do **not** bump `package.json` before releasing. After a successful publish, +the `Publish` workflow opens a pull request on `chore/record-published-version` +bringing `package.json` and `package-lock.json` up to the version that was just +published. Merge it; it is two version strings and the lockfile entries `npm +version` derives from them. + +That pull request is not bookkeeping. Five surfaces answer the question "which +version is this?" — the manifest, the two places the lockfile records it, the +tarball name `npm pack` derives, an SBOM built from a checkout, and +`patchstack-connect --version`. Only the published tarball gets its answer from +the tag; everything read out of the repository gets it from the committed +manifest. For a package whose purpose is to shield known vulnerabilities, a +manifest naming the wrong version is not untidiness: it is someone believing +they have a fix they do not have. Merging keeps the two in step. + +`tests/package-version.test.ts` pins the surfaces that can be checked from the +repository, and the consumer matrix +(`npm run test:consumers`) proves the installed binary reports the version npm +actually resolved. A pull request opened by `GITHUB_TOKEN` does not start +workflow runs, so `Publish` runs that test itself and states the result in the +pull request body; close and reopen the pull request if you want the full suite +to run on it. ## How to release (recommended) diff --git a/package-lock.json b/package-lock.json index c3b41825..d68659d9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@patchstack/connect", - "version": "0.3.6", + "version": "0.3.31", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@patchstack/connect", - "version": "0.3.6", + "version": "0.3.31", "license": "MIT", "bin": { "patchstack-connect": "dist/cli.js" @@ -16,7 +16,8 @@ "fastify": "^5.12.1", "tsup": "^8.0.0", "typescript": "^5.4.0", - "vitest": "^3.0.0" + "vitest": "^3.0.0", + "yaml": "^2.9.0" }, "engines": { "node": ">=18" @@ -2723,6 +2724,22 @@ "engines": { "node": ">=8" } + }, + "node_modules/yaml": { + "version": "2.9.0", + "resolved": "https://registry.npmjs.org/yaml/-/yaml-2.9.0.tgz", + "integrity": "sha512-2AvhNX3mb8zd6Zy7INTtSpl1F15HW6Wnqj0srWlkKLcpYl/gMIMJiyuGq2KeI2YFxUPjdlB+3Lc10seMLtL4cA==", + "dev": true, + "license": "ISC", + "bin": { + "yaml": "bin.mjs" + }, + "engines": { + "node": ">= 14.6" + }, + "funding": { + "url": "https://github.com/sponsors/eemeli" + } } } } diff --git a/package.json b/package.json index 6686cfb0..42542d1a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@patchstack/connect", - "version": "0.3.6", + "version": "0.3.31", "description": "Runtime application security for JavaScript and Node.js: dependency inventory, attack-surface mapping, and an in-process guard that virtually patches known vulnerabilities and hardens responses.", "keywords": [ "patchstack", @@ -92,7 +92,8 @@ "fastify": "^5.12.1", "tsup": "^8.0.0", "typescript": "^5.4.0", - "vitest": "^3.0.0" + "vitest": "^3.0.0", + "yaml": "^2.9.0" }, "repository": { "type": "git", diff --git a/scripts/compat-matrix.mjs b/scripts/compat-matrix.mjs index 58b9eec6..c181b302 100644 --- a/scripts/compat-matrix.mjs +++ b/scripts/compat-matrix.mjs @@ -11,7 +11,7 @@ // // Run: node scripts/compat-matrix.mjs [--manager npm|pnpm|yarn|bun] import { execFileSync } from 'node:child_process'; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync, existsSync, readdirSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync, existsSync, readdirSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; @@ -157,13 +157,28 @@ const SHAPES = [ check: (dir) => run(tsc(dir), ['-p', 'tsconfig.json'], dir), }, { - name: 'cli', why: 'the installed bin runs', + name: 'cli', why: 'the installed bin runs and reports its own version', pkg: { type: 'module' }, deps: [], check: (dir) => { - const out = run(bin(dir), ['--help'], dir); - if (!out.includes('patchstack-connect')) throw new Error(`bin produced no recognisable help:\n${out}`); + const help = run(bin(dir), ['--help'], dir); + if (!help.includes('patchstack-connect')) throw new Error(`bin produced no recognisable help:\n${help}`); - return out; + // The version a bug report is asked for. It has to come from the manifest npm resolved, so it is + // checked against the tarball that was just installed rather than against a constant: a build that + // baked in a stale value, or a manifest left out of `files`, both read as a working `--version`. + const reported = run(bin(dir), ['--version'], dir).trim(); + const installed = JSON.parse( + readFileSync(path.join(dir, 'node_modules', '@patchstack', 'connect', 'package.json'), 'utf8'), + ).version; + + if (reported !== installed) { + throw new Error(`--version reported "${reported}" but the installed manifest says "${installed}"`); + } + if (reported === 'unknown') { + throw new Error('--version could not read the manifest from the installed package'); + } + + return `${help}\n${reported}`; }, }, { diff --git a/src/cli.ts b/src/cli.ts index a16da6eb..641ba49b 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1,4 +1,5 @@ import { readFileSync, writeFileSync } from 'node:fs'; +import { createRequire } from 'node:module'; import { scanLockfile } from './parsers/index.js'; import { buildWirePayload } from './normalize.js'; @@ -135,6 +136,10 @@ Usage: the old one must be updated. Not usable in CI patchstack-connect help Print this message +Global: + --version Print the installed version of this package and exit + --help Print this help and exit + Options (for scan, setup, status, and uninstall): --site-uuid Override the configured site UUID --endpoint Override the API endpoint @@ -998,9 +1003,37 @@ async function runMarkBuild(args: ParsedArgs): Promise { return 0; } +/** + * The installed version of this package. + * + * Read from the manifest beside `dist/` rather than baked in at build time, so there is one source and it + * is the same one npm resolved. A reporter asked which version they are running needs the answer to come + * from the artifact they installed, not from a constant that a partial build could leave stale. + * + * Returns `unknown` rather than throwing: a version string is diagnostic, and no command should fail + * because it could not read one. + */ +function packageVersion(): string { + try { + const require = createRequire(import.meta.url); + const manifest = require('../package.json') as { version?: unknown }; + + return typeof manifest.version === 'string' && manifest.version.length > 0 ? manifest.version : 'unknown'; + } catch { + return 'unknown'; + } +} + async function main(): Promise { const args = parseArgs(process.argv); + // Before help, and before any command: `--version` is what a bug report is asked for, so it must work + // even when the rest of the arguments are wrong. + if (args.flags.has('version') || args.command === 'version') { + console.log(packageVersion()); + return 0; + } + if (args.flags.has('help') || args.command === 'help' || args.command === null) { console.log(HELP); return 0; diff --git a/tests/package-version.test.ts b/tests/package-version.test.ts new file mode 100644 index 00000000..abf062ed --- /dev/null +++ b/tests/package-version.test.ts @@ -0,0 +1,122 @@ +import { describe, it, expect } from 'vitest'; +import { execFileSync, spawnSync } from 'node:child_process'; +import { copyFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// The version this package reports is answered by five different surfaces: `package.json`, the two places +// the lockfile records it, the tarball name `npm pack` derives, the SBOM, and `--version`. They are five +// readings of one fact, and a consumer asked "which version are you running" gets whichever one is nearest +// to hand. For a security package the answer decides whether someone believes they have a fix they do not +// have, so a surface that disagrees is worse than one that is missing. +// +// These are structural checks over the committed files, so they run on every Node version and in the +// publish validation, not as one CI step on one platform. What they cannot see is the *installed* package — +// that link (the bin reporting the version npm actually resolved) is proven against a real install by +// `scripts/compat-matrix.mjs`, because nothing in this repository can stand in for a tarball. + +const url = (p: string) => fileURLToPath(new URL(p, import.meta.url)); +const read = (p: string) => JSON.parse(readFileSync(url(p), 'utf8')); + +const manifest = read('../package.json'); +const lock = read('../package-lock.json'); + +describe('the version is one fact', () => { + it('is a plain release version in the manifest', () => { + // Not merely valid semver: the committed version tracks a RELEASE, so a lingering prerelease or build + // suffix means an interrupted release process left the repository describing something unpublished. + expect(manifest.version).toMatch(/^\d+\.\d+\.\d+$/); + }); + + it('agrees between the manifest and both places the lockfile records it', () => { + // `npm version` writes all three; a hand-edit writes one. The lockfile's root entry is the copy that + // ends up in a consumer's tree, and it is the easiest of the three to forget. + expect({ lockTop: lock.version, lockRoot: lock.packages['']?.version }).toEqual({ + lockTop: manifest.version, + lockRoot: manifest.version, + }); + }); + + it('agrees on the package name too', () => { + expect({ lockTop: lock.name, lockRoot: lock.packages['']?.name }).toEqual({ + lockTop: manifest.name, + lockRoot: manifest.name, + }); + }); + + it('is the version `npm pack` will name the tarball after', () => { + // `npm pack` derives the filename from the manifest, so this cannot drift while the manifest is the + // source — the assertion pins the derivation itself, which a change to `name` or `publishConfig` could + // otherwise move without anyone noticing the tarball no longer matches what the registry serves. + const scopeless = String(manifest.name).replace('@', '').replace('/', '-'); + expect(`${scopeless}-${manifest.version}.tgz`).toBe(`patchstack-connect-${manifest.version}.tgz`); + }); + + it('is what the built CLI reports for --version', () => { + const cli = url('../dist/cli.js'); + // `dist/` is gitignored, so a fresh checkout has nothing to run. CI builds before it tests. + if (!existsSync(cli)) return; + + // Deliberately NOT guarded on dist being newer than the sources, unlike the other dist-dependent + // tests: the CLI reads the manifest at run time rather than having a version baked in at build time, + // so even a stale build must report the current version. If this ever needs a staleness guard, the + // version has been baked in — which is the defect. + const reported = execFileSync(process.execPath, [cli, '--version'], { encoding: 'utf8' }).trim(); + + // The message names the likely cause, because the failure it produces is otherwise puzzling: a `dist/` + // built from a revision without this flag falls through to the help text, so the assertion reports a + // page of usage against a version string. + expect( + reported, + reported.includes('Usage:') ? 'dist/ predates `--version` — run `npm run build`' : 'wrong version reported', + ).toBe(manifest.version); + }); + + it('reports `unknown` rather than failing when the manifest carries no version', () => { + // The fallback, actually exercised. Asserting that the NORMAL invocation is not `unknown` would say + // nothing about the fallback at all — it would pass whether the fallback worked, threw, or printed an + // empty line. + // + // So the CLI is run somewhere its manifest lookup resolves to a real file with no `version` in it: + // `/sub/cli.mjs` reads `/package.json`. The `.mjs` extension matters — a bare `.js` outside + // a `"type": "module"` package is parsed as CommonJS and would fail on the bundle's own syntax, which + // is a different failure wearing the same exit code. + const cli = url('../dist/cli.js'); + if (!existsSync(cli)) return; + + const dir = mkdtempSync(join(tmpdir(), 'ps-version-')); + mkdirSync(join(dir, 'sub')); + writeFileSync(join(dir, 'package.json'), JSON.stringify({ name: 'not-connect', type: 'module' })); + copyFileSync(cli, join(dir, 'sub', 'cli.mjs')); + + const run = spawnSync(process.execPath, [join(dir, 'sub', 'cli.mjs'), '--version'], { encoding: 'utf8' }); + rmSync(dir, { recursive: true, force: true }); + + // Exit 0 as well as the right text: a diagnostic string is never worth failing a command over, and a + // non-zero exit here would break `--version` in exactly the situation someone runs it — when something + // about the installation is already wrong. + expect({ status: run.status, stdout: run.stdout.trim() }).toEqual({ status: 0, stdout: 'unknown' }); + }); + + it('reports `unknown` rather than failing when there is no manifest at all', () => { + // The other half, and the one that reaches the `catch`. The test above finds a real manifest that + // simply has no `version`, which is handled by a type check and never throws — so on its own it leaves + // the catch block untested, and a fallback that rethrew or printed an empty line would pass it. + // + // Here NO `package.json` is written beside the copy. The lookup is a relative specifier, which + // resolves exactly one path and does not walk up the tree, so it cannot find a real manifest from an + // ancestor directory: it throws, which is the path being covered. + const cli = url('../dist/cli.js'); + if (!existsSync(cli)) return; + + const dir = mkdtempSync(join(tmpdir(), 'ps-version-')); + mkdirSync(join(dir, 'sub')); + copyFileSync(cli, join(dir, 'sub', 'cli.mjs')); + + const run = spawnSync(process.execPath, [join(dir, 'sub', 'cli.mjs'), '--version'], { encoding: 'utf8' }); + rmSync(dir, { recursive: true, force: true }); + + expect({ status: run.status, stdout: run.stdout.trim() }).toEqual({ status: 0, stdout: 'unknown' }); + }); +}); diff --git a/tests/workflow-shell.test.ts b/tests/workflow-shell.test.ts new file mode 100644 index 00000000..ab3ee2a9 --- /dev/null +++ b/tests/workflow-shell.test.ts @@ -0,0 +1,188 @@ +import { describe, it, expect } from 'vitest'; +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { parse } from 'yaml'; + +/** + * Every shell script embedded in a workflow has to parse. + * + * A `run:` block is a program that nothing checks. The compiler does not see it, the type checker does not + * see it, and — this is the part that matters — CI usually does not either, because a job only runs on the + * events it is configured for. A release-only job's script is first executed during a release, which is + * the worst available moment to discover an unterminated quote. + * + * So this parses the workflows and puts every bash script through `bash -n`. It is a syntax check and + * nothing more: it cannot tell whether a script does the right thing, only that it is a program. That is + * enough to catch the class of error which is otherwise invisible until it fires. + * + * `bash -n` accepts `${{ … }}` as ordinary text, so the scripts are checked as written rather than with + * expressions substituted — which keeps this from disagreeing with what the runner actually receives. + */ +const workflowDir = fileURLToPath(new URL('../.github/workflows/', import.meta.url)); + +type Step = { run?: unknown; shell?: unknown; uses?: unknown; name?: unknown; if?: unknown; 'continue-on-error'?: unknown }; +type Job = { steps?: Step[]; defaults?: { run?: { shell?: unknown } }; 'runs-on'?: unknown }; +type Workflow = { jobs?: Record; defaults?: { run?: { shell?: unknown } } }; + +/** + * The shell a step will actually run under, following the same precedence the runner uses: the step, then + * the job, then the workflow, then the platform default — `pwsh` on Windows and `bash` everywhere else. + */ +function shellFor(workflow: Workflow, job: Job, step: Step): string { + const declared = step.shell ?? job.defaults?.run?.shell ?? workflow.defaults?.run?.shell; + if (typeof declared === 'string') return declared; + + const runsOn = JSON.stringify(job['runs-on'] ?? ''); + + return /windows/i.test(runsOn) ? 'pwsh' : 'bash'; +} + +type Script = { file: string; job: string; step: string; shell: string; script: string }; + +function collectScripts(): Script[] { + const out: Script[] = []; + + for (const file of readdirSync(workflowDir).filter((f) => /\.ya?ml$/.test(f))) { + const workflow = parse(readFileSync(join(workflowDir, file), 'utf8')) as Workflow; + + for (const [jobName, job] of Object.entries(workflow.jobs ?? {})) { + for (const [index, step] of (job.steps ?? []).entries()) { + if (typeof step.run !== 'string') continue; + out.push({ + file, + job: jobName, + step: typeof step.name === 'string' ? step.name : `step ${index + 1}`, + shell: shellFor(workflow, job, step), + script: step.run, + }); + } + } + } + + return out; +} + +/** + * Steps that must run even when an earlier step in their job has failed. + * + * GitHub ANDs an implicit `success()` into any step condition that contains no status-check function. So a + * step whose `if` reads only `steps.x.outputs.y == 'true'` is silently also "and nothing has failed yet" — + * which is the opposite of what these two need. They exist to record and report a state that has already + * happened, and a failure earlier in the job is exactly when that recording matters. + * + * Named individually rather than checked as a rule, because "must run on failure" is a property of a + * step's purpose and not something derivable from its text. + */ +const MUST_SURVIVE_FAILURE = [ + { workflow: 'publish.yml', job: 'record-version', step: 'Open or update the pull request' }, + { workflow: 'publish.yml', job: 'record-version', step: 'Fail if the invariant did not hold' }, +]; + +describe('steps that must not be skipped by an implicit success()', () => { + it.each(MUST_SURVIVE_FAILURE)('$step keeps a status function in its condition', ({ workflow, job, step }) => { + const parsed = parse(readFileSync(join(workflowDir, workflow), 'utf8')) as Workflow; + const found = (parsed.jobs?.[job]?.steps ?? []).find((s) => s.name === step); + + expect(found, `${workflow} › ${job} › ${step} not found — renamed?`).toBeDefined(); + + const condition = String((found as { if?: unknown }).if ?? ''); + // `cancelled()`, `always()` or `failure()` — any of them suppresses the implicit `success()`. + expect(condition, `condition was: ${condition || '(none)'}`).toMatch(/cancelled\(\)|always\(\)|failure\(\)/); + }); + + it('actually fails the job when it reaches the final step', () => { + // The structural assertions above prove the step RUNS after a failed check. They say nothing about + // whether it fails anything, and a step that runs and exits zero leaves a green workflow over a + // release whose surfaces disagree — which is the state this arrangement exists to make loud. + // + // So the step's own script is executed. `${{ … }}` interpolations are left as written, because bash + // treats them as text and substituting them would test something other than what the runner receives. + const parsed = parse(readFileSync(join(workflowDir, 'publish.yml'), 'utf8')) as Workflow; + const step = (parsed.jobs?.['record-version']?.steps ?? []).find( + (s) => s.name === 'Fail if the invariant did not hold', + ); + + expect(step, 'the failing step is gone or renamed').toBeDefined(); + + const dir = mkdtempSync(join(tmpdir(), 'ps-workflow-fail-')); + const path = join(dir, 'fail.sh'); + writeFileSync(path, String((step as { run?: unknown }).run)); + const run = spawnSync('bash', [path], { encoding: 'utf8' }); + rmSync(dir, { recursive: true, force: true }); + + expect(run.status, `script exited ${run.status}; it must be non-zero:\n${(step as { run?: unknown }).run}`) + .not.toBe(0); + }); + + it('leaves the verification step free to fail without stopping the job', () => { + // The other half. Without `continue-on-error` the job stops at that step whatever the conditions below + // say, and the pull request is never opened. + const parsed = parse(readFileSync(join(workflowDir, 'publish.yml'), 'utf8')) as Workflow; + const verify = (parsed.jobs?.['record-version']?.steps ?? []).find( + (s) => s.name === 'Check the invariant this change exists to satisfy', + ); + + expect((verify as { 'continue-on-error'?: unknown })?.['continue-on-error']).toBe(true); + }); +}); + +describe('embedded workflow shell scripts', () => { + const scripts = collectScripts(); + + it('finds the scripts to check', () => { + // Without this the suite would pass by finding nothing — after a rename of the workflow directory, or + // a parser change that silently returned no jobs. + expect(scripts.length).toBeGreaterThan(10); + }); + + it('covers the jobs that only ever run on a release', () => { + // The specific reason this file exists. None of these run on a pull request, so their scripts are never + // executed by the checks that gate a merge — the first time they run is during a release. Naming them + // means a rename or a split cannot quietly drop one from coverage. + const jobs = new Set(scripts.map((s) => s.job)); + + expect([...jobs]).toEqual( + expect.arrayContaining(['publish', 'verify-published', 'record-version']), + ); + }); + + it('states which scripts it does not check, rather than skipping them silently', () => { + // `bash -n` cannot check PowerShell, and `pwsh` is not present on every machine that runs this suite. + // Pinning the exact list is what keeps the gap honest: a bash script that silently acquired a + // `shell: pwsh` — or a new Windows job — would drop out of coverage without this failing. + const unchecked = scripts + .filter((s) => s.shell !== 'bash' && s.shell !== 'sh') + .map((s) => `${s.file} › ${s.job} › ${s.step} (${s.shell})`); + + expect(unchecked).toEqual([ + 'ci.yml › windows-smoke › Install dependencies (pwsh)', + 'ci.yml › windows-smoke › Consumer shapes against a packed tarball (pwsh)', + ]); + }); + + it('parses every bash script', () => { + const bash = scripts.filter((s) => s.shell === 'bash' || s.shell === 'sh'); + expect(bash.length).toBeGreaterThan(10); + + const dir = mkdtempSync(join(tmpdir(), 'ps-workflow-')); + const broken: string[] = []; + + for (const [index, entry] of bash.entries()) { + const path = join(dir, `script-${index}.sh`); + writeFileSync(path, entry.script); + const check = spawnSync('bash', ['-n', path], { encoding: 'utf8' }); + + if (check.status !== 0) { + broken.push( + `${entry.file} › ${entry.job} › ${entry.step}\n${check.stderr.replaceAll(path, '