Skip to content

ci: validate every org's resources on every pull request - #76

Merged
scott-lowe-vapi merged 1 commit into
mainfrom
ci/validate-resources
Oct 7, 2026
Merged

scott-lowe-vapi merged 1 commit into
mainfrom
ci/validate-resources

Conversation

@scott-lowe-vapi

@scott-lowe-vapi scott-lowe-vapi commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Value

V.A.L.U.E. tier: small — touches .github/workflows/ (a blast-radius path), and changes what customer forks see on their pull requests.

Heads-up for forks: plain push only warned about these errors, so a repo may already carry some. The first PR after this lands will show them, whatever it changes. The troubleshooting guide covers it. apply already refused those configs, so this moves an existing failure earlier rather than adding a new one.

Evidence of value

tests/ci-validate-workflow.test.ts runs the job's real step, read from ci.yml, against copies of the starter example:

Case Result
No org folders passes, "nothing to validate"
Two valid orgs passes, both validated
One org with a 41+ character assistant name, one valid fails; both validated, the error names only the bad org and the reason ("Vapi caps at 40")
A folder that isn't a valid org name fails, naming it
The job's secrets none; checkout doesn't persist credentials; the only key is the placeholder

Mutation: making the loop ignore validate's exit code fails the two failure-case tests.

Testing plan

Refs TEST-141

After review

  • Install: npm ci --ignore-scripts, which skips the native audio builds validate never loads.
  • Reproducing locally: the failure message, the troubleshooting guide and AGENTS.md give a placeholder-key command, so nobody fetches a production key for an offline check.
  • Docs:
    • workflows.md says that requiring the check ties every team's PRs to every org's health, and that merge queues need a merge_group: trigger;
    • CI validates .ts resources without .env.<org>;
    • "the same validator apply runs" instead of "the same checks".
  • Starter example: its one-sided structured-output link is fixed, and the test asserts it validates with no warnings.
  • Tests: the no-secrets test also checks the job's permissions and that pull_request_target isn't a trigger.
  • improvements.md Add GitOps Support for Pronunciation Dictionaries with Versioned Updates #37: reworded (errors vs. warnings) and carries the PR number.

Annotations for warnings come from #77, so I didn't add the sed version here.

🤖 Generated with Claude Code

@chris-garber-vapi chris-garber-vapi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think running validate in CI makes sense

@chris-garber-vapi chris-garber-vapi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the Validate resources job. Nothing here blocks merge (no 🔴 or 🟠). The main themes: the docs overstate what fails (3 of the 5 named rules are warnings, and they stay hidden), the local repro path needs a real org key, env-dependent .ts resources validate differently in CI than under apply, and every org being validated on every PR widens the blast radius. Suggestion blocks were run locally against the PR head unless noted.

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
Comment thread docs/guides/workflows.md
Comment thread improvements.md Outdated
Comment thread improvements.md Outdated
Comment thread tests/ci-validate-workflow.test.ts Outdated
Comment thread tests/ci-validate-workflow.test.ts

scott-lowe-vapi commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor Author

Merge activity

  • Oct 7, 5:47 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Oct 7, 6:07 PM UTC: Graphite rebased this pull request as part of a merge.
  • Oct 7, 6:07 PM UTC: @scott-lowe-vapi merged this pull request with Graphite.

@scott-lowe-vapi
scott-lowe-vapi changed the base branch from ci/workflow-hardening to graphite-base/76 October 7, 2026 18:04
@scott-lowe-vapi
scott-lowe-vapi changed the base branch from graphite-base/76 to main October 7, 2026 18:05
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.<org>.
- 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 <noreply@anthropic.com>
@scott-lowe-vapi
scott-lowe-vapi merged commit 4979ccd into main Oct 7, 2026
6 checks passed
scott-lowe-vapi added a commit that referenced this pull request Oct 7, 2026
)

## Value

**V.A.L.U.E. tier:** small — a behavior change: `validate`, and so `apply` and the Validate resources check, now fail on configs they used to pass.

