Skip to content

feat(validate): catch broken references and show findings on the PR - #77

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

scott-lowe-vapi merged 1 commit into
mainfrom
fix/validate-references

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 — 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 docs: document orphan-YAML gate + --allow-new-files in README and AGENTS #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 ci: validate every org's resources on every pull request #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:

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

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

🤖 Generated with Claude Code

Comment thread tests/validate-refs.test.ts

@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.

Aggressive review pass. Each finding was checked against the code, and most were reproduced by running this branch's validators on the input described in the comment.

Severity: 🔴 blocker · 🟠 fix before merge · 🟡 should fix (a stacked PR is fine where noted) · 🟢 nit

  • 🟠 ×3: validate checks .vapi-ignored files that push skips, which blocks CI and apply. The override-tool-by-name fix advice (model.tools) replaces the member's tool set (message and docs row).
  • 🟡 ×6:
    • Judge references to ignored structured outputs pass both rules.
    • A non-string toolIds entry crashes validate.
    • Override, toolRefs and inline-assistant references aren't checked.
    • reference-by-uuid fires on every PR for dashboard-owned resources.
    • Annotations have no line.
    • The troubleshooting row tells readers to un-ignore resources.
  • 🟢 ×3: prototype-key lookup, duplicated credential walk, #31 status wording.

No 🔴. Tests and tsc pass on the branch as-is.

Comment thread src/validate-cmd.ts Outdated
Comment thread src/validate-refs.ts Outdated
Comment thread docs/guides/troubleshooting.md Outdated
Comment thread src/validate-refs.ts
Comment thread src/validate-refs.ts Outdated
Comment thread src/validate-cmd.ts
Comment thread src/validate-refs.ts Outdated
Comment thread src/validate-refs.ts Outdated
Comment thread docs/guides/troubleshooting.md Outdated
Comment thread improvements.md Outdated

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:09 PM UTC: Graphite rebased this pull request as part of a merge.
  • Oct 7, 6:09 PM UTC: @scott-lowe-vapi merged this pull request with Graphite.

@scott-lowe-vapi
scott-lowe-vapi changed the base branch from ci/validate-resources to graphite-base/77 October 7, 2026 18:06
scott-lowe-vapi added a commit that referenced this pull request Oct 7, 2026
## 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.

- **Problem:** nothing ran `npm run validate` before merge. `apply`, and so promotion, refuses to deploy a config with validation errors: a name over 40 characters, or a per-provider voice schema error. (The validator's other rules, structured-output lockstep, duplicated prompts and the `maxTokens` floor, are warnings and don't fail it.) That refusal came **after** merge: `main` held a config that wouldn't deploy, and promotion stopped until someone opened a fix PR. Plain `push` only warns, then can fail partway with an API 400. Each rule in the validator comes from a real mid-push failure (`improvements.md` #8, #9, #11, #18, #19).
- **Who it affects:** every repository on this template, and especially multi-org repos that promote from `main`.
- **What changes:**
  - **A new Validate resources job in `ci.yml`** runs `validate` for every folder under `resources/` on every pull request. It reports every failing org rather than stopping at the first.
  - **No engine change.** `validate` makes no network call; loading the engine's config only requires 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, so it runs the same on forks and Dependabot PRs.
  - **It's in `ci.yml`, not the PR check workflow,** so every fork gets it without turning on PR checks. On this template, which has no org folders, it does nothing.
  - **Docs:**
    - the README quick start;
    - the `AGENTS.md` change loop: if the check fails, fix the errors and don't weaken the check;
    - the workflows guide: make it a required check;
    - the PR checks and troubleshooting guides;
    - `improvements.md` #37.

**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

- `npm test` (514 tests) and `npx tsc --noEmit` pass.
- actionlint, from #75, lints the new job in this PR's CI.
- **Not tested:** a customer repo with real resources. The validator itself is unchanged, and `apply` already runs it on every deploy.

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 #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](https://claude.com/claude-code)
@scott-lowe-vapi
scott-lowe-vapi changed the base branch from graphite-base/77 to main October 7, 2026 18:07
A reference that names no file and no state entry failed three different
ways depending on the field: silently dropped (toolIds,
structuredOutputIds), sent raw and rejected mid-push (squad members, hook
tools, personalityId, scenarioId), or deferred (improvements.md #31).
validate never checked it, so the new CI check couldn't either.

- src/validate-refs.ts, run by validate (so by apply and CI) and by push:
  - dangling-reference (error): a name with no local file and no state
    entry;
  - malformed-reference (error): an empty or non-name list entry, which
    used to crash validate with no file named;
  - override-tool-by-name (error): toolIds names inside
    assistantOverrides, membersOverrides or targetOverrides, which push
    never resolves; the fix points to tools:append, since model.tools
    there replaces the member's tool set;
  - unresolved-credential (warning): a credential name not in state,
    naming the org's bootstrap pull;
  - reference-by-uuid (warning): only for a UUID this repo tracks, naming
    the file to use; dashboard-owned UUIDs aren't reported.
- One collector (referencesCollect in resolver.ts) feeds both these rules
  and reference-to-ignored, so judge references to ignored structured
  outputs are caught; ID cleaning tolerates non-string entries.
- validate skips .vapi-ignore'd files, as push does, and now also runs
  reference-to-ignored. It reads the committed state file offline.
- On GitHub Actions, validate prints each finding as an annotation, so it
  shows on the file in the PR, warnings included.
- The validate header no longer prints an API URL for an offline command.
- Docs: a rule table in troubleshooting (safe advice for ignored and
  override references), the commands row, AGENTS.md (never edit state or
  .vapi-ignore to make a reference resolve), improvements.md #31 marked
  mitigated with its remaining gaps, and #38 for promotion and stock
  personalities.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scott-lowe-vapi
scott-lowe-vapi force-pushed the fix/validate-references branch from 908d125 to 49d0272 Compare October 7, 2026 18:08
@scott-lowe-vapi
scott-lowe-vapi merged commit b0f37f5 into main Oct 7, 2026
6 checks passed
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