From f2c1553702fbc642cc0a29d518a8cec85a9cd384 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 (a name over 40 characters, a per-provider voice schema error) 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. The validator's other rules (structured-output lockstep, duplicated prompts, the maxTokens floor) are warnings and don't fail it. - 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 key and an unroutable base URL, so nothing can be sent. The job has no secrets and installs without scripts, so forks get it too. No engine change. - The failure message, troubleshooting guide and AGENTS.md give a placeholder-key command to reproduce it, so nobody fetches a real key for an offline check. - The docs say what requiring it costs (every org gates every PR), the merge-queue trigger it needs, and that CI validates .ts resources without .env.. - The starter example's one-sided structured-output link is fixed, so it validates without warnings. - tests/ci-validate-workflow.test.ts runs the step itself against fixture orgs: no orgs, all valid (and warning-free), one invalid org among valid ones, an invalid folder name, and no secrets, permissions, privileged trigger 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 | 58 ++++++ AGENTS.md | 7 +- README.md | 4 +- docs/guides/file-formats.md | 6 +- docs/guides/pr-checks.md | 3 + docs/guides/troubleshooting.md | 27 +++ docs/guides/workflows.md | 15 ++ .../structuredOutputs/call-summary.yml | 2 + improvements.md | 53 ++++++ tests/ci-validate-workflow.test.ts | 177 ++++++++++++++++++ 10 files changed, 349 insertions(+), 3 deletions(-) create mode 100644 tests/ci-validate-workflow.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 231df69..6834ccf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -64,3 +64,61 @@ 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 + + # Install scripts only build the optional audio deps for `npm run call`. + - run: npm ci --ignore-scripts + + # The same validator `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 + # (the base URL is unroutable, so an accidental API call fails here), + # and no secret is available to this job, so it runs the same on forks. + # `.ts` resources run without `.env.` here, unlike under `apply`. + - name: Validate every org + shell: bash + env: + VAPI_PRIVATE_API_KEY: validate-only-never-sent + VAPI_BASE_URL: http://127.0.0.1:9 + 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[*]}. Each org's findings are in its log group above. To reproduce locally without that org's key: VAPI_PRIVATE_API_KEY=validate-only npm run validate -- " + exit 1 + fi + echo "Validated ${#orgs[@]} org(s): ${orgs[*]}" diff --git a/AGENTS.md b/AGENTS.md index aa5e47f..76cee26 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -130,7 +130,12 @@ 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. Without that org's + `.env.`, run `VAPI_PRIVATE_API_KEY=validate-only npm run validate -- `; + never ask for a real key just to validate. 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 b1e80ed..d5fbfae 100644 --- a/README.md +++ b/README.md @@ -144,7 +144,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 validator 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/file-formats.md b/docs/guides/file-formats.md index 4ac0377..c8500e9 100644 --- a/docs/guides/file-formats.md +++ b/docs/guides/file-formats.md @@ -89,6 +89,8 @@ destinations: # examples/starter/resources/starter/structuredOutputs/call-summary.yml name: call-summary type: ai +assistant_ids: + - receptionist description: Summarizes the call for the front-desk log. schema: type: object @@ -180,4 +182,6 @@ simulationIds: Any resource can also be a `.ts` file whose default export is the resource object, useful for generating config. It is executed when loaded, so treat -`.ts` resources like code in review. +`.ts` resources like code in review. CI validates `.ts` resources without your +`.env.`, so build them from files in the repository, not from +`process.env`. 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..680b635 100644 --- a/docs/guides/troubleshooting.md +++ b/docs/guides/troubleshooting.md @@ -81,3 +81,30 @@ 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, and that org's log group lists every +finding. To reproduce locally: + +```bash +VAPI_PRIVATE_API_KEY=validate-only npm run validate -- +``` + +`validate` never calls the API, but the engine won't start without a key, so +a placeholder is enough. You don't need that org's real key or its +`.env.`. + +Each error names the resource (`assistants/`), 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 `Failed to import TypeScript resource … is not set` error from this check +means a `.ts` resource reads a variable from `.env.`, which CI doesn't +have. Build `.ts` resources from files in the repository instead. + +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..6e56d2e 100644 --- a/docs/guides/workflows.md +++ b/docs/guides/workflows.md @@ -17,6 +17,21 @@ npm run validate -- npm run apply -- ``` +CI runs the same validator 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. + +Two things to know before you require it: + +- It validates every org on every pull request, so one org with errors + blocks every pull request in the repository, including those from teams + that never touch that org. +- If you merge through a GitHub merge queue, first add `merge_group:` to + `ci.yml`'s `on:` triggers, or the required check never reports and the + queue stalls. + To deploy only some resources, pass resource types or file paths. `apply` and `push` accept the same scoping: diff --git a/examples/starter/resources/starter/structuredOutputs/call-summary.yml b/examples/starter/resources/starter/structuredOutputs/call-summary.yml index 702df63..5fd3d3e 100644 --- a/examples/starter/resources/starter/structuredOutputs/call-summary.yml +++ b/examples/starter/resources/starter/structuredOutputs/call-summary.yml @@ -1,5 +1,7 @@ name: call-summary type: ai +assistant_ids: + - receptionist description: Summarizes the call for the front-desk log. schema: type: object diff --git a/improvements.md b/improvements.md index 7bfad7d..4ea994b 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 (#76) | **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] (#76)** + +**Discovered:** 2026-10-03, while reviewing which static checks run before +the PR check's simulations. + +### Problem + +`npm run validate` fails on shapes the API rejects mid-push (a name over 40 +characters, per-provider voice schema) and warns on silent inconsistencies +(structured-output lockstep, duplicated prompts, the `maxTokens` floor), 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..082e082 --- /dev/null +++ b/tests/ci-validate-workflow.test.ts @@ -0,0 +1,177 @@ +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 FOUND = JOB.steps.find((s) => s.name === "Validate every org"); +assert.ok( + FOUND, + 'ci.yml has no "Validate every org" step; update this test if it was renamed', +); +const STEP: Step = FOUND; + +// 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, + ), + // The starter is the file new users copy: no warnings either. + run.output.split("No validation issues.").length - 1, + ], + [0, true, 2], + 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", () => { + const workflow = parseYaml(WORKFLOW_TEXT) as { + on: Record; + jobs: { validate: Record }; + }; + assert.deepEqual( + { + // Fork code runs in this job, so it must never get the privileged trigger. + privilegedTrigger: "pull_request_target" in workflow.on, + // Scoped to this job, and catches secrets.X, secrets['X'], + // toJSON(secrets) and `secrets: inherit`. + secrets: /\bsecrets\b/.test(JSON.stringify(workflow.jobs.validate)), + permissions: workflow.jobs.validate.permissions, + env: STEP.env, + checkout: JOB.steps.find((s) => s.uses?.startsWith("actions/checkout")) + ?.with, + }, + { + privilegedTrigger: false, + secrets: false, + permissions: undefined, + // A placeholder key, and an unroutable host so nothing can be sent. + env: { + VAPI_PRIVATE_API_KEY: "validate-only-never-sent", + VAPI_BASE_URL: "http://127.0.0.1:9", + }, + checkout: { "persist-credentials": false }, + }, + ); +});