Skip to content

test: restore the suite after the hash-store migration and run it in CI - #56

Merged
scott-lowe-vapi merged 1 commit into
mainfrom
fix/stale-tests-and-ci
Oct 1, 2026
Merged

test: restore the suite after the hash-store migration and run it in CI#56
scott-lowe-vapi merged 1 commit into
mainfrom
fix/stale-tests-and-ci

Conversation

@scott-lowe-vapi

@scott-lowe-vapi scott-lowe-vapi commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Problem

npm test isn't wired into any workflow (.github/workflows/ only held promotion.yml), so nothing blocks a merge that breaks tests. Since the hash-store migration in #41, 20 of 357 tests have failed on main, unnoticed.

Bisecting first-parent main: 0 failures at #42, 20 failures from #41's merge onward, and no new failures since then (#55 included).

Diagnosis: stale tests, not engine bugs

#41 moved drift baselines out of the state file into .vapi-state-hash/<org>/<uuid>, made upsertState / asResourceState strip legacy fields, and made pull/push/apply refuse legacy-shaped state. The tests were never moved over.

Tests Failures Cause
audit.test.ts 4 Fixtures put lastPulledHash in state; audit.ts correctly reads the hash store now
state-migration.test.ts, 2 in drift.test.ts 4 Asserted legacy fields survive state writes, which is exactly what the migration removed
reconcile-state-key.test.ts 4 Asserted lastPushedHash lands in state; the shared push path records the baseline in the hash store instead
drift.test.ts (checkDriftForUpdate) 3 Missing the new required env argument, so the hash-store path was undefined
drift.test.ts (converged edge) 1 Pinned both-diverged for local == platform ≠ baseline, which classifyDrift now deliberately returns as clean (the documented invariant behind the phantom-drift fix, improvements.md #23)
Pull/push spawn tests 4 Legacy-format state fixtures, refused by the migration gate

The last row matters most. Three of those were the regression guards #41 added for #22 (rename keeps the local filename; same-name clobber) and #23 (a stale baseline must not block a push). They have never passed, so those fixes had no working coverage.

Changes

  • CI: .github/workflows/ci.yml runs npm run build + npm test on every PR and on pushes to main, on Node 20 and 22 (the engines range). It sets a job timeout and read-only permissions.
  • Fixtures moved to the hash store:
    • The spawn tests copy src/ into a temp dir, so the engine's store resolves there; they seed <tmp>/.vapi-state-hash/<env>/<uuid>.
    • drift.test.ts seeds through writeBaseline under a throwaway drift-test-<pid> org and removes it afterwards.
    • audit.ts gains an optional baselineReader DI seam next to stateLoader / listLocalIds. That is the only production-code change, and the default is the existing readBaseline(VAPI_ENV, uuid).
  • push-stale-baseline-noop now tests what it says. With empty credentials, maybeBootstrapState treats state as uninitialized and its bootstrap pull rewrote the stale baseline before the drift check ever ran. Migrating the fixture alone would have produced a green test that exercises nothing. It now seeds a dummy credential and asserts no bootstrap ran.
  • Converged-edge test rewritten to pin clean, with the rationale from drift.ts.
  • Section J (classifier short-circuit) rewritten around the hash store. The original bug (a rebuilt state section dropped the baseline) can't happen by construction now. The new test pins that separation, so moving the baseline back into the state entry fails it.
  • tool-assistant-cycle.test.ts deleted nothing it wrote: updateToolAssistantRefs records a baseline into the developer's real .vapi-state-hash/test-fixture-org/ on every run. It now removes it.
  • improvements.md fix(push,pull): recanonicalize stale UUID-suffixed state keys — root-cause duplicate generation #32.

Evidence

  • npm test: 355 / 355 pass (was 337 / 357; two fewer tests because Section J's four tests became one and one state-migration test was split in two). npm run build is clean.
  • Mutation check: removing the agree-gate (localHash === platformHash in both classifyDrift and checkDriftForUpdate) fails push-stale-baseline-noop and the converged-edge test. Restoring it passes both.
  • Isolation check: after a full run, the real .vapi-state-hash/ holds no test-written baseline. The drift-test-<pid> folder is removed, and the tool-assistant-cycle baseline file is deleted, leaving only an empty gitignored test-fixture-org/ folder.

Not in this PR

tsconfig.json still includes only src/**/*, so the typecheck never sees tests/. That's why stale fixture shapes compiled silently. Including tests/ surfaces 37 existing type errors; that's its own change.

🤖 Generated with Claude Code

@scott-lowe-vapi @claude
test: restore the suite after the hash-store migration and run it in CI
87f55f5 
npm test was not wired into any workflow, so 20 tests failed from the
hash-store migration (#41) onward without anyone noticing. Every failure was
a stale test, not an engine bug: fixtures still put lastPulledHash /
lastPushedHash into state, checkDriftForUpdate calls lacked the new env
argument, and one test pinned both-diverged for the converged edge that
classifyDrift now deliberately treats as clean.

- add .github/workflows/ci.yml: typecheck + npm test on every PR and on
  pushes to main, on Node 20 and 22
- move test baselines into the hash store: spawn-based tests seed
  <tmp>/.vapi-state-hash, drift tests seed via writeBaseline under a
  throwaway org slug, and audit gains a baselineReader DI seam
- push-stale-baseline-noop seeds a credential so push no longer runs a
  bootstrap pull that overwrote the stale baseline before the drift check,
  and asserts it; removing the agree-gate now fails this test
- rewrite the classifier short-circuit regression around the hash store
- tool-assistant-cycle deletes the baseline it wrote into the real store
- improvements.md #32

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scott-lowe-vapi
scott-lowe-vapi merged commit 69c7e83 into main Oct 1, 2026
2 checks passed
@scott-lowe-vapi scott-lowe-vapi mentioned this pull request Oct 1, 2026
Merged
scott-lowe-vapi added a commit that referenced this pull request Oct 3, 2026
@scott-lowe-vapi
ci: type-check tests/ and send a User-Agent on sim API requests (#57)
d7b9a34 
## Value

**V.A.L.U.E. tier:** project — PR 1 of 10 for inline simulation PR checks ([TEST-141](https://linear.app/vapi/issue/TEST-141/gitops-run-simulation-suites-against-pr-changes-inline-as-ci-checks)); this PR is a small, behavior-preserving slice.

- **Problem:** `tests/` was never type-checked (`tsconfig.json` included only `src/`), so 37 type errors piled up silently. That's the gap #56 named as follow-up in `improvements.md` #32. Separately, simulation runs started from gitops can't be told apart in the platform's analytics.
- **Who it affects:** gitops maintainers and contributors, who get a compiler check on test fixtures; and the TEST-141 success measure, which needs a pre-release baseline of gitops-started runs.
- **What changes:**
  - `npm run build`, which CI already runs on every PR, now type-checks `tests/`.
  - The 37 errors are fixed.
  - `npm run sim` sends `User-Agent: vapi-gitops-sim/<version>`. The API records this as `user_agent` on the `[simulation] run started` event.

The CI workflow itself landed in #56, so this PR is smaller than PR 1 in the plan.

## Evidence of value

| Check | `main` (69c7e83) | This branch |
|---|---|---|
| `tsc --noEmit` with `tests/` included | **37 errors** (6 test files) | **0 errors** |
| `npm test` | 355 pass | 357 pass (2 new, 1 rewritten) |
| `User-Agent` on `POST /eval/simulation/run` | none | `vapi-gitops-sim/1.0.0` (asserted against a local HTTP server) |

What the 37 errors were:
- Fixture drift after the hash-store migration: state entries still carrying `lastPulledHash`/`lastPushedHash`, bare-string state values, and an untyped `emptyLoaded()`.
- One real gap: the `reconcile-state-key` harness never passed the required `formatError`, so any test reaching that error path would have thrown a `TypeError` instead of testing it.

Tests that used the removed hash fields as markers (`state-merge`, `recanonicalize`) now mark "which copy won" with distinct UUIDs or object identity, so they still check the same behavior.

## Testing plan

- `npm run build` (now covers `src/` and `tests/`) and `npm test` locally on Node 22: green. CI on this PR runs both on Node 20 and 22.
- New `tests/user-agent.test.ts` covers the header format against `package.json`'s version, and the header actually sent on run create.
- `sim.test.ts` now covers the legacy bare-string state value directly, replacing the old cast-based "forward-compat" test.
- **Not tested:** a live run against the API (the header is asserted locally only), and Node 20 locally (left to CI). `src/` behavior is unchanged apart from the added header.

Refs TEST-141

🤖 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

Reviewers

@roshan-vapi roshan-vapi roshan-vapi approved these changes
@dhruva-vapi dhruva-vapi Awaiting requested review from dhruva-vapi
@steven-diaz-vapi steven-diaz-vapi Awaiting requested review from steven-diaz-vapi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@scott-lowe-vapi @roshan-vapi