- **Problem:** a reference that names no file fails in one of three ways, depending on the field (`improvements.md` #31):
  - it's **silently dropped**: `model.toolIds`, `artifactPlan.structuredOutputIds`;
  - it's **sent raw and rejected partway through a push**: squad members, hook tools, `personalityId`, `scenarioId`;
  - or it's **deferred** to a linking pass.

  `validate` never checked references, so the Validate resources check from #76 couldn't catch a typo'd tool name either. Warnings were also invisible in CI: they don't fail the check, and nobody reads the job log.
- **Who it affects:** everyone who edits resource files by hand or with a coding agent, and reviewers of their PRs.
- **What changes:**
  - **New `src/validate-refs.ts`**, run by `validate` (so by `apply` and CI) and by `push`:

    | Rule | Severity | Catches |
    | --- | --- | --- |
    | `dangling-reference` | error | A name with no local file and no state entry. Uses the shared reference walk, plus scenario judges' `evaluations[].structuredOutputId`. |
    | `malformed-reference` | error | An empty or non-name entry in a reference list (it used to crash `validate`). |
    | `override-tool-by-name` | error | A tool name in `toolIds` inside `assistantOverrides`, `membersOverrides` or `targetOverrides`, where push never resolves names. The fix it gives is `tools:append`, because `model.tools` there replaces the member's tool set. |
    | `unresolved-credential` | warning | A credential name missing from the state file. The message names the org's bootstrap pull. |
    | `reference-by-uuid` | warning | A UUID for a resource this repo tracks, which breaks promotion; it names the file to use. UUIDs the repo doesn't track (dashboard-owned, stock personalities) aren't reported. |
  - **`validate` now also runs `reference-to-ignored`,** as `push` already did. It reads the committed state file and stays offline.
  - **On GitHub Actions, every finding becomes an annotation,** so it shows on the file in the PR, warnings included.
  - **`push`** reports the new rules alongside its existing validators: warnings by default, blocking under `--strict`.
  - **Docs:**
    - a rule-by-rule table in troubleshooting;
    - the `validate` row in the commands guide;
    - `AGENTS.md`: never edit the state file to make a reference resolve;
    - `improvements.md` #31 marked resolved by validation.

## Evidence of value

The starter example with two typos, `scheduler` → `schedular` in the squad and `booking-confirmed` → `booking-confirmd` in a judge:

| | Before (#76) | After |
| --- | --- | --- |
| `npm run validate` | `0 error(s)` — ✅ Validation passed | `2 error(s)`, one `dangling-reference` per typo, naming the file and the missing name |

- **New `tests/validate-refs.test.ts`** covers:
  - names that resolve to local files and to state;
  - a typo in each reference field;
  - ignored references;
  - UUIDs and stock personalities;
  - all three override keys;
  - credentials as names, as UUIDs, and known to state.
- **Annotation format:** escaping of `%`, newlines, `:` and `,` is tested in `tests/validate.test.ts`.
- **End to end:** `tests/ci-validate-workflow.test.ts` runs the CI step with `GITHUB_ACTIONS=true` on the typo'd squad. The step fails, and the `::error` points at `resources/clinic/squads/front-desk.yml`.
- **Mutation:** dropping the judge collection or the override walk fails two tests.
- **Every example org still passes.** The cross-org promotion example (which has no state files) now shows an `unresolved-credential` warning, which is accurate.

## Testing plan

- `npm test` (523 tests) and `npx tsc --noEmit` pass.
- **Behavior to expect:** `apply` validates before it pulls. So a reference to a resource created in the dashboard and never pulled now stops `apply`; the message says to pull first. Before, `apply` went on to pull and push, and the reference resolved only if the pull happened to produce that exact name.
- **Not tested:** a live `apply` or `push`. Neither code path changed except for the added findings.
- **Not in this PR:** a check for secrets committed in resource files. It goes in its own PR, because a false positive there would block a customer's merge.

Refs TEST-141

## After review

- **Ignored files:** `validate` skips `.vapi-ignore`d files, as push does, so ignored files can't fail CI or `apply`.
- **One collector:** `referencesCollect` in `resolver.ts` feeds both `reference-to-ignored` and these rules, so a judge that references an ignored structured output is caught.
- **Lookups:** use `?.uuid`, so `constructor` and friends don't resolve.
- **Troubleshooting:** the table gives safe advice for ignored and override references, and AGENTS.md says never to edit the state file or `.vapi-ignore` to make a reference resolve.
- **improvements.md:** #31 is marked mitigated, with the remaining gaps listed (unchecked override paths, `toolRefs`, inline members, annotation lines). #38 records the promotion and stock-personality limitation found in #66.

Follow-ups: line numbers on annotations, the wider override walk, and deduplicating the credential walkers.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants