From 79281aa2e3493366fc9e5895055189d14049e131 Mon Sep 17 00:00:00 2001 From: Bradenream <51544548+Bradenream@users.noreply.github.com> Date: Tue, 1 Sep 2026 19:29:19 +0000 Subject: [PATCH] ci: gate every pull request on build, vet, format and behaviour (COR-13656) (#22) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why Nothing gates a pull request in this repo. The single workflow, `release.yaml`, fires only on `push: tags: v*` — so **no build, no vet, no test, and no lint has ever run on a PR or on master.** The only check today is Graphite's mergeability check. That has already cost us twice: - **Master once did not compile from a clean checkout** — `go.sum` was incomplete. Nobody noticed until someone tried to build it fresh. - **`v0.232.0` failed at release time** and left six orphaned npm packages behind, from code that had merged cleanly days earlier. Neither defect was exotic. There was simply nothing to catch it. ## What this adds Three jobs, on `pull_request` and on push to `master`: | Job | Runs | |---|---| | **go** | `go mod verify`, gofmt check, `go build ./...`, `go vet ./...`, `go test ./...` | | **cli** | builds `./vf`, then the hermetic behaviour suite | | **lint** | `oxlint` | ## Every gate was verified by reintroducing the defect it catches Not by assuming it would work: | Defect reintroduced | Result | |---|---| | Removed a required module from `go.sum` | `go build` **fails** | | Removed a persisted edit **and its call site** | `go build` **passes** — `yarn test` **fails** | | Added unformatted Go | gofmt check **fails** | | Added a bad `Printf` verb | `go vet` **fails** | **The second row is the point of this PR.** This CLI is generated by Speakeasy, and a handful of hand-written files ride across regenerations as persistent edits. A regeneration that drops one still compiles — nothing looks wrong. Deleting `internal/cli/docs.go` together with its `initDocsCmd` registration builds perfectly clean and fails six behaviour tests. **A build-only gate would have shipped it.** That is why the behaviour job is not redundant with the build job. ## The integration tests now exclude themselves instead of failing `test/vf-*.test.ts` drive the real API. Without credentials, `createProject` sent `--workspace-id=undefined`, so `yarn test` reported **11 failures** for anyone without a token — including CI. A suite that is red by default carries no signal, and it meant the hermetic tests had no gate they could run behind. `vitest.config.mts` now resolves credentials up front and excludes those files when absent, warning that it did so. Verified in both directions: - no credentials → **5 files, 49 tests, all passing** - credentials present → **21 files collected**, integration included again Side effect worth having: `yarn test` now works on a fresh clone. ## The workflow caught two bugs in itself Both fixed here, and worth reading as evidence the gate works. **1. Yarn 3 ignores `setup-node`'s `.npmrc`.** The first run failed at install: ``` Request URL: https://registry.yarnpkg.com/@voiceflow/oxlint-config/-/oxlint-config-1.2.0.tgz Response Code: 404 ``` `NODE_AUTH_TOKEN` and `NPM_CONFIG_USERCONFIG` were both set and both ignored — yarn went to its own default registry unauthenticated. Yarn reads `YARN_NPM_*` instead, so that is what the workflow passes now. **2. `oxfmt --check` reports 193 of 204 files as unformatted.** Formatting has never run in this repo, and the overwhelming majority of those files are Speakeasy output. Reformatting them produces a diff the next regeneration silently undoes; reformatting only the hand-written files still means touching every test file at once, which would collide with the open flag-ergonomics PR. **The format gate is left off rather than left permanently red** — which paths this repo wants to own is a decision worth making deliberately, not one to smuggle in behind a CI change. `oxlint` itself passes (1 warning, 0 errors across 35 files), so it is a real blocking gate. **All three jobs block; the final run is fully green.** ## Follow-ups worth a ticket - **`NPM_TOKEN` is publish-capable** and is now exposed to PR CI for dependency installation. A read-only automation token would be the right scope. (Fork PRs don't receive secrets, so the exposure is limited to branch PRs.) - **A format gate.** Decide which paths to own (`test/`, `scripts/`, configs), reformat them once, then add `oxfmt --check` scoped to those paths. - **`cmd/gendocs` is invoked by nothing**, so `docs/` is stale by construction — the repo's own test says so at `test/rewrite-docs.test.ts:84`. - **Releases are hand-tagged.** Merges reach users only when someone remembers to cut a tag. --- .github/workflows/ci.yaml | 162 ++++++++++++++++++++++++++++++++++++++ vitest.config.mts | 41 ++++++++++ 2 files changed, 203 insertions(+) create mode 100644 .github/workflows/ci.yaml diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml new file mode 100644 index 0000000..fce6b28 --- /dev/null +++ b/.github/workflows/ci.yaml @@ -0,0 +1,162 @@ +# Gates every pull request and every push to master. +# +# Until this existed, nothing ran on a PR — not a build, not a test, not vet. +# The only check was Graphite's mergeability check, and it cost the repo twice: +# a master that did not compile from a clean checkout (go.sum was missing +# spyzhov/ajson), and v0.232.0, which failed at release time and left six +# orphaned npm packages behind. Both were introduced by code that had merged +# cleanly days earlier, and both are caught by the jobs below. +# +# What this protects that is specific to this repo: the CLI is generated by +# Speakeasy, and a handful of hand-written files are carried across +# regenerations as persistent edits. A regeneration that silently drops one +# would still compile — nothing would look wrong. The behaviour tests exercise +# every one of those edits, so a dropped edit fails here instead of shipping. + +name: CI + +on: + pull_request: + push: + branches: [master] + +concurrency: + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + go: + name: Go + runs-on: ubuntu-latest + env: + GOFLAGS: -mod=readonly + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + cache: true + + # Catches an incomplete go.sum, which is what made master uncompilable + # from a clean checkout. A warm local module cache hides this; CI cannot. + # + # -mod=readonly is already the default from Go 1.16, and removing a + # required module from go.sum does make `go build ./...` fail here — but + # relying on a default for a security-relevant guarantee is how it quietly + # stops holding. Set it explicitly, and assert afterwards that nothing in + # this job rewrote the committed dependency metadata. + - name: Verify module integrity + run: go mod verify + + # No vendor/ filter: this repo has no vendored code, and `gofmt -l .` + # emits paths without a leading ./ anyway, so the filter it replaced was + # guarding against nothing. If vendoring is ever added, this will say so. + - name: Check formatting + run: | + unformatted=$(gofmt -l .) + if [ -n "$unformatted" ]; then + echo "::error::gofmt would reformat these files:" + echo "$unformatted" + exit 1 + fi + + - name: Build + run: go build ./... + + - name: Vet + run: go vet ./... + + - name: Unit tests + run: go test ./... + + # If any step above rewrote go.mod or go.sum, what is committed was + # incomplete — fail rather than pass on a state no reviewer approved. + - name: Dependency metadata is unchanged + run: git diff --exit-code go.mod go.sum + + cli: + name: CLI behaviour + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + cache: true + + # The behaviour tests shell out to ./vf, so the binary has to exist at the + # repo root under that exact name before vitest starts. + - name: Build the CLI + run: go build -o vf ./cmd/vf + + - uses: actions/setup-node@v4 + with: + node-version: 20 + registry-url: https://registry.npmjs.org + + # Two devDependencies (@voiceflow/oxlint-config, @voiceflow/oxfmt-config) + # are private, so an unauthenticated install cannot resolve the lockfile + # even though nothing in the test path uses them. + - name: Install dependencies + run: yarn install --immutable + env: + # Must be YARN_NPM_*, not NODE_AUTH_TOKEN. Yarn 3 does not read the + # .npmrc that actions/setup-node writes — the first version of this + # workflow set NODE_AUTH_TOKEN, and yarn went to its own default + # registry unauthenticated and 404'd on both private packages. + YARN_NPM_REGISTRY_SERVER: https://registry.npmjs.org + YARN_NPM_ALWAYS_AUTH: "true" + YARN_NPM_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} + + # test/vf-*.test.ts create and delete real projects, so they need a token + # and a workspace this job does not have. Skipping them is opted into + # explicitly — vitest.config.mts fails rather than quietly running a + # subset — which leaves the hermetic suite: mock server, --dry-run, --help. + # + # Worth revisiting: with a scoped token and a throwaway workspace as + # secrets, CI could run the integration suite too. Nothing covers it today. + - name: Behaviour tests + run: yarn test + env: + VF_SKIP_INTEGRATION_TESTS: "1" + + lint: + name: Lint and format + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: 20 + registry-url: https://registry.npmjs.org + + - name: Install dependencies + run: yarn install --immutable + env: + # Must be YARN_NPM_*, not NODE_AUTH_TOKEN. Yarn 3 does not read the + # .npmrc that actions/setup-node writes — the first version of this + # workflow set NODE_AUTH_TOKEN, and yarn went to its own default + # registry unauthenticated and 404'd on both private packages. + YARN_NPM_REGISTRY_SERVER: https://registry.npmjs.org + YARN_NPM_ALWAYS_AUTH: "true" + YARN_NPM_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} + + # Blocking: verified green on this workflow's own first run (1 warning, + # 0 errors) before the continue-on-error escape hatch was removed. + - name: Lint + run: yarn oxlint + + # No format check here, deliberately. `oxfmt --check` reports 193 of 204 + # files as unformatted: formatting has never run in this repo, and the + # overwhelming majority of those files are Speakeasy output. Reformatting + # them would produce a diff that the next regeneration silently undoes, + # and reformatting only the hand-written files still means touching every + # test file at once. That is a decision about which paths this repo wants + # to own, not something to smuggle in behind a CI change — so the gate is + # left off rather than left permanently red. diff --git a/vitest.config.mts b/vitest.config.mts index 6c27fa2..c529167 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -1,9 +1,50 @@ import baseConfig from '@voiceflow/vitest-config/unit'; +import dotenv from 'dotenv'; import { mergeConfig, type ViteUserConfig } from 'vitest/config'; +// Two kinds of test live in test/: +// +// test/vf-*.test.ts drive the real API — they create and delete projects +// everything else hermetic: a local mock server, --dry-run, or --help +// +// The integration tests used to FAIL rather than skip when no credentials were +// present, because createProject sent `--workspace-id=undefined`. That made +// `yarn test` red for anyone without a token — including CI — so a red suite +// carried no information and the hermetic tests had no gate. +// +// Running a subset must never be the silent default: a suite that quietly stops +// covering what it was written to cover is worse than one that fails. So the +// absence of credentials is an error, and dropping the integration tests has to +// be asked for explicitly. CI asks for it, in the workflow, where it is visible. +// +// Credentials are resolved here rather than in test/setup.ts because setupFiles +// run after the config is built, which is too late to choose what to include. +dotenv.config({ path: '.env.test' }); + +const hasCredentials = Boolean(process.env.VF_TOKEN && process.env.VF_WORKSPACE_ID); +const integrationOptOut = process.env.VF_SKIP_INTEGRATION_TESTS === '1'; + +if (!hasCredentials && !integrationOptOut) { + throw new Error( + [ + 'test/vf-*.test.ts need VF_TOKEN and VF_WORKSPACE_ID — they create and delete real projects.', + '', + ' To run everything: add both to .env.test', + ' To run only the rest: VF_SKIP_INTEGRATION_TESTS=1 yarn test', + ].join('\n'), + ); +} + +const runIntegration = hasCredentials && !integrationOptOut; + +if (!runIntegration) { + console.warn('\n Running without test/vf-*.test.ts (VF_SKIP_INTEGRATION_TESTS=1).\n'); +} + export default mergeConfig(baseConfig, { test: { include: ['test/**/*.test.ts'], + exclude: runIntegration ? [] : ['test/vf-*.test.ts'], setupFiles: ['test/setup.ts'], testTimeout: 30000, },