From 80f6b27d614dd25bd93a1d0fe35267fe7ca7c5b6 Mon Sep 17 00:00:00 2001 From: Scott Lowe Date: Sat, 3 Oct 2026 00:27:44 -0700 Subject: [PATCH] ci: validate every org's resources on every pull request Nothing ran `npm run validate` before merge. A config that `apply` refuses (name length, structured-output lockstep, duplicated prompts, the maxTokens floor, voice schema) could merge green, and deploys and promotion out of main then stopped until a fix landed. Plain `push` only warns, and can fail partway with an API 400. - ci.yml gets a Validate resources job: validate for every folder under resources/, reporting every failing org rather than stopping at the first. validate makes no network call; the engine's config only needs a key to be set, so the step sets a placeholder that is never sent. The job has no secrets, so forks get it too. No engine change. - tests/ci-validate-workflow.test.ts runs the step itself against fixture orgs: no orgs, all valid, one invalid org among valid ones, an invalid folder name, and no secrets or persisted credentials. - README, AGENTS.md (change loop), the workflows, PR checks and troubleshooting guides, and improvements.md #37 describe it. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 54 ++++++++++ AGENTS.md | 5 +- README.md | 4 +- docs/guides/pr-checks.md | 3 + docs/guides/troubleshooting.md | 19 ++++ docs/guides/workflows.md | 6 ++ improvements.md | 53 ++++++++++ tests/ci-validate-workflow.test.ts | 155 +++++++++++++++++++++++++++++ 8 files changed, 297 insertions(+), 2 deletions(-) create mode 100644 tests/ci-validate-workflow.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 231df69..29081ce 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -64,3 +64,57 @@ jobs: - name: Lint workflows run: ./actionlint -color + + validate: + name: Validate resources + runs-on: ubuntu-latest + timeout-minutes: 10 + + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + + - uses: actions/setup-node@v4 + with: + node-version: 22 + cache: npm + + - run: npm ci + + # The same checks `apply` runs before every deploy, here before merge: + # a config that `apply` would refuse never reaches main, where it would + # block deploys and promotion until someone noticed. Every org is + # validated, including resources no PR check targets. + # + # `validate` makes no network call, but loading the engine's config + # needs a key to be set. The placeholder below is never sent anywhere, + # and no secret is available to this job, so it runs the same on forks. + - name: Validate every org + shell: bash + env: + VAPI_PRIVATE_API_KEY: validate-only-never-sent + run: | + set -euo pipefail + shopt -s nullglob + orgs=() + for dir in resources/*/; do + orgs+=("$(basename "$dir")") + done + if (( ${#orgs[@]} == 0 )); then + echo "No org folders under resources/; nothing to validate." + exit 0 + fi + failed=() + for org in "${orgs[@]}"; do + echo "::group::Validate ${org}" + if ! node --import tsx src/validate-cmd.ts "$org"; then + failed+=("$org") + fi + echo "::endgroup::" + done + if (( ${#failed[@]} > 0 )); then + echo "::error::Validation failed for: ${failed[*]}. Run \`npm run validate -- \` locally to see each finding." + exit 1 + fi + echo "Validated ${#orgs[@]} org(s): ${orgs[*]}" diff --git a/AGENTS.md b/AGENTS.md index aa5e47f..e30d589 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -130,7 +130,10 @@ precisely. 2. **Edit the files** under `resources//`. Settings and examples: [resource reference](docs/guides/resource-reference.md); tested files to copy from: [`examples/starter/`](examples/starter/README.md). -3. **Validate:** `npm run validate -- ` (offline). +3. **Validate:** `npm run validate -- ` (offline). CI's **Validate + resources** check runs it for every org on every PR; if that check fails, + run it locally for the org it names and fix the errors. Don't weaken the + check or the workflow to get past it. 4. **Build PR checks offline** if `vapi-checks.yml` exists: `npm run check -- --all --dry-run`. Fix anything it reports. 5. **Deploy only with a yes** (safety rule 1): `npm run apply -- `, or diff --git a/README.md b/README.md index c61b988..0fb25a6 100644 --- a/README.md +++ b/README.md @@ -139,7 +139,9 @@ npm run apply -- my-org # pull the latest, merge, push ``` Commit the changed files and `.vapi-state.my-org.json` so your team shares the -same name → UUID mappings. +same name → UUID mappings. Every pull request runs the same validation for +every org in CI (the **Validate resources** check), so a config `apply` would +refuse fails before it merges. ### 5. Test it diff --git a/docs/guides/pr-checks.md b/docs/guides/pr-checks.md index 8253f8a..fdfa4ff 100644 --- a/docs/guides/pr-checks.md +++ b/docs/guides/pr-checks.md @@ -114,6 +114,9 @@ PR push. It asks for `statuses: write` only to post the direct links. `promotion.yml`, the engine (`src/**`, `package*.json`), nor the check's own `paths` skip it, and `Vapi Evals` posts success. - A newer push cancels the older run. +- Separately, the **Validate resources** check (in `ci.yml`) runs + `npm run validate` on every org, including resources no check targets. + It's offline and runs whether or not PR checks are turned on. ## 7. Make it required (after a burn-in) diff --git a/docs/guides/troubleshooting.md b/docs/guides/troubleshooting.md index a8a7f6d..030238c 100644 --- a/docs/guides/troubleshooting.md +++ b/docs/guides/troubleshooting.md @@ -81,3 +81,22 @@ falling through to a full deploy. Pass either: - a resource type — `npm run push -- my-org assistants`, or - a path — `npm run push -- my-org assistants/foo.yml` (short form) or `npm run push -- my-org resources/my-org/assistants/foo.yml` (long form). + +## "Validate resources" fails in CI + +The check runs `npm run validate` for every org under `resources/`. The +job log names each failing org; run the same command locally to see its +findings: + +```bash +npm run validate -- +``` + +Each error names the file, field and rule. Plain `push` only warns about +these errors, so a repository that has been deploying with `push` can carry +some from before the check existed; they show up on the next pull request, +whatever it changes. Fix them in that PR or a separate one first. `apply` +refuses to deploy until they're fixed anyway. + +A folder under `resources/` that isn't a valid org name (lowercase letters, +digits and hyphens) fails too. Rename it, or move it out of `resources/`. diff --git a/docs/guides/workflows.md b/docs/guides/workflows.md index 14660d2..b6a7f63 100644 --- a/docs/guides/workflows.md +++ b/docs/guides/workflows.md @@ -17,6 +17,12 @@ npm run validate -- npm run apply -- ``` +CI runs the same validation on every pull request, for every org under +`resources/` (the **Validate resources** check in `.github/workflows/ci.yml`). +It needs no secrets, so it runs on forks too. Make it a required check in +branch protection, so a config that `apply` would refuse can't reach +`main`, where it would block deploys and promotion. + To deploy only some resources, pass resource types or file paths. `apply` and `push` accept the same scoping: diff --git a/improvements.md b/improvements.md index 7bfad7d..dda040a 100644 --- a/improvements.md +++ b/improvements.md @@ -88,6 +88,7 @@ you which stack PR closes the row.** | 34 | No pre-merge simulation signal; simulations only tested what was deployed | A PR that breaks an agent merges green | #33 | RESOLVED 2026-10-01 | | 35 | A failed promotion pushed nothing, not even state | git lost track of resources already on the platform | None | RESOLVED 2026-10-01 | | 36 | `cleanup` deletes resources excluded by `.vapi-ignore` | A destructive cleanup can delete resources another team owns | None | RESOLVED 2026-10-03 | +| 37 | Resource validation ran only at deploy time, after merge | A config `apply` refuses could merge and block deploys and promotion | #32 | RESOLVED 2026-10-03 | **Active backlog after cleanup:** `#2`, `#6`, `#8`, `#12`, `#20`, `#24–#26`, `#31`, and the open remainder of `#27` (wiring the listing-completeness verdict into push/delete/audit, and moving `cleanup.ts` onto the shared pager). Resolved entries stay in this file as historical incident notes per the maintenance directive; stale superseded backlog rows are not duplicated. @@ -1933,6 +1934,58 @@ orphan; after it, only the orphan. --- +## 37. Resource validation ran only at deploy time, after merge + +**[RESOLVED 2026-10-03]** + +**Discovered:** 2026-10-03, while reviewing which static checks run before +the PR check's simulations. + +### Problem + +`npm run validate` catches the shapes the API rejects (name length, +structured-output lockstep, duplicated prompts, the `maxTokens` floor, +per-provider voice schema), but nothing ran it before merge. A config that +`apply` refuses could land on `main`, and was found only when someone +deployed or promoted it. + +### Current behavior (Verified) + +- `src/apply.ts` runs `validate` before every deploy and stops on errors. + Promotion deploys through `apply`, so it stops too, but only after the + change merged. +- `src/push.ts` runs the same validators but only warns unless `--strict`. +- `ci.yml` ran the build and tests only. `tests/examples.test.ts` + validates `examples/`, not `resources//`. +- The PR check's payload build (`npm run check -- --dry-run`) covers only + the resources its targets reach, and only in repos that turned PR checks on. + +### Risk + +A broken config merges green. Deploys and promotion out of `main` then stop +until a fix PR lands, or, with plain `push`, the push continues and fails +partway with an API 400. + +### Current mitigation + +None needed once the fix below lands. + +### Possible fix (landed) + +A **Validate resources** job in `.github/workflows/ci.yml` runs `validate` +for every folder under `resources/` on every pull request, reporting every +failing org rather than stopping at the first. `validate` makes no network +call, but loading the engine's config requires a key, so the step sets a +placeholder that is never sent; the job has no secrets, so it runs on forks. +No engine change. `tests/ci-validate-workflow.test.ts` runs the step itself +against fixture orgs. + +### Status + +**RESOLVED 2026-10-03.** + +--- + ## Out of scope (intentionally not improvements) - **State file is identity-only and not git-ignored.** It's intentionally diff --git a/tests/ci-validate-workflow.test.ts b/tests/ci-validate-workflow.test.ts new file mode 100644 index 0000000..c4b0b93 --- /dev/null +++ b/tests/ci-validate-workflow.test.ts @@ -0,0 +1,155 @@ +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import { + cpSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; +import { fileURLToPath } from "node:url"; +import { parse as parseYaml } from "yaml"; + +// ci.yml's "Validate resources" job runs `validate` on every org before +// merge, with no secrets. These tests run the job's real step (read from +// ci.yml, run with bash) against a copy of the engine and fixture orgs. + +const REPO = fileURLToPath(new URL("..", import.meta.url)); +const STARTER = join(REPO, "examples", "starter", "resources", "starter"); +const WORKFLOW_TEXT = readFileSync( + join(REPO, ".github/workflows/ci.yml"), + "utf8", +); + +interface Step { + name?: string; + run?: string; + env?: Record; + uses?: string; + with?: Record; +} + +const JOB = ( + parseYaml(WORKFLOW_TEXT) as { jobs: { validate: { steps: Step[] } } } +).jobs.validate; +const STEP = JOB.steps.find((s) => s.name === "Validate every org")!; + +// Run the step in a scratch repository holding the given org folders. +function validateStepRun(orgs: Record void>): { + code: number | null; + output: string; +} { + const root = mkdtempSync(join(tmpdir(), "vapi-ci-validate-")); + try { + cpSync(join(REPO, "src"), join(root, "src"), { recursive: true }); + cpSync(join(REPO, "package.json"), join(root, "package.json")); + symlinkSync(join(REPO, "node_modules"), join(root, "node_modules"), "dir"); + mkdirSync(join(root, "resources")); + // A file at the top of resources/ is not an org. + writeFileSync(join(root, "resources", ".vapi-ignore.example"), ""); + for (const [org, fill] of Object.entries(orgs)) { + const dir = join(root, "resources", org); + mkdirSync(dir); + fill(dir); + } + // Only what the runner would have: no inherited Vapi keys. + const result = spawnSync("bash", ["-c", STEP.run!], { + cwd: root, + encoding: "utf8", + timeout: 60_000, + env: { PATH: process.env.PATH, HOME: process.env.HOME, ...STEP.env }, + }); + return { code: result.status, output: `${result.stdout}${result.stderr}` }; + } finally { + rmSync(root, { recursive: true, force: true }); + } +} + +const starterCopy = (dir: string) => cpSync(STARTER, dir, { recursive: true }); + +const longNameAdd = (dir: string) => { + starterCopy(dir); + writeFileSync( + join(dir, "assistants", "front-desk-overflow.yml"), + "name: Front Desk Overflow Assistant For Weekend Calls\n", + ); +}; + +test("validate step passes when there are no org folders", () => { + const run = validateStepRun({}); + assert.deepEqual( + [run.code, run.output.includes("nothing to validate")], + [0, true], + run.output, + ); +}); + +test("validate step passes when every org is valid", () => { + const run = validateStepRun({ + clinic: starterCopy, + "clinic-dev": starterCopy, + }); + assert.deepEqual( + [ + run.code, + /Validated 2 org\(s\): (clinic clinic-dev|clinic-dev clinic)\n/.test( + run.output, + ), + ], + [0, true], + run.output, + ); +}); + +test("validate step fails naming only the invalid org, after checking all of them", () => { + const run = validateStepRun({ + clinic: longNameAdd, + "clinic-dev": starterCopy, + }); + assert.deepEqual( + { + code: run.code, + bothValidated: [ + "::group::Validate clinic\n", + "::group::Validate clinic-dev\n", + ].every((group) => run.output.includes(group)), + reason: run.output.includes("Vapi caps at 40"), + error: run.output.includes("::error::Validation failed for: clinic."), + }, + { code: 1, bothValidated: true, reason: true, error: true }, + run.output, + ); +}); + +test("validate step fails on an org folder that isn't a valid org name", () => { + const run = validateStepRun({ Clinic_Prod: starterCopy }); + assert.deepEqual( + [ + run.code, + run.output.includes("::error::Validation failed for: Clinic_Prod."), + ], + [1, true], + run.output, + ); +}); + +test("validate job gets no secrets and keeps no credentials", () => { + assert.deepEqual( + { + secrets: WORKFLOW_TEXT.includes("secrets."), + key: STEP.env, + checkout: JOB.steps.find((s) => s.uses?.startsWith("actions/checkout")) + ?.with, + }, + { + secrets: false, + key: { VAPI_PRIVATE_API_KEY: "validate-only-never-sent" }, + checkout: { "persist-credentials": false }, + }, + ); +});